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

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. This way
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>
> +
>  	/**
>  	 * @quirk_addfb_prefer_xbgr_30bpp:
>  	 *
>

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:
<http://lists.linuxfoundation.org/pipermail/virtualization/attachments/20190707/866f7582/attachment.sig>

On Sun, Jul 07, 2019 at 06:14:50PM +0200, Thomas Zimmermann
wrote:
> 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.
If it annoys you too much, convert all the member docs in the header
comment into the inline style. That's what I usually do when the first
inline kerneldoc comment is warranted (like here), but not yet rolled out.

Or just mix&match if you feel like not doing that, we have lots of that
already :-)
-Daniel
> 
> > 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)
> 



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