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 ? > +}; > + > 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> -- Best regards, Javier Martinez Canillas Linux Engineering Red Hat