[Intel-gfx] [PATCH 3/5] drm: introduce pipe color correction properties

Emil Velikov emil.l.velikov at gmail.com
Fri Feb 26 00:36:12 UTC 2016


Hi Lionel,

A bunch of suggestions - feel free to take or ignore them :-)

On 25 February 2016 at 10:58, Lionel Landwerlin
<lionel.g.landwerlin at intel.com> wrote:
> Patch based on a previous series by Shashank Sharma.
>
> This introduces optional properties to enable color correction at the
> pipe level. It relies on 3 transformations applied to every pixels
> displayed. First a lookup into a degamma table, then a multiplication
> of the rgb components by a 3x3 matrix and finally another lookup into
> a gamma table.
>
> The following properties can be added to a pipe :
>   - DEGAMMA_LUT : blob containing degamma LUT
>   - DEGAMMA_LUT_SIZE : number of elements in DEGAMMA_LUT
>   - CTM : transformation matrix applied after the degamma LUT
>   - GAMMA_LUT : blob containing gamma LUT
>   - GAMMA_LUT_SIZE : number of elements in GAMMA_LUT
>
> DEGAMMA_LUT_SIZE and GAMMA_LUT_SIZE are read only properties, set by
> the driver to tell userspace applications what sizes should be the
> lookup tables in DEGAMMA_LUT and GAMMA_LUT.
>
> A helper is also provided so legacy gamma correction is redirected
> through these new properties.
>
> v2: Register LUT size properties as range
>
> v3: Fix round in drm_color_lut_get_value() helper
>     More docs on how degamma/gamma properties are used
>
> v4: Update contributors
>
> v5: Rename CTM_MATRIX property to CTM (Doh!)
>     Add legacy gamma_set atomic helper
>     Describe CTM/LUT acronyms in the kernel doc
>
> Signed-off-by: Shashank Sharma <shashank.sharma at intel.com>
> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin at intel.com>
> Signed-off-by: Kumar, Kiran S <kiran.s.kumar at intel.com>
> Signed-off-by: Kausal Malladi <kausalmalladi at gmail.com>
The above should be kept in the order of which people worked on them.

> Reviewed-by: Matt Roper <matthew.d.roper at intel.com>

> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c

