[PATCH 3/6] drm/fb-cma-helper: Support device unplug
Noralf Trønnes
noralf at tronnes.org
Tue Aug 29 17:23:14 UTC 2017
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 at ideasonboard.com>
>> Signed-off-by: Noralf Trønnes <noralf at tronnes.org>
>> ---
>> 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.
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.
>> +
>> + 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.
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
>>
More information about the dri-devel
mailing list