[PATCH v3 weston 1/5] compositor-drm: Implement support for GAMMA_LUT drm property

[Daniel Stone]

Hi Harsha,
Some small comments here, but more substantial comments in another patch.

On Thu, 28 Jun 2018 at 14:28, <harsha.manjulamallikarjun at in.bosch.com> wrote:
> +       gamma_prop_id = output->props_crtc[WDRM_CRTC_GAMMA_LUT].prop_id;
> +       gamma_size_prop_id = output->props_crtc[WDRM_CRTC_GAMMA_LUT_SIZE].
> +                                                               prop_id;
> +
> +       if (gamma_prop_id && gamma_size_prop_id) {
> +               lut = malloc(sizeof(struct drm_color_lut) * size);
> +               if (!lut) {
> +                       weston_log("failed to allocate memory for gamma lut\n");
> +                       goto out;
> +               }
> +
> +               for (loop = 0; loop < size; loop++) {
> +                       lut[loop].red = r[loop];
> +                       lut[loop].green = g[loop];
> +                       lut[loop].blue = b[loop];
> +               }

Since this is the new interface, it would be nice to switch Weston's
internals to use the same structure internally; this would mean a copy
when using the old drmModeSetGamma interface, and no copies when using
the property interface.

> @@ -4758,15 +4809,35 @@ drm_output_init_gamma_size(struct drm_output *output)
> +       props_crtc = &output->props_crtc[0];

This should just be output->props_crtc, but per below, can be
completely eliminated.

> +       gamma_prop_id = props_crtc[WDRM_CRTC_GAMMA_LUT].prop_id;
> +       gamma_size_prop_id = props_crtc[WDRM_CRTC_GAMMA_LUT_SIZE].prop_id;
> +
> +       if (gamma_prop_id && gamma_size_prop_id) {
> +               props  = drmModeObjectGetProperties(backend->drm.fd,
> +                                                  output->crtc_id,
> +                                                  DRM_MODE_OBJECT_CRTC);
> +               if (props) {
> +
> +                       output->base.gamma_size = drm_property_get_value(
> +                                      &props_crtc[WDRM_CRTC_GAMMA_LUT_SIZE],
> +                                      props,
> +                                      WDRM_CRTC__COUNT);

The last argument for this function is a default value. You could
simply rewrite this to unconditionally get the CRTC properties and
pass the gamma size from drmModeCrtc as the default, which avoids both
branches and all the temporary variables.

Cheers,
Daniel