[PATCH v2 0/4] bochs fixes and fb-helper documentation updates

[PATCH v2 0/4] bochs fixes and fb-helper documentation updates

[Peter Wu]

Hi,

These series tries to improve the bochs driver and update documentation based on
my brief experience with the fb-helper API. Thank you Daniel and Gerd for your
previous feedback and helpful suggestions.

Patch 2 was previously posted and is unmodified except for acked-by + rebase on
drm-misc-next (9a09a42369a4a37a959c051d8e1a1f948c1529a4). Patch 1 is a trivial
(build) fix that was missing last time.

Patch 3 converts from the legacy API to the modern drm_dev_register approach.
This seems required for the "generic" fbdev API as suggested by Daniel, but as
bochs does not implement the required "gem_prime_vmap" function, the conversion
cannot be completed for now.

Patch 4 includes some documentation updates that would have helped me during
qxl/bochs development and a warning that made me realize that "a virtual address
and that can be mmap'ed" in the documentation referred to "gem_prime_vmap". It
is to my best of understanding, so please correct me if I am wrong.

Side note: I originally tried to fix the unbind/remove crash in QXL and then
turned to bochs as it seemed simpler to learn how to work on DRM drivers.
Hopefully I manage to eventually figure out how to fix QXL. QXL is a bit
strange, it advertises PRIME "support" but it only has stub functions (including
a stub gem_prime_vmap).

After my recent patch ("qxl: fix null-pointer crash during suspend") qxl can
suspend/resume in the normal situation, but doing anything (suspend or unload)
after unbinding just fails (suspend then fails with "failed to wait on release
23 after spincount 301", unload triggers a use-after-free according to KASAN).
On the other hand, bochs passes these tests:

    # 1. s/r with unbound console
    modprobe bochs_drm
    echo 0 > /sys/class/vtconsole/vtcon1/bind
    rtcwake -s 1 -m mem
    # 2. s/r in normal sitation
    echo 1 > /sys/class/vtconsole/vtcon1/bind
    rtcwake -s 1 -m mem
    # 3. unload module (and s/r for good measure)
    echo 0 > /sys/class/vtconsole/vtcon1/bind
    rmmod bochs_drm
    rtcwake -s 1 -m mem

Kind regards,
Peter

Peter Wu (4):
  bochs: use drm_fb_helper_set_suspend_unlocked in suspend/resume
  bochs: convert to drm_fb_helper_fbdev_setup/teardown
  bochs: convert to drm_dev_register
  drm/fb-helper: improve documentation and print warnings

 drivers/gpu/drm/bochs/bochs.h       | 21 ++------
 drivers/gpu/drm/bochs/bochs_drv.c   | 46 +++++++++++------
 drivers/gpu/drm/bochs/bochs_fbdev.c | 79 +++++++----------------------
 drivers/gpu/drm/bochs/bochs_hw.c    |  2 +-
 drivers/gpu/drm/bochs/bochs_kms.c   |  7 +--
 drivers/gpu/drm/bochs/bochs_mm.c    | 74 ---------------------------
 drivers/gpu/drm/drm_fb_helper.c     | 25 ++++++---
 7 files changed, 73 insertions(+), 181 deletions(-)

-- 
2.18.0

[PATCH v2 0/4] bochs fixes and fb-helper documentation updates

[Peter Wu]

Hi,

These series tries to improve the bochs driver and update documentation based on
my brief experience with the fb-helper API. Thank you Daniel and Gerd for your
previous feedback and helpful suggestions.

Patch 2 was previously posted and is unmodified except for acked-by + rebase on
drm-misc-next (9a09a42369a4a37a959c051d8e1a1f948c1529a4). Patch 1 is a trivial
(build) fix that was missing last time.

Patch 3 converts from the legacy API to the modern drm_dev_register approach.
This seems required for the "generic" fbdev API as suggested by Daniel, but as
bochs does not implement the required "gem_prime_vmap" function, the conversion
cannot be completed for now.

Patch 4 includes some documentation updates that would have helped me during
qxl/bochs development and a warning that made me realize that "a virtual address
and that can be mmap'ed" in the documentation referred to "gem_prime_vmap". It
is to my best of understanding, so please correct me if I am wrong.

Side note: I originally tried to fix the unbind/remove crash in QXL and then
turned to bochs as it seemed simpler to learn how to work on DRM drivers.
Hopefully I manage to eventually figure out how to fix QXL. QXL is a bit
strange, it advertises PRIME "support" but it only has stub functions (including
a stub gem_prime_vmap).

After my recent patch ("qxl: fix null-pointer crash during suspend") qxl can
suspend/resume in the normal situation, but doing anything (suspend or unload)
after unbinding just fails (suspend then fails with "failed to wait on release
23 after spincount 301", unload triggers a use-after-free according to KASAN).
On the other hand, bochs passes these tests:

    # 1. s/r with unbound console
    modprobe bochs_drm
    echo 0 > /sys/class/vtconsole/vtcon1/bind
    rtcwake -s 1 -m mem
    # 2. s/r in normal sitation
    echo 1 > /sys/class/vtconsole/vtcon1/bind
    rtcwake -s 1 -m mem
    # 3. unload module (and s/r for good measure)
    echo 0 > /sys/class/vtconsole/vtcon1/bind
    rmmod bochs_drm
    rtcwake -s 1 -m mem

Kind regards,
Peter

Peter Wu (4):
  bochs: use drm_fb_helper_set_suspend_unlocked in suspend/resume
  bochs: convert to drm_fb_helper_fbdev_setup/teardown
  bochs: convert to drm_dev_register
  drm/fb-helper: improve documentation and print warnings

 drivers/gpu/drm/bochs/bochs.h       | 21 ++------
 drivers/gpu/drm/bochs/bochs_drv.c   | 46 +++++++++++------
 drivers/gpu/drm/bochs/bochs_fbdev.c | 79 +++++++----------------------
 drivers/gpu/drm/bochs/bochs_hw.c    |  2 +-
 drivers/gpu/drm/bochs/bochs_kms.c   |  7 +--
 drivers/gpu/drm/bochs/bochs_mm.c    | 74 ---------------------------
 drivers/gpu/drm/drm_fb_helper.c     | 25 ++++++---
 7 files changed, 73 insertions(+), 181 deletions(-)

-- 
2.18.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 1/4] bochs: use drm_fb_helper_set_suspend_unlocked in suspend/resume

[Peter Wu]

The "initialized" member is going away. suspend/resume still works (even
if bochsfb_create is forced to fail).

Signed-off-by: Peter Wu <peter@lekensteyn.nl>
---
 drivers/gpu/drm/bochs/bochs_drv.c | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/bochs/bochs_drv.c b/drivers/gpu/drm/bochs/bochs_drv.c
index c61b40c72b62..0e79d9acf89e 100644
--- a/drivers/gpu/drm/bochs/bochs_drv.c
+++ b/drivers/gpu/drm/bochs/bochs_drv.c
@@ -107,11 +107,7 @@ static int bochs_pm_suspend(struct device *dev)
 
 	drm_kms_helper_poll_disable(drm_dev);
 
-	if (bochs->fb.initialized) {
-		console_lock();
-		drm_fb_helper_set_suspend(&bochs->fb.helper, 1);
-		console_unlock();
-	}
+	drm_fb_helper_set_suspend_unlocked(&bochs->fb.helper, 1);
 
 	return 0;
 }
@@ -124,11 +120,7 @@ static int bochs_pm_resume(struct device *dev)
 
 	drm_helper_resume_force_mode(drm_dev);
 
-	if (bochs->fb.initialized) {
-		console_lock();
-		drm_fb_helper_set_suspend(&bochs->fb.helper, 0);
-		console_unlock();
-	}
+	drm_fb_helper_set_suspend_unlocked(&bochs->fb.helper, 0);
 
 	drm_kms_helper_poll_enable(drm_dev);
 	return 0;
-- 
2.18.0

[PATCH v2 2/4] bochs: convert to drm_fb_helper_fbdev_setup/teardown

[Peter Wu]

Currently unloading bochs_drm (after unbinding the vtconsole) results in
a warning about a leaked connector:

    [drm:drm_mode_config_cleanup] *ERROR* connector Virtual-3 leaked!

While investigating a potential fix I noticed that a lot of open-coded
functionality is already implemented elsewhere, so start converting it:
bochs_fbdev_init -> drm_fb_helper_fbdev_setup: trivial (similar impl).
bochs_fbdev_fini -> drm_fb_helper_fbdev_teardown: requires unembedding
"struct drm_framebuffer" from "struct bochs_framebuffer".

Unembedding drm_framebuffer is made easy using drm_gem_fbdev_fb_create
which can replace bochs_fbdev_destroy and custom routines in bochs_mm.c.
For this to work, the GEM object is moved into "drm_framebuffer". After
that, "bochs_framebuffer" is no longer needed and therefore removed.

Remove the unused "size" and "initialized" fields from fb, the latter is
not necessary as drm_fb_helper_fbdev_teardown can be called even if
bochsfb_create fails. This theory was tested by returning early and
late (just before drm_gem_fbdev_fb_create). Both scenarios fail
gracefully although the latter seems to leak the object from
bochsfb_create_object (not a regression).

Guess on the reason for the encoder leak: drm_framebuffer_cleanup was
previously used, but did not destroy much. drm_fb_helper_fbdev_teardown
is now used and calls drm_framebuffer_remove which does a bit more work.

Tested with 'echo 0 > /sys/class/vtconsole/vtcon1/bind; rmmod bochs_drm'
and also with Xorg + fbdev (startx -> xterm). The latter triggered a
warning in ttm_bo_vm_open that existed before, see
https://lkml.kernel.org/r/1464000533-13140-4-git-send-email-mstaudt@suse.de

Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Peter Wu <peter@lekensteyn.nl>
---
 drivers/gpu/drm/bochs/bochs.h       | 19 ++-----
 drivers/gpu/drm/bochs/bochs_fbdev.c | 79 +++++++----------------------
 drivers/gpu/drm/bochs/bochs_kms.c   |  7 +--
 drivers/gpu/drm/bochs/bochs_mm.c    | 74 ---------------------------
 4 files changed, 22 insertions(+), 157 deletions(-)

diff --git a/drivers/gpu/drm/bochs/bochs.h b/drivers/gpu/drm/bochs/bochs.h
index 375bf92cd04f..8514a84fbdbe 100644
--- a/drivers/gpu/drm/bochs/bochs.h
+++ b/drivers/gpu/drm/bochs/bochs.h
@@ -51,11 +51,6 @@ enum bochs_types {
 	BOCHS_UNKNOWN,
 };
 
