Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1462171 > unrolled thread
| Started by | Noralf Trønnes <noralf@tronnes.org> |
|---|---|
| First post | 2016-08-14 19:00 +0200 |
| Last post | 2016-08-17 13:20 +0200 |
| Articles | 14 — 2 participants |
Back to article view | Back to linux.kernel
[PATCH v3 0/3] drm: add SimpleDRM driver Noralf Trønnes <noralf@tronnes.org> - 2016-08-14 19:00 +0200
[PATCH v3 2/3] drm: simpledrm: add fbdev fallback support Noralf Trønnes <noralf@tronnes.org> - 2016-08-14 19:00 +0200
Re: [PATCH v3 2/3] drm: simpledrm: add fbdev fallback support Daniel Vetter <daniel@ffwll.ch> - 2016-08-15 08:50 +0200
Re: [PATCH v3 2/3] drm: simpledrm: add fbdev fallback support Noralf Trønnes <noralf@tronnes.org> - 2016-08-16 15:20 +0200
Re: [PATCH v3 2/3] drm: simpledrm: add fbdev fallback support Daniel Vetter <daniel@ffwll.ch> - 2016-08-16 17:40 +0200
[PATCH v3 3/3] drm: simpledrm: honour remove_conflicting_framebuffers() Noralf Trønnes <noralf@tronnes.org> - 2016-08-14 19:00 +0200
Re: [PATCH v3 3/3] drm: simpledrm: honour remove_conflicting_framebuffers() Daniel Vetter <daniel@ffwll.ch> - 2016-08-15 09:20 +0200
Re: [PATCH v3 0/3] drm: add SimpleDRM driver Daniel Vetter <daniel@ffwll.ch> - 2016-08-15 09:20 +0200
Re: [PATCH v3 1/3] drm: add SimpleDRM driver Noralf Trønnes <noralf@tronnes.org> - 2016-08-16 15:00 +0200
Re: [PATCH v3 1/3] drm: add SimpleDRM driver Daniel Vetter <daniel@ffwll.ch> - 2016-08-16 17:30 +0200
Re: [PATCH v3 1/3] drm: add SimpleDRM driver Noralf Trønnes <noralf@tronnes.org> - 2016-08-16 21:50 +0200
Re: [PATCH v3 1/3] drm: add SimpleDRM driver Daniel Vetter <daniel@ffwll.ch> - 2016-08-17 11:50 +0200
Re: [PATCH v3 1/3] drm: add SimpleDRM driver Noralf Trønnes <noralf@tronnes.org> - 2016-08-17 12:20 +0200
Re: [PATCH v3 1/3] drm: add SimpleDRM driver Daniel Vetter <daniel@ffwll.ch> - 2016-08-17 13:20 +0200
| From | Noralf Trønnes <noralf@tronnes.org> |
|---|---|
| Date | 2016-08-14 19:00 +0200 |
| Subject | [PATCH v3 0/3] drm: add SimpleDRM driver |
| Message-ID | <s66Vr-265-7@gated-at.bofh.it> |
This patchset adds the simpledrm driver by David Herrmann based on a
patchset[1] from 2014. That patchset also included patches for kicking
out simpledrm by real drivers. I have stayed away from that since it
involves another subsystem and I would probably be unable to answer any
questions about the implementation.
Two main changes in this third version:
Paul Gortmaker pointed out that module.h pulls in 750k and is best
avoided when not necessary. All the source files included it, but only
one needed it. I the same spirit I moved the includes I could from the
header file to the respective source files.
The panic handling I'm working on requires an enabled pipeline to work,
so I have switched the fbdev code to use the drm fb helper. Maybe not
strictly necessary since fbcon can output panic messages, but it gave
me an excuse to do it, making the fbdev code "drm standard".
I have tested simpledrm on a Raspberry Pi B+ with U-boot setting up the
framebuffer and producing this node (legacy, not under /chosen):
/ {
framebuffer@1e887000 {
compatible = "simple-framebuffer";
reg = <0x1e887000 0x36c600>;
format = "r5g6b5";
width = <1824>;
height = <984>;
stride = <3648>;
status = "okay";
};
I have only tested with fbcon and modetest (XR24,RG16).
Noralf.
Changes from version 2:
- Remove superfluos module.h includes
- Move includes from header to source files
- Set plane.fb before flushing in pipe update, or else the previous one
gets flushed
- Added check for vblank event in sdrm_display_pipe_update()
fbdev:
- Switch to using drm_fb_helper in preparation for future panic handling
which needs an enabled pipeline.
- Don't forget to free fb_info when kicked out.
Changes from version 1:
- Move platform_set_drvdata() before drm_dev_register()
- Remove drm_legacy_mmap() call.
- Set mode_config.{min,max}_{width,height} to the actual dimensions
of the native framebuffer
- Remove plane positioning since it won't work with the simple display pipe,
meaning sdrm_display_pipe_check() isn't necessary either
- Support the additions to the Device Tree binding document, including
clocks, regulators and having the node under /chosen
fbdev:
- Honour remove_conflicting_framebuffers()
Changes from previous version[2]:
- Remove FB_SIMPLE=n dependency to avoid kconfig recursive error
- Changed module name to match kconfig help text: sdrm -> simpledrm
- Use drm_simple_display_pipe
- Replace deprecated drm_platform_init()
- sdrm_dumb_create(): drm_gem_object_unreference() -> *_unlocked()
- sdrm_dumb_map_offset(): drm_gem_object_lookup() remove drm_device parameter
- sdrm_drm_mmap() changes:
Remove struct_mutex locking
Add drm_vma_offset_{lock,unlock}_lookup()
drm_mmap() -> drm_legacy_mmap()
- dma_buf_begin_cpu_access() doesn't require start and length anymore
- Use drm_cvt_mode() instead of open coding a mode
- Fix format conversion. In the intermediate step, store the 8/6/5 bit color
value in the upper part of the 16-bit color variable, not the lower.
- Support clips == NULL in sdrm_dirty()
- Set mode_config.preferred_depth
- Attach mode_config.dirty_info_property to connector
fbdev:
- Remove the DRM_SIMPLEDRM_FBDEV kconfig option and use DRM_FBDEV_EMULATION
- Suspend fbcon/fbdev when the pipeline is enabled, resume in lastclose
- Add FBINFO_CAN_FORCE_OUTPUT flag so we get oops'es on the console
[1] https://lists.freedesktop.org/archives/dri-devel/2014-January/052584.html
[2] https://lists.freedesktop.org/archives/dri-devel/2014-January/052594.html
Further history:
[PATCH v4 0/6] SimpleDRM Driver
https://lists.freedesktop.org/archives/dri-devel/2013-September/044638.html
[PATCH v2 00/14] Platform Framebuffers and SimpleDRM
https://lists.freedesktop.org/archives/dri-devel/2013-July/041090.html
[RFC 0/6] SimpleDRM Driver (was: dvbe driver)
https://lists.freedesktop.org/archives/dri-devel/2013-June/040386.html
[PATCH 0/9] System Framebuffer Bus (sysfb)
https://lists.freedesktop.org/archives/dri-devel/2013-February/035013.html
Noralf Trønnes (3):
drm: add SimpleDRM driver
drm: simpledrm: add fbdev fallback support
drm: simpledrm: honour remove_conflicting_framebuffers()
drivers/gpu/drm/Kconfig | 2 +
drivers/gpu/drm/Makefile | 1 +
drivers/gpu/drm/simpledrm/Kconfig | 27 ++
drivers/gpu/drm/simpledrm/Makefile | 5 +
drivers/gpu/drm/simpledrm/simpledrm.h | 129 +++++++
drivers/gpu/drm/simpledrm/simpledrm_damage.c | 302 +++++++++++++++
drivers/gpu/drm/simpledrm/simpledrm_drv.c | 546 +++++++++++++++++++++++++++
drivers/gpu/drm/simpledrm/simpledrm_fbdev.c | 252 +++++++++++++
drivers/gpu/drm/simpledrm/simpledrm_gem.c | 274 ++++++++++++++
drivers/gpu/drm/simpledrm/simpledrm_kms.c | 266 +++++++++++++
10 files changed, 1804 insertions(+)
create mode 100644 drivers/gpu/drm/simpledrm/Kconfig
create mode 100644 drivers/gpu/drm/simpledrm/Makefile
create mode 100644 drivers/gpu/drm/simpledrm/simpledrm.h
create mode 100644 drivers/gpu/drm/simpledrm/simpledrm_damage.c
create mode 100644 drivers/gpu/drm/simpledrm/simpledrm_drv.c
create mode 100644 drivers/gpu/drm/simpledrm/simpledrm_fbdev.c
create mode 100644 drivers/gpu/drm/simpledrm/simpledrm_gem.c
create mode 100644 drivers/gpu/drm/simpledrm/simpledrm_kms.c
--
2.8.2
[toc] | [next] | [standalone]
| From | Noralf Trønnes <noralf@tronnes.org> |
|---|---|
| Date | 2016-08-14 19:00 +0200 |
| Subject | [PATCH v3 2/3] drm: simpledrm: add fbdev fallback support |
| Message-ID | <s66Vr-265-17@gated-at.bofh.it> |
| In reply to | #1462171 |
Create a simple fbdev device during SimpleDRM setup so legacy user-space
and fbcon can use it.
Original work by David Herrmann.
Cc: dh.herrmann@gmail.com
Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
Changes from version 2:
- Switch to using drm_fb_helper in preparation for future panic handling
which needs an enabled pipeline.
Changes from version 1:
No changes
Changes from previous version:
- Remove the DRM_SIMPLEDRM_FBDEV kconfig option and use DRM_FBDEV_EMULATION
- Suspend fbcon/fbdev when the pipeline is enabled, resume in lastclose
- Add FBINFO_CAN_FORCE_OUTPUT flag so we get oops'es on the console
drivers/gpu/drm/simpledrm/Kconfig | 3 +
drivers/gpu/drm/simpledrm/Makefile | 1 +
drivers/gpu/drm/simpledrm/simpledrm.h | 32 ++++-
drivers/gpu/drm/simpledrm/simpledrm_drv.c | 4 +
drivers/gpu/drm/simpledrm/simpledrm_fbdev.c | 201 ++++++++++++++++++++++++++++
drivers/gpu/drm/simpledrm/simpledrm_kms.c | 10 +-
6 files changed, 249 insertions(+), 2 deletions(-)
create mode 100644 drivers/gpu/drm/simpledrm/simpledrm_fbdev.c
diff --git a/drivers/gpu/drm/simpledrm/Kconfig b/drivers/gpu/drm/simpledrm/Kconfig
index f45b25d..9454536 100644
--- a/drivers/gpu/drm/simpledrm/Kconfig
+++ b/drivers/gpu/drm/simpledrm/Kconfig
@@ -13,6 +13,9 @@ config DRM_SIMPLEDRM
SimpleDRM supports "simple-framebuffer" DeviceTree objects and
compatible platform framebuffers.
+ If fbdev support is enabled, this driver will also provide an fbdev
+ compatibility layer.
+
If unsure, say Y.
To compile this driver as a module, choose M here: the
diff --git a/drivers/gpu/drm/simpledrm/Makefile b/drivers/gpu/drm/simpledrm/Makefile
index f6a62dc..7087245 100644
--- a/drivers/gpu/drm/simpledrm/Makefile
+++ b/drivers/gpu/drm/simpledrm/Makefile
@@ -1,4 +1,5 @@
simpledrm-y := simpledrm_drv.o simpledrm_kms.o simpledrm_gem.o \
simpledrm_damage.o
+simpledrm-$(CONFIG_DRM_FBDEV_EMULATION) += simpledrm_fbdev.o
obj-$(CONFIG_DRM_SIMPLEDRM) := simpledrm.o
diff --git a/drivers/gpu/drm/simpledrm/simpledrm.h b/drivers/gpu/drm/simpledrm/simpledrm.h
index 481acc2..f01b75d 100644
--- a/drivers/gpu/drm/simpledrm/simpledrm.h
+++ b/drivers/gpu/drm/simpledrm/simpledrm.h
@@ -17,13 +17,13 @@
struct simplefb_format;
struct regulator;
-struct fb_info;
struct clk;
struct sdrm_device {
struct drm_device *ddev;
struct drm_simple_display_pipe pipe;
struct drm_connector conn;
+ struct sdrm_fbdev *fbdev;
/* framebuffer information */
const struct simplefb_format *fb_sformat;
@@ -46,6 +46,7 @@ struct sdrm_device {
#endif
};
+void sdrm_lastclose(struct drm_device *ddev);
int sdrm_drm_modeset_init(struct sdrm_device *sdrm);
int sdrm_drm_mmap(struct file *filp, struct vm_area_struct *vma);
@@ -87,4 +88,33 @@ struct sdrm_framebuffer {
#define to_sdrm_fb(x) container_of(x, struct sdrm_framebuffer, base)
+#ifdef CONFIG_DRM_FBDEV_EMULATION
+
+void sdrm_fbdev_init(struct sdrm_device *sdrm);
+void sdrm_fbdev_cleanup(struct sdrm_device *sdrm);
+void sdrm_fbdev_display_pipe_update(struct sdrm_device *sdrm,
+ struct drm_framebuffer *fb);
+void sdrm_fbdev_restore_mode(struct sdrm_device *sdrm);
+
+#else
+
+static inline void sdrm_fbdev_init(struct sdrm_device *sdrm)
+{
+}
+
+static inline void sdrm_fbdev_cleanup(struct sdrm_device *sdrm)
+{
+}
+
+static inline void sdrm_fbdev_display_pipe_update(struct sdrm_device *sdrm,
+ struct drm_framebuffer *fb)
+{
+}
+
+static inline void sdrm_fbdev_restore_mode(struct sdrm_device *sdrm)
+{
+}
+
+#endif
+
#endif /* SDRM_DRV_H */
diff --git a/drivers/gpu/drm/simpledrm/simpledrm_drv.c b/drivers/gpu/drm/simpledrm/simpledrm_drv.c
index e5b6f75..a4e6566 100644
--- a/drivers/gpu/drm/simpledrm/simpledrm_drv.c
+++ b/drivers/gpu/drm/simpledrm/simpledrm_drv.c
@@ -42,6 +42,7 @@ static struct drm_driver sdrm_drm_driver = {
.driver_features = DRIVER_GEM | DRIVER_MODESET | DRIVER_PRIME |
DRIVER_ATOMIC,
.fops = &sdrm_drm_fops,
+ .lastclose = sdrm_lastclose,
.gem_free_object = sdrm_gem_free_object,
.prime_fd_to_handle = drm_gem_prime_fd_to_handle,
@@ -448,6 +449,8 @@ static int sdrm_simplefb_probe(struct platform_device *pdev)
if (ret)
goto err_regulators;
+ sdrm_fbdev_init(ddev->dev_private);
+
DRM_INFO("Initialized %s on minor %d\n", ddev->driver->name,
ddev->primary->index);
@@ -473,6 +476,7 @@ static int sdrm_simplefb_remove(struct platform_device *pdev)
struct drm_device *ddev = platform_get_drvdata(pdev);
struct sdrm_device *sdrm = ddev->dev_private;
+ sdrm_fbdev_cleanup(sdrm);
drm_dev_unregister(ddev);
drm_mode_config_cleanup(ddev);
diff --git a/drivers/gpu/drm/simpledrm/simpledrm_fbdev.c b/drivers/gpu/drm/simpledrm/simpledrm_fbdev.c
new file mode 100644
index 0000000..4038dd9
--- /dev/null
+++ b/drivers/gpu/drm/simpledrm/simpledrm_fbdev.c
@@ -0,0 +1,201 @@
+/*
+ * SimpleDRM firmware framebuffer driver
+ * Copyright (c) 2012-2014 David Herrmann <dh.herrmann@gmail.com>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the Free
+ * Software Foundation; either version 2 of the License, or (at your option)
+ * any later version.
+ */
+
+#include <drm/drmP.h>
+#include <drm/drm_crtc_helper.h>
+#include <drm/drm_fb_helper.h>
+#include <linux/console.h>
+#include <linux/fb.h>
+#include <linux/platform_device.h>
+#include <linux/platform_data/simplefb.h>
+
+#include "simpledrm.h"
+
+struct sdrm_fbdev {
+ struct drm_fb_helper fb_helper;
+ struct drm_framebuffer fb;
+};
+
+static inline struct sdrm_fbdev *to_sdrm_fbdev(struct drm_fb_helper *helper)
+{
+ return container_of(helper, struct sdrm_fbdev, fb_helper);
+}
+
+static struct fb_ops sdrm_fbdev_ops = {
+ .owner = THIS_MODULE,
+ .fb_fillrect = drm_fb_helper_cfb_fillrect,
+ .fb_copyarea = drm_fb_helper_cfb_copyarea,
+ .fb_imageblit = drm_fb_helper_cfb_imageblit,
+ .fb_check_var = drm_fb_helper_check_var,
+ .fb_set_par = drm_fb_helper_set_par,
+ .fb_setcmap = drm_fb_helper_setcmap,
+};
+
+static struct drm_framebuffer_funcs sdrm_fb_funcs = {
+ .destroy = drm_framebuffer_cleanup,
+};
+
+static int sdrm_fbdev_create(struct drm_fb_helper *helper,
+ struct drm_fb_helper_surface_size *sizes)
+{
+ struct sdrm_fbdev *fbdev = to_sdrm_fbdev(helper);
+ struct drm_device *ddev = helper->dev;
+ struct sdrm_device *sdrm = ddev->dev_private;
+ struct drm_framebuffer *fb = &fbdev->fb;
+ struct drm_mode_fb_cmd2 mode_cmd = {
+ .width = sdrm->fb_width,
+ .height = sdrm->fb_height,
+ .pitches[0] = sdrm->fb_stride,
+ .pixel_format = sdrm->fb_format,
+ };
+ struct fb_info *fbi;
+ int ret;
+
+ fbi = drm_fb_helper_alloc_fbi(helper);
+ if (IS_ERR(fbi))
+ return PTR_ERR(fbi);
+
+ drm_helper_mode_fill_fb_struct(fb, &mode_cmd);
+
+ ret = drm_framebuffer_init(ddev, fb, &sdrm_fb_funcs);
+ if (ret) {
+ dev_err(ddev->dev, "Failed to initialize framebuffer: %d\n", ret);
+ goto err_fb_info_destroy;
+ }
+
+ helper->fb = fb;
+ fbi->par = helper;
+
+ fbi->flags = FBINFO_DEFAULT | FBINFO_MISC_FIRMWARE |
+ FBINFO_CAN_FORCE_OUTPUT;
+ fbi->fbops = &sdrm_fbdev_ops;
+
+ drm_fb_helper_fill_fix(fbi, fb->pitches[0], fb->depth);
+ drm_fb_helper_fill_var(fbi, helper, fb->width, fb->height);
+
+ strncpy(fbi->fix.id, "simpledrmfb", 15);
+ fbi->fix.smem_start = (unsigned long)sdrm->fb_base;
+ fbi->fix.smem_len = sdrm->fb_size;
+ fbi->screen_base = sdrm->fb_map;
+
+ return 0;
+
+err_fb_info_destroy:
+ drm_fb_helper_release_fbi(helper);
+
+ return ret;
+}
+
+static const struct drm_fb_helper_funcs sdrm_fb_helper_funcs = {
+ .fb_probe = sdrm_fbdev_create,
+};
+
+void sdrm_fbdev_init(struct sdrm_device *sdrm)
+{
+ struct drm_device *ddev = sdrm->ddev;
+ struct drm_fb_helper *fb_helper;
+ struct sdrm_fbdev *fbdev;
+ int ret;
+
+ fbdev = kzalloc(sizeof(*fbdev), GFP_KERNEL);
+ if (!fbdev) {
+ dev_err(ddev->dev, "Failed to allocate drm fbdev.\n");
+ return;
+ }
+
+ fb_helper = &fbdev->fb_helper;
+
+ drm_fb_helper_prepare(ddev, fb_helper, &sdrm_fb_helper_funcs);
+
+ ret = drm_fb_helper_init(ddev, fb_helper, 1, 1);
+ if (ret < 0) {
+ dev_err(ddev->dev, "Failed to initialize drm fb helper.\n");
+ goto err_free;
+ }
+
+ ret = drm_fb_helper_single_add_all_connectors(fb_helper);
+ if (ret < 0) {
+ dev_err(ddev->dev, "Failed to add connectors.\n");
+ goto err_drm_fb_helper_fini;
+ }
+
+ ret = drm_fb_helper_initial_config(fb_helper,
+ ddev->mode_config.preferred_depth);
+ if (ret < 0) {
+ dev_err(ddev->dev, "Failed to set initial hw configuration.\n");
+ goto err_drm_fb_helper_fini;
+ }
+
+ sdrm->fbdev = fbdev;
+
+ return;
+
+err_drm_fb_helper_fini:
+ drm_fb_helper_fini(fb_helper);
+err_free:
+ kfree(fbdev);
+}
+
+void sdrm_fbdev_cleanup(struct sdrm_device *sdrm)
+{
+ struct sdrm_fbdev *fbdev = sdrm->fbdev;
+ struct drm_fb_helper *fb_helper;
+
+ if (!fbdev)
+ return;
+
+ sdrm->fbdev = NULL;
+ fb_helper = &fbdev->fb_helper;
+
+ drm_fb_helper_unregister_fbi(fb_helper);
+ drm_fb_helper_release_fbi(fb_helper);
+
+ drm_framebuffer_unregister_private(&fbdev->fb);
+ drm_framebuffer_cleanup(&fbdev->fb);
+
+ drm_fb_helper_fini(fb_helper);
+ kfree(fbdev);
+}
+
+static void sdrm_fbdev_set_suspend(struct fb_info *fbi, int state)
+{
+ console_lock();
+ fb_set_suspend(fbi, state);
+ console_unlock();
+}
+
+/*
+ * Since fbdev is using the native framebuffer, fbcon has to be disabled
+ * when the drm stack is used.
+ */
+void sdrm_fbdev_display_pipe_update(struct sdrm_device *sdrm,
+ struct drm_framebuffer *fb)
+{
+ struct sdrm_fbdev *fbdev = sdrm->fbdev;
+
+ if (!fbdev || fbdev->fb_helper.fb == fb)
+ return;
+
+ if (fbdev->fb_helper.fbdev->state == FBINFO_STATE_RUNNING)
+ sdrm_fbdev_set_suspend(fbdev->fb_helper.fbdev, 1);
+}
+
+void sdrm_fbdev_restore_mode(struct sdrm_device *sdrm)
+{
+ struct sdrm_fbdev *fbdev = sdrm->fbdev;
+
+ if (!fbdev)
+ return;
+
+ drm_fb_helper_restore_fbdev_mode_unlocked(&fbdev->fb_helper);
+
+ if (fbdev->fb_helper.fbdev->state != FBINFO_STATE_RUNNING)
+ sdrm_fbdev_set_suspend(fbdev->fb_helper.fbdev, 0);
+}
diff --git a/drivers/gpu/drm/simpledrm/simpledrm_kms.c b/drivers/gpu/drm/simpledrm/simpledrm_kms.c
index 00e50d9..92ddc116 100644
--- a/drivers/gpu/drm/simpledrm/simpledrm_kms.c
+++ b/drivers/gpu/drm/simpledrm/simpledrm_kms.c
@@ -24,6 +24,13 @@ static const uint32_t sdrm_formats[] = {
DRM_FORMAT_XRGB8888,
};
+void sdrm_lastclose(struct drm_device *ddev)
+{
+ struct sdrm_device *sdrm = ddev->dev_private;
+
+ sdrm_fbdev_restore_mode(sdrm);
+}
+
static int sdrm_conn_get_modes(struct drm_connector *conn)
{
struct sdrm_device *sdrm = conn->dev->dev_private;
@@ -91,8 +98,9 @@ void sdrm_display_pipe_update(struct drm_simple_display_pipe *pipe,
struct sdrm_device *sdrm = pipe_to_sdrm(pipe);
sdrm_crtc_send_vblank_event(&pipe->crtc);
+ sdrm_fbdev_display_pipe_update(sdrm, fb);
- if (fb) {
+ if (fb && fb->funcs->dirty) {
pipe->plane.fb = fb;
sdrm_dirty_all_locked(sdrm);
}
--
2.8.2
[toc] | [prev] | [next] | [standalone]
| From | Daniel Vetter <daniel@ffwll.ch> |
|---|---|
| Date | 2016-08-15 08:50 +0200 |
| Subject | Re: [PATCH v3 2/3] drm: simpledrm: add fbdev fallback support |
| Message-ID | <s6jSG-25y-7@gated-at.bofh.it> |
| In reply to | #1462172 |
On Sun, Aug 14, 2016 at 06:52:05PM +0200, Noralf Trønnes wrote:
> Create a simple fbdev device during SimpleDRM setup so legacy user-space
> and fbcon can use it.
>
> Original work by David Herrmann.
>
> Cc: dh.herrmann@gmail.com
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> ---
>
> Changes from version 2:
> - Switch to using drm_fb_helper in preparation for future panic handling
> which needs an enabled pipeline.
>
> Changes from version 1:
> No changes
>
> Changes from previous version:
> - Remove the DRM_SIMPLEDRM_FBDEV kconfig option and use DRM_FBDEV_EMULATION
> - Suspend fbcon/fbdev when the pipeline is enabled, resume in lastclose
> - Add FBINFO_CAN_FORCE_OUTPUT flag so we get oops'es on the console
>
> drivers/gpu/drm/simpledrm/Kconfig | 3 +
> drivers/gpu/drm/simpledrm/Makefile | 1 +
> drivers/gpu/drm/simpledrm/simpledrm.h | 32 ++++-
> drivers/gpu/drm/simpledrm/simpledrm_drv.c | 4 +
> drivers/gpu/drm/simpledrm/simpledrm_fbdev.c | 201 ++++++++++++++++++++++++++++
> drivers/gpu/drm/simpledrm/simpledrm_kms.c | 10 +-
> 6 files changed, 249 insertions(+), 2 deletions(-)
> create mode 100644 drivers/gpu/drm/simpledrm/simpledrm_fbdev.c
>
> diff --git a/drivers/gpu/drm/simpledrm/Kconfig b/drivers/gpu/drm/simpledrm/Kconfig
> index f45b25d..9454536 100644
> --- a/drivers/gpu/drm/simpledrm/Kconfig
> +++ b/drivers/gpu/drm/simpledrm/Kconfig
> @@ -13,6 +13,9 @@ config DRM_SIMPLEDRM
> SimpleDRM supports "simple-framebuffer" DeviceTree objects and
> compatible platform framebuffers.
>
> + If fbdev support is enabled, this driver will also provide an fbdev
> + compatibility layer.
> +
> If unsure, say Y.
>
> To compile this driver as a module, choose M here: the
> diff --git a/drivers/gpu/drm/simpledrm/Makefile b/drivers/gpu/drm/simpledrm/Makefile
> index f6a62dc..7087245 100644
> --- a/drivers/gpu/drm/simpledrm/Makefile
> +++ b/drivers/gpu/drm/simpledrm/Makefile
> @@ -1,4 +1,5 @@
> simpledrm-y := simpledrm_drv.o simpledrm_kms.o simpledrm_gem.o \
> simpledrm_damage.o
> +simpledrm-$(CONFIG_DRM_FBDEV_EMULATION) += simpledrm_fbdev.o
>
> obj-$(CONFIG_DRM_SIMPLEDRM) := simpledrm.o
> diff --git a/drivers/gpu/drm/simpledrm/simpledrm.h b/drivers/gpu/drm/simpledrm/simpledrm.h
> index 481acc2..f01b75d 100644
> --- a/drivers/gpu/drm/simpledrm/simpledrm.h
> +++ b/drivers/gpu/drm/simpledrm/simpledrm.h
> @@ -17,13 +17,13 @@
>
> struct simplefb_format;
> struct regulator;
> -struct fb_info;
> struct clk;
>
> struct sdrm_device {
> struct drm_device *ddev;
> struct drm_simple_display_pipe pipe;
> struct drm_connector conn;
> + struct sdrm_fbdev *fbdev;
>
> /* framebuffer information */
> const struct simplefb_format *fb_sformat;
> @@ -46,6 +46,7 @@ struct sdrm_device {
> #endif
> };
>
> +void sdrm_lastclose(struct drm_device *ddev);
> int sdrm_drm_modeset_init(struct sdrm_device *sdrm);
> int sdrm_drm_mmap(struct file *filp, struct vm_area_struct *vma);
>
> @@ -87,4 +88,33 @@ struct sdrm_framebuffer {
>
> #define to_sdrm_fb(x) container_of(x, struct sdrm_framebuffer, base)
>
> +#ifdef CONFIG_DRM_FBDEV_EMULATION
> +
> +void sdrm_fbdev_init(struct sdrm_device *sdrm);
> +void sdrm_fbdev_cleanup(struct sdrm_device *sdrm);
> +void sdrm_fbdev_display_pipe_update(struct sdrm_device *sdrm,
> + struct drm_framebuffer *fb);
> +void sdrm_fbdev_restore_mode(struct sdrm_device *sdrm);
> +
> +#else
> +
> +static inline void sdrm_fbdev_init(struct sdrm_device *sdrm)
> +{
> +}
> +
> +static inline void sdrm_fbdev_cleanup(struct sdrm_device *sdrm)
> +{
> +}
> +
> +static inline void sdrm_fbdev_display_pipe_update(struct sdrm_device *sdrm,
> + struct drm_framebuffer *fb)
> +{
> +}
> +
> +static inline void sdrm_fbdev_restore_mode(struct sdrm_device *sdrm)
> +{
> +}
> +
> +#endif
Why do we need the #ifdefs here? Imo those few bytes of codes we can save
aren't worth the complexity ...
> +
> #endif /* SDRM_DRV_H */
> diff --git a/drivers/gpu/drm/simpledrm/simpledrm_drv.c b/drivers/gpu/drm/simpledrm/simpledrm_drv.c
> index e5b6f75..a4e6566 100644
> --- a/drivers/gpu/drm/simpledrm/simpledrm_drv.c
> +++ b/drivers/gpu/drm/simpledrm/simpledrm_drv.c
> @@ -42,6 +42,7 @@ static struct drm_driver sdrm_drm_driver = {
> .driver_features = DRIVER_GEM | DRIVER_MODESET | DRIVER_PRIME |
> DRIVER_ATOMIC,
> .fops = &sdrm_drm_fops,
> + .lastclose = sdrm_lastclose,
>
> .gem_free_object = sdrm_gem_free_object,
> .prime_fd_to_handle = drm_gem_prime_fd_to_handle,
> @@ -448,6 +449,8 @@ static int sdrm_simplefb_probe(struct platform_device *pdev)
> if (ret)
> goto err_regulators;
>
> + sdrm_fbdev_init(ddev->dev_private);
> +
> DRM_INFO("Initialized %s on minor %d\n", ddev->driver->name,
> ddev->primary->index);
>
> @@ -473,6 +476,7 @@ static int sdrm_simplefb_remove(struct platform_device *pdev)
> struct drm_device *ddev = platform_get_drvdata(pdev);
> struct sdrm_device *sdrm = ddev->dev_private;
>
> + sdrm_fbdev_cleanup(sdrm);
> drm_dev_unregister(ddev);
> drm_mode_config_cleanup(ddev);
>
> diff --git a/drivers/gpu/drm/simpledrm/simpledrm_fbdev.c b/drivers/gpu/drm/simpledrm/simpledrm_fbdev.c
> new file mode 100644
> index 0000000..4038dd9
> --- /dev/null
> +++ b/drivers/gpu/drm/simpledrm/simpledrm_fbdev.c
> @@ -0,0 +1,201 @@
> +/*
> + * SimpleDRM firmware framebuffer driver
> + * Copyright (c) 2012-2014 David Herrmann <dh.herrmann@gmail.com>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License as published by the Free
> + * Software Foundation; either version 2 of the License, or (at your option)
> + * any later version.
> + */
> +
> +#include <drm/drmP.h>
> +#include <drm/drm_crtc_helper.h>
> +#include <drm/drm_fb_helper.h>
> +#include <linux/console.h>
> +#include <linux/fb.h>
> +#include <linux/platform_device.h>
> +#include <linux/platform_data/simplefb.h>
> +
> +#include "simpledrm.h"
> +
> +struct sdrm_fbdev {
> + struct drm_fb_helper fb_helper;
> + struct drm_framebuffer fb;
> +};
> +
> +static inline struct sdrm_fbdev *to_sdrm_fbdev(struct drm_fb_helper *helper)
> +{
> + return container_of(helper, struct sdrm_fbdev, fb_helper);
> +}
> +
> +static struct fb_ops sdrm_fbdev_ops = {
> + .owner = THIS_MODULE,
> + .fb_fillrect = drm_fb_helper_cfb_fillrect,
> + .fb_copyarea = drm_fb_helper_cfb_copyarea,
> + .fb_imageblit = drm_fb_helper_cfb_imageblit,
> + .fb_check_var = drm_fb_helper_check_var,
> + .fb_set_par = drm_fb_helper_set_par,
> + .fb_setcmap = drm_fb_helper_setcmap,
> +};
> +
> +static struct drm_framebuffer_funcs sdrm_fb_funcs = {
> + .destroy = drm_framebuffer_cleanup,
> +};
> +
> +static int sdrm_fbdev_create(struct drm_fb_helper *helper,
> + struct drm_fb_helper_surface_size *sizes)
> +{
> + struct sdrm_fbdev *fbdev = to_sdrm_fbdev(helper);
> + struct drm_device *ddev = helper->dev;
> + struct sdrm_device *sdrm = ddev->dev_private;
> + struct drm_framebuffer *fb = &fbdev->fb;
> + struct drm_mode_fb_cmd2 mode_cmd = {
> + .width = sdrm->fb_width,
> + .height = sdrm->fb_height,
> + .pitches[0] = sdrm->fb_stride,
> + .pixel_format = sdrm->fb_format,
> + };
> + struct fb_info *fbi;
> + int ret;
> +
> + fbi = drm_fb_helper_alloc_fbi(helper);
> + if (IS_ERR(fbi))
> + return PTR_ERR(fbi);
> +
> + drm_helper_mode_fill_fb_struct(fb, &mode_cmd);
> +
> + ret = drm_framebuffer_init(ddev, fb, &sdrm_fb_funcs);
> + if (ret) {
> + dev_err(ddev->dev, "Failed to initialize framebuffer: %d\n", ret);
> + goto err_fb_info_destroy;
> + }
> +
> + helper->fb = fb;
> + fbi->par = helper;
> +
> + fbi->flags = FBINFO_DEFAULT | FBINFO_MISC_FIRMWARE |
> + FBINFO_CAN_FORCE_OUTPUT;
> + fbi->fbops = &sdrm_fbdev_ops;
> +
> + drm_fb_helper_fill_fix(fbi, fb->pitches[0], fb->depth);
> + drm_fb_helper_fill_var(fbi, helper, fb->width, fb->height);
> +
> + strncpy(fbi->fix.id, "simpledrmfb", 15);
> + fbi->fix.smem_start = (unsigned long)sdrm->fb_base;
> + fbi->fix.smem_len = sdrm->fb_size;
> + fbi->screen_base = sdrm->fb_map;
> +
> + return 0;
> +
> +err_fb_info_destroy:
> + drm_fb_helper_release_fbi(helper);
> +
> + return ret;
> +}
> +
> +static const struct drm_fb_helper_funcs sdrm_fb_helper_funcs = {
> + .fb_probe = sdrm_fbdev_create,
> +};
> +
> +void sdrm_fbdev_init(struct sdrm_device *sdrm)
> +{
> + struct drm_device *ddev = sdrm->ddev;
> + struct drm_fb_helper *fb_helper;
> + struct sdrm_fbdev *fbdev;
> + int ret;
> +
> + fbdev = kzalloc(sizeof(*fbdev), GFP_KERNEL);
> + if (!fbdev) {
> + dev_err(ddev->dev, "Failed to allocate drm fbdev.\n");
> + return;
> + }
> +
> + fb_helper = &fbdev->fb_helper;
> +
> + drm_fb_helper_prepare(ddev, fb_helper, &sdrm_fb_helper_funcs);
> +
> + ret = drm_fb_helper_init(ddev, fb_helper, 1, 1);
> + if (ret < 0) {
> + dev_err(ddev->dev, "Failed to initialize drm fb helper.\n");
> + goto err_free;
> + }
> +
> + ret = drm_fb_helper_single_add_all_connectors(fb_helper);
> + if (ret < 0) {
> + dev_err(ddev->dev, "Failed to add connectors.\n");
> + goto err_drm_fb_helper_fini;
> + }
> +
> + ret = drm_fb_helper_initial_config(fb_helper,
> + ddev->mode_config.preferred_depth);
> + if (ret < 0) {
> + dev_err(ddev->dev, "Failed to set initial hw configuration.\n");
> + goto err_drm_fb_helper_fini;
> + }
> +
> + sdrm->fbdev = fbdev;
> +
> + return;
> +
> +err_drm_fb_helper_fini:
> + drm_fb_helper_fini(fb_helper);
> +err_free:
> + kfree(fbdev);
> +}
> +
> +void sdrm_fbdev_cleanup(struct sdrm_device *sdrm)
> +{
> + struct sdrm_fbdev *fbdev = sdrm->fbdev;
> + struct drm_fb_helper *fb_helper;
> +
> + if (!fbdev)
> + return;
> +
> + sdrm->fbdev = NULL;
> + fb_helper = &fbdev->fb_helper;
> +
> + drm_fb_helper_unregister_fbi(fb_helper);
> + drm_fb_helper_release_fbi(fb_helper);
> +
> + drm_framebuffer_unregister_private(&fbdev->fb);
> + drm_framebuffer_cleanup(&fbdev->fb);
> +
> + drm_fb_helper_fini(fb_helper);
> + kfree(fbdev);
> +}
> +
> +static void sdrm_fbdev_set_suspend(struct fb_info *fbi, int state)
> +{
> + console_lock();
> + fb_set_suspend(fbi, state);
Pls use the drm_fb_helper.c wrapper for this. Did you check that sdrm
still compiles even with CONFIG_FB=n?
> + console_unlock();
> +}
> +
> +/*
> + * Since fbdev is using the native framebuffer, fbcon has to be disabled
> + * when the drm stack is used.
> + */
Tbh I wonder whether this really is still worth it, after all your work to
make fbdev defio work smoothly. I think it'd be better if we give fbdev
normal framebuffers too and just depend upon all the defio/dirty handling
that's not wired up. Less complexity ftw ;-)
-Daniel
> +void sdrm_fbdev_display_pipe_update(struct sdrm_device *sdrm,
> + struct drm_framebuffer *fb)
> +{
> + struct sdrm_fbdev *fbdev = sdrm->fbdev;
> +
> + if (!fbdev || fbdev->fb_helper.fb == fb)
> + return;
> +
> + if (fbdev->fb_helper.fbdev->state == FBINFO_STATE_RUNNING)
> + sdrm_fbdev_set_suspend(fbdev->fb_helper.fbdev, 1);
> +}
> +
> +void sdrm_fbdev_restore_mode(struct sdrm_device *sdrm)
> +{
> + struct sdrm_fbdev *fbdev = sdrm->fbdev;
> +
> + if (!fbdev)
> + return;
> +
> + drm_fb_helper_restore_fbdev_mode_unlocked(&fbdev->fb_helper);
> +
> + if (fbdev->fb_helper.fbdev->state != FBINFO_STATE_RUNNING)
> + sdrm_fbdev_set_suspend(fbdev->fb_helper.fbdev, 0);
> +}
> diff --git a/drivers/gpu/drm/simpledrm/simpledrm_kms.c b/drivers/gpu/drm/simpledrm/simpledrm_kms.c
> index 00e50d9..92ddc116 100644
> --- a/drivers/gpu/drm/simpledrm/simpledrm_kms.c
> +++ b/drivers/gpu/drm/simpledrm/simpledrm_kms.c
> @@ -24,6 +24,13 @@ static const uint32_t sdrm_formats[] = {
> DRM_FORMAT_XRGB8888,
> };
>
> +void sdrm_lastclose(struct drm_device *ddev)
> +{
> + struct sdrm_device *sdrm = ddev->dev_private;
> +
> + sdrm_fbdev_restore_mode(sdrm);
> +}
> +
> static int sdrm_conn_get_modes(struct drm_connector *conn)
> {
> struct sdrm_device *sdrm = conn->dev->dev_private;
> @@ -91,8 +98,9 @@ void sdrm_display_pipe_update(struct drm_simple_display_pipe *pipe,
> struct sdrm_device *sdrm = pipe_to_sdrm(pipe);
>
> sdrm_crtc_send_vblank_event(&pipe->crtc);
> + sdrm_fbdev_display_pipe_update(sdrm, fb);
>
> - if (fb) {
> + if (fb && fb->funcs->dirty) {
> pipe->plane.fb = fb;
> sdrm_dirty_all_locked(sdrm);
> }
> --
> 2.8.2
>
> _______________________________________________
> 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
[toc] | [prev] | [next] | [standalone]
| From | Noralf Trønnes <noralf@tronnes.org> |
|---|---|
| Date | 2016-08-16 15:20 +0200 |
| Subject | Re: [PATCH v3 2/3] drm: simpledrm: add fbdev fallback support |
| Message-ID | <s6MrD-3zL-1@gated-at.bofh.it> |
| In reply to | #1462572 |
Den 15.08.2016 08:48, skrev Daniel Vetter:
> On Sun, Aug 14, 2016 at 06:52:05PM +0200, Noralf Trønnes wrote:
>> Create a simple fbdev device during SimpleDRM setup so legacy user-space
>> and fbcon can use it.
>>
>> Original work by David Herrmann.
>>
>> Cc: dh.herrmann@gmail.com
>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
>> ---
>>
>> Changes from version 2:
>> - Switch to using drm_fb_helper in preparation for future panic handling
>> which needs an enabled pipeline.
>>
>> Changes from version 1:
>> No changes
>>
>> Changes from previous version:
>> - Remove the DRM_SIMPLEDRM_FBDEV kconfig option and use DRM_FBDEV_EMULATION
>> - Suspend fbcon/fbdev when the pipeline is enabled, resume in lastclose
>> - Add FBINFO_CAN_FORCE_OUTPUT flag so we get oops'es on the console
>>
>> drivers/gpu/drm/simpledrm/Kconfig | 3 +
>> drivers/gpu/drm/simpledrm/Makefile | 1 +
>> drivers/gpu/drm/simpledrm/simpledrm.h | 32 ++++-
>> drivers/gpu/drm/simpledrm/simpledrm_drv.c | 4 +
>> drivers/gpu/drm/simpledrm/simpledrm_fbdev.c | 201 ++++++++++++++++++++++++++++
>> drivers/gpu/drm/simpledrm/simpledrm_kms.c | 10 +-
>> 6 files changed, 249 insertions(+), 2 deletions(-)
>> create mode 100644 drivers/gpu/drm/simpledrm/simpledrm_fbdev.c
>>
>> diff --git a/drivers/gpu/drm/simpledrm/Kconfig b/drivers/gpu/drm/simpledrm/Kconfig
>> index f45b25d..9454536 100644
>> --- a/drivers/gpu/drm/simpledrm/Kconfig
>> +++ b/drivers/gpu/drm/simpledrm/Kconfig
>> @@ -13,6 +13,9 @@ config DRM_SIMPLEDRM
>> SimpleDRM supports "simple-framebuffer" DeviceTree objects and
>> compatible platform framebuffers.
>>
>> + If fbdev support is enabled, this driver will also provide an fbdev
>> + compatibility layer.
>> +
>> If unsure, say Y.
>>
>> To compile this driver as a module, choose M here: the
>> diff --git a/drivers/gpu/drm/simpledrm/Makefile b/drivers/gpu/drm/simpledrm/Makefile
>> index f6a62dc..7087245 100644
>> --- a/drivers/gpu/drm/simpledrm/Makefile
>> +++ b/drivers/gpu/drm/simpledrm/Makefile
>> @@ -1,4 +1,5 @@
>> simpledrm-y := simpledrm_drv.o simpledrm_kms.o simpledrm_gem.o \
>> simpledrm_damage.o
>> +simpledrm-$(CONFIG_DRM_FBDEV_EMULATION) += simpledrm_fbdev.o
>>
>> obj-$(CONFIG_DRM_SIMPLEDRM) := simpledrm.o
>> diff --git a/drivers/gpu/drm/simpledrm/simpledrm.h b/drivers/gpu/drm/simpledrm/simpledrm.h
>> index 481acc2..f01b75d 100644
>> --- a/drivers/gpu/drm/simpledrm/simpledrm.h
>> +++ b/drivers/gpu/drm/simpledrm/simpledrm.h
>> @@ -17,13 +17,13 @@
>>
>> struct simplefb_format;
>> struct regulator;
>> -struct fb_info;
>> struct clk;
>>
>> struct sdrm_device {
>> struct drm_device *ddev;
>> struct drm_simple_display_pipe pipe;
>> struct drm_connector conn;
>> + struct sdrm_fbdev *fbdev;
>>
>> /* framebuffer information */
>> const struct simplefb_format *fb_sformat;
>> @@ -46,6 +46,7 @@ struct sdrm_device {
>> #endif
>> };
>>
>> +void sdrm_lastclose(struct drm_device *ddev);
>> int sdrm_drm_modeset_init(struct sdrm_device *sdrm);
>> int sdrm_drm_mmap(struct file *filp, struct vm_area_struct *vma);
>>
>> @@ -87,4 +88,33 @@ struct sdrm_framebuffer {
>>
>> #define to_sdrm_fb(x) container_of(x, struct sdrm_framebuffer, base)
>>
>> +#ifdef CONFIG_DRM_FBDEV_EMULATION
>> +
>> +void sdrm_fbdev_init(struct sdrm_device *sdrm);
>> +void sdrm_fbdev_cleanup(struct sdrm_device *sdrm);
>> +void sdrm_fbdev_display_pipe_update(struct sdrm_device *sdrm,
>> + struct drm_framebuffer *fb);
>> +void sdrm_fbdev_restore_mode(struct sdrm_device *sdrm);
>> +
>> +#else
>> +
>> +static inline void sdrm_fbdev_init(struct sdrm_device *sdrm)
>> +{
>> +}
>> +
>> +static inline void sdrm_fbdev_cleanup(struct sdrm_device *sdrm)
>> +{
>> +}
>> +
>> +static inline void sdrm_fbdev_display_pipe_update(struct sdrm_device *sdrm,
>> + struct drm_framebuffer *fb)
>> +{
>> +}
>> +
>> +static inline void sdrm_fbdev_restore_mode(struct sdrm_device *sdrm)
>> +{
>> +}
>> +
>> +#endif
> Why do we need the #ifdefs here? Imo those few bytes of codes we can save
> aren't worth the complexity ...
This one is needed to make fbdev optional, which I assume is what we want?
Actually I can drop sdrm_fbdev_display_pipe_update() and
sdrm_fbdev_restore_mode() if I use drm_fb_helper_set_suspend() as you
remineded me of further down.
Or do you actually refer to the #ifdef's around clk and regulator in
struct sdrm_device? I lifted that as-is from simplefb.c, but I wondered
if I shouldn't just have dropped them since it was only 2 pointers and
2 ints.
<snip>
>> diff --git a/drivers/gpu/drm/simpledrm/simpledrm_fbdev.c b/drivers/gpu/drm/simpledrm/simpledrm_fbdev.c
>> new file mode 100644
>> index 0000000..4038dd9
>> --- /dev/null
>> +++ b/drivers/gpu/drm/simpledrm/simpledrm_fbdev.c
>> @@ -0,0 +1,201 @@
>> +/*
>> + * SimpleDRM firmware framebuffer driver
>> + * Copyright (c) 2012-2014 David Herrmann <dh.herrmann@gmail.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms of the GNU General Public License as published by the Free
>> + * Software Foundation; either version 2 of the License, or (at your option)
>> + * any later version.
>> + */
>> +
>> +#include <drm/drmP.h>
>> +#include <drm/drm_crtc_helper.h>
>> +#include <drm/drm_fb_helper.h>
>> +#include <linux/console.h>
>> +#include <linux/fb.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/platform_data/simplefb.h>
>> +
>> +#include "simpledrm.h"
>> +
>> +struct sdrm_fbdev {
>> + struct drm_fb_helper fb_helper;
>> + struct drm_framebuffer fb;
>> +};
>> +
>> +static inline struct sdrm_fbdev *to_sdrm_fbdev(struct drm_fb_helper *helper)
>> +{
>> + return container_of(helper, struct sdrm_fbdev, fb_helper);
>> +}
>> +
>> +static struct fb_ops sdrm_fbdev_ops = {
>> + .owner = THIS_MODULE,
>> + .fb_fillrect = drm_fb_helper_cfb_fillrect,
>> + .fb_copyarea = drm_fb_helper_cfb_copyarea,
>> + .fb_imageblit = drm_fb_helper_cfb_imageblit,
>> + .fb_check_var = drm_fb_helper_check_var,
>> + .fb_set_par = drm_fb_helper_set_par,
>> + .fb_setcmap = drm_fb_helper_setcmap,
>> +};
>> +
>> +static struct drm_framebuffer_funcs sdrm_fb_funcs = {
>> + .destroy = drm_framebuffer_cleanup,
>> +};
>> +
>> +static int sdrm_fbdev_create(struct drm_fb_helper *helper,
>> + struct drm_fb_helper_surface_size *sizes)
>> +{
>> + struct sdrm_fbdev *fbdev = to_sdrm_fbdev(helper);
>> + struct drm_device *ddev = helper->dev;
>> + struct sdrm_device *sdrm = ddev->dev_private;
>> + struct drm_framebuffer *fb = &fbdev->fb;
>> + struct drm_mode_fb_cmd2 mode_cmd = {
>> + .width = sdrm->fb_width,
>> + .height = sdrm->fb_height,
>> + .pitches[0] = sdrm->fb_stride,
>> + .pixel_format = sdrm->fb_format,
>> + };
>> + struct fb_info *fbi;
>> + int ret;
>> +
>> + fbi = drm_fb_helper_alloc_fbi(helper);
>> + if (IS_ERR(fbi))
>> + return PTR_ERR(fbi);
>> +
>> + drm_helper_mode_fill_fb_struct(fb, &mode_cmd);
>> +
>> + ret = drm_framebuffer_init(ddev, fb, &sdrm_fb_funcs);
>> + if (ret) {
>> + dev_err(ddev->dev, "Failed to initialize framebuffer: %d\n", ret);
>> + goto err_fb_info_destroy;
>> + }
>> +
>> + helper->fb = fb;
>> + fbi->par = helper;
>> +
>> + fbi->flags = FBINFO_DEFAULT | FBINFO_MISC_FIRMWARE |
>> + FBINFO_CAN_FORCE_OUTPUT;
>> + fbi->fbops = &sdrm_fbdev_ops;
>> +
>> + drm_fb_helper_fill_fix(fbi, fb->pitches[0], fb->depth);
>> + drm_fb_helper_fill_var(fbi, helper, fb->width, fb->height);
>> +
>> + strncpy(fbi->fix.id, "simpledrmfb", 15);
>> + fbi->fix.smem_start = (unsigned long)sdrm->fb_base;
>> + fbi->fix.smem_len = sdrm->fb_size;
>> + fbi->screen_base = sdrm->fb_map;
>> +
>> + return 0;
>> +
>> +err_fb_info_destroy:
>> + drm_fb_helper_release_fbi(helper);
>> +
>> + return ret;
>> +}
>> +
>> +static const struct drm_fb_helper_funcs sdrm_fb_helper_funcs = {
>> + .fb_probe = sdrm_fbdev_create,
>> +};
>> +
>> +void sdrm_fbdev_init(struct sdrm_device *sdrm)
>> +{
>> + struct drm_device *ddev = sdrm->ddev;
>> + struct drm_fb_helper *fb_helper;
>> + struct sdrm_fbdev *fbdev;
>> + int ret;
>> +
>> + fbdev = kzalloc(sizeof(*fbdev), GFP_KERNEL);
>> + if (!fbdev) {
>> + dev_err(ddev->dev, "Failed to allocate drm fbdev.\n");
>> + return;
>> + }
>> +
>> + fb_helper = &fbdev->fb_helper;
>> +
>> + drm_fb_helper_prepare(ddev, fb_helper, &sdrm_fb_helper_funcs);
>> +
>> + ret = drm_fb_helper_init(ddev, fb_helper, 1, 1);
>> + if (ret < 0) {
>> + dev_err(ddev->dev, "Failed to initialize drm fb helper.\n");
>> + goto err_free;
>> + }
>> +
>> + ret = drm_fb_helper_single_add_all_connectors(fb_helper);
>> + if (ret < 0) {
>> + dev_err(ddev->dev, "Failed to add connectors.\n");
>> + goto err_drm_fb_helper_fini;
>> + }
>> +
>> + ret = drm_fb_helper_initial_config(fb_helper,
>> + ddev->mode_config.preferred_depth);
>> + if (ret < 0) {
>> + dev_err(ddev->dev, "Failed to set initial hw configuration.\n");
>> + goto err_drm_fb_helper_fini;
>> + }
>> +
>> + sdrm->fbdev = fbdev;
>> +
>> + return;
>> +
>> +err_drm_fb_helper_fini:
>> + drm_fb_helper_fini(fb_helper);
>> +err_free:
>> + kfree(fbdev);
>> +}
>> +
>> +void sdrm_fbdev_cleanup(struct sdrm_device *sdrm)
>> +{
>> + struct sdrm_fbdev *fbdev = sdrm->fbdev;
>> + struct drm_fb_helper *fb_helper;
>> +
>> + if (!fbdev)
>> + return;
>> +
>> + sdrm->fbdev = NULL;
>> + fb_helper = &fbdev->fb_helper;
>> +
>> + drm_fb_helper_unregister_fbi(fb_helper);
>> + drm_fb_helper_release_fbi(fb_helper);
>> +
>> + drm_framebuffer_unregister_private(&fbdev->fb);
>> + drm_framebuffer_cleanup(&fbdev->fb);
>> +
>> + drm_fb_helper_fini(fb_helper);
>> + kfree(fbdev);
>> +}
>> +
>> +static void sdrm_fbdev_set_suspend(struct fb_info *fbi, int state)
>> +{
>> + console_lock();
>> + fb_set_suspend(fbi, state);
> Pls use the drm_fb_helper.c wrapper for this. Did you check that sdrm
> still compiles even with CONFIG_FB=n?
This file is only built if CONFIG_DRM_FBDEV_EMULATION=y, so it's safe,
I've tested that. I forgot about that wrapper, so this function can be
dropped, and I'll just open code it in pipe_update() and lastclose().
In the remove_conflicting_framebuffers patch I switch to using CONFIG_FB
instead of CONFIG_DRM_FBDEV_EMULATION to be on the safe side since I don't
know if all drivers use CONFIG_DRM_FBDEV_EMULATION.
But now I remember that CONFIG_DRM_FBDEV_EMULATION is default yes, so
I think I'll stick to that instead of switching to CONFIG_FB.
Noralf.
>> + console_unlock();
>> +}
>> +
>> +/*
>> + * Since fbdev is using the native framebuffer, fbcon has to be disabled
>> + * when the drm stack is used.
>> + */
> Tbh I wonder whether this really is still worth it, after all your work to
> make fbdev defio work smoothly. I think it'd be better if we give fbdev
> normal framebuffers too and just depend upon all the defio/dirty handling
> that's not wired up. Less complexity ftw ;-)
> -Daniel
>
>> +void sdrm_fbdev_display_pipe_update(struct sdrm_device *sdrm,
>> + struct drm_framebuffer *fb)
>> +{
>> + struct sdrm_fbdev *fbdev = sdrm->fbdev;
>> +
>> + if (!fbdev || fbdev->fb_helper.fb == fb)
>> + return;
>> +
>> + if (fbdev->fb_helper.fbdev->state == FBINFO_STATE_RUNNING)
>> + sdrm_fbdev_set_suspend(fbdev->fb_helper.fbdev, 1);
>> +}
>> +
>> +void sdrm_fbdev_restore_mode(struct sdrm_device *sdrm)
>> +{
>> + struct sdrm_fbdev *fbdev = sdrm->fbdev;
>> +
>> + if (!fbdev)
>> + return;
>> +
>> + drm_fb_helper_restore_fbdev_mode_unlocked(&fbdev->fb_helper);
>> +
>> + if (fbdev->fb_helper.fbdev->state != FBINFO_STATE_RUNNING)
>> + sdrm_fbdev_set_suspend(fbdev->fb_helper.fbdev, 0);
>> +}
>> diff --git a/drivers/gpu/drm/simpledrm/simpledrm_kms.c b/drivers/gpu/drm/simpledrm/simpledrm_kms.c
>> index 00e50d9..92ddc116 100644
>> --- a/drivers/gpu/drm/simpledrm/simpledrm_kms.c
>> +++ b/drivers/gpu/drm/simpledrm/simpledrm_kms.c
>> @@ -24,6 +24,13 @@ static const uint32_t sdrm_formats[] = {
>> DRM_FORMAT_XRGB8888,
>> };
>>
>> +void sdrm_lastclose(struct drm_device *ddev)
>> +{
>> + struct sdrm_device *sdrm = ddev->dev_private;
>> +
>> + sdrm_fbdev_restore_mode(sdrm);
>> +}
>> +
>> static int sdrm_conn_get_modes(struct drm_connector *conn)
>> {
>> struct sdrm_device *sdrm = conn->dev->dev_private;
>> @@ -91,8 +98,9 @@ void sdrm_display_pipe_update(struct drm_simple_display_pipe *pipe,
>> struct sdrm_device *sdrm = pipe_to_sdrm(pipe);
>>
>> sdrm_crtc_send_vblank_event(&pipe->crtc);
>> + sdrm_fbdev_display_pipe_update(sdrm, fb);
>>
>> - if (fb) {
>> + if (fb && fb->funcs->dirty) {
>> pipe->plane.fb = fb;
>> sdrm_dirty_all_locked(sdrm);
>> }
>> --
>> 2.8.2
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
[toc] | [prev] | [next] | [standalone]
| From | Daniel Vetter <daniel@ffwll.ch> |
|---|---|
| Date | 2016-08-16 17:40 +0200 |
| Subject | Re: [PATCH v3 2/3] drm: simpledrm: add fbdev fallback support |
| Message-ID | <s6OD8-4QG-13@gated-at.bofh.it> |
| In reply to | #1463771 |
On Tue, Aug 16, 2016 at 03:10:06PM +0200, Noralf Trønnes wrote:
>
> Den 15.08.2016 08:48, skrev Daniel Vetter:
> > On Sun, Aug 14, 2016 at 06:52:05PM +0200, Noralf Trønnes wrote:
> > > Create a simple fbdev device during SimpleDRM setup so legacy user-space
> > > and fbcon can use it.
> > >
> > > Original work by David Herrmann.
> > >
> > > Cc: dh.herrmann@gmail.com
> > > Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> > > ---
> > >
> > > Changes from version 2:
> > > - Switch to using drm_fb_helper in preparation for future panic handling
> > > which needs an enabled pipeline.
> > >
> > > Changes from version 1:
> > > No changes
> > >
> > > Changes from previous version:
> > > - Remove the DRM_SIMPLEDRM_FBDEV kconfig option and use DRM_FBDEV_EMULATION
> > > - Suspend fbcon/fbdev when the pipeline is enabled, resume in lastclose
> > > - Add FBINFO_CAN_FORCE_OUTPUT flag so we get oops'es on the console
> > >
> > > drivers/gpu/drm/simpledrm/Kconfig | 3 +
> > > drivers/gpu/drm/simpledrm/Makefile | 1 +
> > > drivers/gpu/drm/simpledrm/simpledrm.h | 32 ++++-
> > > drivers/gpu/drm/simpledrm/simpledrm_drv.c | 4 +
> > > drivers/gpu/drm/simpledrm/simpledrm_fbdev.c | 201 ++++++++++++++++++++++++++++
> > > drivers/gpu/drm/simpledrm/simpledrm_kms.c | 10 +-
> > > 6 files changed, 249 insertions(+), 2 deletions(-)
> > > create mode 100644 drivers/gpu/drm/simpledrm/simpledrm_fbdev.c
> > >
> > > diff --git a/drivers/gpu/drm/simpledrm/Kconfig b/drivers/gpu/drm/simpledrm/Kconfig
> > > index f45b25d..9454536 100644
> > > --- a/drivers/gpu/drm/simpledrm/Kconfig
> > > +++ b/drivers/gpu/drm/simpledrm/Kconfig
> > > @@ -13,6 +13,9 @@ config DRM_SIMPLEDRM
> > > SimpleDRM supports "simple-framebuffer" DeviceTree objects and
> > > compatible platform framebuffers.
> > >
> > > + If fbdev support is enabled, this driver will also provide an fbdev
> > > + compatibility layer.
> > > +
> > > If unsure, say Y.
> > >
> > > To compile this driver as a module, choose M here: the
> > > diff --git a/drivers/gpu/drm/simpledrm/Makefile b/drivers/gpu/drm/simpledrm/Makefile
> > > index f6a62dc..7087245 100644
> > > --- a/drivers/gpu/drm/simpledrm/Makefile
> > > +++ b/drivers/gpu/drm/simpledrm/Makefile
> > > @@ -1,4 +1,5 @@
> > > simpledrm-y := simpledrm_drv.o simpledrm_kms.o simpledrm_gem.o \
> > > simpledrm_damage.o
> > > +simpledrm-$(CONFIG_DRM_FBDEV_EMULATION) += simpledrm_fbdev.o
> > >
> > > obj-$(CONFIG_DRM_SIMPLEDRM) := simpledrm.o
> > > diff --git a/drivers/gpu/drm/simpledrm/simpledrm.h b/drivers/gpu/drm/simpledrm/simpledrm.h
> > > index 481acc2..f01b75d 100644
> > > --- a/drivers/gpu/drm/simpledrm/simpledrm.h
> > > +++ b/drivers/gpu/drm/simpledrm/simpledrm.h
> > > @@ -17,13 +17,13 @@
> > >
> > > struct simplefb_format;
> > > struct regulator;
> > > -struct fb_info;
> > > struct clk;
> > >
> > > struct sdrm_device {
> > > struct drm_device *ddev;
> > > struct drm_simple_display_pipe pipe;
> > > struct drm_connector conn;
> > > + struct sdrm_fbdev *fbdev;
> > >
> > > /* framebuffer information */
> > > const struct simplefb_format *fb_sformat;
> > > @@ -46,6 +46,7 @@ struct sdrm_device {
> > > #endif
> > > };
> > >
> > > +void sdrm_lastclose(struct drm_device *ddev);
> > > int sdrm_drm_modeset_init(struct sdrm_device *sdrm);
> > > int sdrm_drm_mmap(struct file *filp, struct vm_area_struct *vma);
> > >
> > > @@ -87,4 +88,33 @@ struct sdrm_framebuffer {
> > >
> > > #define to_sdrm_fb(x) container_of(x, struct sdrm_framebuffer, base)
> > >
> > > +#ifdef CONFIG_DRM_FBDEV_EMULATION
> > > +
> > > +void sdrm_fbdev_init(struct sdrm_device *sdrm);
> > > +void sdrm_fbdev_cleanup(struct sdrm_device *sdrm);
> > > +void sdrm_fbdev_display_pipe_update(struct sdrm_device *sdrm,
> > > + struct drm_framebuffer *fb);
> > > +void sdrm_fbdev_restore_mode(struct sdrm_device *sdrm);
> > > +
> > > +#else
> > > +
> > > +static inline void sdrm_fbdev_init(struct sdrm_device *sdrm)
> > > +{
> > > +}
> > > +
> > > +static inline void sdrm_fbdev_cleanup(struct sdrm_device *sdrm)
> > > +{
> > > +}
> > > +
> > > +static inline void sdrm_fbdev_display_pipe_update(struct sdrm_device *sdrm,
> > > + struct drm_framebuffer *fb)
> > > +{
> > > +}
> > > +
> > > +static inline void sdrm_fbdev_restore_mode(struct sdrm_device *sdrm)
> > > +{
> > > +}
> > > +
> > > +#endif
> > Why do we need the #ifdefs here? Imo those few bytes of codes we can save
> > aren't worth the complexity ...
>
> This one is needed to make fbdev optional, which I assume is what we want?
> Actually I can drop sdrm_fbdev_display_pipe_update() and
> sdrm_fbdev_restore_mode() if I use drm_fb_helper_set_suspend() as you
> remineded me of further down.
>
> Or do you actually refer to the #ifdef's around clk and regulator in
> struct sdrm_device? I lifted that as-is from simplefb.c, but I wondered
> if I shouldn't just have dropped them since it was only 2 pointers and
> 2 ints.
Yeah those ifdefs seem overkill, they make the code ugly for miniscule
gain. But I meant the above ifdef - if you drop them (and unconditionally
compile simpledrm_fbdev.c) then sure there's a bit of code size overhead
for the FB=n case, but not much. But the clear upside is less confusion in
the code. I know that i915 makes the entire file optional, but that's
because there's still a few space hacks in i915 that haven't been ported
to the core helpers yet, and because the optional i915 fbdev support
started before we had it in the core (that's only very recently).
But today I don't think the ifdef are worth it, the compile-out support
from the core should be sufficient.
>
> <snip>
>
> > > diff --git a/drivers/gpu/drm/simpledrm/simpledrm_fbdev.c b/drivers/gpu/drm/simpledrm/simpledrm_fbdev.c
> > > new file mode 100644
> > > index 0000000..4038dd9
> > > --- /dev/null
> > > +++ b/drivers/gpu/drm/simpledrm/simpledrm_fbdev.c
> > > @@ -0,0 +1,201 @@
> > > +/*
> > > + * SimpleDRM firmware framebuffer driver
> > > + * Copyright (c) 2012-2014 David Herrmann <dh.herrmann@gmail.com>
> > > + *
> > > + * This program is free software; you can redistribute it and/or modify it
> > > + * under the terms of the GNU General Public License as published by the Free
> > > + * Software Foundation; either version 2 of the License, or (at your option)
> > > + * any later version.
> > > + */
> > > +
> > > +#include <drm/drmP.h>
> > > +#include <drm/drm_crtc_helper.h>
> > > +#include <drm/drm_fb_helper.h>
> > > +#include <linux/console.h>
> > > +#include <linux/fb.h>
> > > +#include <linux/platform_device.h>
> > > +#include <linux/platform_data/simplefb.h>
> > > +
> > > +#include "simpledrm.h"
> > > +
> > > +struct sdrm_fbdev {
> > > + struct drm_fb_helper fb_helper;
> > > + struct drm_framebuffer fb;
> > > +};
> > > +
> > > +static inline struct sdrm_fbdev *to_sdrm_fbdev(struct drm_fb_helper *helper)
> > > +{
> > > + return container_of(helper, struct sdrm_fbdev, fb_helper);
> > > +}
> > > +
> > > +static struct fb_ops sdrm_fbdev_ops = {
> > > + .owner = THIS_MODULE,
> > > + .fb_fillrect = drm_fb_helper_cfb_fillrect,
> > > + .fb_copyarea = drm_fb_helper_cfb_copyarea,
> > > + .fb_imageblit = drm_fb_helper_cfb_imageblit,
> > > + .fb_check_var = drm_fb_helper_check_var,
> > > + .fb_set_par = drm_fb_helper_set_par,
> > > + .fb_setcmap = drm_fb_helper_setcmap,
> > > +};
> > > +
> > > +static struct drm_framebuffer_funcs sdrm_fb_funcs = {
> > > + .destroy = drm_framebuffer_cleanup,
> > > +};
> > > +
> > > +static int sdrm_fbdev_create(struct drm_fb_helper *helper,
> > > + struct drm_fb_helper_surface_size *sizes)
> > > +{
> > > + struct sdrm_fbdev *fbdev = to_sdrm_fbdev(helper);
> > > + struct drm_device *ddev = helper->dev;
> > > + struct sdrm_device *sdrm = ddev->dev_private;
> > > + struct drm_framebuffer *fb = &fbdev->fb;
> > > + struct drm_mode_fb_cmd2 mode_cmd = {
> > > + .width = sdrm->fb_width,
> > > + .height = sdrm->fb_height,
> > > + .pitches[0] = sdrm->fb_stride,
> > > + .pixel_format = sdrm->fb_format,
> > > + };
> > > + struct fb_info *fbi;
> > > + int ret;
> > > +
> > > + fbi = drm_fb_helper_alloc_fbi(helper);
> > > + if (IS_ERR(fbi))
> > > + return PTR_ERR(fbi);
> > > +
> > > + drm_helper_mode_fill_fb_struct(fb, &mode_cmd);
> > > +
> > > + ret = drm_framebuffer_init(ddev, fb, &sdrm_fb_funcs);
> > > + if (ret) {
> > > + dev_err(ddev->dev, "Failed to initialize framebuffer: %d\n", ret);
> > > + goto err_fb_info_destroy;
> > > + }
> > > +
> > > + helper->fb = fb;
> > > + fbi->par = helper;
> > > +
> > > + fbi->flags = FBINFO_DEFAULT | FBINFO_MISC_FIRMWARE |
> > > + FBINFO_CAN_FORCE_OUTPUT;
> > > + fbi->fbops = &sdrm_fbdev_ops;
> > > +
> > > + drm_fb_helper_fill_fix(fbi, fb->pitches[0], fb->depth);
> > > + drm_fb_helper_fill_var(fbi, helper, fb->width, fb->height);
> > > +
> > > + strncpy(fbi->fix.id, "simpledrmfb", 15);
> > > + fbi->fix.smem_start = (unsigned long)sdrm->fb_base;
> > > + fbi->fix.smem_len = sdrm->fb_size;
> > > + fbi->screen_base = sdrm->fb_map;
> > > +
> > > + return 0;
> > > +
> > > +err_fb_info_destroy:
> > > + drm_fb_helper_release_fbi(helper);
> > > +
> > > + return ret;
> > > +}
> > > +
> > > +static const struct drm_fb_helper_funcs sdrm_fb_helper_funcs = {
> > > + .fb_probe = sdrm_fbdev_create,
> > > +};
> > > +
> > > +void sdrm_fbdev_init(struct sdrm_device *sdrm)
> > > +{
> > > + struct drm_device *ddev = sdrm->ddev;
> > > + struct drm_fb_helper *fb_helper;
> > > + struct sdrm_fbdev *fbdev;
> > > + int ret;
> > > +
> > > + fbdev = kzalloc(sizeof(*fbdev), GFP_KERNEL);
> > > + if (!fbdev) {
> > > + dev_err(ddev->dev, "Failed to allocate drm fbdev.\n");
> > > + return;
> > > + }
> > > +
> > > + fb_helper = &fbdev->fb_helper;
> > > +
> > > + drm_fb_helper_prepare(ddev, fb_helper, &sdrm_fb_helper_funcs);
> > > +
> > > + ret = drm_fb_helper_init(ddev, fb_helper, 1, 1);
> > > + if (ret < 0) {
> > > + dev_err(ddev->dev, "Failed to initialize drm fb helper.\n");
> > > + goto err_free;
> > > + }
> > > +
> > > + ret = drm_fb_helper_single_add_all_connectors(fb_helper);
> > > + if (ret < 0) {
> > > + dev_err(ddev->dev, "Failed to add connectors.\n");
> > > + goto err_drm_fb_helper_fini;
> > > + }
> > > +
> > > + ret = drm_fb_helper_initial_config(fb_helper,
> > > + ddev->mode_config.preferred_depth);
> > > + if (ret < 0) {
> > > + dev_err(ddev->dev, "Failed to set initial hw configuration.\n");
> > > + goto err_drm_fb_helper_fini;
> > > + }
> > > +
> > > + sdrm->fbdev = fbdev;
> > > +
> > > + return;
> > > +
> > > +err_drm_fb_helper_fini:
> > > + drm_fb_helper_fini(fb_helper);
> > > +err_free:
> > > + kfree(fbdev);
> > > +}
> > > +
> > > +void sdrm_fbdev_cleanup(struct sdrm_device *sdrm)
> > > +{
> > > + struct sdrm_fbdev *fbdev = sdrm->fbdev;
> > > + struct drm_fb_helper *fb_helper;
> > > +
> > > + if (!fbdev)
> > > + return;
> > > +
> > > + sdrm->fbdev = NULL;
> > > + fb_helper = &fbdev->fb_helper;
> > > +
> > > + drm_fb_helper_unregister_fbi(fb_helper);
> > > + drm_fb_helper_release_fbi(fb_helper);
> > > +
> > > + drm_framebuffer_unregister_private(&fbdev->fb);
> > > + drm_framebuffer_cleanup(&fbdev->fb);
> > > +
> > > + drm_fb_helper_fini(fb_helper);
> > > + kfree(fbdev);
> > > +}
> > > +
> > > +static void sdrm_fbdev_set_suspend(struct fb_info *fbi, int state)
> > > +{
> > > + console_lock();
> > > + fb_set_suspend(fbi, state);
> > Pls use the drm_fb_helper.c wrapper for this. Did you check that sdrm
> > still compiles even with CONFIG_FB=n?
>
> This file is only built if CONFIG_DRM_FBDEV_EMULATION=y, so it's safe,
> I've tested that. I forgot about that wrapper, so this function can be
> dropped, and I'll just open code it in pipe_update() and lastclose().
See above, but I think excluding files like that and trading it in for a
pile of ifdef isn't really worth it. Very little code is left as-is, since
the compiler will short-circuit most of it. It should even notice that the
fbdev emulation vfunc is unused and garbage-collect that + all the hooks,
leaving nothing behind.
> In the remove_conflicting_framebuffers patch I switch to using CONFIG_FB
> instead of CONFIG_DRM_FBDEV_EMULATION to be on the safe side since I don't
> know if all drivers use CONFIG_DRM_FBDEV_EMULATION.
> But now I remember that CONFIG_DRM_FBDEV_EMULATION is default yes, so
> I think I'll stick to that instead of switching to CONFIG_FB.
See latest drm-misc, there's a wrapper called
drm_fbdev_remove_conflicting_framebuffers. Just use that and drop the
ifdef.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
[toc] | [prev] | [next] | [standalone]
| From | Noralf Trønnes <noralf@tronnes.org> |
|---|---|
| Date | 2016-08-14 19:00 +0200 |
| Subject | [PATCH v3 3/3] drm: simpledrm: honour remove_conflicting_framebuffers() |
| Message-ID | <s66Vr-265-19@gated-at.bofh.it> |
| In reply to | #1462171 |
There is currently no non-fbdev mechanism in place to kick out
simpledrm when the real hw-driver is probed. As a stop gap until
that is in place, honour remove_conflicting_framebuffers() and
delete the simple-framebuffer platform device when it's called.
Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
Changes from version 2:
- Don't forget to free fb_info when kicked out.
drivers/gpu/drm/simpledrm/Kconfig | 5 +++
drivers/gpu/drm/simpledrm/Makefile | 2 +-
drivers/gpu/drm/simpledrm/simpledrm.h | 11 +++++-
drivers/gpu/drm/simpledrm/simpledrm_drv.c | 3 ++
drivers/gpu/drm/simpledrm/simpledrm_fbdev.c | 55 +++++++++++++++++++++++++++--
5 files changed, 72 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/simpledrm/Kconfig b/drivers/gpu/drm/simpledrm/Kconfig
index 9454536..6205b17 100644
--- a/drivers/gpu/drm/simpledrm/Kconfig
+++ b/drivers/gpu/drm/simpledrm/Kconfig
@@ -16,6 +16,11 @@ config DRM_SIMPLEDRM
If fbdev support is enabled, this driver will also provide an fbdev
compatibility layer.
+ WARNING
+ fbdev must be enabled for simpledrm to disable itself when a real
+ hw-driver is probed. It relies on remove_conflicting_framebuffers()
+ to be called by the hw-driver.
+
If unsure, say Y.
To compile this driver as a module, choose M here: the
diff --git a/drivers/gpu/drm/simpledrm/Makefile b/drivers/gpu/drm/simpledrm/Makefile
index 7087245..4b4bcdd 100644
--- a/drivers/gpu/drm/simpledrm/Makefile
+++ b/drivers/gpu/drm/simpledrm/Makefile
@@ -1,5 +1,5 @@
simpledrm-y := simpledrm_drv.o simpledrm_kms.o simpledrm_gem.o \
simpledrm_damage.o
-simpledrm-$(CONFIG_DRM_FBDEV_EMULATION) += simpledrm_fbdev.o
+simpledrm-$(CONFIG_FB) += simpledrm_fbdev.o
obj-$(CONFIG_DRM_SIMPLEDRM) := simpledrm.o
diff --git a/drivers/gpu/drm/simpledrm/simpledrm.h b/drivers/gpu/drm/simpledrm/simpledrm.h
index f01b75d..7bc1292 100644
--- a/drivers/gpu/drm/simpledrm/simpledrm.h
+++ b/drivers/gpu/drm/simpledrm/simpledrm.h
@@ -88,13 +88,15 @@ struct sdrm_framebuffer {
#define to_sdrm_fb(x) container_of(x, struct sdrm_framebuffer, base)
-#ifdef CONFIG_DRM_FBDEV_EMULATION
+#ifdef CONFIG_FB
void sdrm_fbdev_init(struct sdrm_device *sdrm);
void sdrm_fbdev_cleanup(struct sdrm_device *sdrm);
void sdrm_fbdev_display_pipe_update(struct sdrm_device *sdrm,
struct drm_framebuffer *fb);
void sdrm_fbdev_restore_mode(struct sdrm_device *sdrm);
+void sdrm_fbdev_kickout_init(void);
+void sdrm_fbdev_kickout_exit(void);
#else
@@ -115,6 +117,13 @@ static inline void sdrm_fbdev_restore_mode(struct sdrm_device *sdrm)
{
}
+static inline void sdrm_fbdev_kickout_init(void)
+{
+}
+
+static inline void sdrm_fbdev_kickout_exit(void)
+{
+}
#endif
#endif /* SDRM_DRV_H */
diff --git a/drivers/gpu/drm/simpledrm/simpledrm_drv.c b/drivers/gpu/drm/simpledrm/simpledrm_drv.c
index a4e6566..26956c3 100644
--- a/drivers/gpu/drm/simpledrm/simpledrm_drv.c
+++ b/drivers/gpu/drm/simpledrm/simpledrm_drv.c
@@ -527,12 +527,15 @@ static int __init sdrm_init(void)
}
}
+ sdrm_fbdev_kickout_init();
+
return 0;
}
module_init(sdrm_init);
static void __exit sdrm_exit(void)
{
+ sdrm_fbdev_kickout_exit();
platform_driver_unregister(&sdrm_simplefb_driver);
}
module_exit(sdrm_exit);
diff --git a/drivers/gpu/drm/simpledrm/simpledrm_fbdev.c b/drivers/gpu/drm/simpledrm/simpledrm_fbdev.c
index 4038dd9..daf5943 100644
--- a/drivers/gpu/drm/simpledrm/simpledrm_fbdev.c
+++ b/drivers/gpu/drm/simpledrm/simpledrm_fbdev.c
@@ -28,6 +28,16 @@ static inline struct sdrm_fbdev *to_sdrm_fbdev(struct drm_fb_helper *helper)
return container_of(helper, struct sdrm_fbdev, fb_helper);
}
+/*
+ * Releasing has to be done outside the notifier callchain when we're
+ * kicked out, since do_unregister_framebuffer() calls put_fb_info()
+ * after the notifier has run.
+ */
+static void sdrm_fbdev_fb_destroy(struct fb_info *info)
+{
+ drm_fb_helper_release_fbi(info->par);
+}
+
static struct fb_ops sdrm_fbdev_ops = {
.owner = THIS_MODULE,
.fb_fillrect = drm_fb_helper_cfb_fillrect,
@@ -36,6 +46,7 @@ static struct fb_ops sdrm_fbdev_ops = {
.fb_check_var = drm_fb_helper_check_var,
.fb_set_par = drm_fb_helper_set_par,
.fb_setcmap = drm_fb_helper_setcmap,
+ .fb_destroy = sdrm_fbdev_fb_destroy,
};
static struct drm_framebuffer_funcs sdrm_fb_funcs = {
@@ -85,6 +96,9 @@ static int sdrm_fbdev_create(struct drm_fb_helper *helper,
fbi->fix.smem_len = sdrm->fb_size;
fbi->screen_base = sdrm->fb_map;
+ fbi->apertures->ranges[0].base = sdrm->fb_base;
+ fbi->apertures->ranges[0].size = sdrm->fb_size;
+
return 0;
err_fb_info_destroy:
@@ -154,8 +168,11 @@ void sdrm_fbdev_cleanup(struct sdrm_device *sdrm)
sdrm->fbdev = NULL;
fb_helper = &fbdev->fb_helper;
- drm_fb_helper_unregister_fbi(fb_helper);
- drm_fb_helper_release_fbi(fb_helper);
+ /* it might have been kicked out */
+ if (registered_fb[fbdev->fb_helper.fbdev->node])
+ drm_fb_helper_unregister_fbi(fb_helper);
+
+ /* freeing fb_info is done in fb_ops.fb_destroy() */
drm_framebuffer_unregister_private(&fbdev->fb);
drm_framebuffer_cleanup(&fbdev->fb);
@@ -199,3 +216,37 @@ void sdrm_fbdev_restore_mode(struct sdrm_device *sdrm)
if (fbdev->fb_helper.fbdev->state != FBINFO_STATE_RUNNING)
sdrm_fbdev_set_suspend(fbdev->fb_helper.fbdev, 0);
}
+
+static int sdrm_fbdev_event_notify(struct notifier_block *self,
+ unsigned long action, void *data)
+{
+ struct sdrm_device *sdrm;
+ struct fb_event *event = data;
+ struct fb_info *info = event->info;
+ struct drm_fb_helper *fb_helper = info->par;
+
+ if (action != FB_EVENT_FB_UNREGISTERED || !fb_helper ||
+ !fb_helper->dev || fb_helper->fbdev != info)
+ return NOTIFY_DONE;
+
+ sdrm = fb_helper->dev->dev_private;
+
+ if (sdrm && sdrm->fbdev)
+ platform_device_del(sdrm->ddev->platformdev);
+
+ return NOTIFY_DONE;
+}
+
+static struct notifier_block sdrm_fbdev_event_notifier = {
+ .notifier_call = sdrm_fbdev_event_notify,
+};
+
+void sdrm_fbdev_kickout_init(void)
+{
+ fb_register_client(&sdrm_fbdev_event_notifier);
+}
+
+void sdrm_fbdev_kickout_exit(void)
+{
+ fb_unregister_client(&sdrm_fbdev_event_notifier);
+}
--
2.8.2
[toc] | [prev] | [next] | [standalone]
| From | Daniel Vetter <daniel@ffwll.ch> |
|---|---|
| Date | 2016-08-15 09:20 +0200 |
| Subject | Re: [PATCH v3 3/3] drm: simpledrm: honour remove_conflicting_framebuffers() |
| Message-ID | <s6klI-2uR-15@gated-at.bofh.it> |
| In reply to | #1462173 |
On Sun, Aug 14, 2016 at 06:52:06PM +0200, Noralf Trønnes wrote:
> There is currently no non-fbdev mechanism in place to kick out
> simpledrm when the real hw-driver is probed. As a stop gap until
> that is in place, honour remove_conflicting_framebuffers() and
> delete the simple-framebuffer platform device when it's called.
>
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
Pure coincidence, but I just merge a patch to stub out
remove_conflicting_framebuffers with a drm_fb_helper.c function. I guess
we could lift that one to core (probably needs an extended signature) and
extend it. But this should get the job done for now I think.
-Daniel
> ---
>
> Changes from version 2:
> - Don't forget to free fb_info when kicked out.
>
> drivers/gpu/drm/simpledrm/Kconfig | 5 +++
> drivers/gpu/drm/simpledrm/Makefile | 2 +-
> drivers/gpu/drm/simpledrm/simpledrm.h | 11 +++++-
> drivers/gpu/drm/simpledrm/simpledrm_drv.c | 3 ++
> drivers/gpu/drm/simpledrm/simpledrm_fbdev.c | 55 +++++++++++++++++++++++++++--
> 5 files changed, 72 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/simpledrm/Kconfig b/drivers/gpu/drm/simpledrm/Kconfig
> index 9454536..6205b17 100644
> --- a/drivers/gpu/drm/simpledrm/Kconfig
> +++ b/drivers/gpu/drm/simpledrm/Kconfig
> @@ -16,6 +16,11 @@ config DRM_SIMPLEDRM
> If fbdev support is enabled, this driver will also provide an fbdev
> compatibility layer.
>
> + WARNING
> + fbdev must be enabled for simpledrm to disable itself when a real
> + hw-driver is probed. It relies on remove_conflicting_framebuffers()
> + to be called by the hw-driver.
> +
> If unsure, say Y.
>
> To compile this driver as a module, choose M here: the
> diff --git a/drivers/gpu/drm/simpledrm/Makefile b/drivers/gpu/drm/simpledrm/Makefile
> index 7087245..4b4bcdd 100644
> --- a/drivers/gpu/drm/simpledrm/Makefile
> +++ b/drivers/gpu/drm/simpledrm/Makefile
> @@ -1,5 +1,5 @@
> simpledrm-y := simpledrm_drv.o simpledrm_kms.o simpledrm_gem.o \
> simpledrm_damage.o
> -simpledrm-$(CONFIG_DRM_FBDEV_EMULATION) += simpledrm_fbdev.o
> +simpledrm-$(CONFIG_FB) += simpledrm_fbdev.o
>
> obj-$(CONFIG_DRM_SIMPLEDRM) := simpledrm.o
> diff --git a/drivers/gpu/drm/simpledrm/simpledrm.h b/drivers/gpu/drm/simpledrm/simpledrm.h
> index f01b75d..7bc1292 100644
> --- a/drivers/gpu/drm/simpledrm/simpledrm.h
> +++ b/drivers/gpu/drm/simpledrm/simpledrm.h
> @@ -88,13 +88,15 @@ struct sdrm_framebuffer {
>
> #define to_sdrm_fb(x) container_of(x, struct sdrm_framebuffer, base)
>
> -#ifdef CONFIG_DRM_FBDEV_EMULATION
> +#ifdef CONFIG_FB
>
> void sdrm_fbdev_init(struct sdrm_device *sdrm);
> void sdrm_fbdev_cleanup(struct sdrm_device *sdrm);
> void sdrm_fbdev_display_pipe_update(struct sdrm_device *sdrm,
> struct drm_framebuffer *fb);
> void sdrm_fbdev_restore_mode(struct sdrm_device *sdrm);
> +void sdrm_fbdev_kickout_init(void);
> +void sdrm_fbdev_kickout_exit(void);
>
> #else
>
> @@ -115,6 +117,13 @@ static inline void sdrm_fbdev_restore_mode(struct sdrm_device *sdrm)
> {
> }
>
> +static inline void sdrm_fbdev_kickout_init(void)
> +{
> +}
> +
> +static inline void sdrm_fbdev_kickout_exit(void)
> +{
> +}
> #endif
>
> #endif /* SDRM_DRV_H */
> diff --git a/drivers/gpu/drm/simpledrm/simpledrm_drv.c b/drivers/gpu/drm/simpledrm/simpledrm_drv.c
> index a4e6566..26956c3 100644
> --- a/drivers/gpu/drm/simpledrm/simpledrm_drv.c
> +++ b/drivers/gpu/drm/simpledrm/simpledrm_drv.c
> @@ -527,12 +527,15 @@ static int __init sdrm_init(void)
> }
> }
>
> + sdrm_fbdev_kickout_init();
> +
> return 0;
> }
> module_init(sdrm_init);
>
> static void __exit sdrm_exit(void)
> {
> + sdrm_fbdev_kickout_exit();
> platform_driver_unregister(&sdrm_simplefb_driver);
> }
> module_exit(sdrm_exit);
> diff --git a/drivers/gpu/drm/simpledrm/simpledrm_fbdev.c b/drivers/gpu/drm/simpledrm/simpledrm_fbdev.c
> index 4038dd9..daf5943 100644
> --- a/drivers/gpu/drm/simpledrm/simpledrm_fbdev.c
> +++ b/drivers/gpu/drm/simpledrm/simpledrm_fbdev.c
> @@ -28,6 +28,16 @@ static inline struct sdrm_fbdev *to_sdrm_fbdev(struct drm_fb_helper *helper)
> return container_of(helper, struct sdrm_fbdev, fb_helper);
> }
>
> +/*
> + * Releasing has to be done outside the notifier callchain when we're
> + * kicked out, since do_unregister_framebuffer() calls put_fb_info()
> + * after the notifier has run.
> + */
> +static void sdrm_fbdev_fb_destroy(struct fb_info *info)
> +{
> + drm_fb_helper_release_fbi(info->par);
> +}
> +
> static struct fb_ops sdrm_fbdev_ops = {
> .owner = THIS_MODULE,
> .fb_fillrect = drm_fb_helper_cfb_fillrect,
> @@ -36,6 +46,7 @@ static struct fb_ops sdrm_fbdev_ops = {
> .fb_check_var = drm_fb_helper_check_var,
> .fb_set_par = drm_fb_helper_set_par,
> .fb_setcmap = drm_fb_helper_setcmap,
> + .fb_destroy = sdrm_fbdev_fb_destroy,
> };
>
> static struct drm_framebuffer_funcs sdrm_fb_funcs = {
> @@ -85,6 +96,9 @@ static int sdrm_fbdev_create(struct drm_fb_helper *helper,
> fbi->fix.smem_len = sdrm->fb_size;
> fbi->screen_base = sdrm->fb_map;
>
> + fbi->apertures->ranges[0].base = sdrm->fb_base;
> + fbi->apertures->ranges[0].size = sdrm->fb_size;
> +
> return 0;
>
> err_fb_info_destroy:
> @@ -154,8 +168,11 @@ void sdrm_fbdev_cleanup(struct sdrm_device *sdrm)
> sdrm->fbdev = NULL;
> fb_helper = &fbdev->fb_helper;
>
> - drm_fb_helper_unregister_fbi(fb_helper);
> - drm_fb_helper_release_fbi(fb_helper);
> + /* it might have been kicked out */
> + if (registered_fb[fbdev->fb_helper.fbdev->node])
> + drm_fb_helper_unregister_fbi(fb_helper);
> +
> + /* freeing fb_info is done in fb_ops.fb_destroy() */
>
> drm_framebuffer_unregister_private(&fbdev->fb);
> drm_framebuffer_cleanup(&fbdev->fb);
> @@ -199,3 +216,37 @@ void sdrm_fbdev_restore_mode(struct sdrm_device *sdrm)
> if (fbdev->fb_helper.fbdev->state != FBINFO_STATE_RUNNING)
> sdrm_fbdev_set_suspend(fbdev->fb_helper.fbdev, 0);
> }
> +
> +static int sdrm_fbdev_event_notify(struct notifier_block *self,
> + unsigned long action, void *data)
> +{
> + struct sdrm_device *sdrm;
> + struct fb_event *event = data;
> + struct fb_info *info = event->info;
> + struct drm_fb_helper *fb_helper = info->par;
> +
> + if (action != FB_EVENT_FB_UNREGISTERED || !fb_helper ||
> + !fb_helper->dev || fb_helper->fbdev != info)
> + return NOTIFY_DONE;
> +
> + sdrm = fb_helper->dev->dev_private;
> +
> + if (sdrm && sdrm->fbdev)
> + platform_device_del(sdrm->ddev->platformdev);
> +
> + return NOTIFY_DONE;
> +}
> +
> +static struct notifier_block sdrm_fbdev_event_notifier = {
> + .notifier_call = sdrm_fbdev_event_notify,
> +};
> +
> +void sdrm_fbdev_kickout_init(void)
> +{
> + fb_register_client(&sdrm_fbdev_event_notifier);
> +}
> +
> +void sdrm_fbdev_kickout_exit(void)
> +{
> + fb_unregister_client(&sdrm_fbdev_event_notifier);
> +}
> --
> 2.8.2
>
> _______________________________________________
> 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
[toc] | [prev] | [next] | [standalone]
| From | Daniel Vetter <daniel@ffwll.ch> |
|---|---|
| Date | 2016-08-15 09:20 +0200 |
| Message-ID | <s6klI-2uR-33@gated-at.bofh.it> |
| In reply to | #1462171 |
On Sun, Aug 14, 2016 at 06:52:03PM +0200, Noralf Trønnes wrote:
> This patchset adds the simpledrm driver by David Herrmann based on a
> patchset[1] from 2014. That patchset also included patches for kicking
> out simpledrm by real drivers. I have stayed away from that since it
> involves another subsystem and I would probably be unable to answer any
> questions about the implementation.
>
> Two main changes in this third version:
>
> Paul Gortmaker pointed out that module.h pulls in 750k and is best
> avoided when not necessary. All the source files included it, but only
> one needed it. I the same spirit I moved the includes I could from the
> header file to the respective source files.
>
> The panic handling I'm working on requires an enabled pipeline to work,
> so I have switched the fbdev code to use the drm fb helper. Maybe not
> strictly necessary since fbcon can output panic messages, but it gave
> me an excuse to do it, making the fbdev code "drm standard".
>
> I have tested simpledrm on a Raspberry Pi B+ with U-boot setting up the
> framebuffer and producing this node (legacy, not under /chosen):
>
> / {
> framebuffer@1e887000 {
> compatible = "simple-framebuffer";
> reg = <0x1e887000 0x36c600>;
> format = "r5g6b5";
> width = <1824>;
> height = <984>;
> stride = <3648>;
> status = "okay";
> };
>
> I have only tested with fbcon and modetest (XR24,RG16).
I've read the code, some small comments. I think it'd be good if David
Herrmann can review it too, I think he'll be back from vacation next week.
-Daniel
>
>
> Noralf.
>
>
> Changes from version 2:
> - Remove superfluos module.h includes
> - Move includes from header to source files
> - Set plane.fb before flushing in pipe update, or else the previous one
> gets flushed
> - Added check for vblank event in sdrm_display_pipe_update()
> fbdev:
> - Switch to using drm_fb_helper in preparation for future panic handling
> which needs an enabled pipeline.
> - Don't forget to free fb_info when kicked out.
>
> Changes from version 1:
> - Move platform_set_drvdata() before drm_dev_register()
> - Remove drm_legacy_mmap() call.
> - Set mode_config.{min,max}_{width,height} to the actual dimensions
> of the native framebuffer
> - Remove plane positioning since it won't work with the simple display pipe,
> meaning sdrm_display_pipe_check() isn't necessary either
> - Support the additions to the Device Tree binding document, including
> clocks, regulators and having the node under /chosen
> fbdev:
> - Honour remove_conflicting_framebuffers()
>
> Changes from previous version[2]:
> - Remove FB_SIMPLE=n dependency to avoid kconfig recursive error
> - Changed module name to match kconfig help text: sdrm -> simpledrm
> - Use drm_simple_display_pipe
> - Replace deprecated drm_platform_init()
> - sdrm_dumb_create(): drm_gem_object_unreference() -> *_unlocked()
> - sdrm_dumb_map_offset(): drm_gem_object_lookup() remove drm_device parameter
> - sdrm_drm_mmap() changes:
> Remove struct_mutex locking
> Add drm_vma_offset_{lock,unlock}_lookup()
> drm_mmap() -> drm_legacy_mmap()
> - dma_buf_begin_cpu_access() doesn't require start and length anymore
> - Use drm_cvt_mode() instead of open coding a mode
> - Fix format conversion. In the intermediate step, store the 8/6/5 bit color
> value in the upper part of the 16-bit color variable, not the lower.
> - Support clips == NULL in sdrm_dirty()
> - Set mode_config.preferred_depth
> - Attach mode_config.dirty_info_property to connector
> fbdev:
> - Remove the DRM_SIMPLEDRM_FBDEV kconfig option and use DRM_FBDEV_EMULATION
> - Suspend fbcon/fbdev when the pipeline is enabled, resume in lastclose
> - Add FBINFO_CAN_FORCE_OUTPUT flag so we get oops'es on the console
>
> [1] https://lists.freedesktop.org/archives/dri-devel/2014-January/052584.html
> [2] https://lists.freedesktop.org/archives/dri-devel/2014-January/052594.html
>
>
> Further history:
>
> [PATCH v4 0/6] SimpleDRM Driver
> https://lists.freedesktop.org/archives/dri-devel/2013-September/044638.html
>
> [PATCH v2 00/14] Platform Framebuffers and SimpleDRM
> https://lists.freedesktop.org/archives/dri-devel/2013-July/041090.html
>
> [RFC 0/6] SimpleDRM Driver (was: dvbe driver)
> https://lists.freedesktop.org/archives/dri-devel/2013-June/040386.html
>
> [PATCH 0/9] System Framebuffer Bus (sysfb)
> https://lists.freedesktop.org/archives/dri-devel/2013-February/035013.html
>
>
> Noralf Trønnes (3):
> drm: add SimpleDRM driver
> drm: simpledrm: add fbdev fallback support
> drm: simpledrm: honour remove_conflicting_framebuffers()
>
> drivers/gpu/drm/Kconfig | 2 +
> drivers/gpu/drm/Makefile | 1 +
> drivers/gpu/drm/simpledrm/Kconfig | 27 ++
> drivers/gpu/drm/simpledrm/Makefile | 5 +
> drivers/gpu/drm/simpledrm/simpledrm.h | 129 +++++++
> drivers/gpu/drm/simpledrm/simpledrm_damage.c | 302 +++++++++++++++
> drivers/gpu/drm/simpledrm/simpledrm_drv.c | 546 +++++++++++++++++++++++++++
> drivers/gpu/drm/simpledrm/simpledrm_fbdev.c | 252 +++++++++++++
> drivers/gpu/drm/simpledrm/simpledrm_gem.c | 274 ++++++++++++++
> drivers/gpu/drm/simpledrm/simpledrm_kms.c | 266 +++++++++++++
> 10 files changed, 1804 insertions(+)
> create mode 100644 drivers/gpu/drm/simpledrm/Kconfig
> create mode 100644 drivers/gpu/drm/simpledrm/Makefile
> create mode 100644 drivers/gpu/drm/simpledrm/simpledrm.h
> create mode 100644 drivers/gpu/drm/simpledrm/simpledrm_damage.c
> create mode 100644 drivers/gpu/drm/simpledrm/simpledrm_drv.c
> create mode 100644 drivers/gpu/drm/simpledrm/simpledrm_fbdev.c
> create mode 100644 drivers/gpu/drm/simpledrm/simpledrm_gem.c
> create mode 100644 drivers/gpu/drm/simpledrm/simpledrm_kms.c
>
> --
> 2.8.2
>
> _______________________________________________
> 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
[toc] | [prev] | [next] | [standalone]
| From | Noralf Trønnes <noralf@tronnes.org> |
|---|---|
| Date | 2016-08-16 15:00 +0200 |
| Subject | Re: [PATCH v3 1/3] drm: add SimpleDRM driver |
| Message-ID | <s6M8h-3cC-9@gated-at.bofh.it> |
| In reply to | #1462171 |
Den 15.08.2016 08:59, skrev Daniel Vetter:
> On Sun, Aug 14, 2016 at 06:52:04PM +0200, Noralf Trønnes wrote:
>> The SimpleDRM driver binds to simple-framebuffer devices and provides a
>> DRM/KMS API. It provides only a single CRTC+encoder+connector combination
>> plus one initial mode.
>>
>> Userspace can create dumb-buffers which can be blit into the real
>> framebuffer similar to UDL. No access to the real framebuffer is allowed
>> (compared to earlier version of this driver) to avoid security issues.
>> Furthermore, this way we can support arbitrary modes as long as we have a
>> conversion-helper.
>>
>> The driver was originally written by David Herrmann in 2014.
>> My main contribution is to make use of drm_simple_kms_helper and
>> rework the probe path to avoid use of the deprecated drm_platform_init()
>> and drm_driver.{load,unload}().
>> Additions have also been made for later changes to the Device Tree binding
>> document, like support for clocks, regulators and having the node under
>> /chosen. This code was lifted verbatim from simplefb.c.
>>
>> Cc: dh.herrmann@gmail.com
>> Cc: libv@skynet.be
>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
<snip>
>> diff --git a/drivers/gpu/drm/simpledrm/simpledrm_gem.c b/drivers/gpu/drm/simpledrm/simpledrm_gem.c
>> new file mode 100644
>> index 0000000..81bd7c5
>> --- /dev/null
>> +++ b/drivers/gpu/drm/simpledrm/simpledrm_gem.c
<snip>
>> +struct sdrm_gem_object *sdrm_gem_alloc_object(struct drm_device *ddev,
>> + size_t size)
>> +{
>> + struct sdrm_gem_object *obj;
>> +
>> + WARN_ON(!size || (size & ~PAGE_MASK) != 0);
>> +
>> + obj = kzalloc(sizeof(*obj), GFP_KERNEL);
>> + if (!obj)
>> + return NULL;
>> +
>> + drm_gem_private_object_init(ddev, &obj->base, size);
>> + return obj;
>> +}
>> +
>> +void sdrm_gem_free_object(struct drm_gem_object *gobj)
>> +{
>> + struct sdrm_gem_object *obj = to_sdrm_bo(gobj);
>> + struct drm_device *ddev = gobj->dev;
>> +
>> + if (obj->pages) {
>> + /* kill all user-space mappings */
>> + drm_vma_node_unmap(&gobj->vma_node,
>> + ddev->anon_inode->i_mapping);
>> + sdrm_gem_put_pages(obj);
>> + }
>> +
>> + if (gobj->import_attach)
>> + drm_prime_gem_destroy(gobj, obj->sg);
>> +
>> + drm_gem_free_mmap_offset(gobj);
>> + drm_gem_object_release(gobj);
>> + kfree(obj);
>> +}
>> +
>> +int sdrm_dumb_create(struct drm_file *dfile, struct drm_device *ddev,
>> + struct drm_mode_create_dumb *args)
>> +{
>> + struct sdrm_gem_object *obj;
>> + int r;
>> +
>> + if (args->flags)
>> + return -EINVAL;
>> +
>> + /* overflow checks are done by DRM core */
>> + args->pitch = (args->bpp + 7) / 8 * args->width;
>> + args->size = PAGE_ALIGN(args->pitch * args->height);
>> +
>> + obj = sdrm_gem_alloc_object(ddev, args->size);
>> + if (!obj)
>> + return -ENOMEM;
>> +
>> + r = drm_gem_handle_create(dfile, &obj->base, &args->handle);
>> + if (r) {
>> + drm_gem_object_unreference_unlocked(&obj->base);
>> + return r;
>> + }
>> +
>> + /* handle owns a reference */
>> + drm_gem_object_unreference_unlocked(&obj->base);
>> + return 0;
>> +}
>> +
>> +int sdrm_dumb_destroy(struct drm_file *dfile, struct drm_device *ddev,
>> + uint32_t handle)
>> +{
>> + return drm_gem_handle_delete(dfile, handle);
>> +}
> I wonder whether some dumb+gem helpers wouldn't be useful to roll out. At
> least drm_dumb_gem_destroy.c would be pretty simple.
There's already a drm_gem_dumb_destroy() in drm_gem.c
The drm_driver->gem_create_object callback makes it possible to do a
generic dumb create:
int drm_gem_dumb_create(struct drm_file *file, struct drm_device *dev,
struct drm_mode_create_dumb *args)
{
struct drm_gem_object *obj;
int ret;
if (!dev->driver->gem_create_object)
return -EINVAL;
args->pitch = DIV_ROUND_UP(args->width * args->bpp, 8);
args->size = args->pitch * args->height;
obj = dev->driver->gem_create_object(dev, args->size);
if (!obj)
return -ENOMEM;
ret = drm_gem_handle_create(file, obj, &args->handle);
drm_gem_object_unreference_unlocked(obj);
return ret;
}
EXPORT_SYMBOL(drm_gem_dumb_create);
struct drm_gem_object *sdrm_gem_create_object(struct drm_device *ddev,
size_t size)
{
struct sdrm_gem_object *sobj;
sobj = sdrm_gem_alloc_object(ddev, size);
if (!sobj)
return ERR_PTR(-ENOMEM);
return &sobj->base;
}
>> +
>> +int sdrm_dumb_map_offset(struct drm_file *dfile, struct drm_device *ddev,
>> + uint32_t handle, uint64_t *offset)
>> +{
>> + struct drm_gem_object *gobj;
>> + int r;
>> +
>> + mutex_lock(&ddev->struct_mutex);
> There's still some struct mutex here.
>
>> +
>> + gobj = drm_gem_object_lookup(dfile, handle);
>> + if (!gobj) {
>> + r = -ENOENT;
>> + goto out_unlock;
>> + }
>> +
>> + r = drm_gem_create_mmap_offset(gobj);
>> + if (r)
>> + goto out_unref;
>> +
>> + *offset = drm_vma_node_offset_addr(&gobj->vma_node);
>> +
>> +out_unref:
>> + drm_gem_object_unreference(gobj);
>> +out_unlock:
>> + mutex_unlock(&ddev->struct_mutex);
>> + return r;
>> +}
>> +
Maybe this addition to drm_gem.c as well:
int drm_gem_dumb_map_offset(struct drm_file *file, struct drm_device *dev,
uint32_t handle, uint64_t *offset)
{
struct drm_gem_object *obj;
int ret;
obj = drm_gem_object_lookup(file, handle);
if (!obj)
return -ENOENT;
ret = drm_gem_create_mmap_offset(obj);
if (ret)
goto out_unref;
*offset = drm_vma_node_offset_addr(&obj->vma_node);
out_unref:
drm_gem_object_unreference_unlocked(obj);
return ret;
}
>> +int sdrm_drm_mmap(struct file *filp, struct vm_area_struct *vma)
>> +{
>> + struct drm_file *priv = filp->private_data;
>> + struct drm_device *dev = priv->minor->dev;
>> + struct drm_vma_offset_node *node;
>> + struct drm_gem_object *gobj;
>> + struct sdrm_gem_object *obj;
>> + size_t size, i, num;
>> + int r;
>> +
>> + if (drm_device_is_unplugged(dev))
>> + return -ENODEV;
>> +
>> + drm_vma_offset_lock_lookup(dev->vma_offset_manager);
>> + node = drm_vma_offset_exact_lookup_locked(dev->vma_offset_manager,
>> + vma->vm_pgoff,
>> + vma_pages(vma));
>> + drm_vma_offset_unlock_lookup(dev->vma_offset_manager);
>> +
>> + if (!drm_vma_node_is_allowed(node, filp))
>> + return -EACCES;
>> +
>> + gobj = container_of(node, struct drm_gem_object, vma_node);
>> + obj = to_sdrm_bo(gobj);
>> + size = drm_vma_node_size(node) << PAGE_SHIFT;
>> + if (size < vma->vm_end - vma->vm_start)
>> + return r;
>> +
>> + r = sdrm_gem_get_pages(obj);
>> + if (r < 0)
>> + return r;
>> +
>> + /* prevent dmabuf-imported mmap to user-space */
>> + if (!obj->pages)
>> + return -EACCES;
>> +
>> + vma->vm_flags |= VM_DONTEXPAND;
>> + vma->vm_page_prot = pgprot_writecombine(vm_get_page_prot(vma->vm_flags));
>> +
>> + num = (vma->vm_end - vma->vm_start) >> PAGE_SHIFT;
>> + for (i = 0; i < num; ++i) {
>> + r = vm_insert_page(vma, vma->vm_start + i * PAGE_SIZE,
>> + obj->pages[i]);
>> + if (r < 0) {
>> + if (i > 0)
>> + zap_vma_ptes(vma, vma->vm_start, i * PAGE_SIZE);
>> + return r;
>> + }
>> + }
>> +
>> + return 0;
>> +}
> Why can't we just redirect to the underlying shmem mmap here (after
> argument checking)?
I don't know what that means, but looking at vc4 it seems I can use
drm_gem_mmap() to remove some boilerplate.
int sdrm_drm_mmap(struct file *filp, struct vm_area_struct *vma)
{
unsigned long vm_flags = vma->vm_flags;
struct sdrm_gem_object *sobj;
struct drm_gem_object *obj;
size_t i, num;
int r;
r = drm_gem_mmap(filp, vma);
if (r)
return r;
obj = vma->vm_private_data;
sobj = to_sdrm_bo(obj);
r = sdrm_gem_get_pages(obj);
if (r < 0)
return r;
/* prevent dmabuf-imported mmap to user-space */
if (!obj->pages)
return -EACCES;
/* drm_gem_mmap() sets flags that we don't want */
vma->vm_flags = vm_flags | VM_DONTEXPAND;
vma->vm_page_prot =
pgprot_writecombine(vm_get_page_prot(vma->vm_flags));
num = (vma->vm_end - vma->vm_start) >> PAGE_SHIFT;
for (i = 0; i < num; ++i) {
r = vm_insert_page(vma, vma->vm_start + i * PAGE_SIZE,
obj->pages[i]);
if (r < 0) {
if (i > 0)
zap_vma_ptes(vma, vma->vm_start, i * PAGE_SIZE);
return r;
}
}
return 0;
}
drm_gem_mmap() requires drm_driver->gem_vm_ops to be set:
const struct vm_operations_struct sdrm_gem_vm_ops = {
.open = drm_gem_vm_open,
.close = drm_gem_vm_close,
};
And browsing the code a bit more shows that tegra, udl, etnaviv and vgem
does the vm_insert_page() thing in the vm_operations_struct->fault()
handler.
So maybe this makes sense for simpledrm as well:
static int sdrm_gem_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
{
struct drm_gem_object *obj = vma->vm_private_data;
struct sdrm_gem_object *sobj = to_sdrm_bo(obj);
loff_t num_pages;
pgoff_t offset;
int r;
if (!sobj->pages)
return VM_FAULT_SIGBUS;
offset = ((unsigned long)vmf->virtual_address - vma->vm_start) >>
PAGE_SHIFT;
num_pages = DIV_ROUND_UP(obj->base.size, PAGE_SIZE);
if (page_offset > num_pages)
return VM_FAULT_SIGBUS;
r = vm_insert_page(vma, (unsigned long)vmf->virtual_address,
sobj->pages[offset]);
switch (r) {
case -EAGAIN:
case 0:
case -ERESTARTSYS:
case -EINTR:
case -EBUSY:
return VM_FAULT_NOPAGE;
case -ENOMEM:
return VM_FAULT_OOM;
}
return VM_FAULT_SIGBUS;
}
Noralf.
>> +
>> +struct drm_gem_object *sdrm_gem_prime_import(struct drm_device *ddev,
>> + struct dma_buf *dma_buf)
>> +{
>> + struct dma_buf_attachment *attach;
>> + struct sdrm_gem_object *obj;
>> + struct sg_table *sg;
>> + int ret;
>> +
>> + /* need to attach */
>> + attach = dma_buf_attach(dma_buf, ddev->dev);
>> + if (IS_ERR(attach))
>> + return ERR_CAST(attach);
>> +
>> + get_dma_buf(dma_buf);
>> +
>> + sg = dma_buf_map_attachment(attach, DMA_BIDIRECTIONAL);
>> + if (IS_ERR(sg)) {
>> + ret = PTR_ERR(sg);
>> + goto fail_detach;
>> + }
>> +
>> + /*
>> + * dma_buf_vmap() gives us a page-aligned mapping, so lets bump the
>> + * size of the dma-buf to the next page-boundary
>> + */
> dma_buf should be page-aligned already. Not sure we enforce that, but it's
> in practice a given.
>
>> + obj = sdrm_gem_alloc_object(ddev, PAGE_ALIGN(dma_buf->size));
>> + if (!obj) {
>> + ret = -ENOMEM;
>> + goto fail_unmap;
>> + }
>> +
>> + obj->sg = sg;
>> + obj->base.import_attach = attach;
>> +
>> + return &obj->base;
>> +
>> +fail_unmap:
>> + dma_buf_unmap_attachment(attach, sg, DMA_BIDIRECTIONAL);
>> +fail_detach:
>> + dma_buf_detach(dma_buf, attach);
>> + dma_buf_put(dma_buf);
>> + return ERR_PTR(ret);
>> +}
>> diff --git a/drivers/gpu/drm/simpledrm/simpledrm_kms.c b/drivers/gpu/drm/simpledrm/simpledrm_kms.c
>> new file mode 100644
>> index 0000000..00e50d9
>> --- /dev/null
>> +++ b/drivers/gpu/drm/simpledrm/simpledrm_kms.c
>> @@ -0,0 +1,258 @@
>> +/*
>> + * SimpleDRM firmware framebuffer driver
>> + * Copyright (c) 2012-2014 David Herrmann <dh.herrmann@gmail.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms of the GNU General Public License as published by the Free
>> + * Software Foundation; either version 2 of the License, or (at your option)
>> + * any later version.
>> + */
>> +
>> +#include <drm/drmP.h>
>> +#include <drm/drm_atomic_helper.h>
>> +#include <drm/drm_crtc.h>
>> +#include <drm/drm_crtc_helper.h>
>> +#include <drm/drm_gem.h>
>> +#include <drm/drm_simple_kms_helper.h>
>> +#include <linux/slab.h>
>> +
>> +#include "simpledrm.h"
>> +
>> +static const uint32_t sdrm_formats[] = {
>> + DRM_FORMAT_RGB565,
>> + DRM_FORMAT_ARGB8888,
>> + DRM_FORMAT_XRGB8888,
>> +};
>> +
>> +static int sdrm_conn_get_modes(struct drm_connector *conn)
>> +{
>> + struct sdrm_device *sdrm = conn->dev->dev_private;
>> + struct drm_display_mode *mode;
>> +
>> + mode = drm_cvt_mode(sdrm->ddev, sdrm->fb_width, sdrm->fb_height,
>> + 60, false, false, false);
>> + if (!mode)
>> + return 0;
>> +
>> + mode->type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED;
>> + drm_mode_set_name(mode);
>> + drm_mode_probed_add(conn, mode);
>> +
>> + return 1;
>> +}
>> +
>> +static const struct drm_connector_helper_funcs sdrm_conn_hfuncs = {
>> + .get_modes = sdrm_conn_get_modes,
>> + .best_encoder = drm_atomic_helper_best_encoder,
>> +};
>> +
>> +static enum drm_connector_status sdrm_conn_detect(struct drm_connector *conn,
>> + bool force)
>> +{
>> + /*
>> + * We simulate an always connected monitor. simple-fb doesn't
>> + * provide any way to detect whether the connector is active. Hence,
>> + * signal DRM core that it is always connected.
>> + */
>> +
>> + return connector_status_connected;
>> +}
>> +
>> +static const struct drm_connector_funcs sdrm_conn_ops = {
>> + .dpms = drm_atomic_helper_connector_dpms,
>> + .reset = drm_atomic_helper_connector_reset,
>> + .detect = sdrm_conn_detect,
>> + .fill_modes = drm_helper_probe_single_connector_modes,
>> + .destroy = drm_connector_cleanup,
>> + .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
>> + .atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
>> +};
>> +
>> +static inline struct sdrm_device *
>> +pipe_to_sdrm(struct drm_simple_display_pipe *pipe)
>> +{
>> + return container_of(pipe, struct sdrm_device, pipe);
>> +}
>> +
>> +static void sdrm_crtc_send_vblank_event(struct drm_crtc *crtc)
>> +{
>> + if (crtc->state && crtc->state->event) {
>> + spin_lock_irq(&crtc->dev->event_lock);
>> + drm_crtc_send_vblank_event(crtc, crtc->state->event);
>> + spin_unlock_irq(&crtc->dev->event_lock);
>> + crtc->state->event = NULL;
>> + }
>> +}
>> +
>> +void sdrm_display_pipe_update(struct drm_simple_display_pipe *pipe,
>> + struct drm_plane_state *plane_state)
>> +{
>> + struct drm_framebuffer *fb = pipe->plane.state->fb;
>> + struct sdrm_device *sdrm = pipe_to_sdrm(pipe);
>> +
>> + sdrm_crtc_send_vblank_event(&pipe->crtc);
>> +
>> + if (fb) {
>> + pipe->plane.fb = fb;
>> + sdrm_dirty_all_locked(sdrm);
>> + }
>> +}
>> +
>> +static void sdrm_display_pipe_enable(struct drm_simple_display_pipe *pipe,
>> + struct drm_crtc_state *crtc_state)
>> +{
>> + sdrm_crtc_send_vblank_event(&pipe->crtc);
>> +}
>> +
>> +static void sdrm_display_pipe_disable(struct drm_simple_display_pipe *pipe)
>> +{
>> + sdrm_crtc_send_vblank_event(&pipe->crtc);
>> +}
>> +
>> +static const struct drm_simple_display_pipe_funcs sdrm_pipe_funcs = {
>> + .update = sdrm_display_pipe_update,
>> + .enable = sdrm_display_pipe_enable,
>> + .disable = sdrm_display_pipe_disable,
>> +};
>> +
>> +static int sdrm_fb_create_handle(struct drm_framebuffer *fb,
>> + struct drm_file *dfile,
>> + unsigned int *handle)
>> +{
>> + struct sdrm_framebuffer *sfb = to_sdrm_fb(fb);
>> +
>> + return drm_gem_handle_create(dfile, &sfb->obj->base, handle);
>> +}
>> +
>> +static void sdrm_fb_destroy(struct drm_framebuffer *fb)
>> +{
>> + struct sdrm_framebuffer *sfb = to_sdrm_fb(fb);
>> +
>> + drm_framebuffer_cleanup(fb);
>> + drm_gem_object_unreference_unlocked(&sfb->obj->base);
>> + kfree(sfb);
>> +}
>> +
>> +static const struct drm_framebuffer_funcs sdrm_fb_ops = {
>> + .create_handle = sdrm_fb_create_handle,
>> + .dirty = sdrm_dirty,
>> + .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_framebuffer *fb;
>> + struct drm_gem_object *gobj;
>> + u32 bpp, size;
>> + int ret;
>> + void *err;
>> +
>> + if (cmd->flags)
>> + return ERR_PTR(-EINVAL);
>> +
>> + gobj = drm_gem_object_lookup(dfile, cmd->handles[0]);
>> + if (!gobj)
>> + return ERR_PTR(-EINVAL);
>> +
>> + fb = kzalloc(sizeof(*fb), GFP_KERNEL);
>> + if (!fb) {
>> + err = ERR_PTR(-ENOMEM);
>> + goto err_unref;
>> + }
>> + fb->obj = to_sdrm_bo(gobj);
>> +
>> + fb->base.pitches[0] = cmd->pitches[0];
>> + fb->base.offsets[0] = cmd->offsets[0];
>> + fb->base.width = cmd->width;
>> + fb->base.height = cmd->height;
>> + fb->base.pixel_format = cmd->pixel_format;
>> + drm_fb_get_bpp_depth(cmd->pixel_format, &fb->base.depth,
>> + &fb->base.bits_per_pixel);
>> +
>> + /*
>> + * width/height are already clamped into min/max_width/height range,
>> + * so overflows are not possible
>> + */
>> +
>> + bpp = (fb->base.bits_per_pixel + 7) / 8;
>> + size = cmd->pitches[0] * cmd->height;
>> + if (!bpp ||
>> + bpp > 4 ||
>> + cmd->pitches[0] < bpp * fb->base.width ||
>> + cmd->pitches[0] > 0xffffU ||
>> + size + fb->base.offsets[0] < size ||
>> + size + fb->base.offsets[0] > fb->obj->base.size) {
>> + err = ERR_PTR(-EINVAL);
>> + goto err_free;
>> + }
>> +
>> + ret = drm_framebuffer_init(ddev, &fb->base, &sdrm_fb_ops);
>> + if (ret < 0) {
>> + err = ERR_PTR(ret);
>> + goto err_free;
>> + }
>> +
>> + DRM_DEBUG_KMS("[FB:%d] pixel_format: %s\n", fb->base.base.id,
>> + drm_get_format_name(fb->base.pixel_format));
>> +
>> + return &fb->base;
>> +
>> +err_free:
>> + kfree(fb);
>> +err_unref:
>> + drm_gem_object_unreference_unlocked(gobj);
>> +
>> + return err;
>> +}
>> +
>> +static const struct drm_mode_config_funcs sdrm_mode_config_ops = {
>> + .fb_create = sdrm_fb_create,
>> + .atomic_check = drm_atomic_helper_check,
>> + .atomic_commit = drm_atomic_helper_commit,
>> +};
>> +
>> +int sdrm_drm_modeset_init(struct sdrm_device *sdrm)
>> +{
>> + struct drm_connector *conn = &sdrm->conn;
>> + struct drm_device *ddev = sdrm->ddev;
>> + int ret;
>> +
>> + drm_mode_config_init(ddev);
>> + ddev->mode_config.min_width = sdrm->fb_width;
>> + ddev->mode_config.max_width = sdrm->fb_width;
>> + ddev->mode_config.min_height = sdrm->fb_height;
>> + ddev->mode_config.max_height = sdrm->fb_height;
>> + ddev->mode_config.preferred_depth = sdrm->fb_bpp;
>> + ddev->mode_config.funcs = &sdrm_mode_config_ops;
>> +
>> + drm_connector_helper_add(conn, &sdrm_conn_hfuncs);
>> + ret = drm_connector_init(ddev, conn, &sdrm_conn_ops,
>> + DRM_MODE_CONNECTOR_VIRTUAL);
>> + if (ret)
>> + goto err_cleanup;
>> +
>> + ret = drm_mode_create_dirty_info_property(ddev);
>> + if (ret)
>> + goto err_cleanup;
>> +
>> + drm_object_attach_property(&conn->base,
>> + ddev->mode_config.dirty_info_property,
>> + DRM_MODE_DIRTY_ON);
> dirty_info_property is gone.
>
>> +
>> + ret = drm_simple_display_pipe_init(ddev, &sdrm->pipe, &sdrm_pipe_funcs,
>> + sdrm_formats,
>> + ARRAY_SIZE(sdrm_formats), conn);
>> + if (ret)
>> + goto err_cleanup;
>> +
>> + drm_mode_config_reset(ddev);
>> +
>> + return 0;
>> +
>> +err_cleanup:
>> + drm_mode_config_cleanup(ddev);
>> +
>> + return ret;
>> +}
>> --
>> 2.8.2
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
[toc] | [prev] | [next] | [standalone]
| From | Daniel Vetter <daniel@ffwll.ch> |
|---|---|
| Date | 2016-08-16 17:30 +0200 |
| Subject | Re: [PATCH v3 1/3] drm: add SimpleDRM driver |
| Message-ID | <s6Ots-4M4-7@gated-at.bofh.it> |
| In reply to | #1463763 |
On Tue, Aug 16, 2016 at 02:58:38PM +0200, Noralf Trønnes wrote:
>
> Den 15.08.2016 08:59, skrev Daniel Vetter:
> > On Sun, Aug 14, 2016 at 06:52:04PM +0200, Noralf Trønnes wrote:
> > > The SimpleDRM driver binds to simple-framebuffer devices and provides a
> > > DRM/KMS API. It provides only a single CRTC+encoder+connector combination
> > > plus one initial mode.
> > >
> > > Userspace can create dumb-buffers which can be blit into the real
> > > framebuffer similar to UDL. No access to the real framebuffer is allowed
> > > (compared to earlier version of this driver) to avoid security issues.
> > > Furthermore, this way we can support arbitrary modes as long as we have a
> > > conversion-helper.
> > >
> > > The driver was originally written by David Herrmann in 2014.
> > > My main contribution is to make use of drm_simple_kms_helper and
> > > rework the probe path to avoid use of the deprecated drm_platform_init()
> > > and drm_driver.{load,unload}().
> > > Additions have also been made for later changes to the Device Tree binding
> > > document, like support for clocks, regulators and having the node under
> > > /chosen. This code was lifted verbatim from simplefb.c.
> > >
> > > Cc: dh.herrmann@gmail.com
> > > Cc: libv@skynet.be
> > > Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
>
> <snip>
>
> > > diff --git a/drivers/gpu/drm/simpledrm/simpledrm_gem.c b/drivers/gpu/drm/simpledrm/simpledrm_gem.c
> > > new file mode 100644
> > > index 0000000..81bd7c5
> > > --- /dev/null
> > > +++ b/drivers/gpu/drm/simpledrm/simpledrm_gem.c
>
> <snip>
>
> > > +struct sdrm_gem_object *sdrm_gem_alloc_object(struct drm_device *ddev,
> > > + size_t size)
> > > +{
> > > + struct sdrm_gem_object *obj;
> > > +
> > > + WARN_ON(!size || (size & ~PAGE_MASK) != 0);
> > > +
> > > + obj = kzalloc(sizeof(*obj), GFP_KERNEL);
> > > + if (!obj)
> > > + return NULL;
> > > +
> > > + drm_gem_private_object_init(ddev, &obj->base, size);
> > > + return obj;
> > > +}
> > > +
> > > +void sdrm_gem_free_object(struct drm_gem_object *gobj)
> > > +{
> > > + struct sdrm_gem_object *obj = to_sdrm_bo(gobj);
> > > + struct drm_device *ddev = gobj->dev;
> > > +
> > > + if (obj->pages) {
> > > + /* kill all user-space mappings */
> > > + drm_vma_node_unmap(&gobj->vma_node,
> > > + ddev->anon_inode->i_mapping);
> > > + sdrm_gem_put_pages(obj);
> > > + }
> > > +
> > > + if (gobj->import_attach)
> > > + drm_prime_gem_destroy(gobj, obj->sg);
> > > +
> > > + drm_gem_free_mmap_offset(gobj);
> > > + drm_gem_object_release(gobj);
> > > + kfree(obj);
> > > +}
> > > +
> > > +int sdrm_dumb_create(struct drm_file *dfile, struct drm_device *ddev,
> > > + struct drm_mode_create_dumb *args)
> > > +{
> > > + struct sdrm_gem_object *obj;
> > > + int r;
> > > +
> > > + if (args->flags)
> > > + return -EINVAL;
> > > +
> > > + /* overflow checks are done by DRM core */
> > > + args->pitch = (args->bpp + 7) / 8 * args->width;
> > > + args->size = PAGE_ALIGN(args->pitch * args->height);
> > > +
> > > + obj = sdrm_gem_alloc_object(ddev, args->size);
> > > + if (!obj)
> > > + return -ENOMEM;
> > > +
> > > + r = drm_gem_handle_create(dfile, &obj->base, &args->handle);
> > > + if (r) {
> > > + drm_gem_object_unreference_unlocked(&obj->base);
> > > + return r;
> > > + }
> > > +
> > > + /* handle owns a reference */
> > > + drm_gem_object_unreference_unlocked(&obj->base);
> > > + return 0;
> > > +}
> > > +
> > > +int sdrm_dumb_destroy(struct drm_file *dfile, struct drm_device *ddev,
> > > + uint32_t handle)
> > > +{
> > > + return drm_gem_handle_delete(dfile, handle);
> > > +}
> > I wonder whether some dumb+gem helpers wouldn't be useful to roll out. At
> > least drm_dumb_gem_destroy.c would be pretty simple.
>
> There's already a drm_gem_dumb_destroy() in drm_gem.c
>
> The drm_driver->gem_create_object callback makes it possible to do a
> generic dumb create:
>
> int drm_gem_dumb_create(struct drm_file *file, struct drm_device *dev,
> struct drm_mode_create_dumb *args)
> {
> struct drm_gem_object *obj;
> int ret;
>
> if (!dev->driver->gem_create_object)
> return -EINVAL;
>
> args->pitch = DIV_ROUND_UP(args->width * args->bpp, 8);
> args->size = args->pitch * args->height;
>
> obj = dev->driver->gem_create_object(dev, args->size);
> if (!obj)
> return -ENOMEM;
>
> ret = drm_gem_handle_create(file, obj, &args->handle);
> drm_gem_object_unreference_unlocked(obj);
>
> return ret;
> }
> EXPORT_SYMBOL(drm_gem_dumb_create);
>
> struct drm_gem_object *sdrm_gem_create_object(struct drm_device *ddev,
> size_t size)
> {
> struct sdrm_gem_object *sobj;
>
> sobj = sdrm_gem_alloc_object(ddev, size);
> if (!sobj)
> return ERR_PTR(-ENOMEM);
>
> return &sobj->base;
> }
>
> > > +
> > > +int sdrm_dumb_map_offset(struct drm_file *dfile, struct drm_device *ddev,
> > > + uint32_t handle, uint64_t *offset)
> > > +{
> > > + struct drm_gem_object *gobj;
> > > + int r;
> > > +
> > > + mutex_lock(&ddev->struct_mutex);
> > There's still some struct mutex here.
> >
> > > +
> > > + gobj = drm_gem_object_lookup(dfile, handle);
> > > + if (!gobj) {
> > > + r = -ENOENT;
> > > + goto out_unlock;
> > > + }
> > > +
> > > + r = drm_gem_create_mmap_offset(gobj);
> > > + if (r)
> > > + goto out_unref;
> > > +
> > > + *offset = drm_vma_node_offset_addr(&gobj->vma_node);
> > > +
> > > +out_unref:
> > > + drm_gem_object_unreference(gobj);
> > > +out_unlock:
> > > + mutex_unlock(&ddev->struct_mutex);
> > > + return r;
> > > +}
> > > +
>
> Maybe this addition to drm_gem.c as well:
>
> int drm_gem_dumb_map_offset(struct drm_file *file, struct drm_device *dev,
> uint32_t handle, uint64_t *offset)
> {
> struct drm_gem_object *obj;
> int ret;
>
> obj = drm_gem_object_lookup(file, handle);
> if (!obj)
> return -ENOENT;
>
> ret = drm_gem_create_mmap_offset(obj);
> if (ret)
> goto out_unref;
>
> *offset = drm_vma_node_offset_addr(&obj->vma_node);
>
> out_unref:
> drm_gem_object_unreference_unlocked(obj);
>
> return ret;
> }
Yeah, sounds good for adding the above two. Feel free to roll them out to
drivers (or not).
> > > +int sdrm_drm_mmap(struct file *filp, struct vm_area_struct *vma)
> > > +{
> > > + struct drm_file *priv = filp->private_data;
> > > + struct drm_device *dev = priv->minor->dev;
> > > + struct drm_vma_offset_node *node;
> > > + struct drm_gem_object *gobj;
> > > + struct sdrm_gem_object *obj;
> > > + size_t size, i, num;
> > > + int r;
> > > +
> > > + if (drm_device_is_unplugged(dev))
> > > + return -ENODEV;
> > > +
> > > + drm_vma_offset_lock_lookup(dev->vma_offset_manager);
> > > + node = drm_vma_offset_exact_lookup_locked(dev->vma_offset_manager,
> > > + vma->vm_pgoff,
> > > + vma_pages(vma));
> > > + drm_vma_offset_unlock_lookup(dev->vma_offset_manager);
> > > +
> > > + if (!drm_vma_node_is_allowed(node, filp))
> > > + return -EACCES;
> > > +
> > > + gobj = container_of(node, struct drm_gem_object, vma_node);
> > > + obj = to_sdrm_bo(gobj);
> > > + size = drm_vma_node_size(node) << PAGE_SHIFT;
> > > + if (size < vma->vm_end - vma->vm_start)
> > > + return r;
> > > +
> > > + r = sdrm_gem_get_pages(obj);
> > > + if (r < 0)
> > > + return r;
> > > +
> > > + /* prevent dmabuf-imported mmap to user-space */
> > > + if (!obj->pages)
> > > + return -EACCES;
> > > +
> > > + vma->vm_flags |= VM_DONTEXPAND;
> > > + vma->vm_page_prot = pgprot_writecombine(vm_get_page_prot(vma->vm_flags));
> > > +
> > > + num = (vma->vm_end - vma->vm_start) >> PAGE_SHIFT;
> > > + for (i = 0; i < num; ++i) {
> > > + r = vm_insert_page(vma, vma->vm_start + i * PAGE_SIZE,
> > > + obj->pages[i]);
> > > + if (r < 0) {
> > > + if (i > 0)
> > > + zap_vma_ptes(vma, vma->vm_start, i * PAGE_SIZE);
> > > + return r;
> > > + }
> > > + }
> > > +
> > > + return 0;
> > > +}
> > Why can't we just redirect to the underlying shmem mmap here (after
> > argument checking)?
>
> I don't know what that means, but looking at vc4 it seems I can use
> drm_gem_mmap() to remove some boilerplate.
>
> int sdrm_drm_mmap(struct file *filp, struct vm_area_struct *vma)
> {
> unsigned long vm_flags = vma->vm_flags;
> struct sdrm_gem_object *sobj;
> struct drm_gem_object *obj;
> size_t i, num;
> int r;
>
> r = drm_gem_mmap(filp, vma);
> if (r)
> return r;
>
> obj = vma->vm_private_data;
>
> sobj = to_sdrm_bo(obj);
>
> r = sdrm_gem_get_pages(obj);
> if (r < 0)
> return r;
>
> /* prevent dmabuf-imported mmap to user-space */
> if (!obj->pages)
> return -EACCES;
>
> /* drm_gem_mmap() sets flags that we don't want */
> vma->vm_flags = vm_flags | VM_DONTEXPAND;
> vma->vm_page_prot =
> pgprot_writecombine(vm_get_page_prot(vma->vm_flags));
>
> num = (vma->vm_end - vma->vm_start) >> PAGE_SHIFT;
> for (i = 0; i < num; ++i) {
> r = vm_insert_page(vma, vma->vm_start + i * PAGE_SIZE,
> obj->pages[i]);
> if (r < 0) {
> if (i > 0)
> zap_vma_ptes(vma, vma->vm_start, i * PAGE_SIZE);
> return r;
> }
> }
>
> return 0;
> }
>
> drm_gem_mmap() requires drm_driver->gem_vm_ops to be set:
>
> const struct vm_operations_struct sdrm_gem_vm_ops = {
> .open = drm_gem_vm_open,
> .close = drm_gem_vm_close,
> };
>
> And browsing the code a bit more shows that tegra, udl, etnaviv and vgem
> does the vm_insert_page() thing in the vm_operations_struct->fault()
> handler.
>
> So maybe this makes sense for simpledrm as well:
>
> static int sdrm_gem_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
> {
> struct drm_gem_object *obj = vma->vm_private_data;
> struct sdrm_gem_object *sobj = to_sdrm_bo(obj);
> loff_t num_pages;
> pgoff_t offset;
> int r;
>
> if (!sobj->pages)
> return VM_FAULT_SIGBUS;
>
> offset = ((unsigned long)vmf->virtual_address - vma->vm_start) >>
> PAGE_SHIFT;
> num_pages = DIV_ROUND_UP(obj->base.size, PAGE_SIZE);
> if (page_offset > num_pages)
> return VM_FAULT_SIGBUS;
>
> r = vm_insert_page(vma, (unsigned long)vmf->virtual_address,
> sobj->pages[offset]);
> switch (r) {
> case -EAGAIN:
> case 0:
> case -ERESTARTSYS:
> case -EINTR:
> case -EBUSY:
> return VM_FAULT_NOPAGE;
>
> case -ENOMEM:
> return VM_FAULT_OOM;
> }
>
> return VM_FAULT_SIGBUS;
> }
That's still a lot for what amounts to reimplementing mmap on shmem, but
badly. What I mean with redirecting is pointing the entire ->mmap
operation to the mmap implementation for the underlying mmap. Roughly:
/* normal gem mmap checks first */
/* redirect to shmem mmap */
vma->vm_file = obj->filp;
vma->vm_pgoff = 0;
return obj->filp->f_op->mmap(obj->filp, vma);
Much less code ;-)
Cheers, Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
[toc] | [prev] | [next] | [standalone]
| From | Noralf Trønnes <noralf@tronnes.org> |
|---|---|
| Date | 2016-08-16 21:50 +0200 |
| Subject | Re: [PATCH v3 1/3] drm: add SimpleDRM driver |
| Message-ID | <s6Sx4-7iI-19@gated-at.bofh.it> |
| In reply to | #1463889 |
Den 16.08.2016 17:25, skrev Daniel Vetter:
> On Tue, Aug 16, 2016 at 02:58:38PM +0200, Noralf Trønnes wrote:
>> Den 15.08.2016 08:59, skrev Daniel Vetter:
>>> On Sun, Aug 14, 2016 at 06:52:04PM +0200, Noralf Trønnes wrote:
>>>> The SimpleDRM driver binds to simple-framebuffer devices and provides a
>>>> DRM/KMS API. It provides only a single CRTC+encoder+connector combination
>>>> plus one initial mode.
>>>>
>>>> Userspace can create dumb-buffers which can be blit into the real
>>>> framebuffer similar to UDL. No access to the real framebuffer is allowed
>>>> (compared to earlier version of this driver) to avoid security issues.
>>>> Furthermore, this way we can support arbitrary modes as long as we have a
>>>> conversion-helper.
>>>>
>>>> The driver was originally written by David Herrmann in 2014.
>>>> My main contribution is to make use of drm_simple_kms_helper and
>>>> rework the probe path to avoid use of the deprecated drm_platform_init()
>>>> and drm_driver.{load,unload}().
>>>> Additions have also been made for later changes to the Device Tree binding
>>>> document, like support for clocks, regulators and having the node under
>>>> /chosen. This code was lifted verbatim from simplefb.c.
>>>>
>>>> Cc: dh.herrmann@gmail.com
>>>> Cc: libv@skynet.be
>>>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
>> <snip>
>>
>>>> diff --git a/drivers/gpu/drm/simpledrm/simpledrm_gem.c b/drivers/gpu/drm/simpledrm/simpledrm_gem.c
>>>> new file mode 100644
>>>> index 0000000..81bd7c5
>>>> --- /dev/null
>>>> +++ b/drivers/gpu/drm/simpledrm/simpledrm_gem.c
>> <snip>
>>
>>>> +struct sdrm_gem_object *sdrm_gem_alloc_object(struct drm_device *ddev,
>>>> + size_t size)
>>>> +{
>>>> + struct sdrm_gem_object *obj;
>>>> +
>>>> + WARN_ON(!size || (size & ~PAGE_MASK) != 0);
>>>> +
>>>> + obj = kzalloc(sizeof(*obj), GFP_KERNEL);
>>>> + if (!obj)
>>>> + return NULL;
>>>> +
>>>> + drm_gem_private_object_init(ddev, &obj->base, size);
>>>> + return obj;
>>>> +}
>>>> +
>>>> +void sdrm_gem_free_object(struct drm_gem_object *gobj)
>>>> +{
>>>> + struct sdrm_gem_object *obj = to_sdrm_bo(gobj);
>>>> + struct drm_device *ddev = gobj->dev;
>>>> +
>>>> + if (obj->pages) {
>>>> + /* kill all user-space mappings */
>>>> + drm_vma_node_unmap(&gobj->vma_node,
>>>> + ddev->anon_inode->i_mapping);
>>>> + sdrm_gem_put_pages(obj);
>>>> + }
>>>> +
>>>> + if (gobj->import_attach)
>>>> + drm_prime_gem_destroy(gobj, obj->sg);
>>>> +
>>>> + drm_gem_free_mmap_offset(gobj);
>>>> + drm_gem_object_release(gobj);
>>>> + kfree(obj);
>>>> +}
>>>> +
>>>> +int sdrm_dumb_create(struct drm_file *dfile, struct drm_device *ddev,
>>>> + struct drm_mode_create_dumb *args)
>>>> +{
>>>> + struct sdrm_gem_object *obj;
>>>> + int r;
>>>> +
>>>> + if (args->flags)
>>>> + return -EINVAL;
>>>> +
>>>> + /* overflow checks are done by DRM core */
>>>> + args->pitch = (args->bpp + 7) / 8 * args->width;
>>>> + args->size = PAGE_ALIGN(args->pitch * args->height);
>>>> +
>>>> + obj = sdrm_gem_alloc_object(ddev, args->size);
>>>> + if (!obj)
>>>> + return -ENOMEM;
>>>> +
>>>> + r = drm_gem_handle_create(dfile, &obj->base, &args->handle);
>>>> + if (r) {
>>>> + drm_gem_object_unreference_unlocked(&obj->base);
>>>> + return r;
>>>> + }
>>>> +
>>>> + /* handle owns a reference */
>>>> + drm_gem_object_unreference_unlocked(&obj->base);
>>>> + return 0;
>>>> +}
>>>> +
>>>> +int sdrm_dumb_destroy(struct drm_file *dfile, struct drm_device *ddev,
>>>> + uint32_t handle)
>>>> +{
>>>> + return drm_gem_handle_delete(dfile, handle);
>>>> +}
>>> I wonder whether some dumb+gem helpers wouldn't be useful to roll out. At
>>> least drm_dumb_gem_destroy.c would be pretty simple.
>> There's already a drm_gem_dumb_destroy() in drm_gem.c
>>
>> The drm_driver->gem_create_object callback makes it possible to do a
>> generic dumb create:
>>
>> int drm_gem_dumb_create(struct drm_file *file, struct drm_device *dev,
>> struct drm_mode_create_dumb *args)
>> {
>> struct drm_gem_object *obj;
>> int ret;
>>
>> if (!dev->driver->gem_create_object)
>> return -EINVAL;
>>
>> args->pitch = DIV_ROUND_UP(args->width * args->bpp, 8);
>> args->size = args->pitch * args->height;
>>
>> obj = dev->driver->gem_create_object(dev, args->size);
>> if (!obj)
>> return -ENOMEM;
>>
>> ret = drm_gem_handle_create(file, obj, &args->handle);
>> drm_gem_object_unreference_unlocked(obj);
>>
>> return ret;
>> }
>> EXPORT_SYMBOL(drm_gem_dumb_create);
>>
>> struct drm_gem_object *sdrm_gem_create_object(struct drm_device *ddev,
>> size_t size)
>> {
>> struct sdrm_gem_object *sobj;
>>
>> sobj = sdrm_gem_alloc_object(ddev, size);
>> if (!sobj)
>> return ERR_PTR(-ENOMEM);
>>
>> return &sobj->base;
>> }
>>
>>>> +
>>>> +int sdrm_dumb_map_offset(struct drm_file *dfile, struct drm_device *ddev,
>>>> + uint32_t handle, uint64_t *offset)
>>>> +{
>>>> + struct drm_gem_object *gobj;
>>>> + int r;
>>>> +
>>>> + mutex_lock(&ddev->struct_mutex);
>>> There's still some struct mutex here.
>>>
>>>> +
>>>> + gobj = drm_gem_object_lookup(dfile, handle);
>>>> + if (!gobj) {
>>>> + r = -ENOENT;
>>>> + goto out_unlock;
>>>> + }
>>>> +
>>>> + r = drm_gem_create_mmap_offset(gobj);
>>>> + if (r)
>>>> + goto out_unref;
>>>> +
>>>> + *offset = drm_vma_node_offset_addr(&gobj->vma_node);
>>>> +
>>>> +out_unref:
>>>> + drm_gem_object_unreference(gobj);
>>>> +out_unlock:
>>>> + mutex_unlock(&ddev->struct_mutex);
>>>> + return r;
>>>> +}
>>>> +
>> Maybe this addition to drm_gem.c as well:
>>
>> int drm_gem_dumb_map_offset(struct drm_file *file, struct drm_device *dev,
>> uint32_t handle, uint64_t *offset)
>> {
>> struct drm_gem_object *obj;
>> int ret;
>>
>> obj = drm_gem_object_lookup(file, handle);
>> if (!obj)
>> return -ENOENT;
>>
>> ret = drm_gem_create_mmap_offset(obj);
>> if (ret)
>> goto out_unref;
>>
>> *offset = drm_vma_node_offset_addr(&obj->vma_node);
>>
>> out_unref:
>> drm_gem_object_unreference_unlocked(obj);
>>
>> return ret;
>> }
> Yeah, sounds good for adding the above two. Feel free to roll them out to
> drivers (or not).
>
>>>> +int sdrm_drm_mmap(struct file *filp, struct vm_area_struct *vma)
>>>> +{
>>>> + struct drm_file *priv = filp->private_data;
>>>> + struct drm_device *dev = priv->minor->dev;
>>>> + struct drm_vma_offset_node *node;
>>>> + struct drm_gem_object *gobj;
>>>> + struct sdrm_gem_object *obj;
>>>> + size_t size, i, num;
>>>> + int r;
>>>> +
>>>> + if (drm_device_is_unplugged(dev))
>>>> + return -ENODEV;
>>>> +
>>>> + drm_vma_offset_lock_lookup(dev->vma_offset_manager);
>>>> + node = drm_vma_offset_exact_lookup_locked(dev->vma_offset_manager,
>>>> + vma->vm_pgoff,
>>>> + vma_pages(vma));
>>>> + drm_vma_offset_unlock_lookup(dev->vma_offset_manager);
>>>> +
>>>> + if (!drm_vma_node_is_allowed(node, filp))
>>>> + return -EACCES;
>>>> +
>>>> + gobj = container_of(node, struct drm_gem_object, vma_node);
>>>> + obj = to_sdrm_bo(gobj);
>>>> + size = drm_vma_node_size(node) << PAGE_SHIFT;
>>>> + if (size < vma->vm_end - vma->vm_start)
>>>> + return r;
>>>> +
>>>> + r = sdrm_gem_get_pages(obj);
>>>> + if (r < 0)
>>>> + return r;
>>>> +
>>>> + /* prevent dmabuf-imported mmap to user-space */
>>>> + if (!obj->pages)
>>>> + return -EACCES;
>>>> +
>>>> + vma->vm_flags |= VM_DONTEXPAND;
>>>> + vma->vm_page_prot = pgprot_writecombine(vm_get_page_prot(vma->vm_flags));
>>>> +
>>>> + num = (vma->vm_end - vma->vm_start) >> PAGE_SHIFT;
>>>> + for (i = 0; i < num; ++i) {
>>>> + r = vm_insert_page(vma, vma->vm_start + i * PAGE_SIZE,
>>>> + obj->pages[i]);
>>>> + if (r < 0) {
>>>> + if (i > 0)
>>>> + zap_vma_ptes(vma, vma->vm_start, i * PAGE_SIZE);
>>>> + return r;
>>>> + }
>>>> + }
>>>> +
>>>> + return 0;
>>>> +}
>>> Why can't we just redirect to the underlying shmem mmap here (after
>>> argument checking)?
>> I don't know what that means, but looking at vc4 it seems I can use
>> drm_gem_mmap() to remove some boilerplate.
>>
>> int sdrm_drm_mmap(struct file *filp, struct vm_area_struct *vma)
>> {
>> unsigned long vm_flags = vma->vm_flags;
>> struct sdrm_gem_object *sobj;
>> struct drm_gem_object *obj;
>> size_t i, num;
>> int r;
>>
>> r = drm_gem_mmap(filp, vma);
>> if (r)
>> return r;
>>
>> obj = vma->vm_private_data;
>>
>> sobj = to_sdrm_bo(obj);
>>
>> r = sdrm_gem_get_pages(obj);
>> if (r < 0)
>> return r;
>>
>> /* prevent dmabuf-imported mmap to user-space */
>> if (!obj->pages)
>> return -EACCES;
>>
>> /* drm_gem_mmap() sets flags that we don't want */
>> vma->vm_flags = vm_flags | VM_DONTEXPAND;
>> vma->vm_page_prot =
>> pgprot_writecombine(vm_get_page_prot(vma->vm_flags));
>>
>> num = (vma->vm_end - vma->vm_start) >> PAGE_SHIFT;
>> for (i = 0; i < num; ++i) {
>> r = vm_insert_page(vma, vma->vm_start + i * PAGE_SIZE,
>> obj->pages[i]);
>> if (r < 0) {
>> if (i > 0)
>> zap_vma_ptes(vma, vma->vm_start, i * PAGE_SIZE);
>> return r;
>> }
>> }
>>
>> return 0;
>> }
>>
>> drm_gem_mmap() requires drm_driver->gem_vm_ops to be set:
>>
>> const struct vm_operations_struct sdrm_gem_vm_ops = {
>> .open = drm_gem_vm_open,
>> .close = drm_gem_vm_close,
>> };
>>
>> And browsing the code a bit more shows that tegra, udl, etnaviv and vgem
>> does the vm_insert_page() thing in the vm_operations_struct->fault()
>> handler.
>>
>> So maybe this makes sense for simpledrm as well:
>>
>> static int sdrm_gem_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
>> {
>> struct drm_gem_object *obj = vma->vm_private_data;
>> struct sdrm_gem_object *sobj = to_sdrm_bo(obj);
>> loff_t num_pages;
>> pgoff_t offset;
>> int r;
>>
>> if (!sobj->pages)
>> return VM_FAULT_SIGBUS;
>>
>> offset = ((unsigned long)vmf->virtual_address - vma->vm_start) >>
>> PAGE_SHIFT;
>> num_pages = DIV_ROUND_UP(obj->base.size, PAGE_SIZE);
>> if (page_offset > num_pages)
>> return VM_FAULT_SIGBUS;
>>
>> r = vm_insert_page(vma, (unsigned long)vmf->virtual_address,
>> sobj->pages[offset]);
>> switch (r) {
>> case -EAGAIN:
>> case 0:
>> case -ERESTARTSYS:
>> case -EINTR:
>> case -EBUSY:
>> return VM_FAULT_NOPAGE;
>>
>> case -ENOMEM:
>> return VM_FAULT_OOM;
>> }
>>
>> return VM_FAULT_SIGBUS;
>> }
> That's still a lot for what amounts to reimplementing mmap on shmem, but
> badly. What I mean with redirecting is pointing the entire ->mmap
> operation to the mmap implementation for the underlying mmap. Roughly:
>
> /* normal gem mmap checks first */
>
> /* redirect to shmem mmap */
> vma->vm_file = obj->filp;
> vma->vm_pgoff = 0;
>
> return obj->filp->f_op->mmap(obj->filp, vma);
>
> Much less code ;-)
obj->filp is NULL in my case.
And looking at the docs, that's expected since I have driver specific
backing?
/**
* @filp:
*
* SHMEM file node used as backing storage for swappable buffer objects.
* GEM also supports driver private objects with driver-specific backing
* storage (contiguous CMA memory, special reserved blocks). In this
* case @filp is NULL.
*/
Noralf.
[toc] | [prev] | [next] | [standalone]
| From | Daniel Vetter <daniel@ffwll.ch> |
|---|---|
| Date | 2016-08-17 11:50 +0200 |
| Subject | Re: [PATCH v3 1/3] drm: add SimpleDRM driver |
| Message-ID | <s75DX-7DZ-9@gated-at.bofh.it> |
| In reply to | #1464032 |
On Tue, Aug 16, 2016 at 9:38 PM, Noralf Trønnes <noralf@tronnes.org> wrote: >> That's still a lot for what amounts to reimplementing mmap on shmem, but >> badly. What I mean with redirecting is pointing the entire ->mmap >> operation to the mmap implementation for the underlying mmap. Roughly: >> >> /* normal gem mmap checks first */ >> >> /* redirect to shmem mmap */ >> vma->vm_file = obj->filp; >> vma->vm_pgoff = 0; >> >> return obj->filp->f_op->mmap(obj->filp, vma); >> >> Much less code ;-) > > > obj->filp is NULL in my case. > > And looking at the docs, that's expected since I have driver specific > backing? > > /** > * @filp: > * > * SHMEM file node used as backing storage for swappable buffer objects. > * GEM also supports driver private objects with driver-specific backing > * storage (contiguous CMA memory, special reserved blocks). In this > * case @filp is NULL. > */ Hm, I totally misread the driver code. I assumed that we'd just allocate normal shmem gem objects, and then copy them on-demand onto the frontbuffer (in the dirty or plane update callbacks). Essentially treat the firmware fb area as a manual upload display, except that we don't use i2c or spi to do the upload, but normal mmio writes. I think that would greatly simplify the driver, and more important: It would also work like any other kms driver. Currently sdrm is violiting the spec a bit by aliasing all dumb buffers to the same underlying backing storage, and that's a bit evil. The other bit I noticed (and why I was confused): The prime import code reinvents a lot of wheels, and it digs into the backing storage directly. Instead it should just call dma_buf_vmap/dma_buf_vunmap and let the exporter figure out how it works. Sorry I was all confused here and didn't realize what's going on :( -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
[toc] | [prev] | [next] | [standalone]
| From | Noralf Trønnes <noralf@tronnes.org> |
|---|---|
| Date | 2016-08-17 12:20 +0200 |
| Subject | Re: [PATCH v3 1/3] drm: add SimpleDRM driver |
| Message-ID | <s7670-84T-11@gated-at.bofh.it> |
| In reply to | #1464452 |
Den 17.08.2016 11:30, skrev Daniel Vetter: > On Tue, Aug 16, 2016 at 9:38 PM, Noralf Trønnes <noralf@tronnes.org> wrote: >>> That's still a lot for what amounts to reimplementing mmap on shmem, but >>> badly. What I mean with redirecting is pointing the entire ->mmap >>> operation to the mmap implementation for the underlying mmap. Roughly: >>> >>> /* normal gem mmap checks first */ >>> >>> /* redirect to shmem mmap */ >>> vma->vm_file = obj->filp; >>> vma->vm_pgoff = 0; >>> >>> return obj->filp->f_op->mmap(obj->filp, vma); >>> >>> Much less code ;-) >> >> obj->filp is NULL in my case. >> >> And looking at the docs, that's expected since I have driver specific >> backing? >> >> /** >> * @filp: >> * >> * SHMEM file node used as backing storage for swappable buffer objects. >> * GEM also supports driver private objects with driver-specific backing >> * storage (contiguous CMA memory, special reserved blocks). In this >> * case @filp is NULL. >> */ > Hm, I totally misread the driver code. I assumed that we'd just > allocate normal shmem gem objects, and then copy them on-demand onto > the frontbuffer (in the dirty or plane update callbacks). Essentially > treat the firmware fb area as a manual upload display, except that we > don't use i2c or spi to do the upload, but normal mmio writes. I think > that would greatly simplify the driver, and more important: It would > also work like any other kms driver. Currently sdrm is violiting the > spec a bit by aliasing all dumb buffers to the same underlying backing > storage, and that's a bit evil. > > The other bit I noticed (and why I was confused): The prime import > code reinvents a lot of wheels, and it digs into the backing storage > directly. Instead it should just call dma_buf_vmap/dma_buf_vunmap and > let the exporter figure out how it works. Is there a driver I can look at that does it the way we want? I'm afraid that this gem framework is still over my head. Copying code or patterns is easy, starting from scratch demands understanding :-) Noralf.
[toc] | [prev] | [next] | [standalone]
| From | Daniel Vetter <daniel@ffwll.ch> |
|---|---|
| Date | 2016-08-17 13:20 +0200 |
| Subject | Re: [PATCH v3 1/3] drm: add SimpleDRM driver |
| Message-ID | <s7733-g9-9@gated-at.bofh.it> |
| In reply to | #1464475 |
On Wed, Aug 17, 2016 at 12:19:02PM +0200, Noralf Trønnes wrote: > > Den 17.08.2016 11:30, skrev Daniel Vetter: > > On Tue, Aug 16, 2016 at 9:38 PM, Noralf Trønnes <noralf@tronnes.org> wrote: > > > > That's still a lot for what amounts to reimplementing mmap on shmem, but > > > > badly. What I mean with redirecting is pointing the entire ->mmap > > > > operation to the mmap implementation for the underlying mmap. Roughly: > > > > > > > > /* normal gem mmap checks first */ > > > > > > > > /* redirect to shmem mmap */ > > > > vma->vm_file = obj->filp; > > > > vma->vm_pgoff = 0; > > > > > > > > return obj->filp->f_op->mmap(obj->filp, vma); > > > > > > > > Much less code ;-) > > > > > > obj->filp is NULL in my case. > > > > > > And looking at the docs, that's expected since I have driver specific > > > backing? > > > > > > /** > > > * @filp: > > > * > > > * SHMEM file node used as backing storage for swappable buffer objects. > > > * GEM also supports driver private objects with driver-specific backing > > > * storage (contiguous CMA memory, special reserved blocks). In this > > > * case @filp is NULL. > > > */ > > Hm, I totally misread the driver code. I assumed that we'd just > > allocate normal shmem gem objects, and then copy them on-demand onto > > the frontbuffer (in the dirty or plane update callbacks). Essentially > > treat the firmware fb area as a manual upload display, except that we > > don't use i2c or spi to do the upload, but normal mmio writes. I think > > that would greatly simplify the driver, and more important: It would > > also work like any other kms driver. Currently sdrm is violiting the > > spec a bit by aliasing all dumb buffers to the same underlying backing > > storage, and that's a bit evil. > > > > The other bit I noticed (and why I was confused): The prime import > > code reinvents a lot of wheels, and it digs into the backing storage > > directly. Instead it should just call dma_buf_vmap/dma_buf_vunmap and > > let the exporter figure out how it works. > > Is there a driver I can look at that does it the way we want? > I'm afraid that this gem framework is still over my head. > Copying code or patterns is easy, starting from scratch demands > understanding :-) udl_gem.c is pretty much what I had in mind, including the handling of dma-bufs and all that. Not sure how much point there is in sharing code, at least if you can't test udl. What udl doesn't do is directly redirect the mmap to the shmem node (and reject the mmap for dma-buf imported buffers, since that's not a good idea really). -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web