> @@ -376,6 +377,57 @@ int drm_atomic_set_mode_prop_for_crtc(struct drm_crtc_state *state,
>  EXPORT_SYMBOL(drm_atomic_set_mode_prop_for_crtc);
>
>  /**
> + * drm_atomic_replace_property_blob - replace a blob property
> + * @blob: a pointer to the member blob to be replaced
> + * @new_blob: the new blob to replace with
> + * @expected_size: the expected size of the new blob
> + * @replaced: whether the blob has been replaced
> + *
> + * RETURNS:
> + * Zero on success, error code on failure
> + */
> +static int
> +drm_atomic_replace_property_blob(struct drm_property_blob **blob,
> +                                struct drm_property_blob *new_blob,
> +                                bool *replaced)
"Replaced" here and though the rest of the patch is used as "changed".
Worth naming it that way ?

> +{
> +       struct drm_property_blob *old_blob = *blob;
> +
> +       if (old_blob == new_blob)
> +               return 0;
> +
> +       if (old_blob)
> +               drm_property_unreference_blob(old_blob);
> +       if (new_blob)
> +               drm_property_reference_blob(new_blob);
> +       *blob = new_blob;
> +       *replaced = true;
> +
> +       return 0;
The function always succeeds - drop the return value ?

> +}
> +
> +static int
> +drm_atomic_replace_property_blob_from_id(struct drm_crtc *crtc,
> +                                        struct drm_property_blob **blob,
> +                                        uint64_t blob_id,
> +                                        ssize_t expected_size,
> +                                        bool *replaced)
> +{
> +       struct drm_device *dev = crtc->dev;
> +       struct drm_property_blob *new_blob = NULL;
> +
> +       if (blob_id != 0) {
> +               new_blob = drm_property_lookup_blob(dev, blob_id);
> +               if (new_blob == NULL)
> +                       return -EINVAL;
> +               if (expected_size > 0 && expected_size != new_blob->length)
> +                       return -EINVAL;
> +       }
> +
Having a look at drm_atomic_set_mode_prop_for_crtc() I think I can
spot a bug - it shouldn't drop/unref the old blob in case of an error.
A case you handle nicely here. Perhaps it's worth using the
drm_atomic_replace_property_blob() in there ?


> @@ -397,6 +449,7 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
>  {
>         struct drm_device *dev = crtc->dev;
>         struct drm_mode_config *config = &dev->mode_config;
> +       bool replaced = false;
>         int ret;
>
>         if (property == config->prop_active)
> @@ -407,8 +460,31 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
>                 ret = drm_atomic_set_mode_prop_for_crtc(state, mode);
>                 drm_property_unreference_blob(mode);
>                 return ret;
> -       }
> -       else if (crtc->funcs->atomic_set_property)
> +       } else if (property == config->degamma_lut_property) {
> +               ret = drm_atomic_replace_property_blob_from_id(crtc,
> +                                       &state->degamma_lut,
> +                                       val,
> +                                       -1,
> +                                       &replaced);
> +               state->color_mgmt_changed = replaced;
> +               return ret;

> +       } else if (property == config->gamma_lut_property) {
> +               ret = drm_atomic_replace_property_blob_from_id(crtc,
> +                                       &state->gamma_lut,
> +                                       val,
> +                                       -1,
Wondering if these "-1" shouldn't be derived/replaced with the
contents of the respective _size properly ?


> @@ -444,6 +520,12 @@ drm_atomic_crtc_get_property(struct drm_crtc *crtc,
>                 *val = state->active;
>         else if (property == config->prop_mode_id)
>                 *val = (state->mode_blob) ? state->mode_blob->base.id : 0;
> +       else if (property == config->degamma_lut_property)
> +               *val = (state->degamma_lut) ? state->degamma_lut->base.id : 0;
> +       else if (property == config->ctm_property)
> +               *val = (state->ctm) ? state->ctm->base.id : 0;
> +       else if (property == config->gamma_lut_property)
> +               *val = (state->gamma_lut) ? state->gamma_lut->base.id : 0;
>         else if (crtc->funcs->atomic_get_property)
>                 return crtc->funcs->atomic_get_property(crtc, state, property, val);
>         else
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 4da4f2a..7ab8040 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c

> @@ -2557,6 +2564,9 @@ void __drm_atomic_helper_crtc_destroy_state(struct drm_crtc *crtc,
>                                             struct drm_crtc_state *state)
>  {
>         drm_property_unreference_blob(state->mode_blob);
> +       drm_property_unreference_blob(state->degamma_lut);
> +       drm_property_unreference_blob(state->ctm);
> +       drm_property_unreference_blob(state->gamma_lut);
Might want to keep the dtor in reverse order comparing to the ctor -
duplicate_state()


> @@ -2870,3 +2880,96 @@ void drm_atomic_helper_connector_destroy_state(struct drm_connector *connector,
>         kfree(state);
>  }
>  EXPORT_SYMBOL(drm_atomic_helper_connector_destroy_state);
> +
> +/**
> + * drm_atomic_helper_legacy_gamma_set - set the legacy gamma correction table
> + * @crtc: CRTC object
> + * @red: red correction table
> + * @green: green correction table
> + * @blue: green correction table
> + * @start:
> + * @size: size of the tables
> + *
> + * Implements support for legacy gamma correction table for drivers
> + * that support color management through the DEGAMMA_LUT/GAMMA_LUT
> + * properties.
> + */
> +void drm_atomic_helper_legacy_gamma_set(struct drm_crtc *crtc,
> +                                       u16 *red, u16 *green, u16 *blue,
> +                                       uint32_t start, uint32_t size)
> +{
> +       struct drm_device *dev = crtc->dev;
> +       struct drm_mode_config *config = &dev->mode_config;
> +       struct drm_atomic_state *state;
> +       struct drm_crtc_state *crtc_state;
> +       struct drm_property_blob *blob = NULL;
> +       struct drm_color_lut *blob_data;
> +       int i, ret = 0;
> +
> +       state = drm_atomic_state_alloc(crtc->dev);
> +       if (!state)
> +               return;
> +
> +       blob = drm_property_create_blob(dev,
> +                                       sizeof(struct drm_color_lut) * size,
> +                                       NULL);
> +
To keep the bringup/teardown simpler (and complete):
Move create_blob() before to state_alloc() and null check blob
immediately. One would need to add unref_blob() when state_alloc()
fails.

> +       state->acquire_ctx = crtc->dev->mode_config.acquire_ctx;
> +retry:
> +       crtc_state = drm_atomic_get_crtc_state(state, crtc);
> +       if (IS_ERR(crtc_state)) {
> +               ret = PTR_ERR(crtc_state);
> +               goto fail;
> +       }
> +
> +       /* Reset DEGAMMA_LUT and CTM properties. */
> +       ret = drm_atomic_crtc_set_property(crtc, crtc_state,
> +                       config->degamma_lut_property, 0);
> +       if (ret)
> +               goto fail;
Add new blank line please.

> +       ret = drm_atomic_crtc_set_property(crtc, crtc_state,
> +                       config->ctm_property, 0);
> +       if (ret)
> +               goto fail;
> +
> +       /* Set GAMMA_LUT with legacy values. */
> +       if (blob == NULL) {
> +               ret = -ENOMEM;
> +               goto fail;
> +       }
> +
> +       blob_data = (struct drm_color_lut *) blob->data;
> +       for (i = 0; i < size; i++) {
> +               blob_data[i].red = red[i];
> +               blob_data[i].green = green[i];
> +               blob_data[i].blue = blue[i];
> +       }
> +
Move this loop after create_blob()

> +       ret = drm_atomic_crtc_set_property(crtc, crtc_state,
> +                       config->gamma_lut_property, blob->base.id);
> +       if (ret)
> +               goto fail;
> +
> +       ret = drm_atomic_commit(state);
> +       if (ret != 0)
Please check in a consistent way. Currently we have ret != 0 vs ret
and foo == NULL vs !foo.

> +               goto fail;
> +
> +       drm_property_unreference_blob(blob);
> +
> +       /* Driver takes ownership of state on successful commit. */
Move the comment before unreference_blob(), so that it's closer to
atomic_commit() ?


> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -1554,6 +1554,41 @@ static int drm_mode_create_standard_properties(struct drm_device *dev)
>                 return -ENOMEM;
>         dev->mode_config.prop_mode_id = prop;
>
> +       prop = drm_property_create(dev,
> +                       DRM_MODE_PROP_BLOB,
> +                       "DEGAMMA_LUT", 0);

Just wondering -  don't we want this and the remaining properties to
be atomic only ? I doubt we have userspace that [will be updated to]
handle these, yet lacks atomic.


> --- a/drivers/gpu/drm/drm_crtc_helper.c
> +++ b/drivers/gpu/drm/drm_crtc_helper.c
> @@ -1075,3 +1075,36 @@ int drm_helper_crtc_mode_set_base(struct drm_crtc *crtc, int x, int y,
>         return drm_plane_helper_commit(plane, plane_state, old_fb);
>  }
>  EXPORT_SYMBOL(drm_helper_crtc_mode_set_base);
> +
> +/**
> + * drm_helper_crtc_enable_color_mgmt - enable color management properties
> + * @crtc: DRM CRTC
> + * @degamma_lut_size: the size of the degamma lut (before CSC)
> + * @gamma_lut_size: the size of the gamma lut (after CSC)
> + *
> + * This function lets the driver enable the color correction properties on a
> + * CRTC. This includes 3 degamma, csc and gamma properties that userspace can
> + * set and 2 size properties to inform the userspace of the lut sizes.
> + */
> +void drm_helper_crtc_enable_color_mgmt(struct drm_crtc *crtc,
> +                                      int degamma_lut_size,
> +                                      int gamma_lut_size)
> +{
> +       struct drm_device *dev = crtc->dev;
> +       struct drm_mode_config *config = &dev->mode_config;
> +
> +       drm_object_attach_property(&crtc->base,
> +                                  config->degamma_lut_property, 0);
> +       drm_object_attach_property(&crtc->base,
> +                                  config->ctm_property, 0);
> +       drm_object_attach_property(&crtc->base,
> +                                  config->gamma_lut_property, 0);
> +
> +       drm_object_attach_property(&crtc->base,
> +                                  config->degamma_lut_size_property,
> +                                  degamma_lut_size);
> +       drm_object_attach_property(&crtc->base,
> +                                  config->gamma_lut_size_property,
> +                                  gamma_lut_size);
Wondering if we cannot have these listed like elsewhere in the patch.
I.e. have the _size property just after its respective counterpart.

Regards,
Emil


More information about the Intel-gfx mailing list