Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1681302 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  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> - 2017-07-05 08:30 +0200
    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> - 2017-07-06 08:00 +0200
    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> - 2017-07-06 10:40 +0200

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

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

[toc] | [next] | [standalone]

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

On Wed, Jul 5, 2017 at 7:50 PM, Peter Rosin <peda@axentia.se> wrote:
>>> +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.
>
> It doesn't work if I remove it. What is the disconnect?

Good question, didn't spot this at first, but your backoff/retry logic
is proken. When typing drm_modeset_lock locking code please make sure
you've enabled both CONFIG_PROVE_LOCKING and
CONFIG_DEBUG_WW_MUTEX_SLOWPATH. Without these two it's really easy to
get this wrong. Please also read
https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#kms-locking
carefully plus all the kernel-doc of the various hooks. This stuff is
a really tricky locking scheme, it takes a while to understand it and
implement it correctly. Which is why all the locking magic is in
shared code and for normal drivers no need think about it. For the
fundamental algorithm, you can also check out the docs for w/w mutexes
at https://www.kernel.org/doc/Documentation/locking/ww-mutex-design.txt

Might also help to read a bunch of the other locking paths again, with
my patches there's a few just in drm_fbdev_helper.c. I'll leave you
with these snippets here since I think this is fun to learn, but when
you're stuck I'm happy to help learn.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[toc] | [prev] | [next] | [standalone]

#1682191

> >> @@ -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);
> 
> That function is static, and...

Missed these comments here. I think we should just make them unstatic,
that might also explain why gamma_set is going this indirect path. Means
we need some kerneldoc, but that's not much work. Ping the original author
if you have questions.

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web