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@xxxxxxxxx> 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@xxxxxxxxx>
> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@xxxxxxxxx>
> Signed-off-by: Kumar, Kiran S <kiran.s.kumar@xxxxxxxxx>
> Signed-off-by: Kausal Malladi <kausalmalladi@xxxxxxxxx>
The above should be kept in the order of which people worked on them.
> Reviewed-by: Matt Roper <matthew.d.roper@xxxxxxxxx>
> --- 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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
