Re: [PATCH v2 3/6] drm/fb-helper: Instanciate shadow FB if configured in device's mode_config
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
> >> ---
> >> 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, _copy);
> >>}
> >> - helper->fb->funcs->dirty(helper->fb, NULL, 0, 0, _copy, 1);
> >> + if (helper->fb->funcs->dirty)
> >> + helper->fb->funcs->dirty(helper->fb, NULL, 0, 0,
> >> + _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 = >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(>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 +,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 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;
Re: [PATCH v2 3/6] drm/fb-helper: Instanciate shadow FB if configured in device's mode_config
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
>> ---
>> 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, _copy);
>> }
>> -helper->fb->funcs->dirty(helper->fb, NULL, 0, 0, _copy, 1);
>> +if (helper->fb->funcs->dirty)
>> +helper->fb->funcs->dirty(helper->fb, NULL, 0, 0,
>> + _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 = >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(>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 +,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
>
> 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
Thanks for reviewing and testing the patches.
Best regards
Thomas
>
>> +
>> /**
>> * @quirk_addfb_prefer_xbgr_30bpp:
>> *
>>
> ___
> dri-devel mailing list
> dri-de...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
--
Re: [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
> ---
> 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, _copy);
> }
> - helper->fb->funcs->dirty(helper->fb, NULL, 0, 0, _copy, 1);
> + if (helper->fb->funcs->dirty)
> + helper->fb->funcs->dirty(helper->fb, NULL, 0, 0,
> + _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 = >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(>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 +,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
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
> +
> /**
>* @quirk_addfb_prefer_xbgr_30bpp:
>*
>
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v2 3/6] drm/fb-helper: Instanciate shadow FB if configured in device's mode_config
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
---
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, _copy);
}
- helper->fb->funcs->dirty(helper->fb, NULL, 0, 0, _copy, 1);
+ if (helper->fb->funcs->dirty)
+ helper->fb->funcs->dirty(helper->fb, NULL, 0, 0,
+_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 = >dirty_clip;
unsigned long flags;
- if (!helper->fb->funcs->dirty)
- return;
-
spin_lock_irqsave(>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 |
+ !!fb->funcs->dirty;
+}
+
/**
* drm_fb_helper_generic_probe - Generic fbdev emulation probe helper
* @fb_helper: fbdev helper structure
@@ -2213,7 +,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
* @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 */
+ uint32_t prefer_shadow_fbdev;
+
/**
* @quirk_addfb_prefer_xbgr_30bpp:
*
--
2.21.0
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