-struct bochs_framebuffer {
-	struct drm_framebuffer base;
-	struct drm_gem_object *obj;
-};
-
 struct bochs_device {
 	/* hw */
 	void __iomem   *mmio;
@@ -88,15 +83,11 @@ struct bochs_device {
 
 	/* fbdev */
 	struct {
-		struct bochs_framebuffer gfb;
+		struct drm_framebuffer *fb;
 		struct drm_fb_helper helper;
-		int size;
-		bool initialized;
 	} fb;
 };
 
-#define to_bochs_framebuffer(x) container_of(x, struct bochs_framebuffer, base)
-
 struct bochs_bo {
 	struct ttm_buffer_object bo;
 	struct ttm_placement placement;
@@ -148,15 +139,9 @@ int bochs_dumb_create(struct drm_file *file, struct drm_device *dev,
 int bochs_dumb_mmap_offset(struct drm_file *file, struct drm_device *dev,
 			   uint32_t handle, uint64_t *offset);
 
-int bochs_framebuffer_init(struct drm_device *dev,
-			   struct bochs_framebuffer *gfb,
-			   const struct drm_mode_fb_cmd2 *mode_cmd,
-			   struct drm_gem_object *obj);
 int bochs_bo_pin(struct bochs_bo *bo, u32 pl_flag, u64 *gpu_addr);
 int bochs_bo_unpin(struct bochs_bo *bo);
 
-extern const struct drm_mode_config_funcs bochs_mode_funcs;
-
 /* bochs_kms.c */
 int bochs_kms_init(struct bochs_device *bochs);
 void bochs_kms_fini(struct bochs_device *bochs);
@@ -164,3 +149,5 @@ void bochs_kms_fini(struct bochs_device *bochs);
 /* bochs_fbdev.c */
 int bochs_fbdev_init(struct bochs_device *bochs);
 void bochs_fbdev_fini(struct bochs_device *bochs);
+
+extern const struct drm_mode_config_funcs bochs_mode_funcs;
diff --git a/drivers/gpu/drm/bochs/bochs_fbdev.c b/drivers/gpu/drm/bochs/bochs_fbdev.c
index 14eb8d0d5a00..8f4d6c052f7b 100644
--- a/drivers/gpu/drm/bochs/bochs_fbdev.c
+++ b/drivers/gpu/drm/bochs/bochs_fbdev.c
@@ -6,6 +6,7 @@
  */
 
 #include "bochs.h"
+#include <drm/drm_gem_framebuffer_helper.h>
 
 /* ---------------------------------------------------------------------- */
 
@@ -13,9 +14,7 @@ static int bochsfb_mmap(struct fb_info *info,
 			struct vm_area_struct *vma)
 {
 	struct drm_fb_helper *fb_helper = info->par;
-	struct bochs_device *bochs =
-		container_of(fb_helper, struct bochs_device, fb.helper);
-	struct bochs_bo *bo = gem_to_bochs_bo(bochs->fb.gfb.obj);
+	struct bochs_bo *bo = gem_to_bochs_bo(fb_helper->fb->obj[0]);
 
 	return ttm_fbdev_mmap(vma, &bo->bo);
 }
@@ -101,19 +100,20 @@ static int bochsfb_create(struct drm_fb_helper *helper,
 
 	/* init fb device */
 	info = drm_fb_helper_alloc_fbi(helper);
-	if (IS_ERR(info))
+	if (IS_ERR(info)) {
+		DRM_ERROR("Failed to allocate fbi: %ld\n", PTR_ERR(info));
 		return PTR_ERR(info);
+	}
 
 	info->par = &bochs->fb.helper;
 
-	ret = bochs_framebuffer_init(bochs->dev, &bochs->fb.gfb, &mode_cmd, gobj);
-	if (ret)
-		return ret;
-
-	bochs->fb.size = size;
+	fb = drm_gem_fbdev_fb_create(bochs->dev, sizes, 0, gobj, NULL);
+	if (IS_ERR(fb)) {
+		DRM_ERROR("Failed to create framebuffer: %ld\n", PTR_ERR(fb));
+		return PTR_ERR(fb);
+	}
 
 	/* setup helper */
-	fb = &bochs->fb.gfb.base;
 	bochs->fb.helper.fb = fb;
 
 	strcpy(info->fix.id, "bochsdrmfb");
@@ -130,27 +130,6 @@ static int bochsfb_create(struct drm_fb_helper *helper,
 	drm_vma_offset_remove(&bo->bo.bdev->vma_manager, &bo->bo.vma_node);
 	info->fix.smem_start = 0;
 	info->fix.smem_len = size;
-
-	bochs->fb.initialized = true;
-	return 0;
-}
-
-static int bochs_fbdev_destroy(struct bochs_device *bochs)
-{
-	struct bochs_framebuffer *gfb = &bochs->fb.gfb;
-
-	DRM_DEBUG_DRIVER("\n");
-
-	drm_fb_helper_unregister_fbi(&bochs->fb.helper);
-
-	if (gfb->obj) {
-		drm_gem_object_unreference_unlocked(gfb->obj);
-		gfb->obj = NULL;
-	}
-
-	drm_framebuffer_unregister_private(&gfb->base);
-	drm_framebuffer_cleanup(&gfb->base);
-
 	return 0;
 }
 
@@ -158,41 +137,17 @@ static const struct drm_fb_helper_funcs bochs_fb_helper_funcs = {
 	.fb_probe = bochsfb_create,
 };
 
+const struct drm_mode_config_funcs bochs_mode_funcs = {
+	.fb_create = drm_gem_fb_create,
+};
+
 int bochs_fbdev_init(struct bochs_device *bochs)
 {
-	int ret;
-
-	drm_fb_helper_prepare(bochs->dev, &bochs->fb.helper,
-			      &bochs_fb_helper_funcs);
-
-	ret = drm_fb_helper_init(bochs->dev, &bochs->fb.helper, 1);
-	if (ret)
-		return ret;
-
-	ret = drm_fb_helper_single_add_all_connectors(&bochs->fb.helper);
-	if (ret)
-		goto fini;
-
-	drm_helper_disable_unused_functions(bochs->dev);
-
-	ret = drm_fb_helper_initial_config(&bochs->fb.helper, 32);
-	if (ret)
-		goto fini;
-
-	return 0;
-
-fini:
-	drm_fb_helper_fini(&bochs->fb.helper);
-	return ret;
+	return drm_fb_helper_fbdev_setup(bochs->dev, &bochs->fb.helper,
+					 &bochs_fb_helper_funcs, 32, 1);
 }
 
 void bochs_fbdev_fini(struct bochs_device *bochs)
 {
-	if (bochs->fb.initialized)
-		bochs_fbdev_destroy(bochs);
-
-	if (bochs->fb.helper.fbdev)
-		drm_fb_helper_fini(&bochs->fb.helper);
-
-	bochs->fb.initialized = false;
+	drm_fb_helper_fbdev_teardown(bochs->dev);
 }
diff --git a/drivers/gpu/drm/bochs/bochs_kms.c b/drivers/gpu/drm/bochs/bochs_kms.c
index ca5a9afdd5cf..ea9a43d31bf1 100644
--- a/drivers/gpu/drm/bochs/bochs_kms.c
+++ b/drivers/gpu/drm/bochs/bochs_kms.c
@@ -35,14 +35,12 @@ static int bochs_crtc_mode_set_base(struct drm_crtc *crtc, int x, int y,
 {
 	struct bochs_device *bochs =
 		container_of(crtc, struct bochs_device, crtc);
-	struct bochs_framebuffer *bochs_fb;
 	struct bochs_bo *bo;
 	u64 gpu_addr = 0;
 	int ret;
 
 	if (old_fb) {
-		bochs_fb = to_bochs_framebuffer(old_fb);
-		bo = gem_to_bochs_bo(bochs_fb->obj);
+		bo = gem_to_bochs_bo(old_fb->obj[0]);
 		ret = ttm_bo_reserve(&bo->bo, true, false, NULL);
 		if (ret) {
 			DRM_ERROR("failed to reserve old_fb bo\n");
@@ -55,8 +53,7 @@ static int bochs_crtc_mode_set_base(struct drm_crtc *crtc, int x, int y,
 	if (WARN_ON(crtc->primary->fb == NULL))
 		return -EINVAL;
 
-	bochs_fb = to_bochs_framebuffer(crtc->primary->fb);
-	bo = gem_to_bochs_bo(bochs_fb->obj);
+	bo = gem_to_bochs_bo(crtc->primary->fb->obj[0]);
 	ret = ttm_bo_reserve(&bo->bo, true, false, NULL);
 	if (ret)
 		return ret;
diff --git a/drivers/gpu/drm/bochs/bochs_mm.c b/drivers/gpu/drm/bochs/bochs_mm.c
index c9c7097030ca..a61c1ecb2bdc 100644
--- a/drivers/gpu/drm/bochs/bochs_mm.c
+++ b/drivers/gpu/drm/bochs/bochs_mm.c
@@ -457,77 +457,3 @@ int bochs_dumb_mmap_offset(struct drm_file *file, struct drm_device *dev,
 	drm_gem_object_unreference_unlocked(obj);
 	return 0;
 }
-
-/* ---------------------------------------------------------------------- */
-
-static void bochs_user_framebuffer_destroy(struct drm_framebuffer *fb)
-{
-	struct bochs_framebuffer *bochs_fb = to_bochs_framebuffer(fb);
-
-	drm_gem_object_unreference_unlocked(bochs_fb->obj);
-	drm_framebuffer_cleanup(fb);
-	kfree(fb);
-}
-
-static const struct drm_framebuffer_funcs bochs_fb_funcs = {
-	.destroy = bochs_user_framebuffer_destroy,
-};
-
-int bochs_framebuffer_init(struct drm_device *dev,
-			   struct bochs_framebuffer *gfb,
-			   const struct drm_mode_fb_cmd2 *mode_cmd,
-			   struct drm_gem_object *obj)
-{
-	int ret;
-
-	drm_helper_mode_fill_fb_struct(dev, &gfb->base, mode_cmd);
-	gfb->obj = obj;
-	ret = drm_framebuffer_init(dev, &gfb->base, &bochs_fb_funcs);
-	if (ret) {
-		DRM_ERROR("drm_framebuffer_init failed: %d\n", ret);
-		return ret;
-	}
-	return 0;
-}
-
-static struct drm_framebuffer *
-bochs_user_framebuffer_create(struct drm_device *dev,
-			      struct drm_file *filp,
-			      const struct drm_mode_fb_cmd2 *mode_cmd)
-{
-	struct drm_gem_object *obj;
-	struct bochs_framebuffer *bochs_fb;
-	int ret;
-
-	DRM_DEBUG_DRIVER("%dx%d, format %c%c%c%c\n",
-	       mode_cmd->width, mode_cmd->height,
-	       (mode_cmd->pixel_format)       & 0xff,
-	       (mode_cmd->pixel_format >> 8)  & 0xff,
-	       (mode_cmd->pixel_format >> 16) & 0xff,
-	       (mode_cmd->pixel_format >> 24) & 0xff);
-
-	if (mode_cmd->pixel_format != DRM_FORMAT_XRGB8888)
-		return ERR_PTR(-ENOENT);
-
-	obj = drm_gem_object_lookup(filp, mode_cmd->handles[0]);
-	if (obj == NULL)
-		return ERR_PTR(-ENOENT);
-
-	bochs_fb = kzalloc(sizeof(*bochs_fb), GFP_KERNEL);
-	if (!bochs_fb) {
-		drm_gem_object_unreference_unlocked(obj);
-		return ERR_PTR(-ENOMEM);
-	}
-
-	ret = bochs_framebuffer_init(dev, bochs_fb, mode_cmd, obj);
-	if (ret) {
-		drm_gem_object_unreference_unlocked(obj);
-		kfree(bochs_fb);
-		return ERR_PTR(ret);
-	}
-	return &bochs_fb->base;
-}
-
-const struct drm_mode_config_funcs bochs_mode_funcs = {
-	.fb_create = bochs_user_framebuffer_create,
-};
-- 
2.18.0

[PATCH v2 2/4] bochs: convert to drm_fb_helper_fbdev_setup/teardown

[Peter Wu]

Currently unloading bochs_drm (after unbinding the vtconsole) results in
a warning about a leaked connector:

    [drm:drm_mode_config_cleanup] *ERROR* connector Virtual-3 leaked!

While investigating a potential fix I noticed that a lot of open-coded
functionality is already implemented elsewhere, so start converting it:
bochs_fbdev_init -> drm_fb_helper_fbdev_setup: trivial (similar impl).
bochs_fbdev_fini -> drm_fb_helper_fbdev_teardown: requires unembedding
"struct drm_framebuffer" from "struct bochs_framebuffer".

Unembedding drm_framebuffer is made easy using drm_gem_fbdev_fb_create
which can replace bochs_fbdev_destroy and custom routines in bochs_mm.c.
For this to work, the GEM object is moved into "drm_framebuffer". After
that, "bochs_framebuffer" is no longer needed and therefore removed.

Remove the unused "size" and "initialized" fields from fb, the latter is
not necessary as drm_fb_helper_fbdev_teardown can be called even if
bochsfb_create fails. This theory was tested by returning early and
late (just before drm_gem_fbdev_fb_create). Both scenarios fail
gracefully although the latter seems to leak the object from
bochsfb_create_object (not a regression).

Guess on the reason for the encoder leak: drm_framebuffer_cleanup was
previously used, but did not destroy much. drm_fb_helper_fbdev_teardown
is now used and calls drm_framebuffer_remove which does a bit more work.

Tested with 'echo 0 > /sys/class/vtconsole/vtcon1/bind; rmmod bochs_drm'
and also with Xorg + fbdev (startx -> xterm). The latter triggered a
warning in ttm_bo_vm_open that existed before, see
https://lkml.kernel.org/r/1464000533-13140-4-git-send-email-mstaudt@suse.de

Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Peter Wu <peter@lekensteyn.nl>
---
 drivers/gpu/drm/bochs/bochs.h       | 19 ++-----
 drivers/gpu/drm/bochs/bochs_fbdev.c | 79 +++++++----------------------
 drivers/gpu/drm/bochs/bochs_kms.c   |  7 +--
 drivers/gpu/drm/bochs/bochs_mm.c    | 74 ---------------------------
 4 files changed, 22 insertions(+), 157 deletions(-)

diff --git a/drivers/gpu/drm/bochs/bochs.h b/drivers/gpu/drm/bochs/bochs.h
index 375bf92cd04f..8514a84fbdbe 100644
--- a/drivers/gpu/drm/bochs/bochs.h
+++ b/drivers/gpu/drm/bochs/bochs.h
@@ -51,11 +51,6 @@ enum bochs_types {
 	BOCHS_UNKNOWN,
 };
 
-struct bochs_framebuffer {
-	struct drm_framebuffer base;
-	struct drm_gem_object *obj;
-};
-
 struct bochs_device {
 	/* hw */
 	void __iomem   *mmio;
@@ -88,15 +83,11 @@ struct bochs_device {
 
 	/* fbdev */
 	struct {
-		struct bochs_framebuffer gfb;
+		struct drm_framebuffer *fb;
 		struct drm_fb_helper helper;
-		int size;
-		bool initialized;
 	} fb;
 };
 
-#define to_bochs_framebuffer(x) container_of(x, struct bochs_framebuffer, base)
-
 struct bochs_bo {
 	struct ttm_buffer_object bo;
 	struct ttm_placement placement;
@@ -148,15 +139,9 @@ int bochs_dumb_create(struct drm_file *file, struct drm_device *dev,
 int bochs_dumb_mmap_offset(struct drm_file *file, struct drm_device *dev,
 			   uint32_t handle, uint64_t *offset);
 
-int bochs_framebuffer_init(struct drm_device *dev,
-			   struct bochs_framebuffer *gfb,
-			   const struct drm_mode_fb_cmd2 *mode_cmd,
-			   struct drm_gem_object *obj);
 int bochs_bo_pin(struct bochs_bo *bo, u32 pl_flag, u64 *gpu_addr);
 int bochs_bo_unpin(struct bochs_bo *bo);
 
-extern const struct drm_mode_config_funcs bochs_mode_funcs;
-
 /* bochs_kms.c */
 int bochs_kms_init(struct bochs_device *bochs);
 void bochs_kms_fini(struct bochs_device *bochs);
@@ -164,3 +149,5 @@ void bochs_kms_fini(struct bochs_device *bochs);
 /* bochs_fbdev.c */
 int bochs_fbdev_init(struct bochs_device *bochs);
 void bochs_fbdev_fini(struct bochs_device *bochs);
+
+extern const struct drm_mode_config_funcs bochs_mode_funcs;
diff --git a/drivers/gpu/drm/bochs/bochs_fbdev.c b/drivers/gpu/drm/bochs/bochs_fbdev.c
index 14eb8d0d5a00..8f4d6c052f7b 100644
--- a/drivers/gpu/drm/bochs/bochs_fbdev.c
+++ b/drivers/gpu/drm/bochs/bochs_fbdev.c
@@ -6,6 +6,7 @@
  */
 
 #include "bochs.h"
+#include <drm/drm_gem_framebuffer_helper.h>
 
 /* ---------------------------------------------------------------------- */
 
@@ -13,9 +14,7 @@ static int bochsfb_mmap(struct fb_info *info,
 			struct vm_area_struct *vma)
 {
 	struct drm_fb_helper *fb_helper = info->par;
-	struct bochs_device *bochs =
-		container_of(fb_helper, struct bochs_device, fb.helper);
-	struct bochs_bo *bo = gem_to_bochs_bo(bochs->fb.gfb.obj);
+	struct bochs_bo *bo = gem_to_bochs_bo(fb_helper->fb->obj[0]);
 
 	return ttm_fbdev_mmap(vma, &bo->bo);
 }
@@ -101,19 +100,20 @@ static int bochsfb_create(struct drm_fb_helper *helper,
 
 	/* init fb device */
 	info = drm_fb_helper_alloc_fbi(helper);
-	if (IS_ERR(info))
+	if (IS_ERR(info)) {
+		DRM_ERROR("Failed to allocate fbi: %ld\n", PTR_ERR(info));
 		return PTR_ERR(info);
+	}
 
 	info->par = &bochs->fb.helper;
 
-	ret = bochs_framebuffer_init(bochs->dev, &bochs->fb.gfb, &mode_cmd, gobj);
-	if (ret)
-		return ret;
-
-	bochs->fb.size = size;
+	fb = drm_gem_fbdev_fb_create(bochs->dev, sizes, 0, gobj, NULL);
+	if (IS_ERR(fb)) {
+		DRM_ERROR("Failed to create framebuffer: %ld\n", PTR_ERR(fb));
+		return PTR_ERR(fb);
+	}
 
 	/* setup helper */
-	fb = &bochs->fb.gfb.base;
 	bochs->fb.helper.fb = fb;
 
 	strcpy(info->fix.id, "bochsdrmfb");
@@ -130,27 +130,6 @@ static int bochsfb_create(struct drm_fb_helper *helper,
 	drm_vma_offset_remove(&bo->bo.bdev->vma_manager, &bo->bo.vma_node);
 	info->fix.smem_start = 0;
 	info->fix.smem_len = size;
-
-	bochs->fb.initialized = true;
-	return 0;
-}
-
-static int bochs_fbdev_destroy(struct bochs_device *bochs)
-{
-	struct bochs_framebuffer *gfb = &bochs->fb.gfb;
-
-	DRM_DEBUG_DRIVER("\n");
-
-	drm_fb_helper_unregister_fbi(&bochs->fb.helper);
-
-	if (gfb->obj) {
-		drm_gem_object_unreference_unlocked(gfb->obj);
-		gfb->obj = NULL;
-	}
-
-	drm_framebuffer_unregister_private(&gfb->base);
-	drm_framebuffer_cleanup(&gfb->base);
-
 	return 0;
 }
 
@@ -158,41 +137,17 @@ static const struct drm_fb_helper_funcs bochs_fb_helper_funcs = {
 	.fb_probe = bochsfb_create,
 };
 
+const struct drm_mode_config_funcs bochs_mode_funcs = {
+	.fb_create = drm_gem_fb_create,
+};
+
 int bochs_fbdev_init(struct bochs_device *bochs)
 {
-	int ret;
-
-	drm_fb_helper_prepare(bochs->dev, &bochs->fb.helper,
-			      &bochs_fb_helper_funcs);
-
-	ret = drm_fb_helper_init(bochs->dev, &bochs->fb.helper, 1);
-	if (ret)
-		return ret;
-
-	ret = drm_fb_helper_single_add_all_connectors(&bochs->fb.helper);
-	if (ret)
-		goto fini;
-
-	drm_helper_disable_unused_functions(bochs->dev);
-
-	ret = drm_fb_helper_initial_config(&bochs->fb.helper, 32);
-	if (ret)
-		goto fini;
-
-	return 0;
-
-fini:
-	drm_fb_helper_fini(&bochs->fb.helper);
-	return ret;
+	return drm_fb_helper_fbdev_setup(bochs->dev, &bochs->fb.helper,
+					 &bochs_fb_helper_funcs, 32, 1);
 }
 
 void bochs_fbdev_fini(struct bochs_device *bochs)
 {
-	if (bochs->fb.initialized)
-		bochs_fbdev_destroy(bochs);
-
-	if (bochs->fb.helper.fbdev)
-		drm_fb_helper_fini(&bochs->fb.helper);
-
-	bochs->fb.initialized = false;
+	drm_fb_helper_fbdev_teardown(bochs->dev);
 }
diff --git a/drivers/gpu/drm/bochs/bochs_kms.c b/drivers/gpu/drm/bochs/bochs_kms.c
index ca5a9afdd5cf..ea9a43d31bf1 100644
--- a/drivers/gpu/drm/bochs/bochs_kms.c
+++ b/drivers/gpu/drm/bochs/bochs_kms.c
@@ -35,14 +35,12 @@ static int bochs_crtc_mode_set_base(struct drm_crtc *crtc, int x, int y,
 {
 	struct bochs_device *bochs =
 		container_of(crtc, struct bochs_device, crtc);
-	struct bochs_framebuffer *bochs_fb;
 	struct bochs_bo *bo;
 	u64 gpu_addr = 0;
 	int ret;
 
 	if (old_fb) {
-		bochs_fb = to_bochs_framebuffer(old_fb);
-		bo = gem_to_bochs_bo(bochs_fb->obj);
+		bo = gem_to_bochs_bo(old_fb->obj[0]);
 		ret = ttm_bo_reserve(&bo->bo, true, false, NULL);
 		if (ret) {
 			DRM_ERROR("failed to reserve old_fb bo\n");
@@ -55,8 +53,7 @@ static int bochs_crtc_mode_set_base(struct drm_crtc *crtc, int x, int y,
 	if (WARN_ON(crtc->primary->fb == NULL))
 		return -EINVAL;
 
-	bochs_fb = to_bochs_framebuffer(crtc->primary->fb);
-	bo = gem_to_bochs_bo(bochs_fb->obj);
+	bo = gem_to_bochs_bo(crtc->primary->fb->obj[0]);
 	ret = ttm_bo_reserve(&bo->bo, true, false, NULL);
 	if (ret)
 		return ret;
diff --git a/drivers/gpu/drm/bochs/bochs_mm.c b/drivers/gpu/drm/bochs/bochs_mm.c
index c9c7097030ca..a61c1ecb2bdc 100644
--- a/drivers/gpu/drm/bochs/bochs_mm.c
+++ b/drivers/gpu/drm/bochs/bochs_mm.c
@@ -457,77 +457,3 @@ int bochs_dumb_mmap_offset(struct drm_file *file, struct drm_device *dev,
 	drm_gem_object_unreference_unlocked(obj);
 	return 0;
 }
-
-/* ---------------------------------------------------------------------- */
-
-static void bochs_user_framebuffer_destroy(struct drm_framebuffer *fb)
-{
-	struct bochs_framebuffer *bochs_fb = to_bochs_framebuffer(fb);
-
-	drm_gem_object_unreference_unlocked(bochs_fb->obj);
-	drm_framebuffer_cleanup(fb);
-	kfree(fb);
-}
-
-static const struct drm_framebuffer_funcs bochs_fb_funcs = {
-	.destroy = bochs_user_framebuffer_destroy,
-};
-
-int bochs_framebuffer_init(struct drm_device *dev,
-			   struct bochs_framebuffer *gfb,
-			   const struct drm_mode_fb_cmd2 *mode_cmd,
-			   struct drm_gem_object *obj)
-{
-	int ret;
-
-	drm_helper_mode_fill_fb_struct(dev, &gfb->base, mode_cmd);
-	gfb->obj = obj;
-	ret = drm_framebuffer_init(dev, &gfb->base, &bochs_fb_funcs);
-	if (ret) {
-		DRM_ERROR("drm_framebuffer_init failed: %d\n", ret);
-		return ret;
-	}
-	return 0;
-}
-
-static struct drm_framebuffer *
-bochs_user_framebuffer_create(struct drm_device *dev,
-			      struct drm_file *filp,
-			      const struct drm_mode_fb_cmd2 *mode_cmd)
-{
-	struct drm_gem_object *obj;
-	struct bochs_framebuffer *bochs_fb;
-	int ret;
-
-	DRM_DEBUG_DRIVER("%dx%d, format %c%c%c%c\n",
-	       mode_cmd->width, mode_cmd->height,
-	       (mode_cmd->pixel_format)       & 0xff,
-	       (mode_cmd->pixel_format >> 8)  & 0xff,
-	       (mode_cmd->pixel_format >> 16) & 0xff,
-	       (mode_cmd->pixel_format >> 24) & 0xff);
-
-	if (mode_cmd->pixel_format != DRM_FORMAT_XRGB8888)
-		return ERR_PTR(-ENOENT);
-
-	obj = drm_gem_object_lookup(filp, mode_cmd->handles[0]);
-	if (obj == NULL)
-		return ERR_PTR(-ENOENT);
-
-	bochs_fb = kzalloc(sizeof(*bochs_fb), GFP_KERNEL);
-	if (!bochs_fb) {
-		drm_gem_object_unreference_unlocked(obj);
-		return ERR_PTR(-ENOMEM);
-	}
-
-	ret = bochs_framebuffer_init(dev, bochs_fb, mode_cmd, obj);
-	if (ret) {
-		drm_gem_object_unreference_unlocked(obj);
-		kfree(bochs_fb);
-		return ERR_PTR(ret);
-	}
-	return &bochs_fb->base;
-}
-
-const struct drm_mode_config_funcs bochs_mode_funcs = {
-	.fb_create = bochs_user_framebuffer_create,
-};
-- 
2.18.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 3/4] bochs: convert to drm_dev_register

[Peter Wu]

The drm_get_pci_dev API is deprecated, replace it by drm_dev_register.

Signed-off-by: Peter Wu <peter@lekensteyn.nl>
---
 drivers/gpu/drm/bochs/bochs.h     |  2 +-
 drivers/gpu/drm/bochs/bochs_drv.c | 34 +++++++++++++++++++++++++------
 drivers/gpu/drm/bochs/bochs_hw.c  |  2 +-
 3 files changed, 30 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/bochs/bochs.h b/drivers/gpu/drm/bochs/bochs.h
index 8514a84fbdbe..b4f6bb521900 100644
--- a/drivers/gpu/drm/bochs/bochs.h
+++ b/drivers/gpu/drm/bochs/bochs.h
@@ -117,7 +117,7 @@ static inline u64 bochs_bo_mmap_offset(struct bochs_bo *bo)
 /* ---------------------------------------------------------------------- */
 
 /* bochs_hw.c */
-int bochs_hw_init(struct drm_device *dev, uint32_t flags);
+int bochs_hw_init(struct drm_device *dev);
 void bochs_hw_fini(struct drm_device *dev);
 
 void bochs_hw_setmode(struct bochs_device *bochs,
diff --git a/drivers/gpu/drm/bochs/bochs_drv.c b/drivers/gpu/drm/bochs/bochs_drv.c
index 0e79d9acf89e..f3dd66ae990a 100644
--- a/drivers/gpu/drm/bochs/bochs_drv.c
+++ b/drivers/gpu/drm/bochs/bochs_drv.c
@@ -35,7 +35,7 @@ static void bochs_unload(struct drm_device *dev)
 	dev->dev_private = NULL;
 }
 
-static int bochs_load(struct drm_device *dev, unsigned long flags)
+static int bochs_load(struct drm_device *dev)
 {
 	struct bochs_device *bochs;
 	int ret;
@@ -46,7 +46,7 @@ static int bochs_load(struct drm_device *dev, unsigned long flags)
 	dev->dev_private = bochs;
 	bochs->dev = dev;
 
-	ret = bochs_hw_init(dev, flags);
+	ret = bochs_hw_init(dev);
 	if (ret)
 		goto err;
 
@@ -82,8 +82,6 @@ static const struct file_operations bochs_fops = {
 
 static struct drm_driver bochs_driver = {
 	.driver_features	= DRIVER_GEM | DRIVER_MODESET,
-	.load			= bochs_load,
-	.unload			= bochs_unload,
 	.fops			= &bochs_fops,
 	.name			= "bochs-drm",
 	.desc			= "bochs dispi vga interface (qemu stdvga)",
@@ -138,6 +136,7 @@ static const struct dev_pm_ops bochs_pm_ops = {
 static int bochs_pci_probe(struct pci_dev *pdev,
 			   const struct pci_device_id *ent)
 {
+	struct drm_device *dev;
 	unsigned long fbsize;
 	int ret;
 
@@ -151,14 +150,37 @@ static int bochs_pci_probe(struct pci_dev *pdev,
 	if (ret)
 		return ret;
 
-	return drm_get_pci_dev(pdev, ent, &bochs_driver);
+	dev = drm_dev_alloc(&bochs_driver, &pdev->dev);
+	if (IS_ERR(dev))
+		return PTR_ERR(dev);
+
+	dev->pdev = pdev;
+	pci_set_drvdata(pdev, dev);
+
+	ret = bochs_load(dev);
+	if (ret)
+		goto err_free_dev;
+
+	ret = drm_dev_register(dev, 0);
+	if (ret)
+		goto err_unload;
+
+	return ret;
+
+err_unload:
+	bochs_unload(dev);
+err_free_dev:
+	drm_dev_put(dev);
+	return ret;
 }
 
 static void bochs_pci_remove(struct pci_dev *pdev)
 {
 	struct drm_device *dev = pci_get_drvdata(pdev);
 
-	drm_put_dev(dev);
+	drm_dev_unregister(dev);
+	bochs_unload(dev);
+	drm_dev_put(dev);
 }
 
 static const struct pci_device_id bochs_pci_tbl[] = {
diff --git a/drivers/gpu/drm/bochs/bochs_hw.c b/drivers/gpu/drm/bochs/bochs_hw.c
index a39b0343c197..16e4f1caccca 100644
--- a/drivers/gpu/drm/bochs/bochs_hw.c
+++ b/drivers/gpu/drm/bochs/bochs_hw.c
@@ -47,7 +47,7 @@ static void bochs_dispi_write(struct bochs_device *bochs, u16 reg, u16 val)
 	}
 }
 
-int bochs_hw_init(struct drm_device *dev, uint32_t flags)
+int bochs_hw_init(struct drm_device *dev)
 {
 	struct bochs_device *bochs = dev->dev_private;
 	struct pci_dev *pdev = dev->pdev;
-- 
2.18.0

[PATCH v2 3/4] bochs: convert to drm_dev_register

[Peter Wu]

The drm_get_pci_dev API is deprecated, replace it by drm_dev_register.

Signed-off-by: Peter Wu <peter@lekensteyn.nl>
---
 drivers/gpu/drm/bochs/bochs.h     |  2 +-
 drivers/gpu/drm/bochs/bochs_drv.c | 34 +++++++++++++++++++++++++------
 drivers/gpu/drm/bochs/bochs_hw.c  |  2 +-
 3 files changed, 30 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/bochs/bochs.h b/drivers/gpu/drm/bochs/bochs.h
index 8514a84fbdbe..b4f6bb521900 100644
--- a/drivers/gpu/drm/bochs/bochs.h
+++ b/drivers/gpu/drm/bochs/bochs.h
@@ -117,7 +117,7 @@ static inline u64 bochs_bo_mmap_offset(struct bochs_bo *bo)
 /* ---------------------------------------------------------------------- */
 
 /* bochs_hw.c */
-int bochs_hw_init(struct drm_device *dev, uint32_t flags);
+int bochs_hw_init(struct drm_device *dev);
 void bochs_hw_fini(struct drm_device *dev);
 
 void bochs_hw_setmode(struct bochs_device *bochs,
diff --git a/drivers/gpu/drm/bochs/bochs_drv.c b/drivers/gpu/drm/bochs/bochs_drv.c
index 0e79d9acf89e..f3dd66ae990a 100644
--- a/drivers/gpu/drm/bochs/bochs_drv.c
+++ b/drivers/gpu/drm/bochs/bochs_drv.c
@@ -35,7 +35,7 @@ static void bochs_unload(struct drm_device *dev)
 	dev->dev_private = NULL;
 }
 
-static int bochs_load(struct drm_device *dev, unsigned long flags)
+static int bochs_load(struct drm_device *dev)
 {
 	struct bochs_device *bochs;
 	int ret;
@@ -46,7 +46,7 @@ static int bochs_load(struct drm_device *dev, unsigned long flags)
 	dev->dev_private = bochs;
 	bochs->dev = dev;
 
-	ret = bochs_hw_init(dev, flags);
+	ret = bochs_hw_init(dev);
 	if (ret)
 		goto err;
 
@@ -82,8 +82,6 @@ static const struct file_operations bochs_fops = {
 
 static struct drm_driver bochs_driver = {
 	.driver_features	= DRIVER_GEM | DRIVER_MODESET,
-	.load			= bochs_load,
-	.unload			= bochs_unload,
 	.fops			= &bochs_fops,
 	.name			= "bochs-drm",
 	.desc			= "bochs dispi vga interface (qemu stdvga)",
@@ -138,6 +136,7 @@ static const struct dev_pm_ops bochs_pm_ops = {
 static int bochs_pci_probe(struct pci_dev *pdev,
 			   const struct pci_device_id *ent)
 {
+	struct drm_device *dev;
 	unsigned long fbsize;
 	int ret;
 
@@ -151,14 +150,37 @@ static int bochs_pci_probe(struct pci_dev *pdev,
 	if (ret)
 		return ret;
 
-	return drm_get_pci_dev(pdev, ent, &bochs_driver);
+	dev = drm_dev_alloc(&bochs_driver, &pdev->dev);
+	if (IS_ERR(dev))
+		return PTR_ERR(dev);
+
+	dev->pdev = pdev;
+	pci_set_drvdata(pdev, dev);
+
+	ret = bochs_load(dev);
+	if (ret)
+		goto err_free_dev;
+
+	ret = drm_dev_register(dev, 0);
+	if (ret)
+		goto err_unload;
+
+	return ret;
+
+err_unload:
+	bochs_unload(dev);
+err_free_dev:
+	drm_dev_put(dev);
+	return ret;
 }
 
 static void bochs_pci_remove(struct pci_dev *pdev)
 {
 	struct drm_device *dev = pci_get_drvdata(pdev);
 
-	drm_put_dev(dev);
+	drm_dev_unregister(dev);
+	bochs_unload(dev);
+	drm_dev_put(dev);
 }
 
 static const struct pci_device_id bochs_pci_tbl[] = {
diff --git a/drivers/gpu/drm/bochs/bochs_hw.c b/drivers/gpu/drm/bochs/bochs_hw.c
index a39b0343c197..16e4f1caccca 100644
--- a/drivers/gpu/drm/bochs/bochs_hw.c
+++ b/drivers/gpu/drm/bochs/bochs_hw.c
@@ -47,7 +47,7 @@ static void bochs_dispi_write(struct bochs_device *bochs, u16 reg, u16 val)
 	}
 }
 
-int bochs_hw_init(struct drm_device *dev, uint32_t flags)
+int bochs_hw_init(struct drm_device *dev)
 {
 	struct bochs_device *bochs = dev->dev_private;
 	struct pci_dev *pdev = dev->pdev;
-- 
2.18.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 4/4] drm/fb-helper: improve documentation and print warnings

[Peter Wu]

Clarify the relation between drm_fb_helper_fbdev_setup/teardown. Clarify
requirements for the new generic fbdev emulation API and log some more
details in case the driver does something wrong. Fix related typos.

Cc: Noralf Trønnes <noralf@tronnes.org>
Signed-off-by: Peter Wu <peter@lekensteyn.nl>
---
 drivers/gpu/drm/drm_fb_helper.c | 25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 4b0dd20bccb8..7f92ff173986 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -2821,7 +2821,9 @@ EXPORT_SYMBOL(drm_fb_helper_hotplug_event);
  * The caller must to provide a &drm_fb_helper_funcs->fb_probe callback
  * function.
  *
- * See also: drm_fb_helper_initial_config()
+ * Use drm_fb_helper_fbdev_teardown() to destroy the fbdev.
+ *
+ * See also: drm_fb_helper_initial_config(), drm_fbdev_generic_setup().
  *
  * Returns:
  * Zero on success or negative error code on failure.
@@ -3037,7 +3039,7 @@ static struct fb_deferred_io drm_fbdev_defio = {
  * @fb_helper: fbdev helper structure
  * @sizes: describes fbdev size and scanout surface size
  *
- * This function uses the client API to crate a framebuffer backed by a dumb buffer.
+ * This function uses the client API to create a framebuffer backed by a dumb buffer.
  *
  * The _sys_ versions are used for &fb_ops.fb_read, fb_write, fb_fillrect,
  * fb_copyarea, fb_imageblit.
@@ -3165,8 +3167,10 @@ static int drm_fbdev_client_hotplug(struct drm_client_dev *client)
 	if (dev->fb_helper)
 		return drm_fb_helper_hotplug_event(dev->fb_helper);
 
-	if (!dev->mode_config.num_connector)
+	if (!dev->mode_config.num_connector) {
+		DRM_DEV_DEBUG(dev->dev, "No connectors found, will not create framebuffer!\n");
 		return 0;
+	}
 
 	ret = drm_fb_helper_fbdev_setup(dev, fb_helper, &drm_fb_helper_generic_funcs,
 					fb_helper->preferred_bpp, 0);
@@ -3187,13 +3191,15 @@ static const struct drm_client_funcs drm_fbdev_client_funcs = {
 };
 
 /**
- * drm_fb_helper_generic_fbdev_setup() - Setup generic fbdev emulation
+ * drm_fbdev_generic_setup() - Setup generic fbdev emulation
  * @dev: DRM device
  * @preferred_bpp: Preferred bits per pixel for the device.
  *                 @dev->mode_config.preferred_depth is used if this is zero.
  *
  * This function sets up generic fbdev emulation for drivers that supports
- * dumb buffers with a virtual address and that can be mmap'ed.
+ * dumb buffers with a virtual address and that can be mmap'ed. If the driver
+ * does not support these functions, it could use drm_fb_helper_fbdev_setup().
+ * This function can only be used with devices created using drm_dev_register().
  *
  * Restore, hotplug events and teardown are all taken care of. Drivers that do
  * suspend/resume need to call drm_fb_helper_set_suspend_unlocked() themselves.
@@ -3206,6 +3212,8 @@ static const struct drm_client_funcs drm_fbdev_client_funcs = {
  * This function is safe to call even when there are no connectors present.
  * Setup will be retried on the next hotplug event.
  *
+ * The fbdev is destroyed by drm_dev_unregister().
+ *
  * Returns:
  * Zero on success or negative error code on failure.
  */
@@ -3214,6 +3222,8 @@ int drm_fbdev_generic_setup(struct drm_device *dev, unsigned int preferred_bpp)
 	struct drm_fb_helper *fb_helper;
 	int ret;
 
+	WARN(dev->fb_helper, "fb_helper is already set!\n");
+
 	if (!drm_fbdev_emulation)
 		return 0;
 
@@ -3224,12 +3234,15 @@ int drm_fbdev_generic_setup(struct drm_device *dev, unsigned int preferred_bpp)
 	ret = drm_client_new(dev, &fb_helper->client, "fbdev", &drm_fbdev_client_funcs);
 	if (ret) {
 		kfree(fb_helper);
+		DRM_DEV_ERROR(dev->dev, "Failed to register client: %d\n", ret);
 		return ret;
 	}
 
 	fb_helper->preferred_bpp = preferred_bpp;
 
-	drm_fbdev_client_hotplug(&fb_helper->client);
+	ret = drm_fbdev_client_hotplug(&fb_helper->client);
+	if (ret)
+		DRM_DEV_DEBUG(dev->dev, "client hotplug ret=%d\n", ret);
 
 	return 0;
 }
-- 
2.18.0

[PATCH v2 4/4] drm/fb-helper: improve documentation and print warnings

[Peter Wu]

Clarify the relation between drm_fb_helper_fbdev_setup/teardown. Clarify
requirements for the new generic fbdev emulation API and log some more
details in case the driver does something wrong. Fix related typos.

Cc: Noralf Trønnes <noralf@tronnes.org>
Signed-off-by: Peter Wu <peter@lekensteyn.nl>
---
 drivers/gpu/drm/drm_fb_helper.c | 25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 4b0dd20bccb8..7f92ff173986 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -2821,7 +2821,9 @@ EXPORT_SYMBOL(drm_fb_helper_hotplug_event);
  * The caller must to provide a &drm_fb_helper_funcs->fb_probe callback
  * function.
  *
- * See also: drm_fb_helper_initial_config()
+ * Use drm_fb_helper_fbdev_teardown() to destroy the fbdev.
+ *
+ * See also: drm_fb_helper_initial_config(), drm_fbdev_generic_setup().
  *
  * Returns:
  * Zero on success or negative error code on failure.
@@ -3037,7 +3039,7 @@ static struct fb_deferred_io drm_fbdev_defio = {
  * @fb_helper: fbdev helper structure
  * @sizes: describes fbdev size and scanout surface size
  *
- * This function uses the client API to crate a framebuffer backed by a dumb buffer.
+ * This function uses the client API to create a framebuffer backed by a dumb buffer.
  *
  * The _sys_ versions are used for &fb_ops.fb_read, fb_write, fb_fillrect,
  * fb_copyarea, fb_imageblit.
@@ -3165,8 +3167,10 @@ static int drm_fbdev_client_hotplug(struct drm_client_dev *client)
 	if (dev->fb_helper)
 		return drm_fb_helper_hotplug_event(dev->fb_helper);
 
-	if (!dev->mode_config.num_connector)
+	if (!dev->mode_config.num_connector) {
+		DRM_DEV_DEBUG(dev->dev, "No connectors found, will not create framebuffer!\n");
 		return 0;
+	}
 
 	ret = drm_fb_helper_fbdev_setup(dev, fb_helper, &drm_fb_helper_generic_funcs,
 					fb_helper->preferred_bpp, 0);
@@ -3187,13 +3191,15 @@ static const struct drm_client_funcs drm_fbdev_client_funcs = {
 };
 
 /**
- * drm_fb_helper_generic_fbdev_setup() - Setup generic fbdev emulation
+ * drm_fbdev_generic_setup() - Setup generic fbdev emulation
  * @dev: DRM device
  * @preferred_bpp: Preferred bits per pixel for the device.
  *                 @dev->mode_config.preferred_depth is used if this is zero.
  *
  * This function sets up generic fbdev emulation for drivers that supports
- * dumb buffers with a virtual address and that can be mmap'ed.
+ * dumb buffers with a virtual address and that can be mmap'ed. If the driver
+ * does not support these functions, it could use drm_fb_helper_fbdev_setup().
+ * This function can only be used with devices created using drm_dev_register().
  *
  * Restore, hotplug events and teardown are all taken care of. Drivers that do
  * suspend/resume need to call drm_fb_helper_set_suspend_unlocked() themselves.
@@ -3206,6 +3212,8 @@ static const struct drm_client_funcs drm_fbdev_client_funcs = {
  * This function is safe to call even when there are no connectors present.
  * Setup will be retried on the next hotplug event.
  *
+ * The fbdev is destroyed by drm_dev_unregister().
+ *
  * Returns:
  * Zero on success or negative error code on failure.
  */
@@ -3214,6 +3222,8 @@ int drm_fbdev_generic_setup(struct drm_device *dev, unsigned int preferred_bpp)
 	struct drm_fb_helper *fb_helper;
 	int ret;
 
+	WARN(dev->fb_helper, "fb_helper is already set!\n");
+
 	if (!drm_fbdev_emulation)
 		return 0;
 
@@ -3224,12 +3234,15 @@ int drm_fbdev_generic_setup(struct drm_device *dev, unsigned int preferred_bpp)
 	ret = drm_client_new(dev, &fb_helper->client, "fbdev", &drm_fbdev_client_funcs);
 	if (ret) {
 		kfree(fb_helper);
+		DRM_DEV_ERROR(dev->dev, "Failed to register client: %d\n", ret);
 		return ret;
 	}
 
 	fb_helper->preferred_bpp = preferred_bpp;
 
-	drm_fbdev_client_hotplug(&fb_helper->client);
+	ret = drm_fbdev_client_hotplug(&fb_helper->client);
+	if (ret)
+		DRM_DEV_DEBUG(dev->dev, "client hotplug ret=%d\n", ret);
 
 	return 0;
 }
-- 
2.18.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 4/4] drm/fb-helper: improve documentation and print warnings

[Daniel Vetter]

On Fri, Sep 07, 2018 at 12:18:10AM +0200, Peter Wu wrote:
> Clarify the relation between drm_fb_helper_fbdev_setup/teardown. Clarify
> requirements for the new generic fbdev emulation API and log some more
> details in case the driver does something wrong. Fix related typos.
> 
> Cc: Noralf Trønnes <noralf@tronnes.org>
> Signed-off-by: Peter Wu <peter@lekensteyn.nl>
> ---
>  drivers/gpu/drm/drm_fb_helper.c | 25 +++++++++++++++++++------
>  1 file changed, 19 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 4b0dd20bccb8..7f92ff173986 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -2821,7 +2821,9 @@ EXPORT_SYMBOL(drm_fb_helper_hotplug_event);
>   * The caller must to provide a &drm_fb_helper_funcs->fb_probe callback
>   * function.
>   *
> - * See also: drm_fb_helper_initial_config()
> + * Use drm_fb_helper_fbdev_teardown() to destroy the fbdev.
> + *
> + * See also: drm_fb_helper_initial_config(), drm_fbdev_generic_setup().
>   *
>   * Returns:
>   * Zero on success or negative error code on failure.
> @@ -3037,7 +3039,7 @@ static struct fb_deferred_io drm_fbdev_defio = {
>   * @fb_helper: fbdev helper structure
>   * @sizes: describes fbdev size and scanout surface size
>   *
> - * This function uses the client API to crate a framebuffer backed by a dumb buffer.
> + * This function uses the client API to create a framebuffer backed by a dumb buffer.
>   *
>   * The _sys_ versions are used for &fb_ops.fb_read, fb_write, fb_fillrect,
>   * fb_copyarea, fb_imageblit.
> @@ -3165,8 +3167,10 @@ static int drm_fbdev_client_hotplug(struct drm_client_dev *client)
>  	if (dev->fb_helper)
>  		return drm_fb_helper_hotplug_event(dev->fb_helper);
>  
> -	if (!dev->mode_config.num_connector)
> +	if (!dev->mode_config.num_connector) {
> +		DRM_DEV_DEBUG(dev->dev, "No connectors found, will not create framebuffer!\n");
>  		return 0;
> +	}
>  
>  	ret = drm_fb_helper_fbdev_setup(dev, fb_helper, &drm_fb_helper_generic_funcs,
>  					fb_helper->preferred_bpp, 0);
> @@ -3187,13 +3191,15 @@ static const struct drm_client_funcs drm_fbdev_client_funcs = {
>  };
>  
>  /**
> - * drm_fb_helper_generic_fbdev_setup() - Setup generic fbdev emulation
> + * drm_fbdev_generic_setup() - Setup generic fbdev emulation

Hm, drm_fb_helper_ would be the OCD prefix we use everywhere, but better
to make the docs match the code. Feel free to throw a rename patch on top.

>   * @dev: DRM device
>   * @preferred_bpp: Preferred bits per pixel for the device.
>   *                 @dev->mode_config.preferred_depth is used if this is zero.
>   *
>   * This function sets up generic fbdev emulation for drivers that supports
> - * dumb buffers with a virtual address and that can be mmap'ed.
> + * dumb buffers with a virtual address and that can be mmap'ed. If the driver
> + * does not support these functions, it could use drm_fb_helper_fbdev_setup().
> + * This function can only be used with devices created using drm_dev_register().

This last line is misleading, since every drm device is called with
drm_dev_register(). It's just not all called by driver code directly.
With this line removed:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

I'll leave it to Gerd to apply this all.
-Daniel


>   *
>   * Restore, hotplug events and teardown are all taken care of. Drivers that do
>   * suspend/resume need to call drm_fb_helper_set_suspend_unlocked() themselves.
> @@ -3206,6 +3212,8 @@ static const struct drm_client_funcs drm_fbdev_client_funcs = {
>   * This function is safe to call even when there are no connectors present.
>   * Setup will be retried on the next hotplug event.
>   *
> + * The fbdev is destroyed by drm_dev_unregister().
> + *
>   * Returns:
>   * Zero on success or negative error code on failure.
>   */
> @@ -3214,6 +3222,8 @@ int drm_fbdev_generic_setup(struct drm_device *dev, unsigned int preferred_bpp)
>  	struct drm_fb_helper *fb_helper;
>  	int ret;
>  
> +	WARN(dev->fb_helper, "fb_helper is already set!\n");
> +
>  	if (!drm_fbdev_emulation)
>  		return 0;
>  
> @@ -3224,12 +3234,15 @@ int drm_fbdev_generic_setup(struct drm_device *dev, unsigned int preferred_bpp)
>  	ret = drm_client_new(dev, &fb_helper->client, "fbdev", &drm_fbdev_client_funcs);
>  	if (ret) {
>  		kfree(fb_helper);
> +		DRM_DEV_ERROR(dev->dev, "Failed to register client: %d\n", ret);
>  		return ret;
>  	}
>  
>  	fb_helper->preferred_bpp = preferred_bpp;
>  
> -	drm_fbdev_client_hotplug(&fb_helper->client);
> +	ret = drm_fbdev_client_hotplug(&fb_helper->client);
> +	if (ret)
> +		DRM_DEV_DEBUG(dev->dev, "client hotplug ret=%d\n", ret);
>  
>  	return 0;
>  }
> -- 
> 2.18.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH v2 4/4] drm/fb-helper: improve documentation and print warnings

[Daniel Vetter]

On Fri, Sep 07, 2018 at 12:18:10AM +0200, Peter Wu wrote:
> Clarify the relation between drm_fb_helper_fbdev_setup/teardown. Clarify
> requirements for the new generic fbdev emulation API and log some more
> details in case the driver does something wrong. Fix related typos.
> 
> Cc: Noralf Trønnes <noralf@tronnes.org>
> Signed-off-by: Peter Wu <peter@lekensteyn.nl>
> ---
>  drivers/gpu/drm/drm_fb_helper.c | 25 +++++++++++++++++++------
>  1 file changed, 19 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 4b0dd20bccb8..7f92ff173986 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -2821,7 +2821,9 @@ EXPORT_SYMBOL(drm_fb_helper_hotplug_event);
>   * The caller must to provide a &drm_fb_helper_funcs->fb_probe callback
>   * function.
>   *
> - * See also: drm_fb_helper_initial_config()
> + * Use drm_fb_helper_fbdev_teardown() to destroy the fbdev.
> + *
> + * See also: drm_fb_helper_initial_config(), drm_fbdev_generic_setup().
>   *
>   * Returns:
>   * Zero on success or negative error code on failure.
> @@ -3037,7 +3039,7 @@ static struct fb_deferred_io drm_fbdev_defio = {
>   * @fb_helper: fbdev helper structure
>   * @sizes: describes fbdev size and scanout surface size
>   *
> - * This function uses the client API to crate a framebuffer backed by a dumb buffer.
> + * This function uses the client API to create a framebuffer backed by a dumb buffer.
>   *
>   * The _sys_ versions are used for &fb_ops.fb_read, fb_write, fb_fillrect,
>   * fb_copyarea, fb_imageblit.
> @@ -3165,8 +3167,10 @@ static int drm_fbdev_client_hotplug(struct drm_client_dev *client)
>  	if (dev->fb_helper)
>  		return drm_fb_helper_hotplug_event(dev->fb_helper);
>  
> -	if (!dev->mode_config.num_connector)
> +	if (!dev->mode_config.num_connector) {
> +		DRM_DEV_DEBUG(dev->dev, "No connectors found, will not create framebuffer!\n");
>  		return 0;
> +	}
>  
>  	ret = drm_fb_helper_fbdev_setup(dev, fb_helper, &drm_fb_helper_generic_funcs,
>  					fb_helper->preferred_bpp, 0);
> @@ -3187,13 +3191,15 @@ static const struct drm_client_funcs drm_fbdev_client_funcs = {
>  };
>  
>  /**
> - * drm_fb_helper_generic_fbdev_setup() - Setup generic fbdev emulation
> + * drm_fbdev_generic_setup() - Setup generic fbdev emulation

Hm, drm_fb_helper_ would be the OCD prefix we use everywhere, but better
to make the docs match the code. Feel free to throw a rename patch on top.

>   * @dev: DRM device
>   * @preferred_bpp: Preferred bits per pixel for the device.
>   *                 @dev->mode_config.preferred_depth is used if this is zero.
>   *
>   * This function sets up generic fbdev emulation for drivers that supports
> - * dumb buffers with a virtual address and that can be mmap'ed.
> + * dumb buffers with a virtual address and that can be mmap'ed. If the driver
> + * does not support these functions, it could use drm_fb_helper_fbdev_setup().
> + * This function can only be used with devices created using drm_dev_register().

This last line is misleading, since every drm device is called with
drm_dev_register(). It's just not all called by driver code directly.
With this line removed:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

I'll leave it to Gerd to apply this all.
-Daniel


>   *
>   * Restore, hotplug events and teardown are all taken care of. Drivers that do
>   * suspend/resume need to call drm_fb_helper_set_suspend_unlocked() themselves.
> @@ -3206,6 +3212,8 @@ static const struct drm_client_funcs drm_fbdev_client_funcs = {
>   * This function is safe to call even when there are no connectors present.
>   * Setup will be retried on the next hotplug event.
>   *
> + * The fbdev is destroyed by drm_dev_unregister().
> + *
>   * Returns:
>   * Zero on success or negative error code on failure.
>   */
> @@ -3214,6 +3222,8 @@ int drm_fbdev_generic_setup(struct drm_device *dev, unsigned int preferred_bpp)
>  	struct drm_fb_helper *fb_helper;
>  	int ret;
>  
> +	WARN(dev->fb_helper, "fb_helper is already set!\n");
> +
>  	if (!drm_fbdev_emulation)
>  		return 0;
>  
> @@ -3224,12 +3234,15 @@ int drm_fbdev_generic_setup(struct drm_device *dev, unsigned int preferred_bpp)
>  	ret = drm_client_new(dev, &fb_helper->client, "fbdev", &drm_fbdev_client_funcs);
>  	if (ret) {
>  		kfree(fb_helper);
> +		DRM_DEV_ERROR(dev->dev, "Failed to register client: %d\n", ret);
>  		return ret;
>  	}
>  
>  	fb_helper->preferred_bpp = preferred_bpp;
>  
> -	drm_fbdev_client_hotplug(&fb_helper->client);
> +	ret = drm_fbdev_client_hotplug(&fb_helper->client);
> +	if (ret)
> +		DRM_DEV_DEBUG(dev->dev, "client hotplug ret=%d\n", ret);
>  
>  	return 0;
>  }
> -- 
> 2.18.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

Re: [PATCH v2 4/4] drm/fb-helper: improve documentation and print warnings

[Gerd Hoffmann]

> >   * This function sets up generic fbdev emulation for drivers that supports
> > - * dumb buffers with a virtual address and that can be mmap'ed.
> > + * dumb buffers with a virtual address and that can be mmap'ed. If the driver
> > + * does not support these functions, it could use drm_fb_helper_fbdev_setup().
> > + * This function can only be used with devices created using drm_dev_register().
> 
> This last line is misleading, since every drm device is called with
> drm_dev_register(). It's just not all called by driver code directly.
> With this line removed:
> 
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> I'll leave it to Gerd to apply this all.

Fixed and pushed.

cheers,
  Gerd