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

[Javier Martinez Canillas <javierm@redhat.com>]

Thomas Zimmermann <tzimmermann@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 ?

> 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@suse.de>
> ---

Patch looks good to me.

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

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 ?

[...]

> +
> +	/* 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? 

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.

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

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 ?

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat