diff mbox

[v5,7/7] drm/simpledrm: add fbdev fallback support

Message ID 20160902082245.7119-8-dh.herrmann@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

David Herrmann Sept. 2, 2016, 8:22 a.m. UTC 
Create a simple fbdev device during SimpleDRM setup so legacy user-space
and fbcon can use it.

Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
 drivers/gpu/drm/simpledrm/Makefile          |   1 +
 drivers/gpu/drm/simpledrm/simpledrm.h       |   8 ++
 drivers/gpu/drm/simpledrm/simpledrm_drv.c   |  13 +++
 drivers/gpu/drm/simpledrm/simpledrm_fbdev.c | 143 ++++++++++++++++++++++++++++
 drivers/gpu/drm/simpledrm/simpledrm_kms.c   |  55 ++++++-----
 5 files changed, 196 insertions(+), 24 deletions(-)
 create mode 100644 drivers/gpu/drm/simpledrm/simpledrm_fbdev.c

Comments

Noralf Trønnes Sept. 3, 2016, 12:04 p.m. UTC | #1 
Den 02.09.2016 10:22, skrev David Herrmann:
> Create a simple fbdev device during SimpleDRM setup so legacy user-space
> and fbcon can use it.
>
> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>

[...]

> diff --git a/drivers/gpu/drm/simpledrm/simpledrm_fbdev.c b/drivers/gpu/drm/simpledrm/simpledrm_fbdev.c

[...]

> +void sdrm_fbdev_bind(struct sdrm_device *sdrm)
> +{
> +	struct drm_fb_helper *fbdev;
> +	int r;
> +
> +	fbdev = kzalloc(sizeof(*fbdev), GFP_KERNEL);
> +	if (!fbdev)
> +		return;
> +
> +	drm_fb_helper_prepare(sdrm->ddev, fbdev, &sdrm_fbdev_funcs);
> +
> +	r = drm_fb_helper_init(sdrm->ddev, fbdev, 1, 1);
> +	if (r < 0)
> +		goto error;
> +
> +	r = drm_fb_helper_single_add_all_connectors(fbdev);
> +	if (r < 0)
> +		goto error;
> +
> +	r = drm_fb_helper_initial_config(fbdev,
> +				sdrm->ddev->mode_config.preferred_depth);
> +	if (r < 0)
> +		goto error;
> +
> +	if (!fbdev->fbdev)
> +		goto error;
> +
> +	sdrm->fbdev = fbdev;
> +	return;
> +
> +error:
> +	drm_fb_helper_fini(fbdev);
> +	kfree(fbdev);
> +}
> +
> +void sdrm_fbdev_unbind(struct sdrm_device *sdrm)
> +{
> +	struct drm_fb_helper *fbdev = sdrm->fbdev;
> +
> +	if (!fbdev)
> +		return;
> +
> +	sdrm->fbdev = NULL;
> +	drm_fb_helper_unregister_fbi(fbdev);
> +	cancel_work_sync(&fbdev->dirty_work);
> +	drm_fb_helper_release_fbi(fbdev);
> +	drm_framebuffer_unreference(fbdev->fb);

I get a warning that there are still fb's left during unbind:

[   48.666003] WARNING: CPU: 0 PID: 716 at 
drivers/gpu/drm/drm_crtc.c:3855 drm_mode_config_cleanup+0x180/0x1f4 [drm]

This worked:

-       drm_framebuffer_unreference(fbdev->fb);
+       drm_framebuffer_unregister_private(fbdev->fb);
+       drm_framebuffer_cleanup(fbdev->fb);


Noralf.

> +	fbdev->fb = NULL;
> +	drm_fb_helper_fini(fbdev);
> +	kfree(fbdev);
> +}
Noralf Trønnes Sept. 3, 2016, 5:15 p.m. UTC | #2 
Den 03.09.2016 14:04, skrev Noralf Trønnes:
>
> Den 02.09.2016 10:22, skrev David Herrmann:
>> Create a simple fbdev device during SimpleDRM setup so legacy user-space
>> and fbcon can use it.
>>
>> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
>
> [...]
>
>> diff --git a/drivers/gpu/drm/simpledrm/simpledrm_fbdev.c 
>> b/drivers/gpu/drm/simpledrm/simpledrm_fbdev.c
>
> [...]
>
>> +void sdrm_fbdev_bind(struct sdrm_device *sdrm)
>> +{
>> +    struct drm_fb_helper *fbdev;
>> +    int r;
>> +
>> +    fbdev = kzalloc(sizeof(*fbdev), GFP_KERNEL);
>> +    if (!fbdev)
>> +        return;
>> +
>> +    drm_fb_helper_prepare(sdrm->ddev, fbdev, &sdrm_fbdev_funcs);
>> +
>> +    r = drm_fb_helper_init(sdrm->ddev, fbdev, 1, 1);
>> +    if (r < 0)
>> +        goto error;
>> +
>> +    r = drm_fb_helper_single_add_all_connectors(fbdev);
>> +    if (r < 0)
>> +        goto error;
>> +
>> +    r = drm_fb_helper_initial_config(fbdev,
>> +                sdrm->ddev->mode_config.preferred_depth);
>> +    if (r < 0)
>> +        goto error;
>> +
>> +    if (!fbdev->fbdev)
>> +        goto error;
>> +
>> +    sdrm->fbdev = fbdev;
>> +    return;
>> +
>> +error:
>> +    drm_fb_helper_fini(fbdev);
>> +    kfree(fbdev);
>> +}
>> +
>> +void sdrm_fbdev_unbind(struct sdrm_device *sdrm)
>> +{
>> +    struct drm_fb_helper *fbdev = sdrm->fbdev;
>> +
>> +    if (!fbdev)
>> +        return;
>> +
>> +    sdrm->fbdev = NULL;
>> +    drm_fb_helper_unregister_fbi(fbdev);
>> +    cancel_work_sync(&fbdev->dirty_work);
>> +    drm_fb_helper_release_fbi(fbdev);
>> +    drm_framebuffer_unreference(fbdev->fb);
>
> I get a warning that there are still fb's left during unbind:
>
> [   48.666003] WARNING: CPU: 0 PID: 716 at 
> drivers/gpu/drm/drm_crtc.c:3855 drm_mode_config_cleanup+0x180/0x1f4 [drm]
>
> This worked:
>
> -       drm_framebuffer_unreference(fbdev->fb);
> +       drm_framebuffer_unregister_private(fbdev->fb);
> +       drm_framebuffer_cleanup(fbdev->fb);
>

Well not quite, this doesn't free the bo, so maybe this:

-       drm_framebuffer_unreference(fbdev->fb);
+       drm_framebuffer_unregister_private(fbdev->fb);
+       sdrm_fb_destroy(fbdev->fb);

IIRC the reason ref count doesn't drop to zero, had something to do with 
multiple
fb_set_par calls taking reference but not being dropped later.

Noralf.

>
> Noralf.
>
>> +    fbdev->fb = NULL;
>> +    drm_fb_helper_fini(fbdev);
>> +    kfree(fbdev);
>> +}
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
David Herrmann Sept. 5, 2016, 11:21 a.m. UTC | #3 
Hi

