Re: [PATCH v3 05/16] drm/fb-helper: do a generic fb_setcmap helper in terms of crtc .gamma_set

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Jul 04, 2017 at 12:37:01PM +0200, Peter Rosin wrote:
> This makes the redundant fb helpers .load_lut, .gamma_set and .gamma_get
> completely obsolete.
> 
> Signed-off-by: Peter Rosin <peda@axentia.se>
> ---
>  drivers/gpu/drm/drm_fb_helper.c | 165 +++++++++++++++++++++++-----------------
>  1 file changed, 94 insertions(+), 71 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index b75b1f2..7f8199a 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -1257,27 +1257,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;
> @@ -1310,54 +1289,68 @@ 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;
> +	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;
>  
> -	drm_modeset_lock_all(dev);
> -	if (info->fix.visual == FB_VISUAL_TRUECOLOR) {
> -		rc = setcmap_pseudo_palette(cmap, info);
> -		goto out;
> +		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;
>  	}
>  
> -	for (i = 0; i < fb_helper->crtc_count; i++) {
> -		crtc = fb_helper->crtc_info[i].mode_set.crtc;
> -		crtc_funcs = crtc->helper_private;
> +	return ret;
> +}

For the legacy path you need to keep the drm_modeset_lock_all (but only in
setcmap_legacy). Otherwise this part here looks good.

>  
> -		red = cmap->red;
> -		green = cmap->green;
> -		blue = cmap->blue;
> -		transp = cmap->transp;
> -		start = cmap->start;
> +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_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;
>  
> -		if (!crtc->gamma_size) {
> -			rc = -EINVAL;
> +	state = drm_atomic_state_alloc(dev);
> +	if (!state)
> +		return -ENOMEM;
> +	drm_modeset_acquire_init(&ctx, 0);
> +retry:
> +	ret = drm_modeset_lock_all_ctx(dev, &ctx);

With atomic you don't need to grab locks, this is done behind the scenes
(as long as you handle the retry/backoff correctly). See the kerneldoc for
the various drm_atomic_get_*_state functions.

> +	if (ret)
> +		goto fini;
> +	state->acquire_ctx = &ctx;
> +	for (i = 0; i < fb_helper->crtc_count; i++) {
> +		crtc = fb_helper->crtc_info[i].mode_set.crtc;
> +		if (!crtc->funcs->gamma_set) {
> +			ret = -EINVAL;
>  			goto out;
>  		}
>  
> -		if (cmap->start + cmap->len > crtc->gamma_size) {
> -			rc = -EINVAL;
> +		crtc_state = drm_atomic_get_crtc_state(state, crtc);
> +		if (IS_ERR(crtc_state)) {
> +			ret = PTR_ERR(crtc_state);
>  			goto out;
>  		}
>  
> @@ -1369,27 +1362,57 @@ int drm_fb_helper_setcmap(struct fb_cmap *cmap, struct fb_info *info)
>  		memcpy(g + cmap->start, cmap->green, cmap->len * sizeof(*g));
>  		memcpy(b + cmap->start, cmap->blue, cmap->len * sizeof(*b));
>  
> -		for (j = 0; j < cmap->len; j++) {
> -			u16 hred, hgreen, hblue, htransp = 0xffff;
> +		ret = crtc->funcs->gamma_set(crtc, r, g, b,
> +					     crtc->gamma_size, crtc_state);

I guess my description of what I have in mind wasn't really clear. I think
a proper atomic commit should never reuse one of the old hooks
(->gamma_set) here, that's just confusing. Instead what I had in mind is
to do the proper adjusting that gamma_set does here in this function, i.e.

- create the new blob, fill it with the cmap data

- assign that blob to the crtc state:

	drm_atomic_replace_property_blob(&crtc_state->gamma_lut,
					 new_table, &temp);

  Note that the drm_atomic_helper_legacy_gamma_set() does that in the most
  convoluted way by going through a few layers.

- The one thing you need to do on top is check that the gamma_lut property
  is supported (just check whether dev->mode_config.gamma_lut_property
  exists). That check is instead of checking for ->gamma_set.

Checking for matching size is optional, the driver must do that already
(for the atomic property).

This way your previous patch isn't needed, and we don't need to change all
the legacy callbacks. The only downside is that we duplicate a bit of the
atomic commit setup scaffolding, but that's imo ok. You could extract that
into a helper function shared between this code here and
drm_atomic_helper_legacy_gamma_set(), but that seems frankly overkill to
me. Creating atomic commits in the kernel is simply a bit verbose, but the
benefit of the current framework is that the driver side looks a lot
simpler.

> +		if (ret)
> +			goto out;
> +	}
>  
> -			hred = *red++;
> -			hgreen = *green++;
> -			hblue = *blue++;
> +	ret = drm_atomic_commit(state);
> +	if (ret == -EDEADLK) {
> +		drm_modeset_backoff(&ctx);
> +		goto retry;
> +	}
>  
> -			if (transp)
> -				htransp = *transp++;
> +out:
> +	drm_modeset_drop_locks(&ctx);
> +fini:
> +	drm_modeset_acquire_fini(&ctx);
> +	drm_atomic_state_put(state);
>  
> -			rc = setcolreg(crtc, hred, hgreen, hblue, start++, info);
> -			if (rc)
> -				goto out;
> -		}
> -		if (crtc_funcs->load_lut)
> -			crtc_funcs->load_lut(crtc);
> +	return ret;
> +}
> +
> +/**
> + * 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;
> +
> +	if (oops_in_progress)
> +		return -EBUSY;
> +
> +	mutex_lock(&fb_helper->lock);
> +
> +	if (!drm_fb_helper_is_bound(fb_helper)) {
> +		mutex_unlock(&fb_helper->lock);
> +		return -EBUSY;
>  	}
> - 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);
> +
>  	mutex_unlock(&fb_helper->lock);
> -	return rc;
> +
> +	return ret;
>  }
>  EXPORT_SYMBOL(drm_fb_helper_setcmap);

Besides the 2 comments this looks good and will get my r-b once revised.

Also on patches 1-3:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Cheers, Daniel
>  
> -- 
> 2.1.4
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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