Re: [PATCH 3/4] drm/fb-helper: Set up gamma_lut during restore_fbdev_mode_atomic()

[Daniel Vetter <daniel@ffwll.ch>]

On Fri, Jun 07, 2019 at 07:26:10PM +0300, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> i915 will now refuse to enable a C8 plane unless the gamma_lut
> is already enabled (to avoid scanning out whatever garbage got
> left in the hw LUT previously). So in order to make the fbdev
> code work with C8 we need to program the gamma_lut already
> during restore_fbdev_mode_atomic().
> 
> To avoid having to update the hw LUT every time
> restore_fbdev_mode_atomic() is called we hang on to the
> current gamma_lut blob. Note that the first crtc to get
> enabled will dictate the size of the gamma_lut, so this
> approach isn't 100% great for hardware with non-uniform
> crtcs. But that problem already exists in setcmap_atomic()
> so we'll just keep ignoring it.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_fb_helper.c | 165 ++++++++++++++++++++------------
>  include/drm/drm_fb_helper.h     |   7 ++
>  2 files changed, 113 insertions(+), 59 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index bdfa14cd7f6d..0ecec763789f 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -436,10 +436,76 @@ static bool drm_fb_helper_panel_rotation(struct drm_mode_set *modeset,
>  	return true;
>  }
>  
> +static int setcmap_crtc_atomic(struct drm_atomic_state *state,
> +			       struct drm_crtc *crtc,
> +			       struct drm_property_blob *gamma_lut)
> +{
> +	struct drm_crtc_state *crtc_state;
> +	bool replaced;
> +
> +	crtc_state = drm_atomic_get_crtc_state(state, crtc);
> +	if (IS_ERR(crtc_state))
> +		return PTR_ERR(crtc_state);
> +
> +	replaced  = drm_property_replace_blob(&crtc_state->degamma_lut,
> +					      NULL);
> +	replaced |= drm_property_replace_blob(&crtc_state->ctm, NULL);
> +	replaced |= drm_property_replace_blob(&crtc_state->gamma_lut,
> +					      gamma_lut);
> +	crtc_state->color_mgmt_changed |= replaced;
> +
> +	return 0;
> +}
> +
> +static struct drm_property_blob *setcmap_new_gamma_lut(struct drm_crtc *crtc,
> +						       struct fb_cmap *cmap)
> +{
> +	struct drm_device *dev = crtc->dev;
> +	struct drm_property_blob *gamma_lut;
> +	struct drm_color_lut *lut;
> +	int size = crtc->gamma_size;
> +	int i;
> +
> +	if (!size || cmap->start + cmap->len > size)
> +		return ERR_PTR(-EINVAL);
> +
> +	gamma_lut = drm_property_create_blob(dev, sizeof(*lut) * size, NULL);
> +	if (IS_ERR(gamma_lut))
> +		return gamma_lut;
> +
> +	lut = gamma_lut->data;
> +	if (cmap->start || cmap->len != size) {
> +		u16 *r = crtc->gamma_store;
> +		u16 *g = r + crtc->gamma_size;
> +		u16 *b = g + crtc->gamma_size;
> +
> +		for (i = 0; i < cmap->start; i++) {
> +			lut[i].red = r[i];
> +			lut[i].green = g[i];
> +			lut[i].blue = b[i];
> +		}
> +		for (i = cmap->start + cmap->len; i < size; i++) {
> +			lut[i].red = r[i];
> +			lut[i].green = g[i];
> +			lut[i].blue = b[i];
> +		}
> +	}
> +
> +	for (i = 0; i < cmap->len; i++) {
> +		lut[cmap->start + i].red = cmap->red[i];
> +		lut[cmap->start + i].green = cmap->green[i];
> +		lut[cmap->start + i].blue = cmap->blue[i];
> +	}
> +
> +	return gamma_lut;
> +}
> +
>  static int restore_fbdev_mode_atomic(struct drm_fb_helper *fb_helper, bool active)
>  {
> +	struct fb_info *info = fb_helper->fbdev;
>  	struct drm_client_dev *client = &fb_helper->client;
>  	struct drm_device *dev = fb_helper->dev;
> +	struct drm_property_blob *gamma_lut;
>  	struct drm_plane_state *plane_state;
>  	struct drm_plane *plane;
>  	struct drm_atomic_state *state;
> @@ -455,6 +521,10 @@ static int restore_fbdev_mode_atomic(struct drm_fb_helper *fb_helper, bool activ
>  		goto out_ctx;
>  	}
>  
> +	gamma_lut = fb_helper->gamma_lut;
> +	if (gamma_lut)
> +		drm_property_blob_get(gamma_lut);

Why the get/put stuff here?

> +
>  	state->acquire_ctx = &ctx;
>  retry:
>  	drm_for_each_plane(plane, dev) {
> @@ -476,7 +546,8 @@ static int restore_fbdev_mode_atomic(struct drm_fb_helper *fb_helper, bool activ
>  	}
>  
>  	drm_client_for_each_modeset(mode_set, client) {
> -		struct drm_plane *primary = mode_set->crtc->primary;
> +		struct drm_crtc *crtc = mode_set->crtc;
> +		struct drm_plane *primary = crtc->primary;
>  		unsigned int rotation;
>  
>  		if (drm_fb_helper_panel_rotation(mode_set, &rotation)) {
> @@ -489,24 +560,46 @@ static int restore_fbdev_mode_atomic(struct drm_fb_helper *fb_helper, bool activ
>  		if (ret != 0)
>  			goto out_state;
>  
> +		if (info->fix.visual != FB_VISUAL_TRUECOLOR) {
> +			if (!gamma_lut)
> +				gamma_lut = setcmap_new_gamma_lut(crtc, &info->cmap);
> +			if (IS_ERR(gamma_lut)) {
> +				ret = PTR_ERR(gamma_lut);
> +				gamma_lut = NULL;
> +				goto out_state;
> +			}
> +
> +			ret = setcmap_crtc_atomic(state, crtc, gamma_lut);
> +			if (ret)
> +				goto out_state;
> +		}
> +
>  		/*
>  		 * __drm_atomic_helper_set_config() sets active when a
>  		 * mode is set, unconditionally clear it if we force DPMS off
>  		 */
>  		if (!active) {
> -			struct drm_crtc *crtc = mode_set->crtc;
> -			struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
> +			struct drm_crtc_state *crtc_state =
> +				drm_atomic_get_new_crtc_state(state, crtc);
>  
>  			crtc_state->active = false;
>  		}
>  	}
>  
>  	ret = drm_atomic_commit(state);
> +	if (ret)
> +		goto out_state;
> +
> +	if (gamma_lut)
> +		drm_property_blob_get(gamma_lut);
> +	drm_property_blob_put(fb_helper->gamma_lut);
> +	fb_helper->gamma_lut = gamma_lut;

Can't we simplify using drm_property_replace_blob here and below?

>  
>  out_state:
>  	if (ret == -EDEADLK)
>  		goto backoff;
>  
> +	drm_property_blob_put(gamma_lut);
>  	drm_atomic_state_put(state);
>  out_ctx:
>  	drm_modeset_drop_locks(&ctx);
> @@ -996,6 +1089,9 @@ void drm_fb_helper_fini(struct drm_fb_helper *fb_helper)
>  	}
>  	fb_helper->fbdev = NULL;
>  
> +	drm_property_blob_put(fb_helper->gamma_lut);
> +	fb_helper->gamma_lut = NULL;
> +
>  	mutex_lock(&kernel_fb_helper_lock);
>  	if (!list_empty(&fb_helper->kernel_fb_list)) {
>  		list_del(&fb_helper->kernel_fb_list);
> @@ -1390,61 +1486,16 @@ static int setcmap_legacy(struct fb_cmap *cmap, struct fb_info *info)
>  	return ret;
>  }
>  
> -static struct drm_property_blob *setcmap_new_gamma_lut(struct drm_crtc *crtc,
> -						       struct fb_cmap *cmap)
> -{
> -	struct drm_device *dev = crtc->dev;
> -	struct drm_property_blob *gamma_lut;
> -	struct drm_color_lut *lut;
> -	int size = crtc->gamma_size;
> -	int i;
> -
> -	if (!size || cmap->start + cmap->len > size)
> -		return ERR_PTR(-EINVAL);
> -
> -	gamma_lut = drm_property_create_blob(dev, sizeof(*lut) * size, NULL);
> -	if (IS_ERR(gamma_lut))
> -		return gamma_lut;
> -
> -	lut = gamma_lut->data;
> -	if (cmap->start || cmap->len != size) {
> -		u16 *r = crtc->gamma_store;
> -		u16 *g = r + crtc->gamma_size;
> -		u16 *b = g + crtc->gamma_size;
> -
> -		for (i = 0; i < cmap->start; i++) {
> -			lut[i].red = r[i];
> -			lut[i].green = g[i];
> -			lut[i].blue = b[i];
> -		}
> -		for (i = cmap->start + cmap->len; i < size; i++) {
> -			lut[i].red = r[i];
> -			lut[i].green = g[i];
> -			lut[i].blue = b[i];
> -		}
> -	}
> -
> -	for (i = 0; i < cmap->len; i++) {
> -		lut[cmap->start + i].red = cmap->red[i];
> -		lut[cmap->start + i].green = cmap->green[i];
> -		lut[cmap->start + i].blue = cmap->blue[i];
> -	}
> -
> -	return gamma_lut;
> -}
> -
>  static int setcmap_atomic(struct fb_cmap *cmap, struct fb_info *info)
>  {
>  	struct drm_fb_helper *fb_helper = info->par;
>  	struct drm_device *dev = fb_helper->dev;
>  	struct drm_property_blob *gamma_lut = NULL;
>  	struct drm_modeset_acquire_ctx ctx;
> -	struct drm_crtc_state *crtc_state;
>  	struct drm_atomic_state *state;
>  	struct drm_mode_set *modeset;
>  	struct drm_crtc *crtc;
>  	u16 *r, *g, *b;
> -	bool replaced;
>  	int ret = 0;
>  
>  	drm_modeset_acquire_init(&ctx, 0);
> @@ -1468,18 +1519,9 @@ static int setcmap_atomic(struct fb_cmap *cmap, struct fb_info *info)
>  			goto out_state;
>  		}
>  
> -		crtc_state = drm_atomic_get_crtc_state(state, crtc);
> -		if (IS_ERR(crtc_state)) {
> -			ret = PTR_ERR(crtc_state);
> +		ret = setcmap_crtc_atomic(state, crtc, gamma_lut);
> +		if (ret)
>  			goto out_state;
> -		}
> -
> -		replaced  = drm_property_replace_blob(&crtc_state->degamma_lut,
> -						      NULL);
> -		replaced |= drm_property_replace_blob(&crtc_state->ctm, NULL);
> -		replaced |= drm_property_replace_blob(&crtc_state->gamma_lut,
> -						      gamma_lut);
> -		crtc_state->color_mgmt_changed |= replaced;
>  	}
>  
>  	ret = drm_atomic_commit(state);
> @@ -1498,6 +1540,11 @@ static int setcmap_atomic(struct fb_cmap *cmap, struct fb_info *info)
>  		memcpy(b + cmap->start, cmap->blue, cmap->len * sizeof(*b));
>  	}

The above "update legacy color table" stuff is missing from the
restore_fbdev_mode_atomic version. Unify more, i.e. maybe convert
setcmap_atomic to just update the property in fb_helper->gamma_lut, and
then call into restore_fbdev_mode_atomic to do all of the heavy lifting?

Or maybe we should push this into
drm_atomic_helper_update_legacy_modeset_state(), but that doesn't quite
work because locking rules are silly :-/ Or teach the gamma_get ioctl to
look at atomic and ditch this, dunno.
-Daniel

>  
> +	if (gamma_lut)
> +		drm_property_blob_get(gamma_lut);
> +	drm_property_blob_put(fb_helper->gamma_lut);
> +	fb_helper->gamma_lut = gamma_lut;
> +
>  out_state:
>  	if (ret == -EDEADLK)
>  		goto backoff;
> diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
> index 6b334f4d8a22..9c797346ede4 100644
> --- a/include/drm/drm_fb_helper.h
> +++ b/include/drm/drm_fb_helper.h
> @@ -155,6 +155,13 @@ struct drm_fb_helper {
>  	struct work_struct dirty_work;
>  	struct work_struct resume_work;
>  
> +	/**
> +	 * @gamma_lut:
> +	 *
> +	 * Current gamma_lut.
> +	 */
> +	struct drm_property_blob *gamma_lut;
> +
>  	/**
>  	 * @lock:
>  	 *
> -- 
> 2.21.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx