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

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 ?

+{

•   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