Discussion:
[RFC PATCH 00/11] Introduce writeback connectors
(too old to reply)
Brian Starkey
2016-10-11 14:54:51 UTC
Permalink
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
Brian Starkey
2016-10-11 14:54:53 UTC
Permalink
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 <***@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
Daniel Vetter
2016-10-11 15:45:01 UTC
Permalink
Post by Brian Starkey
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.
---
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
Post by Brian Starkey
+
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
Brian Starkey
2016-10-11 16:48:00 UTC
Permalink
Post by Daniel Vetter
Post by Brian Starkey
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.
---
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.
Post by Daniel Vetter
Post by Brian Starkey
+
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
Daniel Vetter
2016-10-11 16:56:21 UTC
Permalink
Post by Brian Starkey
Post by Daniel Vetter
Post by Brian Starkey
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.
---
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
Brian Starkey
2016-10-11 14:55:03 UTC
Permalink
In preparation for adding an atomic version of the disable code, extract
the actual disable operation into a separate function.

Signed-off-by: Brian Starkey <***@arm.com>
---
drivers/gpu/drm/drm_framebuffer.c | 87 +++++++++++++++++++++++--------------
1 file changed, 54 insertions(+), 33 deletions(-)

diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
index 398efd6..528f75d 100644
--- a/drivers/gpu/drm/drm_framebuffer.c
+++ b/drivers/gpu/drm/drm_framebuffer.c
@@ -795,22 +795,61 @@ void drm_framebuffer_cleanup(struct drm_framebuffer *fb)
EXPORT_SYMBOL(drm_framebuffer_cleanup);

/**
- * drm_framebuffer_remove - remove and unreference a framebuffer object
+ * __drm_framebuffer_remove - remove all usage of a framebuffer object
+ * @dev: drm device
* @fb: framebuffer to remove
*
* Scans all the CRTCs and planes in @dev's mode_config. If they're
- * using @fb, removes it, setting it to NULL. Then drops the reference to the
- * passed-in framebuffer. Might take the modeset locks.
+ * using @fb, removes it, setting it to NULL. Takes the modeset locks.
*
- * Note that this function optimizes the cleanup away if the caller holds the
- * last reference to the framebuffer. It is also guaranteed to not take the
- * modeset locks in this case.
+ * Returns:
+ * true if the framebuffer was successfully removed from use
*/
-void drm_framebuffer_remove(struct drm_framebuffer *fb)
+static bool __drm_framebuffer_remove(struct drm_device *dev, struct drm_framebuffer *fb)
{
- struct drm_device *dev;
struct drm_crtc *crtc;
struct drm_plane *plane;
+ bool ret = true;
+
+ drm_modeset_lock_all(dev);
+ /* remove from any CRTC */
+ drm_for_each_crtc(crtc, dev) {
+ if (crtc->primary->fb == fb) {
+ /* should turn off the crtc */
+ if (drm_crtc_force_disable(crtc))
+ ret = false;
+ }
+ }
+
+ drm_for_each_plane(plane, dev) {
+ if (plane->fb == fb)
+ /* TODO: Propagate error here? */
+ drm_plane_force_disable(plane);
+ }
+ drm_modeset_unlock_all(dev);
+
+ return ret;
+}
+
+/**
+ * drm_framebuffer_remove - remove and unreference a framebuffer object
+ * @fb: framebuffer to remove
+ *
+ * drm ABI mandates that we remove any deleted framebuffers from active usage.
+ * This function takes care of this detail, disabling any CRTCs/Planes which
+ * are using the framebuffer being removed.
+ *
+ * Since most sane clients only remove framebuffers they no longer need, we
+ * skip the disable step if the caller holds the last reference to the
+ * framebuffer. It is also guaranteed to not take the modeset locks in
+ * this case.
+ *
+ * Before returning this function drops (what should be) the last reference
+ * on the framebuffer.
+ */
+void drm_framebuffer_remove(struct drm_framebuffer *fb)
+{
+ struct drm_device *dev;

if (!fb)
return;
@@ -820,37 +859,19 @@ void drm_framebuffer_remove(struct drm_framebuffer *fb)
WARN_ON(!list_empty(&fb->filp_head));

/*
- * drm ABI mandates that we remove any deleted framebuffers from active
- * useage. But since most sane clients only remove framebuffers they no
- * longer need, try to optimize this away.
- *
* Since we're holding a reference ourselves, observing a refcount of 1
- * means that we're the last holder and can skip it. Also, the refcount
- * can never increase from 1 again, so we don't need any barriers or
- * locks.
+ * means that we're the last holder and can skip the disable. Also, the
+ * refcount can never increase from 1 again, so we don't need any
+ * barriers or locks.
*
- * Note that userspace could try to race with use and instate a new
+ * Note that userspace could try to race with us and instate a new
* usage _after_ we've cleared all current ones. End result will be an
* 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) {
- drm_modeset_lock_all(dev);
- /* remove from any CRTC */
- drm_for_each_crtc(crtc, dev) {
- if (crtc->primary->fb == fb) {
- /* should turn off the crtc */
- if (drm_crtc_force_disable(crtc))
- DRM_ERROR("failed to reset crtc %p when fb was deleted\n", crtc);
- }
- }
-
- drm_for_each_plane(plane, dev) {
- if (plane->fb == fb)
- drm_plane_force_disable(plane);
- }
- drm_modeset_unlock_all(dev);
- }
+ if (drm_framebuffer_read_refcount(fb) > 1)
+ if (!__drm_framebuffer_remove(dev, fb))
+ DRM_ERROR("failed to remove fb from active usage\n");

