Re: [PATCH 5/9] drm/fb-helper: Add generic fbdev emulation .fb_probe function

[Daniel Vetter <daniel@ffwll.ch>]

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



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel