Re: [PATCH 8/9] drm/gem-shmem: Implement fbdev dumb buffer and mmap helpers
From: Javier Martinez Canillas <javierm@redhat.com>
Date: 2022-03-08 19:29:46
Also in:
dri-devel
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@suse.de> ---
[snip]
quoted hunk ↗ jump to hunk
@@ -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 ?
quoted hunk ↗ jump to hunk
+}; + 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@redhat.com> -- Best regards, Javier Martinez Canillas Linux Engineering Red Hat