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

From: Emil Velikov <emil.l.velikov@gmail.com>
To: Uma Shankar <uma.shankar@intel.com>
Cc: "intel-gfx@lists.freedesktop.org"
	<intel-gfx@lists.freedesktop.org>,
	ville.syrjala@intel.com,
	Maarten Lankhorst <maarten.lankhorst@intel.com>,
	ML dri-devel <dri-devel@lists.freedesktop.org>
Subject: Re: [Intel-gfx] [RFC 1/7] drm: Add Plane Degamma properties
Date: Tue, 7 Nov 2017 15:43:51 +0000	[thread overview]
Message-ID: <CACvgo51+ROeYjrxjHAGd6Gh0YYam2dDdhEKd1P_+sjFqyygyYw@mail.gmail.com> (raw)
In-Reply-To: <1510056391-9684-2-git-send-email-uma.shankar@intel.com>

On 7 November 2017 at 12:06, Uma Shankar <uma.shankar@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@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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel