[Intel-gfx] [RFC 1/7] drm: Add Plane Degamma properties

[Emil Velikov]

On 7 November 2017 at 12:06, Uma Shankar <uma.shankar at intel.com> wrote:
> Add Plane Degamma as a blob property and plane
> degamma size as a range property.
>
> v2: Rebase
>
Hi Uma, seems like something has gone wrong during the rebase.

> Signed-off-by: Uma Shankar <uma.shankar at intel.com>
> ---
>  drivers/gpu/drm/drm_atomic.c        |   12 ++++++++++++
>  drivers/gpu/drm/drm_atomic_helper.c |    6 ++++++
>  drivers/gpu/drm/drm_mode_config.c   |   14 ++++++++++++++
>  include/drm/drm_mode_config.h       |   11 +++++++++++
>  include/drm/drm_plane.h             |   10 ++++++++++
>  5 files changed, 53 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 7e0e7be..30853c7 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -713,6 +713,8 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane,
>  {
>         struct drm_device *dev = plane->dev;
>         struct drm_mode_config *config = &dev->mode_config;
> +       bool replaced = false;
> +       int ret;
>
>         if (property == config->prop_fb_id) {
>                 struct drm_framebuffer *fb = drm_framebuffer_lookup(dev, NULL, val);
> @@ -758,6 +760,12 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane,
>         } else if (plane->funcs->atomic_set_property) {
>                 return plane->funcs->atomic_set_property(plane, state,
>                                 property, val);
> +       } else if (property == config->plane_degamma_lut_property) {
> +               ret = drm_atomic_replace_property_blob_from_id(dev,
> +                                       &state->degamma_lut,
> +                                       val, -1, &replaced);
> +               state->color_mgmt_changed |= replaced;
> +               return ret;
Namely: the driver specific atomic_set_property will be called and the
newly added code will not be reached.
I think we should keep the atomic_set_property call last in the
if/else chain. Converting the lot to a switch statement might make
things a bit more obvious.


>         } else {
>                 return -EINVAL;
>         }
> @@ -816,6 +824,9 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane,
>                 *val = state->zpos;
>         } else if (plane->funcs->atomic_get_property) {
>                 return plane->funcs->atomic_get_property(plane, state, property, val);
> +       } else if (property == config->plane_degamma_lut_property) {
> +               *val = (state->degamma_lut) ?
> +                       state->degamma_lut->base.id : 0;
Analogous thing happens here.

Did you test the updated series through IGT - it should have caught
the above (considering we have tests, and I'm not loosing my marbles).
Same comments apply for CTM and gamma, patches 2 and 3 respectively.

HTH
Emil