[v3,weston,3/5] compositor-drm: add support for CTM property

Submitted by Harsha Manjula Mallikarjun (RBEI/ECF3) on June 28, 2018, 1:27 p.m.

Details

Message ID 1530192480-23334-4-git-send-email-harsha.manjulamallikarjun@in.bosch.com
State Changes Requested
Headers show
Series "Implement support for drm properties "GAMMA_LUT" and "CTM"" ( rev: 3 ) in Wayland

Commit Message

Harsha Manjula Mallikarjun (RBEI/ECF3) June 28, 2018, 1:27 p.m.
From: Harsha M M <harsha.manjulamallikarjun@in.bosch.com>

Implement support for setting color transformation matrix
using drm CTM property.

Signed-off-by: Harsha M M <harsha.manjulamallikarjun@in.bosch.com>
---
 libweston/compositor-drm.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 66 insertions(+)

Patch hide | download patch | download mbox

diff --git a/libweston/compositor-drm.c b/libweston/compositor-drm.c
index 27c95da..7aa30a7 100644
--- a/libweston/compositor-drm.c
+++ b/libweston/compositor-drm.c
@@ -198,6 +198,7 @@  enum wdrm_crtc_property {
 	WDRM_CRTC_ACTIVE,
 	WDRM_CRTC_GAMMA_LUT,
 	WDRM_CRTC_GAMMA_LUT_SIZE,
+	WDRM_CRTC_CTM,
 	WDRM_CRTC__COUNT
 };
 
@@ -206,6 +207,7 @@  static const struct drm_property_info crtc_props[] = {
 	[WDRM_CRTC_ACTIVE] = { .name = "ACTIVE", },
 	[WDRM_CRTC_GAMMA_LUT] = { .name = "GAMMA_LUT", },
 	[WDRM_CRTC_GAMMA_LUT_SIZE] = { .name = "GAMMA_LUT_SIZE", },
+	[WDRM_CRTC_CTM] = { .name = "CTM", },
 };
 
 /**
@@ -1748,6 +1750,67 @@  drm_output_render(struct drm_output_state *state, pixman_region32_t *damage)
 				 &c->primary_plane.damage, damage);
 }
 
+static int64_t
+drm_convert_to_ctm_format(float coefficient)
+{
+	int64_t ret;
+	int64_t max_coefficient = (int64_t)(1ULL << 31);
+
+	/* convert float to S31.32 format as needed by drm_color_ctm
+	 *  matrix co-effecients*/
+	if (coefficient <= (float)-max_coefficient)
+		ret = (int64_t)(~0ULL);
+	else if (coefficient >= (float)max_coefficient)
+		ret = (int64_t)(~(1ULL << 63));
+	else if (coefficient < 0) {
+		ret = (int64_t)((-coefficient * ((int64_t) 1L << 32)) + 0.5);
+		ret |= 1ULL << 63;
+	} else
+		ret = (int64_t)((coefficient * ((int64_t) 1L << 32)) + 0.5);
+
+	return ret;
+}
+
+static void
+drm_output_set_ctm(struct weston_output *output_base,
+		   struct weston_matrix *ctm_matrix)
+{
+	struct drm_output *output = to_drm_output(output_base);
+	struct drm_backend *backend = to_drm_backend(output->base.compositor);
+	struct drm_color_ctm ctm;
+	uint32_t ctm_blobid = 0;
+	uint32_t ctm_prop_id;
+	int row;
+	int col;
+	int rc;
+	float coefficient;
+
+	ctm_prop_id = output->props_crtc[WDRM_CRTC_CTM].prop_id;
+	if (!ctm_prop_id)
+		return;
+
+	for (row = 0; row < 3; row++) {
+		for (col = 0; col < 3; col++) {
+			coefficient = ctm_matrix->d[(row * 4) + col];
+			ctm.matrix[(row * 3) +  col] =
+					drm_convert_to_ctm_format(coefficient);
+		}
+	}
+
+	rc = drmModeCreatePropertyBlob(backend->drm.fd, &ctm, sizeof(ctm),
+				       &ctm_blobid);
+	if (!rc) {
+		rc = drmModeObjectSetProperty(backend->drm.fd, output->crtc_id,
+					      DRM_MODE_OBJECT_CRTC, ctm_prop_id,
+					      ctm_blobid);
+		if (rc)
+			weston_log("failed to set ctm for crtc %d: %m\n",
+				   output->crtc_id);
+
+		drmModeDestroyPropertyBlob(backend->drm.fd, ctm_blobid);
+	}
+}
+
 static void
 drm_output_set_gamma(struct weston_output *output_base,
 		     uint16_t size, uint16_t *r, uint16_t *g, uint16_t *b)
@@ -5166,6 +5229,9 @@  drm_output_enable(struct weston_output *base)
 	output->base.set_dpms = drm_set_dpms;
 	output->base.switch_mode = drm_output_switch_mode;
 	output->base.set_gamma = drm_output_set_gamma;
+
+	if (output->props_crtc[WDRM_CRTC_CTM].prop_id)
+		output->base.set_ctm = drm_output_set_ctm;
 
 	if (output->cursor_plane)
 		weston_compositor_stack_plane(b->compositor,

Comments

Hi Harsha,

On Thu, 28 Jun 2018 at 14:29, <harsha.manjulamallikarjun@in.bosch.com> wrote:
> +       struct drm_output *output = to_drm_output(output_base);
> +       struct drm_backend *backend = to_drm_backend(output->base.compositor);
> +       struct drm_color_ctm ctm;
> +       uint32_t ctm_blobid = 0;
> +       uint32_t ctm_prop_id;
> +       int row;
> +       int col;
> +       int rc;
> +       float coefficient;
> +
> +       ctm_prop_id = output->props_crtc[WDRM_CRTC_CTM].prop_id;
> +       if (!ctm_prop_id)
> +               return;
> +
> +       for (row = 0; row < 3; row++) {
> +               for (col = 0; col < 3; col++) {
> +                       coefficient = ctm_matrix->d[(row * 4) + col];
> +                       ctm.matrix[(row * 3) +  col] =
> +                                       drm_convert_to_ctm_format(coefficient);
> +               }
> +       }
> +
> +       rc = drmModeCreatePropertyBlob(backend->drm.fd, &ctm, sizeof(ctm),
> +                                      &ctm_blobid);
> +       if (!rc) {
> +               rc = drmModeObjectSetProperty(backend->drm.fd, output->crtc_id,
> +                                             DRM_MODE_OBJECT_CRTC, ctm_prop_id,
> +                                             ctm_blobid);
> +               if (rc)
> +                       weston_log("failed to set ctm for crtc %d: %m\n",
> +                                  output->crtc_id);
> +
> +               drmModeDestroyPropertyBlob(backend->drm.fd, ctm_blobid);
> +       }
> +}

Rather than having this directly applied in the function, I would like
to see this moved to where drm_output_state is applied to an output.
For atomic, this means that it's just another property set with all
the others, making the code a little shorter. Doing the same for gamma
as well would mean that the atomic path gets much shorter, and also
that gamma and CTM updates are synchronised with each other - plus the
pre-CTM degamma LUT if people want to use that, which would be a
pretty trivial addition to the code.

I'm not sure there's much point in supporting CTM on pre-atomic
drivers: I don't know of any drivers which support CTM but not atomic.

In order to do this, you would need to figure out when the gamma/CTM
have changed relative to the current output configuration, and only
apply it then. Maybe you could do this by checking the blob ID?

Cheers,
Daniel
> -----Original Message-----

> From: Daniel Stone [mailto:daniel@fooishbar.org]

> Sent: Saturday, July 21, 2018 5:48 PM

> To: Harsha Manjula Mallikarjun (RBEI/ECF3)

> <Harsha.ManjulaMallikarjun@in.bosch.com>

> Cc: wayland <wayland-devel@lists.freedesktop.org>

> Subject: Re: [PATCH v3 weston 3/5] compositor-drm: add support for CTM

> property

> 

> Hi Harsha,

> 

> On Thu, 28 Jun 2018 at 14:29, <harsha.manjulamallikarjun@in.bosch.com>

> wrote:

> > +       struct drm_output *output = to_drm_output(output_base);

> > +       struct drm_backend *backend = to_drm_backend(output-

> >base.compositor);

> > +       struct drm_color_ctm ctm;

> > +       uint32_t ctm_blobid = 0;

> > +       uint32_t ctm_prop_id;

> > +       int row;

> > +       int col;

> > +       int rc;

> > +       float coefficient;

> > +

> > +       ctm_prop_id = output->props_crtc[WDRM_CRTC_CTM].prop_id;

> > +       if (!ctm_prop_id)

> > +               return;

> > +

> > +       for (row = 0; row < 3; row++) {

> > +               for (col = 0; col < 3; col++) {

> > +                       coefficient = ctm_matrix->d[(row * 4) + col];

> > +                       ctm.matrix[(row * 3) +  col] =

> > +                                       drm_convert_to_ctm_format(coefficient);

> > +               }

> > +       }

> > +

> > +       rc = drmModeCreatePropertyBlob(backend->drm.fd, &ctm,

> sizeof(ctm),

> > +                                      &ctm_blobid);

> > +       if (!rc) {

> > +               rc = drmModeObjectSetProperty(backend->drm.fd, output-

> >crtc_id,

> > +                                             DRM_MODE_OBJECT_CRTC, ctm_prop_id,

> > +                                             ctm_blobid);

> > +               if (rc)

> > +                       weston_log("failed to set ctm for crtc %d: %m\n",

> > +                                  output->crtc_id);

> > +

> > +               drmModeDestroyPropertyBlob(backend->drm.fd, ctm_blobid);

> > +       }

> > +}

> 

> Rather than having this directly applied in the function, I would like

> to see this moved to where drm_output_state is applied to an output.

> For atomic, this means that it's just another property set with all

> the others, making the code a little shorter. Doing the same for gamma

> as well would mean that the atomic path gets much shorter, and also

> that gamma and CTM updates are synchronised with each other - plus the

> pre-CTM degamma LUT if people want to use that, which would be a

> pretty trivial addition to the code.

> 

> I'm not sure there's much point in supporting CTM on pre-atomic

> drivers: I don't know of any drivers which support CTM but not atomic.

> 

> In order to do this, you would need to figure out when the gamma/CTM

> have changed relative to the current output configuration, and only

> apply it then. Maybe you could do this by checking the blob ID?

 
Thanks for the feedback. Yes, we can do this. I will figure out and provide
the next version of patches, mostly next week.

> 

> Cheers,

> Daniel