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 at 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 at ffwll.ch>
> >
> > > +
> > > #endif
> > > static inline int
> > > --
> > > 2.15.1
> > >
> > > _______________________________________________
> > > dri-devel mailing list
> > > dri-devel at lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch