Date:   Tue, 11 Jul 2017 10:10:13 +0200
From:   Daniel Vetter <daniel@...ll.ch>
To:     Peter Rosin <peda@...ntia.se>
Cc:     linux-kernel@...r.kernel.org,
        Boris Brezillon <boris.brezillon@...e-electrons.com>,
        dri-devel@...ts.freedesktop.org,
        Daniel Vetter <daniel.vetter@...el.com>
Subject: Re: [PATCH v4 03/14] drm/fb-helper: separate the fb_setcmap helper
 into atomic and legacy paths

On Thu, Jul 06, 2017 at 02:20:37PM +0200, Peter Rosin wrote:
> The legacy path implements setcmap in terms of crtc .gamma_set.
> 
> The atomic path implements setcmap by directly updating the crtc gamma_lut
> property.
> 
> This has a couple of benefits:
> - it makes the redundant fb helpers .load_lut, .gamma_set and .gamma_get
>   completely obsolete. They are now unused and subject for removal.
> - atomic drivers that support clut modes get fbdev support for those from
>   the drm core. This includes atmel-hlcdc, but perhaps others as well?
> 
> Signed-off-by: Peter Rosin <peda@...ntia.se>
> ---
>  drivers/gpu/drm/drm_fb_helper.c | 232 ++++++++++++++++++++++++++++------------
>  1 file changed, 161 insertions(+), 71 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 721511d..32d6ea1 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -1195,27 +1195,6 @@ void drm_fb_helper_set_suspend_unlocked(struct drm_fb_helper *fb_helper,
>  }
>  EXPORT_SYMBOL(drm_fb_helper_set_suspend_unlocked);
>  
> -static int setcolreg(struct drm_crtc *crtc, u16 red, u16 green,
> -		     u16 blue, u16 regno, struct fb_info *info)
> -{
> -	struct drm_fb_helper *fb_helper = info->par;
> -	struct drm_framebuffer *fb = fb_helper->fb;
> -
> -	/*
> -	 * The driver really shouldn't advertise pseudo/directcolor
> -	 * visuals if it can't deal with the palette.
> -	 */
> -	if (WARN_ON(!fb_helper->funcs->gamma_set ||
> -		    !fb_helper->funcs->gamma_get))
> -		return -EINVAL;
> -
> -	WARN_ON(fb->format->cpp[0] != 1);
> -
> -	fb_helper->funcs->gamma_set(crtc, red, green, blue, regno);
> -
> -	return 0;
> -}
> -
>  static int setcmap_pseudo_palette(struct fb_cmap *cmap, struct fb_info *info)
>  {
>  	u32 *palette = (u32 *)info->pseudo_palette;
> @@ -1248,57 +1227,140 @@ static int setcmap_pseudo_palette(struct fb_cmap *cmap, struct fb_info *info)
>  	return 0;
>  }
>  
> -/**
> - * drm_fb_helper_setcmap - implementation for &fb_ops.fb_setcmap
> - * @cmap: cmap to set
> - * @info: fbdev registered by the helper
> - */
> -int drm_fb_helper_setcmap(struct fb_cmap *cmap, struct fb_info *info)
> +static int setcmap_legacy(struct fb_cmap *cmap, struct fb_info *info)
>  {
>  	struct drm_fb_helper *fb_helper = info->par;
> -	struct drm_device *dev = fb_helper->dev;
> -	const struct drm_crtc_helper_funcs *crtc_funcs;
> -	u16 *red, *green, *blue, *transp;
>  	struct drm_crtc *crtc;
>  	u16 *r, *g, *b;
> -	int i, j, rc = 0;
> -	int start;
> +	int i, ret = 0;
>  
> -	if (oops_in_progress)
> -		return -EBUSY;
> +	drm_modeset_lock_all(fb_helper->dev);
> +	for (i = 0; i < fb_helper->crtc_count; i++) {
> +		crtc = fb_helper->crtc_info[i].mode_set.crtc;
> +		if (!crtc->funcs->gamma_set || !crtc->gamma_size)
> +			return -EINVAL;
>  
> -	mutex_lock(&fb_helper->lock);
> -	if (!drm_fb_helper_is_bound(fb_helper)) {
> -		mutex_unlock(&fb_helper->lock);
> -		return -EBUSY;
> +		if (cmap->start + cmap->len > crtc->gamma_size)
> +			return -EINVAL;
> +
> +		r = crtc->gamma_store;
> +		g = r + crtc->gamma_size;
> +		b = g + crtc->gamma_size;
> +
> +		memcpy(r + cmap->start, cmap->red, cmap->len * sizeof(*r));
> +		memcpy(g + cmap->start, cmap->green, cmap->len * sizeof(*g));
> +		memcpy(b + cmap->start, cmap->blue, cmap->len * sizeof(*b));
> +
> +		ret = crtc->funcs->gamma_set(crtc, r, g, b,
> +					     crtc->gamma_size, NULL);
> +		if (ret)
> +			return ret;
>  	}
> +	drm_modeset_unlock_all(fb_helper->dev);
>  
> -	drm_modeset_lock_all(dev);
> -	if (info->fix.visual == FB_VISUAL_TRUECOLOR) {
> -		rc = setcmap_pseudo_palette(cmap, info);
> -		goto out;
> +	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 = (struct drm_color_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;
> +	struct drm_modeset_acquire_ctx ctx;
> +	struct drm_crtc_state *crtc_state;
> +	struct drm_atomic_state *state;
> +	struct drm_crtc *crtc;
> +	u16 *r, *g, *b;
> +	int i, ret = 0;
> +
> +	drm_modeset_acquire_init(&ctx, 0);
> +
> +	state = drm_atomic_state_alloc(dev);
> +	if (!state) {
> +		ret = -ENOMEM;
> +		goto out_ctx;
> +	}
> +
> +	state->acquire_ctx = &ctx;
> +retry:
>  	for (i = 0; i < fb_helper->crtc_count; i++) {
> -		crtc = fb_helper->crtc_info[i].mode_set.crtc;
> -		crtc_funcs = crtc->helper_private;
> +		bool replaced = false;
>  
> -		red = cmap->red;
> -		green = cmap->green;
> -		blue = cmap->blue;
> -		transp = cmap->transp;
> -		start = cmap->start;
> +		crtc = fb_helper->crtc_info[i].mode_set.crtc;
>  
> -		if (!crtc->gamma_size) {
> -			rc = -EINVAL;
> -			goto out;
> +		gamma_lut = setcmap_new_gamma_lut(crtc, cmap);

Tiny nit you might want to improve (since you need to respin for my naming
bikeshed of the property_replace_blob function anyway): Properties are
refcounting and invariant, which means you can just create the property
once, and then use it for all the CRTC. Slightly cleaner code.

Otherwise I've carefully reviewed this, and it all looks perfect now.

Thanks, Daniel

> +		if (IS_ERR(gamma_lut)) {
> +			ret = PTR_ERR(gamma_lut);
> +			goto out_state;
>  		}
>  
> -		if (cmap->start + cmap->len > crtc->gamma_size) {
> -			rc = -EINVAL;
> -			goto out;
> +		crtc_state = drm_atomic_get_crtc_state(state, crtc);
> +		if (IS_ERR(crtc_state)) {
> +			drm_property_blob_put(gamma_lut);
> +			ret = PTR_ERR(crtc_state);
> +			goto out_state;
>  		}
>  
> +		drm_atomic_replace_property_blob(&crtc_state->degamma_lut,
> +						 NULL, &replaced);
> +		drm_atomic_replace_property_blob(&crtc_state->ctm,
> +						 NULL, &replaced);
> +		drm_atomic_replace_property_blob(&crtc_state->gamma_lut,
> +						 gamma_lut, &replaced);
> +		crtc_state->color_mgmt_changed |= replaced;
> +		drm_property_blob_put(gamma_lut);
> +	}
> +
> +	ret = drm_atomic_commit(state);
> +	if (ret)
> +		goto out_state;
> +
> +	for (i = 0; i < fb_helper->crtc_count; i++) {
> +		crtc = fb_helper->crtc_info[i].mode_set.crtc;
> +
>  		r = crtc->gamma_store;
>  		g = r + crtc->gamma_size;
>  		b = g + crtc->gamma_size;
> @@ -1306,28 +1368,56 @@ int drm_fb_helper_setcmap(struct fb_cmap *cmap, struct fb_info *info)
>  		memcpy(r + cmap->start, cmap->red, cmap->len * sizeof(*r));
>  		memcpy(g + cmap->start, cmap->green, cmap->len * sizeof(*g));
>  		memcpy(b + cmap->start, cmap->blue, cmap->len * sizeof(*b));
> +	}
> +
> +out_state:
> +	if (ret == -EDEADLK)
> +		goto backoff;
> +
> +	drm_atomic_state_put(state);
> +out_ctx:
> +	drm_modeset_drop_locks(&ctx);
> +	drm_modeset_acquire_fini(&ctx);
>  
> -		for (j = 0; j < cmap->len; j++) {
> -			u16 hred, hgreen, hblue, htransp = 0xffff;
> +	return ret;
>  
> -			hred = *red++;
> -			hgreen = *green++;
> -			hblue = *blue++;
> +backoff:
> +	drm_atomic_state_clear(state);
> +	drm_modeset_backoff(&ctx);
> +	goto retry;
> +}
>  
> -			if (transp)
> -				htransp = *transp++;
> +/**
> + * drm_fb_helper_setcmap - implementation for &fb_ops.fb_setcmap
> + * @cmap: cmap to set
> + * @info: fbdev registered by the helper
> + */
> +int drm_fb_helper_setcmap(struct fb_cmap *cmap, struct fb_info *info)
> +{
> +	struct drm_fb_helper *fb_helper = info->par;
> +	int ret;
>  
> -			rc = setcolreg(crtc, hred, hgreen, hblue, start++, info);
> -			if (rc)
> -				goto out;
> -		}
> -		if (crtc_funcs->load_lut)
> -			crtc_funcs->load_lut(crtc);
> +	if (oops_in_progress)
> +		return -EBUSY;
> +
> +	mutex_lock(&fb_helper->lock);
> +
> +	if (!drm_fb_helper_is_bound(fb_helper)) {
> +		ret = -EBUSY;
> +		goto out;
>  	}
> - out:
> -	drm_modeset_unlock_all(dev);
> +
> +	if (info->fix.visual == FB_VISUAL_TRUECOLOR)
> +		ret = setcmap_pseudo_palette(cmap, info);
> +	else if (drm_drv_uses_atomic_modeset(fb_helper->dev))
> +		ret = setcmap_atomic(cmap, info);
> +	else
> +		ret = setcmap_legacy(cmap, info);
> +
> +out:
>  	mutex_unlock(&fb_helper->lock);
> -	return rc;
> +
> +	return ret;
>  }
>  EXPORT_SYMBOL(drm_fb_helper_setcmap);
>  
> -- 
> 2.1.4
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@...ts.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Powered by blists - more mailing lists