| Message ID | 20180523143411.64070-6-noralf@tronnes.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, May 23, 2018 at 04:34:07PM +0200, Noralf Trønnes wrote: > This is the first step in getting generic fbdev emulation. > A drm_fb_helper_funcs.fb_probe function is added which uses the > DRM client API to get a framebuffer backed by a dumb buffer. > > A transitional hack for tinydrm is needed in order to switch over all > CMA helper drivers in a later patch. This hack will be removed when > tinydrm moves to vmalloc buffers. > > Signed-off-by: Noralf Trønnes <noralf@tronnes.org> > --- > drivers/gpu/drm/drm_fb_helper.c | 164 ++++++++++++++++++++++++++++++++++++++++ > include/drm/drm_fb_helper.h | 26 +++++++ > 2 files changed, 190 insertions(+) > > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c > index 2ee1eaa66188..444c2b4040ea 100644 > --- a/drivers/gpu/drm/drm_fb_helper.c > +++ b/drivers/gpu/drm/drm_fb_helper.c > @@ -30,11 +30,13 @@ > #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > > #include <linux/console.h> > +#include <linux/dma-buf.h> > #include <linux/kernel.h> > #include <linux/sysrq.h> > #include <linux/slab.h> > #include <linux/module.h> > #include <drm/drmP.h> > +#include <drm/drm_client.h> > #include <drm/drm_crtc.h> > #include <drm/drm_fb_helper.h> > #include <drm/drm_crtc_helper.h> > @@ -2928,6 +2930,168 @@ void drm_fb_helper_output_poll_changed(struct drm_device *dev) > } > EXPORT_SYMBOL(drm_fb_helper_output_poll_changed); > > +/* @user: 1=userspace, 0=fbcon */ > +static int drm_fbdev_fb_open(struct fb_info *info, int user) > +{ > + struct drm_fb_helper *fb_helper = info->par; > + > + if (!try_module_get(fb_helper->dev->driver->fops->owner)) > + return -ENODEV; > + > + return 0; > +} > + > +static int drm_fbdev_fb_release(struct fb_info *info, int user) > +{ > + struct drm_fb_helper *fb_helper = info->par; > + > + module_put(fb_helper->dev->driver->fops->owner); > + > + return 0; > +} Hm, I thought earlier versions of your patch series had this separately, for everyone. What's the reasons for merging this into the fb_probe implementation. > + > +/* > + * fb_ops.fb_destroy is called by the last put_fb_info() call at the end of > + * unregister_framebuffer() or fb_release(). > + */ > +static void drm_fbdev_fb_destroy(struct fb_info *info) > +{ > + struct drm_fb_helper *fb_helper = info->par; > + struct fb_ops *fbops = NULL; > + > + DRM_DEBUG("\n"); > + > + if (fb_helper->fbdev->fbdefio) { > + fb_deferred_io_cleanup(fb_helper->fbdev); > + fbops = fb_helper->fbdev->fbops; > + } > + > + drm_fb_helper_fini(fb_helper); > + drm_client_framebuffer_delete(fb_helper->buffer); > + drm_client_free(fb_helper->client); > + kfree(fb_helper); > + kfree(fbops); > +} Hm, if we go with the idea that drm_clients could auto-unregister through a callback, then I expect this will lead to some control inversion. But we can fix that later on. > + > +static int drm_fbdev_fb_mmap(struct fb_info *info, struct vm_area_struct *vma) > +{ > + struct drm_fb_helper *fb_helper = info->par; > + > + if (fb_helper->dev->driver->gem_prime_mmap) > + return fb_helper->dev->driver->gem_prime_mmap(fb_helper->buffer->gem, vma); > + else > + return -ENODEV; > +} > + > +static struct fb_ops drm_fbdev_fb_ops = { > + /* No need to set owner, this module is already pinned by the driver. */ I'd still set it, means less thinking since more obviously correct. > + DRM_FB_HELPER_DEFAULT_OPS, > + .fb_open = drm_fbdev_fb_open, > + .fb_release = drm_fbdev_fb_release, > + .fb_destroy = drm_fbdev_fb_destroy, > + .fb_mmap = drm_fbdev_fb_mmap, > + .fb_read = drm_fb_helper_sys_read, > + .fb_write = drm_fb_helper_sys_write, > + .fb_fillrect = drm_fb_helper_sys_fillrect, > + .fb_copyarea = drm_fb_helper_sys_copyarea, > + .fb_imageblit = drm_fb_helper_sys_imageblit, Hm, some drivers require the cfb versions of these. In practice I guess there's not much of a difference really, at least on x86 and arm. We might want to document that though. > +}; > + > +static struct fb_deferred_io drm_fbdev_defio = { > + .delay = HZ / 20, > + .deferred_io = drm_fb_helper_deferred_io, > +}; > + > +/* > + * TODO: Remove this when tinydrm is converted to vmalloc buffers. > + */ > +static int drm_fbdev_cma_deferred_io_mmap(struct fb_info *info, > + struct vm_area_struct *vma) > +{ > + fb_deferred_io_mmap(info, vma); > + vma->vm_page_prot = pgprot_writecombine(vma->vm_page_prot); > + > + return 0; > +} > + Needs kerneldoc here. > +int drm_fb_helper_generic_probe(struct drm_fb_helper *fb_helper, > + struct drm_fb_helper_surface_size *sizes) > +{ > + struct drm_client_dev *client = fb_helper->client; > + struct drm_client_buffer *buffer; > + struct drm_framebuffer *fb; > + struct fb_info *fbi; > + u32 format; > + int ret; > + > + DRM_DEBUG_KMS("surface width(%d), height(%d) and bpp(%d)\n", > + sizes->surface_width, sizes->surface_height, > + sizes->surface_bpp); > + > + format = drm_mode_legacy_fb_format(sizes->surface_bpp, sizes->surface_depth); > + buffer = drm_client_framebuffer_create(client, sizes->surface_width, > + sizes->surface_height, format); > + if (IS_ERR(buffer)) > + return PTR_ERR(buffer); > + > + fb_helper->buffer = buffer; > + fb_helper->fb = buffer->fb; > + fb = buffer->fb; > + > + fbi = drm_fb_helper_alloc_fbi(fb_helper); > + if (IS_ERR(fbi)) { > + ret = PTR_ERR(fbi); > + goto err_free_buffer; > + } > + > + fbi->par = fb_helper; > + fbi->fbops = &drm_fbdev_fb_ops; > + fbi->screen_size = fb->height * fb->pitches[0]; > + fbi->fix.smem_len = fbi->screen_size; > + fbi->screen_buffer = buffer->vaddr; > + strcpy(fbi->fix.id, "DRM emulated"); > + > + drm_fb_helper_fill_fix(fbi, fb->pitches[0], fb->format->depth); > + drm_fb_helper_fill_var(fbi, fb_helper, sizes->fb_width, sizes->fb_height); > + > + if (fb->funcs->dirty) { > + struct fb_ops *fbops; > + > + /* > + * fb_deferred_io_cleanup() clears &fbops->fb_mmap so a per > + * instance version is necessary. > + */ > + fbops = kzalloc(sizeof(*fbops), GFP_KERNEL); > + if (!fbops) { > + ret = -ENOMEM; > + goto err_fb_info_destroy; > + } > + > + *fbops = *fbi->fbops; > + fbi->fbops = fbops; > + > + fbi->fbdefio = &drm_fbdev_defio; > + > + /* TODO: Remove this when tinydrm is converted to vmalloc buffers. */ > + fbi->fix.smem_start = page_to_phys(virt_to_page(buffer->vaddr)); > + > + fb_deferred_io_init(fbi); > + > + /* TODO: Remove this when tinydrm is converted to vmalloc buffers. */ > + fbi->fbops->fb_mmap = drm_fbdev_cma_deferred_io_mmap; > + } Ugh. Yeah defio and generic allocator through dumb buffers don't mix well. The only true generic solution for this would be to give userspace (and only userspace, for fbcon we can intercept everything) a staging buffer, and then upload things using the dirty callback. But that introduces another copy step, so isn't cool. I think a check for is_vmalloc_addr and if that's false, not doing any of the defio mmap setup would be good. Until we have a better idea. And yes that would need to be done after tinydrm is moved over. > + > + return 0; > + > +err_fb_info_destroy: > + drm_fb_helper_fini(fb_helper); > +err_free_buffer: > + drm_client_framebuffer_delete(buffer); > + > + return ret; > +} > +EXPORT_SYMBOL(drm_fb_helper_generic_probe); > + > /* The Kconfig DRM_KMS_HELPER selects FRAMEBUFFER_CONSOLE (if !EXPERT) > * but the module doesn't depend on any fb console symbols. At least > * attempt to load fbcon to avoid leaving the system without a usable console. > diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h > index b069433e7fc1..bb38469a9502 100644 > --- a/include/drm/drm_fb_helper.h > +++ b/include/drm/drm_fb_helper.h > @@ -30,6 +30,8 @@ > #ifndef DRM_FB_HELPER_H > #define DRM_FB_HELPER_H > > +struct drm_client_buffer; > +struct drm_client_dev; > struct drm_fb_helper; > > #include <drm/drm_crtc.h> > @@ -232,6 +234,20 @@ struct drm_fb_helper { > * See also: @deferred_setup > */ > int preferred_bpp; > + > + /** > + * @client: > + * > + * DRM client used by the generic fbdev emulation. > + */ > + struct drm_client_dev *client; > + > + /** > + * @buffer: > + * > + * Framebuffer used by the generic fbdev emulation. > + */ > + struct drm_client_buffer *buffer; > }; > > /** > @@ -330,6 +346,9 @@ void drm_fb_helper_fbdev_teardown(struct drm_device *dev); > > void drm_fb_helper_lastclose(struct drm_device *dev); > void drm_fb_helper_output_poll_changed(struct drm_device *dev); > + > +int drm_fb_helper_generic_probe(struct drm_fb_helper *fb_helper, > + struct drm_fb_helper_surface_size *sizes); > #else > static inline void drm_fb_helper_prepare(struct drm_device *dev, > struct drm_fb_helper *helper, > @@ -564,6 +583,13 @@ static inline void drm_fb_helper_output_poll_changed(struct drm_device *dev) > { > } > > +static inline int > +drm_fb_helper_generic_probe(struct drm_fb_helper *fb_helper, > + struct drm_fb_helper_surface_size *sizes) > +{ > + return 0; > +} Ok, I think this patch looks ok. With the missing kerneldoc added (which also explains the current limitations) this is Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > + > #endif > > static inline int > -- > 2.15.1 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Thu, May 24, 2018 at 11:16:05AM +0200, Daniel Vetter wrote: > On Wed, May 23, 2018 at 04:34:07PM +0200, Noralf Trønnes wrote: > > +int drm_fb_helper_generic_probe(struct drm_fb_helper *fb_helper, > > + struct drm_fb_helper_surface_size *sizes) > > +{ > > + struct drm_client_dev *client = fb_helper->client; > > + struct drm_client_buffer *buffer; > > + struct drm_framebuffer *fb; > > + struct fb_info *fbi; > > + u32 format; > > + int ret; > > + > > + DRM_DEBUG_KMS("surface width(%d), height(%d) and bpp(%d)\n", > > + sizes->surface_width, sizes->surface_height, > > + sizes->surface_bpp); > > + > > + format = drm_mode_legacy_fb_format(sizes->surface_bpp, sizes->surface_depth); > > + buffer = drm_client_framebuffer_create(client, sizes->surface_width, > > + sizes->surface_height, format); > > + if (IS_ERR(buffer)) > > + return PTR_ERR(buffer); > > + > > + fb_helper->buffer = buffer; > > + fb_helper->fb = buffer->fb; > > + fb = buffer->fb; > > + > > + fbi = drm_fb_helper_alloc_fbi(fb_helper); > > + if (IS_ERR(fbi)) { > > + ret = PTR_ERR(fbi); > > + goto err_free_buffer; > > + } > > + > > + fbi->par = fb_helper; > > + fbi->fbops = &drm_fbdev_fb_ops; > > + fbi->screen_size = fb->height * fb->pitches[0]; > > + fbi->fix.smem_len = fbi->screen_size; > > + fbi->screen_buffer = buffer->vaddr; > > + strcpy(fbi->fix.id, "DRM emulated"); > > + > > + drm_fb_helper_fill_fix(fbi, fb->pitches[0], fb->format->depth); > > + drm_fb_helper_fill_var(fbi, fb_helper, sizes->fb_width, sizes->fb_height); > > + > > + if (fb->funcs->dirty) { > > + struct fb_ops *fbops; > > + > > + /* > > + * fb_deferred_io_cleanup() clears &fbops->fb_mmap so a per > > + * instance version is necessary. > > + */ > > + fbops = kzalloc(sizeof(*fbops), GFP_KERNEL); > > + if (!fbops) { > > + ret = -ENOMEM; > > + goto err_fb_info_destroy; > > + } > > + > > + *fbops = *fbi->fbops; > > + fbi->fbops = fbops; > > + > > + fbi->fbdefio = &drm_fbdev_defio; > > + > > + /* TODO: Remove this when tinydrm is converted to vmalloc buffers. */ > > + fbi->fix.smem_start = page_to_phys(virt_to_page(buffer->vaddr)); > > + > > + fb_deferred_io_init(fbi); > > + > > + /* TODO: Remove this when tinydrm is converted to vmalloc buffers. */ > > + fbi->fbops->fb_mmap = drm_fbdev_cma_deferred_io_mmap; > > + } > > Ugh. Yeah defio and generic allocator through dumb buffers don't mix well. > The only true generic solution for this would be to give userspace (and > only userspace, for fbcon we can intercept everything) a staging buffer, > and then upload things using the dirty callback. > > But that introduces another copy step, so isn't cool. > > I think a check for is_vmalloc_addr and if that's false, not doing any of > the defio mmap setup would be good. Until we have a better idea. And yes > that would need to be done after tinydrm is moved over. Looking at the cma conversion patch, this stuff here is actually required. Or we'll break cma. I think your TODO comments need to be adjusted to reflect that. There's also the problem of how to handle the wc vs cached memory issue, we might need more flags for that. -Daniel
Den 24.05.2018 11.16, skrev Daniel Vetter: > On Wed, May 23, 2018 at 04:34:07PM +0200, Noralf Trønnes wrote: >> This is the first step in getting generic fbdev emulation. >> A drm_fb_helper_funcs.fb_probe function is added which uses the >> DRM client API to get a framebuffer backed by a dumb buffer. >> >> A transitional hack for tinydrm is needed in order to switch over all >> CMA helper drivers in a later patch. This hack will be removed when >> tinydrm moves to vmalloc buffers. >> >> Signed-off-by: Noralf Trønnes <noralf@tronnes.org> >> --- >> drivers/gpu/drm/drm_fb_helper.c | 164 ++++++++++++++++++++++++++++++++++++++++ >> include/drm/drm_fb_helper.h | 26 +++++++ >> 2 files changed, 190 insertions(+) >> >> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c >> index 2ee1eaa66188..444c2b4040ea 100644 >> --- a/drivers/gpu/drm/drm_fb_helper.c >> +++ b/drivers/gpu/drm/drm_fb_helper.c >> @@ -30,11 +30,13 @@ >> #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt >> >> #include <linux/console.h> >> +#include <linux/dma-buf.h> >> #include <linux/kernel.h> >> #include <linux/sysrq.h> >> #include <linux/slab.h> >> #include <linux/module.h> >> #include <drm/drmP.h> >> +#include <drm/drm_client.h> >> #include <drm/drm_crtc.h> >> #include <drm/drm_fb_helper.h> >> #include <drm/drm_crtc_helper.h> >> @@ -2928,6 +2930,168 @@ void drm_fb_helper_output_poll_changed(struct drm_device *dev) >> } >> EXPORT_SYMBOL(drm_fb_helper_output_poll_changed); >> >> +/* @user: 1=userspace, 0=fbcon */ >> +static int drm_fbdev_fb_open(struct fb_info *info, int user) >> +{ >> + struct drm_fb_helper *fb_helper = info->par; >> + >> + if (!try_module_get(fb_helper->dev->driver->fops->owner)) >> + return -ENODEV; >> + >> + return 0; >> +} >> + >> +static int drm_fbdev_fb_release(struct fb_info *info, int user) >> +{ >> + struct drm_fb_helper *fb_helper = info->par; >> + >> + module_put(fb_helper->dev->driver->fops->owner); >> + >> + return 0; >> +} > Hm, I thought earlier versions of your patch series had this separately, > for everyone. What's the reasons for merging this into the fb_probe > implementation. This is only necessary when struct fb_ops is defined in a library where the owner field is the library module and not the driver module. I realised that with this generic version it's highly unlikely that we get another library that defines struct fb_ops, so it's only needed here. Noralf. >> + >> +/* >> + * fb_ops.fb_destroy is called by the last put_fb_info() call at the end of >> + * unregister_framebuffer() or fb_release(). >> + */ >> +static void drm_fbdev_fb_destroy(struct fb_info *info) >> +{ >> + struct drm_fb_helper *fb_helper = info->par; >> + struct fb_ops *fbops = NULL; >> + >> + DRM_DEBUG("\n"); >> + >> + if (fb_helper->fbdev->fbdefio) { >> + fb_deferred_io_cleanup(fb_helper->fbdev); >> + fbops = fb_helper->fbdev->fbops; >> + } >> + >> + drm_fb_helper_fini(fb_helper); >> + drm_client_framebuffer_delete(fb_helper->buffer); >> + drm_client_free(fb_helper->client); >> + kfree(fb_helper); >> + kfree(fbops); >> +} > Hm, if we go with the idea that drm_clients could auto-unregister through > a callback, then I expect this will lead to some control inversion. But we > can fix that later on. > >> + >> +static int drm_fbdev_fb_mmap(struct fb_info *info, struct vm_area_struct *vma) >> +{ >> + struct drm_fb_helper *fb_helper = info->par; >> + >> + if (fb_helper->dev->driver->gem_prime_mmap) >> + return fb_helper->dev->driver->gem_prime_mmap(fb_helper->buffer->gem, vma); >> + else >> + return -ENODEV; >> +} >> + >> +static struct fb_ops drm_fbdev_fb_ops = { >> + /* No need to set owner, this module is already pinned by the driver. */ > I'd still set it, means less thinking since more obviously correct. > >> + DRM_FB_HELPER_DEFAULT_OPS, >> + .fb_open = drm_fbdev_fb_open, >> + .fb_release = drm_fbdev_fb_release, >> + .fb_destroy = drm_fbdev_fb_destroy, >> + .fb_mmap = drm_fbdev_fb_mmap, >> + .fb_read = drm_fb_helper_sys_read, >> + .fb_write = drm_fb_helper_sys_write, >> + .fb_fillrect = drm_fb_helper_sys_fillrect, >> + .fb_copyarea = drm_fb_helper_sys_copyarea, >> + .fb_imageblit = drm_fb_helper_sys_imageblit, > Hm, some drivers require the cfb versions of these. In practice I guess > there's not much of a difference really, at least on x86 and arm. > > We might want to document that though. > >> +}; >> + >> +static struct fb_deferred_io drm_fbdev_defio = { >> + .delay = HZ / 20, >> + .deferred_io = drm_fb_helper_deferred_io, >> +}; >> + >> +/* >> + * TODO: Remove this when tinydrm is converted to vmalloc buffers. >> + */ >> +static int drm_fbdev_cma_deferred_io_mmap(struct fb_info *info, >> + struct vm_area_struct *vma) >> +{ >> + fb_deferred_io_mmap(info, vma); >> + vma->vm_page_prot = pgprot_writecombine(vma->vm_page_prot); >> + >> + return 0; >> +} >> + > Needs kerneldoc here. > >> +int drm_fb_helper_generic_probe(struct drm_fb_helper *fb_helper, >> + struct drm_fb_helper_surface_size *sizes) >> +{ >> + struct drm_client_dev *client = fb_helper->client; >> + struct drm_client_buffer *buffer; >> + struct drm_framebuffer *fb; >> + struct fb_info *fbi; >> + u32 format; >> + int ret; >> + >> + DRM_DEBUG_KMS("surface width(%d), height(%d) and bpp(%d)\n", >> + sizes->surface_width, sizes->surface_height, >> + sizes->surface_bpp); >> + >> + format = drm_mode_legacy_fb_format(sizes->surface_bpp, sizes->surface_depth); >> + buffer = drm_client_framebuffer_create(client, sizes->surface_width, >> + sizes->surface_height, format); >> + if (IS_ERR(buffer)) >> + return PTR_ERR(buffer); >> + >> + fb_helper->buffer = buffer; >> + fb_helper->fb = buffer->fb; >> + fb = buffer->fb; >> + >> + fbi = drm_fb_helper_alloc_fbi(fb_helper); >> + if (IS_ERR(fbi)) { >> + ret = PTR_ERR(fbi); >> + goto err_free_buffer; >> + } >> + >> + fbi->par = fb_helper; >> + fbi->fbops = &drm_fbdev_fb_ops; >> + fbi->screen_size = fb->height * fb->pitches[0]; >> + fbi->fix.smem_len = fbi->screen_size; >> + fbi->screen_buffer = buffer->vaddr; >> + strcpy(fbi->fix.id, "DRM emulated"); >> + >> + drm_fb_helper_fill_fix(fbi, fb->pitches[0], fb->format->depth); >> + drm_fb_helper_fill_var(fbi, fb_helper, sizes->fb_width, sizes->fb_height); >> + >> + if (fb->funcs->dirty) { >> + struct fb_ops *fbops; >> + >> + /* >> + * fb_deferred_io_cleanup() clears &fbops->fb_mmap so a per >> + * instance version is necessary. >> + */ >> + fbops = kzalloc(sizeof(*fbops), GFP_KERNEL); >> + if (!fbops) { >> + ret = -ENOMEM; >> + goto err_fb_info_destroy; >> + } >> + >> + *fbops = *fbi->fbops; >> + fbi->fbops = fbops; >> + >> + fbi->fbdefio = &drm_fbdev_defio; >> + >> + /* TODO: Remove this when tinydrm is converted to vmalloc buffers. */ >> + fbi->fix.smem_start = page_to_phys(virt_to_page(buffer->vaddr)); >> + >> + fb_deferred_io_init(fbi); >> + >> + /* TODO: Remove this when tinydrm is converted to vmalloc buffers. */ >> + fbi->fbops->fb_mmap = drm_fbdev_cma_deferred_io_mmap; >> + } > Ugh. Yeah defio and generic allocator through dumb buffers don't mix well. > The only true generic solution for this would be to give userspace (and > only userspace, for fbcon we can intercept everything) a staging buffer, > and then upload things using the dirty callback. > > But that introduces another copy step, so isn't cool. > > I think a check for is_vmalloc_addr and if that's false, not doing any of > the defio mmap setup would be good. Until we have a better idea. And yes > that would need to be done after tinydrm is moved over. > >> + >> + return 0; >> + >> +err_fb_info_destroy: >> + drm_fb_helper_fini(fb_helper); >> +err_free_buffer: >> + drm_client_framebuffer_delete(buffer); >> + >> + return ret; >> +} >> +EXPORT_SYMBOL(drm_fb_helper_generic_probe); >> + >> /* The Kconfig DRM_KMS_HELPER selects FRAMEBUFFER_CONSOLE (if !EXPERT) >> * but the module doesn't depend on any fb console symbols. At least >> * attempt to load fbcon to avoid leaving the system without a usable console. >> diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h >> index b069433e7fc1..bb38469a9502 100644 >> --- a/include/drm/drm_fb_helper.h >> +++ b/include/drm/drm_fb_helper.h >> @@ -30,6 +30,8 @@ >> #ifndef DRM_FB_HELPER_H >> #define DRM_FB_HELPER_H >> >> +struct drm_client_buffer; >> +struct drm_client_dev; >> struct drm_fb_helper; >> >> #include <drm/drm_crtc.h> >> @@ -232,6 +234,20 @@ struct drm_fb_helper { >> * See also: @deferred_setup >> */ >> int preferred_bpp; >> + >> + /** >> + * @client: >> + * >> + * DRM client used by the generic fbdev emulation. >> + */ >> + struct drm_client_dev *client; >> + >> + /** >> + * @buffer: >> + * >> + * Framebuffer used by the generic fbdev emulation. >> + */ >> + struct drm_client_buffer *buffer; >> }; >> >> /** >> @@ -330,6 +346,9 @@ void drm_fb_helper_fbdev_teardown(struct drm_device *dev); >> >> void drm_fb_helper_lastclose(struct drm_device *dev); >> void drm_fb_helper_output_poll_changed(struct drm_device *dev); >> + >> +int drm_fb_helper_generic_probe(struct drm_fb_helper *fb_helper, >> + struct drm_fb_helper_surface_size *sizes); >> #else >> static inline void drm_fb_helper_prepare(struct drm_device *dev, >> struct drm_fb_helper *helper, >> @@ -564,6 +583,13 @@ static inline void drm_fb_helper_output_poll_changed(struct drm_device *dev) >> { >> } >> >> +static inline int >> +drm_fb_helper_generic_probe(struct drm_fb_helper *fb_helper, >> + struct drm_fb_helper_surface_size *sizes) >> +{ >> + return 0; >> +} > Ok, I think this patch looks ok. With the missing kerneldoc added (which > also explains the current limitations) this is > > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > >> + >> #endif >> >> static inline int >> -- >> 2.15.1 >> >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Fri, May 25, 2018 at 02:42:02PM +0200, Noralf Trønnes wrote: > > Den 24.05.2018 11.16, skrev Daniel Vetter: > > On Wed, May 23, 2018 at 04:34:07PM +0200, Noralf Trønnes wrote: > > > This is the first step in getting generic fbdev emulation. > > > A drm_fb_helper_funcs.fb_probe function is added which uses the > > > DRM client API to get a framebuffer backed by a dumb buffer. > > > > > > A transitional hack for tinydrm is needed in order to switch over all > > > CMA helper drivers in a later patch. This hack will be removed when > > > tinydrm moves to vmalloc buffers. > > > > > > Signed-off-by: Noralf Trønnes <noralf@tronnes.org> > > > --- > > > drivers/gpu/drm/drm_fb_helper.c | 164 ++++++++++++++++++++++++++++++++++++++++ > > > include/drm/drm_fb_helper.h | 26 +++++++ > > > 2 files changed, 190 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c > > > index 2ee1eaa66188..444c2b4040ea 100644 > > > --- a/drivers/gpu/drm/drm_fb_helper.c > > > +++ b/drivers/gpu/drm/drm_fb_helper.c > > > @@ -30,11 +30,13 @@ > > > #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > > > #include <linux/console.h> > > > +#include <linux/dma-buf.h> > > > #include <linux/kernel.h> > > > #include <linux/sysrq.h> > > > #include <linux/slab.h> > > > #include <linux/module.h> > > > #include <drm/drmP.h> > > > +#include <drm/drm_client.h> > > > #include <drm/drm_crtc.h> > > > #include <drm/drm_fb_helper.h> > > > #include <drm/drm_crtc_helper.h> > > > @@ -2928,6 +2930,168 @@ void drm_fb_helper_output_poll_changed(struct drm_device *dev) > > > } > > > EXPORT_SYMBOL(drm_fb_helper_output_poll_changed); > > > +/* @user: 1=userspace, 0=fbcon */ > > > +static int drm_fbdev_fb_open(struct fb_info *info, int user) > > > +{ > > > + struct drm_fb_helper *fb_helper = info->par; > > > + > > > + if (!try_module_get(fb_helper->dev->driver->fops->owner)) > > > + return -ENODEV; > > > + > > > + return 0; > > > +} > > > + > > > +static int drm_fbdev_fb_release(struct fb_info *info, int user) > > > +{ > > > + struct drm_fb_helper *fb_helper = info->par; > > > + > > > + module_put(fb_helper->dev->driver->fops->owner); > > > + > > > + return 0; > > > +} > > Hm, I thought earlier versions of your patch series had this separately, > > for everyone. What's the reasons for merging this into the fb_probe > > implementation. > > This is only necessary when struct fb_ops is defined in a library where > the owner field is the library module and not the driver module. > I realised that with this generic version it's highly unlikely that we > get another library that defines struct fb_ops, so it's only needed here. Hm, I'm still not 100% convinced we can fully subsume the tinydrm fbdev code with the generic one, due to the varios slight issues around defio tracking that we still have. But it's also easy to export this later on. If you add a comment explaining what you explained here to the commit message I think this is all fine with me as-is. -Daniel > > Noralf. > > > > + > > > +/* > > > + * fb_ops.fb_destroy is called by the last put_fb_info() call at the end of > > > + * unregister_framebuffer() or fb_release(). > > > + */ > > > +static void drm_fbdev_fb_destroy(struct fb_info *info) > > > +{ > > > + struct drm_fb_helper *fb_helper = info->par; > > > + struct fb_ops *fbops = NULL; > > > + > > > + DRM_DEBUG("\n"); > > > + > > > + if (fb_helper->fbdev->fbdefio) { > > > + fb_deferred_io_cleanup(fb_helper->fbdev); > > > + fbops = fb_helper->fbdev->fbops; > > > + } > > > + > > > + drm_fb_helper_fini(fb_helper); > > > + drm_client_framebuffer_delete(fb_helper->buffer); > > > + drm_client_free(fb_helper->client); > > > + kfree(fb_helper); > > > + kfree(fbops); > > > +} > > Hm, if we go with the idea that drm_clients could auto-unregister through > > a callback, then I expect this will lead to some control inversion. But we > > can fix that later on. > > > > > + > > > +static int drm_fbdev_fb_mmap(struct fb_info *info, struct vm_area_struct *vma) > > > +{ > > > + struct drm_fb_helper *fb_helper = info->par; > > > + > > > + if (fb_helper->dev->driver->gem_prime_mmap) > > > + return fb_helper->dev->driver->gem_prime_mmap(fb_helper->buffer->gem, vma); > > > + else > > > + return -ENODEV; > > > +} > > > + > > > +static struct fb_ops drm_fbdev_fb_ops = { > > > + /* No need to set owner, this module is already pinned by the driver. */ > > I'd still set it, means less thinking since more obviously correct. > > > > > + DRM_FB_HELPER_DEFAULT_OPS, > > > + .fb_open = drm_fbdev_fb_open, > > > + .fb_release = drm_fbdev_fb_release, > > > + .fb_destroy = drm_fbdev_fb_destroy, > > > + .fb_mmap = drm_fbdev_fb_mmap, > > > + .fb_read = drm_fb_helper_sys_read, > > > + .fb_write = drm_fb_helper_sys_write, > > > + .fb_fillrect = drm_fb_helper_sys_fillrect, > > > + .fb_copyarea = drm_fb_helper_sys_copyarea, > > > + .fb_imageblit = drm_fb_helper_sys_imageblit, > > Hm, some drivers require the cfb versions of these. In practice I guess > > there's not much of a difference really, at least on x86 and arm. > > > > We might want to document that though. > > > > > +}; > > > + > > > +static struct fb_deferred_io drm_fbdev_defio = { > > > + .delay = HZ / 20, > > > + .deferred_io = drm_fb_helper_deferred_io, > > > +}; > > > + > > > +/* > > > + * TODO: Remove this when tinydrm is converted to vmalloc buffers. > > > + */ > > > +static int drm_fbdev_cma_deferred_io_mmap(struct fb_info *info, > > > + struct vm_area_struct *vma) > > > +{ > > > + fb_deferred_io_mmap(info, vma); > > > + vma->vm_page_prot = pgprot_writecombine(vma->vm_page_prot); > > > + > > > + return 0; > > > +} > > > + > > Needs kerneldoc here. > > > > > +int drm_fb_helper_generic_probe(struct drm_fb_helper *fb_helper, > > > + struct drm_fb_helper_surface_size *sizes) > > > +{ > > > + struct drm_client_dev *client = fb_helper->client; > > > + struct drm_client_buffer *buffer; > > > + struct drm_framebuffer *fb; > > > + struct fb_info *fbi; > > > + u32 format; > > > + int ret; > > > + > > > + DRM_DEBUG_KMS("surface width(%d), height(%d) and bpp(%d)\n", > > > + sizes->surface_width, sizes->surface_height, > > > + sizes->surface_bpp); > > > + > > > + format = drm_mode_legacy_fb_format(sizes->surface_bpp, sizes->surface_depth); > > > + buffer = drm_client_framebuffer_create(client, sizes->surface_width, > > > + sizes->surface_height, format); > > > + if (IS_ERR(buffer)) > > > + return PTR_ERR(buffer); > > > + > > > + fb_helper->buffer = buffer; > > > + fb_helper->fb = buffer->fb; > > > + fb = buffer->fb; > > > + > > > + fbi = drm_fb_helper_alloc_fbi(fb_helper); > > > + if (IS_ERR(fbi)) { > > > + ret = PTR_ERR(fbi); > > > + goto err_free_buffer; > > > + } > > > + > > > + fbi->par = fb_helper; > > > + fbi->fbops = &drm_fbdev_fb_ops; > > > + fbi->screen_size = fb->height * fb->pitches[0]; > > > + fbi->fix.smem_len = fbi->screen_size; > > > + fbi->screen_buffer = buffer->vaddr; > > > + strcpy(fbi->fix.id, "DRM emulated"); > > > + > > > + drm_fb_helper_fill_fix(fbi, fb->pitches[0], fb->format->depth); > > > + drm_fb_helper_fill_var(fbi, fb_helper, sizes->fb_width, sizes->fb_height); > > > + > > > + if (fb->funcs->dirty) { > > > + struct fb_ops *fbops; > > > + > > > + /* > > > + * fb_deferred_io_cleanup() clears &fbops->fb_mmap so a per > > > + * instance version is necessary. > > > + */ > > > + fbops = kzalloc(sizeof(*fbops), GFP_KERNEL); > > > + if (!fbops) { > > > + ret = -ENOMEM; > > > + goto err_fb_info_destroy; > > > + } > > > + > > > + *fbops = *fbi->fbops; > > > + fbi->fbops = fbops; > > > + > > > + fbi->fbdefio = &drm_fbdev_defio; > > > + > > > + /* TODO: Remove this when tinydrm is converted to vmalloc buffers. */ > > > + fbi->fix.smem_start = page_to_phys(virt_to_page(buffer->vaddr)); > > > + > > > + fb_deferred_io_init(fbi); > > > + > > > + /* TODO: Remove this when tinydrm is converted to vmalloc buffers. */ > > > + fbi->fbops->fb_mmap = drm_fbdev_cma_deferred_io_mmap; > > > + } > > Ugh. Yeah defio and generic allocator through dumb buffers don't mix well. > > The only true generic solution for this would be to give userspace (and > > only userspace, for fbcon we can intercept everything) a staging buffer, > > and then upload things using the dirty callback. > > > > But that introduces another copy step, so isn't cool. > > > > I think a check for is_vmalloc_addr and if that's false, not doing any of > > the defio mmap setup would be good. Until we have a better idea. And yes > > that would need to be done after tinydrm is moved over. > > > > > + > > > + return 0; > > > + > > > +err_fb_info_destroy: > > > + drm_fb_helper_fini(fb_helper); > > > +err_free_buffer: > > > + drm_client_framebuffer_delete(buffer); > > > + > > > + return ret; > > > +} > > > +EXPORT_SYMBOL(drm_fb_helper_generic_probe); > > > + > > > /* The Kconfig DRM_KMS_HELPER selects FRAMEBUFFER_CONSOLE (if !EXPERT) > > > * but the module doesn't depend on any fb console symbols. At least > > > * attempt to load fbcon to avoid leaving the system without a usable console. > > > diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h > > > index b069433e7fc1..bb38469a9502 100644 > > > --- a/include/drm/drm_fb_helper.h > > > +++ b/include/drm/drm_fb_helper.h > > > @@ -30,6 +30,8 @@ > > > #ifndef DRM_FB_HELPER_H > > > #define DRM_FB_HELPER_H > > > +struct drm_client_buffer; > > > +struct drm_client_dev; > > > struct drm_fb_helper; > > > #include <drm/drm_crtc.h> > > > @@ -232,6 +234,20 @@ struct drm_fb_helper { > > > * See also: @deferred_setup > > > */ > > > int preferred_bpp; > > > + > > > + /** > > > + * @client: > > > + * > > > + * DRM client used by the generic fbdev emulation. > > > + */ > > > + struct drm_client_dev *client; > > > + > > > + /** > > > + * @buffer: > > > + * > > > + * Framebuffer used by the generic fbdev emulation. > > > + */ > > > + struct drm_client_buffer *buffer; > > > }; > > > /** > > > @@ -330,6 +346,9 @@ void drm_fb_helper_fbdev_teardown(struct drm_device *dev); > > > void drm_fb_helper_lastclose(struct drm_device *dev); > > > void drm_fb_helper_output_poll_changed(struct drm_device *dev); > > > + > > > +int drm_fb_helper_generic_probe(struct drm_fb_helper *fb_helper, > > > + struct drm_fb_helper_surface_size *sizes); > > > #else > > > static inline void drm_fb_helper_prepare(struct drm_device *dev, > > > struct drm_fb_helper *helper, > > > @@ -564,6 +583,13 @@ static inline void drm_fb_helper_output_poll_changed(struct drm_device *dev) > > > { > > > } > > > +static inline int > > > +drm_fb_helper_generic_probe(struct drm_fb_helper *fb_helper, > > > + struct drm_fb_helper_surface_size *sizes) > > > +{ > > > + return 0; > > > +} > > Ok, I think this patch looks ok. With the missing kerneldoc added (which > > also explains the current limitations) this is > > > > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > > > > + > > > #endif > > > static inline int > > > -- > > > 2.15.1 > > > > > > _______________________________________________ > > > dri-devel mailing list > > > dri-devel@lists.freedesktop.org > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel >
On Tue, May 29, 2018 at 9:54 AM Daniel Vetter <daniel@ffwll.ch> wrote: > > On Fri, May 25, 2018 at 02:42:02PM +0200, Noralf Trønnes wrote: > > > > Den 24.05.2018 11.16, skrev Daniel Vetter: > > > On Wed, May 23, 2018 at 04:34:07PM +0200, Noralf Trønnes wrote: > > > > This is the first step in getting generic fbdev emulation. > > > > A drm_fb_helper_funcs.fb_probe function is added which uses the > > > > DRM client API to get a framebuffer backed by a dumb buffer. > > > > > > > > A transitional hack for tinydrm is needed in order to switch over all > > > > CMA helper drivers in a later patch. This hack will be removed when > > > > tinydrm moves to vmalloc buffers. > > > > > > > > Signed-off-by: Noralf Trønnes <noralf@tronnes.org> > > > > --- > > > > drivers/gpu/drm/drm_fb_helper.c | 164 ++++++++++++++++++++++++++++++++++++++++ > > > > include/drm/drm_fb_helper.h | 26 +++++++ > > > > 2 files changed, 190 insertions(+) > > > > > > > > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c > > > > index 2ee1eaa66188..444c2b4040ea 100644 > > > > --- a/drivers/gpu/drm/drm_fb_helper.c > > > > +++ b/drivers/gpu/drm/drm_fb_helper.c > > > > @@ -30,11 +30,13 @@ > > > > #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > > > > #include <linux/console.h> > > > > +#include <linux/dma-buf.h> > > > > #include <linux/kernel.h> > > > > #include <linux/sysrq.h> > > > > #include <linux/slab.h> > > > > #include <linux/module.h> > > > > #include <drm/drmP.h> > > > > +#include <drm/drm_client.h> > > > > #include <drm/drm_crtc.h> > > > > #include <drm/drm_fb_helper.h> > > > > #include <drm/drm_crtc_helper.h> > > > > @@ -2928,6 +2930,168 @@ void drm_fb_helper_output_poll_changed(struct drm_device *dev) > > > > } > > > > EXPORT_SYMBOL(drm_fb_helper_output_poll_changed); > > > > +/* @user: 1=userspace, 0=fbcon */ > > > > +static int drm_fbdev_fb_open(struct fb_info *info, int user) > > > > +{ > > > > + struct drm_fb_helper *fb_helper = info->par; > > > > + > > > > + if (!try_module_get(fb_helper->dev->driver->fops->owner)) > > > > + return -ENODEV; > > > > + > > > > + return 0; > > > > +} > > > > + > > > > +static int drm_fbdev_fb_release(struct fb_info *info, int user) > > > > +{ > > > > + struct drm_fb_helper *fb_helper = info->par; > > > > + > > > > + module_put(fb_helper->dev->driver->fops->owner); > > > > + > > > > + return 0; > > > > +} > > > Hm, I thought earlier versions of your patch series had this separately, > > > for everyone. What's the reasons for merging this into the fb_probe > > > implementation. > > > > This is only necessary when struct fb_ops is defined in a library where > > the owner field is the library module and not the driver module. > > I realised that with this generic version it's highly unlikely that we > > get another library that defines struct fb_ops, so it's only needed here. > > Hm, I'm still not 100% convinced we can fully subsume the tinydrm fbdev > code with the generic one, due to the varios slight issues around defio > tracking that we still have. I have a new idea for 100% generic defio support in the fbdev helpers. Haven't tried it thought, but I think this could work around the problem if your mmap isn't struct page backed: - In the drm_fbdev_fb_mmap helper, if we need defio, change the default page prots to write-protected with something like this: vma->vm_page_prot = pgprot_wrprotect(vma->vm_page_prot); - After the driver's mmap function completed, copy the vm_ops structure and WARN_ON if it has an mkwrite and sync callback set. There's no real race here as long as we do all this before we return to userspace. - Set the mkwrite and fsync callbacks with similar implementions to the core fbdev defio stuff. These should all work on plain ptes, they don't actually require a struct page. uff. These should all work on plain ptes, they don't actually require a struct page. - We might also need to overwrite the vma_ops fault callback, just forwarding to the underlying vma_ops instead of copying them would be cleaner. The overhead won't matter, especially not for defio drivers. - Also copypaste all the other bits of the core fbdev defio code we'll need, that would allow us to also clean up the cleanup() warts ... All of this would ofc only be done if the fb has a ->dirty callback. We can also just stuff this into todo.rst. Cheers, Daniel > > But it's also easy to export this later on. If you add a comment > explaining what you explained here to the commit message I think this is > all fine with me as-is. > -Daniel > > > > > Noralf. > > > > > > + > > > > +/* > > > > + * fb_ops.fb_destroy is called by the last put_fb_info() call at the end of > > > > + * unregister_framebuffer() or fb_release(). > > > > + */ > > > > +static void drm_fbdev_fb_destroy(struct fb_info *info) > > > > +{ > > > > + struct drm_fb_helper *fb_helper = info->par; > > > > + struct fb_ops *fbops = NULL; > > > > + > > > > + DRM_DEBUG("\n"); > > > > + > > > > + if (fb_helper->fbdev->fbdefio) { > > > > + fb_deferred_io_cleanup(fb_helper->fbdev); > > > > + fbops = fb_helper->fbdev->fbops; > > > > + } > > > > + > > > > + drm_fb_helper_fini(fb_helper); > > > > + drm_client_framebuffer_delete(fb_helper->buffer); > > > > + drm_client_free(fb_helper->client); > > > > + kfree(fb_helper); > > > > + kfree(fbops); > > > > +} > > > Hm, if we go with the idea that drm_clients could auto-unregister through > > > a callback, then I expect this will lead to some control inversion. But we > > > can fix that later on. > > > > > > > + > > > > +static int drm_fbdev_fb_mmap(struct fb_info *info, struct vm_area_struct *vma) > > > > +{ > > > > + struct drm_fb_helper *fb_helper = info->par; > > > > + > > > > + if (fb_helper->dev->driver->gem_prime_mmap) > > > > + return fb_helper->dev->driver->gem_prime_mmap(fb_helper->buffer->gem, vma); > > > > + else > > > > + return -ENODEV; > > > > +} > > > > + > > > > +static struct fb_ops drm_fbdev_fb_ops = { > > > > + /* No need to set owner, this module is already pinned by the driver. */ > > > I'd still set it, means less thinking since more obviously correct. > > > > > > > + DRM_FB_HELPER_DEFAULT_OPS, > > > > + .fb_open = drm_fbdev_fb_open, > > > > + .fb_release = drm_fbdev_fb_release, > > > > + .fb_destroy = drm_fbdev_fb_destroy, > > > > + .fb_mmap = drm_fbdev_fb_mmap, > > > > + .fb_read = drm_fb_helper_sys_read, > > > > + .fb_write = drm_fb_helper_sys_write, > > > > + .fb_fillrect = drm_fb_helper_sys_fillrect, > > > > + .fb_copyarea = drm_fb_helper_sys_copyarea, > > > > + .fb_imageblit = drm_fb_helper_sys_imageblit, > > > Hm, some drivers require the cfb versions of these. In practice I guess > > > there's not much of a difference really, at least on x86 and arm. > > > > > > We might want to document that though. > > > > > > > +}; > > > > + > > > > +static struct fb_deferred_io drm_fbdev_defio = { > > > > + .delay = HZ / 20, > > > > + .deferred_io = drm_fb_helper_deferred_io, > > > > +}; > > > > + > > > > +/* > > > > + * TODO: Remove this when tinydrm is converted to vmalloc buffers. > > > > + */ > > > > +static int drm_fbdev_cma_deferred_io_mmap(struct fb_info *info, > > > > + struct vm_area_struct *vma) > > > > +{ > > > > + fb_deferred_io_mmap(info, vma); > > > > + vma->vm_page_prot = pgprot_writecombine(vma->vm_page_prot); > > > > + > > > > + return 0; > > > > +} > > > > + > > > Needs kerneldoc here. > > > > > > > +int drm_fb_helper_generic_probe(struct drm_fb_helper *fb_helper, > > > > + struct drm_fb_helper_surface_size *sizes) > > > > +{ > > > > + struct drm_client_dev *client = fb_helper->client; > > > > + struct drm_client_buffer *buffer; > > > > + struct drm_framebuffer *fb; > > > > + struct fb_info *fbi; > > > > + u32 format; > > > > + int ret; > > > > + > > > > + DRM_DEBUG_KMS("surface width(%d), height(%d) and bpp(%d)\n", > > > > + sizes->surface_width, sizes->surface_height, > > > > + sizes->surface_bpp); > > > > + > > > > + format = drm_mode_legacy_fb_format(sizes->surface_bpp, sizes->surface_depth); > > > > + buffer = drm_client_framebuffer_create(client, sizes->surface_width, > > > > + sizes->surface_height, format); > > > > + if (IS_ERR(buffer)) > > > > + return PTR_ERR(buffer); > > > > + > > > > + fb_helper->buffer = buffer; > > > > + fb_helper->fb = buffer->fb; > > > > + fb = buffer->fb; > > > > + > > > > + fbi = drm_fb_helper_alloc_fbi(fb_helper); > > > > + if (IS_ERR(fbi)) { > > > > + ret = PTR_ERR(fbi); > > > > + goto err_free_buffer; > > > > + } > > > > + > > > > + fbi->par = fb_helper; > > > > + fbi->fbops = &drm_fbdev_fb_ops; > > > > + fbi->screen_size = fb->height * fb->pitches[0]; > > > > + fbi->fix.smem_len = fbi->screen_size; > > > > + fbi->screen_buffer = buffer->vaddr; > > > > + strcpy(fbi->fix.id, "DRM emulated"); > > > > + > > > > + drm_fb_helper_fill_fix(fbi, fb->pitches[0], fb->format->depth); > > > > + drm_fb_helper_fill_var(fbi, fb_helper, sizes->fb_width, sizes->fb_height); > > > > + > > > > + if (fb->funcs->dirty) { > > > > + struct fb_ops *fbops; > > > > + > > > > + /* > > > > + * fb_deferred_io_cleanup() clears &fbops->fb_mmap so a per > > > > + * instance version is necessary. > > > > + */ > > > > + fbops = kzalloc(sizeof(*fbops), GFP_KERNEL); > > > > + if (!fbops) { > > > > + ret = -ENOMEM; > > > > + goto err_fb_info_destroy; > > > > + } > > > > + > > > > + *fbops = *fbi->fbops; > > > > + fbi->fbops = fbops; > > > > + > > > > + fbi->fbdefio = &drm_fbdev_defio; > > > > + > > > > + /* TODO: Remove this when tinydrm is converted to vmalloc buffers. */ > > > > + fbi->fix.smem_start = page_to_phys(virt_to_page(buffer->vaddr)); > > > > + > > > > + fb_deferred_io_init(fbi); > > > > + > > > > + /* TODO: Remove this when tinydrm is converted to vmalloc buffers. */ > > > > + fbi->fbops->fb_mmap = drm_fbdev_cma_deferred_io_mmap; > > > > + } > > > Ugh. Yeah defio and generic allocator through dumb buffers don't mix well. > > > The only true generic solution for this would be to give userspace (and > > > only userspace, for fbcon we can intercept everything) a staging buffer, > > > and then upload things using the dirty callback. > > > > > > But that introduces another copy step, so isn't cool. > > > > > > I think a check for is_vmalloc_addr and if that's false, not doing any of > > > the defio mmap setup would be good. Until we have a better idea. And yes > > > that would need to be done after tinydrm is moved over. > > > > > > > + > > > > + return 0; > > > > + > > > > +err_fb_info_destroy: > > > > + drm_fb_helper_fini(fb_helper); > > > > +err_free_buffer: > > > > + drm_client_framebuffer_delete(buffer); > > > > + > > > > + return ret; > > > > +} > > > > +EXPORT_SYMBOL(drm_fb_helper_generic_probe); > > > > + > > > > /* The Kconfig DRM_KMS_HELPER selects FRAMEBUFFER_CONSOLE (if !EXPERT) > > > > * but the module doesn't depend on any fb console symbols. At least > > > > * attempt to load fbcon to avoid leaving the system without a usable console. > > > > diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h > > > > index b069433e7fc1..bb38469a9502 100644 > > > > --- a/include/drm/drm_fb_helper.h > > > > +++ b/include/drm/drm_fb_helper.h > > > > @@ -30,6 +30,8 @@ > > > > #ifndef DRM_FB_HELPER_H > > > > #define DRM_FB_HELPER_H > > > > +struct drm_client_buffer; > > > > +struct drm_client_dev; > > > > struct drm_fb_helper; > > > > #include <drm/drm_crtc.h> > > > > @@ -232,6 +234,20 @@ struct drm_fb_helper { > > > > * See also: @deferred_setup > > > > */ > > > > int preferred_bpp; > > > > + > > > > + /** > > > > + * @client: > > > > + * > > > > + * DRM client used by the generic fbdev emulation. > > > > + */ > > > > + struct drm_client_dev *client; > > > > + > > > > + /** > > > > + * @buffer: > > > > + * > > > > + * Framebuffer used by the generic fbdev emulation. > > > > + */ > > > > + struct drm_client_buffer *buffer; > > > > }; > > > > /** > > > > @@ -330,6 +346,9 @@ void drm_fb_helper_fbdev_teardown(struct drm_device *dev); > > > > void drm_fb_helper_lastclose(struct drm_device *dev); > > > > void drm_fb_helper_output_poll_changed(struct drm_device *dev); > > > > + > > > > +int drm_fb_helper_generic_probe(struct drm_fb_helper *fb_helper, > > > > + struct drm_fb_helper_surface_size *sizes); > > > > #else > > > > static inline void drm_fb_helper_prepare(struct drm_device *dev, > > > > struct drm_fb_helper *helper, > > > > @@ -564,6 +583,13 @@ static inline void drm_fb_helper_output_poll_changed(struct drm_device *dev) > > > > { > > > > } > > > > +static inline int > > > > +drm_fb_helper_generic_probe(struct drm_fb_helper *fb_helper, > > > > + struct drm_fb_helper_surface_size *sizes) > > > > +{ > > > > + return 0; > > > > +} > > > Ok, I think this patch looks ok. With the missing kerneldoc added (which > > > also explains the current limitations) this is > > > > > > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > > > > > > + > > > > #endif > > > > static inline int > > > > -- > > > > 2.15.1 > > > > > > > > _______________________________________________ > > > > dri-devel mailing list > > > > dri-devel@lists.freedesktop.org > > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel > > > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
Den 28.12.2018 21.38, skrev Daniel Vetter: > On Tue, May 29, 2018 at 9:54 AM Daniel Vetter <daniel@ffwll.ch> wrote: >> >> On Fri, May 25, 2018 at 02:42:02PM +0200, Noralf Trønnes wrote: >>> >>> Den 24.05.2018 11.16, skrev Daniel Vetter: >>>> On Wed, May 23, 2018 at 04:34:07PM +0200, Noralf Trønnes wrote: >>>>> This is the first step in getting generic fbdev emulation. >>>>> A drm_fb_helper_funcs.fb_probe function is added which uses the >>>>> DRM client API to get a framebuffer backed by a dumb buffer. >>>>> >>>>> A transitional hack for tinydrm is needed in order to switch over all >>>>> CMA helper drivers in a later patch. This hack will be removed when >>>>> tinydrm moves to vmalloc buffers. >>>>> >>>>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org> >>>>> --- >>>>> drivers/gpu/drm/drm_fb_helper.c | 164 ++++++++++++++++++++++++++++++++++++++++ >>>>> include/drm/drm_fb_helper.h | 26 +++++++ >>>>> 2 files changed, 190 insertions(+) >>>>> >>>>> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c >>>>> index 2ee1eaa66188..444c2b4040ea 100644 >>>>> --- a/drivers/gpu/drm/drm_fb_helper.c >>>>> +++ b/drivers/gpu/drm/drm_fb_helper.c >>>>> @@ -30,11 +30,13 @@ >>>>> #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt >>>>> #include <linux/console.h> >>>>> +#include <linux/dma-buf.h> >>>>> #include <linux/kernel.h> >>>>> #include <linux/sysrq.h> >>>>> #include <linux/slab.h> >>>>> #include <linux/module.h> >>>>> #include <drm/drmP.h> >>>>> +#include <drm/drm_client.h> >>>>> #include <drm/drm_crtc.h> >>>>> #include <drm/drm_fb_helper.h> >>>>> #include <drm/drm_crtc_helper.h> >>>>> @@ -2928,6 +2930,168 @@ void drm_fb_helper_output_poll_changed(struct drm_device *dev) >>>>> } >>>>> EXPORT_SYMBOL(drm_fb_helper_output_poll_changed); >>>>> +/* @user: 1=userspace, 0=fbcon */ >>>>> +static int drm_fbdev_fb_open(struct fb_info *info, int user) >>>>> +{ >>>>> + struct drm_fb_helper *fb_helper = info->par; >>>>> + >>>>> + if (!try_module_get(fb_helper->dev->driver->fops->owner)) >>>>> + return -ENODEV; >>>>> + >>>>> + return 0; >>>>> +} >>>>> + >>>>> +static int drm_fbdev_fb_release(struct fb_info *info, int user) >>>>> +{ >>>>> + struct drm_fb_helper *fb_helper = info->par; >>>>> + >>>>> + module_put(fb_helper->dev->driver->fops->owner); >>>>> + >>>>> + return 0; >>>>> +} >>>> Hm, I thought earlier versions of your patch series had this separately, >>>> for everyone. What's the reasons for merging this into the fb_probe >>>> implementation. >>> >>> This is only necessary when struct fb_ops is defined in a library where >>> the owner field is the library module and not the driver module. >>> I realised that with this generic version it's highly unlikely that we >>> get another library that defines struct fb_ops, so it's only needed here. >> >> Hm, I'm still not 100% convinced we can fully subsume the tinydrm fbdev >> code with the generic one, due to the varios slight issues around defio >> tracking that we still have. > > I have a new idea for 100% generic defio support in the fbdev helpers. > Haven't tried it thought, but I think this could work around the > problem if your mmap isn't struct page backed: > > - In the drm_fbdev_fb_mmap helper, if we need defio, change the > default page prots to write-protected with something like this: > > vma->vm_page_prot = pgprot_wrprotect(vma->vm_page_prot); > > - After the driver's mmap function completed, copy the vm_ops > structure and WARN_ON if it has an mkwrite and sync callback set. > There's no real race here as long as we do all this before we return > to userspace. > > - Set the mkwrite and fsync callbacks with similar implementions to > the core fbdev defio stuff. These should all work on plain ptes, they > don't actually require a struct page. > uff. These should all work on plain ptes, they don't actually require > a struct page. > > - We might also need to overwrite the vma_ops fault callback, just > forwarding to the underlying vma_ops instead of copying them would be > cleaner. The overhead won't matter, especially not for defio drivers. > > - Also copypaste all the other bits of the core fbdev defio code we'll > need, that would allow us to also clean up the cleanup() warts ... > > All of this would ofc only be done if the fb has a ->dirty callback. > > We can also just stuff this into todo.rst. > Hmm, do you think it's worth spending more time on fbdev? The point would be to speed it up, right? Avoid the blitting. My hope is that when I'm done with DRM client, David Herrmann will pick up and finish his DRM/KMS console. At that point fbdev is only needed for legacy userspace. I suck at estimating how long it takes to do stuff, but I really hope I'm done with DRM client by the end of this year. Noralf. > Cheers, Daniel > > >> >> But it's also easy to export this later on. If you add a comment >> explaining what you explained here to the commit message I think this is >> all fine with me as-is. >> -Daniel >> >>> >>> Noralf. >>> >>>>> + >>>>> +/* >>>>> + * fb_ops.fb_destroy is called by the last put_fb_info() call at the end of >>>>> + * unregister_framebuffer() or fb_release(). >>>>> + */ >>>>> +static void drm_fbdev_fb_destroy(struct fb_info *info) >>>>> +{ >>>>> + struct drm_fb_helper *fb_helper = info->par; >>>>> + struct fb_ops *fbops = NULL; >>>>> + >>>>> + DRM_DEBUG("\n"); >>>>> + >>>>> + if (fb_helper->fbdev->fbdefio) { >>>>> + fb_deferred_io_cleanup(fb_helper->fbdev); >>>>> + fbops = fb_helper->fbdev->fbops; >>>>> + } >>>>> + >>>>> + drm_fb_helper_fini(fb_helper); >>>>> + drm_client_framebuffer_delete(fb_helper->buffer); >>>>> + drm_client_free(fb_helper->client); >>>>> + kfree(fb_helper); >>>>> + kfree(fbops); >>>>> +} >>>> Hm, if we go with the idea that drm_clients could auto-unregister through >>>> a callback, then I expect this will lead to some control inversion. But we >>>> can fix that later on. >>>> >>>>> + >>>>> +static int drm_fbdev_fb_mmap(struct fb_info *info, struct vm_area_struct *vma) >>>>> +{ >>>>> + struct drm_fb_helper *fb_helper = info->par; >>>>> + >>>>> + if (fb_helper->dev->driver->gem_prime_mmap) >>>>> + return fb_helper->dev->driver->gem_prime_mmap(fb_helper->buffer->gem, vma); >>>>> + else >>>>> + return -ENODEV; >>>>> +} >>>>> + >>>>> +static struct fb_ops drm_fbdev_fb_ops = { >>>>> + /* No need to set owner, this module is already pinned by the driver. */ >>>> I'd still set it, means less thinking since more obviously correct. >>>> >>>>> + DRM_FB_HELPER_DEFAULT_OPS, >>>>> + .fb_open = drm_fbdev_fb_open, >>>>> + .fb_release = drm_fbdev_fb_release, >>>>> + .fb_destroy = drm_fbdev_fb_destroy, >>>>> + .fb_mmap = drm_fbdev_fb_mmap, >>>>> + .fb_read = drm_fb_helper_sys_read, >>>>> + .fb_write = drm_fb_helper_sys_write, >>>>> + .fb_fillrect = drm_fb_helper_sys_fillrect, >>>>> + .fb_copyarea = drm_fb_helper_sys_copyarea, >>>>> + .fb_imageblit = drm_fb_helper_sys_imageblit, >>>> Hm, some drivers require the cfb versions of these. In practice I guess >>>> there's not much of a difference really, at least on x86 and arm. >>>> >>>> We might want to document that though. >>>> >>>>> +}; >>>>> + >>>>> +static struct fb_deferred_io drm_fbdev_defio = { >>>>> + .delay = HZ / 20, >>>>> + .deferred_io = drm_fb_helper_deferred_io, >>>>> +}; >>>>> + >>>>> +/* >>>>> + * TODO: Remove this when tinydrm is converted to vmalloc buffers. >>>>> + */ >>>>> +static int drm_fbdev_cma_deferred_io_mmap(struct fb_info *info, >>>>> + struct vm_area_struct *vma) >>>>> +{ >>>>> + fb_deferred_io_mmap(info, vma); >>>>> + vma->vm_page_prot = pgprot_writecombine(vma->vm_page_prot); >>>>> + >>>>> + return 0; >>>>> +} >>>>> + >>>> Needs kerneldoc here. >>>> >>>>> +int drm_fb_helper_generic_probe(struct drm_fb_helper *fb_helper, >>>>> + struct drm_fb_helper_surface_size *sizes) >>>>> +{ >>>>> + struct drm_client_dev *client = fb_helper->client; >>>>> + struct drm_client_buffer *buffer; >>>>> + struct drm_framebuffer *fb; >>>>> + struct fb_info *fbi; >>>>> + u32 format; >>>>> + int ret; >>>>> + >>>>> + DRM_DEBUG_KMS("surface width(%d), height(%d) and bpp(%d)\n", >>>>> + sizes->surface_width, sizes->surface_height, >>>>> + sizes->surface_bpp); >>>>> + >>>>> + format = drm_mode_legacy_fb_format(sizes->surface_bpp, sizes->surface_depth); >>>>> + buffer = drm_client_framebuffer_create(client, sizes->surface_width, >>>>> + sizes->surface_height, format); >>>>> + if (IS_ERR(buffer)) >>>>> + return PTR_ERR(buffer); >>>>> + >>>>> + fb_helper->buffer = buffer; >>>>> + fb_helper->fb = buffer->fb; >>>>> + fb = buffer->fb; >>>>> + >>>>> + fbi = drm_fb_helper_alloc_fbi(fb_helper); >>>>> + if (IS_ERR(fbi)) { >>>>> + ret = PTR_ERR(fbi); >>>>> + goto err_free_buffer; >>>>> + } >>>>> + >>>>> + fbi->par = fb_helper; >>>>> + fbi->fbops = &drm_fbdev_fb_ops; >>>>> + fbi->screen_size = fb->height * fb->pitches[0]; >>>>> + fbi->fix.smem_len = fbi->screen_size; >>>>> + fbi->screen_buffer = buffer->vaddr; >>>>> + strcpy(fbi->fix.id, "DRM emulated"); >>>>> + >>>>> + drm_fb_helper_fill_fix(fbi, fb->pitches[0], fb->format->depth); >>>>> + drm_fb_helper_fill_var(fbi, fb_helper, sizes->fb_width, sizes->fb_height); >>>>> + >>>>> + if (fb->funcs->dirty) { >>>>> + struct fb_ops *fbops; >>>>> + >>>>> + /* >>>>> + * fb_deferred_io_cleanup() clears &fbops->fb_mmap so a per >>>>> + * instance version is necessary. >>>>> + */ >>>>> + fbops = kzalloc(sizeof(*fbops), GFP_KERNEL); >>>>> + if (!fbops) { >>>>> + ret = -ENOMEM; >>>>> + goto err_fb_info_destroy; >>>>> + } >>>>> + >>>>> + *fbops = *fbi->fbops; >>>>> + fbi->fbops = fbops; >>>>> + >>>>> + fbi->fbdefio = &drm_fbdev_defio; >>>>> + >>>>> + /* TODO: Remove this when tinydrm is converted to vmalloc buffers. */ >>>>> + fbi->fix.smem_start = page_to_phys(virt_to_page(buffer->vaddr)); >>>>> + >>>>> + fb_deferred_io_init(fbi); >>>>> + >>>>> + /* TODO: Remove this when tinydrm is converted to vmalloc buffers. */ >>>>> + fbi->fbops->fb_mmap = drm_fbdev_cma_deferred_io_mmap; >>>>> + } >>>> Ugh. Yeah defio and generic allocator through dumb buffers don't mix well. >>>> The only true generic solution for this would be to give userspace (and >>>> only userspace, for fbcon we can intercept everything) a staging buffer, >>>> and then upload things using the dirty callback. >>>> >>>> But that introduces another copy step, so isn't cool. >>>> >>>> I think a check for is_vmalloc_addr and if that's false, not doing any of >>>> the defio mmap setup would be good. Until we have a better idea. And yes >>>> that would need to be done after tinydrm is moved over. >>>> >>>>> + >>>>> + return 0; >>>>> + >>>>> +err_fb_info_destroy: >>>>> + drm_fb_helper_fini(fb_helper); >>>>> +err_free_buffer: >>>>> + drm_client_framebuffer_delete(buffer); >>>>> + >>>>> + return ret; >>>>> +} >>>>> +EXPORT_SYMBOL(drm_fb_helper_generic_probe); >>>>> + >>>>> /* The Kconfig DRM_KMS_HELPER selects FRAMEBUFFER_CONSOLE (if !EXPERT) >>>>> * but the module doesn't depend on any fb console symbols. At least >>>>> * attempt to load fbcon to avoid leaving the system without a usable console. >>>>> diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h >>>>> index b069433e7fc1..bb38469a9502 100644 >>>>> --- a/include/drm/drm_fb_helper.h >>>>> +++ b/include/drm/drm_fb_helper.h >>>>> @@ -30,6 +30,8 @@ >>>>> #ifndef DRM_FB_HELPER_H >>>>> #define DRM_FB_HELPER_H >>>>> +struct drm_client_buffer; >>>>> +struct drm_client_dev; >>>>> struct drm_fb_helper; >>>>> #include <drm/drm_crtc.h> >>>>> @@ -232,6 +234,20 @@ struct drm_fb_helper { >>>>> * See also: @deferred_setup >>>>> */ >>>>> int preferred_bpp; >>>>> + >>>>> + /** >>>>> + * @client: >>>>> + * >>>>> + * DRM client used by the generic fbdev emulation. >>>>> + */ >>>>> + struct drm_client_dev *client; >>>>> + >>>>> + /** >>>>> + * @buffer: >>>>> + * >>>>> + * Framebuffer used by the generic fbdev emulation. >>>>> + */ >>>>> + struct drm_client_buffer *buffer; >>>>> }; >>>>> /** >>>>> @@ -330,6 +346,9 @@ void drm_fb_helper_fbdev_teardown(struct drm_device *dev); >>>>> void drm_fb_helper_lastclose(struct drm_device *dev); >>>>> void drm_fb_helper_output_poll_changed(struct drm_device *dev); >>>>> + >>>>> +int drm_fb_helper_generic_probe(struct drm_fb_helper *fb_helper, >>>>> + struct drm_fb_helper_surface_size *sizes); >>>>> #else >>>>> static inline void drm_fb_helper_prepare(struct drm_device *dev, >>>>> struct drm_fb_helper *helper, >>>>> @@ -564,6 +583,13 @@ static inline void drm_fb_helper_output_poll_changed(struct drm_device *dev) >>>>> { >>>>> } >>>>> +static inline int >>>>> +drm_fb_helper_generic_probe(struct drm_fb_helper *fb_helper, >>>>> + struct drm_fb_helper_surface_size *sizes) >>>>> +{ >>>>> + return 0; >>>>> +} >>>> Ok, I think this patch looks ok. With the missing kerneldoc added (which >>>> also explains the current limitations) this is >>>> >>>> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> >>>> >>>>> + >>>>> #endif >>>>> static inline int >>>>> -- >>>>> 2.15.1 >>>>> >>>>> _______________________________________________ >>>>> dri-devel mailing list >>>>> dri-devel@lists.freedesktop.org >>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel >>> >> >> -- >> Daniel Vetter >> Software Engineer, Intel Corporation >> http://blog.ffwll.ch > > >
On Thu, Jan 03, 2019 at 06:06:53PM +0100, Noralf Trønnes wrote: > > > Den 28.12.2018 21.38, skrev Daniel Vetter: > > On Tue, May 29, 2018 at 9:54 AM Daniel Vetter <daniel@ffwll.ch> wrote: > > > > > > On Fri, May 25, 2018 at 02:42:02PM +0200, Noralf Trønnes wrote: > > > > > > > > Den 24.05.2018 11.16, skrev Daniel Vetter: > > > > > On Wed, May 23, 2018 at 04:34:07PM +0200, Noralf Trønnes wrote: > > > > > > This is the first step in getting generic fbdev emulation. > > > > > > A drm_fb_helper_funcs.fb_probe function is added which uses the > > > > > > DRM client API to get a framebuffer backed by a dumb buffer. > > > > > > > > > > > > A transitional hack for tinydrm is needed in order to switch over all > > > > > > CMA helper drivers in a later patch. This hack will be removed when > > > > > > tinydrm moves to vmalloc buffers. > > > > > > > > > > > > Signed-off-by: Noralf Trønnes <noralf@tronnes.org> > > > > > > --- > > > > > > drivers/gpu/drm/drm_fb_helper.c | 164 ++++++++++++++++++++++++++++++++++++++++ > > > > > > include/drm/drm_fb_helper.h | 26 +++++++ > > > > > > 2 files changed, 190 insertions(+) > > > > > > > > > > > > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c > > > > > > index 2ee1eaa66188..444c2b4040ea 100644 > > > > > > --- a/drivers/gpu/drm/drm_fb_helper.c > > > > > > +++ b/drivers/gpu/drm/drm_fb_helper.c > > > > > > @@ -30,11 +30,13 @@ > > > > > > #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > > > > > > #include <linux/console.h> > > > > > > +#include <linux/dma-buf.h> > > > > > > #include <linux/kernel.h> > > > > > > #include <linux/sysrq.h> > > > > > > #include <linux/slab.h> > > > > > > #include <linux/module.h> > > > > > > #include <drm/drmP.h> > > > > > > +#include <drm/drm_client.h> > > > > > > #include <drm/drm_crtc.h> > > > > > > #include <drm/drm_fb_helper.h> > > > > > > #include <drm/drm_crtc_helper.h> > > > > > > @@ -2928,6 +2930,168 @@ void drm_fb_helper_output_poll_changed(struct drm_device *dev) > > > > > > } > > > > > > EXPORT_SYMBOL(drm_fb_helper_output_poll_changed); > > > > > > +/* @user: 1=userspace, 0=fbcon */ > > > > > > +static int drm_fbdev_fb_open(struct fb_info *info, int user) > > > > > > +{ > > > > > > + struct drm_fb_helper *fb_helper = info->par; > > > > > > + > > > > > > + if (!try_module_get(fb_helper->dev->driver->fops->owner)) > > > > > > + return -ENODEV; > > > > > > + > > > > > > + return 0; > > > > > > +} > > > > > > + > > > > > > +static int drm_fbdev_fb_release(struct fb_info *info, int user) > > > > > > +{ > > > > > > + struct drm_fb_helper *fb_helper = info->par; > > > > > > + > > > > > > + module_put(fb_helper->dev->driver->fops->owner); > > > > > > + > > > > > > + return 0; > > > > > > +} > > > > > Hm, I thought earlier versions of your patch series had this separately, > > > > > for everyone. What's the reasons for merging this into the fb_probe > > > > > implementation. > > > > > > > > This is only necessary when struct fb_ops is defined in a library where > > > > the owner field is the library module and not the driver module. > > > > I realised that with this generic version it's highly unlikely that we > > > > get another library that defines struct fb_ops, so it's only needed here. > > > > > > Hm, I'm still not 100% convinced we can fully subsume the tinydrm fbdev > > > code with the generic one, due to the varios slight issues around defio > > > tracking that we still have. > > > > I have a new idea for 100% generic defio support in the fbdev helpers. > > Haven't tried it thought, but I think this could work around the > > problem if your mmap isn't struct page backed: > > > > - In the drm_fbdev_fb_mmap helper, if we need defio, change the > > default page prots to write-protected with something like this: > > > > vma->vm_page_prot = pgprot_wrprotect(vma->vm_page_prot); > > > > - After the driver's mmap function completed, copy the vm_ops > > structure and WARN_ON if it has an mkwrite and sync callback set. > > There's no real race here as long as we do all this before we return > > to userspace. > > > > - Set the mkwrite and fsync callbacks with similar implementions to > > the core fbdev defio stuff. These should all work on plain ptes, they > > don't actually require a struct page. > > uff. These should all work on plain ptes, they don't actually require > > a struct page. > > > > - We might also need to overwrite the vma_ops fault callback, just > > forwarding to the underlying vma_ops instead of copying them would be > > cleaner. The overhead won't matter, especially not for defio drivers. > > > > - Also copypaste all the other bits of the core fbdev defio code we'll > > need, that would allow us to also clean up the cleanup() warts ... > > > > All of this would ofc only be done if the fb has a ->dirty callback. > > > > We can also just stuff this into todo.rst. > > > > Hmm, do you think it's worth spending more time on fbdev? > The point would be to speed it up, right? Avoid the blitting. > > My hope is that when I'm done with DRM client, David Herrmann will pick > up and finish his DRM/KMS console. At that point fbdev is only needed > for legacy userspace. This is only about the legacy mmap stuff, fbcon doesn't use that. And the current defio fbdev mmap stuff is fairly horrible and very driver-specific. Doing the above would be a neat cleanup I think, since it avoids that new drivers have to care about fbdev. Aside: Not sure David is still interested in the drm console stuff. I haven't chatted with him since ages about this at least ... > I suck at estimating how long it takes to do stuff, but I really hope I'm > done with DRM client by the end of this year. It's going to be a sizeable chunk of work I think, and might not work. Was really just an idea, I think better to stuff it into todo.rst. I'll do a patch. Cheers, Daniel > > Noralf. > > > Cheers, Daniel > > > > > > > > > > But it's also easy to export this later on. If you add a comment > > > explaining what you explained here to the commit message I think this is > > > all fine with me as-is. > > > -Daniel > > > > > > > > > > > Noralf. > > > > > > > > > > + > > > > > > +/* > > > > > > + * fb_ops.fb_destroy is called by the last put_fb_info() call at the end of > > > > > > + * unregister_framebuffer() or fb_release(). > > > > > > + */ > > > > > > +static void drm_fbdev_fb_destroy(struct fb_info *info) > > > > > > +{ > > > > > > + struct drm_fb_helper *fb_helper = info->par; > > > > > > + struct fb_ops *fbops = NULL; > > > > > > + > > > > > > + DRM_DEBUG("\n"); > > > > > > + > > > > > > + if (fb_helper->fbdev->fbdefio) { > > > > > > + fb_deferred_io_cleanup(fb_helper->fbdev); > > > > > > + fbops = fb_helper->fbdev->fbops; > > > > > > + } > > > > > > + > > > > > > + drm_fb_helper_fini(fb_helper); > > > > > > + drm_client_framebuffer_delete(fb_helper->buffer); > > > > > > + drm_client_free(fb_helper->client); > > > > > > + kfree(fb_helper); > > > > > > + kfree(fbops); > > > > > > +} > > > > > Hm, if we go with the idea that drm_clients could auto-unregister through > > > > > a callback, then I expect this will lead to some control inversion. But we > > > > > can fix that later on. > > > > > > > > > > > + > > > > > > +static int drm_fbdev_fb_mmap(struct fb_info *info, struct vm_area_struct *vma) > > > > > > +{ > > > > > > + struct drm_fb_helper *fb_helper = info->par; > > > > > > + > > > > > > + if (fb_helper->dev->driver->gem_prime_mmap) > > > > > > + return fb_helper->dev->driver->gem_prime_mmap(fb_helper->buffer->gem, vma); > > > > > > + else > > > > > > + return -ENODEV; > > > > > > +} > > > > > > + > > > > > > +static struct fb_ops drm_fbdev_fb_ops = { > > > > > > + /* No need to set owner, this module is already pinned by the driver. */ > > > > > I'd still set it, means less thinking since more obviously correct. > > > > > > > > > > > + DRM_FB_HELPER_DEFAULT_OPS, > > > > > > + .fb_open = drm_fbdev_fb_open, > > > > > > + .fb_release = drm_fbdev_fb_release, > > > > > > + .fb_destroy = drm_fbdev_fb_destroy, > > > > > > + .fb_mmap = drm_fbdev_fb_mmap, > > > > > > + .fb_read = drm_fb_helper_sys_read, > > > > > > + .fb_write = drm_fb_helper_sys_write, > > > > > > + .fb_fillrect = drm_fb_helper_sys_fillrect, > > > > > > + .fb_copyarea = drm_fb_helper_sys_copyarea, > > > > > > + .fb_imageblit = drm_fb_helper_sys_imageblit, > > > > > Hm, some drivers require the cfb versions of these. In practice I guess > > > > > there's not much of a difference really, at least on x86 and arm. > > > > > > > > > > We might want to document that though. > > > > > > > > > > > +}; > > > > > > + > > > > > > +static struct fb_deferred_io drm_fbdev_defio = { > > > > > > + .delay = HZ / 20, > > > > > > + .deferred_io = drm_fb_helper_deferred_io, > > > > > > +}; > > > > > > + > > > > > > +/* > > > > > > + * TODO: Remove this when tinydrm is converted to vmalloc buffers. > > > > > > + */ > > > > > > +static int drm_fbdev_cma_deferred_io_mmap(struct fb_info *info, > > > > > > + struct vm_area_struct *vma) > > > > > > +{ > > > > > > + fb_deferred_io_mmap(info, vma); > > > > > > + vma->vm_page_prot = pgprot_writecombine(vma->vm_page_prot); > > > > > > + > > > > > > + return 0; > > > > > > +} > > > > > > + > > > > > Needs kerneldoc here. > > > > > > > > > > > +int drm_fb_helper_generic_probe(struct drm_fb_helper *fb_helper, > > > > > > + struct drm_fb_helper_surface_size *sizes) > > > > > > +{ > > > > > > + struct drm_client_dev *client = fb_helper->client; > > > > > > + struct drm_client_buffer *buffer; > > > > > > + struct drm_framebuffer *fb; > > > > > > + struct fb_info *fbi; > > > > > > + u32 format; > > > > > > + int ret; > > > > > > + > > > > > > + DRM_DEBUG_KMS("surface width(%d), height(%d) and bpp(%d)\n", > > > > > > + sizes->surface_width, sizes->surface_height, > > > > > > + sizes->surface_bpp); > > > > > > + > > > > > > + format = drm_mode_legacy_fb_format(sizes->surface_bpp, sizes->surface_depth); > > > > > > + buffer = drm_client_framebuffer_create(client, sizes->surface_width, > > > > > > + sizes->surface_height, format); > > > > > > + if (IS_ERR(buffer)) > > > > > > + return PTR_ERR(buffer); > > > > > > + > > > > > > + fb_helper->buffer = buffer; > > > > > > + fb_helper->fb = buffer->fb; > > > > > > + fb = buffer->fb; > > > > > > + > > > > > > + fbi = drm_fb_helper_alloc_fbi(fb_helper); > > > > > > + if (IS_ERR(fbi)) { > > > > > > + ret = PTR_ERR(fbi); > > > > > > + goto err_free_buffer; > > > > > > + } > > > > > > + > > > > > > + fbi->par = fb_helper; > > > > > > + fbi->fbops = &drm_fbdev_fb_ops; > > > > > > + fbi->screen_size = fb->height * fb->pitches[0]; > > > > > > + fbi->fix.smem_len = fbi->screen_size; > > > > > > + fbi->screen_buffer = buffer->vaddr; > > > > > > + strcpy(fbi->fix.id, "DRM emulated"); > > > > > > + > > > > > > + drm_fb_helper_fill_fix(fbi, fb->pitches[0], fb->format->depth); > > > > > > + drm_fb_helper_fill_var(fbi, fb_helper, sizes->fb_width, sizes->fb_height); > > > > > > + > > > > > > + if (fb->funcs->dirty) { > > > > > > + struct fb_ops *fbops; > > > > > > + > > > > > > + /* > > > > > > + * fb_deferred_io_cleanup() clears &fbops->fb_mmap so a per > > > > > > + * instance version is necessary. > > > > > > + */ > > > > > > + fbops = kzalloc(sizeof(*fbops), GFP_KERNEL); > > > > > > + if (!fbops) { > > > > > > + ret = -ENOMEM; > > > > > > + goto err_fb_info_destroy; > > > > > > + } > > > > > > + > > > > > > + *fbops = *fbi->fbops; > > > > > > + fbi->fbops = fbops; > > > > > > + > > > > > > + fbi->fbdefio = &drm_fbdev_defio; > > > > > > + > > > > > > + /* TODO: Remove this when tinydrm is converted to vmalloc buffers. */ > > > > > > + fbi->fix.smem_start = page_to_phys(virt_to_page(buffer->vaddr)); > > > > > > + > > > > > > + fb_deferred_io_init(fbi); > > > > > > + > > > > > > + /* TODO: Remove this when tinydrm is converted to vmalloc buffers. */ > > > > > > + fbi->fbops->fb_mmap = drm_fbdev_cma_deferred_io_mmap; > > > > > > + } > > > > > Ugh. Yeah defio and generic allocator through dumb buffers don't mix well. > > > > > The only true generic solution for this would be to give userspace (and > > > > > only userspace, for fbcon we can intercept everything) a staging buffer, > > > > > and then upload things using the dirty callback. > > > > > > > > > > But that introduces another copy step, so isn't cool. > > > > > > > > > > I think a check for is_vmalloc_addr and if that's false, not doing any of > > > > > the defio mmap setup would be good. Until we have a better idea. And yes > > > > > that would need to be done after tinydrm is moved over. > > > > > > > > > > > + > > > > > > + return 0; > > > > > > + > > > > > > +err_fb_info_destroy: > > > > > > + drm_fb_helper_fini(fb_helper); > > > > > > +err_free_buffer: > > > > > > + drm_client_framebuffer_delete(buffer); > > > > > > + > > > > > > + return ret; > > > > > > +} > > > > > > +EXPORT_SYMBOL(drm_fb_helper_generic_probe); > > > > > > + > > > > > > /* The Kconfig DRM_KMS_HELPER selects FRAMEBUFFER_CONSOLE (if !EXPERT) > > > > > > * but the module doesn't depend on any fb console symbols. At least > > > > > > * attempt to load fbcon to avoid leaving the system without a usable console. > > > > > > diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h > > > > > > index b069433e7fc1..bb38469a9502 100644 > > > > > > --- a/include/drm/drm_fb_helper.h > > > > > > +++ b/include/drm/drm_fb_helper.h > > > > > > @@ -30,6 +30,8 @@ > > > > > > #ifndef DRM_FB_HELPER_H > > > > > > #define DRM_FB_HELPER_H > > > > > > +struct drm_client_buffer; > > > > > > +struct drm_client_dev; > > > > > > struct drm_fb_helper; > > > > > > #include <drm/drm_crtc.h> > > > > > > @@ -232,6 +234,20 @@ struct drm_fb_helper { > > > > > > * See also: @deferred_setup > > > > > > */ > > > > > > int preferred_bpp; > > > > > > + > > > > > > + /** > > > > > > + * @client: > > > > > > + * > > > > > > + * DRM client used by the generic fbdev emulation. > > > > > > + */ > > > > > > + struct drm_client_dev *client; > > > > > > + > > > > > > + /** > > > > > > + * @buffer: > > > > > > + * > > > > > > + * Framebuffer used by the generic fbdev emulation. > > > > > > + */ > > > > > > + struct drm_client_buffer *buffer; > > > > > > }; > > > > > > /** > > > > > > @@ -330,6 +346,9 @@ void drm_fb_helper_fbdev_teardown(struct drm_device *dev); > > > > > > void drm_fb_helper_lastclose(struct drm_device *dev); > > > > > > void drm_fb_helper_output_poll_changed(struct drm_device *dev); > > > > > > + > > > > > > +int drm_fb_helper_generic_probe(struct drm_fb_helper *fb_helper, > > > > > > + struct drm_fb_helper_surface_size *sizes); > > > > > > #else > > > > > > static inline void drm_fb_helper_prepare(struct drm_device *dev, > > > > > > struct drm_fb_helper *helper, > > > > > > @@ -564,6 +583,13 @@ static inline void drm_fb_helper_output_poll_changed(struct drm_device *dev) > > > > > > { > > > > > > } > > > > > > +static inline int > > > > > > +drm_fb_helper_generic_probe(struct drm_fb_helper *fb_helper, > > > > > > + struct drm_fb_helper_surface_size *sizes) > > > > > > +{ > > > > > > + return 0; > > > > > > +} > > > > > Ok, I think this patch looks ok. With the missing kerneldoc added (which > > > > > also explains the current limitations) this is > > > > > > > > > > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > > > > > > > > > > + > > > > > > #endif > > > > > > static inline int > > > > > > -- > > > > > > 2.15.1 > > > > > > > > > > > > _______________________________________________ > > > > > > dri-devel mailing list > > > > > > dri-devel@lists.freedesktop.org > > > > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel > > > > > > > > > > -- > > > Daniel Vetter > > > Software Engineer, Intel Corporation > > > http://blog.ffwll.ch > > > > > >
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index 2ee1eaa66188..444c2b4040ea 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -30,11 +30,13 @@ #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt #include <linux/console.h> +#include <linux/dma-buf.h> #include <linux/kernel.h> #include <linux/sysrq.h> #include <linux/slab.h> #include <linux/module.h> #include <drm/drmP.h> +#include <drm/drm_client.h> #include <drm/drm_crtc.h> #include <drm/drm_fb_helper.h> #include <drm/drm_crtc_helper.h> @@ -2928,6 +2930,168 @@ void drm_fb_helper_output_poll_changed(struct drm_device *dev) } EXPORT_SYMBOL(drm_fb_helper_output_poll_changed); +/* @user: 1=userspace, 0=fbcon */ +static int drm_fbdev_fb_open(struct fb_info *info, int user) +{ + struct drm_fb_helper *fb_helper = info->par; + + if (!try_module_get(fb_helper->dev->driver->fops->owner)) + return -ENODEV; + + return 0; +} + +static int drm_fbdev_fb_release(struct fb_info *info, int user) +{ + struct drm_fb_helper *fb_helper = info->par; + + module_put(fb_helper->dev->driver->fops->owner); + + return 0; +} + +/* + * fb_ops.fb_destroy is called by the last put_fb_info() call at the end of + * unregister_framebuffer() or fb_release(). + */ +static void drm_fbdev_fb_destroy(struct fb_info *info) +{ + struct drm_fb_helper *fb_helper = info->par; + struct fb_ops *fbops = NULL; + + DRM_DEBUG("\n"); + + if (fb_helper->fbdev->fbdefio) { + fb_deferred_io_cleanup(fb_helper->fbdev); + fbops = fb_helper->fbdev->fbops; + } + + drm_fb_helper_fini(fb_helper); + drm_client_framebuffer_delete(fb_helper->buffer); + drm_client_free(fb_helper->client); + kfree(fb_helper); + kfree(fbops); +} + +static int drm_fbdev_fb_mmap(struct fb_info *info, struct vm_area_struct *vma) +{ + struct drm_fb_helper *fb_helper = info->par; + + if (fb_helper->dev->driver->gem_prime_mmap) + return fb_helper->dev->driver->gem_prime_mmap(fb_helper->buffer->gem, vma); + else + return -ENODEV; +} + +static struct fb_ops drm_fbdev_fb_ops = { + /* No need to set owner, this module is already pinned by the driver. */ + DRM_FB_HELPER_DEFAULT_OPS, + .fb_open = drm_fbdev_fb_open, + .fb_release = drm_fbdev_fb_release, + .fb_destroy = drm_fbdev_fb_destroy, + .fb_mmap = drm_fbdev_fb_mmap, + .fb_read = drm_fb_helper_sys_read, + .fb_write = drm_fb_helper_sys_write, + .fb_fillrect = drm_fb_helper_sys_fillrect, + .fb_copyarea = drm_fb_helper_sys_copyarea, + .fb_imageblit = drm_fb_helper_sys_imageblit, +}; + +static struct fb_deferred_io drm_fbdev_defio = { + .delay = HZ / 20, + .deferred_io = drm_fb_helper_deferred_io, +}; + +/* + * TODO: Remove this when tinydrm is converted to vmalloc buffers. + */ +static int drm_fbdev_cma_deferred_io_mmap(struct fb_info *info, + struct vm_area_struct *vma) +{ + fb_deferred_io_mmap(info, vma); + vma->vm_page_prot = pgprot_writecombine(vma->vm_page_prot); + + return 0; +} + +int drm_fb_helper_generic_probe(struct drm_fb_helper *fb_helper, + struct drm_fb_helper_surface_size *sizes) +{ + struct drm_client_dev *client = fb_helper->client; + struct drm_client_buffer *buffer; + struct drm_framebuffer *fb; + struct fb_info *fbi; + u32 format; + int ret; + + DRM_DEBUG_KMS("surface width(%d), height(%d) and bpp(%d)\n", + sizes->surface_width, sizes->surface_height, + sizes->surface_bpp); + + format = drm_mode_legacy_fb_format(sizes->surface_bpp, sizes->surface_depth); + buffer = drm_client_framebuffer_create(client, sizes->surface_width, + sizes->surface_height, format); + if (IS_ERR(buffer)) + return PTR_ERR(buffer); + + fb_helper->buffer = buffer; + fb_helper->fb = buffer->fb; + fb = buffer->fb; + + fbi = drm_fb_helper_alloc_fbi(fb_helper); + if (IS_ERR(fbi)) { + ret = PTR_ERR(fbi); + goto err_free_buffer; + } + + fbi->par = fb_helper; + fbi->fbops = &drm_fbdev_fb_ops; + fbi->screen_size = fb->height * fb->pitches[0]; + fbi->fix.smem_len = fbi->screen_size; + fbi->screen_buffer = buffer->vaddr; + strcpy(fbi->fix.id, "DRM emulated"); + + drm_fb_helper_fill_fix(fbi, fb->pitches[0], fb->format->depth); + drm_fb_helper_fill_var(fbi, fb_helper, sizes->fb_width, sizes->fb_height); + + if (fb->funcs->dirty) { + struct fb_ops *fbops; + + /* + * fb_deferred_io_cleanup() clears &fbops->fb_mmap so a per + * instance version is necessary. + */ + fbops = kzalloc(sizeof(*fbops), GFP_KERNEL); + if (!fbops) { + ret = -ENOMEM; + goto err_fb_info_destroy; + } + + *fbops = *fbi->fbops; + fbi->fbops = fbops; + + fbi->fbdefio = &drm_fbdev_defio; + + /* TODO: Remove this when tinydrm is converted to vmalloc buffers. */ + fbi->fix.smem_start = page_to_phys(virt_to_page(buffer->vaddr)); + + fb_deferred_io_init(fbi); + + /* TODO: Remove this when tinydrm is converted to vmalloc buffers. */ + fbi->fbops->fb_mmap = drm_fbdev_cma_deferred_io_mmap; + } + + return 0; + +err_fb_info_destroy: + drm_fb_helper_fini(fb_helper); +err_free_buffer: + drm_client_framebuffer_delete(buffer); + + return ret; +} +EXPORT_SYMBOL(drm_fb_helper_generic_probe); + /* The Kconfig DRM_KMS_HELPER selects FRAMEBUFFER_CONSOLE (if !EXPERT) * but the module doesn't depend on any fb console symbols. At least * attempt to load fbcon to avoid leaving the system without a usable console. diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h index b069433e7fc1..bb38469a9502 100644 --- a/include/drm/drm_fb_helper.h +++ b/include/drm/drm_fb_helper.h @@ -30,6 +30,8 @@ #ifndef DRM_FB_HELPER_H #define DRM_FB_HELPER_H +struct drm_client_buffer; +struct drm_client_dev; struct drm_fb_helper; #include <drm/drm_crtc.h> @@ -232,6 +234,20 @@ struct drm_fb_helper { * See also: @deferred_setup */ int preferred_bpp; + + /** + * @client: + * + * DRM client used by the generic fbdev emulation. + */ + struct drm_client_dev *client; + + /** + * @buffer: + * + * Framebuffer used by the generic fbdev emulation. + */ + struct drm_client_buffer *buffer; }; /** @@ -330,6 +346,9 @@ void drm_fb_helper_fbdev_teardown(struct drm_device *dev); void drm_fb_helper_lastclose(struct drm_device *dev); void drm_fb_helper_output_poll_changed(struct drm_device *dev); + +int drm_fb_helper_generic_probe(struct drm_fb_helper *fb_helper, + struct drm_fb_helper_surface_size *sizes); #else static inline void drm_fb_helper_prepare(struct drm_device *dev, struct drm_fb_helper *helper, @@ -564,6 +583,13 @@ static inline void drm_fb_helper_output_poll_changed(struct drm_device *dev) { } +static inline int +drm_fb_helper_generic_probe(struct drm_fb_helper *fb_helper, + struct drm_fb_helper_surface_size *sizes) +{ + return 0; +} + #endif static inline int
This is the first step in getting generic fbdev emulation. A drm_fb_helper_funcs.fb_probe function is added which uses the DRM client API to get a framebuffer backed by a dumb buffer. A transitional hack for tinydrm is needed in order to switch over all CMA helper drivers in a later patch. This hack will be removed when tinydrm moves to vmalloc buffers. Signed-off-by: Noralf Trønnes <noralf@tronnes.org> --- drivers/gpu/drm/drm_fb_helper.c | 164 ++++++++++++++++++++++++++++++++++++++++ include/drm/drm_fb_helper.h | 26 +++++++ 2 files changed, 190 insertions(+)