Re: [PATCH v2 08/43] drm/fbdev: Add fbdev-shmem

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Javier

Am 16.04.24 um 13:25 schrieb Javier Martinez Canillas:
Thomas Zimmermann <tzimmermann@xxxxxxx> writes:

Hello Thomas,

Add an fbdev emulation for SHMEM-based memory managers. The code is
similar to fbdev-generic, but does not require an addition shadow
"additional" I think ?

Yes.


buffer for mmap(). Fbdev-shmem operates directly on the buffer object's
SHMEM pages. Fbdev's deferred-I/O mechanism updates the hardware state
on write operations.

v2:
- use drm_driver_legacy_fb_format() (Geert)

Signed-off-by: Thomas Zimmermann <tzimmermann@xxxxxxx>
---
Patch looks good to me.

Reviewed-by: Javier Martinez Canillas <javierm@xxxxxxxxxx>

Thanks for reviewing.


Just a couple of questions below:

  drivers/gpu/drm/Makefile          |   1 +
  drivers/gpu/drm/drm_fbdev_shmem.c | 316 ++++++++++++++++++++++++++++++
Should fbdev-generic then be renamed to fbdev_shmem_shadow or something
like that ?

We'll do that in patch 42. :)


[...]

+
+	/* screen */
+	info->flags |= FBINFO_VIRTFB; /* system memory */
+	if (!shmem->map_wc)
+		info->flags |= FBINFO_READS_FAST; /* signal caching */
+	info->screen_size = sizes->surface_height * fb->pitches[0];
+	info->screen_buffer = map.vaddr;
+	info->fix.smem_len = info->screen_size;
+
Do I understand correctly that info->fix.smem_start doesn't have to be set
because that's only used for I/O memory?

It's the start of the framebuffer memory in physical memory. Setting smem_start only makes sense if the framebuffer is physically continuous, which isn't the case here.


Since drm_fbdev_shmem_fb_mmap() calls fb_deferred_io_mmap() which in turn
sets vma->vm_ops = &fb_deferred_io_vm_ops and struct vm_operations_struct
fb_deferred_io_vm_ops .fault function handler is fb_deferred_io_fault()
that calls fb_deferred_io_page() which uses info->fix.smem_start value.

Right, except in this case, which we don't trigger. Patch 5 adds an additional check to ensure this.


I guess is OK because is_vmalloc_addr() is always true for this case ?

It's not a vmalloc'ed address, but see patch 7. Fbdev-shmem uses a new get_page callback in fb_defio. It provides the necessary page directly to fb_defio.



This also made me think why info->fix.smem_len is really needed. Can't we
make the fbdev core to only look at that if info->screen_size is not set ?

The fbdev core doesn't use smem_len AFAICT. But smem_len is part of the fbdev UAPI, so I set it. I assume that programs use it to go to the end of the framebuffer memory.

Best regards
Thomas



--
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux