Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1456429 > unrolled thread

[PATCH 0/2] drm: add SimpleDRM driver

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/2] drm: add SimpleDRM driver Noralf Trønnes <noralf@tronnes.org> - 2016-08-04 16:20 +0200
    [PATCH 2/2] drm: simpledrm: add fbdev fallback support Noralf Trønnes <noralf@tronnes.org> - 2016-08-04 16:20 +0200
    Re: [PATCH 0/2] drm: add SimpleDRM driver Luc Verhaegen <libv@skynet.be> - 2016-08-04 16:30 +0200
      Re: [PATCH 0/2] drm: add SimpleDRM driver Daniel Vetter <daniel@ffwll.ch> - 2016-08-04 17:10 +0200
        Re: [PATCH 0/2] drm: add SimpleDRM driver Luc Verhaegen <libv@skynet.be> - 2016-08-04 17:40 +0200
          Re: [PATCH 0/2] drm: add SimpleDRM driver David Herrmann <dh.herrmann@gmail.com> - 2016-08-04 17:50 +0200
            Re: [PATCH 0/2] drm: add SimpleDRM driver Luc Verhaegen <libv@skynet.be> - 2016-08-04 18:00 +0200
              Re: [PATCH 0/2] drm: add SimpleDRM driver Daniel Vetter <daniel@ffwll.ch> - 2016-08-04 19:20 +0200
        Re: [PATCH 0/2] drm: add SimpleDRM driver One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk> - 2016-08-04 20:10 +0200
      Re: [PATCH 0/2] drm: add SimpleDRM driver Noralf Trønnes <noralf@tronnes.org> - 2016-08-04 19:00 +0200
        Re: [PATCH 0/2] drm: add SimpleDRM driver Luc Verhaegen <libv@skynet.be> - 2016-08-04 20:10 +0200
          Re: [PATCH 0/2] drm: add SimpleDRM driver Hans de Goede <hdegoede@redhat.com> - 2016-08-05 09:20 +0200
    Re: [PATCH 0/2] drm: add SimpleDRM driver Daniel Vetter <daniel@ffwll.ch> - 2016-08-04 16:40 +0200
      Re: [PATCH 0/2] drm: add SimpleDRM driver Noralf Trønnes <noralf@tronnes.org> - 2016-08-04 19:40 +0200

#1456429 — [PATCH 0/2] drm: add SimpleDRM driver

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.

I have done my best to bring simpledrm up to speed. However I was unable to
loose drm_legacy_mmap() in sdrm_drm_mmap() since I don't understand much of
the gem code.

I was left with some questions after doing this:

- Is there any reason why simpledrm can't use drm_gem_cma_helper?
  One obvious difference is that of contiguous memory allocation:

  sdrm_gem_get_pages():
	obj->pages = drm_malloc_ab(num, sizeof(*obj->pages));
	for (i = 0; i < num; ++i) {
		obj->pages[i] = alloc_page(GFP_KERNEL | __GFP_ZERO);
	obj->vmapping = vmap(obj->pages, num, 0, PAGE_KERNEL);

  drm_gem_cma_create():
        cma_obj->vaddr = dma_alloc_wc(drm->dev, size, &cma_obj->paddr,
                                      GFP_KERNEL | __GFP_NOWARN);


- Could we set this range to the actual width/height of the native framebuffer?

  sdrm_drm_modeset_init():
	ddev->mode_config.min_width = 1;
	ddev->mode_config.min_height = 1;
	ddev->mode_config.max_width = 8192;
	ddev->mode_config.max_height = 8192;


- Is there a usecase for offsetting the dumb buffer when blitted onto the
  native buffer?

  sdrm_blit():
	/* get scanout offsets */
	xoff = 0;
	yoff = 0;
	if (sdrm->pipe.plane.fb == fb) {
		xoff = sdrm->pipe.crtc.x;
		yoff = sdrm->pipe.crtc.y;
	}


I have tested simpledrm on a Raspberry Pi B+ with U-boot setting up the
framebuffer and producing this node:

        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 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 (2):
  drm: add SimpleDRM driver
  drm: simpledrm: add fbdev fallback support

 drivers/gpu/drm/Kconfig                      |   2 +
 drivers/gpu/drm/Makefile                     |   1 +
 drivers/gpu/drm/simpledrm/Kconfig            |  22 ++
 drivers/gpu/drm/simpledrm/Makefile           |   5 +
 drivers/gpu/drm/simpledrm/simpledrm.h        | 111 ++++++++++
 drivers/gpu/drm/simpledrm/simpledrm_damage.c | 314 +++++++++++++++++++++++++++
 drivers/gpu/drm/simpledrm/simpledrm_drv.c    | 298 +++++++++++++++++++++++++
 drivers/gpu/drm/simpledrm/simpledrm_fbdev.c  | 160 ++++++++++++++
 drivers/gpu/drm/simpledrm/simpledrm_gem.c    | 276 +++++++++++++++++++++++
 drivers/gpu/drm/simpledrm/simpledrm_kms.c    | 288 ++++++++++++++++++++++++
 10 files changed, 1477 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]

#1456430 — [PATCH 2/2] drm: simpledrm: add fbdev fallback support

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 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       |  24 +++++
 drivers/gpu/drm/simpledrm/simpledrm_drv.c   |   3 +
 drivers/gpu/drm/simpledrm/simpledrm_fbdev.c | 160 ++++++++++++++++++++++++++++
 drivers/gpu/drm/simpledrm/simpledrm_kms.c   |  12 +++
 6 files changed, 203 insertions(+)
 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 e841655..a1ed251 100644
--- a/drivers/gpu/drm/simpledrm/simpledrm.h
+++ b/drivers/gpu/drm/simpledrm/simpledrm.h
@@ -30,6 +30,7 @@ struct sdrm_device {
 	struct drm_device *ddev;
 	struct drm_simple_display_pipe pipe;
 	struct drm_connector conn;
+	struct fb_info *fbdev;

 	/* framebuffer information */
 	const struct simplefb_format *fb_sformat;
@@ -43,6 +44,7 @@ struct sdrm_device {
 	void *fb_map;
 };

+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);

@@ -84,4 +86,26 @@ 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_set_suspend(struct sdrm_device *sdrm, int state);
+
+#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_set_suspend(struct sdrm_device *sdrm, int state)
+{
+}
+
+#endif
+
 #endif /* SDRM_DRV_H */
diff --git a/drivers/gpu/drm/simpledrm/simpledrm_drv.c b/drivers/gpu/drm/simpledrm/simpledrm_drv.c
index 26b4ca9..5574fec 100644
--- a/drivers/gpu/drm/simpledrm/simpledrm_drv.c
+++ b/drivers/gpu/drm/simpledrm/simpledrm_drv.c
@@ -36,6 +36,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,
@@ -223,6 +224,7 @@ static int sdrm_simplefb_probe(struct platform_device *pdev)
 	if (ret)
 		goto err_cleanup;

+	sdrm_fbdev_init(ddev->dev_private);
 	platform_set_drvdata(pdev, ddev);

 	DRM_INFO("Initialized %s on minor %d\n", ddev->driver->name,
@@ -246,6 +248,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..b83646b
--- /dev/null
+++ b/drivers/gpu/drm/simpledrm/simpledrm_fbdev.c
@@ -0,0 +1,160 @@
+/*
+ * 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.
+ */
+
+/*
+ * fbdev compatibility layer
+ * We provide a basic fbdev device for the same framebuffer that is used for
+ * the pseudo CRTC.
+ */
+
+#include <linux/console.h>
+#include <linux/errno.h>
+#include <linux/kernel.h>
+#include <linux/mm.h>
+#include <linux/module.h>
+#include <linux/string.h>
+#include <linux/fb.h>
+#include "simpledrm.h"
+
+struct sdrm_fbdev {
+	u32 palette[16];
+};
+
+static int sdrm_fbdev_setcolreg(u_int regno, u_int red, u_int green,
+				u_int blue, u_int transp, struct fb_info *info)
+{
+	u32 *pal = info->pseudo_palette;
+	u32 cr = red >> (16 - info->var.red.length);
+	u32 cg = green >> (16 - info->var.green.length);
+	u32 cb = blue >> (16 - info->var.blue.length);
+	u32 value;
+
+	if (regno >= 16)
+		return -EINVAL;
+
+	value = (cr << info->var.red.offset) |
+		(cg << info->var.green.offset) |
+		(cb << info->var.blue.offset);
+
+	if (info->var.transp.length > 0) {
+		u32 mask = (1 << info->var.transp.length) - 1;
+
+		mask <<= info->var.transp.offset;
+		value |= mask;
+	}
+
+	pal[regno] = value;
+
+	return 0;
+}
+
+static struct fb_ops sdrm_fbdev_ops = {
+	.owner		= THIS_MODULE,
+	.fb_setcolreg	= sdrm_fbdev_setcolreg,
+	.fb_fillrect	= cfb_fillrect,
+	.fb_copyarea	= cfb_copyarea,
+	.fb_imageblit	= cfb_imageblit,
+};
+
+void sdrm_fbdev_init(struct sdrm_device *sdrm)
+{
+	struct sdrm_fbdev *fb;
+	struct fb_info *info;
+	int ret;
+
+	if (fb_get_options("simpledrmfb", NULL))
+		return;
+
+	info = framebuffer_alloc(sizeof(struct sdrm_fbdev), sdrm->ddev->dev);
+	if (!info)
+		goto err_out;
+
+	fb = info->par;
+	info->flags = FBINFO_DEFAULT | FBINFO_MISC_FIRMWARE |
+		      FBINFO_CAN_FORCE_OUTPUT;
+	info->pseudo_palette = fb->palette;
+	info->fbops = &sdrm_fbdev_ops;
+	info->screen_base = sdrm->fb_map;
+
+	strncpy(info->fix.id, "simpledrmfb", 15);
+	info->fix.type = FB_TYPE_PACKED_PIXELS;
+	info->fix.visual = FB_VISUAL_TRUECOLOR;
+	info->fix.accel = FB_ACCEL_NONE;
+	info->fix.smem_start = (unsigned long)sdrm->fb_base;
+	info->fix.smem_len = sdrm->fb_size;
+	info->fix.line_length = sdrm->fb_stride;
+
+	info->var.activate = FB_ACTIVATE_NOW;
+	info->var.vmode = FB_VMODE_NONINTERLACED;
+	info->var.bits_per_pixel = sdrm->fb_bpp;
+	info->var.height = -1;
+	info->var.width = -1;
+	info->var.xres = sdrm->fb_width;
+	info->var.yres = sdrm->fb_height;
+	info->var.xres_virtual = info->var.xres;
+	info->var.yres_virtual = info->var.yres;
+	info->var.red = sdrm->fb_sformat->red;
+	info->var.green = sdrm->fb_sformat->green;
+	info->var.blue = sdrm->fb_sformat->blue;
+	info->var.transp = sdrm->fb_sformat->transp;
+
+	/* some dummy values for timing to make fbset happy */
+	info->var.pixclock = 10000000 / info->var.xres * 1000 / info->var.yres;
+	info->var.left_margin = (info->var.xres / 8) & 0xf8;
+	info->var.right_margin = 32;
+	info->var.upper_margin = 16;
+	info->var.lower_margin = 4;
+	info->var.hsync_len = (info->var.xres / 8) & 0xf8;
+	info->var.vsync_len = 4;
+
+	ret = register_framebuffer(info);
+	if (ret < 0)
+		goto err_free;
+
+	sdrm->fbdev = info;
+	dev_info(sdrm->ddev->dev, "fbdev frontend %s as fb%d\n",
+		 info->fix.id, info->node);
+
+	return;
+
+err_free:
+	framebuffer_release(info);
+err_out:
+	dev_warn(sdrm->ddev->dev, "cannot create fbdev frontend\n");
+}
+
+void sdrm_fbdev_cleanup(struct sdrm_device *sdrm)
+{
+	struct fb_info *info;
+
+	if (!sdrm->fbdev)
+		return;
+
+	info = sdrm->fbdev;
+	sdrm->fbdev = NULL;
+
+	dev_info(sdrm->ddev->dev, "remove fbdev frontend %s (fb%d)\n",
+		 info->fix.id, info->node);
+
+	if (unregister_framebuffer(info))
+		dev_err(sdrm->ddev->dev, "unregister_framebuffer() failed, leaking fw-fb\n");
+	else
+		framebuffer_release(info);
+}
+
+void sdrm_fbdev_set_suspend(struct sdrm_device *sdrm, int state)
+{
+	if (!sdrm->fbdev)
+		return;
+
+	console_lock();
+	fb_set_suspend(sdrm->fbdev, state);
+	console_unlock();
+}
diff --git a/drivers/gpu/drm/simpledrm/simpledrm_kms.c b/drivers/gpu/drm/simpledrm/simpledrm_kms.c
index 6295a9f..dd176f2 100644
--- a/drivers/gpu/drm/simpledrm/simpledrm_kms.c
+++ b/drivers/gpu/drm/simpledrm/simpledrm_kms.c
@@ -25,6 +25,14 @@ static const uint32_t sdrm_formats[] = {
 	DRM_FORMAT_XRGB8888,
 };

+void sdrm_lastclose(struct drm_device *ddev)
+{
+	struct sdrm_device *sdrm = ddev->dev_private;
+
+	/* resume fbcon/fbdev */
+	sdrm_fbdev_set_suspend(sdrm, 0);
+}
+
 static int sdrm_conn_get_modes(struct drm_connector *conn)
 {
 	struct sdrm_device *sdrm = conn->dev->dev_private;
@@ -118,7 +126,11 @@ static void sdrm_crtc_send_vblank_event(struct drm_crtc *crtc)
 static void sdrm_display_pipe_enable(struct drm_simple_display_pipe *pipe,
 				     struct drm_crtc_state *crtc_state)
 {
+	struct sdrm_device *sdrm = pipe_to_sdrm(pipe);
+
 	sdrm_crtc_send_vblank_event(&pipe->crtc);
+	/* suspend fbcon/fbdev */
+	sdrm_fbdev_set_suspend(sdrm, 1);
 }

 static void sdrm_display_pipe_disable(struct drm_simple_display_pipe *pipe)
--
2.8.2

[toc] | [prev] | [next] | [standalone]

#1456433

On Thu, Aug 04, 2016 at 04:03:18PM +0200, Noralf Trønnes wrote:
> 
> I have tested simpledrm on a Raspberry Pi B+ with U-boot setting up the
> framebuffer and producing this node:
> 
>         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).

Please do not make the same mistake as simplefb in making this purely a 
rpifb. Know that you will need some power and clock management for 
properly free devices that do not depend on a binary only RTOS which 
does _everything_ behind our backs.

Cfr this useless, endless and ridiculous discussion: 
http://lists.infradead.org/pipermail/linux-arm-kernel/2014-August/279071.html

Luc Verhaegen.

[toc] | [prev] | [next] | [standalone]

#1456461

On Thu, Aug 04, 2016 at 04:15:25PM +0200, Luc Verhaegen wrote:
> On Thu, Aug 04, 2016 at 04:03:18PM +0200, Noralf Trønnes wrote:
> > 
> > I have tested simpledrm on a Raspberry Pi B+ with U-boot setting up the
> > framebuffer and producing this node:
> > 
> >         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).
> 
> Please do not make the same mistake as simplefb in making this purely a 
> rpifb. Know that you will need some power and clock management for 
> properly free devices that do not depend on a binary only RTOS which 
> does _everything_ behind our backs.
> 
> Cfr this useless, endless and ridiculous discussion: 
> http://lists.infradead.org/pipermail/linux-arm-kernel/2014-August/279071.html

simpledrm isn't a real driver, but only meant to be used to drive the
firmware framebuffer in early boot until a real driver takes over. It's a
replacement really for all the various uefi/vesa/whatever fbdev drivers.
Full reliance on the firmware very much intended.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[toc] | [prev] | [next] | [standalone]

#1456489

On Thu, Aug 04, 2016 at 05:08:43PM +0200, Daniel Vetter wrote:
> On Thu, Aug 04, 2016 at 04:15:25PM +0200, Luc Verhaegen wrote:
> > On Thu, Aug 04, 2016 at 04:03:18PM +0200, Noralf Trønnes wrote:
> > > 
> > > I have tested simpledrm on a Raspberry Pi B+ with U-boot setting up the
> > > framebuffer and producing this node:
> > > 
> > >         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).
> > 
> > Please do not make the same mistake as simplefb in making this purely a 
> > rpifb. Know that you will need some power and clock management for 
> > properly free devices that do not depend on a binary only RTOS which 
> > does _everything_ behind our backs.
> > 
> > Cfr this useless, endless and ridiculous discussion: 
> > http://lists.infradead.org/pipermail/linux-arm-kernel/2014-August/279071.html
> 
> simpledrm isn't a real driver, but only meant to be used to drive the
> firmware framebuffer in early boot until a real driver takes over. It's a
> replacement really for all the various uefi/vesa/whatever fbdev drivers.
> Full reliance on the firmware very much intended.
> -Daniel

In the sunxi case, that firmware was u-boot, and the clocks were 
properly declared and then also properly disabled, which meant the 
display block got disabled.

Is simpledrm only an intermediate solution until the real driver is 
loaded? What stops it from providing a rudimentary display driver for 
the whole uptime of the machine? What stops the kernel from disabling 
the clocks while the supposed real driver is not fully loaded yet?

Do we really want to recreate a 400+ email thread again, or are we 
capable of learning from the past?

Luc Verhaegen.

[toc] | [prev] | [next] | [standalone]

#1456502

Hi

On Thu, Aug 4, 2016 at 5:34 PM, Luc Verhaegen <libv@skynet.be> wrote:
> Do we really want to recreate a 400+ email thread again, or are we
> capable of learning from the past?

No we don't. And no-one intends to. I am fully aware of the discussion
that introduced the clock-dependencies to simplefb, and I gladly
accept patches that add such support to SimpleDRM. Did anyone say
otherwise? This series adds initial support for the devices _we_ know
and can test (which is x86 and RPi, in my case). If someone wants
support for more devices, please send patches. Why does this have to
be included (or even discussed) as part of this submission?

Thanks
David

[toc] | [prev] | [next] | [standalone]

#1456514

On Thu, Aug 04, 2016 at 05:44:23PM +0200, David Herrmann wrote:
> Hi
> 
> On Thu, Aug 4, 2016 at 5:34 PM, Luc Verhaegen <libv@skynet.be> wrote:
> > Do we really want to recreate a 400+ email thread again, or are we
> > capable of learning from the past?
> 
> No we don't. And no-one intends to. I am fully aware of the discussion
> that introduced the clock-dependencies to simplefb, and I gladly
> accept patches that add such support to SimpleDRM. Did anyone say
> otherwise? This series adds initial support for the devices _we_ know
> and can test (which is x86 and RPi, in my case). If someone wants
> support for more devices, please send patches. Why does this have to
> be included (or even discussed) as part of this submission?

You're right, it does not have to be included until there is a usecase.

But on the otherhand i have become pretty allergic to this as that other 
discussion was completely useless and pointless and stemmed only from 
the fact that people had been kidding themselves all along that simplefb 
was going to be simple and not would need anything extra.

If we can avoid fooling ourselves to the same extent as we did before, 
then putting this off until there is a real usecase is perfectly fine by 
me.

If not, just add this trivial generic addition straight from an existing 
example. 

Luc Verhaegen.

[toc] | [prev] | [next] | [standalone]

#1456597

On Thu, Aug 04, 2016 at 05:59:42PM +0200, Luc Verhaegen wrote:
> On Thu, Aug 04, 2016 at 05:44:23PM +0200, David Herrmann wrote:
> > On Thu, Aug 4, 2016 at 5:34 PM, Luc Verhaegen <libv@skynet.be> wrote:
> > > Do we really want to recreate a 400+ email thread again, or are we
> > > capable of learning from the past?
> > 
> > No we don't. And no-one intends to. I am fully aware of the discussion
> > that introduced the clock-dependencies to simplefb, and I gladly
> > accept patches that add such support to SimpleDRM. Did anyone say
> > otherwise? This series adds initial support for the devices _we_ know
> > and can test (which is x86 and RPi, in my case). If someone wants
> > support for more devices, please send patches. Why does this have to
> > be included (or even discussed) as part of this submission?
> 
> You're right, it does not have to be included until there is a usecase.
> 
> But on the otherhand i have become pretty allergic to this as that other 
> discussion was completely useless and pointless and stemmed only from 
> the fact that people had been kidding themselves all along that simplefb 
> was going to be simple and not would need anything extra.
> 
> If we can avoid fooling ourselves to the same extent as we did before, 
> then putting this off until there is a real usecase is perfectly fine by 
> me.
> 
> If not, just add this trivial generic addition straight from an existing 
> example. 

I stand corrected on this, seems indeed we need this to avoid issues. I
guess on x86 we don't need this since a pci device is a mostly
well-defined entity, and as long as we stop vesa/uefi for simpledrm before
the real driver loads it'll be all fine.

But on arm it's indeed different with clocks (and I guess sooner or later
some power domains too, or whatever else really). I think as long as it
doesn't go beyond "grab resource on load, drop it again on unload" I'm
prefectly fine with adding all that's needed to simpledrmfb. And totally
up to the folks enabling this whether it should be there upfront or only
once needed for a given platform.
-Daniel

> 
> Luc Verhaegen.
> _______________________________________________
> 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]

#1456616

> firmware framebuffer in early boot until a real driver takes over. It's a
> replacement really for all the various uefi/vesa/whatever fbdev drivers.
> Full reliance on the firmware very much intended.

Most of those have firmware interfaces for things like colour setting and
hardware scrolling. It's a replacement for far less than might first
appear but still useful.

Alan

[toc] | [prev] | [next] | [standalone]

#1456543

Den 04.08.2016 16:15, skrev Luc Verhaegen:
> On Thu, Aug 04, 2016 at 04:03:18PM +0200, Noralf Trønnes wrote:
>> I have tested simpledrm on a Raspberry Pi B+ with U-boot setting up the
>> framebuffer and producing this node:
>>
>>          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).
> Please do not make the same mistake as simplefb in making this purely a
> rpifb. Know that you will need some power and clock management for
> properly free devices that do not depend on a binary only RTOS which
> does _everything_ behind our backs.

I didn't read the binding document[1], which I should have done.
If simpledrm claims to be compatible with simple-framebuffer I assume it
should support the entire binding doc which includes clocks, regulators
and having the node under /chosen.
I will lift the necessary code from simplefb.c and put it in the next
version.

The binding doc also mentions an optional display phandle property, but I
can't find any reference to this in simplefb.c.


Noralf.

[1] Documentation/devicetree/bindings/display/simple-framebuffer.txt

[toc] | [prev] | [next] | [standalone]

#1456617

On Thu, Aug 04, 2016 at 06:58:55PM +0200, Noralf Trønnes wrote:
> 
> I didn't read the binding document[1], which I should have done.
> If simpledrm claims to be compatible with simple-framebuffer I assume it
> should support the entire binding doc which includes clocks, regulators
> and having the node under /chosen.
> I will lift the necessary code from simplefb.c and put it in the next
> version.

Smashing, repeat of a massive pain avoided, thanks :)

> The binding doc also mentions an optional display phandle property, but I
> can't find any reference to this in simplefb.c.

By that time i had long, long given up on this, i think Hans de Goede, 
who is a few orders of magnitude more patient than me, should know. He 
valiantly whipped this thing through back then.

Luc Verhaegen.

[toc] | [prev] | [next] | [standalone]

#1456907

Hi,

On 04-08-16 20:12, Luc Verhaegen wrote:
> On Thu, Aug 04, 2016 at 06:58:55PM +0200, Noralf Trønnes wrote:
>>
>> I didn't read the binding document[1], which I should have done.
>> If simpledrm claims to be compatible with simple-framebuffer I assume it
>> should support the entire binding doc which includes clocks, regulators
>> and having the node under /chosen.
>> I will lift the necessary code from simplefb.c and put it in the next
>> version.
>
> Smashing, repeat of a massive pain avoided, thanks :)
>
>> The binding doc also mentions an optional display phandle property, but I
>> can't find any reference to this in simplefb.c.

Ah yes, the display phandle, so the idea behind this is that the
simplefb node would have a display phandle pointing to a node
describing the "primary" node describing the actual display-pipe hardware.

The primary language is there because a display pipeline typically
consists of multiple blocks and thus has multiple nodes describing it.

This way the hardware driver would be able to figure out which simplefb
to disable if there is more then 1.

In practice the remove_conflicting_framebuffers kernel API is used for this and
that takes a framebuffer address, so that bit of the bindings is essentially
unused. Either way that bit is only relevant to the actual display hardware driver
(so that it can disable sumplefb when it takes over the display) and for
simpledrm you can simply ignore it.

Regards,

Hans

p.s.

Noralf, I recognize your name from the ft6236 touchscreen driver, I've mailed
you about this in the past because it is a duplicate driver, the edt-ft5x06
driver already speaks the same protocol. I see now that I made a copy and paste
error in your email address, so you never got my mails on this. I'll resend
my latest mail (a kernel patch removing the duplicate driver!) with a fixed
email address.

[toc] | [prev] | [next] | [standalone]

#1456442

On Thu, Aug 04, 2016 at 04:03:18PM +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.

Need David's input on this, but I think the force-removal of simpledrm
when other drivers take over is required. Otherwise hilarity can ensue.
If we leave this out for the inital merge then I think we need a really
big warning in Kconfig that if people aren't careful it could result in a
kaboom.

> I have done my best to bring simpledrm up to speed. However I was unable to
> loose drm_legacy_mmap() in sdrm_drm_mmap() since I don't understand much of
> the gem code.

You can just nuke drm_legacy_mmap and the if check, plus the drm_legacy.h
include. There should be no reason at all for that.

> I was left with some questions after doing this:
> 
> - Is there any reason why simpledrm can't use drm_gem_cma_helper?
>   One obvious difference is that of contiguous memory allocation:
> 
>   sdrm_gem_get_pages():
> 	obj->pages = drm_malloc_ab(num, sizeof(*obj->pages));
> 	for (i = 0; i < num; ++i) {
> 		obj->pages[i] = alloc_page(GFP_KERNEL | __GFP_ZERO);
> 	obj->vmapping = vmap(obj->pages, num, 0, PAGE_KERNEL);
> 
>   drm_gem_cma_create():
>         cma_obj->vaddr = dma_alloc_wc(drm->dev, size, &cma_obj->paddr,
>                                       GFP_KERNEL | __GFP_NOWARN);

cma means contiguous memory allocator, that's pretty much the entire
point. Once you have that there's not much left really. Note that since
sdrm was submitted we gained some helpers for normal (shmem-backed) gem
buffers. I think we could extend those a bit to provide the basics for
dumb gem based buffers and use that in sdrm. Otoh there's probably only
udl which could benefit, so not sure this is worth it much. And mmap
helper, maybe.

> - Could we set this range to the actual width/height of the native framebuffer?
> 
>   sdrm_drm_modeset_init():
> 	ddev->mode_config.min_width = 1;
> 	ddev->mode_config.min_height = 1;
> 	ddev->mode_config.max_width = 8192;
> 	ddev->mode_config.max_height = 8192;

Probably a good idea if we don't support modeset changes with simpledrm.

> - Is there a usecase for offsetting the dumb buffer when blitted onto the
>   native buffer?
> 
>   sdrm_blit():
> 	/* get scanout offsets */
> 	xoff = 0;
> 	yoff = 0;
> 	if (sdrm->pipe.plane.fb == fb) {
> 		xoff = sdrm->pipe.crtc.x;
> 		yoff = sdrm->pipe.crtc.y;
> 	}

I don't think allowing the plane to be positioned is a good idea. And
since it's using the simple pipe helpers now that should be even
impossible.

> I have tested simpledrm on a Raspberry Pi B+ with U-boot setting up the
> framebuffer and producing this node:
> 
>         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).

If you can, booting this on a vesa/uefi platform would be interesting too,
just to make sure.
-Daniel


> 
> 
> Noralf.
> 
> 
> 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 (2):
>   drm: add SimpleDRM driver
>   drm: simpledrm: add fbdev fallback support
> 
>  drivers/gpu/drm/Kconfig                      |   2 +
>  drivers/gpu/drm/Makefile                     |   1 +
>  drivers/gpu/drm/simpledrm/Kconfig            |  22 ++
>  drivers/gpu/drm/simpledrm/Makefile           |   5 +
>  drivers/gpu/drm/simpledrm/simpledrm.h        | 111 ++++++++++
>  drivers/gpu/drm/simpledrm/simpledrm_damage.c | 314 +++++++++++++++++++++++++++
>  drivers/gpu/drm/simpledrm/simpledrm_drv.c    | 298 +++++++++++++++++++++++++
>  drivers/gpu/drm/simpledrm/simpledrm_fbdev.c  | 160 ++++++++++++++
>  drivers/gpu/drm/simpledrm/simpledrm_gem.c    | 276 +++++++++++++++++++++++
>  drivers/gpu/drm/simpledrm/simpledrm_kms.c    | 288 ++++++++++++++++++++++++
>  10 files changed, 1477 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]

#1456603

Den 04.08.2016 16:36, skrev Daniel Vetter:
> On Thu, Aug 04, 2016 at 04:03:18PM +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.
> Need David's input on this, but I think the force-removal of simpledrm
> when other drivers take over is required. Otherwise hilarity can ensue.
> If we leave this out for the inital merge then I think we need a really
> big warning in Kconfig that if people aren't careful it could result in a
> kaboom.

I understand. I anticipated this, but I was hoping that if I moved this 
part forward,
which I was capable of doing, maybe someone else could do the rest :-)
Anyways, if no one chimes in to do this, lets add a warning.

> If you can, booting this on a vesa/uefi platform would be interesting too,
> just to make sure.

I'm a bit limited here I'm afraid. I only have a Raspberry Pi to test with.
My other Linux installations are running on a VMware ESXi server.


Noralf.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web