Re: [PATCH 3/6] drm/fb-cma-helper: Support device unplug

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Aug 29, 2017 at 07:23:14PM +0200, Noralf Trønnes wrote:
> 
> Den 28.08.2017 23.46, skrev Daniel Vetter:
> > On Mon, Aug 28, 2017 at 07:17:45PM +0200, Noralf Trønnes wrote:
> > > Add drm_fbdev_cma_dev_unplug() and use the drm_fb_helper device unplug
> > > support. Pin driver module on fb_open().
> > > 
> > > Cc: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> > > Signed-off-by: Noralf Trønnes <noralf@xxxxxxxxxxx>
> > > ---
> > >   drivers/gpu/drm/drm_fb_cma_helper.c | 139 +++++++++++++++++-------------------
> > >   include/drm/drm_fb_cma_helper.h     |   1 +
> > >   2 files changed, 68 insertions(+), 72 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c
> > > index f2ee883..2b044be 100644
> > > --- a/drivers/gpu/drm/drm_fb_cma_helper.c
> > > +++ b/drivers/gpu/drm/drm_fb_cma_helper.c
> > > @@ -25,8 +25,6 @@
> > >   #include <drm/drm_fb_cma_helper.h>
> > >   #include <linux/module.h>
> > > -#define DEFAULT_FBDEFIO_DELAY_MS 50
> > > -
> > >   struct drm_fbdev_cma {
> > >   	struct drm_fb_helper	fb_helper;
> > >   	const struct drm_framebuffer_funcs *fb_funcs;
> > > @@ -238,6 +236,34 @@ int drm_fb_cma_debugfs_show(struct seq_file *m, void *arg)
> > >   EXPORT_SYMBOL_GPL(drm_fb_cma_debugfs_show);
> > >   #endif
> > > +static int drm_fbdev_cma_fb_open(struct fb_info *info, int user)
> > > +{
> > > +	struct drm_fb_helper *fb_helper = info->par;
> > > +	struct drm_device *dev = fb_helper->dev;
> > > +
> > > +	/*
> > > +	 * The fb_ops definition resides in this library, meaning fb_open()
> > > +	 * will take a ref on the library instead of the driver. Make sure the
> > > +	 * driver module is pinned. Skip fbcon (user==0) since it can detach
> > > +	 * itself on unregister_framebuffer().
> > > +	 */
> > > +	if (user && !try_module_get(dev->driver->fops->owner))
> > > +		return -ENODEV;
> > I thought we've fixed this by redoing the fb_ops as a macro? Maybe tinydrm
> > isn't fixed yet, but then we need to fix up tinydrm. This here otoh kinda
> > smells a bit like a hack ...
> > 
> > If we need it, then I'd vote to put it into the fbdev helpers, not the cma
> > specific parts.
> 
> This is only necessary in this library atm because it contains this:
> 
> static struct fb_ops drm_fbdev_cma_ops = {
>     .owner        = THIS_MODULE,
> 
> Other drivers have this definition in their own module, so fb_open()
> takes a ref on the right module.
> 
> My shmem gem library will also wrap struct fb_ops and needs to do this.
> I can add them to drm_fb_helper with a note that they should only be
> used by libraries.
> 
> Yes it's kind of a hack, but I find the cma library is very nice in this
> respect, that it makes fbdev emulation very easy for drivers. It wraps
> up basic fbdev in 3 function calls.
> 
> If we were to do this strictly by the book, we would have to add
> something like this to every cma driver:
> 
> DEFINE_DRM_FBDEV_CMA_FB_OPS(driver_fb_ops);
> 
> static int driver_fb_probe(struct drm_fb_helper *helper,
>             struct drm_fb_helper_surface_size *sizes)
> {
>     return drm_fbdev_cma_create(helper, sizes, &driver_fb_ops);
> }
> 
> static const struct drm_fb_helper_funcs driver_helper_funcs = {
>     .fb_probe = driver_fb_probe,
> };
> 
> static int driver_fbdev_init()
> {
> -    fbdev_cma = drm_fbdev_cma_init(dev, preferred_bpp, max_conn_count);
> +    fbdev_cma = drm_fbdev_cma_init(dev, preferred_bpp, max_conn_count,
> +                    &driver_helper_funcs);
> }
> 
> Well, maybe not so bad after all :-)
> Maybe I should do it like this in the shmem gem library.

Hm, this does add considerable amounts of boilerplate everywhere, so not
sure it's a good idea.

> I can't do refactoring like this, this time around. I need to do that
> shmem gem library before I run out of steam. A refactoring like this
> will have to wait to later.

I think your fb_open/close hooks are just fine, but instead of putting
them into cma helpers only let's just put them into the core helpers (and
add them to DRM_FB_HELPER_DEFAULT_OPS). Them being a special case for cma
only was what really irked me, not so much what they do. And doing the
get/put module by default shouldn't really hurt (for drivers where the
fb_ops and the drm_driver ops match it'll mean double refcounting, but
that doesn't matter really).

> 
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int drm_fbdev_cma_fb_release(struct fb_info *info, int user)
> > > +{
> > > +	struct drm_fb_helper *fb_helper = info->par;
> > > +	struct drm_device *dev = fb_helper->dev;
> > > +
> > > +	if (user)
> > > +		module_put(dev->driver->fops->owner);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > >   static int drm_fb_cma_mmap(struct fb_info *info, struct vm_area_struct *vma)
> > >   {
> > >   	return dma_mmap_writecombine(info->device, vma, info->screen_base,
> > > @@ -247,10 +273,13 @@ static int drm_fb_cma_mmap(struct fb_info *info, struct vm_area_struct *vma)
> > >   static struct fb_ops drm_fbdev_cma_ops = {
> > >   	.owner		= THIS_MODULE,
> > >   	DRM_FB_HELPER_DEFAULT_OPS,
> > > +	.fb_open	= drm_fbdev_cma_fb_open,
> > > +	.fb_release	= drm_fbdev_cma_fb_release,
> > >   	.fb_fillrect	= drm_fb_helper_sys_fillrect,
> > >   	.fb_copyarea	= drm_fb_helper_sys_copyarea,
> > >   	.fb_imageblit	= drm_fb_helper_sys_imageblit,
> > >   	.fb_mmap	= drm_fb_cma_mmap,
> > > +	.fb_destroy	= drm_fb_helper_fb_destroy,
> > >   };
> > >   static int drm_fbdev_cma_deferred_io_mmap(struct fb_info *info,
> > > @@ -262,52 +291,26 @@ static int drm_fbdev_cma_deferred_io_mmap(struct fb_info *info,
> > >   	return 0;
> > >   }
> > > -static int drm_fbdev_cma_defio_init(struct fb_info *fbi,
> > > +static int drm_fbdev_cma_defio_init(struct drm_fb_helper *helper,
> > >   				    struct drm_gem_cma_object *cma_obj)
> > >   {
> > > -	struct fb_deferred_io *fbdefio;
> > > -	struct fb_ops *fbops;
> > > +	struct fb_info *fbi = helper->fbdev;
> > > +	int ret;
> > > -	/*
> > > -	 * Per device structures are needed because:
> > > -	 * fbops: fb_deferred_io_cleanup() clears fbops.fb_mmap
> > > -	 * fbdefio: individual delays
> > > -	 */
> > > -	fbdefio = kzalloc(sizeof(*fbdefio), GFP_KERNEL);
> > > -	fbops = kzalloc(sizeof(*fbops), GFP_KERNEL);
> > > -	if (!fbdefio || !fbops) {
> > > -		kfree(fbdefio);
> > > -		kfree(fbops);
> > > -		return -ENOMEM;
> > > -	}
> > > +	ret = drm_fb_helper_defio_init(helper);
> > > +	if (ret)
> > > +		return ret;
> > >   	/* can't be offset from vaddr since dirty() uses cma_obj */
> > >   	fbi->screen_buffer = cma_obj->vaddr;
> > >   	/* fb_deferred_io_fault() needs a physical address */
> > >   	fbi->fix.smem_start = page_to_phys(virt_to_page(fbi->screen_buffer));
> > > -	*fbops = *fbi->fbops;
> > > -	fbi->fbops = fbops;
> > > -
> > > -	fbdefio->delay = msecs_to_jiffies(DEFAULT_FBDEFIO_DELAY_MS);
> > > -	fbdefio->deferred_io = drm_fb_helper_deferred_io;
> > > -	fbi->fbdefio = fbdefio;
> > > -	fb_deferred_io_init(fbi);
> > >   	fbi->fbops->fb_mmap = drm_fbdev_cma_deferred_io_mmap;
> > >   	return 0;
> > >   }
> > > -static void drm_fbdev_cma_defio_fini(struct fb_info *fbi)
> > > -{
> > > -	if (!fbi->fbdefio)
> > > -		return;
> > > -
> > > -	fb_deferred_io_cleanup(fbi);
> > > -	kfree(fbi->fbdefio);
> > > -	kfree(fbi->fbops);
> > > -}
> > > -
> > Above changes look like unrelated cleanup? Should be a separate patch I
> > think.
> > 
> > >   static int
> > >   drm_fbdev_cma_create(struct drm_fb_helper *helper,
> > >   	struct drm_fb_helper_surface_size *sizes)
> > > @@ -365,7 +368,7 @@ drm_fbdev_cma_create(struct drm_fb_helper *helper,
> > >   	fbi->fix.smem_len = size;
> > >   	if (fbdev_cma->fb_funcs->dirty) {
> > > -		ret = drm_fbdev_cma_defio_init(fbi, obj);
> > > +		ret = drm_fbdev_cma_defio_init(helper, obj);
> > >   		if (ret)
> > >   			goto err_cma_destroy;
> > >   	}
> > > @@ -399,7 +402,6 @@ struct drm_fbdev_cma *drm_fbdev_cma_init_with_funcs(struct drm_device *dev,
> > >   	const struct drm_framebuffer_funcs *funcs)
> > >   {
> > >   	struct drm_fbdev_cma *fbdev_cma;
> > > -	struct drm_fb_helper *helper;
> > >   	int ret;
> > >   	fbdev_cma = kzalloc(sizeof(*fbdev_cma), GFP_KERNEL);
> > > @@ -409,37 +411,15 @@ struct drm_fbdev_cma *drm_fbdev_cma_init_with_funcs(struct drm_device *dev,
> > >   	}
> > >   	fbdev_cma->fb_funcs = funcs;
> > > -	helper = &fbdev_cma->fb_helper;
> > > -
> > > -	drm_fb_helper_prepare(dev, helper, &drm_fb_cma_helper_funcs);
> > > -
> > > -	ret = drm_fb_helper_init(dev, helper, max_conn_count);
> > > -	if (ret < 0) {
> > > -		dev_err(dev->dev, "Failed to initialize drm fb helper.\n");
> > > -		goto err_free;
> > > -	}
> > > -
> > > -	ret = drm_fb_helper_single_add_all_connectors(helper);
> > > -	if (ret < 0) {
> > > -		dev_err(dev->dev, "Failed to add connectors.\n");
> > > -		goto err_drm_fb_helper_fini;
> > > -
> > > -	}
> > > -
> > > -	ret = drm_fb_helper_initial_config(helper, preferred_bpp);
> > > -	if (ret < 0) {
> > > -		dev_err(dev->dev, "Failed to set initial hw configuration.\n");
> > > -		goto err_drm_fb_helper_fini;
> > > +	ret = drm_fb_helper_simple_init(dev, &fbdev_cma->fb_helper,
> > > +					preferred_bpp, max_conn_count,
> > > +					&drm_fb_cma_helper_funcs);
> > Yeah the conversion to simple* should be split out I think. But then I
> > thought more of this will be unified with the overall fbdev helpers?
> 
> I agree, if we find a way to support unplug within the existing helpers.
> 
> > > +	if (ret) {
> > > +		kfree(fbdev_cma);
> > > +		return ERR_PTR(ret);
> > >   	}
> > >   	return fbdev_cma;
> > > -
> > > -err_drm_fb_helper_fini:
> > > -	drm_fb_helper_fini(helper);
> > > -err_free:
> > > -	kfree(fbdev_cma);
> > > -
> > > -	return ERR_PTR(ret);
> > >   }
> > >   EXPORT_SYMBOL_GPL(drm_fbdev_cma_init_with_funcs);
> > > @@ -468,17 +448,15 @@ EXPORT_SYMBOL_GPL(drm_fbdev_cma_init);
> > >   /**
> > >    * drm_fbdev_cma_fini() - Free drm_fbdev_cma struct
> > >    * @fbdev_cma: The drm_fbdev_cma struct
> > > + *
> > > + * This function calls drm_fb_helper_simple_fini() and frees @fbdev_cma.
> > > + *
> > > + * Don't use this function together with drm_fbdev_cma_dev_unplug().
> > >    */
> > >   void drm_fbdev_cma_fini(struct drm_fbdev_cma *fbdev_cma)
> > >   {
> > > -	drm_fb_helper_unregister_fbi(&fbdev_cma->fb_helper);
> > > -	if (fbdev_cma->fb_helper.fbdev)
> > > -		drm_fbdev_cma_defio_fini(fbdev_cma->fb_helper.fbdev);
> > > -
> > > -	if (fbdev_cma->fb_helper.fb)
> > > -		drm_framebuffer_remove(fbdev_cma->fb_helper.fb);
> > > -
> > > -	drm_fb_helper_fini(&fbdev_cma->fb_helper);
> > > +	if (fbdev_cma)
> > > +		drm_fb_helper_simple_fini(&fbdev_cma->fb_helper);
> > >   	kfree(fbdev_cma);
> > >   }
> > >   EXPORT_SYMBOL_GPL(drm_fbdev_cma_fini);
> > > @@ -542,3 +520,20 @@ void drm_fbdev_cma_set_suspend_unlocked(struct drm_fbdev_cma *fbdev_cma,
> > >   						   state);
> > >   }
> > >   EXPORT_SYMBOL(drm_fbdev_cma_set_suspend_unlocked);
> > > +
> > > +/**
> > > + * drm_fbdev_cma_dev_unplug - wrapper around drm_fb_helper_dev_unplug
> > > + * @fbdev_cma: The drm_fbdev_cma struct, may be NULL
> > > + *
> > > + * This unplugs the fbdev emulation for a hotpluggable DRM device. See
> > > + * drm_fb_helper_dev_unplug() for details.
> > > + *
> > > + * Drivers must call drm_mode_config_cleanup() and free @fbdev_cma in their
> > > + * &drm_driver->release callback.
> > > + */
> > > +void drm_fbdev_cma_dev_unplug(struct drm_fbdev_cma *fbdev_cma)
> > > +{
> > > +	if (fbdev_cma)
> > > +		drm_fb_helper_dev_unplug(&fbdev_cma->fb_helper);
> > > +}
> > I thought we're trying to phase out the cma helpers, why do we need a
> > wrapper for a wrapper?
> 
> Because struct drm_fbdev_cma is private to this file.
> When tinydrm switches to shmem, we can remove the dirtyfb stuff from
> this helper and we will end up with this:
> 
> struct drm_fbdev_cma {
>     struct drm_fb_helper    fb_helper;
> };
> 
> Now it is possible to refactor and switch to using drm_fb_helper
> directly and drop the fbdev wrappers.
> 
> We can ofc avoid adding a wrapper by making struct drm_fbdev_cma public.

Hm, no I understand. I'd vote to make struct drm_fbdev_cma public. We
generally don't try to hide stuff in structs only in .c files, makes it
easier to up/downcast and have static inline helpers.

Moving the struct should be a separate patch I think.
-Daniel

> 
> 
> Noralf.
> 
> > > +EXPORT_SYMBOL(drm_fbdev_cma_dev_unplug);
> > > diff --git a/include/drm/drm_fb_cma_helper.h b/include/drm/drm_fb_cma_helper.h
> > > index a323781..a6aafae 100644
> > > --- a/include/drm/drm_fb_cma_helper.h
> > > +++ b/include/drm/drm_fb_cma_helper.h
> > > @@ -27,6 +27,7 @@ void drm_fbdev_cma_hotplug_event(struct drm_fbdev_cma *fbdev_cma);
> > >   void drm_fbdev_cma_set_suspend(struct drm_fbdev_cma *fbdev_cma, bool state);
> > >   void drm_fbdev_cma_set_suspend_unlocked(struct drm_fbdev_cma *fbdev_cma,
> > >   					bool state);
> > > +void drm_fbdev_cma_dev_unplug(struct drm_fbdev_cma *fbdev_cma);
> > >   void drm_fb_cma_destroy(struct drm_framebuffer *fb);
> > >   int drm_fb_cma_create_handle(struct drm_framebuffer *fb,
> > > -- 
> > > 2.7.4
> > > 
> 

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




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux