Re: [PATCH v4 03/14] drm/fb-helper: separate the fb_setcmap helper into atomic and legacy paths

[Daniel Vetter]

On Tue, Jul 11, 2017 at 02:12:26PM +0200, Peter Rosin wrote:
> On 2017-07-11 10:10, Daniel Vetter wrote:
> > 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@xxxxxxxxxx>
> >> ---
> >>  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.
> 
> Yes, I thought about that, but ended up not. The reason is that as far
> as I could tell, all involved crtc need not have the same original
> gamma_lut. Sure, if all crtc have the same history, that should be the
> case, but isn't it possible to tie things together one way first and
> set some clut, then "rewire" things so that the crtc no longer have the
> same history?
> 
> But if you in the light of that still think it's wise to set the same
> clut for all crtc I will factor that part out of the loop.

Blob properties are invariant, if you want to change a lut you _have_ to
create a new blob property. They're also reference-counted, which means
users of a blob property can come&go as they wish, it will only get freed
when the last one is released.

So even when you change the lut of 1 CRTC the other CRTCs will be able to
keep using the existing lut blob property unchanged. That's the beauty of
having refcounted objects with invariant data over their lifetime, makes a
lot of things a lot simpler. drm_framebuffer work the same (only their
metadata is invariant, the data of the actual backing storage can change
ofc, but not where that backing storage is). Allows you to do simple
pointer comparison of objects to check whether their equal or something
has changed.

tldr; sharing blobs is perfectly safe and how this is designed to work.

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