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

[Peter Rosin]

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.

Cheers,
peda

> 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@xxxxxxxxxxxxxxxxxxxxx
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>