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.
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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