drm_framebuffer_unreference(fb);
}
--
1.7.9.5
Brian Starkey
2016-10-11 14:55:22 UTC
Permalink
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 <***@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
Brian Starkey
2016-10-11 14:55:31 UTC
Permalink
From: 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 <***@arm.com>
Signed-off-by: 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
Brian Starkey
2016-10-11 14:55:44 UTC
Permalink
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 <***@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 <***@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
Brian Starkey
2016-10-11 14:55:57 UTC
Permalink
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 <***@arm.com>
Reviewed-by: 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
Brian Starkey
2016-10-11 14:56:13 UTC
Permalink
So that userspace can determine what pixel formats are supported for a
writeback connector's framebuffer, add a pixel format list to writeback
connectors. This is in the form of an immutable blob containing an array
of formats, and an immutable uint holding the array size.

Signed-off-by: Brian Starkey <***@arm.com>
---
drivers/gpu/drm/drm_connector.c | 73 ++++++++++++++++++++++++++++++++++++++-
include/drm/drm_connector.h | 12 +++++++
include/drm/drm_crtc.h | 12 +++++++
3 files changed, 96 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index fb83870..2f1f61d 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -249,9 +249,14 @@ int drm_connector_init(struct drm_device *dev,
drm_object_attach_property(&connector->base, config->prop_crtc_id, 0);
}

- if (connector_type == DRM_MODE_CONNECTOR_WRITEBACK)
+ if (connector_type == DRM_MODE_CONNECTOR_WRITEBACK) {
drm_object_attach_property(&connector->base,
config->prop_fb_id, 0);
+ drm_object_attach_property(&connector->base,
+ config->pixel_formats_property, 0);
+ drm_object_attach_property(&connector->base,
+ config->pixel_formats_size_property, 0);
+ }

connector->debugfs_entry = NULL;
out_put_type_id:
@@ -851,6 +856,45 @@ int drm_mode_create_suggested_offset_properties(struct drm_device *dev)
EXPORT_SYMBOL(drm_mode_create_suggested_offset_properties);

/**
+ * drm_mode_create_writeback_connector_properties - create writeback connector properties
+ * @dev: DRM device
+ *
+ * Create the properties specific to writeback connectors. These will be attached
+ * to writeback connectors by drm_connector_init. Drivers can set these
+ * properties using drm_mode_connector_set_writeback_formats().
+ *
+ * "PIXEL_FORMATS":
+ * Immutable blob property to store the supported pixel formats table. The
+ * data is an array of u32 DRM_FORMAT_* fourcc values.
+ * Userspace can use this blob to find out what pixel formats are supported
+ * by the connector's writeback engine.
+ *
+ * "PIXEL_FORMATS_SIZE":
+ * Immutable unsigned range property storing the number of entries in the
+ * PIXEL_FORMATS array.
+ */
+int drm_mode_create_writeback_connector_properties(struct drm_device *dev)
+{
+ if (dev->mode_config.pixel_formats_property &&
+ dev->mode_config.pixel_formats_size_property)
+ return 0;
+
+ dev->mode_config.pixel_formats_property =
+ drm_property_create(dev, DRM_MODE_PROP_BLOB | DRM_MODE_PROP_IMMUTABLE,
+ "PIXEL_FORMATS", 0);
+
+ dev->mode_config.pixel_formats_size_property =
+ drm_property_create_range(dev, DRM_MODE_PROP_IMMUTABLE,
+ "PIXEL_FORMATS_SIZE", 0, UINT_MAX);
+
+ if (dev->mode_config.pixel_formats_property == NULL ||
+ dev->mode_config.pixel_formats_size_property == NULL)
+ return -ENOMEM;
+ return 0;
+}
+EXPORT_SYMBOL(drm_mode_create_writeback_connector_properties);
+
+/**
* drm_mode_connector_set_path_property - set tile property on connector
* @connector: connector to set property on.
* @path: path to use for property; must not be NULL.
@@ -957,6 +1001,33 @@ int drm_mode_connector_update_edid_property(struct drm_connector *connector,
}
EXPORT_SYMBOL(drm_mode_connector_update_edid_property);

+int drm_mode_connector_set_writeback_formats(struct drm_connector *connector,
+ u32 *formats,
+ unsigned int n_formats)
+{
+ struct drm_device *dev = connector->dev;
+ size_t size = n_formats * sizeof(*formats);
+ int ret;
+
+ if (connector->connector_type != DRM_MODE_CONNECTOR_WRITEBACK)
+ return -EINVAL;
+
+ ret = drm_property_replace_global_blob(dev,
+ &connector->pixel_formats_blob_ptr,
+ size,
+ formats,
+ &connector->base,
+ dev->mode_config.pixel_formats_property);
+
+ if (!ret)
+ drm_object_property_set_value(&connector->base,
+ dev->mode_config.pixel_formats_size_property,
+ n_formats);
+
+ return 0;
+}
+EXPORT_SYMBOL(drm_mode_connector_set_writeback_formats);
+
int drm_mode_connector_set_obj_prop(struct drm_mode_object *obj,
struct drm_property *property,
uint64_t value)
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 30a766a..e77ae5c 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -615,6 +615,14 @@ struct drm_connector {
*/
struct drm_property_blob *tile_blob_ptr;

