Groups > linux.kernel > #1498946 > unrolled thread

[RFC PATCH 00/11] Introduce writeback connectors

Started by	Brian Starkey <brian.starkey@arm.com>
First post	2016-10-11 17:00 +0200
Last post	2016-10-12 09:40 +0200
Articles	20 on this page of 24 — 4 participants

Back to article view | Back to linux.kernel

  [RFC PATCH 00/11] Introduce writeback connectors Brian Starkey <brian.starkey@arm.com> - 2016-10-11 17:00 +0200
    [RFC PATCH 10/11] drm: mali-dp: Add support for writeback on DP550/DP650 Brian Starkey <brian.starkey@arm.com> - 2016-10-11 17:00 +0200
    [RFC PATCH 09/11] drm: mali-dp: Add RGB writeback formats for DP550/DP650 Brian Starkey <brian.starkey@arm.com> - 2016-10-11 17:00 +0200
    [RFC PATCH 02/11] drm/fb-helper: Skip writeback connectors Brian Starkey <brian.starkey@arm.com> - 2016-10-11 17:00 +0200
      Re: [RFC PATCH 02/11] drm/fb-helper: Skip writeback connectors Daniel Vetter <daniel@ffwll.ch> - 2016-10-11 17:50 +0200
        Re: [RFC PATCH 02/11] drm/fb-helper: Skip writeback connectors Brian Starkey <brian.starkey@arm.com> - 2016-10-11 18:50 +0200
          Re: [RFC PATCH 02/11] drm/fb-helper: Skip writeback connectors Daniel Vetter <daniel@ffwll.ch> - 2016-10-11 19:00 +0200
    [RFC PATCH 05/11] drm: Add fb to connector state Brian Starkey <brian.starkey@arm.com> - 2016-10-11 17:00 +0200
    [RFC PATCH 01/11] drm: Add writeback connector type Brian Starkey <brian.starkey@arm.com> - 2016-10-11 17:00 +0200
    [RFC PATCH 04/11] drm: Add __drm_framebuffer_remove_atomic Brian Starkey <brian.starkey@arm.com> - 2016-10-11 17:00 +0200
      Re: [RFC PATCH 04/11] drm: Add __drm_framebuffer_remove_atomic Daniel Vetter <daniel@ffwll.ch> - 2016-10-11 18:10 +0200
    [RFC PATCH 08/11] drm: mali-dp: Rename malidp_input_format Brian Starkey <brian.starkey@arm.com> - 2016-10-11 17:00 +0200
    [RFC PATCH 11/11] drm: mali-dp: Add writeback connector Brian Starkey <brian.starkey@arm.com> - 2016-10-11 17:00 +0200
    Re: [RFC PATCH 00/11] Introduce writeback connectors Daniel Vetter <daniel@ffwll.ch> - 2016-10-11 17:50 +0200
      Re: [RFC PATCH 00/11] Introduce writeback connectors Brian Starkey <brian.starkey@arm.com> - 2016-10-11 19:00 +0200
        Re: [RFC PATCH 00/11] Introduce writeback connectors Daniel Vetter <daniel@ffwll.ch> - 2016-10-11 19:10 +0200
          Re: [RFC PATCH 00/11] Introduce writeback connectors Brian Starkey <brian.starkey@arm.com> - 2016-10-11 21:50 +0200
            Re: [RFC PATCH 00/11] Introduce writeback connectors Daniel Vetter <daniel@ffwll.ch> - 2016-10-11 22:10 +0200
              Re: [RFC PATCH 00/11] Introduce writeback connectors Brian Starkey <brian.starkey@arm.com> - 2016-10-11 23:30 +0200
                Re: [RFC PATCH 00/11] Introduce writeback connectors Daniel Vetter <daniel@ffwll.ch> - 2016-10-12 09:00 +0200
    Re: [RFC PATCH 00/11] Introduce writeback connectors Daniel Vetter <daniel@ffwll.ch> - 2016-10-11 18:30 +0200
    Re: [RFC PATCH 00/11] Introduce writeback connectors Ville Syrjälä <ville.syrjala@linux.intel.com> - 2016-10-11 18:30 +0200
    Re: [RFC PATCH 00/11] Introduce writeback connectors Eric Anholt <eric@anholt.net> - 2016-10-11 21:10 +0200
      Re: [RFC PATCH 00/11] Introduce writeback connectors Brian Starkey <brian.starkey@arm.com> - 2016-10-12 09:40 +0200

Page 1 of 2 [1] 2 Next page →

#1498946 — [RFC PATCH 00/11] Introduce writeback connectors

From	Brian Starkey <brian.starkey@arm.com>
Date	2016-10-11 17:00 +0200
Subject	[RFC PATCH 00/11] Introduce writeback connectors
Message-ID	<sr6H7-81a-3@gated-at.bofh.it>

Hi,

This RFC series introduces a new connector type:
 DRM_MODE_CONNECTOR_WRITEBACK
It is a follow-on from a previous discussion: [1]

Writeback connectors are used to expose the memory writeback engines
found in some display controllers, which can write a CRTC's
composition result to a memory buffer.
This is useful e.g. for testing, screen-recording, screenshots,
wireless display, display cloning, memory-to-memory composition.

Patches 1-7 include the core framework changes required, and patches
8-11 implement a writeback connector for the Mali-DP writeback engine.
The Mali-DP patches depend on this other series: [2].

The connector is given the FB_ID property for the output framebuffer,
and two new read-only properties: PIXEL_FORMATS and
PIXEL_FORMATS_SIZE, which expose the supported framebuffer pixel
formats of the engine.

The EDID property is not exposed for writeback connectors.

Writeback connector usage:
--------------------------
Due to connector routing changes being treated as "full modeset"
operations, any client which wishes to use a writeback connector
should include the connector in every modeset. The writeback will not
actually become active until a framebuffer is attached.

The writeback itself is enabled by attaching a framebuffer to the
FB_ID property of the connector. The driver must then ensure that the
CRTC content of that atomic commit is written into the framebuffer.

The writeback works in a one-shot mode with each atomic commit. This
prevents the same content from being written multiple times.
In some cases (front-buffer rendering) there might be a desire for
continuous operation - I think a property could be added later for
this kind of control.

Writeback can be disabled by setting FB_ID to zero.

Known issues:
-------------
 * I'm not sure what "DPMS" should mean for writeback connectors.
   It could be used to disable writeback (even when a framebuffer is
   attached), or it could be hidden entirely (which would break the
   legacy DPMS call for writeback connectors).
 * With Daniel's recent re-iteration of the userspace API rules, I
   fully expect to provide some userspace code to support this. The
   question is what, and where? We want to use writeback for testing,
   so perhaps some tests in igt is suitable.
 * Documentation. Probably some portion of this cover letter needs to
   make it into Documentation/
 * Synchronisation. Our hardware will finish the writeback by the next
   vsync. I've not implemented fence support here, but it would be an
   obvious addition.

See Also:
---------
[1] https://lists.freedesktop.org/archives/dri-devel/2016-July/113197.html
[2] https://lists.freedesktop.org/archives/dri-devel/2016-October/120486.html

I welcome any comments, especially if this approach does/doesn't fit
well with anyone else's hardware.

Thanks,

-Brian

---

Brian Starkey (10):
  drm: add writeback connector type
  drm/fb-helper: skip writeback connectors
  drm: extract CRTC/plane disable from drm_framebuffer_remove
  drm: add __drm_framebuffer_remove_atomic
  drm: add fb to connector state
  drm: expose fb_id property for writeback connectors
  drm: add writeback-connector pixel format properties
  drm: mali-dp: rename malidp_input_format
  drm: mali-dp: add RGB writeback formats for DP550/DP650
  drm: mali-dp: add writeback connector

Liviu Dudau (1):
  drm: mali-dp: Add support for writeback on DP550/DP650

 drivers/gpu/drm/arm/Makefile        |    1 +
 drivers/gpu/drm/arm/malidp_crtc.c   |   10 ++
 drivers/gpu/drm/arm/malidp_drv.c    |   25 +++-
 drivers/gpu/drm/arm/malidp_drv.h    |    5 +
 drivers/gpu/drm/arm/malidp_hw.c     |  104 ++++++++++----
 drivers/gpu/drm/arm/malidp_hw.h     |   27 +++-
 drivers/gpu/drm/arm/malidp_mw.c     |  268 +++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/arm/malidp_planes.c |    8 +-
 drivers/gpu/drm/arm/malidp_regs.h   |   15 ++
 drivers/gpu/drm/drm_atomic.c        |   40 ++++++
 drivers/gpu/drm/drm_atomic_helper.c |    4 +
 drivers/gpu/drm/drm_connector.c     |   79 ++++++++++-
 drivers/gpu/drm/drm_crtc.c          |   14 +-
 drivers/gpu/drm/drm_fb_helper.c     |    4 +
 drivers/gpu/drm/drm_framebuffer.c   |  249 ++++++++++++++++++++++++++++----
 drivers/gpu/drm/drm_ioctl.c         |    7 +
 include/drm/drmP.h                  |    2 +
 include/drm/drm_atomic.h            |    3 +
 include/drm/drm_connector.h         |   15 ++
 include/drm/drm_crtc.h              |   12 ++
 include/uapi/drm/drm.h              |   10 ++
 include/uapi/drm/drm_mode.h         |    1 +
 22 files changed, 830 insertions(+), 73 deletions(-)
 create mode 100644 drivers/gpu/drm/arm/malidp_mw.c

-- 
1.7.9.5

[toc] | [next] | [standalone]

#1498947 — [RFC PATCH 10/11] drm: mali-dp: Add support for writeback on DP550/DP650

From	Brian Starkey <brian.starkey@arm.com>
Date	2016-10-11 17:00 +0200
Subject	[RFC PATCH 10/11] drm: mali-dp: Add support for writeback on DP550/DP650
Message-ID	<sr6H8-81a-19@gated-at.bofh.it>
In reply to	#1498946

From: Liviu Dudau <Liviu.Dudau@arm.com>

Mali-DP display processors are able to write the composition result to a
memory buffer via the SE.

Add entry points in the HAL for enabling/disabling this feature, and
implement support for it on DP650 and DP550. DP500 acts differently and
so is omitted from this change.

Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
Signed-off-by: Brian Starkey <brian.starkey@arm.com>
---
 drivers/gpu/drm/arm/malidp_hw.c   |   52 +++++++++++++++++++++++++++++++++++--
 drivers/gpu/drm/arm/malidp_hw.h   |   18 +++++++++++++
 drivers/gpu/drm/arm/malidp_regs.h |   15 +++++++++++
 3 files changed, 83 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/arm/malidp_hw.c b/drivers/gpu/drm/arm/malidp_hw.c
index 5235d0b..dee7605 100644
--- a/drivers/gpu/drm/arm/malidp_hw.c
+++ b/drivers/gpu/drm/arm/malidp_hw.c
@@ -387,6 +387,48 @@ static int malidp550_rotmem_required(struct malidp_hw_device *hwdev, u16 w, u16
 	return w * bytes_per_col;
 }
 
+static int malidp550_enable_memwrite(struct malidp_hw_device *hwdev,
+				     dma_addr_t *addrs, s32 *pitches,
+				     int num_planes, u16 w, u16 h, u32 fmt_id)
+{
+	u32 base = MALIDP550_SE_MEMWRITE_BASE;
+	u32 de_base = malidp_get_block_base(hwdev, MALIDP_DE_BLOCK);
+
+	/* enable the scaling engine block */
+	malidp_hw_setbits(hwdev, MALIDP_SCALE_ENGINE_EN, de_base + MALIDP_DE_DISPLAY_FUNC);
+
+	malidp_hw_write(hwdev, fmt_id, base + MALIDP_MW_FORMAT);
+	switch (num_planes) {
+	case 2:
+		malidp_hw_write(hwdev, lower_32_bits(addrs[1]), base + MALIDP_MW_P2_PTR_LOW);
+		malidp_hw_write(hwdev, upper_32_bits(addrs[1]), base + MALIDP_MW_P2_PTR_HIGH);
+		malidp_hw_write(hwdev, pitches[1], base + MALIDP_MW_P2_STRIDE);
+		/* fall through */
+	case 1:
+		malidp_hw_write(hwdev, lower_32_bits(addrs[0]), base + MALIDP_MW_P1_PTR_LOW);
+		malidp_hw_write(hwdev, upper_32_bits(addrs[0]), base + MALIDP_MW_P1_PTR_HIGH);
+		malidp_hw_write(hwdev, pitches[0], base + MALIDP_MW_P1_STRIDE);
+		break;
+	default:
+		WARN(1, "Invalid number of planes");
+	}
+
+	malidp_hw_write(hwdev, MALIDP_DE_H_ACTIVE(w) | MALIDP_DE_V_ACTIVE(h),
+			MALIDP550_SE_MEMWRITE_OUT_SIZE);
+	malidp_hw_setbits(hwdev, MALIDP550_SE_MEMWRITE_ONESHOT | MALIDP_SE_MEMWRITE_EN,
+			  MALIDP550_SE_CONTROL);
+
+	return 0;
+}
+
+static void malidp550_disable_memwrite(struct malidp_hw_device *hwdev)
+{
+	u32 base = malidp_get_block_base(hwdev, MALIDP_DE_BLOCK);
+	malidp_hw_clearbits(hwdev, MALIDP550_SE_MEMWRITE_ONESHOT | MALIDP_SE_MEMWRITE_EN,
+			    MALIDP550_SE_CONTROL);
+	malidp_hw_clearbits(hwdev, MALIDP_SCALE_ENGINE_EN, base + MALIDP_DE_DISPLAY_FUNC);
+}
+
 static int malidp650_query_hw(struct malidp_hw_device *hwdev)
 {
 	u32 conf = malidp_hw_read(hwdev, MALIDP550_CONFIG_ID);
@@ -469,7 +511,8 @@ const struct malidp_hw_device malidp_device[MALIDP_MAX_DEVICES] = {
 					    MALIDP550_SE_IRQ_AXI_ERR,
 			},
 			.dc_irq_map = {
-				.irq_mask = MALIDP550_DC_IRQ_CONF_VALID,
+				.irq_mask = MALIDP550_DC_IRQ_CONF_VALID |
+					    MALIDP550_DC_IRQ_SE,
 				.vsync_irq = MALIDP550_DC_IRQ_CONF_VALID,
 			},
 			.pixel_formats = malidp550_de_formats,
@@ -483,6 +526,8 @@ const struct malidp_hw_device malidp_device[MALIDP_MAX_DEVICES] = {
 		.set_config_valid = malidp550_set_config_valid,
 		.modeset = malidp550_modeset,
 		.rotmem_required = malidp550_rotmem_required,
+		.enable_memwrite = malidp550_enable_memwrite,
+		.disable_memwrite = malidp550_disable_memwrite,
 	},
 	[MALIDP_650] = {
 		.map = {
@@ -503,7 +548,8 @@ const struct malidp_hw_device malidp_device[MALIDP_MAX_DEVICES] = {
 					    MALIDP550_SE_IRQ_AXI_ERR,
 			},
 			.dc_irq_map = {
-				.irq_mask = MALIDP550_DC_IRQ_CONF_VALID,
+				.irq_mask = MALIDP550_DC_IRQ_CONF_VALID |
+					    MALIDP550_DC_IRQ_SE,
 				.vsync_irq = MALIDP550_DC_IRQ_CONF_VALID,
 			},
 			.pixel_formats = malidp550_de_formats,
@@ -517,6 +563,8 @@ const struct malidp_hw_device malidp_device[MALIDP_MAX_DEVICES] = {
 		.set_config_valid = malidp550_set_config_valid,
 		.modeset = malidp550_modeset,
 		.rotmem_required = malidp550_rotmem_required,
+		.enable_memwrite = malidp550_enable_memwrite,
+		.disable_memwrite = malidp550_disable_memwrite,
 	},
 };
 
diff --git a/drivers/gpu/drm/arm/malidp_hw.h b/drivers/gpu/drm/arm/malidp_hw.h
index ce4ea55..8056efa 100644
--- a/drivers/gpu/drm/arm/malidp_hw.h
+++ b/drivers/gpu/drm/arm/malidp_hw.h
@@ -147,6 +147,24 @@ struct malidp_hw_device {
 	 */
 	int (*rotmem_required)(struct malidp_hw_device *hwdev, u16 w, u16 h, u32 fmt);
 
+	/**
+	 * Enable writing to memory the content of the next frame
+	 * @param hwdev - malidp_hw_device structure containing the HW description
+	 * @param addrs - array of addresses for each plane
+	 * @param pitches - array of pitches for each plane
+	 * @param num_planes - number of planes to be written
+	 * @param w - width of the output frame
+	 * @param h - height of the output frame
+	 * @param fmt_id - internal format ID of output buffer
+	 */
+	int (*enable_memwrite)(struct malidp_hw_device *hwdev, dma_addr_t *addrs,
+			       s32 *pitches, int num_planes, u16 w, u16 h, u32 fmt_id);
+
+	/*
+	 * Disable the writing to memory of the next frame's content.
+	 */
+	void (*disable_memwrite)(struct malidp_hw_device *hwdev);
+
 	u8 features;
 
 	u8 min_line_size;
diff --git a/drivers/gpu/drm/arm/malidp_regs.h b/drivers/gpu/drm/arm/malidp_regs.h
index 73fecb3..cab086c 100644
--- a/drivers/gpu/drm/arm/malidp_regs.h
+++ b/drivers/gpu/drm/arm/malidp_regs.h
@@ -64,6 +64,8 @@
 /* bit masks that are common between products */
 #define   MALIDP_CFG_VALID		(1 << 0)
 #define   MALIDP_DISP_FUNC_ILACED	(1 << 8)
+#define   MALIDP_SCALE_ENGINE_EN	(1 << 16)
+#define   MALIDP_SE_MEMWRITE_EN		(2 << 5)
 
 /* register offsets for IRQ management */
 #define MALIDP_REG_STATUS		0x00000
@@ -92,6 +94,15 @@
 #define MALIDP_DE_H_ACTIVE(x)		(((x) & 0x1fff) << 0)
 #define MALIDP_DE_V_ACTIVE(x)		(((x) & 0x1fff) << 16)
 
+/* register offsets relative to MALIDP5x0_SE_MEMWRITE_BASE */
+#define MALIDP_MW_FORMAT		0x00000
+#define MALIDP_MW_P1_STRIDE		0x00004
+#define MALIDP_MW_P2_STRIDE		0x00008
+#define MALIDP_MW_P1_PTR_LOW		0x0000c
+#define MALIDP_MW_P1_PTR_HIGH		0x00010
+#define MALIDP_MW_P2_PTR_LOW		0x0002c
+#define MALIDP_MW_P2_PTR_HIGH		0x00030
+
 /* register offsets and bits specific to DP500 */
 #define MALIDP500_DC_BASE		0x00000
 #define MALIDP500_DC_CONTROL		0x0000c
@@ -149,6 +160,10 @@
 #define MALIDP550_DE_LS_PTR_BASE	0x0042c
 #define MALIDP550_DE_PERF_BASE		0x00500
 #define MALIDP550_SE_BASE		0x08000
+#define MALIDP550_SE_CONTROL		0x08010
+#define   MALIDP550_SE_MEMWRITE_ONESHOT	(1 << 7)
+#define MALIDP550_SE_MEMWRITE_OUT_SIZE	0x08030
+#define MALIDP550_SE_MEMWRITE_BASE	0x08100
 #define MALIDP550_DC_BASE		0x0c000
 #define MALIDP550_DC_CONTROL		0x0c010
 #define   MALIDP550_DC_CONFIG_REQ	(1 << 16)
-- 
1.7.9.5

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

#1498950 — [RFC PATCH 09/11] drm: mali-dp: Add RGB writeback formats for DP550/DP650

From	Brian Starkey <brian.starkey@arm.com>
Date	2016-10-11 17:00 +0200
Subject	[RFC PATCH 09/11] drm: mali-dp: Add RGB writeback formats for DP550/DP650
Message-ID	<sr6H8-81a-25@gated-at.bofh.it>
In reply to	#1498946

Add a layer bit for the SE memory-write, and add it to the pixel format
matrix for DP550/DP650.

Signed-off-by: Brian Starkey <brian.starkey@arm.com>
---
 drivers/gpu/drm/arm/malidp_hw.c |   28 ++++++++++++++--------------
 drivers/gpu/drm/arm/malidp_hw.h |    1 +
 2 files changed, 15 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/arm/malidp_hw.c b/drivers/gpu/drm/arm/malidp_hw.c
index 44a9d10..5235d0b 100644
--- a/drivers/gpu/drm/arm/malidp_hw.c
+++ b/drivers/gpu/drm/arm/malidp_hw.c
@@ -46,20 +46,20 @@ static const struct malidp_format_id malidp500_de_formats[] = {
 
 #define MALIDP_COMMON_FORMATS \
 	/*    fourcc,   layers supporting the format,      internal id   */ \
-	{ DRM_FORMAT_ARGB2101010, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2, MALIDP_ID(0, 0) }, \
-	{ DRM_FORMAT_ABGR2101010, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2, MALIDP_ID(0, 1) }, \
-	{ DRM_FORMAT_RGBA1010102, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2, MALIDP_ID(0, 2) }, \
-	{ DRM_FORMAT_BGRA1010102, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2, MALIDP_ID(0, 3) }, \
-	{ DRM_FORMAT_ARGB8888, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2 | DE_SMART, MALIDP_ID(1, 0) }, \
-	{ DRM_FORMAT_ABGR8888, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2 | DE_SMART, MALIDP_ID(1, 1) }, \
-	{ DRM_FORMAT_RGBA8888, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2 | DE_SMART, MALIDP_ID(1, 2) }, \
-	{ DRM_FORMAT_BGRA8888, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2 | DE_SMART, MALIDP_ID(1, 3) }, \
-	{ DRM_FORMAT_XRGB8888, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2 | DE_SMART, MALIDP_ID(2, 0) }, \
-	{ DRM_FORMAT_XBGR8888, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2 | DE_SMART, MALIDP_ID(2, 1) }, \
-	{ DRM_FORMAT_RGBX8888, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2 | DE_SMART, MALIDP_ID(2, 2) }, \
-	{ DRM_FORMAT_BGRX8888, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2 | DE_SMART, MALIDP_ID(2, 3) }, \
-	{ DRM_FORMAT_RGB888, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2, MALIDP_ID(3, 0) }, \
-	{ DRM_FORMAT_BGR888, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2, MALIDP_ID(3, 1) }, \
+	{ DRM_FORMAT_ARGB2101010, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2 | SE_MEMWRITE, MALIDP_ID(0, 0) }, \
+	{ DRM_FORMAT_ABGR2101010, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2 | SE_MEMWRITE, MALIDP_ID(0, 1) }, \
+	{ DRM_FORMAT_RGBA1010102, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2 | SE_MEMWRITE, MALIDP_ID(0, 2) }, \
+	{ DRM_FORMAT_BGRA1010102, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2 | SE_MEMWRITE, MALIDP_ID(0, 3) }, \
+	{ DRM_FORMAT_ARGB8888, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2 | DE_SMART | SE_MEMWRITE, MALIDP_ID(1, 0) }, \
+	{ DRM_FORMAT_ABGR8888, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2 | DE_SMART | SE_MEMWRITE, MALIDP_ID(1, 1) }, \
+	{ DRM_FORMAT_RGBA8888, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2 | DE_SMART | SE_MEMWRITE, MALIDP_ID(1, 2) }, \
+	{ DRM_FORMAT_BGRA8888, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2 | DE_SMART | SE_MEMWRITE, MALIDP_ID(1, 3) }, \
+	{ DRM_FORMAT_XRGB8888, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2 | DE_SMART | SE_MEMWRITE, MALIDP_ID(2, 0) }, \
+	{ DRM_FORMAT_XBGR8888, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2 | DE_SMART | SE_MEMWRITE, MALIDP_ID(2, 1) }, \
+	{ DRM_FORMAT_RGBX8888, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2 | DE_SMART | SE_MEMWRITE, MALIDP_ID(2, 2) }, \
+	{ DRM_FORMAT_BGRX8888, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2 | DE_SMART | SE_MEMWRITE, MALIDP_ID(2, 3) }, \
+	{ DRM_FORMAT_RGB888, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2 | SE_MEMWRITE, MALIDP_ID(3, 0) }, \
+	{ DRM_FORMAT_BGR888, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2 | SE_MEMWRITE, MALIDP_ID(3, 1) }, \
 	{ DRM_FORMAT_RGBA5551, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2, MALIDP_ID(4, 0) }, \
 	{ DRM_FORMAT_ABGR1555, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2, MALIDP_ID(4, 1) }, \
 	{ DRM_FORMAT_RGB565, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2, MALIDP_ID(4, 2) }, \
diff --git a/drivers/gpu/drm/arm/malidp_hw.h b/drivers/gpu/drm/arm/malidp_hw.h
index 4f8c884..ce4ea55 100644
--- a/drivers/gpu/drm/arm/malidp_hw.h
+++ b/drivers/gpu/drm/arm/malidp_hw.h
@@ -33,6 +33,7 @@ enum {
 	DE_GRAPHICS2 = BIT(2), /* used only in DP500 */
 	DE_VIDEO2 = BIT(3),
 	DE_SMART = BIT(4),
+	SE_MEMWRITE = BIT(5),
 };
 
 struct malidp_format_id {
-- 
1.7.9.5

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

#1498951 — [RFC PATCH 02/11] drm/fb-helper: Skip writeback connectors

From	Brian Starkey <brian.starkey@arm.com>
Date	2016-10-11 17:00 +0200
Subject	[RFC PATCH 02/11] drm/fb-helper: Skip writeback connectors
Message-ID	<sr6H8-81a-33@gated-at.bofh.it>
In reply to	#1498946

Writeback connectors aren't much use to the fbdev helpers, as they won't
show anything to the user. Skip them when looking for candidate output
configurations.

Signed-off-by: Brian Starkey <brian.starkey@arm.com>
---
 drivers/gpu/drm/drm_fb_helper.c |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 03414bd..dedf6e7 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -2016,6 +2016,10 @@ static int drm_pick_crtcs(struct drm_fb_helper *fb_helper,
 	if (modes[n] == NULL)
 		return best_score;
 
+	/* Writeback connectors aren't much use for fbdev */
+	if (connector->connector_type == DRM_MODE_CONNECTOR_WRITEBACK)
+		return best_score;
+
 	crtcs = kzalloc(fb_helper->connector_count *
 			sizeof(struct drm_fb_helper_crtc *), GFP_KERNEL);
 	if (!crtcs)
-- 
1.7.9.5

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

#1498990 — Re: [RFC PATCH 02/11] drm/fb-helper: Skip writeback connectors

From	Daniel Vetter <daniel@ffwll.ch>
Date	2016-10-11 17:50 +0200
Subject	Re: [RFC PATCH 02/11] drm/fb-helper: Skip writeback connectors
Message-ID	<sr7tx-4W-45@gated-at.bofh.it>
In reply to	#1498951

On Tue, Oct 11, 2016 at 03:53:59PM +0100, Brian Starkey wrote:
> Writeback connectors aren't much use to the fbdev helpers, as they won't
> show anything to the user. Skip them when looking for candidate output
> configurations.
> 
> Signed-off-by: Brian Starkey <brian.starkey@arm.com>
> ---
>  drivers/gpu/drm/drm_fb_helper.c |    4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 03414bd..dedf6e7 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -2016,6 +2016,10 @@ static int drm_pick_crtcs(struct drm_fb_helper *fb_helper,
>  	if (modes[n] == NULL)
>  		return best_score;
>  
> +	/* Writeback connectors aren't much use for fbdev */
> +	if (connector->connector_type == DRM_MODE_CONNECTOR_WRITEBACK)
> +		return best_score;

I think we could handle this by always marking writeback connectors as
disconnected. Userspace and fbdev emulation should then avoid them,
always.
-Daniel

> +
>  	crtcs = kzalloc(fb_helper->connector_count *
>  			sizeof(struct drm_fb_helper_crtc *), GFP_KERNEL);
>  	if (!crtcs)
> -- 
> 1.7.9.5
> 

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

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

#1499011 — Re: [RFC PATCH 02/11] drm/fb-helper: Skip writeback connectors

From	Brian Starkey <brian.starkey@arm.com>
Date	2016-10-11 18:50 +0200
Subject	Re: [RFC PATCH 02/11] drm/fb-helper: Skip writeback connectors
Message-ID	<sr8pA-DS-9@gated-at.bofh.it>
In reply to	#1498990

On Tue, Oct 11, 2016 at 05:44:48PM +0200, Daniel Vetter wrote:
>On Tue, Oct 11, 2016 at 03:53:59PM +0100, Brian Starkey wrote:
>> Writeback connectors aren't much use to the fbdev helpers, as they won't
>> show anything to the user. Skip them when looking for candidate output
>> configurations.
>>
>> Signed-off-by: Brian Starkey <brian.starkey@arm.com>
>> ---
>>  drivers/gpu/drm/drm_fb_helper.c |    4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
>> index 03414bd..dedf6e7 100644
>> --- a/drivers/gpu/drm/drm_fb_helper.c
>> +++ b/drivers/gpu/drm/drm_fb_helper.c
>> @@ -2016,6 +2016,10 @@ static int drm_pick_crtcs(struct drm_fb_helper *fb_helper,
>>  	if (modes[n] == NULL)
>>  		return best_score;
>>
>> +	/* Writeback connectors aren't much use for fbdev */
>> +	if (connector->connector_type == DRM_MODE_CONNECTOR_WRITEBACK)
>> +		return best_score;
>
>I think we could handle this by always marking writeback connectors as
>disconnected. Userspace and fbdev emulation should then avoid them,
>always.
>-Daniel
>

Good idea; I'll need to take a closer look at how it would interact
with the probe helper (connector->force etc).

Are you thinking instead-of or in-addition-to the client cap? I'd be
worried about apps doing strange things and trying to use even
disconnected connectors.

>> +
>>  	crtcs = kzalloc(fb_helper->connector_count *
>>  			sizeof(struct drm_fb_helper_crtc *), GFP_KERNEL);
>>  	if (!crtcs)
>> --
>> 1.7.9.5
>>
>
>-- 
>Daniel Vetter
>Software Engineer, Intel Corporation
>http://blog.ffwll.ch
>

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

#1499016 — Re: [RFC PATCH 02/11] drm/fb-helper: Skip writeback connectors

From	Daniel Vetter <daniel@ffwll.ch>
Date	2016-10-11 19:00 +0200
Subject	Re: [RFC PATCH 02/11] drm/fb-helper: Skip writeback connectors
Message-ID	<sr8zf-H0-15@gated-at.bofh.it>
In reply to	#1499011

On Tue, Oct 11, 2016 at 6:47 PM, Brian Starkey <brian.starkey@arm.com> wrote:
> On Tue, Oct 11, 2016 at 05:44:48PM +0200, Daniel Vetter wrote:
>>
>> On Tue, Oct 11, 2016 at 03:53:59PM +0100, Brian Starkey wrote:
>>>
>>> Writeback connectors aren't much use to the fbdev helpers, as they won't
>>> show anything to the user. Skip them when looking for candidate output
>>> configurations.
>>>
>>> Signed-off-by: Brian Starkey <brian.starkey@arm.com>
>>> ---
>>>  drivers/gpu/drm/drm_fb_helper.c |    4 ++++
>>>  1 file changed, 4 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/drm_fb_helper.c
>>> b/drivers/gpu/drm/drm_fb_helper.c
>>> index 03414bd..dedf6e7 100644
>>> --- a/drivers/gpu/drm/drm_fb_helper.c
>>> +++ b/drivers/gpu/drm/drm_fb_helper.c
>>> @@ -2016,6 +2016,10 @@ static int drm_pick_crtcs(struct drm_fb_helper
>>> *fb_helper,
>>>         if (modes[n] == NULL)
>>>                 return best_score;
>>>
>>> +       /* Writeback connectors aren't much use for fbdev */
>>> +       if (connector->connector_type == DRM_MODE_CONNECTOR_WRITEBACK)
>>> +               return best_score;
>>
>>
>> I think we could handle this by always marking writeback connectors as
>> disconnected. Userspace and fbdev emulation should then avoid them,
>> always.
>> -Daniel
>>
>
> Good idea; I'll need to take a closer look at how it would interact
> with the probe helper (connector->force etc).
>
> Are you thinking instead-of or in-addition-to the client cap? I'd be
> worried about apps doing strange things and trying to use even
> disconnected connectors.

Apps shouldn't try to use disconnected connectors, at least by
default. I think we wouldn't need the cap in that case.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

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

#1498952 — [RFC PATCH 05/11] drm: Add fb to connector state

From	Brian Starkey <brian.starkey@arm.com>
Date	2016-10-11 17:00 +0200
Subject	[RFC PATCH 05/11] drm: Add fb to connector state
Message-ID	<sr6H8-81a-35@gated-at.bofh.it>
In reply to	#1498946

Add a framebuffer to the connector state, for use as the output target
by writeback connectors.

If a framebuffer is in use by a writeback connector when userspace
removes it, it is handled by removing the framebuffer from the connector.

Signed-off-by: Brian Starkey <brian.starkey@arm.com>
---
 drivers/gpu/drm/drm_atomic.c        |   31 +++++++++++++++++++++++++++++++
 drivers/gpu/drm/drm_atomic_helper.c |    4 ++++
 drivers/gpu/drm/drm_framebuffer.c   |   24 ++++++++++++++++++++----
 include/drm/drm_atomic.h            |    3 +++
 include/drm/drm_connector.h         |    3 +++
 5 files changed, 61 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 2373960..b16b4fc 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -1205,6 +1205,37 @@ drm_atomic_set_crtc_for_connector(struct drm_connector_state *conn_state,
 EXPORT_SYMBOL(drm_atomic_set_crtc_for_connector);
 
 /**
+ * drm_atomic_set_fb_for_connector - set framebuffer for (writeback) connector
+ * @connector_state: atomic state object for the connector
+ * @fb: fb to use for the connector
+ *
+ * This is used to set the framebuffer for a writeback connector, which outputs
+ * to a buffer instead of an actual physical connector.
+ * Changing the assigned framebuffer requires us to grab a reference to the new
+ * fb and drop the reference to the old fb, if there is one. This function
+ * takes care of all these details besides updating the pointer in the
+ * state object itself.
+ */
+void
+drm_atomic_set_fb_for_connector(struct drm_connector_state *conn_state,
+				struct drm_framebuffer *fb)
+{
+	if (conn_state->fb)
+		drm_framebuffer_unreference(conn_state->fb);
+	if (fb)
+		drm_framebuffer_reference(fb);
+	conn_state->fb = fb;
+
+	if (fb)
+		DRM_DEBUG_ATOMIC("Set [FB:%d] for connector state %p\n",
+				 fb->base.id, conn_state);
+	else
+		DRM_DEBUG_ATOMIC("Set [NOFB] for connector state %p\n",
+				 conn_state);
+}
+EXPORT_SYMBOL(drm_atomic_set_fb_for_connector);
+
+/**
  * drm_atomic_add_affected_connectors - add connectors for crtc
  * @state: atomic state
  * @crtc: DRM crtc
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 3eecfc1..78ea735 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -3234,6 +3234,8 @@ __drm_atomic_helper_connector_duplicate_state(struct drm_connector *connector,
 	memcpy(state, connector->state, sizeof(*state));
 	if (state->crtc)
 		drm_connector_reference(connector);
+	if (state->fb)
+		drm_framebuffer_reference(state->fb);
 }
 EXPORT_SYMBOL(__drm_atomic_helper_connector_duplicate_state);
 
@@ -3361,6 +3363,8 @@ __drm_atomic_helper_connector_destroy_state(struct drm_connector_state *state)
 	 */
 	if (state->crtc)
 		drm_connector_unreference(state->connector);
+	if (state->fb)
+		drm_framebuffer_unreference(state->fb);
 }
 EXPORT_SYMBOL(__drm_atomic_helper_connector_destroy_state);
 
diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
index b02cf73..f66908b1 100644
--- a/drivers/gpu/drm/drm_framebuffer.c
+++ b/drivers/gpu/drm/drm_framebuffer.c
@@ -24,6 +24,7 @@
 #include <drm/drmP.h>
 #include <drm/drm_atomic.h>
 #include <drm/drm_auth.h>
+#include <drm/drm_connector.h>
 #include <drm/drm_framebuffer.h>
 
 #include "drm_crtc_internal.h"
@@ -808,6 +809,8 @@ EXPORT_SYMBOL(drm_framebuffer_cleanup);
  * it is removed and the CRTC/plane disabled.
  * The legacy references are dropped and the ->fb pointers set to NULL
  * accordingly.
+ * It also checks for (writeback) connectors which are using @fb, and removes
+ * it if found.
  *
  * Returns:
  * true if the framebuffer was successfully removed from use
@@ -900,7 +903,7 @@ retry:
 		plane_state->src_h = 0;
 	}
 
-	/* All of the connectors in state need disabling */
+	/* All of the connectors currently in state need disabling */
 	for_each_connector_in_state(state, connector, conn_state, i) {
 		ret = drm_atomic_set_crtc_for_connector(conn_state,
 							NULL);
@@ -908,10 +911,23 @@ retry:
 			goto fail;
 	}
 
-	if (WARN_ON(!plane_mask)) {
-		DRM_ERROR("Couldn't find any usage of [FB:%d]\n", fb->base.id);
-		ret = -ENOENT;
+	/* Now find any writeback connectors that need handling */
+	ret = drm_modeset_lock(&state->dev->mode_config.connection_mutex,
+			       state->acquire_ctx);
+	if (ret)
 		goto fail;
+
+	drm_for_each_connector(connector, dev) {
+		conn_state = drm_atomic_get_connector_state(state, connector);
+		if (IS_ERR(conn_state)) {
+			ret = PTR_ERR(conn_state);
+			goto fail;
+		}
+
+		if (conn_state->fb != fb)
+			continue;
+
+		drm_atomic_set_fb_for_connector(conn_state, NULL);
 	}
 
 	ret = drm_atomic_commit(state);
diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
index 9701f2d..d9aff06 100644
--- a/include/drm/drm_atomic.h
+++ b/include/drm/drm_atomic.h
@@ -319,6 +319,9 @@ void drm_atomic_set_fb_for_plane(struct drm_plane_state *plane_state,
 int __must_check
 drm_atomic_set_crtc_for_connector(struct drm_connector_state *conn_state,
 				  struct drm_crtc *crtc);
+void
+drm_atomic_set_fb_for_connector(struct drm_connector_state *conn_state,
+				struct drm_framebuffer *fb);
 int __must_check
 drm_atomic_add_affected_connectors(struct drm_atomic_state *state,
 				   struct drm_crtc *crtc);
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 287a610..30a766a 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -198,6 +198,7 @@ int drm_display_info_set_bus_formats(struct drm_display_info *info,
  * @connector: backpointer to the connector
  * @best_encoder: can be used by helpers and drivers to select the encoder
  * @state: backpointer to global drm_atomic_state
+ * @fb: Writeback framebuffer, for DRM_MODE_CONNECTOR_WRITEBACK
  */
 struct drm_connector_state {
 	struct drm_connector *connector;
@@ -213,6 +214,8 @@ struct drm_connector_state {
 	struct drm_encoder *best_encoder;
 
 	struct drm_atomic_state *state;
+
+	struct drm_framebuffer *fb;  /* do not write directly, use drm_atomic_set_fb_for_connector() */
 };
 
 /**
-- 
1.7.9.5

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

#1498953 — [RFC PATCH 01/11] drm: Add writeback connector type

From	Brian Starkey <brian.starkey@arm.com>
Date	2016-10-11 17:00 +0200
Subject	[RFC PATCH 01/11] drm: Add writeback connector type
Message-ID	<sr6H8-81a-37@gated-at.bofh.it>
In reply to	#1498946

Writeback connectors represent writeback engines which can write the
CRTC output to a memory framebuffer.

Add a writeback connector type, hidden from userspace behind a client
cap. They are hidden from non-aware clients so that they do not attempt
to use writeback connectors to provide visual output to the user.

Signed-off-by: Brian Starkey <brian.starkey@arm.com>
---
 drivers/gpu/drm/drm_connector.c |    4 +++-
 drivers/gpu/drm/drm_crtc.c      |   14 +++++++++++++-
 drivers/gpu/drm/drm_ioctl.c     |    7 +++++++
 include/drm/drmP.h              |    2 ++
 include/uapi/drm/drm.h          |   10 ++++++++++
 include/uapi/drm/drm_mode.h     |    1 +
 6 files changed, 36 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index 26bb78c7..027d7a9 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -86,6 +86,7 @@ static struct drm_conn_prop_enum_list drm_connector_enum_list[] = {
 	{ DRM_MODE_CONNECTOR_VIRTUAL, "Virtual" },
 	{ DRM_MODE_CONNECTOR_DSI, "DSI" },
 	{ DRM_MODE_CONNECTOR_DPI, "DPI" },
+	{ DRM_MODE_CONNECTOR_WRITEBACK, "Writeback" },
 };
 
 void drm_connector_ida_init(void)
@@ -235,7 +236,8 @@ int drm_connector_init(struct drm_device *dev,
 	list_add_tail(&connector->head, &config->connector_list);
 	config->num_connector++;
 
-	if (connector_type != DRM_MODE_CONNECTOR_VIRTUAL)
+	if ((connector_type != DRM_MODE_CONNECTOR_VIRTUAL) &&
+	    (connector_type != DRM_MODE_CONNECTOR_WRITEBACK))
 		drm_object_attach_property(&connector->base,
 					      config->edid_property,
 					      0);
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 2d7bedf..33f66e2 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -422,6 +422,14 @@ static int drm_mode_create_standard_properties(struct drm_device *dev)
 	return 0;
 }
 
+static bool
+drm_connector_expose_to_userspace(const struct drm_connector *conn,
+				  const struct drm_file *file_priv)
+{
+	return (file_priv->writeback_connectors) ||
+	       (conn->connector_type != DRM_MODE_CONNECTOR_WRITEBACK);
+}
+
 /**
  * drm_mode_getresources - get graphics configuration
  * @dev: drm device for the ioctl
@@ -491,7 +499,8 @@ int drm_mode_getresources(struct drm_device *dev, void *data,
 		crtc_count++;
 
 	drm_for_each_connector(connector, dev)
-		connector_count++;
+		if (drm_connector_expose_to_userspace(connector, file_priv))
+			connector_count++;
 
 	drm_for_each_encoder(encoder, dev)
 		encoder_count++;
@@ -535,6 +544,9 @@ int drm_mode_getresources(struct drm_device *dev, void *data,
 		copied = 0;
 		connector_id = (uint32_t __user *)(unsigned long)card_res->connector_id_ptr;
 		drm_for_each_connector(connector, dev) {
+			if (!drm_connector_expose_to_userspace(connector, file_priv))
+				continue;
+
 			if (put_user(connector->base.id,
 				     connector_id + copied)) {
 				ret = -EFAULT;
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index 0ad2c47..838a6e8 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -308,6 +308,13 @@ drm_setclientcap(struct drm_device *dev, void *data, struct drm_file *file_priv)
 		file_priv->atomic = req->value;
 		file_priv->universal_planes = req->value;
 		break;
+	case DRM_CLIENT_CAP_WRITEBACK_CONNECTORS:
+		if (!drm_core_check_feature(dev, DRIVER_ATOMIC))
+			return -EINVAL;
+		if (req->value > 1)
+			return -EINVAL;
+		file_priv->writeback_connectors = req->value;
+		break;
 	default:
 		return -EINVAL;
 	}
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 0e99669..222d5dc 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -388,6 +388,8 @@ struct drm_file {
 	unsigned universal_planes:1;
 	/* true if client understands atomic properties */
 	unsigned atomic:1;
+	/* true if client understands writeback connectors */
+	unsigned writeback_connectors:1;
 	/*
 	 * This client is the creator of @master.
 	 * Protected by struct drm_device::master_mutex.
diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
index b2c5284..d2b4543 100644
--- a/include/uapi/drm/drm.h
+++ b/include/uapi/drm/drm.h
@@ -678,6 +678,16 @@ struct drm_get_cap {
  */
 #define DRM_CLIENT_CAP_ATOMIC	3
 
+/**
+ * DRM_CLIENT_CAP_WRITEBACK_CONNECTORS
+ *
+ * If set to 1, the DRM core will expose writeback connectors to userspace.
+ * Writeback connectors act differently to normal connectors (e.g. there will
+ * be no screen output if only writeback connectors are enabled), so we hide
+ * them from non-aware clients.
+ */
+#define DRM_CLIENT_CAP_WRITEBACK_CONNECTORS 4
+
 /** DRM_IOCTL_SET_CLIENT_CAP ioctl argument type */
 struct drm_set_client_cap {
 	__u64 capability;
diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
index df0e350..e9cb4fe 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -247,6 +247,7 @@ struct drm_mode_get_encoder {
 #define DRM_MODE_CONNECTOR_VIRTUAL      15
 #define DRM_MODE_CONNECTOR_DSI		16
 #define DRM_MODE_CONNECTOR_DPI		17
+#define DRM_MODE_CONNECTOR_WRITEBACK	18
 
 struct drm_mode_get_connector {
 
-- 
1.7.9.5

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

#1498954 — [RFC PATCH 04/11] drm: Add __drm_framebuffer_remove_atomic

From	Brian Starkey <brian.starkey@arm.com>
Date	2016-10-11 17:00 +0200
Subject	[RFC PATCH 04/11] drm: Add __drm_framebuffer_remove_atomic
Message-ID	<sr6H8-81a-29@gated-at.bofh.it>
In reply to	#1498946

Implement the CRTC/Plane disable functionality of drm_framebuffer_remove
using the atomic API, and use it if possible.

For atomic drivers, this removes the possibility of several commits when
a framebuffer is in use by more than one CRTC/plane.

Additionally, this will provide a suitable place to support the removal
of a framebuffer from a writeback connector, in the case that a
writeback connector is still actively using a framebuffer when it is
removed by userspace.

Signed-off-by: Brian Starkey <brian.starkey@arm.com>
---
 drivers/gpu/drm/drm_framebuffer.c |  154 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 152 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
index 528f75d..b02cf73 100644
--- a/drivers/gpu/drm/drm_framebuffer.c
+++ b/drivers/gpu/drm/drm_framebuffer.c
@@ -22,6 +22,7 @@
 
 #include <linux/export.h>
 #include <drm/drmP.h>
+#include <drm/drm_atomic.h>
 #include <drm/drm_auth.h>
 #include <drm/drm_framebuffer.h>
 
@@ -795,6 +796,148 @@ void drm_framebuffer_cleanup(struct drm_framebuffer *fb)
 EXPORT_SYMBOL(drm_framebuffer_cleanup);
 
 /**
+ * __drm_framebuffer_remove_atomic - atomic version of __drm_framebuffer_remove
+ * @dev: drm device
+ * @fb: framebuffer to remove
+ *
+ * If the driver implements the atomic API, we can handle the disabling of all
+ * CRTCs/planes which use a framebuffer which is going away in a single atomic
+ * commit.
+ *
+ * This scans all CRTCs and planes in @dev's mode_config. If they're using @fb,
+ * it is removed and the CRTC/plane disabled.
+ * The legacy references are dropped and the ->fb pointers set to NULL
+ * accordingly.
+ *
+ * Returns:
+ * true if the framebuffer was successfully removed from use
+ */
+static bool __drm_framebuffer_remove_atomic(struct drm_device *dev,
+		struct drm_framebuffer *fb)
+{
+	struct drm_modeset_acquire_ctx ctx;
+	struct drm_atomic_state *state;
+	struct drm_connector_state *conn_state;
+	struct drm_connector *connector;
+	struct drm_plane *plane;
+	struct drm_crtc *crtc;
+	unsigned plane_mask;
+	int i, ret;
+
+	drm_modeset_acquire_init(&ctx, 0);
+
+	state = drm_atomic_state_alloc(dev);
+	if (!state)
+		return false;
+
+	state->acquire_ctx = &ctx;
+
+retry:
+	drm_for_each_crtc(crtc, dev) {
+		struct drm_plane_state *primary_state;
+		struct drm_crtc_state *crtc_state;
+
+		primary_state = drm_atomic_get_plane_state(state, crtc->primary);
+		if (IS_ERR(primary_state)) {
+			ret = PTR_ERR(primary_state);
+			goto fail;
+		}
+
+		if (primary_state->fb != fb)
+			continue;
+
+		crtc_state = drm_atomic_get_crtc_state(state, crtc);
+		if (IS_ERR(crtc_state)) {
+			ret = PTR_ERR(crtc_state);
+			goto fail;
+		}
+
+		/* Only handle the CRTC itself here, handle the plane later */
+		ret = drm_atomic_set_mode_for_crtc(crtc_state, NULL);
+		if (ret != 0)
+			goto fail;
+
+		crtc_state->active = false;
+
+		/* Get the connectors in order to disable them */
+		ret = drm_atomic_add_affected_connectors(state, crtc);
+		if (ret)
+			goto fail;
+	}
+
+	plane_mask = 0;
+	drm_for_each_plane(plane, dev) {
+		struct drm_plane_state *plane_state;
+
+		plane_state = drm_atomic_get_plane_state(state, plane);
+		if (IS_ERR(plane_state)) {
+			ret = PTR_ERR(plane_state);
+			goto fail;
+		}
+
+		if (plane_state->fb != fb)
+			continue;
+
+		plane->old_fb = plane->fb;
+		plane_mask |= 1 << drm_plane_index(plane);
+
+		/*
+		 * Open-coded copy of __drm_atomic_helper_disable_plane to avoid
+		 * a dependency on atomic-helper
+		 */
+		ret = drm_atomic_set_crtc_for_plane(plane_state, NULL);
+		if (ret != 0)
+			goto fail;
+
+		drm_atomic_set_fb_for_plane(plane_state, NULL);
+		plane_state->crtc_x = 0;
+		plane_state->crtc_y = 0;
+		plane_state->crtc_w = 0;
+		plane_state->crtc_h = 0;
+		plane_state->src_x = 0;
+		plane_state->src_y = 0;
+		plane_state->src_w = 0;
+		plane_state->src_h = 0;
+	}
+
+	/* All of the connectors in state need disabling */
+	for_each_connector_in_state(state, connector, conn_state, i) {
+		ret = drm_atomic_set_crtc_for_connector(conn_state,
+							NULL);
+		if (ret)
+			goto fail;
+	}
+
+	if (WARN_ON(!plane_mask)) {
+		DRM_ERROR("Couldn't find any usage of [FB:%d]\n", fb->base.id);
+		ret = -ENOENT;
+		goto fail;
+	}
+
+	ret = drm_atomic_commit(state);
+
+fail:
+	drm_atomic_clean_old_fb(dev, plane_mask, ret);
+
+	if (ret == -EDEADLK)
+		goto backoff;
+
+	if (ret != 0)
+		drm_atomic_state_free(state);
+
+	drm_modeset_drop_locks(&ctx);
+	drm_modeset_acquire_fini(&ctx);
+
+	return ret ? false : true;
+
+backoff:
+	drm_atomic_state_clear(state);
+	drm_modeset_backoff(&ctx);
+
+	goto retry;
+}
+
+/**
  * __drm_framebuffer_remove - remove all usage of a framebuffer object
  * @dev: drm device
  * @fb: framebuffer to remove
@@ -869,9 +1012,16 @@ void drm_framebuffer_remove(struct drm_framebuffer *fb)
 	 * in-use fb with fb-id == 0. Userspace is allowed to shoot its own foot
 	 * in this manner.
 	 */
-	if (drm_framebuffer_read_refcount(fb) > 1)
-		if (!__drm_framebuffer_remove(dev, fb))
+	if (drm_framebuffer_read_refcount(fb) > 1) {
+		bool removed;
+		if (dev->mode_config.funcs->atomic_commit)
+			removed = __drm_framebuffer_remove_atomic(dev, fb);
+		else
+			removed = __drm_framebuffer_remove(dev, fb);
+
+		if (!removed)
 			DRM_ERROR("failed to remove fb from active usage\n");
+	}
 
 	drm_framebuffer_unreference(fb);
 }
-- 
1.7.9.5

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

#1498999 — Re: [RFC PATCH 04/11] drm: Add __drm_framebuffer_remove_atomic

From	Daniel Vetter <daniel@ffwll.ch>
Date	2016-10-11 18:10 +0200
Subject	Re: [RFC PATCH 04/11] drm: Add __drm_framebuffer_remove_atomic
Message-ID	<sr7MS-qu-9@gated-at.bofh.it>
In reply to	#1498954

On Tue, Oct 11, 2016 at 03:54:01PM +0100, Brian Starkey wrote:
> Implement the CRTC/Plane disable functionality of drm_framebuffer_remove
> using the atomic API, and use it if possible.
> 
> For atomic drivers, this removes the possibility of several commits when
> a framebuffer is in use by more than one CRTC/plane.
> 
> Additionally, this will provide a suitable place to support the removal
> of a framebuffer from a writeback connector, in the case that a
> writeback connector is still actively using a framebuffer when it is
> removed by userspace.
> 
> Signed-off-by: Brian Starkey <brian.starkey@arm.com>

Just the small comment here: Last time around I wanted toland an atomic
disable function for fb remove code it blew up. Need to check out git
history to make sure we've addressed those isses.  Caveat emperor ;-)
-Daniel

> ---
>  drivers/gpu/drm/drm_framebuffer.c |  154 ++++++++++++++++++++++++++++++++++++-
>  1 file changed, 152 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
> index 528f75d..b02cf73 100644
> --- a/drivers/gpu/drm/drm_framebuffer.c
> +++ b/drivers/gpu/drm/drm_framebuffer.c
> @@ -22,6 +22,7 @@
>  
>  #include <linux/export.h>
>  #include <drm/drmP.h>
> +#include <drm/drm_atomic.h>
>  #include <drm/drm_auth.h>
>  #include <drm/drm_framebuffer.h>
>  
> @@ -795,6 +796,148 @@ void drm_framebuffer_cleanup(struct drm_framebuffer *fb)
>  EXPORT_SYMBOL(drm_framebuffer_cleanup);
>  
>  /**
> + * __drm_framebuffer_remove_atomic - atomic version of __drm_framebuffer_remove
> + * @dev: drm device
> + * @fb: framebuffer to remove
> + *
> + * If the driver implements the atomic API, we can handle the disabling of all
> + * CRTCs/planes which use a framebuffer which is going away in a single atomic
> + * commit.
> + *
> + * This scans all CRTCs and planes in @dev's mode_config. If they're using @fb,
> + * it is removed and the CRTC/plane disabled.
> + * The legacy references are dropped and the ->fb pointers set to NULL
> + * accordingly.
> + *
> + * Returns:
> + * true if the framebuffer was successfully removed from use
> + */
> +static bool __drm_framebuffer_remove_atomic(struct drm_device *dev,
> +		struct drm_framebuffer *fb)
> +{
> +	struct drm_modeset_acquire_ctx ctx;
> +	struct drm_atomic_state *state;
> +	struct drm_connector_state *conn_state;
> +	struct drm_connector *connector;
> +	struct drm_plane *plane;
> +	struct drm_crtc *crtc;
> +	unsigned plane_mask;
> +	int i, ret;
> +
> +	drm_modeset_acquire_init(&ctx, 0);
> +
> +	state = drm_atomic_state_alloc(dev);
> +	if (!state)
> +		return false;
> +
> +	state->acquire_ctx = &ctx;
> +
> +retry:
> +	drm_for_each_crtc(crtc, dev) {
> +		struct drm_plane_state *primary_state;
> +		struct drm_crtc_state *crtc_state;
> +
> +		primary_state = drm_atomic_get_plane_state(state, crtc->primary);
> +		if (IS_ERR(primary_state)) {
> +			ret = PTR_ERR(primary_state);
> +			goto fail;
> +		}
> +
> +		if (primary_state->fb != fb)
> +			continue;
> +
> +		crtc_state = drm_atomic_get_crtc_state(state, crtc);
> +		if (IS_ERR(crtc_state)) {
> +			ret = PTR_ERR(crtc_state);
> +			goto fail;
> +		}
> +
> +		/* Only handle the CRTC itself here, handle the plane later */
> +		ret = drm_atomic_set_mode_for_crtc(crtc_state, NULL);
> +		if (ret != 0)
> +			goto fail;
> +
> +		crtc_state->active = false;
> +
> +		/* Get the connectors in order to disable them */
> +		ret = drm_atomic_add_affected_connectors(state, crtc);
> +		if (ret)
> +			goto fail;
> +	}
> +
> +	plane_mask = 0;
> +	drm_for_each_plane(plane, dev) {
> +		struct drm_plane_state *plane_state;
> +
> +		plane_state = drm_atomic_get_plane_state(state, plane);
> +		if (IS_ERR(plane_state)) {
> +			ret = PTR_ERR(plane_state);
> +			goto fail;
> +		}
> +
> +		if (plane_state->fb != fb)
> +			continue;
> +
> +		plane->old_fb = plane->fb;
> +		plane_mask |= 1 << drm_plane_index(plane);
> +
> +		/*
> +		 * Open-coded copy of __drm_atomic_helper_disable_plane to avoid
> +		 * a dependency on atomic-helper
> +		 */
> +		ret = drm_atomic_set_crtc_for_plane(plane_state, NULL);
> +		if (ret != 0)
> +			goto fail;
> +
> +		drm_atomic_set_fb_for_plane(plane_state, NULL);
> +		plane_state->crtc_x = 0;
> +		plane_state->crtc_y = 0;
> +		plane_state->crtc_w = 0;
> +		plane_state->crtc_h = 0;
> +		plane_state->src_x = 0;
> +		plane_state->src_y = 0;
> +		plane_state->src_w = 0;
> +		plane_state->src_h = 0;
> +	}
> +
> +	/* All of the connectors in state need disabling */
> +	for_each_connector_in_state(state, connector, conn_state, i) {
> +		ret = drm_atomic_set_crtc_for_connector(conn_state,
> +							NULL);
> +		if (ret)
> +			goto fail;
> +	}
> +
> +	if (WARN_ON(!plane_mask)) {
> +		DRM_ERROR("Couldn't find any usage of [FB:%d]\n", fb->base.id);
> +		ret = -ENOENT;
> +		goto fail;
> +	}
> +
> +	ret = drm_atomic_commit(state);
> +
> +fail:
> +	drm_atomic_clean_old_fb(dev, plane_mask, ret);
> +
> +	if (ret == -EDEADLK)
> +		goto backoff;
> +
> +	if (ret != 0)
> +		drm_atomic_state_free(state);
> +
> +	drm_modeset_drop_locks(&ctx);
> +	drm_modeset_acquire_fini(&ctx);
> +
> +	return ret ? false : true;
> +
> +backoff:
> +	drm_atomic_state_clear(state);
> +	drm_modeset_backoff(&ctx);
> +
> +	goto retry;
> +}
> +
> +/**
>   * __drm_framebuffer_remove - remove all usage of a framebuffer object
>   * @dev: drm device
>   * @fb: framebuffer to remove
> @@ -869,9 +1012,16 @@ void drm_framebuffer_remove(struct drm_framebuffer *fb)
>  	 * in-use fb with fb-id == 0. Userspace is allowed to shoot its own foot
>  	 * in this manner.
>  	 */
> -	if (drm_framebuffer_read_refcount(fb) > 1)
> -		if (!__drm_framebuffer_remove(dev, fb))
> +	if (drm_framebuffer_read_refcount(fb) > 1) {
> +		bool removed;
> +		if (dev->mode_config.funcs->atomic_commit)
> +			removed = __drm_framebuffer_remove_atomic(dev, fb);
> +		else
> +			removed = __drm_framebuffer_remove(dev, fb);
> +
> +		if (!removed)
>  			DRM_ERROR("failed to remove fb from active usage\n");
> +	}
>  
>  	drm_framebuffer_unreference(fb);
>  }
> -- 
> 1.7.9.5
> 

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

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

#1498955 — [RFC PATCH 08/11] drm: mali-dp: Rename malidp_input_format

From	Brian Starkey <brian.starkey@arm.com>
Date	2016-10-11 17:00 +0200
Subject	[RFC PATCH 08/11] drm: mali-dp: Rename malidp_input_format
Message-ID	<sr6H8-81a-41@gated-at.bofh.it>
In reply to	#1498946

We're going to use the same format list for output formats, so rename
everything related to input formats to avoid confusion.

Signed-off-by: Brian Starkey <brian.starkey@arm.com>
Reviewed-by: Liviu Dudau <Liviu.Dudau@arm.com>
---
 drivers/gpu/drm/arm/malidp_hw.c     |   24 ++++++++++++------------
 drivers/gpu/drm/arm/malidp_hw.h     |    8 ++++----
 drivers/gpu/drm/arm/malidp_planes.c |    8 ++++----
 3 files changed, 20 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/arm/malidp_hw.c b/drivers/gpu/drm/arm/malidp_hw.c
index 7f4a0bd..44a9d10 100644
--- a/drivers/gpu/drm/arm/malidp_hw.c
+++ b/drivers/gpu/drm/arm/malidp_hw.c
@@ -21,7 +21,7 @@
 #include "malidp_drv.h"
 #include "malidp_hw.h"
 
-static const struct malidp_input_format malidp500_de_formats[] = {
+static const struct malidp_format_id malidp500_de_formats[] = {
 	/*    fourcc,   layers supporting the format,     internal id  */
 	{ DRM_FORMAT_ARGB2101010, DE_VIDEO1 | DE_GRAPHICS1 | DE_GRAPHICS2,  0 },
 	{ DRM_FORMAT_ABGR2101010, DE_VIDEO1 | DE_GRAPHICS1 | DE_GRAPHICS2,  1 },
@@ -69,7 +69,7 @@ static const struct malidp_input_format malidp500_de_formats[] = {
 	{ DRM_FORMAT_NV12, DE_VIDEO1 | DE_VIDEO2, MALIDP_ID(5, 6) },	\
 	{ DRM_FORMAT_YUV420, DE_VIDEO1 | DE_VIDEO2, MALIDP_ID(5, 7) }
 
-static const struct malidp_input_format malidp550_de_formats[] = {
+static const struct malidp_format_id malidp550_de_formats[] = {
 	MALIDP_COMMON_FORMATS,
 };
 
@@ -439,8 +439,8 @@ const struct malidp_hw_device malidp_device[MALIDP_MAX_DEVICES] = {
 				.irq_mask = MALIDP500_DE_IRQ_CONF_VALID,
 				.vsync_irq = MALIDP500_DE_IRQ_CONF_VALID,
 			},
-			.input_formats = malidp500_de_formats,
-			.n_input_formats = ARRAY_SIZE(malidp500_de_formats),
+			.pixel_formats = malidp500_de_formats,
+			.n_pixel_formats = ARRAY_SIZE(malidp500_de_formats),
 			.bus_align_bytes = 8,
 		},
 		.query_hw = malidp500_query_hw,
@@ -472,8 +472,8 @@ const struct malidp_hw_device malidp_device[MALIDP_MAX_DEVICES] = {
 				.irq_mask = MALIDP550_DC_IRQ_CONF_VALID,
 				.vsync_irq = MALIDP550_DC_IRQ_CONF_VALID,
 			},
-			.input_formats = malidp550_de_formats,
-			.n_input_formats = ARRAY_SIZE(malidp550_de_formats),
+			.pixel_formats = malidp550_de_formats,
+			.n_pixel_formats = ARRAY_SIZE(malidp550_de_formats),
 			.bus_align_bytes = 8,
 		},
 		.query_hw = malidp550_query_hw,
@@ -506,8 +506,8 @@ const struct malidp_hw_device malidp_device[MALIDP_MAX_DEVICES] = {
 				.irq_mask = MALIDP550_DC_IRQ_CONF_VALID,
 				.vsync_irq = MALIDP550_DC_IRQ_CONF_VALID,
 			},
-			.input_formats = malidp550_de_formats,
-			.n_input_formats = ARRAY_SIZE(malidp550_de_formats),
+			.pixel_formats = malidp550_de_formats,
+			.n_pixel_formats = ARRAY_SIZE(malidp550_de_formats),
 			.bus_align_bytes = 16,
 		},
 		.query_hw = malidp650_query_hw,
@@ -525,10 +525,10 @@ u8 malidp_hw_get_format_id(const struct malidp_hw_regmap *map,
 {
 	unsigned int i;
 
-	for (i = 0; i < map->n_input_formats; i++) {
-		if (((map->input_formats[i].layer & layer_id) == layer_id) &&
-		    (map->input_formats[i].format == format))
-			return map->input_formats[i].id;
+	for (i = 0; i < map->n_pixel_formats; i++) {
+		if (((map->pixel_formats[i].layer & layer_id) == layer_id) &&
+		    (map->pixel_formats[i].format == format))
+			return map->pixel_formats[i].id;
 	}
 
 	return MALIDP_INVALID_FORMAT_ID;
diff --git a/drivers/gpu/drm/arm/malidp_hw.h b/drivers/gpu/drm/arm/malidp_hw.h
index 087e1202..4f8c884 100644
--- a/drivers/gpu/drm/arm/malidp_hw.h
+++ b/drivers/gpu/drm/arm/malidp_hw.h
@@ -35,7 +35,7 @@ enum {
 	DE_SMART = BIT(4),
 };
 
-struct malidp_input_format {
+struct malidp_format_id {
 	u32 format;		/* DRM fourcc */
 	u8 layer;		/* bitmask of layers supporting it */
 	u8 id;			/* used internally */
@@ -85,9 +85,9 @@ struct malidp_hw_regmap {
 	const struct malidp_irq_map se_irq_map;
 	const struct malidp_irq_map dc_irq_map;
 
-	/* list of supported input formats for each layer */
-	const struct malidp_input_format *input_formats;
-	const u8 n_input_formats;
+	/* list of supported pixel formats for each layer */
+	const struct malidp_format_id *pixel_formats;
+	const u8 n_pixel_formats;
 
 	/* pitch alignment requirement in bytes */
 	const u8 bus_align_bytes;
diff --git a/drivers/gpu/drm/arm/malidp_planes.c b/drivers/gpu/drm/arm/malidp_planes.c
index 80f389b..e3fe11d 100644
--- a/drivers/gpu/drm/arm/malidp_planes.c
+++ b/drivers/gpu/drm/arm/malidp_planes.c
@@ -253,7 +253,7 @@ int malidp_de_planes_init(struct drm_device *drm)
 	u32 *formats;
 	int ret, i, j, n;
 
-	formats = kcalloc(map->n_input_formats, sizeof(*formats), GFP_KERNEL);
+	formats = kcalloc(map->n_pixel_formats, sizeof(*formats), GFP_KERNEL);
 	if (!formats) {
 		ret = -ENOMEM;
 		goto cleanup;
@@ -269,9 +269,9 @@ int malidp_de_planes_init(struct drm_device *drm)
 		}
 
 		/* build the list of DRM supported formats based on the map */
-		for (n = 0, j = 0;  j < map->n_input_formats; j++) {
-			if ((map->input_formats[j].layer & id) == id)
-				formats[n++] = map->input_formats[j].format;
+		for (n = 0, j = 0;  j < map->n_pixel_formats; j++) {
+			if ((map->pixel_formats[j].layer & id) == id)
+				formats[n++] = map->pixel_formats[j].format;
 		}
 
 		plane_type = (i == 0) ? DRM_PLANE_TYPE_PRIMARY :
-- 
1.7.9.5

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

#1498956 — [RFC PATCH 11/11] drm: mali-dp: Add writeback connector

From	Brian Starkey <brian.starkey@arm.com>
Date	2016-10-11 17:00 +0200
Subject	[RFC PATCH 11/11] drm: mali-dp: Add writeback connector
Message-ID	<sr6H8-81a-39@gated-at.bofh.it>
In reply to	#1498946

Mali-DP has a memory writeback engine which can be used to write the
composition result to a memory buffer.
Expose this functionality as a DRM writeback connector on supported
hardware.

Signed-off-by: Brian Starkey <brian.starkey@arm.com>
---
 drivers/gpu/drm/arm/Makefile      |    1 +
 drivers/gpu/drm/arm/malidp_crtc.c |   10 ++
 drivers/gpu/drm/arm/malidp_drv.c  |   25 +++-
 drivers/gpu/drm/arm/malidp_drv.h  |    5 +
 drivers/gpu/drm/arm/malidp_mw.c   |  268 +++++++++++++++++++++++++++++++++++++
 5 files changed, 305 insertions(+), 4 deletions(-)
 create mode 100644 drivers/gpu/drm/arm/malidp_mw.c

diff --git a/drivers/gpu/drm/arm/Makefile b/drivers/gpu/drm/arm/Makefile
index bb8b158..3bf31d1 100644
--- a/drivers/gpu/drm/arm/Makefile
+++ b/drivers/gpu/drm/arm/Makefile
@@ -1,4 +1,5 @@
 hdlcd-y := hdlcd_drv.o hdlcd_crtc.o
 obj-$(CONFIG_DRM_HDLCD)	+= hdlcd.o
 mali-dp-y := malidp_drv.o malidp_hw.o malidp_planes.o malidp_crtc.o
+mali-dp-y += malidp_mw.o
 obj-$(CONFIG_DRM_MALI_DISPLAY)	+= mali-dp.o
diff --git a/drivers/gpu/drm/arm/malidp_crtc.c b/drivers/gpu/drm/arm/malidp_crtc.c
index 08e6a71..98ddcea 100644
--- a/drivers/gpu/drm/arm/malidp_crtc.c
+++ b/drivers/gpu/drm/arm/malidp_crtc.c
@@ -68,6 +68,16 @@ static void malidp_crtc_enable(struct drm_crtc *crtc)
 	clk_set_rate(hwdev->pxlclk, crtc->state->adjusted_mode.crtc_clock * 1000);
 
 	hwdev->modeset(hwdev, &vm);
+	/*
+	 * We should always disable the memory write when leaving config mode,
+	 * otherwise the hardware will start writing right away - possibly with
+	 * a stale config, and definitely before we've had a chance to configure
+	 * the planes.
+	 * If the memory write needs to be enabled, that will get taken care
+	 * of later during the atomic commit
+	 */
+	if (hwdev->disable_memwrite)
+		hwdev->disable_memwrite(hwdev);
 	hwdev->leave_config_mode(hwdev);
 	drm_crtc_vblank_on(crtc);
 }
diff --git a/drivers/gpu/drm/arm/malidp_drv.c b/drivers/gpu/drm/arm/malidp_drv.c
index 62a29f6..e20266e 100644
--- a/drivers/gpu/drm/arm/malidp_drv.c
+++ b/drivers/gpu/drm/arm/malidp_drv.c
@@ -91,7 +91,16 @@ static void malidp_atomic_commit_tail(struct drm_atomic_state *state)
 	struct drm_device *drm = state->dev;
 
 	drm_atomic_helper_commit_modeset_disables(drm, state);
+
 	drm_atomic_helper_commit_modeset_enables(drm, state);
+
+	/*
+	 * The order here is important. We must configure memory-write after
+	 * the CRTC is already enabled, so that its configuration update is
+	 * gated on the next CVAL.
+	 */
+	malidp_mw_atomic_commit(drm, state);
+
 	drm_atomic_helper_commit_planes(drm, state,
 					DRM_PLANE_COMMIT_ACTIVE_ONLY);
 
@@ -148,12 +157,20 @@ static int malidp_init(struct drm_device *drm)
 	drm->mode_config.helper_private = &malidp_mode_config_helpers;
 
 	ret = malidp_crtc_init(drm);
-	if (ret) {
-		drm_mode_config_cleanup(drm);
-		return ret;
-	}
+	if (ret)
+		goto crtc_fail;
+
+	ret = malidp_mw_connector_init(drm);
+	if (ret)
+		goto mw_fail;
 
 	return 0;
+
+mw_fail:
+	malidp_de_planes_destroy(drm);
+crtc_fail:
+	drm_mode_config_cleanup(drm);
+	return ret;
 }
 
 static void malidp_fini(struct drm_device *drm)
diff --git a/drivers/gpu/drm/arm/malidp_drv.h b/drivers/gpu/drm/arm/malidp_drv.h
index 9fc8a2e..905c104 100644
--- a/drivers/gpu/drm/arm/malidp_drv.h
+++ b/drivers/gpu/drm/arm/malidp_drv.h
@@ -22,6 +22,8 @@ struct malidp_drm {
 	struct drm_fbdev_cma *fbdev;
 	struct list_head event_list;
 	struct drm_crtc crtc;
+	struct drm_encoder mw_encoder;
+	struct drm_connector mw_connector;
 	wait_queue_head_t wq;
 	atomic_t config_valid;
 };
@@ -50,6 +52,9 @@ struct malidp_plane_state {
 int malidp_de_planes_init(struct drm_device *drm);
 void malidp_de_planes_destroy(struct drm_device *drm);
 int malidp_crtc_init(struct drm_device *drm);
+int malidp_mw_connector_init(struct drm_device *drm);
+void malidp_mw_atomic_commit(struct drm_device *drm,
+			     struct drm_atomic_state *old_state);
 
 /* often used combination of rotational bits */
 #define MALIDP_ROTATED_MASK	(DRM_ROTATE_90 | DRM_ROTATE_270)
diff --git a/drivers/gpu/drm/arm/malidp_mw.c b/drivers/gpu/drm/arm/malidp_mw.c
new file mode 100644
index 0000000..72df3fd
--- /dev/null
+++ b/drivers/gpu/drm/arm/malidp_mw.c
@@ -0,0 +1,268 @@
+/*
+ * (C) COPYRIGHT 2016 ARM Limited. All rights reserved.
+ * Author: Brian Starkey <brian.starkey@arm.com>
+ *
+ * This program is free software and is provided to you under the terms of the
+ * GNU General Public License version 2 as published by the Free Software
+ * Foundation, and any use by you of this program is subject to the terms
+ * of such GNU licence.
+ *
+ * ARM Mali DP Writeback connector implementation
+ */
+#include <drm/drmP.h>
+#include <drm/drm_crtc.h>
+#include <drm/drm_atomic.h>
+#include <drm/drm_atomic_helper.h>
+#include <drm/drm_crtc_helper.h>
+#include <drm/drm_fb_cma_helper.h>
+#include <drm/drm_gem_cma_helper.h>
+
+#include "malidp_drv.h"
+#include "malidp_hw.h"
+
+#define mw_conn_to_malidp_device(x) container_of(x, struct malidp_drm, mw_connector)
+#define to_mw_state(_state) (struct malidp_mw_connector_state *)(_state)
+
+struct malidp_mw_connector_state {
+	struct drm_connector_state base;
+	dma_addr_t addrs[2];
+	s32 pitches[2];
+	u8 format;
+	u8 n_planes;
+	u8 crtc_active:1;
+};
+
+static int malidp_mw_connector_get_modes(struct drm_connector *connector)
+{
+	struct drm_device *dev = connector->dev;
+
+	return drm_add_modes_noedid(connector, dev->mode_config.max_width,
+				    dev->mode_config.max_height);
+}
+
+static enum drm_mode_status
+malidp_mw_connector_mode_valid(struct drm_connector *connector,
+			       struct drm_display_mode *mode)
+{
+	struct drm_device *dev = connector->dev;
+	struct drm_mode_config *mode_config = &dev->mode_config;
+	int w = mode->hdisplay, h = mode->vdisplay;
+
+	if ((w < mode_config->min_width) || (w > mode_config->max_width))
+		return MODE_BAD_HVALUE;
+
+	if ((h < mode_config->min_height) || (h > mode_config->max_height))
+		return MODE_BAD_VVALUE;
+
+	return MODE_OK;
+}
+
+const struct drm_connector_helper_funcs malidp_mw_connector_helper_funcs = {
+	.get_modes = malidp_mw_connector_get_modes,
+	.mode_valid = malidp_mw_connector_mode_valid,
+};
+
+static enum drm_connector_status
+malidp_mw_connector_detect(struct drm_connector *connector, bool force)
+{
+	return connector_status_connected;
+}
+
+static void malidp_mw_connector_destroy(struct drm_connector *connector)
+{
+	drm_connector_cleanup(connector);
+}
+
+static struct drm_connector_state *
+malidp_mw_connector_duplicate_state(struct drm_connector *connector)
+{
+	struct malidp_mw_connector_state *mw_state, *old_state;
+
+	if (WARN_ON(!connector->state))
+		return NULL;
+
+	mw_state = kmalloc(sizeof(*mw_state), GFP_KERNEL);
+	if (!mw_state)
+		return NULL;
+
+	old_state = (struct malidp_mw_connector_state *)connector->state;
+	memcpy(mw_state, old_state, sizeof(*mw_state));
+	/*
+	 * This duplicates a little memcpy, but it ensures we still do the
+	 * right thing if drm_connector_state changes
+	 */
+	__drm_atomic_helper_connector_duplicate_state(connector, &mw_state->base);
+
+	return &mw_state->base;
+}
+
+static void malidp_mw_connector_destroy_state(struct drm_connector *connector,
+					      struct drm_connector_state *state)
+{
+	struct malidp_mw_connector_state *mw_state =
+		(struct malidp_mw_connector_state *)state;
+
+	__drm_atomic_helper_connector_destroy_state(&mw_state->base);
+	kfree(mw_state);
+}
+
+static const struct drm_connector_funcs malidp_mw_connector_funcs = {
+	.dpms = drm_atomic_helper_connector_dpms,
+	.reset = drm_atomic_helper_connector_reset,
+	.detect = malidp_mw_connector_detect,
+	.fill_modes = drm_helper_probe_single_connector_modes,
+	.destroy = malidp_mw_connector_destroy,
+	.atomic_duplicate_state = malidp_mw_connector_duplicate_state,
+	.atomic_destroy_state = malidp_mw_connector_destroy_state,
+};
+
+static int
+malidp_mw_encoder_atomic_check(struct drm_encoder *encoder,
+			       struct drm_crtc_state *crtc_state,
+			       struct drm_connector_state *conn_state)
+{
+	struct drm_connector *conn = conn_state->connector;
+	struct drm_framebuffer *fb = conn_state->fb;
+	struct malidp_drm *malidp = mw_conn_to_malidp_device(conn);
+	struct malidp_mw_connector_state *mw_state;
+	int i, n_planes;
+
+	mw_state = (struct malidp_mw_connector_state *)conn_state;
+	mw_state->crtc_active = crtc_state->active;
+
+	if (!conn_state->fb)
+		return 0;
+
+	if ((fb->width != crtc_state->mode.hdisplay) ||
+	    (fb->height != crtc_state->mode.vdisplay)) {
+		DRM_DEBUG_KMS("Invalid framebuffer size %ux%u\n",
+				fb->width, fb->height);
+		return -EINVAL;
+	}
+
+	mw_state->format =
+		malidp_hw_get_format_id(&malidp->dev->map, SE_MEMWRITE,
+					fb->pixel_format);
+	if (mw_state->format == MALIDP_INVALID_FORMAT_ID) {
+		char *format_name = drm_get_format_name(fb->pixel_format);
+		DRM_DEBUG_KMS("Invalid pixel format %s\n", format_name);
+		kfree(format_name);
+		return -EINVAL;
+	}
+
+	n_planes = drm_format_num_planes(fb->pixel_format);
+	for (i = 0; i < n_planes; i++) {
+		struct drm_gem_cma_object *obj = drm_fb_cma_get_gem_obj(fb, i);
+		if (!malidp_hw_pitch_valid(malidp->dev, fb->pitches[i])) {
+			DRM_DEBUG_KMS("Invalid pitch %u for plane %d\n",
+				      fb->pitches[i], i);
+			return -EINVAL;
+		}
+		mw_state->pitches[i] = fb->pitches[i];
+		mw_state->addrs[i] = obj->paddr + fb->offsets[i];
+	}
+	mw_state->n_planes = n_planes;
+
+	return 0;
+}
+
+static const struct drm_encoder_helper_funcs malidp_mw_encoder_helper_funcs = {
+	.atomic_check = malidp_mw_encoder_atomic_check,
+};
+
+static void malidp_mw_encoder_destroy(struct drm_encoder *encoder)
+{
+	drm_encoder_cleanup(encoder);
+}
+
+static const struct drm_encoder_funcs malidp_mw_encoder_funcs = {
+	.destroy = malidp_mw_encoder_destroy,
+};
+
+int malidp_mw_connector_init(struct drm_device *drm)
+{
+	struct malidp_drm *malidp = drm->dev_private;
+	const struct malidp_hw_regmap *map = &malidp->dev->map;
+	u32 *formats;
+	int ret, n, i;
+
+	if (!malidp->dev->enable_memwrite)
+		return 0;
+
+	ret = drm_mode_create_writeback_connector_properties(drm);
+	if (ret)
+		return ret;
+
+	drm_encoder_helper_add(&malidp->mw_encoder, &malidp_mw_encoder_helper_funcs);
+	malidp->mw_encoder.possible_crtcs = 1 << drm_crtc_index(&malidp->crtc);
+	ret = drm_encoder_init(drm, &malidp->mw_encoder, &malidp_mw_encoder_funcs,
+			       DRM_MODE_ENCODER_VIRTUAL, NULL);
+	if (ret)
+		return ret;
+
+	drm_connector_helper_add(&malidp->mw_connector,
+				 &malidp_mw_connector_helper_funcs);
+	malidp->mw_connector.interlace_allowed = 0;
+	ret = drm_connector_init(drm, &malidp->mw_connector,
+				 &malidp_mw_connector_funcs,
+				 DRM_MODE_CONNECTOR_WRITEBACK);
+	if (ret)
+		goto err_encoder;
+
+	ret = drm_mode_connector_attach_encoder(&malidp->mw_connector,
+						&malidp->mw_encoder);
+	if (ret)
+		goto err_connector;
+
+	formats = kcalloc(map->n_pixel_formats, sizeof(*formats), GFP_KERNEL);
+	if (!formats)
+		goto err_connector;
+
+	for (n = 0, i = 0;  i < map->n_pixel_formats; i++) {
+		if (map->pixel_formats[i].layer & SE_MEMWRITE)
+			formats[n++] = map->pixel_formats[i].format;
+	}
+
+	ret = drm_mode_connector_set_writeback_formats(&malidp->mw_connector,
+						       formats, n);
+	kfree(formats);
+	if (ret)
+		goto err_connector;
+
+	return 0;
+
+err_connector:
+	drm_connector_cleanup(&malidp->mw_connector);
+err_encoder:
+	drm_encoder_cleanup(&malidp->mw_encoder);
+	return ret;
+}
+
+void malidp_mw_atomic_commit(struct drm_device *drm,
+			     struct drm_atomic_state *old_state)
+{
+	struct malidp_mw_connector_state *mw_state;
+	struct malidp_drm *malidp = drm->dev_private;
+	struct malidp_hw_device *hwdev = malidp->dev;
+	struct drm_connector *mw_conn = &malidp->mw_connector;
+	struct drm_framebuffer *fb;
+
+	mw_state = to_mw_state(mw_conn->state);
+	if (!mw_state || !mw_state->crtc_active)
+		return;
+
+	if (!mw_state->base.fb) {
+		struct malidp_mw_connector_state *old_mw_state;
+		old_mw_state = to_mw_state(drm_atomic_get_existing_connector_state(old_state,
+										   mw_conn));
+		if (old_mw_state && old_mw_state->base.fb)
+			hwdev->disable_memwrite(hwdev);
+
+		return;
+	}
+
+	fb = mw_state->base.fb;
+	hwdev->enable_memwrite(hwdev, mw_state->addrs, mw_state->pitches,
+			       mw_state->n_planes, fb->width, fb->height,
+			       mw_state->format);
+}
-- 
1.7.9.5

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

#1498978

From	Daniel Vetter <daniel@ffwll.ch>
Date	2016-10-11 17:50 +0200
Message-ID	<sr7tv-4W-1@gated-at.bofh.it>
In reply to	#1498946

On Tue, Oct 11, 2016 at 03:53:57PM +0100, Brian Starkey wrote:
> Hi,
> 
> This RFC series introduces a new connector type:
>  DRM_MODE_CONNECTOR_WRITEBACK
> It is a follow-on from a previous discussion: [1]
> 
> Writeback connectors are used to expose the memory writeback engines
> found in some display controllers, which can write a CRTC's
> composition result to a memory buffer.
> This is useful e.g. for testing, screen-recording, screenshots,
> wireless display, display cloning, memory-to-memory composition.
> 
> Patches 1-7 include the core framework changes required, and patches
> 8-11 implement a writeback connector for the Mali-DP writeback engine.
> The Mali-DP patches depend on this other series: [2].
> 
> The connector is given the FB_ID property for the output framebuffer,
> and two new read-only properties: PIXEL_FORMATS and
> PIXEL_FORMATS_SIZE, which expose the supported framebuffer pixel
> formats of the engine.
> 
> The EDID property is not exposed for writeback connectors.
> 
> Writeback connector usage:
> --------------------------
> Due to connector routing changes being treated as "full modeset"
> operations, any client which wishes to use a writeback connector
> should include the connector in every modeset. The writeback will not
> actually become active until a framebuffer is attached.

Erhm, this is just the default, drivers can override this. And we could
change the atomic helpers to not mark a modeset as a modeset if the
connector that changed is a writeback one.

> The writeback itself is enabled by attaching a framebuffer to the
> FB_ID property of the connector. The driver must then ensure that the
> CRTC content of that atomic commit is written into the framebuffer.
> 
> The writeback works in a one-shot mode with each atomic commit. This
> prevents the same content from being written multiple times.
> In some cases (front-buffer rendering) there might be a desire for
> continuous operation - I think a property could be added later for
> this kind of control.
> 
> Writeback can be disabled by setting FB_ID to zero.

This seems to contradict itself: If it's one-shot, there's no need to
disable it - it will auto-disable.

In other cases where we write a property as a one-shot thing (fences for
android). In that case when you read that property it's always 0 (well, -1
for fences since file descriptor). That also avoids the issues when
userspace unconditionally saves/restores all properties (this is needed
for generic compositor switching).

I think a better behaviour would be to do the same trick, with FB_ID on
the connector always returning 0 as the current value. That encodes the
one-shot behaviour directly.

For one-shot vs continuous: Maybe we want to simply have a separate
writeback property for continues, e.g. FB_WRITEBACK_ONE_SHOT_ID and
FB_WRITEBACK_CONTINUOUS_ID.

> Known issues:
> -------------
>  * I'm not sure what "DPMS" should mean for writeback connectors.
>    It could be used to disable writeback (even when a framebuffer is
>    attached), or it could be hidden entirely (which would break the
>    legacy DPMS call for writeback connectors).

dpms is legacy, in atomic land the only thing you have is "ACTIVE" on the
crtc. it disables everything, i.e. also writeback.

>  * With Daniel's recent re-iteration of the userspace API rules, I
>    fully expect to provide some userspace code to support this. The
>    question is what, and where? We want to use writeback for testing,
>    so perhaps some tests in igt is suitable.

Hm, testing would be better as a debugfs interface, but I understand the
appeal of doing this with atomic (since semantics fit so well). Another
use-case of this is compositing, but if the main goal is igt and testing,
I think integration into igt crc based testcases is a perfectly fine
userspace.

>  * Documentation. Probably some portion of this cover letter needs to
>    make it into Documentation/

Yeah, an overview DOC: section in a separate source file (with all the the
infrastructure work) would be great - aka needed from my pov ;-)

>  * Synchronisation. Our hardware will finish the writeback by the next
>    vsync. I've not implemented fence support here, but it would be an
>    obvious addition.

Probably just want an additional WRITEBACK_FENCE_ID property to signal
completion. Some hw definitely will take longer to write back than just a
vblank. But we can delay that until it's needed.
-Daniel

> 
> See Also:
> ---------
> [1] https://lists.freedesktop.org/archives/dri-devel/2016-July/113197.html
> [2] https://lists.freedesktop.org/archives/dri-devel/2016-October/120486.html
> 
> I welcome any comments, especially if this approach does/doesn't fit
> well with anyone else's hardware.
> 
> Thanks,
> 
> -Brian
> 
> ---
> 
> Brian Starkey (10):
>   drm: add writeback connector type
>   drm/fb-helper: skip writeback connectors
>   drm: extract CRTC/plane disable from drm_framebuffer_remove
>   drm: add __drm_framebuffer_remove_atomic
>   drm: add fb to connector state
>   drm: expose fb_id property for writeback connectors
>   drm: add writeback-connector pixel format properties
>   drm: mali-dp: rename malidp_input_format
>   drm: mali-dp: add RGB writeback formats for DP550/DP650
>   drm: mali-dp: add writeback connector
> 
> Liviu Dudau (1):
>   drm: mali-dp: Add support for writeback on DP550/DP650
> 
>  drivers/gpu/drm/arm/Makefile        |    1 +
>  drivers/gpu/drm/arm/malidp_crtc.c   |   10 ++
>  drivers/gpu/drm/arm/malidp_drv.c    |   25 +++-
>  drivers/gpu/drm/arm/malidp_drv.h    |    5 +
>  drivers/gpu/drm/arm/malidp_hw.c     |  104 ++++++++++----
>  drivers/gpu/drm/arm/malidp_hw.h     |   27 +++-
>  drivers/gpu/drm/arm/malidp_mw.c     |  268 +++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/arm/malidp_planes.c |    8 +-
>  drivers/gpu/drm/arm/malidp_regs.h   |   15 ++
>  drivers/gpu/drm/drm_atomic.c        |   40 ++++++
>  drivers/gpu/drm/drm_atomic_helper.c |    4 +
>  drivers/gpu/drm/drm_connector.c     |   79 ++++++++++-
>  drivers/gpu/drm/drm_crtc.c          |   14 +-
>  drivers/gpu/drm/drm_fb_helper.c     |    4 +
>  drivers/gpu/drm/drm_framebuffer.c   |  249 ++++++++++++++++++++++++++++----
>  drivers/gpu/drm/drm_ioctl.c         |    7 +
>  include/drm/drmP.h                  |    2 +
>  include/drm/drm_atomic.h            |    3 +
>  include/drm/drm_connector.h         |   15 ++
>  include/drm/drm_crtc.h              |   12 ++
>  include/uapi/drm/drm.h              |   10 ++
>  include/uapi/drm/drm_mode.h         |    1 +
>  22 files changed, 830 insertions(+), 73 deletions(-)
>  create mode 100644 drivers/gpu/drm/arm/malidp_mw.c
> 
> -- 
> 1.7.9.5
> 

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

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

#1499018

From	Brian Starkey <brian.starkey@arm.com>
Date	2016-10-11 19:00 +0200
Message-ID	<sr8zg-H0-33@gated-at.bofh.it>
In reply to	#1498978

Hi Daniel,

Firstly thanks very much for having a look.

On Tue, Oct 11, 2016 at 05:43:59PM +0200, Daniel Vetter wrote:
>On Tue, Oct 11, 2016 at 03:53:57PM +0100, Brian Starkey wrote:
>> Hi,
>>
>> This RFC series introduces a new connector type:
>>  DRM_MODE_CONNECTOR_WRITEBACK
>> It is a follow-on from a previous discussion: [1]
>>
>> Writeback connectors are used to expose the memory writeback engines
>> found in some display controllers, which can write a CRTC's
>> composition result to a memory buffer.
>> This is useful e.g. for testing, screen-recording, screenshots,
>> wireless display, display cloning, memory-to-memory composition.
>>
>> Patches 1-7 include the core framework changes required, and patches
>> 8-11 implement a writeback connector for the Mali-DP writeback engine.
>> The Mali-DP patches depend on this other series: [2].
>>
>> The connector is given the FB_ID property for the output framebuffer,
>> and two new read-only properties: PIXEL_FORMATS and
>> PIXEL_FORMATS_SIZE, which expose the supported framebuffer pixel
>> formats of the engine.
>>
>> The EDID property is not exposed for writeback connectors.
>>
>> Writeback connector usage:
>> --------------------------
>> Due to connector routing changes being treated as "full modeset"
>> operations, any client which wishes to use a writeback connector
>> should include the connector in every modeset. The writeback will not
>> actually become active until a framebuffer is attached.
>
>Erhm, this is just the default, drivers can override this. And we could
>change the atomic helpers to not mark a modeset as a modeset if the
>connector that changed is a writeback one.
>

Hmm, maybe. I don't think it's ideal - the driver would need to
re-implement drm_atomic_helper_check_modeset, which is quite a chunk
of code (along with exposing update_connector_routing, mode_fixup,
maybe others), and even after that it would have to lie and set
crtc_state->connectors_changed to false so that
drm_crtc_needs_modeset returns false to drm_atomic_check_only.

I tried to keep special-casing of writeback connectors in the core to
a bare minimum, because I think it will quickly get messy and fragile
otherwise.

Honestly, I don't see modesetting the writeback connectors at
start-of-day as a big problem.

>> The writeback itself is enabled by attaching a framebuffer to the
>> FB_ID property of the connector. The driver must then ensure that the
>> CRTC content of that atomic commit is written into the framebuffer.
>>
>> The writeback works in a one-shot mode with each atomic commit. This
>> prevents the same content from being written multiple times.
>> In some cases (front-buffer rendering) there might be a desire for
>> continuous operation - I think a property could be added later for
>> this kind of control.
>>
>> Writeback can be disabled by setting FB_ID to zero.
>
>This seems to contradict itself: If it's one-shot, there's no need to
>disable it - it will auto-disable.

I should have explained one-shot more clearly. What I mean is, one
drmModeAtomicCommit == one write to memory. This is as opposed to
writing the same thing to memory every vsync until it is stopped
(which our HW is capable of doing).

A subsequent drmModeAtomicCommit which doesn't touch the writeback 
FB_ID will write (again - but with whatever scene updates) to the same
framebuffer.

This continues for every drmModeAtomicCommit until FB_ID is set to
zero - to disable writing - or changed to a different framebuffer, in
which case we write to the new one.

IMO this behaviour makes sense in the context of the rest of Atomic,
and as the FB_ID is indeed persistent across atomic commits, I think
it should be read-able.

>
>In other cases where we write a property as a one-shot thing (fences for
>android). In that case when you read that property it's always 0 (well, -1
>for fences since file descriptor). That also avoids the issues when
>userspace unconditionally saves/restores all properties (this is needed
>for generic compositor switching).
>
>I think a better behaviour would be to do the same trick, with FB_ID on
>the connector always returning 0 as the current value. That encodes the
>one-shot behaviour directly.
>
>For one-shot vs continuous: Maybe we want to simply have a separate
>writeback property for continues, e.g. FB_WRITEBACK_ONE_SHOT_ID and
>FB_WRITEBACK_CONTINUOUS_ID.
>
>> Known issues:
>> -------------
>>  * I'm not sure what "DPMS" should mean for writeback connectors.
>>    It could be used to disable writeback (even when a framebuffer is
>>    attached), or it could be hidden entirely (which would break the
>>    legacy DPMS call for writeback connectors).
>
>dpms is legacy, in atomic land the only thing you have is "ACTIVE" on the
>crtc. it disables everything, i.e. also writeback.
>

So removing the DPMS property is a viable option for writeback 
connectors in your opinion?

>>  * With Daniel's recent re-iteration of the userspace API rules, I
>>    fully expect to provide some userspace code to support this. The
>>    question is what, and where? We want to use writeback for testing,
>>    so perhaps some tests in igt is suitable.
>
>Hm, testing would be better as a debugfs interface, but I understand the
>appeal of doing this with atomic (since semantics fit so well). Another
>use-case of this is compositing, but if the main goal is igt and testing,
>I think integration into igt crc based testcases is a perfectly fine
>userspace.
>
>>  * Documentation. Probably some portion of this cover letter needs 
>>  to
>>    make it into Documentation/
>
>Yeah, an overview DOC: section in a separate source file (with all the the
>infrastructure work) would be great - aka needed from my pov ;-)
>

Sure, I'll a look at splitting into a drm_writeback.c

>>  * Synchronisation. Our hardware will finish the writeback by the next
>>    vsync. I've not implemented fence support here, but it would be an
>>    obvious addition.
>
>Probably just want an additional WRITEBACK_FENCE_ID property to signal
>completion. Some hw definitely will take longer to write back than just a
>vblank. But we can delay that until it's needed.
>-Daniel
>
>>
>> See Also:
>> ---------
>> [1] https://lists.freedesktop.org/archives/dri-devel/2016-July/113197.html
>> [2] https://lists.freedesktop.org/archives/dri-devel/2016-October/120486.html
>>
>> I welcome any comments, especially if this approach does/doesn't fit
>> well with anyone else's hardware.
>>
>> Thanks,
>>
>> -Brian
>>
>> ---
>>
>> Brian Starkey (10):
>>   drm: add writeback connector type
>>   drm/fb-helper: skip writeback connectors
>>   drm: extract CRTC/plane disable from drm_framebuffer_remove
>>   drm: add __drm_framebuffer_remove_atomic
>>   drm: add fb to connector state
>>   drm: expose fb_id property for writeback connectors
>>   drm: add writeback-connector pixel format properties
>>   drm: mali-dp: rename malidp_input_format
>>   drm: mali-dp: add RGB writeback formats for DP550/DP650
>>   drm: mali-dp: add writeback connector
>>
>> Liviu Dudau (1):
>>   drm: mali-dp: Add support for writeback on DP550/DP650
>>
>>  drivers/gpu/drm/arm/Makefile        |    1 +
>>  drivers/gpu/drm/arm/malidp_crtc.c   |   10 ++
>>  drivers/gpu/drm/arm/malidp_drv.c    |   25 +++-
>>  drivers/gpu/drm/arm/malidp_drv.h    |    5 +
>>  drivers/gpu/drm/arm/malidp_hw.c     |  104 ++++++++++----
>>  drivers/gpu/drm/arm/malidp_hw.h     |   27 +++-
>>  drivers/gpu/drm/arm/malidp_mw.c     |  268 +++++++++++++++++++++++++++++++++++
>>  drivers/gpu/drm/arm/malidp_planes.c |    8 +-
>>  drivers/gpu/drm/arm/malidp_regs.h   |   15 ++
>>  drivers/gpu/drm/drm_atomic.c        |   40 ++++++
>>  drivers/gpu/drm/drm_atomic_helper.c |    4 +
>>  drivers/gpu/drm/drm_connector.c     |   79 ++++++++++-
>>  drivers/gpu/drm/drm_crtc.c          |   14 +-
>>  drivers/gpu/drm/drm_fb_helper.c     |    4 +
>>  drivers/gpu/drm/drm_framebuffer.c   |  249 ++++++++++++++++++++++++++++----
>>  drivers/gpu/drm/drm_ioctl.c         |    7 +
>>  include/drm/drmP.h                  |    2 +
>>  include/drm/drm_atomic.h            |    3 +
>>  include/drm/drm_connector.h         |   15 ++
>>  include/drm/drm_crtc.h              |   12 ++
>>  include/uapi/drm/drm.h              |   10 ++
>>  include/uapi/drm/drm_mode.h         |    1 +
>>  22 files changed, 830 insertions(+), 73 deletions(-)
>>  create mode 100644 drivers/gpu/drm/arm/malidp_mw.c
>>
>> --
>> 1.7.9.5
>>
>
>-- 
>Daniel Vetter
>Software Engineer, Intel Corporation
>http://blog.ffwll.ch
>

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

#1499020

From	Daniel Vetter <daniel@ffwll.ch>
Date	2016-10-11 19:10 +0200
Message-ID	<sr8IW-Zh-1@gated-at.bofh.it>
In reply to	#1499018

On Tue, Oct 11, 2016 at 6:43 PM, Brian Starkey <brian.starkey@arm.com> wrote:
> Hi Daniel,
>
> Firstly thanks very much for having a look.
>
>
> On Tue, Oct 11, 2016 at 05:43:59PM +0200, Daniel Vetter wrote:
>>
>> On Tue, Oct 11, 2016 at 03:53:57PM +0100, Brian Starkey wrote:
>>>
>>> Hi,
>>>
>>> This RFC series introduces a new connector type:
>>>  DRM_MODE_CONNECTOR_WRITEBACK
>>> It is a follow-on from a previous discussion: [1]
>>>
>>> Writeback connectors are used to expose the memory writeback engines
>>> found in some display controllers, which can write a CRTC's
>>> composition result to a memory buffer.
>>> This is useful e.g. for testing, screen-recording, screenshots,
>>> wireless display, display cloning, memory-to-memory composition.
>>>
>>> Patches 1-7 include the core framework changes required, and patches
>>> 8-11 implement a writeback connector for the Mali-DP writeback engine.
>>> The Mali-DP patches depend on this other series: [2].
>>>
>>> The connector is given the FB_ID property for the output framebuffer,
>>> and two new read-only properties: PIXEL_FORMATS and
>>> PIXEL_FORMATS_SIZE, which expose the supported framebuffer pixel
>>> formats of the engine.
>>>
>>> The EDID property is not exposed for writeback connectors.
>>>
>>> Writeback connector usage:
>>> --------------------------
>>> Due to connector routing changes being treated as "full modeset"
>>> operations, any client which wishes to use a writeback connector
>>> should include the connector in every modeset. The writeback will not
>>> actually become active until a framebuffer is attached.
>>
>>
>> Erhm, this is just the default, drivers can override this. And we could
>> change the atomic helpers to not mark a modeset as a modeset if the
>> connector that changed is a writeback one.
>>
>
> Hmm, maybe. I don't think it's ideal - the driver would need to
> re-implement drm_atomic_helper_check_modeset, which is quite a chunk
> of code (along with exposing update_connector_routing, mode_fixup,
> maybe others), and even after that it would have to lie and set
> crtc_state->connectors_changed to false so that
> drm_crtc_needs_modeset returns false to drm_atomic_check_only.

You only need to update the property in your encoders's ->atomic_check
function. No need for more, and I think being consistent with
computing when you need a modeset is really a crucial part of the
atomic ioctl that we should imo try to implement correctly as much as
possible.

> I tried to keep special-casing of writeback connectors in the core to
> a bare minimum, because I think it will quickly get messy and fragile
> otherwise.

Please always make the disdinction between core and shared drm
helpers. Special cases in core == probably not good. Special cases in
helpers == perfectly fine imo.

> Honestly, I don't see modesetting the writeback connectors at
> start-of-day as a big problem.

It's inconsistent. Claiming it needs a modeset when it doesn't isn't
great. Making that more discoverable to userspace is the entire point
of atomic. And there might be hw where reconfiguring for writeback
might need a full modeset.

>>> The writeback itself is enabled by attaching a framebuffer to the
>>> FB_ID property of the connector. The driver must then ensure that the
>>> CRTC content of that atomic commit is written into the framebuffer.
>>>
>>> The writeback works in a one-shot mode with each atomic commit. This
>>> prevents the same content from being written multiple times.
>>> In some cases (front-buffer rendering) there might be a desire for
>>> continuous operation - I think a property could be added later for
>>> this kind of control.
>>>
>>> Writeback can be disabled by setting FB_ID to zero.
>>
>>
>> This seems to contradict itself: If it's one-shot, there's no need to
>> disable it - it will auto-disable.
>
>
> I should have explained one-shot more clearly. What I mean is, one
> drmModeAtomicCommit == one write to memory. This is as opposed to
> writing the same thing to memory every vsync until it is stopped
> (which our HW is capable of doing).
>
> A subsequent drmModeAtomicCommit which doesn't touch the writeback FB_ID
> will write (again - but with whatever scene updates) to the same
> framebuffer.
>
> This continues for every drmModeAtomicCommit until FB_ID is set to
> zero - to disable writing - or changed to a different framebuffer, in
> which case we write to the new one.
>
> IMO this behaviour makes sense in the context of the rest of Atomic,
> and as the FB_ID is indeed persistent across atomic commits, I think
> it should be read-able.

tbh I don't like that, I think it'd be better to make this truly
one-shot. Otherwise we'll have real fun problems with hw where the
writeback can take longer than a vblank (it happens ...). So one-shot,
with auto-clearing to NULL/0 is imo the right approach.

>> In other cases where we write a property as a one-shot thing (fences for
>> android). In that case when you read that property it's always 0 (well, -1
>> for fences since file descriptor). That also avoids the issues when
>> userspace unconditionally saves/restores all properties (this is needed
>> for generic compositor switching).
>>
>> I think a better behaviour would be to do the same trick, with FB_ID on
>> the connector always returning 0 as the current value. That encodes the
>> one-shot behaviour directly.
>>
>> For one-shot vs continuous: Maybe we want to simply have a separate
>> writeback property for continues, e.g. FB_WRITEBACK_ONE_SHOT_ID and
>> FB_WRITEBACK_CONTINUOUS_ID.
>>
>>> Known issues:
>>> -------------
>>>  * I'm not sure what "DPMS" should mean for writeback connectors.
>>>    It could be used to disable writeback (even when a framebuffer is
>>>    attached), or it could be hidden entirely (which would break the
>>>    legacy DPMS call for writeback connectors).
>>
>>
>> dpms is legacy, in atomic land the only thing you have is "ACTIVE" on the
>> crtc. it disables everything, i.e. also writeback.
>>
>
> So removing the DPMS property is a viable option for writeback connectors in
> your opinion?

Nah, that's part of the abi now. But atomic internally remaps it to
"ACTIVE", in short you don't need to care (as long as you fill out the
dpms hook with the provided helper. drm_writeback_connector_init
should probably do that).

Cheers, Daniel

>>>  * With Daniel's recent re-iteration of the userspace API rules, I
>>>    fully expect to provide some userspace code to support this. The
>>>    question is what, and where? We want to use writeback for testing,
>>>    so perhaps some tests in igt is suitable.
>>
>>
>> Hm, testing would be better as a debugfs interface, but I understand the
>> appeal of doing this with atomic (since semantics fit so well). Another
>> use-case of this is compositing, but if the main goal is igt and testing,
>> I think integration into igt crc based testcases is a perfectly fine
>> userspace.
>>
>>>  * Documentation. Probably some portion of this cover letter needs  to
>>>    make it into Documentation/
>>
>>
>> Yeah, an overview DOC: section in a separate source file (with all the the
>> infrastructure work) would be great - aka needed from my pov ;-)
>>
>
> Sure, I'll a look at splitting into a drm_writeback.c
>
>
>>>  * Synchronisation. Our hardware will finish the writeback by the next
>>>    vsync. I've not implemented fence support here, but it would be an
>>>    obvious addition.
>>
>>
>> Probably just want an additional WRITEBACK_FENCE_ID property to signal
>> completion. Some hw definitely will take longer to write back than just a
>> vblank. But we can delay that until it's needed.
>> -Daniel
>>
>>>
>>> See Also:
>>> ---------
>>> [1]
>>> https://lists.freedesktop.org/archives/dri-devel/2016-July/113197.html
>>> [2]
>>> https://lists.freedesktop.org/archives/dri-devel/2016-October/120486.html
>>>
>>> I welcome any comments, especially if this approach does/doesn't fit
>>> well with anyone else's hardware.
>>>
>>> Thanks,
>>>
>>> -Brian
>>>
>>> ---
>>>
>>> Brian Starkey (10):
>>>   drm: add writeback connector type
>>>   drm/fb-helper: skip writeback connectors
>>>   drm: extract CRTC/plane disable from drm_framebuffer_remove
>>>   drm: add __drm_framebuffer_remove_atomic
>>>   drm: add fb to connector state
>>>   drm: expose fb_id property for writeback connectors
>>>   drm: add writeback-connector pixel format properties
>>>   drm: mali-dp: rename malidp_input_format
>>>   drm: mali-dp: add RGB writeback formats for DP550/DP650
>>>   drm: mali-dp: add writeback connector
>>>
>>> Liviu Dudau (1):
>>>   drm: mali-dp: Add support for writeback on DP550/DP650
>>>
>>>  drivers/gpu/drm/arm/Makefile        |    1 +
>>>  drivers/gpu/drm/arm/malidp_crtc.c   |   10 ++
>>>  drivers/gpu/drm/arm/malidp_drv.c    |   25 +++-
>>>  drivers/gpu/drm/arm/malidp_drv.h    |    5 +
>>>  drivers/gpu/drm/arm/malidp_hw.c     |  104 ++++++++++----
>>>  drivers/gpu/drm/arm/malidp_hw.h     |   27 +++-
>>>  drivers/gpu/drm/arm/malidp_mw.c     |  268
>>> +++++++++++++++++++++++++++++++++++
>>>  drivers/gpu/drm/arm/malidp_planes.c |    8 +-
>>>  drivers/gpu/drm/arm/malidp_regs.h   |   15 ++
>>>  drivers/gpu/drm/drm_atomic.c        |   40 ++++++
>>>  drivers/gpu/drm/drm_atomic_helper.c |    4 +
>>>  drivers/gpu/drm/drm_connector.c     |   79 ++++++++++-
>>>  drivers/gpu/drm/drm_crtc.c          |   14 +-
>>>  drivers/gpu/drm/drm_fb_helper.c     |    4 +
>>>  drivers/gpu/drm/drm_framebuffer.c   |  249
>>> ++++++++++++++++++++++++++++----
>>>  drivers/gpu/drm/drm_ioctl.c         |    7 +
>>>  include/drm/drmP.h                  |    2 +
>>>  include/drm/drm_atomic.h            |    3 +
>>>  include/drm/drm_connector.h         |   15 ++
>>>  include/drm/drm_crtc.h              |   12 ++
>>>  include/uapi/drm/drm.h              |   10 ++
>>>  include/uapi/drm/drm_mode.h         |    1 +
>>>  22 files changed, 830 insertions(+), 73 deletions(-)
>>>  create mode 100644 drivers/gpu/drm/arm/malidp_mw.c
>>>
>>> --
>>> 1.7.9.5
>>>
>>
>> --
>> Daniel Vetter
>> Software Engineer, Intel Corporation
>> http://blog.ffwll.ch
>>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

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

#1499170

From	Brian Starkey <brian.starkey@arm.com>
Date	2016-10-11 21:50 +0200
Message-ID	<srbdL-2oc-7@gated-at.bofh.it>
In reply to	#1499020

Hi,

On Tue, Oct 11, 2016 at 07:01:33PM +0200, Daniel Vetter wrote:
>On Tue, Oct 11, 2016 at 6:43 PM, Brian Starkey <brian.starkey@arm.com> wrote:
>> Hi Daniel,
>>
>> Firstly thanks very much for having a look.
>>
>>
>> On Tue, Oct 11, 2016 at 05:43:59PM +0200, Daniel Vetter wrote:
>>>
>>> On Tue, Oct 11, 2016 at 03:53:57PM +0100, Brian Starkey wrote:
>>>>
>>>> Hi,
>>>>
>>>> This RFC series introduces a new connector type:
>>>>  DRM_MODE_CONNECTOR_WRITEBACK
>>>> It is a follow-on from a previous discussion: [1]
>>>>
>>>> Writeback connectors are used to expose the memory writeback engines
>>>> found in some display controllers, which can write a CRTC's
>>>> composition result to a memory buffer.
>>>> This is useful e.g. for testing, screen-recording, screenshots,
>>>> wireless display, display cloning, memory-to-memory composition.
>>>>
>>>> Patches 1-7 include the core framework changes required, and patches
>>>> 8-11 implement a writeback connector for the Mali-DP writeback engine.
>>>> The Mali-DP patches depend on this other series: [2].
>>>>
>>>> The connector is given the FB_ID property for the output framebuffer,
>>>> and two new read-only properties: PIXEL_FORMATS and
>>>> PIXEL_FORMATS_SIZE, which expose the supported framebuffer pixel
>>>> formats of the engine.
>>>>
>>>> The EDID property is not exposed for writeback connectors.
>>>>
>>>> Writeback connector usage:
>>>> --------------------------
>>>> Due to connector routing changes being treated as "full modeset"
>>>> operations, any client which wishes to use a writeback connector
>>>> should include the connector in every modeset. The writeback will not
>>>> actually become active until a framebuffer is attached.
>>>
>>>
>>> Erhm, this is just the default, drivers can override this. And we could
>>> change the atomic helpers to not mark a modeset as a modeset if the
>>> connector that changed is a writeback one.
>>>
>>
>> Hmm, maybe. I don't think it's ideal - the driver would need to
>> re-implement drm_atomic_helper_check_modeset, which is quite a chunk
>> of code (along with exposing update_connector_routing, mode_fixup,
>> maybe others), and even after that it would have to lie and set
>> crtc_state->connectors_changed to false so that
>> drm_crtc_needs_modeset returns false to drm_atomic_check_only.
>
>You only need to update the property in your encoders's ->atomic_check
>function. No need for more, and I think being consistent with
>computing when you need a modeset is really a crucial part of the
>atomic ioctl that we should imo try to implement correctly as much as
>possible.
>

Sorry I really don't follow. Which property? CRTC_ID?

Userspace changing CRTC_ID will change connector_state->crtc (before
we even get to a driver callback).

After that, drm_atomic_helper_check_modeset derives connectors_changed
based on the ->crtc pointers.

After that, my encoder ->atomic_check *could* clear
connectors_changed (or I could achieve the same thing by wrapping
drm_atomic_helper_check), but it seems wrong to do so, considering
that the connector routing *has* changed.

If you think changing CRTC_ID shouldn't require a full modeset, I'd
rather give drivers a ->needs_modeset callback to override the default
drm_atomic_crtc_needs_modeset behaviour, instead of "tricking" it into
returning false.

I can imagine some hardware will need a full modeset to changed the
writeback CRTC binding anyway.

>> I tried to keep special-casing of writeback connectors in the core 
>> to
>> a bare minimum, because I think it will quickly get messy and fragile
>> otherwise.
>
>Please always make the disdinction between core and shared drm
>helpers. Special cases in core == probably not good. Special cases in
>helpers == perfectly fine imo.
>
>> Honestly, I don't see modesetting the writeback connectors at
>> start-of-day as a big problem.
>
>It's inconsistent. Claiming it needs a modeset when it doesn't isn't
>great. Making that more discoverable to userspace is the entire point
>of atomic. And there might be hw where reconfiguring for writeback
>might need a full modeset.
>

I'm a little confused - what bit exactly is inconsistent?

My implementation here is consistent with other connectors.
Updating the writeback connector CRTC_ID property requires a full
modeset, the same as other connectors.

Changing the FB_ID does *not* require a full modeset, because our
hardware has no such restriction. This is analogous to updating the
FB_ID on our planes, and is consistent with the other instances of the
FB_ID property.

If there is hardware which does have a restriction on changing FB_ID, 
I think that driver must be responsible for handling it in the same
way as drivers which can't handle plane updates without a full
modeset.

Are you saying that because setting CRTC_ID on Mali-DP is a no-op, it
shouldn't require a full modeset? I'd rather somehow hard-code the
CRTC_ID for our writeback connector to have it always attached to
the CRTC in that case.

>>>> The writeback itself is enabled by attaching a framebuffer to the
>>>> FB_ID property of the connector. The driver must then ensure that the
>>>> CRTC content of that atomic commit is written into the framebuffer.
>>>>
>>>> The writeback works in a one-shot mode with each atomic commit. This
>>>> prevents the same content from being written multiple times.
>>>> In some cases (front-buffer rendering) there might be a desire for
>>>> continuous operation - I think a property could be added later for
>>>> this kind of control.
>>>>
>>>> Writeback can be disabled by setting FB_ID to zero.
>>>
>>>
>>> This seems to contradict itself: If it's one-shot, there's no need to
>>> disable it - it will auto-disable.
>>
>>
>> I should have explained one-shot more clearly. What I mean is, one
>> drmModeAtomicCommit == one write to memory. This is as opposed to
>> writing the same thing to memory every vsync until it is stopped
>> (which our HW is capable of doing).
>>
>> A subsequent drmModeAtomicCommit which doesn't touch the writeback FB_ID
>> will write (again - but with whatever scene updates) to the same
>> framebuffer.
>>
>> This continues for every drmModeAtomicCommit until FB_ID is set to
>> zero - to disable writing - or changed to a different framebuffer, in
>> which case we write to the new one.
>>
>> IMO this behaviour makes sense in the context of the rest of Atomic,
>> and as the FB_ID is indeed persistent across atomic commits, I think
>> it should be read-able.
>
>tbh I don't like that, I think it'd be better to make this truly
>one-shot. Otherwise we'll have real fun problems with hw where the
>writeback can take longer than a vblank (it happens ...). So one-shot,
>with auto-clearing to NULL/0 is imo the right approach.
>

That's an interesting point about hardware which won't finish within
one frame; but I don't see how "true one-shot" helps.

What's the expected behaviour if userspace makes a new atomic commit
with a writeback framebuffer whilst a previous writeback is ongoing?

In both cases, you either need to block or fail the commit - whether
the framebuffer gets removed when it's done is immaterial.

>>> In other cases where we write a property as a one-shot thing (fences for
>>> android). In that case when you read that property it's always 0 (well, -1
>>> for fences since file descriptor). That also avoids the issues when
>>> userspace unconditionally saves/restores all properties (this is needed
>>> for generic compositor switching).
>>>
>>> I think a better behaviour would be to do the same trick, with FB_ID on
>>> the connector always returning 0 as the current value. That encodes the
>>> one-shot behaviour directly.
>>>
>>> For one-shot vs continuous: Maybe we want to simply have a separate
>>> writeback property for continues, e.g. FB_WRITEBACK_ONE_SHOT_ID and
>>> FB_WRITEBACK_CONTINUOUS_ID.
>>>
>>>> Known issues:
>>>> -------------
>>>>  * I'm not sure what "DPMS" should mean for writeback connectors.
>>>>    It could be used to disable writeback (even when a framebuffer is
>>>>    attached), or it could be hidden entirely (which would break the
>>>>    legacy DPMS call for writeback connectors).
>>>
>>>
>>> dpms is legacy, in atomic land the only thing you have is "ACTIVE" on the
>>> crtc. it disables everything, i.e. also writeback.
>>>
>>
>> So removing the DPMS property is a viable option for writeback connectors in
>> your opinion?
>
>Nah, that's part of the abi now. But atomic internally remaps it to
>"ACTIVE", in short you don't need to care (as long as you fill out the
>dpms hook with the provided helper. drm_writeback_connector_init
>should probably do that).
>

A connector can still be DPMS-ed individually, so a CRTC can be
"ACTIVE", attached to an "OFF" writeback connector, and the writeback
connector would still be able to actively write to memory.

I'm OK with that, and it's what I already implemented, but I thought
that userspace might reasonably expect a writeback connector with DPMS
set to "OFF" to be completely inert.

Cheers,
-Brian

>Cheers, Daniel
>
>>>>  * With Daniel's recent re-iteration of the userspace API rules, I
>>>>    fully expect to provide some userspace code to support this. The
>>>>    question is what, and where? We want to use writeback for testing,
>>>>    so perhaps some tests in igt is suitable.
>>>
>>>
>>> Hm, testing would be better as a debugfs interface, but I understand the
>>> appeal of doing this with atomic (since semantics fit so well). Another
>>> use-case of this is compositing, but if the main goal is igt and testing,
>>> I think integration into igt crc based testcases is a perfectly fine
>>> userspace.
>>>
>>>>  * Documentation. Probably some portion of this cover letter needs  to
>>>>    make it into Documentation/
>>>
>>>
>>> Yeah, an overview DOC: section in a separate source file (with all the the
>>> infrastructure work) would be great - aka needed from my pov ;-)
>>>
>>
>> Sure, I'll a look at splitting into a drm_writeback.c
>>
>>
>>>>  * Synchronisation. Our hardware will finish the writeback by the next
>>>>    vsync. I've not implemented fence support here, but it would be an
>>>>    obvious addition.
>>>
>>>
>>> Probably just want an additional WRITEBACK_FENCE_ID property to signal
>>> completion. Some hw definitely will take longer to write back than just a
>>> vblank. But we can delay that until it's needed.
>>> -Daniel
>>>
>>>>
>>>> See Also:
>>>> ---------
>>>> [1]
>>>> https://lists.freedesktop.org/archives/dri-devel/2016-July/113197.html
>>>> [2]
>>>> https://lists.freedesktop.org/archives/dri-devel/2016-October/120486.html
>>>>
>>>> I welcome any comments, especially if this approach does/doesn't fit
>>>> well with anyone else's hardware.
>>>>
>>>> Thanks,
>>>>
>>>> -Brian
>>>>
>>>> ---
>>>>
>>>> Brian Starkey (10):
>>>>   drm: add writeback connector type
>>>>   drm/fb-helper: skip writeback connectors
>>>>   drm: extract CRTC/plane disable from drm_framebuffer_remove
>>>>   drm: add __drm_framebuffer_remove_atomic
>>>>   drm: add fb to connector state
>>>>   drm: expose fb_id property for writeback connectors
>>>>   drm: add writeback-connector pixel format properties
>>>>   drm: mali-dp: rename malidp_input_format
>>>>   drm: mali-dp: add RGB writeback formats for DP550/DP650
>>>>   drm: mali-dp: add writeback connector
>>>>
>>>> Liviu Dudau (1):
>>>>   drm: mali-dp: Add support for writeback on DP550/DP650
>>>>
>>>>  drivers/gpu/drm/arm/Makefile        |    1 +
>>>>  drivers/gpu/drm/arm/malidp_crtc.c   |   10 ++
>>>>  drivers/gpu/drm/arm/malidp_drv.c    |   25 +++-
>>>>  drivers/gpu/drm/arm/malidp_drv.h    |    5 +
>>>>  drivers/gpu/drm/arm/malidp_hw.c     |  104 ++++++++++----
>>>>  drivers/gpu/drm/arm/malidp_hw.h     |   27 +++-
>>>>  drivers/gpu/drm/arm/malidp_mw.c     |  268
>>>> +++++++++++++++++++++++++++++++++++
>>>>  drivers/gpu/drm/arm/malidp_planes.c |    8 +-
>>>>  drivers/gpu/drm/arm/malidp_regs.h   |   15 ++
>>>>  drivers/gpu/drm/drm_atomic.c        |   40 ++++++
>>>>  drivers/gpu/drm/drm_atomic_helper.c |    4 +
>>>>  drivers/gpu/drm/drm_connector.c     |   79 ++++++++++-
>>>>  drivers/gpu/drm/drm_crtc.c          |   14 +-
>>>>  drivers/gpu/drm/drm_fb_helper.c     |    4 +
>>>>  drivers/gpu/drm/drm_framebuffer.c   |  249
>>>> ++++++++++++++++++++++++++++----
>>>>  drivers/gpu/drm/drm_ioctl.c         |    7 +
>>>>  include/drm/drmP.h                  |    2 +
>>>>  include/drm/drm_atomic.h            |    3 +
>>>>  include/drm/drm_connector.h         |   15 ++
>>>>  include/drm/drm_crtc.h              |   12 ++
>>>>  include/uapi/drm/drm.h              |   10 ++
>>>>  include/uapi/drm/drm_mode.h         |    1 +
>>>>  22 files changed, 830 insertions(+), 73 deletions(-)
>>>>  create mode 100644 drivers/gpu/drm/arm/malidp_mw.c
>>>>
>>>> --
>>>> 1.7.9.5
>>>>
>>>
>>> --
>>> Daniel Vetter
>>> Software Engineer, Intel Corporation
>>> http://blog.ffwll.ch
>>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
>
>
>-- 
>Daniel Vetter
>Software Engineer, Intel Corporation
>+41 (0) 79 365 57 48 - http://blog.ffwll.ch
>

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

#1499181

From	Daniel Vetter <daniel@ffwll.ch>
Date	2016-10-11 22:10 +0200
Message-ID	<srbx7-2Kh-11@gated-at.bofh.it>
In reply to	#1499170

On Tue, Oct 11, 2016 at 9:44 PM, Brian Starkey <brian.starkey@arm.com> wrote:
> Hi,
>
>
> On Tue, Oct 11, 2016 at 07:01:33PM +0200, Daniel Vetter wrote:
>>
>> On Tue, Oct 11, 2016 at 6:43 PM, Brian Starkey <brian.starkey@arm.com>
>> wrote:
>>>
>>> Hi Daniel,
>>>
>>> Firstly thanks very much for having a look.
>>>
>>>
>>> On Tue, Oct 11, 2016 at 05:43:59PM +0200, Daniel Vetter wrote:
>>>>
>>>>
>>>> On Tue, Oct 11, 2016 at 03:53:57PM +0100, Brian Starkey wrote:
>>>>>
>>>>>
>>>>> Hi,
>>>>>
>>>>> This RFC series introduces a new connector type:
>>>>>  DRM_MODE_CONNECTOR_WRITEBACK
>>>>> It is a follow-on from a previous discussion: [1]
>>>>>
>>>>> Writeback connectors are used to expose the memory writeback engines
>>>>> found in some display controllers, which can write a CRTC's
>>>>> composition result to a memory buffer.
>>>>> This is useful e.g. for testing, screen-recording, screenshots,
>>>>> wireless display, display cloning, memory-to-memory composition.
>>>>>
>>>>> Patches 1-7 include the core framework changes required, and patches
>>>>> 8-11 implement a writeback connector for the Mali-DP writeback engine.
>>>>> The Mali-DP patches depend on this other series: [2].
>>>>>
>>>>> The connector is given the FB_ID property for the output framebuffer,
>>>>> and two new read-only properties: PIXEL_FORMATS and
>>>>> PIXEL_FORMATS_SIZE, which expose the supported framebuffer pixel
>>>>> formats of the engine.
>>>>>
>>>>> The EDID property is not exposed for writeback connectors.
>>>>>
>>>>> Writeback connector usage:
>>>>> --------------------------
>>>>> Due to connector routing changes being treated as "full modeset"
>>>>> operations, any client which wishes to use a writeback connector
>>>>> should include the connector in every modeset. The writeback will not
>>>>> actually become active until a framebuffer is attached.
>>>>
>>>>
>>>>
>>>> Erhm, this is just the default, drivers can override this. And we could
>>>> change the atomic helpers to not mark a modeset as a modeset if the
>>>> connector that changed is a writeback one.
>>>>
>>>
>>> Hmm, maybe. I don't think it's ideal - the driver would need to
>>> re-implement drm_atomic_helper_check_modeset, which is quite a chunk
>>> of code (along with exposing update_connector_routing, mode_fixup,
>>> maybe others), and even after that it would have to lie and set
>>> crtc_state->connectors_changed to false so that
>>> drm_crtc_needs_modeset returns false to drm_atomic_check_only.
>>
>>
>> You only need to update the property in your encoders's ->atomic_check
>> function. No need for more, and I think being consistent with
>> computing when you need a modeset is really a crucial part of the
>> atomic ioctl that we should imo try to implement correctly as much as
>> possible.
>>
>
> Sorry I really don't follow. Which property? CRTC_ID?
>
> Userspace changing CRTC_ID will change connector_state->crtc (before
> we even get to a driver callback).
>
> After that, drm_atomic_helper_check_modeset derives connectors_changed
> based on the ->crtc pointers.
>
> After that, my encoder ->atomic_check *could* clear
> connectors_changed (or I could achieve the same thing by wrapping
> drm_atomic_helper_check), but it seems wrong to do so, considering
> that the connector routing *has* changed.
>
> If you think changing CRTC_ID shouldn't require a full modeset, I'd
> rather give drivers a ->needs_modeset callback to override the default
> drm_atomic_crtc_needs_modeset behaviour, instead of "tricking" it into
> returning false.

The problem with just that is that there's lots of different things
that can feed into the overall needs_modeset variable. That's why we
split it up into multiple booleans.

So yes you're supposed to clear connectors_changed if there is some
change that you can handle without a full modeset. If you want, think
of connectors_changed as
needs_modeset_due_to_change_in_connnector_state, but that's cumbersome
to type and too long ;-)

> I can imagine some hardware will need a full modeset to changed the
> writeback CRTC binding anyway.

Yup, and then they can upgrade this again. With all these flow-control
booleans the idea is very much that helpers give a default that works
for 90% of all cases, and driver callbacks can then change it for the
other 10%.

>>> I tried to keep special-casing of writeback connectors in the core to
>>> a bare minimum, because I think it will quickly get messy and fragile
>>> otherwise.
>>
>>
>> Please always make the disdinction between core and shared drm
>> helpers. Special cases in core == probably not good. Special cases in
>> helpers == perfectly fine imo.
>>
>>> Honestly, I don't see modesetting the writeback connectors at
>>> start-of-day as a big problem.
>>
>>
>> It's inconsistent. Claiming it needs a modeset when it doesn't isn't
>> great. Making that more discoverable to userspace is the entire point
>> of atomic. And there might be hw where reconfiguring for writeback
>> might need a full modeset.
>>
>
> I'm a little confused - what bit exactly is inconsistent?

Not being truthful for when you need a modeset and when not.

> My implementation here is consistent with other connectors.
> Updating the writeback connector CRTC_ID property requires a full
> modeset, the same as other connectors.

It's not about consistency with other implementations, it's about
consistency with what your hw can do. E.g. i915 clears
crtc_state->mode_changed when we can do a mode change without a full
modeset. The goal of atomic is to expose the full features of each hw
(including all quirks), not reduce it all to a least common set of
shared features.

> Changing the FB_ID does *not* require a full modeset, because our
> hardware has no such restriction. This is analogous to updating the
> FB_ID on our planes, and is consistent with the other instances of the
> FB_ID property.

Well that's inconsistent with connector properties, because in general
they all do require a full modeset to change ;-) I.e. consistency with
other drivers really isn't a good argument.

> If there is hardware which does have a restriction on changing FB_ID, I
> think that driver must be responsible for handling it in the same
> way as drivers which can't handle plane updates without a full
> modeset.
>
> Are you saying that because setting CRTC_ID on Mali-DP is a no-op, it
> shouldn't require a full modeset? I'd rather somehow hard-code the
> CRTC_ID for our writeback connector to have it always attached to
> the CRTC in that case.

Yup, I think if changing the CRTC_ID of the writeback connector
doesn't require a modeset, then your driver better not require a full
modeset to do that change. Maybe there's only one writeback port, and
userspace wants to move it around. And if the hw supports that without
a full modeset, then I think we should allow that. I also think that
most hw will get away with changing the writeback routing without
doing a full modeset. I might be mistaken about that though. And if
it's not clear-cut we could add a new writeback_changed boolean to
track this.

And from a user experience pov I really think we should avoid modesets
like the plague. Plugging in a chromecast stick and then watching how
your panel flickers is just not nice.

>>>>> The writeback itself is enabled by attaching a framebuffer to the
>>>>> FB_ID property of the connector. The driver must then ensure that the
>>>>> CRTC content of that atomic commit is written into the framebuffer.
>>>>>
>>>>> The writeback works in a one-shot mode with each atomic commit. This
>>>>> prevents the same content from being written multiple times.
>>>>> In some cases (front-buffer rendering) there might be a desire for
>>>>> continuous operation - I think a property could be added later for
>>>>> this kind of control.
>>>>>
>>>>> Writeback can be disabled by setting FB_ID to zero.
>>>>
>>>>
>>>>
>>>> This seems to contradict itself: If it's one-shot, there's no need to
>>>> disable it - it will auto-disable.
>>>
>>>
>>>
>>> I should have explained one-shot more clearly. What I mean is, one
>>> drmModeAtomicCommit == one write to memory. This is as opposed to
>>> writing the same thing to memory every vsync until it is stopped
>>> (which our HW is capable of doing).
>>>
>>> A subsequent drmModeAtomicCommit which doesn't touch the writeback FB_ID
>>> will write (again - but with whatever scene updates) to the same
>>> framebuffer.
>>>
>>> This continues for every drmModeAtomicCommit until FB_ID is set to
>>> zero - to disable writing - or changed to a different framebuffer, in
>>> which case we write to the new one.
>>>
>>> IMO this behaviour makes sense in the context of the rest of Atomic,
>>> and as the FB_ID is indeed persistent across atomic commits, I think
>>> it should be read-able.
>>
>>
>> tbh I don't like that, I think it'd be better to make this truly
>> one-shot. Otherwise we'll have real fun problems with hw where the
>> writeback can take longer than a vblank (it happens ...). So one-shot,
>> with auto-clearing to NULL/0 is imo the right approach.
>>
>
> That's an interesting point about hardware which won't finish within
> one frame; but I don't see how "true one-shot" helps.
>
> What's the expected behaviour if userspace makes a new atomic commit
> with a writeback framebuffer whilst a previous writeback is ongoing?
>
> In both cases, you either need to block or fail the commit - whether
> the framebuffer gets removed when it's done is immaterial.

See Eric's question. We need to define that, and I think the simplest
approach is a completion fence/sync_file. It's destaged now in 4.9, we
can use them. I think the simplest uabi would be a pointer property
(u64) where we write the fd of the fence we'll signal when write-out
completes.

>>>> In other cases where we write a property as a one-shot thing (fences for
>>>> android). In that case when you read that property it's always 0 (well,
>>>> -1
>>>> for fences since file descriptor). That also avoids the issues when
>>>> userspace unconditionally saves/restores all properties (this is needed
>>>> for generic compositor switching).
>>>>
>>>> I think a better behaviour would be to do the same trick, with FB_ID on
>>>> the connector always returning 0 as the current value. That encodes the
>>>> one-shot behaviour directly.
>>>>
>>>> For one-shot vs continuous: Maybe we want to simply have a separate
>>>> writeback property for continues, e.g. FB_WRITEBACK_ONE_SHOT_ID and
>>>> FB_WRITEBACK_CONTINUOUS_ID.
>>>>
>>>>> Known issues:
>>>>> -------------
>>>>>  * I'm not sure what "DPMS" should mean for writeback connectors.
>>>>>    It could be used to disable writeback (even when a framebuffer is
>>>>>    attached), or it could be hidden entirely (which would break the
>>>>>    legacy DPMS call for writeback connectors).
>>>>
>>>>
>>>>
>>>> dpms is legacy, in atomic land the only thing you have is "ACTIVE" on
>>>> the
>>>> crtc. it disables everything, i.e. also writeback.
>>>>
>>>
>>> So removing the DPMS property is a viable option for writeback connectors
>>> in
>>> your opinion?
>>
>>
>> Nah, that's part of the abi now. But atomic internally remaps it to
>> "ACTIVE", in short you don't need to care (as long as you fill out the
>> dpms hook with the provided helper. drm_writeback_connector_init
>> should probably do that).
>>
>
> A connector can still be DPMS-ed individually, so a CRTC can be
> "ACTIVE", attached to an "OFF" writeback connector, and the writeback
> connector would still be able to actively write to memory.

Yes, but atomic drivers ignore that. You should too. I won't take
patches which create special behaviour for dpms on the writeback
connector. If you want to change the writeback separately, then we can
change the CRTC_ID of the writeback connector. And the driver should
report correctly whether that needs a modeset or not.

> I'm OK with that, and it's what I already implemented, but I thought
> that userspace might reasonably expect a writeback connector with DPMS
> set to "OFF" to be completely inert.

Nope, DPMS turned out to be a mistake in kms (no one supports the
intermediate stages, they don't make sense) and we nerfed it in
atomic. Please don't resurrect zombies ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

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

#1499212

From	Brian Starkey <brian.starkey@arm.com>
Date	2016-10-11 23:30 +0200
Message-ID	<srcMx-3ql-1@gated-at.bofh.it>
In reply to	#1499181

On Tue, Oct 11, 2016 at 10:02:43PM +0200, Daniel Vetter wrote:
>On Tue, Oct 11, 2016 at 9:44 PM, Brian Starkey <brian.starkey@arm.com> wrote:
>> Hi,
>>
>>
>> On Tue, Oct 11, 2016 at 07:01:33PM +0200, Daniel Vetter wrote:
>>>
>>> On Tue, Oct 11, 2016 at 6:43 PM, Brian Starkey <brian.starkey@arm.com>
>>> wrote:
>>>>
>>>> Hi Daniel,
>>>>
>>>> Firstly thanks very much for having a look.
>>>>
>>>>
>>>> On Tue, Oct 11, 2016 at 05:43:59PM +0200, Daniel Vetter wrote:
>>>>>
>>>>>
>>>>> On Tue, Oct 11, 2016 at 03:53:57PM +0100, Brian Starkey wrote:
>>>>>>
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> This RFC series introduces a new connector type:
>>>>>>  DRM_MODE_CONNECTOR_WRITEBACK
>>>>>> It is a follow-on from a previous discussion: [1]
>>>>>>
>>>>>> Writeback connectors are used to expose the memory writeback engines
>>>>>> found in some display controllers, which can write a CRTC's
>>>>>> composition result to a memory buffer.
>>>>>> This is useful e.g. for testing, screen-recording, screenshots,
>>>>>> wireless display, display cloning, memory-to-memory composition.
>>>>>>
>>>>>> Patches 1-7 include the core framework changes required, and patches
>>>>>> 8-11 implement a writeback connector for the Mali-DP writeback engine.
>>>>>> The Mali-DP patches depend on this other series: [2].
>>>>>>
>>>>>> The connector is given the FB_ID property for the output framebuffer,
>>>>>> and two new read-only properties: PIXEL_FORMATS and
>>>>>> PIXEL_FORMATS_SIZE, which expose the supported framebuffer pixel
>>>>>> formats of the engine.
>>>>>>
>>>>>> The EDID property is not exposed for writeback connectors.
>>>>>>
>>>>>> Writeback connector usage:
>>>>>> --------------------------
>>>>>> Due to connector routing changes being treated as "full modeset"
>>>>>> operations, any client which wishes to use a writeback connector
>>>>>> should include the connector in every modeset. The writeback will not
>>>>>> actually become active until a framebuffer is attached.
>>>>>
>>>>>
>>>>>
>>>>> Erhm, this is just the default, drivers can override this. And we could
>>>>> change the atomic helpers to not mark a modeset as a modeset if the
>>>>> connector that changed is a writeback one.
>>>>>
>>>>
>>>> Hmm, maybe. I don't think it's ideal - the driver would need to
>>>> re-implement drm_atomic_helper_check_modeset, which is quite a chunk
>>>> of code (along with exposing update_connector_routing, mode_fixup,
>>>> maybe others), and even after that it would have to lie and set
>>>> crtc_state->connectors_changed to false so that
>>>> drm_crtc_needs_modeset returns false to drm_atomic_check_only.
>>>
>>>
>>> You only need to update the property in your encoders's ->atomic_check
>>> function. No need for more, and I think being consistent with
>>> computing when you need a modeset is really a crucial part of the
>>> atomic ioctl that we should imo try to implement correctly as much as
>>> possible.
>>>
>>
>> Sorry I really don't follow. Which property? CRTC_ID?
>>
>> Userspace changing CRTC_ID will change connector_state->crtc (before
>> we even get to a driver callback).
>>
>> After that, drm_atomic_helper_check_modeset derives connectors_changed
>> based on the ->crtc pointers.
>>
>> After that, my encoder ->atomic_check *could* clear
>> connectors_changed (or I could achieve the same thing by wrapping
>> drm_atomic_helper_check), but it seems wrong to do so, considering
>> that the connector routing *has* changed.
>>
>> If you think changing CRTC_ID shouldn't require a full modeset, I'd
>> rather give drivers a ->needs_modeset callback to override the default
>> drm_atomic_crtc_needs_modeset behaviour, instead of "tricking" it into
>> returning false.
>
>The problem with just that is that there's lots of different things
>that can feed into the overall needs_modeset variable. That's why we
>split it up into multiple booleans.
>
>So yes you're supposed to clear connectors_changed if there is some
>change that you can handle without a full modeset. If you want, think
>of connectors_changed as
>needs_modeset_due_to_change_in_connnector_state, but that's cumbersome
>to type and too long ;-)
>

All right, got it :-). This intention wasn't clear to me from the
comments in the code.

>> I can imagine some hardware will need a full modeset to changed the
>> writeback CRTC binding anyway.
>
>Yup, and then they can upgrade this again. With all these flow-control
>booleans the idea is very much that helpers give a default that works
>for 90% of all cases, and driver callbacks can then change it for the
>other 10%.
>
>>>> I tried to keep special-casing of writeback connectors in the 
>>>> core to
>>>> a bare minimum, because I think it will quickly get messy and fragile
>>>> otherwise.
>>>
>>>
>>> Please always make the disdinction between core and shared drm
>>> helpers. Special cases in core == probably not good. Special cases in
>>> helpers == perfectly fine imo.
>>>
>>>> Honestly, I don't see modesetting the writeback connectors at
>>>> start-of-day as a big problem.
>>>
>>>
>>> It's inconsistent. Claiming it needs a modeset when it doesn't isn't
>>> great. Making that more discoverable to userspace is the entire point
>>> of atomic. And there might be hw where reconfiguring for writeback
>>> might need a full modeset.
>>>
>>
>> I'm a little confused - what bit exactly is inconsistent?
>
>Not being truthful for when you need a modeset and when not.
>
>> My implementation here is consistent with other connectors.
>> Updating the writeback connector CRTC_ID property requires a full
>> modeset, the same as other connectors.
>
>It's not about consistency with other implementations, it's about
>consistency with what your hw can do. E.g. i915 clears
>crtc_state->mode_changed when we can do a mode change without a full
>modeset. The goal of atomic is to expose the full features of each hw
>(including all quirks), not reduce it all to a least common set of
>shared features.
>

Understood, I will make sure that we don't require a modeset unless
absolutely necessary.

>> Changing the FB_ID does *not* require a full modeset, because our
>> hardware has no such restriction. This is analogous to updating the
>> FB_ID on our planes, and is consistent with the other instances of the
>> FB_ID property.
>
>Well that's inconsistent with connector properties, because in general
>they all do require a full modeset to change ;-) I.e. consistency with
>other drivers really isn't a good argument.
>
>> If there is hardware which does have a restriction on changing FB_ID, I
>> think that driver must be responsible for handling it in the same
>> way as drivers which can't handle plane updates without a full
>> modeset.
>>
>> Are you saying that because setting CRTC_ID on Mali-DP is a no-op, it
>> shouldn't require a full modeset? I'd rather somehow hard-code the
>> CRTC_ID for our writeback connector to have it always attached to
>> the CRTC in that case.
>
>Yup, I think if changing the CRTC_ID of the writeback connector
>doesn't require a modeset, then your driver better not require a full
>modeset to do that change. Maybe there's only one writeback port, and
>userspace wants to move it around. And if the hw supports that without
>a full modeset, then I think we should allow that. I also think that
>most hw will get away with changing the writeback routing without
>doing a full modeset. I might be mistaken about that though. And if
>it's not clear-cut we could add a new writeback_changed boolean to
>track this.
>
>And from a user experience pov I really think we should avoid modesets
>like the plague. Plugging in a chromecast stick and then watching how
>your panel flickers is just not nice.
>

Yup, makes sense. I think my mindset is still a bit stuck in SETCRTC-
land.

>>>>>> The writeback itself is enabled by attaching a framebuffer to the
>>>>>> FB_ID property of the connector. The driver must then ensure that the
>>>>>> CRTC content of that atomic commit is written into the framebuffer.
>>>>>>
>>>>>> The writeback works in a one-shot mode with each atomic commit. This
>>>>>> prevents the same content from being written multiple times.
>>>>>> In some cases (front-buffer rendering) there might be a desire for
>>>>>> continuous operation - I think a property could be added later for
>>>>>> this kind of control.
>>>>>>
>>>>>> Writeback can be disabled by setting FB_ID to zero.
>>>>>
>>>>>
>>>>>
>>>>> This seems to contradict itself: If it's one-shot, there's no need to
>>>>> disable it - it will auto-disable.
>>>>
>>>>
>>>>
>>>> I should have explained one-shot more clearly. What I mean is, one
>>>> drmModeAtomicCommit == one write to memory. This is as opposed to
>>>> writing the same thing to memory every vsync until it is stopped
>>>> (which our HW is capable of doing).
>>>>
>>>> A subsequent drmModeAtomicCommit which doesn't touch the writeback FB_ID
>>>> will write (again - but with whatever scene updates) to the same
>>>> framebuffer.
>>>>
>>>> This continues for every drmModeAtomicCommit until FB_ID is set to
>>>> zero - to disable writing - or changed to a different framebuffer, in
>>>> which case we write to the new one.
>>>>
>>>> IMO this behaviour makes sense in the context of the rest of Atomic,
>>>> and as the FB_ID is indeed persistent across atomic commits, I think
>>>> it should be read-able.
>>>
>>>
>>> tbh I don't like that, I think it'd be better to make this truly
>>> one-shot. Otherwise we'll have real fun problems with hw where the
>>> writeback can take longer than a vblank (it happens ...). So one-shot,
>>> with auto-clearing to NULL/0 is imo the right approach.
>>>
>>
>> That's an interesting point about hardware which won't finish within
>> one frame; but I don't see how "true one-shot" helps.
>>
>> What's the expected behaviour if userspace makes a new atomic commit
>> with a writeback framebuffer whilst a previous writeback is ongoing?
>>
>> In both cases, you either need to block or fail the commit - whether
>> the framebuffer gets removed when it's done is immaterial.
>
>See Eric's question. We need to define that, and I think the simplest
>approach is a completion fence/sync_file. It's destaged now in 4.9, we
>can use them. I think the simplest uabi would be a pointer property
>(u64) where we write the fd of the fence we'll signal when write-out
>completes.
>

That tells userspace that the previous writeback is finished, I agree 
that's needed. It doesn't define any behaviour in case userspace asks 
for another writeback before that fence fires though.

>>>>> In other cases where we write a property as a one-shot thing (fences for
>>>>> android). In that case when you read that property it's always 0 (well,
>>>>> -1
>>>>> for fences since file descriptor). That also avoids the issues when
>>>>> userspace unconditionally saves/restores all properties (this is needed
>>>>> for generic compositor switching).
>>>>>
>>>>> I think a better behaviour would be to do the same trick, with FB_ID on
>>>>> the connector always returning 0 as the current value. That encodes the
>>>>> one-shot behaviour directly.
>>>>>

I had more of a think about this. I think you're right that
one-shot-write-only makes sense for the framebuffer - at least I can't
think of a decent use case needing the persistent behaviour which
couldn't easily be achieved using the one-shot style.

Thanks!
-Brian

>>>>> For one-shot vs continuous: Maybe we want to simply have a separate
>>>>> writeback property for continues, e.g. FB_WRITEBACK_ONE_SHOT_ID and
>>>>> FB_WRITEBACK_CONTINUOUS_ID.
>>>>>
>>>>>> Known issues:
>>>>>> -------------
>>>>>>  * I'm not sure what "DPMS" should mean for writeback connectors.
>>>>>>    It could be used to disable writeback (even when a framebuffer is
>>>>>>    attached), or it could be hidden entirely (which would break the
>>>>>>    legacy DPMS call for writeback connectors).
>>>>>
>>>>>
>>>>>
>>>>> dpms is legacy, in atomic land the only thing you have is "ACTIVE" on
>>>>> the
>>>>> crtc. it disables everything, i.e. also writeback.
>>>>>
>>>>
>>>> So removing the DPMS property is a viable option for writeback connectors
>>>> in
>>>> your opinion?
>>>
>>>
>>> Nah, that's part of the abi now. But atomic internally remaps it to
>>> "ACTIVE", in short you don't need to care (as long as you fill out the
>>> dpms hook with the provided helper. drm_writeback_connector_init
>>> should probably do that).
>>>
>>
>> A connector can still be DPMS-ed individually, so a CRTC can be
>> "ACTIVE", attached to an "OFF" writeback connector, and the writeback
>> connector would still be able to actively write to memory.
>
>Yes, but atomic drivers ignore that. You should too. I won't take
>patches which create special behaviour for dpms on the writeback
>connector. If you want to change the writeback separately, then we can
>change the CRTC_ID of the writeback connector. And the driver should
>report correctly whether that needs a modeset or not.
>
>> I'm OK with that, and it's what I already implemented, but I thought
>> that userspace might reasonably expect a writeback connector with DPMS
>> set to "OFF" to be completely inert.
>
>Nope, DPMS turned out to be a mistake in kms (no one supports the
>intermediate stages, they don't make sense) and we nerfed it in
>atomic. Please don't resurrect zombies ;-)
>-Daniel
>-- 
>Daniel Vetter
>Software Engineer, Intel Corporation
>+41 (0) 79 365 57 48 - http://blog.ffwll.ch
>

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

#1499375

From	Daniel Vetter <daniel@ffwll.ch>
Date	2016-10-12 09:00 +0200
Message-ID	<srlG9-Ku-1@gated-at.bofh.it>
In reply to	#1499212

On Tue, Oct 11, 2016 at 10:24:23PM +0100, Brian Starkey wrote:
> On Tue, Oct 11, 2016 at 10:02:43PM +0200, Daniel Vetter wrote:
> > The problem with just that is that there's lots of different things
> > that can feed into the overall needs_modeset variable. That's why we
> > split it up into multiple booleans.
> > 
> > So yes you're supposed to clear connectors_changed if there is some
> > change that you can handle without a full modeset. If you want, think
> > of connectors_changed as
> > needs_modeset_due_to_change_in_connnector_state, but that's cumbersome
> > to type and too long ;-)
> > 
> 
> All right, got it :-). This intention wasn't clear to me from the
> comments in the code.

A patch to update the kernel-doc to make it clearer (there's mode_changed,
connectors_changed and active_changed, plus drm_crtc_needs_modeset) would
be awesome. I'm trying to write useful docs, but since I designed this all
I sometimes forget to make the non-obvious assumptions clear enough.

Volunteered?

> > > > tbh I don't like that, I think it'd be better to make this truly
> > > > one-shot. Otherwise we'll have real fun problems with hw where the
> > > > writeback can take longer than a vblank (it happens ...). So one-shot,
> > > > with auto-clearing to NULL/0 is imo the right approach.
> > > 
> > > That's an interesting point about hardware which won't finish within
> > > one frame; but I don't see how "true one-shot" helps.
> > > 
> > > What's the expected behaviour if userspace makes a new atomic commit
> > > with a writeback framebuffer whilst a previous writeback is ongoing?
> > > 
> > > In both cases, you either need to block or fail the commit - whether
> > > the framebuffer gets removed when it's done is immaterial.
> > 
> > See Eric's question. We need to define that, and I think the simplest
> > approach is a completion fence/sync_file. It's destaged now in 4.9, we
> > can use them. I think the simplest uabi would be a pointer property
> > (u64) where we write the fd of the fence we'll signal when write-out
> > completes.
> > 
> 
> That tells userspace that the previous writeback is finished, I agree that's
> needed. It doesn't define any behaviour in case userspace asks for another
> writeback before that fence fires though.

Hm, good point. I guess we could just state that if userspace does a
writeback, and issues a new writeback before both a) the atomic flip and
b) the write back complete fence signalled will lead to undefined
behaviour. Undefined as in: data corruption, rejected atomic commit or
anything else than a kernel crash is allowed. This is similar to doing a
page flip and starting to render to the old buffers before the flip event
signalled completion: Userspace gets the mess it asked for ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

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

Page 1 of 2 [1] 2 Next page →

csiph-web

[RFC PATCH 00/11] Introduce writeback connectors

Contents

#1498946 — [RFC PATCH 00/11] Introduce writeback connectors

#1498947 — [RFC PATCH 10/11] drm: mali-dp: Add support for writeback on DP550/DP650

#1498950 — [RFC PATCH 09/11] drm: mali-dp: Add RGB writeback formats for DP550/DP650

#1498951 — [RFC PATCH 02/11] drm/fb-helper: Skip writeback connectors

#1498990 — Re: [RFC PATCH 02/11] drm/fb-helper: Skip writeback connectors

#1499011 — Re: [RFC PATCH 02/11] drm/fb-helper: Skip writeback connectors

#1499016 — Re: [RFC PATCH 02/11] drm/fb-helper: Skip writeback connectors

#1498952 — [RFC PATCH 05/11] drm: Add fb to connector state

#1498953 — [RFC PATCH 01/11] drm: Add writeback connector type

#1498954 — [RFC PATCH 04/11] drm: Add __drm_framebuffer_remove_atomic

#1498999 — Re: [RFC PATCH 04/11] drm: Add __drm_framebuffer_remove_atomic

#1498955 — [RFC PATCH 08/11] drm: mali-dp: Rename malidp_input_format

#1498956 — [RFC PATCH 11/11] drm: mali-dp: Add writeback connector

#1498978

#1499018

#1499020

#1499170

#1499181

#1499212

#1499375