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

[Thomas Zimmermann]

Hi Javier

Am 16.04.24 um 13:25 schrieb Javier Martinez Canillas:
> Thomas Zimmermann <tzimmermann at suse.de> 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 at suse.de>
>> ---
> Patch looks good to me.
>
> Reviewed-by: Javier Martinez Canillas <javierm at redhat.com>

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)