+ /**
+ * @pixel_formats_blob_ptr
+ *
+ * DRM blob property data for the pixel formats list on writeback
+ * connectors
+ */
+ struct drm_property_blob *pixel_formats_blob_ptr;
+
/* should we poll this connector for connects and disconnects */
/* hot plug detectable */
#define DRM_CONNECTOR_POLL_HPD (1 << 0)
@@ -757,12 +765,16 @@ int drm_mode_create_tv_properties(struct drm_device *dev,
int drm_mode_create_scaling_mode_property(struct drm_device *dev);
int drm_mode_create_aspect_ratio_property(struct drm_device *dev);
int drm_mode_create_suggested_offset_properties(struct drm_device *dev);
+int drm_mode_create_writeback_connector_properties(struct drm_device *dev);

int drm_mode_connector_set_path_property(struct drm_connector *connector,
const char *path);
int drm_mode_connector_set_tile_property(struct drm_connector *connector);
int drm_mode_connector_update_edid_property(struct drm_connector *connector,
const struct edid *edid);
+int drm_mode_connector_set_writeback_formats(struct drm_connector *connector,
+ u32 *formats,
+ unsigned int n_formats);

/**
* drm_for_each_connector - iterate over all connectors
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 61932f5..c4a3164 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -1302,6 +1302,18 @@ struct drm_mode_config {
*/
struct drm_property *suggested_y_property;

+ /**
+ * @pixel_formats_property: Property for writeback connectors, storing
+ * an array of the supported pixel formats for the writeback engine
+ * (read-only).
+ */
+ struct drm_property *pixel_formats_property;
+ /**
+ * @pixel_formats_size_property: Property for writeback connectors,
+ * stating the size of the pixel formats array (read-only).
+ */
+ struct drm_property *pixel_formats_size_property;
+
/* dumb ioctl parameters */
uint32_t preferred_depth, prefer_shadow;
--
1.7.9.5
Daniel Vetter
2016-10-11 15:49:52 UTC
Permalink
Post by Brian Starkey
So that userspace can determine what pixel formats are supported for a
writeback connector's framebuffer, add a pixel format list to writeback
connectors. This is in the form of an immutable blob containing an array
of formats, and an immutable uint holding the array size.
I think we should have a dedicated writeback property registration
function, e.g. drm_writeback_connector_init(). That would then take the
pixel format list and everything else and make sure it's set up correctly.
For safety we might want to put a WARN_ON(type == WRITEBACK) into
drm_connector_init, to make sure no one botches this up.

Maybe even put all that into a new drm_writeback.c file, that then also
gives you a nice place to group all the documentation (including the DOC:
overview comment).
Post by Brian Starkey
---
drivers/gpu/drm/drm_connector.c | 73 ++++++++++++++++++++++++++++++++++++++-
include/drm/drm_connector.h | 12 +++++++
include/drm/drm_crtc.h | 12 +++++++
3 files changed, 96 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index fb83870..2f1f61d 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -249,9 +249,14 @@ int drm_connector_init(struct drm_device *dev,
drm_object_attach_property(&connector->base, config->prop_crtc_id, 0);
}
- if (connector_type == DRM_MODE_CONNECTOR_WRITEBACK)
+ if (connector_type == DRM_MODE_CONNECTOR_WRITEBACK) {
drm_object_attach_property(&connector->base,
config->prop_fb_id, 0);
+ drm_object_attach_property(&connector->base,
+ config->pixel_formats_property, 0);
+ drm_object_attach_property(&connector->base,
+ config->pixel_formats_size_property, 0);
+ }
connector->debugfs_entry = NULL;
@@ -851,6 +856,45 @@ int drm_mode_create_suggested_offset_properties(struct drm_device *dev)
EXPORT_SYMBOL(drm_mode_create_suggested_offset_properties);
/**
+ * drm_mode_create_writeback_connector_properties - create writeback connector properties
+ *
+ * Create the properties specific to writeback connectors. These will be attached
+ * to writeback connectors by drm_connector_init. Drivers can set these
+ * properties using drm_mode_connector_set_writeback_formats().
+ *
+ * Immutable blob property to store the supported pixel formats table. The
+ * data is an array of u32 DRM_FORMAT_* fourcc values.
+ * Userspace can use this blob to find out what pixel formats are supported
+ * by the connector's writeback engine.
+ *
+ * Immutable unsigned range property storing the number of entries in the
+ * PIXEL_FORMATS array.
+ */
+int drm_mode_create_writeback_connector_properties(struct drm_device *dev)
+{
+ if (dev->mode_config.pixel_formats_property &&
+ dev->mode_config.pixel_formats_size_property)
+ return 0;
+
+ dev->mode_config.pixel_formats_property =
+ drm_property_create(dev, DRM_MODE_PROP_BLOB | DRM_MODE_PROP_IMMUTABLE,
+ "PIXEL_FORMATS", 0);
+
+ dev->mode_config.pixel_formats_size_property =
+ drm_property_create_range(dev, DRM_MODE_PROP_IMMUTABLE,
+ "PIXEL_FORMATS_SIZE", 0, UINT_MAX);
+
+ if (dev->mode_config.pixel_formats_property == NULL ||
+ dev->mode_config.pixel_formats_size_property == NULL)
+ return -ENOMEM;
+ return 0;
+}
+EXPORT_SYMBOL(drm_mode_create_writeback_connector_properties);
+
+/**
* drm_mode_connector_set_path_property - set tile property on connector
@@ -957,6 +1001,33 @@ int drm_mode_connector_update_edid_property(struct drm_connector *connector,
}
EXPORT_SYMBOL(drm_mode_connector_update_edid_property);
+int drm_mode_connector_set_writeback_formats(struct drm_connector *connector,
+ u32 *formats,
+ unsigned int n_formats)
+{
+ struct drm_device *dev = connector->dev;
+ size_t size = n_formats * sizeof(*formats);
+ int ret;
+
+ if (connector->connector_type != DRM_MODE_CONNECTOR_WRITEBACK)
+ return -EINVAL;
+
+ ret = drm_property_replace_global_blob(dev,
+ &connector->pixel_formats_blob_ptr,
+ size,
+ formats,
+ &connector->base,
+ dev->mode_config.pixel_formats_property);
+
+ if (!ret)
+ drm_object_property_set_value(&connector->base,
+ dev->mode_config.pixel_formats_size_property,
+ n_formats);
+
+ return 0;
+}
+EXPORT_SYMBOL(drm_mode_connector_set_writeback_formats);
+
int drm_mode_connector_set_obj_prop(struct drm_mode_object *obj,
struct drm_property *property,
uint64_t value)
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 30a766a..e77ae5c 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -615,6 +615,14 @@ struct drm_connector {
*/
struct drm_property_blob *tile_blob_ptr;
+ /**
+ *
+ * DRM blob property data for the pixel formats list on writeback
+ * connectors
+ */
+ struct drm_property_blob *pixel_formats_blob_ptr;
+
/* should we poll this connector for connects and disconnects */
/* hot plug detectable */
#define DRM_CONNECTOR_POLL_HPD (1 << 0)
@@ -757,12 +765,16 @@ int drm_mode_create_tv_properties(struct drm_device *dev,
int drm_mode_create_scaling_mode_property(struct drm_device *dev);
int drm_mode_create_aspect_ratio_property(struct drm_device *dev);
int drm_mode_create_suggested_offset_properties(struct drm_device *dev);
+int drm_mode_create_writeback_connector_properties(struct drm_device *dev);
int drm_mode_connector_set_path_property(struct drm_connector *connector,
const char *path);
int drm_mode_connector_set_tile_property(struct drm_connector *connector);
int drm_mode_connector_update_edid_property(struct drm_connector *connector,
const struct edid *edid);
+int drm_mode_connector_set_writeback_formats(struct drm_connector *connector,
+ u32 *formats,
+ unsigned int n_formats);
/**
* drm_for_each_connector - iterate over all connectors
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 61932f5..c4a3164 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -1302,6 +1302,18 @@ struct drm_mode_config {
*/
struct drm_property *suggested_y_property;
+ /**
+ * an array of the supported pixel formats for the writeback engine
+ * (read-only).
I love cross-references in kernel-doc. I think mentioning
drm_writeback_connector_init here would be perfect (for both of them).
-Daniel
Post by Brian Starkey
+ */
+ struct drm_property *pixel_formats_property;
+ /**
+ * stating the size of the pixel formats array (read-only).
+ */
+ struct drm_property *pixel_formats_size_property;
+
/* dumb ioctl parameters */
uint32_t preferred_depth, prefer_shadow;
--
1.7.9.5
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
Brian Starkey
2016-10-11 14:56:34 UTC
Permalink
Expose the framebuffer for writeback connectors to userspace by
attaching the fb_id property to them.

Signed-off-by: Brian Starkey <***@arm.com>
---
drivers/gpu/drm/drm_atomic.c | 9 +++++++++
drivers/gpu/drm/drm_connector.c | 4 ++++
2 files changed, 13 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index b16b4fc..82e8e3a 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -986,12 +986,19 @@ int drm_atomic_connector_set_property(struct drm_connector *connector,
* now?) atomic writes to DPMS property:
*/
return -EINVAL;
+ } else if (property == config->prop_fb_id) {
+ struct drm_framebuffer *fb = drm_framebuffer_lookup(dev, val);
+ drm_atomic_set_fb_for_connector(state, fb);
+ if (fb)
+ drm_framebuffer_unreference(fb);
} else if (connector->funcs->atomic_set_property) {
return connector->funcs->atomic_set_property(connector,
state, property, val);
} else {
return -EINVAL;
}
+
+ return 0;
}
EXPORT_SYMBOL(drm_atomic_connector_set_property);

