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
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/[email protected]
Acked-by: Daniel Vetter <[email protected]>
Signed-off-by: Peter Wu <[email protected]>
---
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
The drm_get_pci_dev API is deprecated, replace it by drm_dev_register.
Signed-off-by: Peter Wu <[email protected]>
---
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
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 <[email protected]>
Signed-off-by: Peter Wu <[email protected]>
---
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
The "initialized" member is going away. suspend/resume still works (even
if bochsfb_create is forced to fail).
Signed-off-by: Peter Wu <[email protected]>
---
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
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 <[email protected]>
> Signed-off-by: Peter Wu <[email protected]>
> ---
> 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 <[email protected]>
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
> [email protected]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
> > * 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 <[email protected]>
>
> I'll leave it to Gerd to apply this all.
Fixed and pushed.
cheers,
Gerd