[PATCH v2 3/6] drm/fb-helper: Instanciate shadow FB if configured in device's mode_config

[Thomas Zimmermann]

Hi

Am 07.07.19 um 16:37 schrieb Noralf Trønnes:
> 
> 
> Den 05.07.2019 11.26, skrev Thomas Zimmermann:
>> Generic framebuffer emulation uses a shadow buffer for framebuffers with
>> dirty() function. If drivers want to use the shadow FB without such a
>> function, they can now set prefer_shadow or prefer_shadow_fbdev in their
>> mode_config structures. The former flag is exported to userspace, the latter
>> flag is fbdev-only.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann at suse.de>
>> ---
>>  drivers/gpu/drm/drm_fb_helper.c | 19 ++++++++++++++-----
>>  include/drm/drm_mode_config.h   |  5 +++++
>>  2 files changed, 19 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
>> index 7ba6a0255821..56ef169e1814 100644
>> --- a/drivers/gpu/drm/drm_fb_helper.c
>> +++ b/drivers/gpu/drm/drm_fb_helper.c
>> @@ -421,7 +421,9 @@ static void drm_fb_helper_dirty_work(struct work_struct *work)
>>  				return;
>>  			drm_fb_helper_dirty_blit_real(helper, &clip_copy);
>>  		}
>> -		helper->fb->funcs->dirty(helper->fb, NULL, 0, 0, &clip_copy, 1);
>> +		if (helper->fb->funcs->dirty)
>> +			helper->fb->funcs->dirty(helper->fb, NULL, 0, 0,
>> +						 &clip_copy, 1);
>>  
>>  		if (helper->buffer)
>>  			drm_client_buffer_vunmap(helper->buffer);
>> @@ -620,9 +622,6 @@ static void drm_fb_helper_dirty(struct fb_info *info, u32 x, u32 y,
>>  	struct drm_clip_rect *clip = &helper->dirty_clip;
>>  	unsigned long flags;
>>  
>> -	if (!helper->fb->funcs->dirty)
>> -		return;
> 
> drm_fb_helper_dirty() is called unconditionally by
> drm_fb_helper_sys_imageblit() et al, so we need check with
> drm_fbdev_use_shadow_fb() here.
> 
>> -
>>  	spin_lock_irqsave(&helper->dirty_lock, flags);
>>  	clip->x1 = min_t(u32, clip->x1, x);
>>  	clip->y1 = min_t(u32, clip->y1, y);
>> @@ -2166,6 +2165,16 @@ static struct fb_deferred_io drm_fbdev_defio = {
>>  	.deferred_io	= drm_fb_helper_deferred_io,
>>  };
>>  
>> +static bool drm_fbdev_use_shadow_fb(struct drm_fb_helper *fb_helper)
>> +{
>> +	struct drm_device *dev = fb_helper->dev;
>> +	struct drm_framebuffer *fb = fb_helper->fb;
>> +
>> +	return dev->mode_config.prefer_shadow_fbdev |
>> +	       dev->mode_config.prefer_shadow |
> 
> Use logical OR here
> 
>> +	       !!fb->funcs->dirty;
> 
> and you can drop the the double NOT here.
> 
>> +}
>> +
>>  /**
>>   * drm_fb_helper_generic_probe - Generic fbdev emulation probe helper
>>   * @fb_helper: fbdev helper structure
>> @@ -2213,7 +2222,7 @@ int drm_fb_helper_generic_probe(struct drm_fb_helper *fb_helper,
>>  
>>  	drm_fb_helper_fill_info(fbi, fb_helper, sizes);
>>  
>> -	if (fb->funcs->dirty) {
>> +	if (drm_fbdev_use_shadow_fb(fb_helper)) {
>>  		struct fb_ops *fbops;
>>  		void *shadow;
>>  
>> diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
>> index 759d462d028b..e1c751aca353 100644
>> --- a/include/drm/drm_mode_config.h
>> +++ b/include/drm/drm_mode_config.h
>> @@ -347,6 +347,8 @@ struct drm_mode_config_funcs {
>>   * @output_poll_work: delayed work for polling in process context
>>   * @preferred_depth: preferred RBG pixel depth, used by fb helpers
>>   * @prefer_shadow: hint to userspace to prefer shadow-fb rendering
>> + * @prefer_shadow_fbdev: hint to framebuffer emulation to prefer shadow-fb \
>> +	rendering
> 
> It's preferred to have the doc together with the struct member.

I just tried to follow the file's existing style, but OK, I don't mind.

> it's less likely to be forgotten when things change. And we don't use
> line cont. when the doc line is too long. Just continue on the next line
> after an asterix.
> 
>>   * @cursor_width: hint to userspace for max cursor width
>>   * @cursor_height: hint to userspace for max cursor height
>>   * @helper_private: mid-layer private data
>> @@ -852,6 +854,9 @@ struct drm_mode_config {
>>  	/* dumb ioctl parameters */
>>  	uint32_t preferred_depth, prefer_shadow;
>>  
>> +	/* fbdev parameters */
> 
> No need for this comment.
> 
> Doc can look like this, I've done s/framebuffer/fbdev/:
> 	/**
> 	 * @prefer_shadow_fbdev:
> 	 *
> 	 * Hint to fbdev emulation to prefer shadow-fb rendering.
> 	 */
> 
>> +	uint32_t prefer_shadow_fbdev;
> 
> Use bool here.
> 
> With that:
> 
> Reviewed-by: Noralf Trønnes <noralf at tronnes.org>
> 
> I have tested this on 2 drivers that use generic fbdev: vc4 (no shadow
> buf) and mi0283qt which has a dirty callback.
> 
> Tested-by: Noralf Trønnes <noralf at tronnes.org>

Thanks for reviewing and testing the patches.

Best regards
Thomas

> 
>> +
>>  	/**
>>  	 * @quirk_addfb_prefer_xbgr_30bpp:
>>  	 *
>>
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Linux GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah
HRB 21284 (AG Nürnberg)

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20190707/866f7582/attachment.sig>