@@ -1022,6 +1029,8 @@ drm_atomic_connector_get_property(struct drm_connector *connector,
*val = (state->crtc) ? state->crtc->base.id : 0;
} else if (property == config->dpms_property) {
*val = connector->dpms;
+ } else if (property == config->prop_fb_id) {
+ *val = (state->fb) ? state->fb->base.id : 0;
} else if (connector->funcs->atomic_get_property) {
return connector->funcs->atomic_get_property(connector,
state, property, val);
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index 027d7a9..fb83870 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -249,6 +249,10 @@ int drm_connector_init(struct drm_device *dev,
drm_object_attach_property(&connector->base, config->prop_crtc_id, 0);
}

+ if (connector_type == DRM_MODE_CONNECTOR_WRITEBACK)
+ drm_object_attach_property(&connector->base,
+ config->prop_fb_id, 0);
+
connector->debugfs_entry = NULL;
out_put_type_id:
if (ret)
--
1.7.9.5
Brian Starkey
2016-10-11 14:56:44 UTC
Permalink
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 <***@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
Brian Starkey
2016-10-11 14:57:04 UTC
Permalink
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 <***@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
Brian Starkey
2016-10-11 14:57:15 UTC
Permalink
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 <***@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
Daniel Vetter
2016-10-11 16:08:00 UTC
Permalink
Post by Brian Starkey
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.
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
Post by Brian Starkey
---
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
+ *
+ * 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.
+ *
+ * it is removed and the CRTC/plane disabled.
+ * The legacy references are dropped and the ->fb pointers set to NULL
+ * accordingly.
+ *
+ * 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;
+
+ 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);
+
+ 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;
+
+ drm_atomic_state_clear(state);
+ drm_modeset_backoff(&ctx);
+
+ goto retry;
+}
+
+/**
* __drm_framebuffer_remove - remove all usage of a framebuffer object
@@ -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
Daniel Vetter
2016-10-11 15:44:13 UTC
Permalink
Post by Brian Starkey
Hi,
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.
--------------------------
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.
Post by Brian Starkey
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.
Post by Brian Starkey
-------------
* 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.
Post by Brian Starkey
* 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.
Post by Brian Starkey
* 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 ;-)
Post by Brian Starkey
* 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
Post by Brian Starkey
---------
[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
---
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
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
Brian Starkey
2016-10-11 16:52:48 UTC
Permalink
Hi Daniel,

Firstly thanks very much for having a look.
Post by Daniel Vetter
Post by Brian Starkey
Hi,
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.
--------------------------
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.
Post by Daniel Vetter
Post by Brian Starkey
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.
Post by Daniel Vetter
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.
Post by Brian Starkey
-------------
* 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?
Post by Daniel Vetter
Post by Brian Starkey
* 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.
Post by Brian Starkey
* 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
Post by Daniel Vetter
Post by Brian Starkey
* 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
Post by Brian Starkey
---------
[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
---
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
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
Daniel Vetter
2016-10-11 17:02:34 UTC
Permalink
Post by Brian Starkey
Hi Daniel,
Firstly thanks very much for having a look.
Post by Daniel Vetter
Post by Brian Starkey
Hi,
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.
--------------------------
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.
Post by Brian Starkey
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.
Post by Brian Starkey
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.
Post by Brian Starkey
Post by Daniel Vetter
Post by Brian Starkey
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.
Post by Brian Starkey
Post by Daniel Vetter
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.
Post by Brian Starkey
-------------
* 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
Post by Brian Starkey
Post by Daniel Vetter
Post by Brian Starkey
* 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.
Post by Brian Starkey
* 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
Post by Daniel Vetter
Post by Brian Starkey
* 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
Post by Brian Starkey
---------
[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
---
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
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
https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
Brian Starkey
2016-10-11 19:47:14 UTC
Permalink
Hi,
Post by Daniel Vetter
Post by Brian Starkey
Hi Daniel,
Firstly thanks very much for having a look.
Post by Daniel Vetter
Post by Brian Starkey
Hi,
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.
--------------------------
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.
Post by Daniel Vetter
Post by Brian Starkey
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.
Post by Brian Starkey
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.
Post by Daniel Vetter
Post by Brian Starkey
Post by Daniel Vetter
Post by Brian Starkey
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.
Post by Daniel Vetter
Post by Brian Starkey
Post by Daniel Vetter
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.
Post by Brian Starkey
-------------
* 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
Post by Daniel Vetter
Cheers, Daniel
Post by Brian Starkey
Post by Daniel Vetter
Post by Brian Starkey
* 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.
Post by Brian Starkey
* 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
Post by Daniel Vetter
Post by Brian Starkey
* 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
Post by Brian Starkey
---------
[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
---
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
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
https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
Daniel Vetter
2016-10-11 20:04:22 UTC
Permalink
Post by Brian Starkey
Hi,
Post by Daniel Vetter
Post by Brian Starkey
Hi Daniel,
Firstly thanks very much for having a look.
Post by Daniel Vetter
Post by Brian Starkey
Hi,
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.
--------------------------
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 ;-)
Post by Brian Starkey
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%.
Post by Brian Starkey
Post by Daniel Vetter
Post by Brian Starkey
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.
Post by Brian Starkey
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.
Post by Brian Starkey
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.
Post by Brian Starkey
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.
Post by Brian Starkey
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.
Post by Brian Starkey
Post by Daniel Vetter
Post by Brian Starkey
Post by Daniel Vetter
Post by Brian Starkey
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.
Post by Brian Starkey
Post by Daniel Vetter
Post by Brian Starkey
Post by Daniel Vetter
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.
Post by Brian Starkey
-------------
* 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.
Post by Brian Starkey
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
Brian Starkey
2016-10-11 21:26:21 UTC
Permalink
Post by Daniel Vetter
Post by Brian Starkey
Hi,
Post by Daniel Vetter
Post by Brian Starkey
Hi Daniel,
Firstly thanks very much for having a look.
Post by Daniel Vetter
Post by Brian Starkey
Hi,
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.
--------------------------
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.
Post by Daniel Vetter
Post by Brian Starkey
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%.
Post by Brian Starkey
Post by Daniel Vetter
Post by Brian Starkey
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.
Post by Brian Starkey
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.
Post by Brian Starkey
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.
Post by Daniel Vetter
Post by Brian Starkey
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.
Post by Brian Starkey
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.
Post by Daniel Vetter
Post by Brian Starkey
Post by Daniel Vetter
Post by Brian Starkey
Post by Daniel Vetter
Post by Brian Starkey
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.
Post by Daniel Vetter
Post by Brian Starkey
Post by Daniel Vetter
Post by Brian Starkey
Post by Daniel Vetter
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
Post by Daniel Vetter
Post by Brian Starkey
Post by Daniel Vetter
Post by Brian Starkey
Post by Daniel Vetter
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.
Post by Brian Starkey
-------------
* 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.
Post by Brian Starkey
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
Daniel Vetter
2016-10-12 06:56:46 UTC
Permalink
Post by Brian Starkey
Post by Daniel Vetter
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?
Post by Brian Starkey
Post by Daniel Vetter
Post by Brian Starkey
Post by Daniel Vetter
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
Brian Starkey
2016-10-13 09:47:29 UTC
Permalink
Add some additional comments to more explicitly describe the meaning and
usage of the three CRTC modeset detection booleans: mode_changed,
connectors_changed and active_changed.

Suggested-by: Daniel Vetter <***@ffwll.ch>
Signed-off-by: Brian Starkey <***@arm.com>
---

Hi Daniel,

I guess I asked for this one :-), please just check my understanding
is correct.

Thanks,
Brian

drivers/gpu/drm/drm_atomic_helper.c | 9 +++++----
include/drm/drm_atomic.h | 11 ++++++++++-
include/drm/drm_crtc.h | 5 +++++
3 files changed, 20 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 78ea735..fb4071a 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -458,10 +458,11 @@ mode_fixup(struct drm_atomic_state *state)
* removed from the crtc.
* crtc_state->active_changed is set when crtc_state->active changes,
* which is used for dpms.
+ * See also: drm_atomic_crtc_needs_modeset()
*
* IMPORTANT:
*
- * Drivers which update ->mode_changed (e.g. in their ->atomic_check hooks if a
+ * Drivers which set ->mode_changed (e.g. in their ->atomic_check hooks if a
* plane update can't be done without a full modeset) _must_ call this function
* afterwards after that change. It is permitted to call this function multiple
* times for the same update, e.g. when the ->atomic_check functions depend upon
@@ -510,9 +511,9 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,

for_each_connector_in_state(state, connector, connector_state, i) {
/*
- * This only sets crtc->mode_changed for routing changes,
- * drivers must set crtc->mode_changed themselves when connector
- * properties need to be updated.
+ * This only sets crtc->connectors_changed for routing changes,
+ * drivers must set crtc->connectors_changed themselves when
+ * connector properties need to be updated.
*/
ret = update_connector_routing(state, connector,
connector_state);
diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
index d9aff06..1ce255f 100644
--- a/include/drm/drm_atomic.h
+++ b/include/drm/drm_atomic.h
@@ -368,8 +368,17 @@ int __must_check drm_atomic_nonblocking_commit(struct drm_atomic_state *state);
*
* To give drivers flexibility struct &drm_crtc_state has 3 booleans to track
* whether the state CRTC changed enough to need a full modeset cycle:
- * connectors_changed, mode_changed and active_change. This helper simply
+ * connectors_changed, mode_changed and active_changed. This helper simply
* combines these three to compute the overall need for a modeset for @state.
+ *
+ * The atomic helper code sets these booleans, but drivers can and should
+ * change them appropriately to accurately represent whether a modeset is
+ * really needed. In general, drivers should avoid full modesets whenever
+ * possible.
+ *
+ * For example if the CRTC mode has changed, and the hardware is able to enact
+ * the requested mode change without going through a full modeset, the driver
+ * should clear mode_changed during its ->atomic_check.
*/
static inline bool
drm_atomic_crtc_needs_modeset(struct drm_crtc_state *state)
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index c4a3164..1f094d2 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -116,6 +116,11 @@ struct drm_plane_helper_funcs;
* never return in a failure from the ->atomic_check callback. Userspace assumes
* that a DPMS On will always succeed. In other words: @enable controls resource
* assignment, @active controls the actual hardware state.
+ *
+ * The three booleans active_changed, connectors_changed and mode_changed are
+ * intended to indicate whether a full modeset is needed, rather than strictly
+ * describing what has changed in a commit.
+ * See also: drm_atomic_crtc_needs_modeset()
*/
struct drm_crtc_state {
struct drm_crtc *crtc;
--
1.7.9.5
Alex Deucher
2016-10-13 14:55:29 UTC
Permalink
Post by Brian Starkey
Add some additional comments to more explicitly describe the meaning and
usage of the three CRTC modeset detection booleans: mode_changed,
connectors_changed and active_changed.
---
Hi Daniel,
I guess I asked for this one :-), please just check my understanding
is correct.
The whole thread was very enlightening for me with respect to those
flags as well. The patch looks good to me.
Post by Brian Starkey
Thanks,
Brian
drivers/gpu/drm/drm_atomic_helper.c | 9 +++++----
include/drm/drm_atomic.h | 11 ++++++++++-
include/drm/drm_crtc.h | 5 +++++
3 files changed, 20 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 78ea735..fb4071a 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -458,10 +458,11 @@ mode_fixup(struct drm_atomic_state *state)
* removed from the crtc.
* crtc_state->active_changed is set when crtc_state->active changes,
* which is used for dpms.
+ * See also: drm_atomic_crtc_needs_modeset()
*
*
- * Drivers which update ->mode_changed (e.g. in their ->atomic_check hooks if a
+ * Drivers which set ->mode_changed (e.g. in their ->atomic_check hooks if a
* plane update can't be done without a full modeset) _must_ call this function
* afterwards after that change. It is permitted to call this function multiple
* times for the same update, e.g. when the ->atomic_check functions depend upon
@@ -510,9 +511,9 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
for_each_connector_in_state(state, connector, connector_state, i) {
/*
- * This only sets crtc->mode_changed for routing changes,
- * drivers must set crtc->mode_changed themselves when connector
- * properties need to be updated.
+ * This only sets crtc->connectors_changed for routing changes,
+ * drivers must set crtc->connectors_changed themselves when
+ * connector properties need to be updated.
*/
ret = update_connector_routing(state, connector,
connector_state);
diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
index d9aff06..1ce255f 100644
--- a/include/drm/drm_atomic.h
+++ b/include/drm/drm_atomic.h
@@ -368,8 +368,17 @@ int __must_check drm_atomic_nonblocking_commit(struct drm_atomic_state *state);
*
* To give drivers flexibility struct &drm_crtc_state has 3 booleans to track
- * connectors_changed, mode_changed and active_change. This helper simply
+ * connectors_changed, mode_changed and active_changed. This helper simply
+ *
+ * The atomic helper code sets these booleans, but drivers can and should
+ * change them appropriately to accurately represent whether a modeset is
+ * really needed. In general, drivers should avoid full modesets whenever
+ * possible.
+ *
+ * For example if the CRTC mode has changed, and the hardware is able to enact
+ * the requested mode change without going through a full modeset, the driver
+ * should clear mode_changed during its ->atomic_check.
*/
static inline bool
drm_atomic_crtc_needs_modeset(struct drm_crtc_state *state)
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index c4a3164..1f094d2 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -116,6 +116,11 @@ struct drm_plane_helper_funcs;
* never return in a failure from the ->atomic_check callback. Userspace assumes
+ *
+ * The three booleans active_changed, connectors_changed and mode_changed are
+ * intended to indicate whether a full modeset is needed, rather than strictly
+ * describing what has changed in a commit.
+ * See also: drm_atomic_crtc_needs_modeset()
*/
struct drm_crtc_state {
struct drm_crtc *crtc;
--
1.7.9.5
_______________________________________________
dri-devel mailing list
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Daniel Vetter
2016-10-11 16:29:28 UTC
Permalink
On Tue, Oct 11, 2016 at 6:25 PM, Ville Syrjälä
Post by Brian Starkey
--------------------------
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.
I though people agreed that this sort of thing would go through v4l.
Continously writing to the same buffer isn't perhaps all that sensible
anyway, and so we'd need queueing, which is what v4l has already. Well,
I guess we might add some queueing to atomic eventually?
I guess for front buffer rendering type of thing you might have some
use for a continuous mode targeting a single fb. Though I think
peridically triggering a new write could do as well. Of course either
way would likely tear horribly, and having multiple buffers seems like
the better option
Yeah, momentarily entirely forgot about v4l. I think making FB_ID
one-shot (perhaps better to call it WRITEBACK_FB_ID to avoid
confusion) is the right thing to do, and then push everything
continuous to some form of drm/v4l integration.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
Ville Syrjälä
2016-10-11 16:29:51 UTC
Permalink
Post by Brian Starkey
Hi,
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.
--------------------------
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.
I though people agreed that this sort of thing would go through v4l.
Continously writing to the same buffer isn't perhaps all that sensible
anyway, and so we'd need queueing, which is what v4l has already. Well,
I guess we might add some queueing to atomic eventually?

I guess for front buffer rendering type of thing you might have some
use for a continuous mode targeting a single fb. Though I think
peridically triggering a new write could do as well. Of course either
way would likely tear horribly, and having multiple buffers seems like
the better option.
Post by Brian Starkey
Writeback can be disabled by setting FB_ID to zero.
-------------
* 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.
---------
[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
---
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
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
--
Ville Syrjälä
Intel OTC
Brian Starkey
2016-10-12 07:36:21 UTC
Permalink
Hi Eric,
Post by Brian Starkey
Hi,
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.
--------------------------
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.
I think this sounds great, and the interface is just right IMO.
Thanks, glad you like it! Hopefully you're equally agreeable with the
changes Daniel has been suggesting.
I don't really see a use for continuous mode -- a sequence of one-shots
makes a lot more sense because then you can know what data has changed,
which anyone trying to use the writeback buffer would need to know.
Agreed - we've never found a use for it.
Post by Brian Starkey
-------------
* 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.
My hardware won't necessarily finish by the next vsync -- it trickles
out at whatever rate it can find memory bandwidth to get the job done,
and fires an interrupt when it's finished.
Is it bounded? You presumably have to finish the write-out before you
can change any input buffers?
So I would like some definition for how syncing works. One answer would
be that these flips don't trigger their pageflip events until the
writeback is done (so I need to collect both the vsync irq and the
writeback irq before sending). Another would be that manage an
independent fence for the writeback fb, so that you still immediately
know when framebuffers from the previous scanout-only frame are idle.
I much prefer the sound of the explicit fence approach.

Hopefully we can agree that a new atomic commit can't be completed
whilst there's a writeback ongoing, otherwise managing the fence and
framebuffer lifetime sounds really tricky - they'd need to be decoupled
from the atomic_state and outlive the commit that spawned them.

Cheers,
-Brian
Also, tests for this in igt, please. Writeback in igt will give us so
much more ability to cover KMS functionality on non-Intel hardware.
Archit Taneja
2016-10-14 10:50:38 UTC
Permalink
Hi Brian,
Post by Brian Starkey
Hi,
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.
--------------------------
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.
-------------
* 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.
---------
[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 for working on this! Some points below.

- Writeback hardware generally allows us to specify the region within
the framebuffer we want to write to. It's analogous to the SRC_X/Y/W/H
plane properties. We could have similar props for the writeback
connectors, and maybe set them to the FB_ID dimensions if they aren't
configured by userspace.

- Besides the above property, writeback hardware can have provisions
for scaling, color space conversion and rotation. This would mean that
we'd eventually add more writeback specific props/params in
drm_connector/drm_connector_state. Would we be okay adding more such
props for connectors?

Thanks,
Archit
Post by Brian Starkey
Thanks,
-Brian
---
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
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
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
Brian Starkey
2016-10-14 12:39:58 UTC
Permalink
Hi Archit,
Post by Archit Taneja
Hi Brian,
Post by Brian Starkey
Hi,
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.
--------------------------
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.
-------------
* 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.
---------
[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 for working on this! Some points below.
- Writeback hardware generally allows us to specify the region within
the framebuffer we want to write to. It's analogous to the SRC_X/Y/W/H
plane properties. We could have similar props for the writeback
connectors, and maybe set them to the FB_ID dimensions if they aren't
configured by userspace.
- Besides the above property, writeback hardware can have provisions
for scaling, color space conversion and rotation. This would mean that
we'd eventually add more writeback specific props/params in
drm_connector/drm_connector_state. Would we be okay adding more such
props for connectors?
I've wondered the same thing about bloating non-writeback connectors
with writeback-specific stuff. If it does become significant, maybe
we should subclass drm_connector and add a drm_writeback_state pointer
to drm_connector_state.

Ville touched on scaling support previously, suggesting adding a
fixed_mode property (for all types of connectors) - on writeback this
would represent scaling the framebuffer, and on normal connectors it
could control output scaling (like panel-fitting).

Certainly destination coords, color-space converstion etc. are things
that are worth adding, but IMO I'd rather keep this initial
implementation small so we can enable the basic case right away. For
the most part, the additional things are "just properties" which
should be easily added later without impacting the overall interface.

Cheers,
Brian
Post by Archit Taneja
Thanks,
Archit
Post by Brian Starkey
Thanks,
-Brian
---
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
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
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
More majordomo info at http://vger.kernel.org/majordomo-info.html
Ville Syrjälä
2016-10-14 12:50:39 UTC
Permalink
Post by Brian Starkey
Hi Archit,
Post by Archit Taneja
Hi Brian,
Post by Brian Starkey
Hi,
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.
--------------------------
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.
-------------
* 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.
---------
[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 for working on this! Some points below.
- Writeback hardware generally allows us to specify the region within
the framebuffer we want to write to. It's analogous to the SRC_X/Y/W/H
plane properties. We could have similar props for the writeback
connectors, and maybe set them to the FB_ID dimensions if they aren't
configured by userspace.
- Besides the above property, writeback hardware can have provisions
for scaling, color space conversion and rotation. This would mean that
we'd eventually add more writeback specific props/params in
drm_connector/drm_connector_state. Would we be okay adding more such
props for connectors?
I've wondered the same thing about bloating non-writeback connectors
with writeback-specific stuff. If it does become significant, maybe
we should subclass drm_connector and add a drm_writeback_state pointer
to drm_connector_state.
Ville touched on scaling support previously, suggesting adding a
fixed_mode property (for all types of connectors) - on writeback this
would represent scaling the framebuffer, and on normal connectors it
could control output scaling (like panel-fitting).
We got some patches [1] posted for i915 recently that added a bunch of new
properties to control post-blending scaling, but I'm not sure I like the
approach since it seems harder to reconcile with the current way we deal
with scaling for eDP/LVDS/DSI/etc. So I'm still somewhat partial to the
fixed mode idea. Just FYI.

[1] https://lists.freedesktop.org/archives/intel-gfx/2016-August/105557.html
Post by Brian Starkey
Certainly destination coords, color-space converstion etc. are things
that are worth adding, but IMO I'd rather keep this initial
implementation small so we can enable the basic case right away. For
the most part, the additional things are "just properties" which
should be easily added later without impacting the overall interface.
Cheers,
Brian
Post by Archit Taneja
Thanks,
Archit
Post by Brian Starkey
Thanks,
-Brian
---
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
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
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Ville Syrjälä
Intel OTC
Loading...