diff mbox series

[1/9] drm/simpledrm: Use fbdev defaults for shadow buffering

Message ID 20220303205839.28484-2-tzimmermann@suse.de
State New
Headers show
Series drm: Support GEM SHMEM fbdev without shadow FB | expand

Commit Message

Thomas Zimmermann March 3, 2022, 8:58 p.m. UTC
Don't select shadow buffering for the fbdev console explicitly. The
fbdev emulation's heuristic will enable it for any framebuffer with
.dirty callback.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/tiny/simpledrm.c | 1 -
 1 file changed, 1 deletion(-)

Comments

Javier Martinez Canillas March 8, 2022, 9:31 a.m. UTC | #1
On 3/3/22 21:58, Thomas Zimmermann wrote:
> Don't select shadow buffering for the fbdev console explicitly. The
> fbdev emulation's heuristic will enable it for any framebuffer with
> .dirty callback.
>

Indeed it does. Not related to your series but looking at this
patch I noticed that drivers/gpu/drm/tiny/bochs.c will be the
only driver that sets .prefer_shadow_fbdev after this lands.

The driver is using GEM so I wonder if after your series this
DRM driver could have a .dirty callback and the field just be
dropped? Or there would still be a case where it is needed ?

Anyway, just wanted to mention in case I forget later.

Your patch looks good to me and I think it could be pushed
without waiting for the other patches in the series.

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
Thomas Zimmermann March 8, 2022, 9:56 a.m. UTC | #2
Hi

Am 08.03.22 um 10:31 schrieb Javier Martinez Canillas:
> On 3/3/22 21:58, Thomas Zimmermann wrote:
>> Don't select shadow buffering for the fbdev console explicitly. The
>> fbdev emulation's heuristic will enable it for any framebuffer with
>> .dirty callback.
>>
> 
> Indeed it does. Not related to your series but looking at this
> patch I noticed that drivers/gpu/drm/tiny/bochs.c will be the
> only driver that sets .prefer_shadow_fbdev after this lands.
> 
> The driver is using GEM so I wonder if after your series this
> DRM driver could have a .dirty callback and the field just be
> dropped? Or there would still be a case where it is needed ?
Bochs uses VRAM helpers (i.e., TTM). Fbdev and userspace would directly 
write into that buffer memory without a copy. So the dirty function 
would be empty.

Other drivers with VRAM helpers (e.g., hibmc, ast) operate on uncached 
I/O memory AFAICT. So they set .prefer_shadow, which also affects 
userspace. Bochs uses cached memory and shouldn't need prefer_shadow. 
Setting prefer_shadow_fbdev is only there for making the fbdev buffer 
object evictable from video memory.

As it stands, using prefer_shadow_fbdev is the cleanest solution, even 
if bochs is the only user of that field.

Alternatively, we could make it a requirement that qemu provides enough 
video memory for bochs to unconditionally pin the fbdev BO there without 
ever evicting. I guess, that would mean 32 MiB of VRAM at least.

Best regards
Thomas

> 
> Anyway, just wanted to mention in case I forget later.
> 
> Your patch looks good to me and I think it could be pushed
> without waiting for the other patches in the series.
> 
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
>
Javier Martinez Canillas March 8, 2022, 9:58 a.m. UTC | #3
On 3/8/22 10:56, Thomas Zimmermann wrote:
> Hi
> 
> Am 08.03.22 um 10:31 schrieb Javier Martinez Canillas:
>> On 3/3/22 21:58, Thomas Zimmermann wrote:
>>> Don't select shadow buffering for the fbdev console explicitly. The
>>> fbdev emulation's heuristic will enable it for any framebuffer with
>>> .dirty callback.
>>>
>>
>> Indeed it does. Not related to your series but looking at this
>> patch I noticed that drivers/gpu/drm/tiny/bochs.c will be the
>> only driver that sets .prefer_shadow_fbdev after this lands.
>>
>> The driver is using GEM so I wonder if after your series this
>> DRM driver could have a .dirty callback and the field just be
>> dropped? Or there would still be a case where it is needed ?
> Bochs uses VRAM helpers (i.e., TTM). Fbdev and userspace would directly 
> write into that buffer memory without a copy. So the dirty function 
> would be empty.
> 
> Other drivers with VRAM helpers (e.g., hibmc, ast) operate on uncached 
> I/O memory AFAICT. So they set .prefer_shadow, which also affects 
> userspace. Bochs uses cached memory and shouldn't need prefer_shadow. 
> Setting prefer_shadow_fbdev is only there for making the fbdev buffer 
> object evictable from video memory.
> 
> As it stands, using prefer_shadow_fbdev is the cleanest solution, even 
> if bochs is the only user of that field.
> 
> Alternatively, we could make it a requirement that qemu provides enough 
> video memory for bochs to unconditionally pin the fbdev BO there without 
> ever evicting. I guess, that would mean 32 MiB of VRAM at least.
>

I see. Thanks a lot for this explanation.
 
> Best regards
> Thomas
>
diff mbox series

Patch

diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
index f5b8e864a5cd..768242a78e2b 100644
--- a/drivers/gpu/drm/tiny/simpledrm.c
+++ b/drivers/gpu/drm/tiny/simpledrm.c
@@ -801,7 +801,6 @@  static int simpledrm_device_init_modeset(struct simpledrm_device *sdev)
 	dev->mode_config.max_width = max_width;
 	dev->mode_config.min_height = mode->vdisplay;
 	dev->mode_config.max_height = max_height;
-	dev->mode_config.prefer_shadow_fbdev = true;
 	dev->mode_config.preferred_depth = sdev->format->cpp[0] * 8;
 	dev->mode_config.funcs = &simpledrm_mode_config_funcs;