On Sat, Sep 3, 2016 at 7:15 PM, Noralf Trønnes <noralf@tronnes.org> wrote:
>
> Den 03.09.2016 14:04, skrev Noralf Trønnes:
>>
>>
>> Den 02.09.2016 10:22, skrev David Herrmann:
>>>
>>> Create a simple fbdev device during SimpleDRM setup so legacy user-space
>>> and fbcon can use it.
>>>
>>> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
>>
>>
>> [...]
>>
>>> diff --git a/drivers/gpu/drm/simpledrm/simpledrm_fbdev.c
>>> b/drivers/gpu/drm/simpledrm/simpledrm_fbdev.c
>>
>>
>> [...]
>>
>>> +void sdrm_fbdev_bind(struct sdrm_device *sdrm)
>>> +{
>>> +    struct drm_fb_helper *fbdev;
>>> +    int r;
>>> +
>>> +    fbdev = kzalloc(sizeof(*fbdev), GFP_KERNEL);
>>> +    if (!fbdev)
>>> +        return;
>>> +
>>> +    drm_fb_helper_prepare(sdrm->ddev, fbdev, &sdrm_fbdev_funcs);
>>> +
>>> +    r = drm_fb_helper_init(sdrm->ddev, fbdev, 1, 1);
>>> +    if (r < 0)
>>> +        goto error;
>>> +
>>> +    r = drm_fb_helper_single_add_all_connectors(fbdev);
>>> +    if (r < 0)
>>> +        goto error;
>>> +
>>> +    r = drm_fb_helper_initial_config(fbdev,
>>> +                sdrm->ddev->mode_config.preferred_depth);
>>> +    if (r < 0)
>>> +        goto error;
>>> +
>>> +    if (!fbdev->fbdev)
>>> +        goto error;
>>> +
>>> +    sdrm->fbdev = fbdev;
>>> +    return;
>>> +
>>> +error:
>>> +    drm_fb_helper_fini(fbdev);
>>> +    kfree(fbdev);
>>> +}
>>> +
>>> +void sdrm_fbdev_unbind(struct sdrm_device *sdrm)
>>> +{
>>> +    struct drm_fb_helper *fbdev = sdrm->fbdev;
>>> +
>>> +    if (!fbdev)
>>> +        return;
>>> +
>>> +    sdrm->fbdev = NULL;
>>> +    drm_fb_helper_unregister_fbi(fbdev);
>>> +    cancel_work_sync(&fbdev->dirty_work);
>>> +    drm_fb_helper_release_fbi(fbdev);
>>> +    drm_framebuffer_unreference(fbdev->fb);
>>
>>
>> I get a warning that there are still fb's left during unbind:
>>
>> [   48.666003] WARNING: CPU: 0 PID: 716 at drivers/gpu/drm/drm_crtc.c:3855
>> drm_mode_config_cleanup+0x180/0x1f4 [drm]
>>
>> This worked:
>>
>> -       drm_framebuffer_unreference(fbdev->fb);
>> +       drm_framebuffer_unregister_private(fbdev->fb);
>> +       drm_framebuffer_cleanup(fbdev->fb);
>>
>
> Well not quite, this doesn't free the bo, so maybe this:
>
> -       drm_framebuffer_unreference(fbdev->fb);
> +       drm_framebuffer_unregister_private(fbdev->fb);
> +       sdrm_fb_destroy(fbdev->fb);
>
> IIRC the reason ref count doesn't drop to zero, had something to do with
> multiple
> fb_set_par calls taking reference but not being dropped later.

So I don't like this at all. If we leak references, we should fix the
ref-leak or properly document why this is fine to do. Daniel, what is
the exact reason we have unregister_private()? Maybe I should try and
trace the ref-leak.

Thanks
David
diff mbox

Patch

diff --git a/drivers/gpu/drm/simpledrm/Makefile b/drivers/gpu/drm/simpledrm/Makefile
index d7b179d..e368d14 100644
--- a/drivers/gpu/drm/simpledrm/Makefile
+++ b/drivers/gpu/drm/simpledrm/Makefile
@@ -1,6 +1,7 @@ 
 simpledrm-y := \
 	simpledrm_damage.o \
 	simpledrm_drv.o \
+	simpledrm_fbdev.o \
 	simpledrm_gem.o \
 	simpledrm_kms.o \
 	simpledrm_of.o
diff --git a/drivers/gpu/drm/simpledrm/simpledrm.h b/drivers/gpu/drm/simpledrm/simpledrm.h
index ed6d725..d7a2045 100644
--- a/drivers/gpu/drm/simpledrm/simpledrm.h
+++ b/drivers/gpu/drm/simpledrm/simpledrm.h
@@ -19,6 +19,7 @@ 
 #include <linux/mutex.h>
 
 struct clk;
+struct drm_fb_helper;
 struct regulator;
 struct simplefb_format;
 
@@ -49,6 +50,7 @@  struct sdrm_device {
 	atomic_t n_used;
 	struct drm_device *ddev;
 	struct sdrm_hw *hw;
+	struct drm_fb_helper *fbdev;
 
 	size_t n_clks;
 	size_t n_regulators;
@@ -66,6 +68,9 @@  void sdrm_of_unbind(struct sdrm_device *sdrm);
 int sdrm_kms_bind(struct sdrm_device *sdrm);
 void sdrm_kms_unbind(struct sdrm_device *sdrm);
 
+void sdrm_fbdev_bind(struct sdrm_device *sdrm);
+void sdrm_fbdev_unbind(struct sdrm_device *sdrm);
+
 void sdrm_dirty(struct sdrm_fb *fb, u32 x, u32 y, u32 width, u32 height);
 
 struct sdrm_bo *sdrm_bo_new(struct drm_device *ddev, size_t size);
@@ -80,4 +85,7 @@  int sdrm_dumb_map_offset(struct drm_file *dfile,
 			 uint32_t handle,
 			 uint64_t *offset);
 
+struct sdrm_fb *sdrm_fb_new(struct sdrm_bo *bo,
+			    const struct drm_mode_fb_cmd2 *cmd);
+
 #endif /* __SDRM_SIMPLEDRM_H */
diff --git a/drivers/gpu/drm/simpledrm/simpledrm_drv.c b/drivers/gpu/drm/simpledrm/simpledrm_drv.c
index d569120..6aefe49 100644
--- a/drivers/gpu/drm/simpledrm/simpledrm_drv.c
+++ b/drivers/gpu/drm/simpledrm/simpledrm_drv.c
@@ -9,6 +9,7 @@ 
 
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 #include <drm/drmP.h>
+#include <drm/drm_fb_helper.h>
 #include <linux/atomic.h>
 #include <linux/err.h>
 #include <linux/io.h>
@@ -210,6 +211,7 @@  error:
 static void sdrm_device_unbind(struct sdrm_device *sdrm)
 {
 	if (sdrm) {
+		sdrm_fbdev_unbind(sdrm);
 		sdrm_kms_unbind(sdrm);
 		sdrm_hw_unbind(sdrm->hw);
 		sdrm_of_unbind(sdrm);
@@ -232,6 +234,8 @@  static int sdrm_device_bind(struct sdrm_device *sdrm)
 	if (r < 0)
 		goto error;
 
+	sdrm_fbdev_bind(sdrm);
+
 	return 0;
 
 error:
@@ -253,6 +257,14 @@  static void sdrm_device_release(struct sdrm_device *sdrm)
 	}
 }
 
+static void sdrm_device_lastclose(struct drm_device *ddev)
+{
+	struct sdrm_device *sdrm = ddev->dev_private;
+
+	if (sdrm->fbdev)
+		drm_fb_helper_restore_fbdev_mode_unlocked(sdrm->fbdev);
+}
+
 static int sdrm_fop_open(struct inode *inode, struct file *file)
 {
 	struct drm_device *ddev;
@@ -411,6 +423,7 @@  static const struct file_operations sdrm_drm_fops = {
 static struct drm_driver sdrm_drm_driver = {
 	.driver_features = DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC,
 	.fops = &sdrm_drm_fops,
+	.lastclose = sdrm_device_lastclose,
 
 	.gem_free_object = sdrm_bo_free,
 
diff --git a/drivers/gpu/drm/simpledrm/simpledrm_fbdev.c b/drivers/gpu/drm/simpledrm/simpledrm_fbdev.c
new file mode 100644
index 0000000..17e4e83
--- /dev/null
+++ b/drivers/gpu/drm/simpledrm/simpledrm_fbdev.c
@@ -0,0 +1,143 @@ 
+/*
+ * Copyright (C) 2012-2016 Red Hat, Inc.
+ * Copyright (C) 2016 Noralf Trønnes
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU Lesser General Public License as published by the
+ * Free Software Foundation; either version 2.1 of the License, or (at your
+ * option) any later version.
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+#include <drm/drmP.h>
+#include <drm/drm_crtc_helper.h>
+#include <drm/drm_fb_helper.h>
+#include <linux/err.h>
+#include <linux/fb.h>
+#include <linux/kernel.h>
+#include <linux/slab.h>
+#include "simpledrm.h"
+
+static int sdrm_fbdev_mmap(struct fb_info *info, struct vm_area_struct *vma)
+{
+	return -ENODEV;
+}
+
+static struct fb_ops sdrm_fbdev_ops = {
+	.owner		= THIS_MODULE,
+	.fb_fillrect	= drm_fb_helper_sys_fillrect,
+	.fb_copyarea	= drm_fb_helper_sys_copyarea,
+	.fb_imageblit	= drm_fb_helper_sys_imageblit,
+	.fb_check_var	= drm_fb_helper_check_var,
+	.fb_set_par	= drm_fb_helper_set_par,
+	.fb_setcmap	= drm_fb_helper_setcmap,
+	.fb_mmap	= sdrm_fbdev_mmap,
+};
+
+static int sdrm_fbdev_probe(struct drm_fb_helper *fbdev,
+			    struct drm_fb_helper_surface_size *sizes)
+{
+	struct sdrm_device *sdrm = fbdev->dev->dev_private;
+	struct drm_mode_fb_cmd2 cmd = {
+		.width = sdrm->hw->width,
+		.height = sdrm->hw->height,
+		.pitches[0] = sdrm->hw->stride,
+		.pixel_format = sdrm->hw->format,
+	};
+	struct fb_info *fbi;
+	struct sdrm_bo *bo;
+	struct sdrm_fb *fb;
+	int r;
+
+	fbi = drm_fb_helper_alloc_fbi(fbdev);
+	if (IS_ERR(fbi))
+		return PTR_ERR(fbi);
+
+	bo = sdrm_bo_new(sdrm->ddev,
+			 PAGE_ALIGN(sdrm->hw->height * sdrm->hw->stride));
+	if (!bo) {
+		r = -ENOMEM;
+		goto error;
+	}
+
+	fb = sdrm_fb_new(bo, &cmd);
+	drm_gem_object_unreference_unlocked(&bo->base);
+	if (IS_ERR(fb)) {
+		r = PTR_ERR(fb);
+		goto error;
+	}
+
+	fbdev->fb = &fb->base;
+	fbi->par = fbdev;
+	fbi->flags = FBINFO_DEFAULT | FBINFO_MISC_FIRMWARE;
+	fbi->fbops = &sdrm_fbdev_ops;
+
+	drm_fb_helper_fill_fix(fbi, fb->base.pitches[0], fb->base.depth);
+	drm_fb_helper_fill_var(fbi, fbdev, fb->base.width, fb->base.height);
+
+	strncpy(fbi->fix.id, "simpledrmfb", sizeof(fbi->fix.id) - 1);
+	fbi->screen_base = bo->vmapping;
+	fbi->fix.smem_len = bo->base.size;
+
+	return 0;
+
+error:
+	drm_fb_helper_release_fbi(fbdev);
+	return r;
+}
+
+static const struct drm_fb_helper_funcs sdrm_fbdev_funcs = {
+	.fb_probe = sdrm_fbdev_probe,
+};
+
+void sdrm_fbdev_bind(struct sdrm_device *sdrm)
+{
+	struct drm_fb_helper *fbdev;
+	int r;
+
+	fbdev = kzalloc(sizeof(*fbdev), GFP_KERNEL);
+	if (!fbdev)
+		return;
+
+	drm_fb_helper_prepare(sdrm->ddev, fbdev, &sdrm_fbdev_funcs);
+
+	r = drm_fb_helper_init(sdrm->ddev, fbdev, 1, 1);
+	if (r < 0)
+		goto error;
+
+	r = drm_fb_helper_single_add_all_connectors(fbdev);
+	if (r < 0)
+		goto error;
+
+	r = drm_fb_helper_initial_config(fbdev,
+				sdrm->ddev->mode_config.preferred_depth);
+	if (r < 0)
+		goto error;
+
+	if (!fbdev->fbdev)
+		goto error;
+
+	sdrm->fbdev = fbdev;
+	return;
+
+error:
+	drm_fb_helper_fini(fbdev);
+	kfree(fbdev);
+}
+
+void sdrm_fbdev_unbind(struct sdrm_device *sdrm)
+{
+	struct drm_fb_helper *fbdev = sdrm->fbdev;
+
+	if (!fbdev)
+		return;
+
+	sdrm->fbdev = NULL;
+	drm_fb_helper_unregister_fbi(fbdev);
+	cancel_work_sync(&fbdev->dirty_work);
+	drm_fb_helper_release_fbi(fbdev);
+	drm_framebuffer_unreference(fbdev->fb);
+	fbdev->fb = NULL;
+	drm_fb_helper_fini(fbdev);
+	kfree(fbdev);
+}
diff --git a/drivers/gpu/drm/simpledrm/simpledrm_kms.c b/drivers/gpu/drm/simpledrm/simpledrm_kms.c
index 00101c9..8f67fe5 100644
--- a/drivers/gpu/drm/simpledrm/simpledrm_kms.c
+++ b/drivers/gpu/drm/simpledrm/simpledrm_kms.c
@@ -169,53 +169,60 @@  static const struct drm_framebuffer_funcs sdrm_fb_ops = {
 	.destroy		= sdrm_fb_destroy,
 };
 
-static struct drm_framebuffer *
-sdrm_fb_create(struct drm_device *ddev,
-	       struct drm_file *dfile,
-	       const struct drm_mode_fb_cmd2 *cmd)
+struct sdrm_fb *sdrm_fb_new(struct sdrm_bo *bo,
+			    const struct drm_mode_fb_cmd2 *cmd)
 {
-	struct drm_gem_object *dobj;
-	struct sdrm_fb *fb = NULL;
-	struct sdrm_bo *bo;
+	struct sdrm_fb *fb;
 	int r;
 
 	if (cmd->flags)
 		return ERR_PTR(-EINVAL);
 
-	dobj = drm_gem_object_lookup(dfile, cmd->handles[0]);
-	if (!dobj)
-		return ERR_PTR(-EINVAL);
-
-	bo = container_of(dobj, struct sdrm_bo, base);
-
 	r = sdrm_bo_vmap(bo);
 	if (r < 0)
-		goto error;
+		return ERR_PTR(r);
 
 	fb = kzalloc(sizeof(*fb), GFP_KERNEL);
-	if (!fb) {
-		r = -ENOMEM;
-		goto error;
-	}
+	if (!fb)
+		return ERR_PTR(r);
 
+	drm_gem_object_reference(&bo->base);
 	fb->bo = bo;
 	drm_helper_mode_fill_fb_struct(&fb->base, cmd);
 
-	r = drm_framebuffer_init(ddev, &fb->base, &sdrm_fb_ops);
+	r = drm_framebuffer_init(bo->base.dev, &fb->base, &sdrm_fb_ops);
 	if (r < 0)
 		goto error;
 
-	DRM_DEBUG_KMS("[FB:%d] pixel_format: %s\n", fb->base.base.id,
-		      drm_get_format_name(fb->base.pixel_format));
-
-	return &fb->base;
+	return fb;
 
 error:
+	drm_gem_object_unreference_unlocked(&bo->base);
 	kfree(fb);
-	drm_gem_object_unreference_unlocked(dobj);
 	return ERR_PTR(r);
 }
 
+static struct drm_framebuffer *
+sdrm_fb_create(struct drm_device *ddev,
+	       struct drm_file *dfile,
+	       const struct drm_mode_fb_cmd2 *cmd)
+{
+	struct drm_gem_object *dobj;
+	struct sdrm_fb *fb;
+
+	dobj = drm_gem_object_lookup(dfile, cmd->handles[0]);
+	if (!dobj)
+		return ERR_PTR(-EINVAL);
+
+	fb = sdrm_fb_new(container_of(dobj, struct sdrm_bo, base), cmd);
+	drm_gem_object_unreference_unlocked(dobj);
+
+	if (IS_ERR(fb))
+		return ERR_CAST(fb);
+
+	return &fb->base;
+}
+
 static const struct drm_mode_config_funcs sdrm_mode_config_ops = {
 	.fb_create		= sdrm_fb_create,
 	.atomic_check		= drm_atomic_helper_check,