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

[Daniel Vetter <daniel@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
> > 
> > 
> > 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx