Hi Am 08.03.22 um 20:29 schrieb Javier Martinez Canillas:
On 3/3/22 21:58, Thomas Zimmermann wrote:Implement struct drm_driver.dumb_create_fbdev for GEM SHMEM. The created buffer object returned by this function implements deferred I/O for its mmap operation. Add this feature to a number of drivers that use GEM SHMEM helpers as shadow planes over their regular video memory. The new macro DRM_GEM_SHMEM_DRIVER_OPS_WITH_SHADOW_PLANES sets the regular GEM functions and dumb_create_fbdev in struct drm_driver. Fbdev emulation on these drivers will now mmap the GEM SHMEM pages directly with deferred I/O without an intermediate shadow buffer. Signed-off-by: Thomas Zimmermann <tzimmermann@xxxxxxx> ---[snip]@@ -49,8 +50,20 @@ static const struct drm_gem_object_funcs drm_gem_shmem_funcs = { .vm_ops = &drm_gem_shmem_vm_ops, };+static const struct drm_gem_object_funcs drm_gem_shmem_funcs_fbdev = {+ .free = drm_gem_shmem_object_free, + .print_info = drm_gem_shmem_object_print_info, + .pin = drm_gem_shmem_object_pin, + .unpin = drm_gem_shmem_object_unpin, + .get_sg_table = drm_gem_shmem_object_get_sg_table, + .vmap = drm_gem_shmem_object_vmap, + .vunmap = drm_gem_shmem_object_vunmap, + .mmap = drm_gem_shmem_object_mmap_fbdev, + .vm_ops = &drm_gem_shmem_vm_ops_fbdev,The drm_gem_shmem_funcs_fbdev is the same than drm_gem_shmem_funcs but .mmap and .vm_ops callbacks. Maybe adding a macro to declare these two struct drm_gem_object_funcs could make easier to maintain it long term ?
I see you point, but I'm undecided about this. Such macros also add some amount of obfuscation. I'm not sure if it's worth it for an internal constant. And since the fbdev buffer is never exported, we could remove some of the callbacks. AFAICT we never call pin, unpin or get_sg_table.
Best regards Thomas
+}; + static struct drm_gem_shmem_object * -__drm_gem_shmem_create(struct drm_device *dev, size_t size, bool private) +__drm_gem_shmem_create(struct drm_device *dev, size_t size, bool private, bool fbdev) { struct drm_gem_shmem_object *shmem; struct drm_gem_object *obj; @@ -70,8 +83,12 @@ __drm_gem_shmem_create(struct drm_device *dev, size_t size, bool private) obj = &shmem->base; }- if (!obj->funcs)- obj->funcs = &drm_gem_shmem_funcs; + if (!obj->funcs) { + if (fbdev)Same question than in patch #6, maybe if (defined(CONFIG_DRM_FBDEV_EMULATION) && fbdev) ? [snip]+ */ +int drm_gem_shmem_dumb_create_fbdev(struct drm_file *file, struct drm_device *dev, + struct drm_mode_create_dumb *args) +{ +#if defined(CONFIG_DRM_FBDEV_EMULATION) + return __drm_gem_shmem_dumb_create(file, dev, true, args); +#else + return -ENOSYS; +#endif +}I believe the correct errno code here should be -ENOTSUPP. [snip]+ /* We don't use vmf->pgoff since that has the fake offset */ + page_offset = (vmf->address - vma->vm_start) >> PAGE_SHIFT;I see this (vmf->address - vma->vm_start) calculation in many places of this series. Maybe we could add a macro to calculate the offset instead ? Reviewed-by: Javier Martinez Canillas <javierm@xxxxxxxxxx>
-- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Ivo Totev
Attachment:
OpenPGP_signature
Description: OpenPGP digital signature
