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

[Harsha Manjula Mallikarjun (RBEI/ECF3)]

> -----Original Message-----
> From: Daniel Stone [mailto:daniel at fooishbar.org]
> Sent: Saturday, July 21, 2018 5:48 PM
> To: Harsha Manjula Mallikarjun (RBEI/ECF3)
> <Harsha.ManjulaMallikarjun at in.bosch.com>
> Cc: wayland <wayland-devel at 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 at 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