Re: [PATCH 3/5] drm: introduce pipe color correction properties

[Lionel Landwerlin <lionel.g.landwerlin@intel.com>]

On 26/02/16 00:36, Emil Velikov wrote:
> 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@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@intel.com>
>> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
>> Signed-off-by: Kumar, Kiran S <kiran.s.kumar@intel.com>
>> Signed-off-by: Kausal Malladi <kausalmalladi@gmail.com>
> The above should be kept in the order of which people worked on them.
>
>> Reviewed-by: Matt Roper <matthew.d.roper@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 ?
I think the former describes the action, the later the state.

>
>> +{
>> +       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 ?
Well spotted, dropping.

>> +}
>> +
>> +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 ?

I'm not sure it matters as the drm_crtc_state you're set properties on 
will be discarded if there is an error.
The current drm_crtc_state that has been applied onto the hardware 
should be untouched.

>
>> @@ -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 ?

This is because we accept more than one size of degamma/gamma LUT 
(legacy -> 256 elements, new LUT -> (de)gamma_lut_size elements).
It's up for the driver the check the size and raise an error in its 
atomic_check() vfunc.

>
>
>> @@ -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.
Moving the if (blob == NULL) right after the blob allocation to make it 
simpler.

What about completeness? Is there something inherently wrong here?
>
>> +       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.

Sure.
>
>> +       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()
Thanks, indeed no need to refill it in case of retry.

>
>> +       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.

Sure.

>
>> +               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() ?

Sure.
>
>> --- 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.
This was pointed out by Matt already. Here is Daniel Stone's response :
https://lists.freedesktop.org/archives/intel-gfx/2016-January/086120.html

I think it's fine to have these properties not atomic because it's not 
really something you update very often (maybe just when starting your UI).
That's actually how we would like to use them in ChromiumOS as a first 
step, until eventually ChromiumOS switches to 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
>

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx