Skip to contentSkip to conversation

Armada DRM fixes

PATCH New Russell King - ARM Linux 7 participants 42 messages Dec 8, 2017 – Jul 20, 2018

Subsystems (2)
lore
Share & tools
lore

Russell King - ARM Linux

author maintainer
#0 lore raw

v2 cover Armada DRM fixes

Hi David,

This series fixes some issues in the Armada DRM driver:
- A memory leak while cleaning up in the CRTC initialisation path
- Avoid powering down YUV FIFOs when disabling the graphics plane
- Several fixes for the overlay plane positioning

Minor updates for the comments from Sean Paul, and applies cleanly to your drm-fixes branch, but otherwise the same.

Thanks.

 drivers/gpu/drm/armada/armada_crtc.c    | 47 +++++++++++++++++++--------------
 drivers/gpu/drm/armada/armada_crtc.h    |  2 ++
 drivers/gpu/drm/armada/armada_overlay.c | 38 +++++++++++++-------------
 3 files changed, 48 insertions(+), 39 deletions(-)
Signature · 7 lines
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
#1 lore raw

PATCH 1/5 drm/armada: fix leak of crtc structure

Fix the leak of the CRTC structure in the failure paths of armada_drm_crtc_create().

---
 drivers/gpu/drm/armada/armada_crtc.c | 25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)
drivers/gpu/drm/armada/armada_crtc.c +13 -12
index 2a4d163ac76f..79ce877bf45f 100644
@@ -1225,17 +1225,13 @@ static int armada_drm_crtc_create(struct drm_device *drm, struct device *dev,
 
 	ret = devm_request_irq(dev, irq, armada_drm_irq, 0, "armada_drm_crtc",
 			       dcrtc);
-	if (ret < 0) {
-		kfree(dcrtc);
-		return ret;
-	}
+	if (ret < 0)
+		goto err_crtc;
 
 	if (dcrtc->variant->init) {
 		ret = dcrtc->variant->init(dcrtc, dev);
-		if (ret) {
-			kfree(dcrtc);
-			return ret;
-		}
+		if (ret)
+			goto err_crtc;
 	}
 
 	/* Ensure AXI pipeline is enabled */
@@ -1246,13 +1242,15 @@ static int armada_drm_crtc_create(struct drm_device *drm, struct device *dev,
 	dcrtc->crtc.port = port;
 
 	primary = kzalloc(sizeof(*primary), GFP_KERNEL);
-	if (!primary)
-		return -ENOMEM;
+	if (!primary) {
+		ret = -ENOMEM;
+		goto err_crtc;
+	}
 
 	ret = armada_drm_plane_init(primary);
 	if (ret) {
 		kfree(primary);
-		return ret;
+		goto err_crtc;
 	}
 
 	ret = drm_universal_plane_init(drm, &primary->base, 0,
@@ -1263,7 +1261,7 @@ static int armada_drm_crtc_create(struct drm_device *drm, struct device *dev,
 				       DRM_PLANE_TYPE_PRIMARY, NULL);
 	if (ret) {
 		kfree(primary);
-		return ret;
+		goto err_crtc;
 	}
 
 	ret = drm_crtc_init_with_planes(drm, &dcrtc->crtc, &primary->base, NULL,
@@ -1282,6 +1280,9 @@ static int armada_drm_crtc_create(struct drm_device *drm, struct device *dev,
 
 err_crtc_init:
 	primary->base.funcs->destroy(&primary->base);
+err_crtc:
+	kfree(dcrtc);
+
 	return ret;
 }
 
Signature · 6 lines
2.7.4

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
#2 lore raw

PATCH 2/5 drm/armada: fix SRAM powerdown

In reply to Russell King - ARM Linux

Avoid powering down the overlay SRAM banks when disabling the primary plane, thereby masking any overlay video. This feature is supposed to allow us to cut the bandwidth required while displaying full-frame overlay video.

---
 drivers/gpu/drm/armada/armada_crtc.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)
drivers/gpu/drm/armada/armada_crtc.c +4 -5
index 79ce877bf45f..77fc038cbfae 100644
@@ -744,15 +744,14 @@ void armada_drm_crtc_plane_disable(struct armada_crtc *dcrtc,
 	if (plane->fb)
 		drm_framebuffer_put(plane->fb);
 
-	/* Power down the Y/U/V FIFOs */
-	sram_para1 = CFG_PDWN16x66 | CFG_PDWN32x66;
-
 	/* Power down most RAMs and FIFOs if this is the primary plane */
 	if (plane->type == DRM_PLANE_TYPE_PRIMARY) {
-		sram_para1 |= CFG_PDWN256x32 | CFG_PDWN256x24 | CFG_PDWN256x8 |
-			      CFG_PDWN32x32 | CFG_PDWN64x66;
+		sram_para1 = CFG_PDWN256x32 | CFG_PDWN256x24 | CFG_PDWN256x8 |
+			     CFG_PDWN32x32 | CFG_PDWN64x66;
 		dma_ctrl0_mask = CFG_GRA_ENA;
 	} else {
+		/* Power down the Y/U/V FIFOs */
+		sram_para1 = CFG_PDWN16x66 | CFG_PDWN32x66;
 		dma_ctrl0_mask = CFG_DMA_ENA;
 	}
 
Signature · 6 lines
2.7.4

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
#3 lore raw

PATCH 3/5 drm/armada: fix UV swap code

In reply to Russell King - ARM Linux

The UV swap code was not always programming things correctly when the source origin box has been offset. Fix this.

---
 drivers/gpu/drm/armada/armada_crtc.h    |  2 ++
 drivers/gpu/drm/armada/armada_overlay.c | 38 ++++++++++++++++-----------------
 2 files changed, 21 insertions(+), 19 deletions(-)
drivers/gpu/drm/armada/armada_crtc.h +2 -0
index bab11f483575..bfd3514fbe9b 100644
@@ -42,6 +42,8 @@ struct armada_plane_work {
 };
 
 struct armada_plane_state {
+	u16 src_x;
+	u16 src_y;
 	u32 src_hw;
 	u32 dst_hw;
 	u32 dst_yx;
drivers/gpu/drm/armada/armada_overlay.c +19 -19
index edc44910d79f..72f8094a75ae 100644
@@ -99,6 +99,7 @@ armada_ovl_plane_update(struct drm_plane *plane, struct drm_crtc *crtc,
 {
 	struct armada_ovl_plane *dplane = drm_to_armada_ovl_plane(plane);
 	struct armada_crtc *dcrtc = drm_to_armada_crtc(crtc);
+	const struct drm_format_info *format;
 	struct drm_rect src = {
 		.x1 = src_x,
 		.y1 = src_y,
@@ -117,7 +118,7 @@ armada_ovl_plane_update(struct drm_plane *plane, struct drm_crtc *crtc,
 	};
 	uint32_t val, ctrl0;
 	unsigned idx = 0;
-	bool visible;
+	bool visible, fb_changed;
 	int ret;
 
 	trace_armada_ovl_plane_update(plane, crtc, fb,
@@ -138,6 +139,18 @@ armada_ovl_plane_update(struct drm_plane *plane, struct drm_crtc *crtc,
 	if (!visible)
 		ctrl0 &= ~CFG_DMA_ENA;
 
+	/*
+	 * Shifting a YUV packed format image by one pixel causes the U/V
+	 * planes to swap.  Compensate for it by also toggling the UV swap.
+	 */
+	format = fb->format;
+	if (format->num_planes == 1 && src.x1 >> 16 & (format->hsub - 1))
+		ctrl0 ^= CFG_DMA_MOD(CFG_SWAPUV);
+
+	fb_changed = plane->fb != fb ||
+		     dplane->base.state.src_x != src.x1 >> 16 ||
+	             dplane->base.state.src_y != src.y1 >> 16;
+
 	if (!dcrtc->plane) {
 		dcrtc->plane = plane;
 		armada_ovl_update_attr(&dplane->prop, dcrtc);
@@ -145,7 +158,7 @@ armada_ovl_plane_update(struct drm_plane *plane, struct drm_crtc *crtc,
 
 	/* FIXME: overlay on an interlaced display */
 	/* Just updating the position/size? */
-	if (plane->fb == fb && dplane->base.state.ctrl0 == ctrl0) {
+	if (!fb_changed && dplane->base.state.ctrl0 == ctrl0) {
 		val = (drm_rect_height(&src) & 0xffff0000) |
 		      drm_rect_width(&src) >> 16;
 		dplane->base.state.src_hw = val;
@@ -169,9 +182,8 @@ armada_ovl_plane_update(struct drm_plane *plane, struct drm_crtc *crtc,
 	if (armada_drm_plane_work_wait(&dplane->base, HZ / 25) == 0)
 		armada_drm_plane_work_cancel(dcrtc, &dplane->base);
 
-	if (plane->fb != fb) {
-		u32 addrs[3], pixel_format;
-		int num_planes, hsub;
+	if (fb_changed) {
+		u32 addrs[3];
 
 		/*
 		 * Take a reference on the new framebuffer - we want to
@@ -182,23 +194,11 @@ armada_ovl_plane_update(struct drm_plane *plane, struct drm_crtc *crtc,
 		if (plane->fb)
 			armada_ovl_retire_fb(dplane, plane->fb);
 
-		src_y = src.y1 >> 16;
-		src_x = src.x1 >> 16;
+		dplane->base.state.src_y = src_y = src.y1 >> 16;
+		dplane->base.state.src_x = src_x = src.x1 >> 16;
 
 		armada_drm_plane_calc_addrs(addrs, fb, src_x, src_y);
 
-		pixel_format = fb->format->format;
-		hsub = drm_format_horz_chroma_subsampling(pixel_format);
-		num_planes = fb->format->num_planes;
-
-		/*
-		 * Annoyingly, shifting a YUYV-format image by one pixel
-		 * causes the U/V planes to toggle.  Toggle the UV swap.
-		 * (Unfortunately, this causes momentary colour flickering.)
-		 */
-		if (src_x & (hsub - 1) && num_planes == 1)
-			ctrl0 ^= CFG_DMA_MOD(CFG_SWAPUV);
-
 		armada_reg_queue_set(dplane->vbl.regs, idx, addrs[0],
 				     LCD_SPU_DMA_START_ADDR_Y0);
 		armada_reg_queue_set(dplane->vbl.regs, idx, addrs[1],
Signature · 6 lines
2.7.4

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
#4 lore raw

PATCH 4/5 drm/armada: improve efficiency of armada_drm_plane_calc_addrs()

In reply to Russell King - ARM Linux

Lookup the drm_format_info structure once when computing all the framebuffer plane addresses by using drm_format_info(), rather than repetitive lookups via drm_format_plane_cpp().

---
 drivers/gpu/drm/armada/armada_crtc.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
drivers/gpu/drm/armada/armada_crtc.c +3 -2
index 77fc038cbfae..748de4691224 100644
@@ -168,8 +168,9 @@ static void armada_drm_crtc_update(struct armada_crtc *dcrtc)
 void armada_drm_plane_calc_addrs(u32 *addrs, struct drm_framebuffer *fb,
 	int x, int y)
 {
+	const struct drm_format_info *format = fb->format;
+	unsigned int num_planes = format->num_planes;
 	u32 addr = drm_fb_obj(fb)->dev_addr;
-	int num_planes = fb->format->num_planes;
 	int i;
 
 	if (num_planes > 3)
@@ -177,7 +178,7 @@ void armada_drm_plane_calc_addrs(u32 *addrs, struct drm_framebuffer *fb,
 
 	for (i = 0; i < num_planes; i++)
 		addrs[i] = addr + fb->offsets[i] + y * fb->pitches[i] +
-			     x * fb->format->cpp[i];
+			     x * format->cpp[i];
 	for (; i < 3; i++)
 		addrs[i] = 0;
 }
Signature · 6 lines
2.7.4

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
#5 lore raw

PATCH 5/5 drm/armada: fix YUV planar format framebuffer offsets

In reply to Russell King - ARM Linux

We weren't correctly calculating the YUV planar offsets for subsampled chroma planes correctly - fix up the coordinates for planes 1 and 2.

---
 drivers/gpu/drm/armada/armada_crtc.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)
drivers/gpu/drm/armada/armada_crtc.c +7 -1
index 748de4691224..17edd340f43e 100644
@@ -176,7 +176,13 @@ void armada_drm_plane_calc_addrs(u32 *addrs, struct drm_framebuffer *fb,
 	if (num_planes > 3)
 		num_planes = 3;
 
-	for (i = 0; i < num_planes; i++)
+	addrs[0] = addr + fb->offsets[0] + y * fb->pitches[0] +
+		   x * format->cpp[0];
+
+	y /= format->vsub;
+	x /= format->hsub;
+
+	for (i = 1; i < num_planes; i++)
 		addrs[i] = addr + fb->offsets[i] + y * fb->pitches[i] +
 			     x * format->cpp[i];
 	for (; i < 3; i++)
Signature · 6 lines
2.7.4

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
author maintainer
#6 lore raw

cover Armada DRM development for 4.16

In reply to Russell King - ARM Linux

Hi David,

This series builds upon the set of fixes previously submitted to move Armada DRM closer to atomic modeset. We're nowhere near yet, but this series helps to get us closer by unifying some of the differences between the primary and overlay planes.

New features added allows userspace to disable the primary plane if overlay is full screen and there's nothing obscuring the colorkey - this saves having to fetch an entire buffer containing nothing but colorkey when displaying full screen video.

Also we attach a property to the overlay plane to describe the colorimetry, which overrides the previous CRTC property doing the same thing. This is more in keeping with atomic modeset, and also means that the property becomes part of the metadata for the buffer it concerns.

 drivers/gpu/drm/armada/armada_crtc.c    | 420 ++++++++++++++++++++++++--------
 drivers/gpu/drm/armada/armada_crtc.h    |  26 +-
 drivers/gpu/drm/armada/armada_drm.h     |   1 +
 drivers/gpu/drm/armada/armada_overlay.c | 305 +++++++++++------------
 drivers/gpu/drm/armada/armada_trace.h   |  24 +-
 5 files changed, 507 insertions(+), 269 deletions(-)
Signature · 7 lines
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
#7 lore raw

PATCH 1/25 drm/armada: remove armada_drm_plane_work_cancel() return value

armada_drm_plane_work_cancel()'s returned work structure is never used or referenced, so it's pointless returning it. It's also pointless because the caller doesn't have a clue what kind of work structure it is.

---
 drivers/gpu/drm/armada/armada_crtc.c | 8 +++-----
 drivers/gpu/drm/armada/armada_crtc.h | 4 ++--
 2 files changed, 5 insertions(+), 7 deletions(-)
drivers/gpu/drm/armada/armada_crtc.c +3 -5
index a0f4d2a2a481..7d2dfdfffb5e 100644
@@ -254,15 +254,13 @@ int armada_drm_plane_work_wait(struct armada_plane *plane, long timeout)
 	return wait_event_timeout(plane->frame_wait, !plane->work, timeout);
 }
 
-struct armada_plane_work *armada_drm_plane_work_cancel(
-	struct armada_crtc *dcrtc, struct armada_plane *plane)
+void armada_drm_plane_work_cancel(struct armada_crtc *dcrtc,
+	struct armada_plane *dplane)
 {
-	struct armada_plane_work *work = xchg(&plane->work, NULL);
+	struct armada_plane_work *work = xchg(&dplane->work, NULL);
 
 	if (work)
 		drm_crtc_vblank_put(&dcrtc->crtc);
-
-	return work;
 }
 
 static int armada_drm_crtc_queue_frame_work(struct armada_crtc *dcrtc,
drivers/gpu/drm/armada/armada_crtc.h +2 -2
index bfd3514fbe9b..a054527eb962 100644
@@ -62,8 +62,8 @@ int armada_drm_plane_init(struct armada_plane *plane);
 int armada_drm_plane_work_queue(struct armada_crtc *dcrtc,
 	struct armada_plane *plane, struct armada_plane_work *work);
 int armada_drm_plane_work_wait(struct armada_plane *plane, long timeout);
-struct armada_plane_work *armada_drm_plane_work_cancel(
-	struct armada_crtc *dcrtc, struct armada_plane *plane);
+void armada_drm_plane_work_cancel(struct armada_crtc *dcrtc,
+	struct armada_plane *plane);
 void armada_drm_plane_calc_addrs(u32 *addrs, struct drm_framebuffer *fb,
 	int x, int y);
 
Signature · 6 lines
2.7.4

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
#8 lore raw

PATCH 2/25 drm/armada: add a common frame work allocator

In reply to Russell King - ARM Linux

Add and use a common frame work allocator, initialising the frame work to a sane state.

---
 drivers/gpu/drm/armada/armada_crtc.c | 24 +++++++++++++++++-------
 1 file changed, 17 insertions(+), 7 deletions(-)
drivers/gpu/drm/armada/armada_crtc.c +17 -7
index 7d2dfdfffb5e..8606f6e35986 100644
@@ -293,6 +293,21 @@ static void armada_drm_crtc_complete_frame_work(struct armada_crtc *dcrtc,
 	kfree(fwork);
 }
 
+static struct armada_frame_work *armada_drm_crtc_alloc_frame_work(void)
+{
+	struct armada_frame_work *work;
+	int i = 0;
+
+	work = kzalloc(sizeof(*work), GFP_KERNEL);
+	if (!work)
+		return NULL;
+
+	work->work.fn = armada_drm_crtc_complete_frame_work;
+	armada_reg_queue_end(work->regs, i);
+
+	return work;
+}
+
 static void armada_drm_crtc_finish_fb(struct armada_crtc *dcrtc,
 	struct drm_framebuffer *fb, bool force)
 {
@@ -307,13 +322,9 @@ static void armada_drm_crtc_finish_fb(struct armada_crtc *dcrtc,
 		return;
 	}
 
-	work = kmalloc(sizeof(*work), GFP_KERNEL);
+	work = armada_drm_crtc_alloc_frame_work();
 	if (work) {
-		int i = 0;
-		work->work.fn = armada_drm_crtc_complete_frame_work;
-		work->event = NULL;
 		work->old_fb = fb;
-		armada_reg_queue_end(work->regs, i);
 
 		if (armada_drm_crtc_queue_frame_work(dcrtc, work) == 0)
 			return;
@@ -1033,11 +1044,10 @@ static int armada_drm_crtc_page_flip(struct drm_crtc *crtc,
 	if (fb->format != crtc->primary->fb->format)
 		return -EINVAL;
 
-	work = kmalloc(sizeof(*work), GFP_KERNEL);
+	work = armada_drm_crtc_alloc_frame_work();
 	if (!work)
 		return -ENOMEM;
 
-	work->work.fn = armada_drm_crtc_complete_frame_work;
 	work->event = event;
 	work->old_fb = dcrtc->crtc.primary->fb;
 
Signature · 6 lines
2.7.4

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
#9 lore raw

PATCH 3/25 drm/armada: store plane in armada_plane_work

In reply to Russell King - ARM Linux

Store the plane in the armada_plane_work structure rather than passing it around; it doesn't get used very much in the work structures, so passing it around is a needless expense.

---
 drivers/gpu/drm/armada/armada_crtc.c    | 27 +++++++++++----------------
 drivers/gpu/drm/armada/armada_crtc.h    |  7 +++----
 drivers/gpu/drm/armada/armada_overlay.c | 12 +++++++-----
 3 files changed, 21 insertions(+), 25 deletions(-)
drivers/gpu/drm/armada/armada_crtc.c +11 -16
index 8606f6e35986..4d3db441466e 100644
@@ -224,7 +224,7 @@ static void armada_drm_plane_work_run(struct armada_crtc *dcrtc,
 
 	/* Handle any pending frame work. */
 	if (work) {
-		work->fn(dcrtc, dplane, work);
+		work->fn(dcrtc, work);
 		drm_crtc_vblank_put(&dcrtc->crtc);
 	}
 
@@ -232,8 +232,9 @@ static void armada_drm_plane_work_run(struct armada_crtc *dcrtc,
 }
 
 int armada_drm_plane_work_queue(struct armada_crtc *dcrtc,
-	struct armada_plane *plane, struct armada_plane_work *work)
+	struct armada_plane_work *work)
 {
+	struct armada_plane *plane = drm_to_armada_plane(work->plane);
 	int ret;
 
 	ret = drm_crtc_vblank_get(&dcrtc->crtc);
@@ -263,16 +264,8 @@ void armada_drm_plane_work_cancel(struct armada_crtc *dcrtc,
 		drm_crtc_vblank_put(&dcrtc->crtc);
 }
 
-static int armada_drm_crtc_queue_frame_work(struct armada_crtc *dcrtc,
-	struct armada_frame_work *work)
-{
-	struct armada_plane *plane = drm_to_armada_plane(dcrtc->crtc.primary);
-
-	return armada_drm_plane_work_queue(dcrtc, plane, &work->work);
-}
-
 static void armada_drm_crtc_complete_frame_work(struct armada_crtc *dcrtc,
-	struct armada_plane *plane, struct armada_plane_work *work)
+	struct armada_plane_work *work)
 {
 	struct armada_frame_work *fwork = container_of(work, struct armada_frame_work, work);
 	struct drm_device *dev = dcrtc->crtc.dev;
@@ -293,7 +286,8 @@ static void armada_drm_crtc_complete_frame_work(struct armada_crtc *dcrtc,
 	kfree(fwork);
 }
 
-static struct armada_frame_work *armada_drm_crtc_alloc_frame_work(void)
+static struct armada_frame_work *
+armada_drm_crtc_alloc_frame_work(struct drm_plane *plane)
 {
 	struct armada_frame_work *work;
 	int i = 0;
@@ -302,6 +296,7 @@ static struct armada_frame_work *armada_drm_crtc_alloc_frame_work(void)
 	if (!work)
 		return NULL;
 
+	work->work.plane = plane;
 	work->work.fn = armada_drm_crtc_complete_frame_work;
 	armada_reg_queue_end(work->regs, i);
 
@@ -322,11 +317,11 @@ static void armada_drm_crtc_finish_fb(struct armada_crtc *dcrtc,
 		return;
 	}
 
-	work = armada_drm_crtc_alloc_frame_work();
+	work = armada_drm_crtc_alloc_frame_work(dcrtc->crtc.primary);
 	if (work) {
 		work->old_fb = fb;
 
-		if (armada_drm_crtc_queue_frame_work(dcrtc, work) == 0)
+		if (armada_drm_plane_work_queue(dcrtc, work) == 0)
 			return;
 
 		kfree(work);
@@ -1044,7 +1039,7 @@ static int armada_drm_crtc_page_flip(struct drm_crtc *crtc,
 	if (fb->format != crtc->primary->fb->format)
 		return -EINVAL;
 
-	work = armada_drm_crtc_alloc_frame_work();
+	work = armada_drm_crtc_alloc_frame_work(dcrtc->crtc.primary);
 	if (!work)
 		return -ENOMEM;
 
@@ -1061,7 +1056,7 @@ static int armada_drm_crtc_page_flip(struct drm_crtc *crtc,
 	 */
 	drm_framebuffer_get(fb);
 
-	ret = armada_drm_crtc_queue_frame_work(dcrtc, work);
+	ret = armada_drm_plane_work_queue(dcrtc, work);
 	if (ret) {
 		/* Undo our reference above */
 		drm_framebuffer_put(fb);
drivers/gpu/drm/armada/armada_crtc.h +3 -4
index a054527eb962..821c0dd21e45 100644
@@ -36,9 +36,8 @@ struct armada_plane;
 struct armada_variant;
 
 struct armada_plane_work {
-	void			(*fn)(struct armada_crtc *,
-				      struct armada_plane *,
-				      struct armada_plane_work *);
+	void (*fn)(struct armada_crtc *, struct armada_plane_work *);
+	struct drm_plane *plane;
 };
 
 struct armada_plane_state {
@@ -60,7 +59,7 @@ struct armada_plane {
 
 int armada_drm_plane_init(struct armada_plane *plane);
 int armada_drm_plane_work_queue(struct armada_crtc *dcrtc,
-	struct armada_plane *plane, struct armada_plane_work *work);
+	struct armada_plane_work *work);
 int armada_drm_plane_work_wait(struct armada_plane *plane, long timeout);
 void armada_drm_plane_work_cancel(struct armada_crtc *dcrtc,
 	struct armada_plane *plane);
drivers/gpu/drm/armada/armada_overlay.c +7 -5
index aba947696178..1fa8ea8cb2de 100644
@@ -80,11 +80,12 @@ static void armada_ovl_retire_fb(struct armada_ovl_plane *dplane,
 
 /* === Plane support === */
 static void armada_ovl_plane_work(struct armada_crtc *dcrtc,
-	struct armada_plane *plane, struct armada_plane_work *work)
+	struct armada_plane_work *work)
 {
-	struct armada_ovl_plane *dplane = container_of(plane, struct armada_ovl_plane, base);
+	struct armada_ovl_plane *dplane = container_of(work->plane,
+					struct armada_ovl_plane, base.base);
 
-	trace_armada_ovl_plane_work(&dcrtc->crtc, &plane->base);
+	trace_armada_ovl_plane_work(&dcrtc->crtc, work->plane);
 
 	armada_drm_crtc_update_regs(dcrtc, dplane->vbl.regs);
 	armada_ovl_retire_fb(dplane, NULL);
@@ -252,8 +253,8 @@ armada_ovl_plane_update(struct drm_plane *plane, struct drm_crtc *crtc,
 	}
 	if (idx) {
 		armada_reg_queue_end(dplane->vbl.regs, idx);
-		armada_drm_plane_work_queue(dcrtc, &dplane->base,
-					    &dplane->vbl.work);
+		/* Queue it for update on the next interrupt if we are enabled */
+		armada_drm_plane_work_queue(dcrtc, &dplane->vbl.work);
 	}
 	return 0;
 }
@@ -454,6 +455,7 @@ int armada_overlay_plane_create(struct drm_device *dev, unsigned long crtcs)
 		return ret;
 	}
 
+	dplane->vbl.work.plane = &dplane->base.base;
 	dplane->vbl.work.fn = armada_ovl_plane_work;
 
 	ret = drm_universal_plane_init(dev, &dplane->base.base, crtcs,
Signature · 6 lines
2.7.4

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
#10 lore raw

PATCH 4/25 drm/armada: add work cancel callback

In reply to Russell King - ARM Linux

Add a work cancel callback, so that work items can add functionality to clean themselves up when they are cancelled.

---
 drivers/gpu/drm/armada/armada_crtc.c | 23 ++++++++++++++++-------
 drivers/gpu/drm/armada/armada_crtc.h |  1 +
 2 files changed, 17 insertions(+), 7 deletions(-)
drivers/gpu/drm/armada/armada_crtc.c +16 -7
index 4d3db441466e..f10ab0275ce7 100644
@@ -216,6 +216,19 @@ static unsigned armada_drm_crtc_calc_fb(struct drm_framebuffer *fb,
 	return i;
 }
 
+static void armada_drm_plane_work_call(struct armada_crtc *dcrtc,
+	struct armada_plane_work *work,
+	void (*fn)(struct armada_crtc *, struct armada_plane_work *))
+{
+	struct armada_plane *dplane = drm_to_armada_plane(work->plane);
+
+	if (fn)
+		fn(dcrtc, work);
+	drm_crtc_vblank_put(&dcrtc->crtc);
+
+	wake_up(&dplane->frame_wait);
+}
+
 static void armada_drm_plane_work_run(struct armada_crtc *dcrtc,
 	struct drm_plane *plane)
 {
@@ -223,12 +236,8 @@ static void armada_drm_plane_work_run(struct armada_crtc *dcrtc,
 	struct armada_plane_work *work = xchg(&dplane->work, NULL);
 
 	/* Handle any pending frame work. */
-	if (work) {
-		work->fn(dcrtc, work);
-		drm_crtc_vblank_put(&dcrtc->crtc);
-	}
-
-	wake_up(&dplane->frame_wait);
+	if (work)
+		armada_drm_plane_work_call(dcrtc, work, work->fn);
 }
 
 int armada_drm_plane_work_queue(struct armada_crtc *dcrtc,
@@ -261,7 +270,7 @@ void armada_drm_plane_work_cancel(struct armada_crtc *dcrtc,
 	struct armada_plane_work *work = xchg(&dplane->work, NULL);
 
 	if (work)
-		drm_crtc_vblank_put(&dcrtc->crtc);
+		armada_drm_plane_work_call(dcrtc, work, work->cancel);
 }
 
 static void armada_drm_crtc_complete_frame_work(struct armada_crtc *dcrtc,
drivers/gpu/drm/armada/armada_crtc.h +1 -0
index 821c0dd21e45..c26814c2ce08 100644
@@ -37,6 +37,7 @@ struct armada_variant;
 
 struct armada_plane_work {
 	void (*fn)(struct armada_crtc *, struct armada_plane_work *);
+	void (*cancel)(struct armada_crtc *, struct armada_plane_work *);
 	struct drm_plane *plane;
 };
 
Signature · 6 lines
2.7.4

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
#11 lore raw

PATCH 5/25 drm/armada: wait and cancel any pending frame work at disable

In reply to Russell King - ARM Linux

Wait for a second, and if we time out, cancel any pending work when disabling the primary plane. This ensures that any pending work is completed or cleaned up prior to the disable taking effect.

---
 drivers/gpu/drm/armada/armada_crtc.c    | 28 ++++++++++++++++++++++------
 drivers/gpu/drm/armada/armada_overlay.c |  2 --
 2 files changed, 22 insertions(+), 8 deletions(-)
drivers/gpu/drm/armada/armada_crtc.c +22 -6
index f10ab0275ce7..328f030ffbdc 100644
@@ -273,18 +273,15 @@ void armada_drm_plane_work_cancel(struct armada_crtc *dcrtc,
 		armada_drm_plane_work_call(dcrtc, work, work->cancel);
 }
 
-static void armada_drm_crtc_complete_frame_work(struct armada_crtc *dcrtc,
+static void armada_drm_crtc_finish_frame_work(struct armada_crtc *dcrtc,
 	struct armada_plane_work *work)
 {
 	struct armada_frame_work *fwork = container_of(work, struct armada_frame_work, work);
-	struct drm_device *dev = dcrtc->crtc.dev;
 	unsigned long flags;
 
-	spin_lock_irqsave(&dcrtc->irq_lock, flags);
-	armada_drm_crtc_update_regs(dcrtc, fwork->regs);
-	spin_unlock_irqrestore(&dcrtc->irq_lock, flags);
-
 	if (fwork->event) {
+		struct drm_device *dev = dcrtc->crtc.dev;
+
 		spin_lock_irqsave(&dev->event_lock, flags);
 		drm_crtc_send_vblank_event(&dcrtc->crtc, fwork->event);
 		spin_unlock_irqrestore(&dev->event_lock, flags);
@@ -295,6 +292,19 @@ static void armada_drm_crtc_complete_frame_work(struct armada_crtc *dcrtc,
 	kfree(fwork);
 }
 
+static void armada_drm_crtc_complete_frame_work(struct armada_crtc *dcrtc,
+	struct armada_plane_work *work)
+{
+	struct armada_frame_work *fwork = container_of(work, struct armada_frame_work, work);
+	unsigned long flags;
+
+	spin_lock_irqsave(&dcrtc->irq_lock, flags);
+	armada_drm_crtc_update_regs(dcrtc, fwork->regs);
+	spin_unlock_irqrestore(&dcrtc->irq_lock, flags);
+
+	armada_drm_crtc_finish_frame_work(dcrtc, work);
+}
+
 static struct armada_frame_work *
 armada_drm_crtc_alloc_frame_work(struct drm_plane *plane)
 {
@@ -307,6 +317,7 @@ armada_drm_crtc_alloc_frame_work(struct drm_plane *plane)
 
 	work->work.plane = plane;
 	work->work.fn = armada_drm_crtc_complete_frame_work;
+	work->work.cancel = armada_drm_crtc_finish_frame_work;
 	armada_reg_queue_end(work->regs, i);
 
 	return work;
@@ -752,6 +763,7 @@ static int armada_drm_crtc_mode_set_base(struct drm_crtc *crtc, int x, int y,
 void armada_drm_crtc_plane_disable(struct armada_crtc *dcrtc,
 	struct drm_plane *plane)
 {
+	struct armada_plane *dplane = drm_to_armada_plane(plane);
 	u32 sram_para1, dma_ctrl0_mask;
 
 	/*
@@ -775,6 +787,10 @@ void armada_drm_crtc_plane_disable(struct armada_crtc *dcrtc,
 		dma_ctrl0_mask = CFG_DMA_ENA;
 	}
 
+	/* Wait for any preceding work to complete, but don't wedge */
+	if (WARN_ON(!armada_drm_plane_work_wait(dplane, HZ)))
+		armada_drm_plane_work_cancel(dcrtc, dplane);
+
 	spin_lock_irq(&dcrtc->irq_lock);
 	armada_updatel(0, dma_ctrl0_mask, dcrtc->base + LCD_SPU_DMA_CTRL0);
 	spin_unlock_irq(&dcrtc->irq_lock);
drivers/gpu/drm/armada/armada_overlay.c +0 -2
index 1fa8ea8cb2de..cf8442583bfc 100644
@@ -270,8 +270,6 @@ static int armada_ovl_plane_disable(struct drm_plane *plane,
 		return 0;
 
 	dcrtc = drm_to_armada_crtc(dplane->base.base.crtc);
-
-	armada_drm_plane_work_cancel(dcrtc, &dplane->base);
 	armada_drm_crtc_plane_disable(dcrtc, plane);
 
 	dcrtc->plane = NULL;
Signature · 6 lines
2.7.4

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
#12 lore raw

PATCH 6/25 drm/armada: allow the primary plane to be disabled

In reply to Russell King - ARM Linux

Add our own hook to allow the primary plane to be disabled.

---
 drivers/gpu/drm/armada/armada_crtc.c | 99 ++++++++++++++++++++----------------
 1 file changed, 55 insertions(+), 44 deletions(-)
drivers/gpu/drm/armada/armada_crtc.c +55 -44
index 328f030ffbdc..7f7f21166488 100644
@@ -341,7 +341,7 @@ static void armada_drm_crtc_finish_fb(struct armada_crtc *dcrtc,
 	if (work) {
 		work->old_fb = fb;
 
-		if (armada_drm_plane_work_queue(dcrtc, work) == 0)
+		if (armada_drm_plane_work_queue(dcrtc, &work->work) == 0)
 			return;
 
 		kfree(work);
@@ -760,51 +760,13 @@ static int armada_drm_crtc_mode_set_base(struct drm_crtc *crtc, int x, int y,
 	return 0;
 }
 
-void armada_drm_crtc_plane_disable(struct armada_crtc *dcrtc,
-	struct drm_plane *plane)
-{
-	struct armada_plane *dplane = drm_to_armada_plane(plane);
-	u32 sram_para1, dma_ctrl0_mask;
-
-	/*
-	 * Drop our reference on any framebuffer attached to this plane.
-	 * We don't need to NULL this out as drm_plane_force_disable(),
-	 * and __setplane_internal() will do so for an overlay plane, and
-	 * __drm_helper_disable_unused_functions() will do so for the
-	 * primary plane.
-	 */
-	if (plane->fb)
-		drm_framebuffer_put(plane->fb);
-
-	/* Power down most RAMs and FIFOs if this is the primary plane */
-	if (plane->type == DRM_PLANE_TYPE_PRIMARY) {
-		sram_para1 = CFG_PDWN256x32 | CFG_PDWN256x24 | CFG_PDWN256x8 |
-			     CFG_PDWN32x32 | CFG_PDWN64x66;
-		dma_ctrl0_mask = CFG_GRA_ENA;
-	} else {
-		/* Power down the Y/U/V FIFOs */
-		sram_para1 = CFG_PDWN16x66 | CFG_PDWN32x66;
-		dma_ctrl0_mask = CFG_DMA_ENA;
-	}
-
-	/* Wait for any preceding work to complete, but don't wedge */
-	if (WARN_ON(!armada_drm_plane_work_wait(dplane, HZ)))
-		armada_drm_plane_work_cancel(dcrtc, dplane);
-
-	spin_lock_irq(&dcrtc->irq_lock);
-	armada_updatel(0, dma_ctrl0_mask, dcrtc->base + LCD_SPU_DMA_CTRL0);
-	spin_unlock_irq(&dcrtc->irq_lock);
-
-	armada_updatel(sram_para1, 0, dcrtc->base + LCD_SPU_SRAM_PARA1);
-}
-
 /* The mode_config.mutex will be held for this call */
 static void armada_drm_crtc_disable(struct drm_crtc *crtc)
 {
-	struct armada_crtc *dcrtc = drm_to_armada_crtc(crtc);
-
 	armada_drm_crtc_dpms(crtc, DRM_MODE_DPMS_OFF);
-	armada_drm_crtc_plane_disable(dcrtc, crtc->primary);
+
+	/* Disable our primary plane when we disable the CRTC. */
+	crtc->primary->funcs->disable_plane(crtc->primary, NULL);
 }
 
 static const struct drm_crtc_helper_funcs armada_crtc_helper_funcs = {
@@ -1081,7 +1043,7 @@ static int armada_drm_crtc_page_flip(struct drm_crtc *crtc,
 	 */
 	drm_framebuffer_get(fb);
 
-	ret = armada_drm_plane_work_queue(dcrtc, work);
+	ret = armada_drm_plane_work_queue(dcrtc, &work->work);
 	if (ret) {
 		/* Undo our reference above */
 		drm_framebuffer_put(fb);
@@ -1161,9 +1123,58 @@ static const struct drm_crtc_funcs armada_crtc_funcs = {
 	.disable_vblank	= armada_drm_crtc_disable_vblank,
 };
 
+void armada_drm_crtc_plane_disable(struct armada_crtc *dcrtc,
+	struct drm_plane *plane)
+{
+	struct armada_plane *dplane = drm_to_armada_plane(plane);
+	u32 sram_para1, dma_ctrl0_mask;
+
+	/*
+	 * Drop our reference on any framebuffer attached to this plane.
+	 * We don't need to NULL this out as drm_plane_force_disable(),
+	 * and __setplane_internal() will do so for an overlay plane, and
+	 * __drm_helper_disable_unused_functions() will do so for the
+	 * primary plane.
+	 */
+	if (plane->fb)
+		drm_framebuffer_put(plane->fb);
+
+	/* Power down most RAMs and FIFOs if this is the primary plane */
+	if (plane->type == DRM_PLANE_TYPE_PRIMARY) {
+		sram_para1 = CFG_PDWN256x32 | CFG_PDWN256x24 | CFG_PDWN256x8 |
+			     CFG_PDWN32x32 | CFG_PDWN64x66;
+		dma_ctrl0_mask = CFG_GRA_ENA;
+	} else {
+		/* Power down the Y/U/V FIFOs */
+		sram_para1 = CFG_PDWN16x66 | CFG_PDWN32x66;
+		dma_ctrl0_mask = CFG_DMA_ENA;
+	}
+
+	/* Wait for any preceding work to complete, but don't wedge */
+	if (WARN_ON(!armada_drm_plane_work_wait(dplane, HZ)))
+		armada_drm_plane_work_cancel(dcrtc, dplane);
+
+	spin_lock_irq(&dcrtc->irq_lock);
+	armada_updatel(0, dma_ctrl0_mask, dcrtc->base + LCD_SPU_DMA_CTRL0);
+	spin_unlock_irq(&dcrtc->irq_lock);
+
+	armada_updatel(sram_para1, 0, dcrtc->base + LCD_SPU_SRAM_PARA1);
+}
+
+static int armada_drm_primary_disable(struct drm_plane *plane,
+				      struct drm_modeset_acquire_ctx *ctx)
+{
+	if (plane->crtc) {
+		struct armada_crtc *dcrtc = drm_to_armada_crtc(plane->crtc);
+
+		armada_drm_crtc_plane_disable(dcrtc, plane);
+	}
+	return 0;
+}
+
 static const struct drm_plane_funcs armada_primary_plane_funcs = {
 	.update_plane	= drm_primary_helper_update,
-	.disable_plane	= drm_primary_helper_disable,
+	.disable_plane	= armada_drm_primary_disable,
 	.destroy	= drm_primary_helper_destroy,
 };
 
Signature · 6 lines
2.7.4

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
#13 lore raw

PATCH 7/25 drm/armada: clean up armada_drm_crtc_plane_disable()

In reply to Russell King - ARM Linux

Merge armada_drm_primary_disable() into armada_drm_crtc_plane_disable() and rename to armada_drm_plane_disable(). Use this to simplify armada_ovl_plane_disable().

---
 drivers/gpu/drm/armada/armada_crtc.c    | 21 +++++++++------------
 drivers/gpu/drm/armada/armada_crtc.h    |  4 ++--
 drivers/gpu/drm/armada/armada_overlay.c |  9 +++------
 3 files changed, 14 insertions(+), 20 deletions(-)
drivers/gpu/drm/armada/armada_crtc.c +9 -12
index 7f7f21166488..3287b72e48cc 100644
@@ -1123,12 +1123,16 @@ static const struct drm_crtc_funcs armada_crtc_funcs = {
 	.disable_vblank	= armada_drm_crtc_disable_vblank,
 };
 
-void armada_drm_crtc_plane_disable(struct armada_crtc *dcrtc,
-	struct drm_plane *plane)
+int armada_drm_plane_disable(struct drm_plane *plane,
+			     struct drm_modeset_acquire_ctx *ctx)
 {
 	struct armada_plane *dplane = drm_to_armada_plane(plane);
+	struct armada_crtc *dcrtc;
 	u32 sram_para1, dma_ctrl0_mask;
 
+	if (!plane->crtc)
+		return 0;
+
 	/*
 	 * Drop our reference on any framebuffer attached to this plane.
 	 * We don't need to NULL this out as drm_plane_force_disable(),
@@ -1150,6 +1154,8 @@ void armada_drm_crtc_plane_disable(struct armada_crtc *dcrtc,
 		dma_ctrl0_mask = CFG_DMA_ENA;
 	}
 
+	dcrtc = drm_to_armada_crtc(plane->crtc);
+
 	/* Wait for any preceding work to complete, but don't wedge */
 	if (WARN_ON(!armada_drm_plane_work_wait(dplane, HZ)))
 		armada_drm_plane_work_cancel(dcrtc, dplane);
@@ -1159,22 +1165,13 @@ void armada_drm_crtc_plane_disable(struct armada_crtc *dcrtc,
 	spin_unlock_irq(&dcrtc->irq_lock);
 
 	armada_updatel(sram_para1, 0, dcrtc->base + LCD_SPU_SRAM_PARA1);
-}
-
-static int armada_drm_primary_disable(struct drm_plane *plane,
-				      struct drm_modeset_acquire_ctx *ctx)
-{
-	if (plane->crtc) {
-		struct armada_crtc *dcrtc = drm_to_armada_crtc(plane->crtc);
 
-		armada_drm_crtc_plane_disable(dcrtc, plane);
-	}
 	return 0;
 }
 
 static const struct drm_plane_funcs armada_primary_plane_funcs = {
 	.update_plane	= drm_primary_helper_update,
-	.disable_plane	= armada_drm_primary_disable,
+	.disable_plane	= armada_drm_plane_disable,
 	.destroy	= drm_primary_helper_destroy,
 };
 
drivers/gpu/drm/armada/armada_crtc.h +2 -2
index c26814c2ce08..521ae5b6ad86 100644
@@ -106,8 +106,8 @@ struct armada_crtc {
 
 void armada_drm_crtc_update_regs(struct armada_crtc *, struct armada_regs *);
 
-void armada_drm_crtc_plane_disable(struct armada_crtc *dcrtc,
-	struct drm_plane *plane);
+int armada_drm_plane_disable(struct drm_plane *plane,
+			     struct drm_modeset_acquire_ctx *ctx);
 
 extern struct platform_driver armada_lcd_platform_driver;
 
drivers/gpu/drm/armada/armada_overlay.c +3 -6
index cf8442583bfc..a53e7dd26b0b 100644
@@ -264,15 +264,12 @@ static int armada_ovl_plane_disable(struct drm_plane *plane,
 {
 	struct armada_ovl_plane *dplane = drm_to_armada_ovl_plane(plane);
 	struct drm_framebuffer *fb;
-	struct armada_crtc *dcrtc;
 
-	if (!dplane->base.base.crtc)
-		return 0;
+	armada_drm_plane_disable(plane, ctx);
 
-	dcrtc = drm_to_armada_crtc(dplane->base.base.crtc);
-	armada_drm_crtc_plane_disable(dcrtc, plane);
+	if (dplane->base.base.crtc)
+		drm_to_armada_crtc(dplane->base.base.crtc)->plane = NULL;
 
-	dcrtc->plane = NULL;
 	dplane->base.state.ctrl0 = 0;
 
 	fb = xchg(&dplane->old_fb, NULL);
Signature · 6 lines
2.7.4

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
#14 lore raw

PATCH 8/25 drm/armada: clear plane enable bit when disabling

In reply to Russell King - ARM Linux

Clear the plane enable bit in the software state within armada_drm_plane_disable() when disabling either the primary or overlay planes.

---
 drivers/gpu/drm/armada/armada_crtc.c    | 10 ++++++----
 drivers/gpu/drm/armada/armada_overlay.c |  2 --
 2 files changed, 6 insertions(+), 6 deletions(-)
drivers/gpu/drm/armada/armada_crtc.c +6 -4
index 3287b72e48cc..bedcaed81ffa 100644
@@ -1128,7 +1128,7 @@ int armada_drm_plane_disable(struct drm_plane *plane,
 {
 	struct armada_plane *dplane = drm_to_armada_plane(plane);
 	struct armada_crtc *dcrtc;
-	u32 sram_para1, dma_ctrl0_mask;
+	u32 sram_para1, enable_mask;
 
 	if (!plane->crtc)
 		return 0;
@@ -1147,13 +1147,15 @@ int armada_drm_plane_disable(struct drm_plane *plane,
 	if (plane->type == DRM_PLANE_TYPE_PRIMARY) {
 		sram_para1 = CFG_PDWN256x32 | CFG_PDWN256x24 | CFG_PDWN256x8 |
 			     CFG_PDWN32x32 | CFG_PDWN64x66;
-		dma_ctrl0_mask = CFG_GRA_ENA;
+		enable_mask = CFG_GRA_ENA;
 	} else {
 		/* Power down the Y/U/V FIFOs */
 		sram_para1 = CFG_PDWN16x66 | CFG_PDWN32x66;
-		dma_ctrl0_mask = CFG_DMA_ENA;
+		enable_mask = CFG_DMA_ENA;
 	}
 
+	dplane->state.ctrl0 &= ~enable_mask;
+
 	dcrtc = drm_to_armada_crtc(plane->crtc);
 
 	/* Wait for any preceding work to complete, but don't wedge */
@@ -1161,7 +1163,7 @@ int armada_drm_plane_disable(struct drm_plane *plane,
 		armada_drm_plane_work_cancel(dcrtc, dplane);
 
 	spin_lock_irq(&dcrtc->irq_lock);
-	armada_updatel(0, dma_ctrl0_mask, dcrtc->base + LCD_SPU_DMA_CTRL0);
+	armada_updatel(0, enable_mask, dcrtc->base + LCD_SPU_DMA_CTRL0);
 	spin_unlock_irq(&dcrtc->irq_lock);
 
 	armada_updatel(sram_para1, 0, dcrtc->base + LCD_SPU_SRAM_PARA1);
drivers/gpu/drm/armada/armada_overlay.c +0 -2
index a53e7dd26b0b..995463cd542d 100644
@@ -270,8 +270,6 @@ static int armada_ovl_plane_disable(struct drm_plane *plane,
 	if (dplane->base.base.crtc)
 		drm_to_armada_crtc(dplane->base.base.crtc)->plane = NULL;
 
-	dplane->base.state.ctrl0 = 0;
-
 	fb = xchg(&dplane->old_fb, NULL);
 	if (fb)
 		drm_framebuffer_put(fb);
Signature · 6 lines
2.7.4

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
#15 lore raw

PATCH 9/25 drm/armada: move overlay plane work out from under spinlock

In reply to Russell King - ARM Linux

Move the overlay plane work out from under the spinlock so that both the primary and overlay planes run their work in the same context. This is necessary so that we can use frame works with the overlay plane.

However, we must update the CRTC registers under the spinlock, so fix up the overlay code for that.

---
 drivers/gpu/drm/armada/armada_crtc.c    | 2 +-
 drivers/gpu/drm/armada/armada_overlay.c | 4 ++++
 2 files changed, 5 insertions(+), 1 deletion(-)
drivers/gpu/drm/armada/armada_crtc.c +1 -1
index bedcaed81ffa..be3fd82ef516 100644
@@ -471,11 +471,11 @@ static void armada_drm_crtc_irq(struct armada_crtc *dcrtc, u32 stat)
 	if (stat & VSYNC_IRQ)
 		drm_crtc_handle_vblank(&dcrtc->crtc);
 
-	spin_lock(&dcrtc->irq_lock);
 	ovl_plane = dcrtc->plane;
 	if (ovl_plane)
 		armada_drm_plane_work_run(dcrtc, ovl_plane);
 
+	spin_lock(&dcrtc->irq_lock);
 	if (stat & GRA_FRAME_IRQ && dcrtc->interlaced) {
 		int i = stat & GRA_FRAME_IRQ0 ? 0 : 1;
 		uint32_t val;
drivers/gpu/drm/armada/armada_overlay.c +4 -0
index 995463cd542d..04746ade74e6 100644
@@ -84,10 +84,14 @@ static void armada_ovl_plane_work(struct armada_crtc *dcrtc,
 {
 	struct armada_ovl_plane *dplane = container_of(work->plane,
 					struct armada_ovl_plane, base.base);
+	unsigned long flags;
 
 	trace_armada_ovl_plane_work(&dcrtc->crtc, work->plane);
 
+	spin_lock_irqsave(&dcrtc->irq_lock, flags);
 	armada_drm_crtc_update_regs(dcrtc, dplane->vbl.regs);
+	spin_unlock_irqrestore(&dcrtc->irq_lock, flags);
+
 	armada_ovl_retire_fb(dplane, NULL);
 }
 
Signature · 6 lines
2.7.4

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
#16 lore raw

PATCH 10/25 drm/armada: move fb retirement into armada_plane_work

In reply to Russell King - ARM Linux

Both the primary and overlay planes retire framebuffers in a similar manner; this can be consolidated by moving the retirement up to the armada_plane_work layer.

---
 drivers/gpu/drm/armada/armada_crtc.c    | 12 +++++++-----
 drivers/gpu/drm/armada/armada_crtc.h    |  1 +
 drivers/gpu/drm/armada/armada_overlay.c | 30 +++++-------------------------
 3 files changed, 13 insertions(+), 30 deletions(-)
drivers/gpu/drm/armada/armada_crtc.c +7 -5
index be3fd82ef516..d1f4171966cc 100644
@@ -24,7 +24,6 @@ struct armada_frame_work {
 	struct armada_plane_work work;
 	struct drm_pending_vblank_event *event;
 	struct armada_regs regs[4];
-	struct drm_framebuffer *old_fb;
 };
 
 enum csc_mode {
@@ -221,11 +220,16 @@ static void armada_drm_plane_work_call(struct armada_crtc *dcrtc,
 	void (*fn)(struct armada_crtc *, struct armada_plane_work *))
 {
 	struct armada_plane *dplane = drm_to_armada_plane(work->plane);
+	struct drm_framebuffer *fb = work->old_fb;
 
 	if (fn)
 		fn(dcrtc, work);
 	drm_crtc_vblank_put(&dcrtc->crtc);
 
+	/* Finally, queue the process-half of the cleanup. */
+	if (fb)
+		armada_drm_queue_unref_work(dcrtc->crtc.dev, fb);
+
 	wake_up(&dplane->frame_wait);
 }
 
@@ -287,8 +291,6 @@ static void armada_drm_crtc_finish_frame_work(struct armada_crtc *dcrtc,
 		spin_unlock_irqrestore(&dev->event_lock, flags);
 	}
 
-	/* Finally, queue the process-half of the cleanup. */
-	__armada_drm_queue_unref_work(dcrtc->crtc.dev, fwork->old_fb);
 	kfree(fwork);
 }
 
@@ -339,7 +341,7 @@ static void armada_drm_crtc_finish_fb(struct armada_crtc *dcrtc,
 
 	work = armada_drm_crtc_alloc_frame_work(dcrtc->crtc.primary);
 	if (work) {
-		work->old_fb = fb;
+		work->work.old_fb = fb;
 
 		if (armada_drm_plane_work_queue(dcrtc, &work->work) == 0)
 			return;
@@ -1031,7 +1033,7 @@ static int armada_drm_crtc_page_flip(struct drm_crtc *crtc,
 		return -ENOMEM;
 
 	work->event = event;
-	work->old_fb = dcrtc->crtc.primary->fb;
+	work->work.old_fb = dcrtc->crtc.primary->fb;
 
 	i = armada_drm_crtc_calc_fb(fb, crtc->x, crtc->y, work->regs,
 				    dcrtc->interlaced);
drivers/gpu/drm/armada/armada_crtc.h +1 -0
index 521ae5b6ad86..b40db72c61d8 100644
@@ -39,6 +39,7 @@ struct armada_plane_work {
 	void (*fn)(struct armada_crtc *, struct armada_plane_work *);
 	void (*cancel)(struct armada_crtc *, struct armada_plane_work *);
 	struct drm_plane *plane;
+	struct drm_framebuffer *old_fb;
 };
 
 struct armada_plane_state {
drivers/gpu/drm/armada/armada_overlay.c +5 -25
index 04746ade74e6..01087c952916 100644
@@ -32,7 +32,6 @@ struct armada_ovl_plane_properties {
 
 struct armada_ovl_plane {
 	struct armada_plane base;
-	struct drm_framebuffer *old_fb;
 	struct {
 		struct armada_plane_work work;
 		struct armada_regs regs[13];
@@ -67,17 +66,6 @@ armada_ovl_update_attr(struct armada_ovl_plane_properties *prop,
 	spin_unlock_irq(&dcrtc->irq_lock);
 }
 
-static void armada_ovl_retire_fb(struct armada_ovl_plane *dplane,
-	struct drm_framebuffer *fb)
-{
-	struct drm_framebuffer *old_fb;
-
-	old_fb = xchg(&dplane->old_fb, fb);
-
-	if (old_fb)
-		armada_drm_queue_unref_work(dplane->base.base.dev, old_fb);
-}
-
 /* === Plane support === */
 static void armada_ovl_plane_work(struct armada_crtc *dcrtc,
 	struct armada_plane_work *work)
@@ -91,8 +79,6 @@ static void armada_ovl_plane_work(struct armada_crtc *dcrtc,
 	spin_lock_irqsave(&dcrtc->irq_lock, flags);
 	armada_drm_crtc_update_regs(dcrtc, dplane->vbl.regs);
 	spin_unlock_irqrestore(&dcrtc->irq_lock, flags);
-
-	armada_ovl_retire_fb(dplane, NULL);
 }
 
 static int
@@ -196,8 +182,7 @@ armada_ovl_plane_update(struct drm_plane *plane, struct drm_crtc *crtc,
 		 */
 		drm_framebuffer_get(fb);
 
-		if (plane->fb)
-			armada_ovl_retire_fb(dplane, plane->fb);
+		dplane->vbl.work.old_fb = plane->fb;
 
 		dplane->base.state.src_y = src_y = src.y1 >> 16;
 		dplane->base.state.src_x = src_x = src.x1 >> 16;
@@ -223,6 +208,8 @@ armada_ovl_plane_update(struct drm_plane *plane, struct drm_crtc *crtc,
 		val = fb->pitches[1] << 16 | fb->pitches[2];
 		armada_reg_queue_set(dplane->vbl.regs, idx, val,
 				     LCD_SPU_DMA_PITCH_UV);
+	} else {
+		dplane->vbl.work.old_fb = NULL;
 	}
 
 	val = (drm_rect_height(&src) & 0xffff0000) | drm_rect_width(&src) >> 16;
@@ -266,17 +253,10 @@ armada_ovl_plane_update(struct drm_plane *plane, struct drm_crtc *crtc,
 static int armada_ovl_plane_disable(struct drm_plane *plane,
 				    struct drm_modeset_acquire_ctx *ctx)
 {
-	struct armada_ovl_plane *dplane = drm_to_armada_ovl_plane(plane);
-	struct drm_framebuffer *fb;
-
 	armada_drm_plane_disable(plane, ctx);
 
-	if (dplane->base.base.crtc)
-		drm_to_armada_crtc(dplane->base.base.crtc)->plane = NULL;
-
-	fb = xchg(&dplane->old_fb, NULL);
-	if (fb)
-		drm_framebuffer_put(fb);
+	if (plane->crtc)
+		drm_to_armada_crtc(plane->crtc)->plane = NULL;
 
 	return 0;
 }
Signature · 6 lines
2.7.4

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
#17 lore raw

PATCH 11/25 drm/armada: move event sending into armada_plane_work

In reply to Russell King - ARM Linux

Move the sending of events into the armada_plane_work structure, and combine the processing in armada_drm_plane_work_call().

---
 drivers/gpu/drm/armada/armada_crtc.c | 27 +++++++++++++--------------
 drivers/gpu/drm/armada/armada_crtc.h |  1 +
 2 files changed, 14 insertions(+), 14 deletions(-)
drivers/gpu/drm/armada/armada_crtc.c +13 -14
index d1f4171966cc..b043766c416c 100644
@@ -22,7 +22,6 @@
 
 struct armada_frame_work {
 	struct armada_plane_work work;
-	struct drm_pending_vblank_event *event;
 	struct armada_regs regs[4];
 };
 
@@ -220,15 +219,24 @@ static void armada_drm_plane_work_call(struct armada_crtc *dcrtc,
 	void (*fn)(struct armada_crtc *, struct armada_plane_work *))
 {
 	struct armada_plane *dplane = drm_to_armada_plane(work->plane);
+	struct drm_pending_vblank_event *event = work->event;
 	struct drm_framebuffer *fb = work->old_fb;
 
 	if (fn)
 		fn(dcrtc, work);
 	drm_crtc_vblank_put(&dcrtc->crtc);
 
-	/* Finally, queue the process-half of the cleanup. */
-	if (fb)
-		armada_drm_queue_unref_work(dcrtc->crtc.dev, fb);
+	if (event || fb) {
+		struct drm_device *dev = dcrtc->crtc.dev;
+		unsigned long flags;
+
+		spin_lock_irqsave(&dev->event_lock, flags);
+		if (event)
+			drm_crtc_send_vblank_event(&dcrtc->crtc, event);
+		if (fb)
+			__armada_drm_queue_unref_work(dev, fb);
+		spin_unlock_irqrestore(&dev->event_lock, flags);
+	}
 
 	wake_up(&dplane->frame_wait);
 }
@@ -281,15 +289,6 @@ static void armada_drm_crtc_finish_frame_work(struct armada_crtc *dcrtc,
 	struct armada_plane_work *work)
 {
 	struct armada_frame_work *fwork = container_of(work, struct armada_frame_work, work);
-	unsigned long flags;
-
-	if (fwork->event) {
-		struct drm_device *dev = dcrtc->crtc.dev;
-
-		spin_lock_irqsave(&dev->event_lock, flags);
-		drm_crtc_send_vblank_event(&dcrtc->crtc, fwork->event);
-		spin_unlock_irqrestore(&dev->event_lock, flags);
-	}
 
 	kfree(fwork);
 }
@@ -1032,7 +1031,7 @@ static int armada_drm_crtc_page_flip(struct drm_crtc *crtc,
 	if (!work)
 		return -ENOMEM;
 
-	work->event = event;
+	work->work.event = event;
 	work->work.old_fb = dcrtc->crtc.primary->fb;
 
 	i = armada_drm_crtc_calc_fb(fb, crtc->x, crtc->y, work->regs,
drivers/gpu/drm/armada/armada_crtc.h +1 -0
index b40db72c61d8..4cdd2f0eabd9 100644
@@ -40,6 +40,7 @@ struct armada_plane_work {
 	void (*cancel)(struct armada_crtc *, struct armada_plane_work *);
 	struct drm_plane *plane;
 	struct drm_framebuffer *old_fb;
+	struct drm_pending_vblank_event *event;
 };
 
 struct armada_plane_state {
Signature · 6 lines
2.7.4

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
#18 lore raw

PATCH 12/25 drm/armada: move regs into armada_plane_work

In reply to Russell King - ARM Linux

Move the register update structure out of the overlay private structure into armada_plane_work, as this is common to both the primary and overlay planes.

---
 drivers/gpu/drm/armada/armada_crtc.c    | 42 ++++++++++++------------------
 drivers/gpu/drm/armada/armada_crtc.h    |  1 +
 drivers/gpu/drm/armada/armada_overlay.c | 46 +++++++++++++++------------------
 3 files changed, 39 insertions(+), 50 deletions(-)
drivers/gpu/drm/armada/armada_crtc.c +17 -25
index b043766c416c..02eefdc6f062 100644
@@ -20,11 +20,6 @@
 #include "armada_hw.h"
 #include "armada_trace.h"
 
-struct armada_frame_work {
-	struct armada_plane_work work;
-	struct armada_regs regs[4];
-};
-
 enum csc_mode {
 	CSC_AUTO = 0,
 	CSC_YUV_CCIR601 = 1,
@@ -288,37 +283,34 @@ void armada_drm_plane_work_cancel(struct armada_crtc *dcrtc,
 static void armada_drm_crtc_finish_frame_work(struct armada_crtc *dcrtc,
 	struct armada_plane_work *work)
 {
-	struct armada_frame_work *fwork = container_of(work, struct armada_frame_work, work);
-
-	kfree(fwork);
+	kfree(work);
 }
 
 static void armada_drm_crtc_complete_frame_work(struct armada_crtc *dcrtc,
 	struct armada_plane_work *work)
 {
-	struct armada_frame_work *fwork = container_of(work, struct armada_frame_work, work);
 	unsigned long flags;
 
 	spin_lock_irqsave(&dcrtc->irq_lock, flags);
-	armada_drm_crtc_update_regs(dcrtc, fwork->regs);
+	armada_drm_crtc_update_regs(dcrtc, work->regs);
 	spin_unlock_irqrestore(&dcrtc->irq_lock, flags);
 
 	armada_drm_crtc_finish_frame_work(dcrtc, work);
 }
 
-static struct armada_frame_work *
-armada_drm_crtc_alloc_frame_work(struct drm_plane *plane)
+static struct armada_plane_work *
+armada_drm_crtc_alloc_plane_work(struct drm_plane *plane)
 {
-	struct armada_frame_work *work;
+	struct armada_plane_work *work;
 	int i = 0;
 
 	work = kzalloc(sizeof(*work), GFP_KERNEL);
 	if (!work)
 		return NULL;
 
-	work->work.plane = plane;
-	work->work.fn = armada_drm_crtc_complete_frame_work;
-	work->work.cancel = armada_drm_crtc_finish_frame_work;
+	work->plane = plane;
+	work->fn = armada_drm_crtc_complete_frame_work;
+	work->cancel = armada_drm_crtc_finish_frame_work;
 	armada_reg_queue_end(work->regs, i);
 
 	return work;
@@ -327,7 +319,7 @@ armada_drm_crtc_alloc_frame_work(struct drm_plane *plane)
 static void armada_drm_crtc_finish_fb(struct armada_crtc *dcrtc,
 	struct drm_framebuffer *fb, bool force)
 {
-	struct armada_frame_work *work;
+	struct armada_plane_work *work;
 
 	if (!fb)
 		return;
@@ -338,11 +330,11 @@ static void armada_drm_crtc_finish_fb(struct armada_crtc *dcrtc,
 		return;
 	}
 
-	work = armada_drm_crtc_alloc_frame_work(dcrtc->crtc.primary);
+	work = armada_drm_crtc_alloc_plane_work(dcrtc->crtc.primary);
 	if (work) {
-		work->work.old_fb = fb;
+		work->old_fb = fb;
 
-		if (armada_drm_plane_work_queue(dcrtc, &work->work) == 0)
+		if (armada_drm_plane_work_queue(dcrtc, work) == 0)
 			return;
 
 		kfree(work);
@@ -1019,7 +1011,7 @@ static int armada_drm_crtc_page_flip(struct drm_crtc *crtc,
 	struct drm_modeset_acquire_ctx *ctx)
 {
 	struct armada_crtc *dcrtc = drm_to_armada_crtc(crtc);
-	struct armada_frame_work *work;
+	struct armada_plane_work *work;
 	unsigned i;
 	int ret;
 
@@ -1027,12 +1019,12 @@ static int armada_drm_crtc_page_flip(struct drm_crtc *crtc,
 	if (fb->format != crtc->primary->fb->format)
 		return -EINVAL;
 
-	work = armada_drm_crtc_alloc_frame_work(dcrtc->crtc.primary);
+	work = armada_drm_crtc_alloc_plane_work(dcrtc->crtc.primary);
 	if (!work)
 		return -ENOMEM;
 
-	work->work.event = event;
-	work->work.old_fb = dcrtc->crtc.primary->fb;
+	work->event = event;
+	work->old_fb = dcrtc->crtc.primary->fb;
 
 	i = armada_drm_crtc_calc_fb(fb, crtc->x, crtc->y, work->regs,
 				    dcrtc->interlaced);
@@ -1044,7 +1036,7 @@ static int armada_drm_crtc_page_flip(struct drm_crtc *crtc,
 	 */
 	drm_framebuffer_get(fb);
 
-	ret = armada_drm_plane_work_queue(dcrtc, &work->work);
+	ret = armada_drm_plane_work_queue(dcrtc, work);
 	if (ret) {
 		/* Undo our reference above */
 		drm_framebuffer_put(fb);
drivers/gpu/drm/armada/armada_crtc.h +1 -0
index 4cdd2f0eabd9..12ef9688a45a 100644
@@ -41,6 +41,7 @@ struct armada_plane_work {
 	struct drm_plane *plane;
 	struct drm_framebuffer *old_fb;
 	struct drm_pending_vblank_event *event;
+	struct armada_regs regs[14];
 };
 
 struct armada_plane_state {
drivers/gpu/drm/armada/armada_overlay.c +21 -25

128 lines not on this page · raw message

Signature · 6 lines
2.7.4

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
#19 lore raw

PATCH 13/25 drm/armada: move writes of LCD_SPU_SRAM_PARA1 under lock

In reply to Russell King - ARM Linux

Move writes of LCD_SPU_SRAM_PARA1 under the irq lock, so that we can add this to the frame updates at interrupt time when disabling a plane.

---
 drivers/gpu/drm/armada/armada_crtc.c    | 10 ++++++----
 drivers/gpu/drm/armada/armada_overlay.c |  5 +++--
 2 files changed, 9 insertions(+), 6 deletions(-)
drivers/gpu/drm/armada/armada_crtc.c +6 -4
index 02eefdc6f062..b0091142a79f 100644
@@ -657,8 +657,6 @@ static int armada_drm_crtc_mode_set(struct drm_crtc *crtc,
 	/* Now compute the divider for real */
 	dcrtc->variant->compute_clock(dcrtc, adj, &sclk);
 
-	/* Ensure graphic fifo is enabled */
-	armada_reg_queue_mod(regs, i, 0, CFG_PDWN64x66, LCD_SPU_SRAM_PARA1);
 	armada_reg_queue_set(regs, i, sclk, LCD_CFG_SCLK_DIV);
 
 	if (interlaced ^ dcrtc->interlaced) {
@@ -671,6 +669,9 @@ static int armada_drm_crtc_mode_set(struct drm_crtc *crtc,
 
 	spin_lock_irqsave(&dcrtc->irq_lock, flags);
 
+	/* Ensure graphic fifo is enabled */
+	armada_reg_queue_mod(regs, i, 0, CFG_PDWN64x66, LCD_SPU_SRAM_PARA1);
+
 	/* Even interlaced/progressive frame */
 	dcrtc->v[1].spu_v_h_total = adj->crtc_vtotal << 16 |
 				    adj->crtc_htotal;
@@ -869,9 +870,11 @@ static int armada_drm_crtc_cursor_update(struct armada_crtc *dcrtc, bool reload)
 		return 0;
 	}
 
+	spin_lock_irq(&dcrtc->irq_lock);
 	para1 = readl_relaxed(dcrtc->base + LCD_SPU_SRAM_PARA1);
 	armada_updatel(CFG_CSB_256x32, CFG_CSB_256x32 | CFG_PDWN256x32,
 		       dcrtc->base + LCD_SPU_SRAM_PARA1);
+	spin_unlock_irq(&dcrtc->irq_lock);
 
 	/*
 	 * Initialize the transparency if the SRAM was powered down.
@@ -1157,9 +1160,8 @@ int armada_drm_plane_disable(struct drm_plane *plane,
 
 	spin_lock_irq(&dcrtc->irq_lock);
 	armada_updatel(0, enable_mask, dcrtc->base + LCD_SPU_DMA_CTRL0);
-	spin_unlock_irq(&dcrtc->irq_lock);
-
 	armada_updatel(sram_para1, 0, dcrtc->base + LCD_SPU_SRAM_PARA1);
+	spin_unlock_irq(&dcrtc->irq_lock);
 
 	return 0;
 }
drivers/gpu/drm/armada/armada_overlay.c +3 -2
index 200223861bfb..e02d0d9d4c23 100644
@@ -162,8 +162,9 @@ armada_ovl_plane_update(struct drm_plane *plane, struct drm_crtc *crtc,
 		return 0;
 	} else if (~dplane->base.state.ctrl0 & ctrl0 & CFG_DMA_ENA) {
 		/* Power up the Y/U/V FIFOs on ENA 0->1 transitions */
-		armada_updatel(0, CFG_PDWN16x66 | CFG_PDWN32x66,
-			       dcrtc->base + LCD_SPU_SRAM_PARA1);
+		armada_reg_queue_mod(work->regs, idx,
+				     0, CFG_PDWN16x66 | CFG_PDWN32x66,
+				     LCD_SPU_SRAM_PARA1);
 	}
 
 	if (armada_drm_plane_work_wait(&dplane->base, HZ / 25) == 0)
Signature · 6 lines
2.7.4

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
#20 lore raw

PATCH 14/25 drm/armada: only enable HSMOOTH if scaling horizontally

In reply to Russell King - ARM Linux

Only enable the HSMOOTH control bit if we are scaling horizontally, otherwise it makes no sense to enable the horizontal scaler.

---
 drivers/gpu/drm/armada/armada_crtc.c    |  5 +++--
 drivers/gpu/drm/armada/armada_overlay.c | 10 +++++-----
 2 files changed, 8 insertions(+), 7 deletions(-)
drivers/gpu/drm/armada/armada_crtc.c +3 -2
index b0091142a79f..401ad854d751 100644
@@ -583,7 +583,8 @@ static void armada_drm_primary_set(struct drm_crtc *crtc,
 	armada_reg_queue_mod(regs, i, ctrl0, CFG_GRAFORMAT |
 			     CFG_GRA_MOD(CFG_SWAPRB | CFG_SWAPUV |
 					 CFG_SWAPYU | CFG_YUV2RGB) |
-			     CFG_PALETTE_ENA | CFG_GRA_FTOGGLE,
+			     CFG_PALETTE_ENA | CFG_GRA_FTOGGLE |
+			     CFG_GRA_HSMOOTH | CFG_GRA_ENA,
 			     LCD_SPU_DMA_CTRL0);
 	armada_reg_queue_end(regs, i);
 	armada_drm_crtc_update_regs(dcrtc, regs);
@@ -605,7 +606,7 @@ static int armada_drm_crtc_mode_set(struct drm_crtc *crtc,
 
 	interlaced = !!(adj->flags & DRM_MODE_FLAG_INTERLACE);
 
-	val = CFG_GRA_ENA | CFG_GRA_HSMOOTH;
+	val = CFG_GRA_ENA;
 	val |= CFG_GRA_FMT(drm_fb_to_armada_fb(dcrtc->crtc.primary->fb)->fmt);
 	val |= CFG_GRA_MOD(drm_fb_to_armada_fb(dcrtc->crtc.primary->fb)->mod);
 
drivers/gpu/drm/armada/armada_overlay.c +5 -5
index e02d0d9d4c23..19fce1a7159f 100644
@@ -120,11 +120,11 @@ armada_ovl_plane_update(struct drm_plane *plane, struct drm_crtc *crtc,
 
 	ctrl0 = CFG_DMA_FMT(drm_fb_to_armada_fb(fb)->fmt) |
 		CFG_DMA_MOD(drm_fb_to_armada_fb(fb)->mod) |
-		CFG_CBSH_ENA | CFG_DMA_HSMOOTH | CFG_DMA_ENA;
-
-	/* Does the position/size result in nothing to display? */
-	if (!visible)
-		ctrl0 &= ~CFG_DMA_ENA;
+		CFG_CBSH_ENA;
+	if (visible)
+		ctrl0 |= CFG_DMA_ENA;
+	if (drm_rect_width(&src) >> 16 != drm_rect_width(&dest))
+		ctrl0 |= CFG_DMA_HSMOOTH;
 
 	/*
 	 * Shifting a YUV packed format image by one pixel causes the U/V
Signature · 6 lines
2.7.4

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
#21 lore raw

PATCH 15/25 drm/armada: use drm_plane_helper_check_state()

In reply to Russell King - ARM Linux

Use drm_plane_helper_check_state() to check the overlay plane state rather than drm_plane_helper_check_update(), as:

(1) using drm_plane_helper_check_state() provides a better migration path to atomic modeset
(2) it avoids needless copies of drm rectangle structures, and so is more efficient.

---
 drivers/gpu/drm/armada/armada_overlay.c | 61 +++++++++++++++++----------------
 1 file changed, 32 insertions(+), 29 deletions(-)
drivers/gpu/drm/armada/armada_overlay.c +32 -29
index 19fce1a7159f..010f3e438607 100644
@@ -87,17 +87,19 @@ armada_ovl_plane_update(struct drm_plane *plane, struct drm_crtc *crtc,
 	struct armada_crtc *dcrtc = drm_to_armada_crtc(crtc);
 	struct armada_plane_work *work = &dplane->work;
 	const struct drm_format_info *format;
-	struct drm_rect src = {
-		.x1 = src_x,
-		.y1 = src_y,
-		.x2 = src_x + src_w,
-		.y2 = src_y + src_h,
-	};
-	struct drm_rect dest = {
-		.x1 = crtc_x,
-		.y1 = crtc_y,
-		.x2 = crtc_x + crtc_w,
-		.y2 = crtc_y + crtc_h,
+	struct drm_plane_state state = {
+		.plane = plane,
+		.crtc = crtc,
+		.fb = fb,
+		.src_x = src_x,
+		.src_y = src_y,
+		.src_w = src_w,
+		.src_h = src_h,
+		.crtc_x = crtc_x,
+		.crtc_y = crtc_y,
+		.crtc_w = crtc_w,
+		.crtc_h = crtc_h,
+		.rotation = DRM_MODE_ROTATE_0,
 	};
 	const struct drm_rect clip = {
 		.x2 = crtc->mode.hdisplay,
@@ -105,25 +107,24 @@ armada_ovl_plane_update(struct drm_plane *plane, struct drm_crtc *crtc,
 	};
 	uint32_t val, ctrl0;
 	unsigned idx = 0;
-	bool visible, fb_changed;
+	bool fb_changed;
 	int ret;
 
 	trace_armada_ovl_plane_update(plane, crtc, fb,
 				 crtc_x, crtc_y, crtc_w, crtc_h,
 				 src_x, src_y, src_w, src_h);
 
-	ret = drm_plane_helper_check_update(plane, crtc, fb, &src, &dest, &clip,
-					    DRM_MODE_ROTATE_0,
-					    0, INT_MAX, true, false, &visible);
+	ret = drm_plane_helper_check_state(&state, &clip, 0, INT_MAX, true,
+					    false);
 	if (ret)
 		return ret;
 
 	ctrl0 = CFG_DMA_FMT(drm_fb_to_armada_fb(fb)->fmt) |
 		CFG_DMA_MOD(drm_fb_to_armada_fb(fb)->mod) |
 		CFG_CBSH_ENA;
-	if (visible)
+	if (state.visible)
 		ctrl0 |= CFG_DMA_ENA;
-	if (drm_rect_width(&src) >> 16 != drm_rect_width(&dest))
+	if (drm_rect_width(&state.src) >> 16 != drm_rect_width(&state.dst))
 		ctrl0 |= CFG_DMA_HSMOOTH;
 
 	/*
@@ -131,12 +132,12 @@ armada_ovl_plane_update(struct drm_plane *plane, struct drm_crtc *crtc,
 	 * planes to swap.  Compensate for it by also toggling the UV swap.
 	 */
 	format = fb->format;
-	if (format->num_planes == 1 && src.x1 >> 16 & (format->hsub - 1))
+	if (format->num_planes == 1 && state.src.x1 >> 16 & (format->hsub - 1))
 		ctrl0 ^= CFG_DMA_MOD(CFG_SWAPUV);
 
 	fb_changed = plane->fb != fb ||
-		     dplane->base.state.src_x != src.x1 >> 16 ||
-	             dplane->base.state.src_y != src.y1 >> 16;
+		     dplane->base.state.src_x != state.src.x1 >> 16 ||
+	             dplane->base.state.src_y != state.src.y1 >> 16;
 
 	if (!dcrtc->plane) {
 		dcrtc->plane = plane;
@@ -146,16 +147,17 @@ armada_ovl_plane_update(struct drm_plane *plane, struct drm_crtc *crtc,
 	/* FIXME: overlay on an interlaced display */
 	/* Just updating the position/size? */
 	if (!fb_changed && dplane->base.state.ctrl0 == ctrl0) {
-		val = (drm_rect_height(&src) & 0xffff0000) |
-		      drm_rect_width(&src) >> 16;
+		val = (drm_rect_height(&state.src) & 0xffff0000) |
+		       drm_rect_width(&state.src) >> 16;
 		dplane->base.state.src_hw = val;
 		writel_relaxed(val, dcrtc->base + LCD_SPU_DMA_HPXL_VLN);
 
-		val = drm_rect_height(&dest) << 16 | drm_rect_width(&dest);
+		val = drm_rect_height(&state.dst) << 16 |
+		      drm_rect_width(&state.dst);
 		dplane->base.state.dst_hw = val;
 		writel_relaxed(val, dcrtc->base + LCD_SPU_DZM_HPXL_VLN);
 
-		val = dest.y1 << 16 | dest.x1;
+		val = state.dst.y1 << 16 | state.dst.x1;
 		dplane->base.state.dst_yx = val;
 		writel_relaxed(val, dcrtc->base + LCD_SPU_DMA_OVSA_HPXL_VLN);
 
@@ -181,8 +183,8 @@ armada_ovl_plane_update(struct drm_plane *plane, struct drm_crtc *crtc,
 
 		work->old_fb = plane->fb;
 
-		dplane->base.state.src_y = src_y = src.y1 >> 16;
-		dplane->base.state.src_x = src_x = src.x1 >> 16;
+		dplane->base.state.src_y = src_y = state.src.y1 >> 16;
+		dplane->base.state.src_x = src_x = state.src.x1 >> 16;
 
 		armada_drm_plane_calc_addrs(addrs, fb, src_x, src_y);
 
@@ -209,21 +211,22 @@ armada_ovl_plane_update(struct drm_plane *plane, struct drm_crtc *crtc,
 		work->old_fb = NULL;
 	}
 
-	val = (drm_rect_height(&src) & 0xffff0000) | drm_rect_width(&src) >> 16;
+	val = (drm_rect_height(&state.src) & 0xffff0000) |
+	       drm_rect_width(&state.src) >> 16;
 	if (dplane->base.state.src_hw != val) {
 		dplane->base.state.src_hw = val;
 		armada_reg_queue_set(work->regs, idx, val,
 				     LCD_SPU_DMA_HPXL_VLN);
 	}
 
-	val = drm_rect_height(&dest) << 16 | drm_rect_width(&dest);
+	val = drm_rect_height(&state.dst) << 16 | drm_rect_width(&state.dst);
 	if (dplane->base.state.dst_hw != val) {
 		dplane->base.state.dst_hw = val;
 		armada_reg_queue_set(work->regs, idx, val,
 				     LCD_SPU_DZM_HPXL_VLN);
 	}
 
-	val = dest.y1 << 16 | dest.x1;
+	val = state.dst.y1 << 16 | state.dst.x1;
 	if (dplane->base.state.dst_yx != val) {
 		dplane->base.state.dst_yx = val;
 		armada_reg_queue_set(work->regs, idx, val,
Signature · 6 lines
2.7.4

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
#22 lore raw

PATCH 16/25 drm/armada: allow armada_drm_plane_work_queue() to silently fail

In reply to Russell King - ARM Linux

Avoid printing an error message when armada_drm_plane_work_queue() is unable to get the vblank (eg, because we're doing a modeset.) Continue to report the failure to the caller, so the caller can handle this.

Move the error message into armada_ovl_plane_update().

---
 drivers/gpu/drm/armada/armada_crtc.c    | 4 +---
 drivers/gpu/drm/armada/armada_overlay.c | 4 +++-
 2 files changed, 4 insertions(+), 4 deletions(-)
drivers/gpu/drm/armada/armada_crtc.c +1 -3
index 401ad854d751..2f8e45976444 100644
@@ -254,10 +254,8 @@ int armada_drm_plane_work_queue(struct armada_crtc *dcrtc,
 	int ret;
 
 	ret = drm_crtc_vblank_get(&dcrtc->crtc);
-	if (ret) {
-		DRM_ERROR("failed to acquire vblank counter\n");
+	if (ret)
 		return ret;
-	}
 
 	ret = cmpxchg(&plane->work, NULL, work) ? -EBUSY : 0;
 	if (ret)
drivers/gpu/drm/armada/armada_overlay.c +3 -1
index 010f3e438607..53edf42c5863 100644
@@ -245,7 +245,9 @@ armada_ovl_plane_update(struct drm_plane *plane, struct drm_crtc *crtc,
 	if (idx) {
 		armada_reg_queue_end(work->regs, idx);
 		/* Queue it for update on the next interrupt if we are enabled */
-		armada_drm_plane_work_queue(dcrtc, work);
+		ret = armada_drm_plane_work_queue(dcrtc, work);
+		if (ret)
+			DRM_ERROR("failed to queue plane work: %d\n", ret);
 	}
 	return 0;
 }
Signature · 6 lines
2.7.4

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
#23 lore raw

PATCH 17/25 drm/armada: avoid work allocation

In reply to Russell King - ARM Linux
---
 drivers/gpu/drm/armada/armada_crtc.c    | 24 +++++++++++++-----------
 drivers/gpu/drm/armada/armada_crtc.h    |  3 +++
 drivers/gpu/drm/armada/armada_overlay.c | 11 +++++++----
 3 files changed, 23 insertions(+), 15 deletions(-)
drivers/gpu/drm/armada/armada_crtc.c +13 -11
index 2f8e45976444..98fb955f6889 100644
@@ -214,13 +214,15 @@ static void armada_drm_plane_work_call(struct armada_crtc *dcrtc,
 	void (*fn)(struct armada_crtc *, struct armada_plane_work *))
 {
 	struct armada_plane *dplane = drm_to_armada_plane(work->plane);
-	struct drm_pending_vblank_event *event = work->event;
-	struct drm_framebuffer *fb = work->old_fb;
+	struct drm_pending_vblank_event *event;
+	struct drm_framebuffer *fb;
 
 	if (fn)
 		fn(dcrtc, work);
 	drm_crtc_vblank_put(&dcrtc->crtc);
 
+	event = work->event;
+	fb = work->old_fb;
 	if (event || fb) {
 		struct drm_device *dev = dcrtc->crtc.dev;
 		unsigned long flags;
@@ -233,6 +235,9 @@ static void armada_drm_plane_work_call(struct armada_crtc *dcrtc,
 		spin_unlock_irqrestore(&dev->event_lock, flags);
 	}
 
+	if (work->need_kfree)
+		kfree(work);
+
 	wake_up(&dplane->frame_wait);
 }
 
@@ -278,12 +283,6 @@ void armada_drm_plane_work_cancel(struct armada_crtc *dcrtc,
 		armada_drm_plane_work_call(dcrtc, work, work->cancel);
 }
 
-static void armada_drm_crtc_finish_frame_work(struct armada_crtc *dcrtc,
-	struct armada_plane_work *work)
-{
-	kfree(work);
-}
-
 static void armada_drm_crtc_complete_frame_work(struct armada_crtc *dcrtc,
 	struct armada_plane_work *work)
 {
@@ -292,8 +291,6 @@ static void armada_drm_crtc_complete_frame_work(struct armada_crtc *dcrtc,
 	spin_lock_irqsave(&dcrtc->irq_lock, flags);
 	armada_drm_crtc_update_regs(dcrtc, work->regs);
 	spin_unlock_irqrestore(&dcrtc->irq_lock, flags);
-
-	armada_drm_crtc_finish_frame_work(dcrtc, work);
 }
 
 static struct armada_plane_work *
@@ -308,7 +305,7 @@ armada_drm_crtc_alloc_plane_work(struct drm_plane *plane)
 
 	work->plane = plane;
 	work->fn = armada_drm_crtc_complete_frame_work;
-	work->cancel = armada_drm_crtc_finish_frame_work;
+	work->need_kfree = true;
 	armada_reg_queue_end(work->regs, i);
 
 	return work;
@@ -1173,6 +1170,11 @@ static const struct drm_plane_funcs armada_primary_plane_funcs = {
 
 int armada_drm_plane_init(struct armada_plane *plane)
 {
+	unsigned int i;
+
+	for (i = 0; i < ARRAY_SIZE(plane->works); i++)
+		plane->works[i].plane = &plane->base;
+
 	init_waitqueue_head(&plane->frame_wait);
 
 	return 0;
drivers/gpu/drm/armada/armada_crtc.h +3 -0
index 12ef9688a45a..0c7b519c09e8 100644
@@ -38,6 +38,7 @@ struct armada_variant;
 struct armada_plane_work {
 	void (*fn)(struct armada_crtc *, struct armada_plane_work *);
 	void (*cancel)(struct armada_crtc *, struct armada_plane_work *);
+	bool need_kfree;
 	struct drm_plane *plane;
 	struct drm_framebuffer *old_fb;
 	struct drm_pending_vblank_event *event;
@@ -56,6 +57,8 @@ struct armada_plane_state {
 struct armada_plane {
 	struct drm_plane	base;
 	wait_queue_head_t	frame_wait;
+	bool			next_work;
+	struct armada_plane_work works[2];
 	struct armada_plane_work *work;
 	struct armada_plane_state state;
 };
drivers/gpu/drm/armada/armada_overlay.c +7 -4
index 53edf42c5863..bad966ae6758 100644
@@ -32,7 +32,6 @@ struct armada_ovl_plane_properties {
 
 struct armada_ovl_plane {
 	struct armada_plane base;
-	struct armada_plane_work work;
 	struct armada_ovl_plane_properties prop;
 };
 #define drm_to_armada_ovl_plane(p) \
@@ -85,7 +84,7 @@ armada_ovl_plane_update(struct drm_plane *plane, struct drm_crtc *crtc,
 {
 	struct armada_ovl_plane *dplane = drm_to_armada_ovl_plane(plane);
 	struct armada_crtc *dcrtc = drm_to_armada_crtc(crtc);
-	struct armada_plane_work *work = &dplane->work;
+	struct armada_plane_work *work;
 	const struct drm_format_info *format;
 	struct drm_plane_state state = {
 		.plane = plane,
@@ -119,6 +118,8 @@ armada_ovl_plane_update(struct drm_plane *plane, struct drm_crtc *crtc,
 	if (ret)
 		return ret;
 
+	work = &dplane->base.works[dplane->base.next_work];
+
 	ctrl0 = CFG_DMA_FMT(drm_fb_to_armada_fb(fb)->fmt) |
 		CFG_DMA_MOD(drm_fb_to_armada_fb(fb)->mod) |
 		CFG_CBSH_ENA;
@@ -248,6 +249,8 @@ armada_ovl_plane_update(struct drm_plane *plane, struct drm_crtc *crtc,
 		ret = armada_drm_plane_work_queue(dcrtc, work);
 		if (ret)
 			DRM_ERROR("failed to queue plane work: %d\n", ret);
+
+		dplane->base.next_work = !dplane->base.next_work;
 	}
 	return 0;
 }
@@ -434,8 +437,8 @@ int armada_overlay_plane_create(struct drm_device *dev, unsigned long crtcs)
 		return ret;
 	}
 
-	dplane->work.plane = &dplane->base.base;
-	dplane->work.fn = armada_ovl_plane_work;
+	dplane->base.works[0].fn = armada_ovl_plane_work;
+	dplane->base.works[1].fn = armada_ovl_plane_work;
 
 	ret = drm_universal_plane_init(dev, &dplane->base.base, crtcs,
 				       &armada_ovl_plane_funcs,
Signature · 6 lines
2.7.4

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
#24 lore raw

PATCH 18/25 drm/armada: disable planes at next blanking period

In reply to Russell King - ARM Linux

Disable planes at the next blanking period rather than immediately. In order to achieve this, we need to delay the clearing of dcrtc->plane until after the next blanking period, so move that into a separate work function. To avoid races, we also need to move its assignment in the overlay code.

---
 drivers/gpu/drm/armada/armada_crtc.c    | 59 ++++++++++++++++++++++++---------
 drivers/gpu/drm/armada/armada_overlay.c | 23 ++++---------
 2 files changed, 50 insertions(+), 32 deletions(-)
drivers/gpu/drm/armada/armada_crtc.c +44 -15
index 98fb955f6889..c38a1409a14e 100644
@@ -293,6 +293,19 @@ static void armada_drm_crtc_complete_frame_work(struct armada_crtc *dcrtc,
 	spin_unlock_irqrestore(&dcrtc->irq_lock, flags);
 }
 
+static void armada_drm_crtc_complete_disable_work(struct armada_crtc *dcrtc,
+	struct armada_plane_work *work)
+{
+	unsigned long flags;
+
+	if (dcrtc->plane == work->plane)
+		dcrtc->plane = NULL;
+
+	spin_lock_irqsave(&dcrtc->irq_lock, flags);
+	armada_drm_crtc_update_regs(dcrtc, work->regs);
+	spin_unlock_irqrestore(&dcrtc->irq_lock, flags);
+}
+
 static struct armada_plane_work *
 armada_drm_crtc_alloc_plane_work(struct drm_plane *plane)
 {
@@ -392,8 +405,11 @@ static void armada_drm_crtc_prepare(struct drm_crtc *crtc)
 	 * the new mode parameters.
 	 */
 	plane = dcrtc->plane;
-	if (plane)
+	if (plane) {
 		drm_plane_force_disable(plane);
+		WARN_ON(!armada_drm_plane_work_wait(drm_to_armada_plane(plane),
+						    HZ));
+	}
 }
 
 /* The mode_config.mutex will be held for this call */
@@ -1120,28 +1136,22 @@ int armada_drm_plane_disable(struct drm_plane *plane,
 {
 	struct armada_plane *dplane = drm_to_armada_plane(plane);
 	struct armada_crtc *dcrtc;
+	struct armada_plane_work *work;
+	unsigned int idx = 0;
 	u32 sram_para1, enable_mask;
 
 	if (!plane->crtc)
 		return 0;
 
 	/*
-	 * Drop our reference on any framebuffer attached to this plane.
-	 * We don't need to NULL this out as drm_plane_force_disable(),
-	 * and __setplane_internal() will do so for an overlay plane, and
-	 * __drm_helper_disable_unused_functions() will do so for the
-	 * primary plane.
+	 * Arrange to power down most RAMs and FIFOs if this is the primary
+	 * plane, otherwise just the YUV FIFOs for the overlay plane.
 	 */
-	if (plane->fb)
-		drm_framebuffer_put(plane->fb);
-
-	/* Power down most RAMs and FIFOs if this is the primary plane */
 	if (plane->type == DRM_PLANE_TYPE_PRIMARY) {
 		sram_para1 = CFG_PDWN256x32 | CFG_PDWN256x24 | CFG_PDWN256x8 |
 			     CFG_PDWN32x32 | CFG_PDWN64x66;
 		enable_mask = CFG_GRA_ENA;
 	} else {
-		/* Power down the Y/U/V FIFOs */
 		sram_para1 = CFG_PDWN16x66 | CFG_PDWN32x66;
 		enable_mask = CFG_DMA_ENA;
 	}
@@ -1150,14 +1160,33 @@ int armada_drm_plane_disable(struct drm_plane *plane,
 
 	dcrtc = drm_to_armada_crtc(plane->crtc);
 
+	/*
+	 * Try to disable the plane and drop our ref on the framebuffer
+	 * at the next frame update. If we fail for any reason, disable
+	 * the plane immediately.
+	 */
+	work = &dplane->works[dplane->next_work];
+	work->fn = armada_drm_crtc_complete_disable_work;
+	work->cancel = armada_drm_crtc_complete_disable_work;
+	work->old_fb = plane->fb;
+
+	armada_reg_queue_mod(work->regs, idx,
+			     0, enable_mask, LCD_SPU_DMA_CTRL0);
+	armada_reg_queue_mod(work->regs, idx,
+			     sram_para1, 0, LCD_SPU_SRAM_PARA1);
+	armada_reg_queue_end(work->regs, idx);
+
 	/* Wait for any preceding work to complete, but don't wedge */
 	if (WARN_ON(!armada_drm_plane_work_wait(dplane, HZ)))
 		armada_drm_plane_work_cancel(dcrtc, dplane);
 
-	spin_lock_irq(&dcrtc->irq_lock);
-	armada_updatel(0, enable_mask, dcrtc->base + LCD_SPU_DMA_CTRL0);
-	armada_updatel(sram_para1, 0, dcrtc->base + LCD_SPU_SRAM_PARA1);
-	spin_unlock_irq(&dcrtc->irq_lock);
+	if (armada_drm_plane_work_queue(dcrtc, work)) {
+		work->fn(dcrtc, work);
+		if (work->old_fb)
+			drm_framebuffer_unreference(work->old_fb);
+	}
+
+	dplane->next_work = !dplane->next_work;
 
 	return 0;
 }
drivers/gpu/drm/armada/armada_overlay.c +6 -17
index bad966ae6758..0fe3f2db8ff5 100644
@@ -140,11 +140,6 @@ armada_ovl_plane_update(struct drm_plane *plane, struct drm_crtc *crtc,
 		     dplane->base.state.src_x != state.src.x1 >> 16 ||
 	             dplane->base.state.src_y != state.src.y1 >> 16;
 
-	if (!dcrtc->plane) {
-		dcrtc->plane = plane;
-		armada_ovl_update_attr(&dplane->prop, dcrtc);
-	}
-
 	/* FIXME: overlay on an interlaced display */
 	/* Just updating the position/size? */
 	if (!fb_changed && dplane->base.state.ctrl0 == ctrl0) {
@@ -173,6 +168,11 @@ armada_ovl_plane_update(struct drm_plane *plane, struct drm_crtc *crtc,
 	if (armada_drm_plane_work_wait(&dplane->base, HZ / 25) == 0)
 		armada_drm_plane_work_cancel(dcrtc, &dplane->base);
 
+	if (!dcrtc->plane) {
+		dcrtc->plane = plane;
+		armada_ovl_update_attr(&dplane->prop, dcrtc);
+	}
+
 	if (fb_changed) {
 		u32 addrs[3];
 
@@ -255,17 +255,6 @@ armada_ovl_plane_update(struct drm_plane *plane, struct drm_crtc *crtc,
 	return 0;
 }
 
-static int armada_ovl_plane_disable(struct drm_plane *plane,
-				    struct drm_modeset_acquire_ctx *ctx)
-{
-	armada_drm_plane_disable(plane, ctx);
-
-	if (plane->crtc)
-		drm_to_armada_crtc(plane->crtc)->plane = NULL;
-
-	return 0;
-}
-
 static void armada_ovl_plane_destroy(struct drm_plane *plane)
 {
 	struct armada_ovl_plane *dplane = drm_to_armada_ovl_plane(plane);
@@ -345,7 +334,7 @@ static int armada_ovl_plane_set_property(struct drm_plane *plane,
 
 static const struct drm_plane_funcs armada_ovl_plane_funcs = {
 	.update_plane	= armada_ovl_plane_update,
-	.disable_plane	= armada_ovl_plane_disable,
+	.disable_plane	= armada_drm_plane_disable,
 	.destroy	= armada_ovl_plane_destroy,
 	.set_property	= armada_ovl_plane_set_property,
 };
Signature · 6 lines
2.7.4

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
#25 lore raw

PATCH 19/25 drm/armada: re-organise overlay register update generation

In reply to Russell King - ARM Linux

Re-organise overlay register generation so that we do not have to wait for the previous update to complete while creating the new state. This allows the update to be fully prepared before queueing it for the next interrupt.

---
 drivers/gpu/drm/armada/armada_overlay.c | 52 ++++++++++++++-------------------
 1 file changed, 22 insertions(+), 30 deletions(-)
drivers/gpu/drm/armada/armada_overlay.c +22 -30
index 0fe3f2db8ff5..00da2c58701c 100644
@@ -136,43 +136,18 @@ armada_ovl_plane_update(struct drm_plane *plane, struct drm_crtc *crtc,
 	if (format->num_planes == 1 && state.src.x1 >> 16 & (format->hsub - 1))
 		ctrl0 ^= CFG_DMA_MOD(CFG_SWAPUV);
 
-	fb_changed = plane->fb != fb ||
-		     dplane->base.state.src_x != state.src.x1 >> 16 ||
-	             dplane->base.state.src_y != state.src.y1 >> 16;
-
-	/* FIXME: overlay on an interlaced display */
-	/* Just updating the position/size? */
-	if (!fb_changed && dplane->base.state.ctrl0 == ctrl0) {
-		val = (drm_rect_height(&state.src) & 0xffff0000) |
-		       drm_rect_width(&state.src) >> 16;
-		dplane->base.state.src_hw = val;
-		writel_relaxed(val, dcrtc->base + LCD_SPU_DMA_HPXL_VLN);
-
-		val = drm_rect_height(&state.dst) << 16 |
-		      drm_rect_width(&state.dst);
-		dplane->base.state.dst_hw = val;
-		writel_relaxed(val, dcrtc->base + LCD_SPU_DZM_HPXL_VLN);
-
-		val = state.dst.y1 << 16 | state.dst.x1;
-		dplane->base.state.dst_yx = val;
-		writel_relaxed(val, dcrtc->base + LCD_SPU_DMA_OVSA_HPXL_VLN);
-
-		return 0;
-	} else if (~dplane->base.state.ctrl0 & ctrl0 & CFG_DMA_ENA) {
+	if (~dplane->base.state.ctrl0 & ctrl0 & CFG_DMA_ENA) {
 		/* Power up the Y/U/V FIFOs on ENA 0->1 transitions */
 		armada_reg_queue_mod(work->regs, idx,
 				     0, CFG_PDWN16x66 | CFG_PDWN32x66,
 				     LCD_SPU_SRAM_PARA1);
 	}
 
-	if (armada_drm_plane_work_wait(&dplane->base, HZ / 25) == 0)
-		armada_drm_plane_work_cancel(dcrtc, &dplane->base);
-
-	if (!dcrtc->plane) {
-		dcrtc->plane = plane;
-		armada_ovl_update_attr(&dplane->prop, dcrtc);
-	}
+	fb_changed = plane->fb != fb ||
+		     dplane->base.state.src_x != state.src.x1 >> 16 ||
+	             dplane->base.state.src_y != state.src.y1 >> 16;
 
+	/* FIXME: overlay on an interlaced display */
 	if (fb_changed) {
 		u32 addrs[3];
 
@@ -243,6 +218,23 @@ armada_ovl_plane_update(struct drm_plane *plane, struct drm_crtc *crtc,
 			CFG_YUV2RGB) | CFG_DMA_ENA,
 			LCD_SPU_DMA_CTRL0);
 	}
+
+	/* Just updating the position/size? */
+	if (!fb_changed && dplane->base.state.ctrl0 == ctrl0) {
+		armada_reg_queue_end(work->regs, idx);
+		armada_ovl_plane_work(dcrtc, work);
+		return 0;
+	}
+
+	/* Wait for pending work to complete */
+	if (armada_drm_plane_work_wait(&dplane->base, HZ / 25) == 0)
+		armada_drm_plane_work_cancel(dcrtc, &dplane->base);
+
+	if (!dcrtc->plane) {
+		dcrtc->plane = plane;
+		armada_ovl_update_attr(&dplane->prop, dcrtc);
+	}
+
 	if (idx) {
 		armada_reg_queue_end(work->regs, idx);
 		/* Queue it for update on the next interrupt if we are enabled */
Signature · 6 lines
2.7.4

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
#26 lore raw

PATCH 20/25 drm/armada: move overlay plane register update generation

In reply to Russell King - ARM Linux

Move the overlay plane register update generation to a separate function as this is independent of the legacy or atomic update.

---
 drivers/gpu/drm/armada/armada_crtc.h    |   2 +
 drivers/gpu/drm/armada/armada_overlay.c | 203 +++++++++++++++++---------------
 2 files changed, 112 insertions(+), 93 deletions(-)
drivers/gpu/drm/armada/armada_crtc.h +2 -0
index 0c7b519c09e8..445829b8877a 100644
@@ -52,6 +52,8 @@ struct armada_plane_state {
 	u32 dst_hw;
 	u32 dst_yx;
 	u32 ctrl0;
+	bool changed;
+	bool vsync_update;
 };
 
 struct armada_plane {
drivers/gpu/drm/armada/armada_overlay.c +110 -93

278 lines not on this page · raw message

Signature · 6 lines
2.7.4

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
#27 lore raw

PATCH 21/25 drm/armada: wait for previous work when moving overlay window

In reply to Russell King - ARM Linux

We must wait for the previous plane work to complete before moving the overlay window, as it could overwrite our positioning update.

---
 drivers/gpu/drm/armada/armada_overlay.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
drivers/gpu/drm/armada/armada_overlay.c +4 -4
index e5fa346f572b..853f889e84f5 100644
@@ -239,16 +239,16 @@ armada_ovl_plane_update(struct drm_plane *plane, struct drm_crtc *crtc,
 	if (!dplane->base.state.changed)
 		return 0;
 
+	/* Wait for pending work to complete */
+	if (armada_drm_plane_work_wait(&dplane->base, HZ / 25) == 0)
+		armada_drm_plane_work_cancel(dcrtc, &dplane->base);
+
 	/* Just updating the position/size? */
 	if (!dplane->base.state.vsync_update) {
 		armada_ovl_plane_work(dcrtc, work);
 		return 0;
 	}
 
-	/* Wait for pending work to complete */
-	if (armada_drm_plane_work_wait(&dplane->base, HZ / 25) == 0)
-		armada_drm_plane_work_cancel(dcrtc, &dplane->base);
-
 	if (!dcrtc->plane) {
 		dcrtc->plane = plane;
 		armada_ovl_update_attr(&dplane->prop, dcrtc);
Signature · 6 lines
2.7.4

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
#28 lore raw

PATCH 22/25 drm/armada: extract register generation from armada_drm_primary_set()

In reply to Russell King - ARM Linux

Extract the register generation from armada_drm_primary_set(), so that it can be re-used.

---
 drivers/gpu/drm/armada/armada_crtc.c | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)
drivers/gpu/drm/armada/armada_crtc.c +16 -9
index c38a1409a14e..9958f2d3d0e8 100644
@@ -571,18 +571,14 @@ static uint32_t armada_drm_crtc_calculate_csc(struct armada_crtc *dcrtc)
 	return val;
 }
 
-static void armada_drm_primary_set(struct drm_crtc *crtc,
-	struct drm_plane *plane, int x, int y)
+static void armada_drm_gra_plane_regs(struct armada_regs *regs,
+	struct drm_framebuffer *fb, struct armada_plane_state *state,
+	int x, int y, bool interlaced)
 {
-	struct armada_plane_state *state = &drm_to_armada_plane(plane)->state;
-	struct armada_crtc *dcrtc = drm_to_armada_crtc(crtc);
-	struct armada_regs regs[8];
-	bool interlaced = dcrtc->interlaced;
-	unsigned i;
+	unsigned int i;
 	u32 ctrl0;
 
-	i = armada_drm_crtc_calc_fb(plane->fb, x, y, regs, interlaced);
-
+	i = armada_drm_crtc_calc_fb(fb, x, y, regs, interlaced);
 	armada_reg_queue_set(regs, i, state->dst_yx, LCD_SPU_GRA_OVSA_HPXL_VLN);
 	armada_reg_queue_set(regs, i, state->src_hw, LCD_SPU_GRA_HPXL_VLN);
 	armada_reg_queue_set(regs, i, state->dst_hw, LCD_SPU_GZM_HPXL_VLN);
@@ -598,6 +594,17 @@ static void armada_drm_primary_set(struct drm_crtc *crtc,
 			     CFG_GRA_HSMOOTH | CFG_GRA_ENA,
 			     LCD_SPU_DMA_CTRL0);
 	armada_reg_queue_end(regs, i);
+}
+
+static void armada_drm_primary_set(struct drm_crtc *crtc,
+	struct drm_plane *plane, int x, int y)
+{
+	struct armada_plane_state *state = &drm_to_armada_plane(plane)->state;
+	struct armada_crtc *dcrtc = drm_to_armada_crtc(crtc);
+	struct armada_regs regs[8];
+	bool interlaced = dcrtc->interlaced;
+
+	armada_drm_gra_plane_regs(regs, plane->fb, state, x, y, interlaced);
 	armada_drm_crtc_update_regs(dcrtc, regs);
 }
 
Signature · 6 lines
2.7.4

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
#29 lore raw

PATCH 23/25 drm/armada: implement primary plane update

In reply to Russell King - ARM Linux

Implement primary plane update without having to go through a modeset to achieve that; the hardware does not require such complexity. This means we treat the primary plane as any other, allowing the format, size, position and scaling to be updated via the normal plane ioctls.

This also allows us to seemlessly disable and re-enable the primary plane when (eg) displaying full-frame video without any graphic clipping the overlaid video - which saves wasting memory bandwidth needlessly verifying that the colorkey is indeed filling the entire primary plane.

---
 drivers/gpu/drm/armada/armada_crtc.c | 118 ++++++++++++++++++++++++++++++++++-
 1 file changed, 117 insertions(+), 1 deletion(-)
drivers/gpu/drm/armada/armada_crtc.c +117 -1
index 9958f2d3d0e8..8b66377a4890 100644
@@ -1138,6 +1138,122 @@ static const struct drm_crtc_funcs armada_crtc_funcs = {
 	.disable_vblank	= armada_drm_crtc_disable_vblank,
 };
 
+static void armada_drm_primary_update_state(struct drm_plane_state *state,
+	struct armada_regs *regs)
+{
+	struct armada_plane *dplane = drm_to_armada_plane(state->plane);
+	struct armada_crtc *dcrtc = drm_to_armada_crtc(state->crtc);
+	struct armada_framebuffer *dfb = drm_fb_to_armada_fb(state->fb);
+	bool was_disabled;
+	unsigned int idx = 0;
+	u32 val;
+
+	val = CFG_GRA_FMT(dfb->fmt) | CFG_GRA_MOD(dfb->mod);
+	if (dfb->fmt > CFG_420)
+		val |= CFG_PALETTE_ENA;
+	if (state->visible)
+		val |= CFG_GRA_ENA;
+	if (drm_rect_width(&state->src) >> 16 != drm_rect_width(&state->dst))
+		val |= CFG_GRA_HSMOOTH;
+
+	was_disabled = !(dplane->state.ctrl0 & CFG_GRA_ENA);
+	if (was_disabled)
+		armada_reg_queue_mod(regs, idx,
+				     0, CFG_PDWN64x66, LCD_SPU_SRAM_PARA1);
+
+	dplane->state.ctrl0 = val;
+	dplane->state.src_hw = (drm_rect_height(&state->src) & 0xffff0000) |
+				drm_rect_width(&state->src) >> 16;
+	dplane->state.dst_hw = drm_rect_height(&state->dst) << 16 |
+			       drm_rect_width(&state->dst);
+	dplane->state.dst_yx = state->dst.y1 << 16 | state->dst.x1;
+
+	armada_drm_gra_plane_regs(regs + idx, &dfb->fb, &dplane->state,
+				  state->src.x1 >> 16, state->src.y1 >> 16,
+				  dcrtc->interlaced);
+
+	dplane->state.vsync_update = !was_disabled;
+	dplane->state.changed = true;
+}
+
+static int armada_drm_primary_update(struct drm_plane *plane,
+	struct drm_crtc *crtc, struct drm_framebuffer *fb,
+	int crtc_x, int crtc_y, unsigned int crtc_w, unsigned int crtc_h,
+	uint32_t src_x, uint32_t src_y, uint32_t src_w, uint32_t src_h,
+	struct drm_modeset_acquire_ctx *ctx)
+{
+	struct armada_plane *dplane = drm_to_armada_plane(plane);
+	struct armada_crtc *dcrtc = drm_to_armada_crtc(crtc);
+	struct armada_plane_work *work;
+	struct drm_plane_state state = {
+		.plane = plane,
+		.crtc = crtc,
+		.fb = fb,
+		.src_x = src_x,
+		.src_y = src_y,
+		.src_w = src_w,
+		.src_h = src_h,
+		.crtc_x = crtc_x,
+		.crtc_y = crtc_y,
+		.crtc_w = crtc_w,
+		.crtc_h = crtc_h,
+		.rotation = DRM_MODE_ROTATE_0,
+	};
+	const struct drm_rect clip = {
+		.x2 = crtc->mode.hdisplay,
+		.y2 = crtc->mode.vdisplay,
+	};
+	int ret;
+
+	ret = drm_plane_helper_check_state(&state, &clip, 0, INT_MAX, true,
+					    false);
+	if (ret)
+		return ret;
+
+	work = &dplane->works[dplane->next_work];
+	work->fn = armada_drm_crtc_complete_frame_work;
+
+	if (plane->fb != fb) {
+		/*
+		 * Take a reference on the new framebuffer - we want to
+		 * hold on to it while the hardware is displaying it.
+		 */
+		drm_framebuffer_reference(fb);
+
+		work->old_fb = plane->fb;
+	} else {
+		work->old_fb = NULL;
+	}
+
+	armada_drm_primary_update_state(&state, work->regs);
+
+	if (!dplane->state.changed)
+		return 0;
+
+	/* Wait for pending work to complete */
+	if (armada_drm_plane_work_wait(dplane, HZ / 10) == 0)
+		armada_drm_plane_work_cancel(dcrtc, dplane);
+
+	if (!dplane->state.vsync_update) {
+		work->fn(dcrtc, work);
+		if (work->old_fb)
+			drm_framebuffer_unreference(work->old_fb);
+		return 0;
+	}
+
+	/* Queue it for update on the next interrupt if we are enabled */
+	ret = armada_drm_plane_work_queue(dcrtc, work);
+	if (ret) {
+		work->fn(dcrtc, work);
+		if (work->old_fb)
+			drm_framebuffer_unreference(work->old_fb);
+	}
+
+	dplane->next_work = !dplane->next_work;
+
+	return 0;
+}
+
 int armada_drm_plane_disable(struct drm_plane *plane,
 			     struct drm_modeset_acquire_ctx *ctx)
 {
@@ -1199,7 +1315,7 @@ int armada_drm_plane_disable(struct drm_plane *plane,
 }
 
 static const struct drm_plane_funcs armada_primary_plane_funcs = {
-	.update_plane	= drm_primary_helper_update,
+	.update_plane	= armada_drm_primary_update,
 	.disable_plane	= armada_drm_plane_disable,
 	.destroy	= drm_primary_helper_destroy,
 };
Signature · 6 lines
2.7.4

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
#30 lore raw

PATCH 24/25 drm/armada: expand overlay trace entry

In reply to Russell King - ARM Linux

Add CRTC and source positions to the Armada overlay trace entry.

---
 drivers/gpu/drm/armada/armada_trace.h | 24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)
drivers/gpu/drm/armada/armada_trace.h +22 -2
index 8dbfea7a00fe..f03a56bda596 100644
@@ -34,14 +34,34 @@ TRACE_EVENT(armada_ovl_plane_update,
 		__field(struct drm_plane *, plane)
 		__field(struct drm_crtc *, crtc)
 		__field(struct drm_framebuffer *, fb)
+		__field(int, crtc_x)
+		__field(int, crtc_y)
+		__field(unsigned int, crtc_w)
+		__field(unsigned int, crtc_h)
+		__field(u32, src_x)
+		__field(u32, src_y)
+		__field(u32, src_w)
+		__field(u32, src_h)
 	),
 	TP_fast_assign(
 		__entry->plane = plane;
 		__entry->crtc = crtc;
 		__entry->fb = fb;
+		__entry->crtc_x = crtc_x;
+		__entry->crtc_y = crtc_y;
+		__entry->crtc_w = crtc_w;
+		__entry->crtc_h = crtc_h;
+		__entry->src_x = src_x;
+		__entry->src_y = src_y;
+		__entry->src_w = src_w;
+		__entry->src_h = src_h;
 	),
-	TP_printk("plane %p crtc %p fb %p",
-		__entry->plane, __entry->crtc, __entry->fb)
+	TP_printk("plane %p crtc %p fb %p crtc @ (%d,%d, %ux%u) src @ (%u,%u, %ux%u)",
+		__entry->plane, __entry->crtc, __entry->fb,
+		__entry->crtc_x, __entry->crtc_y,
+		__entry->crtc_w, __entry->crtc_h,
+		__entry->src_x >> 16, __entry->src_y >> 16,
+		__entry->src_w >> 16, __entry->src_h >> 16)
 );
 
 TRACE_EVENT(armada_ovl_plane_work,
Signature · 6 lines
2.7.4

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
#31 lore raw

PATCH 25/25 drm/armada: add iturbt_709 plane property to control YUV colorspace

In reply to Russell King - ARM Linux

Add the defacto-standard "iturbt_709" property to the overlay plane to control the YUV to RGB colorspace conversion. This is mutually exclusive with the CSC_YUV CRTC property - the last property to be set determines the resulting colorspace conversion.

---
 drivers/gpu/drm/armada/armada_crtc.c    | 26 +++++++++++++++++++++++---
 drivers/gpu/drm/armada/armada_crtc.h    |  2 ++
 drivers/gpu/drm/armada/armada_drm.h     |  1 +
 drivers/gpu/drm/armada/armada_overlay.c | 25 ++++++++++++++++++++++---
 4 files changed, 48 insertions(+), 6 deletions(-)
drivers/gpu/drm/armada/armada_crtc.c +23 -3
index 8b66377a4890..1bf632bb8855 100644
@@ -535,12 +535,30 @@ static irqreturn_t armada_drm_irq(int irq, void *arg)
 	return IRQ_NONE;
 }
 
+void armada_drm_crtc_set_yuv_colorimetry(struct armada_crtc *dcrtc, u32 csc)
+{
+	armada_updatel(csc, CFG_CSC_YUV_CCIR709,
+		       dcrtc->base + LCD_SPU_IOPAD_CONTROL);
+
+	dcrtc->csc_yuv_ovl_mode = csc == CFG_CSC_YUV_CCIR709 ?
+				CSC_YUV_CCIR709 : CSC_YUV_CCIR601;
+}
+
 static uint32_t armada_drm_crtc_calculate_csc(struct armada_crtc *dcrtc)
 {
 	struct drm_display_mode *adj = &dcrtc->crtc.mode;
-	uint32_t val = 0;
+	u32 val = 0;
+	u8 csc_yuv_mode;
+
+	/*
+	 * If the iturbt_709 overlay plane property is used, it overrides
+	 * the CSC_YUV crtc property until the CSC_YUV property is set.
+	 */
+	csc_yuv_mode = dcrtc->csc_yuv_ovl_mode;
+	if (csc_yuv_mode == CSC_AUTO)
+		csc_yuv_mode = dcrtc->csc_yuv_mode;
 
-	if (dcrtc->csc_yuv_mode == CSC_YUV_CCIR709)
+	if (csc_yuv_mode == CSC_YUV_CCIR709)
 		val |= CFG_CSC_YUV_CCIR709;
 	if (dcrtc->csc_rgb_mode == CSC_RGB_STUDIO)
 		val |= CFG_CSC_RGB_STUDIO;
@@ -555,7 +573,7 @@ static uint32_t armada_drm_crtc_calculate_csc(struct armada_crtc *dcrtc)
 	if ((adj->hdisplay == 1280 && adj->vdisplay == 720 &&
 	     !(adj->flags & DRM_MODE_FLAG_INTERLACE)) ||
 	    (adj->hdisplay == 1920 && adj->vdisplay == 1080)) {
-		if (dcrtc->csc_yuv_mode == CSC_AUTO)
+		if (csc_yuv_mode == CSC_AUTO)
 			val |= CFG_CSC_YUV_CCIR709;
 	}
 
@@ -1094,6 +1112,7 @@ armada_drm_crtc_set_property(struct drm_crtc *crtc,
 
 	if (property == priv->csc_yuv_prop) {
 		dcrtc->csc_yuv_mode = val;
+		dcrtc->csc_yuv_ovl_mode = CSC_AUTO;
 		update_csc = true;
 	} else if (property == priv->csc_rgb_prop) {
 		dcrtc->csc_rgb_mode = val;
@@ -1396,6 +1415,7 @@ static int armada_drm_crtc_create(struct drm_device *drm, struct device *dev,
 	dcrtc->num = drm->mode_config.num_crtc;
 	dcrtc->clk = ERR_PTR(-EINVAL);
 	dcrtc->csc_yuv_mode = CSC_AUTO;
+	dcrtc->csc_yuv_ovl_mode = CSC_AUTO;
 	dcrtc->csc_rgb_mode = CSC_AUTO;
 	dcrtc->cfg_dumb_ctrl = DUMB24_RGB888_0;
 	dcrtc->spu_iopad_ctrl = CFG_VSCALE_LN_EN | CFG_IOPAD_DUMB24;
drivers/gpu/drm/armada/armada_crtc.h +2 -0
index 445829b8877a..5d70b4af20b7 100644
@@ -90,6 +90,7 @@ struct armada_crtc {
 	bool			interlaced;
 	bool			cursor_update;
 	uint8_t			csc_yuv_mode;
+	uint8_t			csc_yuv_ovl_mode;
 	uint8_t			csc_rgb_mode;
 
 	struct drm_plane	*plane;
@@ -113,6 +114,7 @@ struct armada_crtc {
 #define drm_to_armada_crtc(c) container_of(c, struct armada_crtc, crtc)
 
 void armada_drm_crtc_update_regs(struct armada_crtc *, struct armada_regs *);
+void armada_drm_crtc_set_yuv_colorimetry(struct armada_crtc *dcrtc, u32 csc);
 
 int armada_drm_plane_disable(struct drm_plane *plane,
 			     struct drm_modeset_acquire_ctx *ctx);
drivers/gpu/drm/armada/armada_drm.h +1 -0
index b064879ecdbd..45d5168d5748 100644
@@ -60,6 +60,7 @@ struct armada_private {
 	struct armada_crtc	*dcrtc[2];
 	struct drm_mm		linear; /* protected by linear_lock */
 	struct mutex		linear_lock;
+	struct drm_property	*iturbt709_prop;
 	struct drm_property	*csc_yuv_prop;
 	struct drm_property	*csc_rgb_prop;
 	struct drm_property	*colorkey_prop;
drivers/gpu/drm/armada/armada_overlay.c +22 -3
index 853f889e84f5..d7a9c79f0ada 100644
@@ -28,6 +28,8 @@ struct armada_ovl_plane_properties {
 	uint16_t contrast;
 	uint16_t saturation;
 	uint32_t colorkey_mode;
+	uint32_t csc;
+	bool csc_set;
 };
 
 struct armada_ovl_plane {
@@ -252,6 +254,10 @@ armada_ovl_plane_update(struct drm_plane *plane, struct drm_crtc *crtc,
 	if (!dcrtc->plane) {
 		dcrtc->plane = plane;
 		armada_ovl_update_attr(&dplane->prop, dcrtc);
+		if (dplane->prop.csc_set) {
+			armada_drm_crtc_set_yuv_colorimetry(dcrtc, dplane->prop.csc);
+			dplane->prop.csc_set = false;
+		}
 	}
 
 	/* Queue it for update on the next interrupt if we are enabled */
@@ -332,11 +338,21 @@ static int armada_ovl_plane_set_property(struct drm_plane *plane,
 	} else if (property == priv->saturation_prop) {
 		dplane->prop.saturation = val;
 		update_attr = true;
+	} else if (property == priv->iturbt709_prop) {
+		dplane->prop.csc = val ? CFG_CSC_YUV_CCIR709 :
+					 CFG_CSC_YUV_CCIR601;
+		dplane->prop.csc_set = true;
 	}
 
-	if (update_attr && dplane->base.base.crtc)
-		armada_ovl_update_attr(&dplane->prop,
-				       drm_to_armada_crtc(dplane->base.base.crtc));
+	if (plane->crtc) {
+		struct armada_crtc *dcrtc = drm_to_armada_crtc(plane->crtc);
+		if (update_attr)
+			armada_ovl_update_attr(&dplane->prop, dcrtc);
+		if (dplane->prop.csc_set) {
+			armada_drm_crtc_set_yuv_colorimetry(dcrtc, dplane->prop.csc);
+			dplane->prop.csc_set = false;
+		}
+	}
 
 	return 0;
 }
@@ -407,6 +423,8 @@ static int armada_overlay_create_properties(struct drm_device *dev)
 				"contrast", 0, 0x7fff);
 	priv->saturation_prop = drm_property_create_range(dev, 0,
 				"saturation", 0, 0x7fff);
+	priv->iturbt709_prop = drm_property_create_bool(dev, 0,
+				"iturbt_709");
 
 	if (!priv->colorkey_prop)
 		return -ENOMEM;
@@ -475,6 +493,7 @@ int armada_overlay_plane_create(struct drm_device *dev, unsigned long crtcs)
 				   dplane->prop.contrast);
 	drm_object_attach_property(mobj, priv->saturation_prop,
 				   dplane->prop.saturation);
+	drm_object_attach_property(mobj, priv->iturbt709_prop, false);
 
 	return 0;
 }
Signature · 6 lines
2.7.4

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
#32 lore raw

Re: [PATCH 25/25] drm/armada: add iturbt_709 plane property to control YUV colorspace

Quoted 6 lines · Russell King

Add the defacto-standard "iturbt_709" property to the overlay plane to control the YUV to RGB colorspace conversion. This is mutually exclusive with the CSC_YUV CRTC property - the last property to be set determines the resulting colorspace conversion.

Can you pls provide a link to the corresponding userspace code using this?

Thanks, Daniel

Quoted 174 lines
---
 drivers/gpu/drm/armada/armada_crtc.c    | 26 +++++++++++++++++++++++---
 drivers/gpu/drm/armada/armada_crtc.h    |  2 ++
 drivers/gpu/drm/armada/armada_drm.h     |  1 +
 drivers/gpu/drm/armada/armada_overlay.c | 25 ++++++++++++++++++++++---
 4 files changed, 48 insertions(+), 6 deletions(-)
drivers/gpu/drm/armada/armada_crtc.c +23 -3
index 8b66377a4890..1bf632bb8855 100644
@@ -535,12 +535,30 @@ static irqreturn_t armada_drm_irq(int irq, void *arg)
 	return IRQ_NONE;
 }
 
+void armada_drm_crtc_set_yuv_colorimetry(struct armada_crtc *dcrtc, u32 csc)
+{
+	armada_updatel(csc, CFG_CSC_YUV_CCIR709,
+		       dcrtc->base + LCD_SPU_IOPAD_CONTROL);
+
+	dcrtc->csc_yuv_ovl_mode = csc == CFG_CSC_YUV_CCIR709 ?
+				CSC_YUV_CCIR709 : CSC_YUV_CCIR601;
+}
+
 static uint32_t armada_drm_crtc_calculate_csc(struct armada_crtc *dcrtc)
 {
 	struct drm_display_mode *adj = &dcrtc->crtc.mode;
-	uint32_t val = 0;
+	u32 val = 0;
+	u8 csc_yuv_mode;
+
+	/*
+	 * If the iturbt_709 overlay plane property is used, it overrides
+	 * the CSC_YUV crtc property until the CSC_YUV property is set.
+	 */
+	csc_yuv_mode = dcrtc->csc_yuv_ovl_mode;
+	if (csc_yuv_mode == CSC_AUTO)
+		csc_yuv_mode = dcrtc->csc_yuv_mode;
 
-	if (dcrtc->csc_yuv_mode == CSC_YUV_CCIR709)
+	if (csc_yuv_mode == CSC_YUV_CCIR709)
 		val |= CFG_CSC_YUV_CCIR709;
 	if (dcrtc->csc_rgb_mode == CSC_RGB_STUDIO)
 		val |= CFG_CSC_RGB_STUDIO;
@@ -555,7 +573,7 @@ static uint32_t armada_drm_crtc_calculate_csc(struct armada_crtc *dcrtc)
 	if ((adj->hdisplay == 1280 && adj->vdisplay == 720 &&
 	     !(adj->flags & DRM_MODE_FLAG_INTERLACE)) ||
 	    (adj->hdisplay == 1920 && adj->vdisplay == 1080)) {
-		if (dcrtc->csc_yuv_mode == CSC_AUTO)
+		if (csc_yuv_mode == CSC_AUTO)
 			val |= CFG_CSC_YUV_CCIR709;
 	}
 
@@ -1094,6 +1112,7 @@ armada_drm_crtc_set_property(struct drm_crtc *crtc,
 
 	if (property == priv->csc_yuv_prop) {
 		dcrtc->csc_yuv_mode = val;
+		dcrtc->csc_yuv_ovl_mode = CSC_AUTO;
 		update_csc = true;
 	} else if (property == priv->csc_rgb_prop) {
 		dcrtc->csc_rgb_mode = val;
@@ -1396,6 +1415,7 @@ static int armada_drm_crtc_create(struct drm_device *drm, struct device *dev,
 	dcrtc->num = drm->mode_config.num_crtc;
 	dcrtc->clk = ERR_PTR(-EINVAL);
 	dcrtc->csc_yuv_mode = CSC_AUTO;
+	dcrtc->csc_yuv_ovl_mode = CSC_AUTO;
 	dcrtc->csc_rgb_mode = CSC_AUTO;
 	dcrtc->cfg_dumb_ctrl = DUMB24_RGB888_0;
 	dcrtc->spu_iopad_ctrl = CFG_VSCALE_LN_EN | CFG_IOPAD_DUMB24;
drivers/gpu/drm/armada/armada_crtc.h +2 -0
index 445829b8877a..5d70b4af20b7 100644
@@ -90,6 +90,7 @@ struct armada_crtc {
 	bool			interlaced;
 	bool			cursor_update;
 	uint8_t			csc_yuv_mode;
+	uint8_t			csc_yuv_ovl_mode;
 	uint8_t			csc_rgb_mode;
 
 	struct drm_plane	*plane;
@@ -113,6 +114,7 @@ struct armada_crtc {
 #define drm_to_armada_crtc(c) container_of(c, struct armada_crtc, crtc)
 
 void armada_drm_crtc_update_regs(struct armada_crtc *, struct armada_regs *);
+void armada_drm_crtc_set_yuv_colorimetry(struct armada_crtc *dcrtc, u32 csc);
 
 int armada_drm_plane_disable(struct drm_plane *plane,
 			     struct drm_modeset_acquire_ctx *ctx);
drivers/gpu/drm/armada/armada_drm.h +1 -0
index b064879ecdbd..45d5168d5748 100644
@@ -60,6 +60,7 @@ struct armada_private {
 	struct armada_crtc	*dcrtc[2];
 	struct drm_mm		linear; /* protected by linear_lock */
 	struct mutex		linear_lock;
+	struct drm_property	*iturbt709_prop;
 	struct drm_property	*csc_yuv_prop;
 	struct drm_property	*csc_rgb_prop;
 	struct drm_property	*colorkey_prop;
drivers/gpu/drm/armada/armada_overlay.c +22 -3
index 853f889e84f5..d7a9c79f0ada 100644
@@ -28,6 +28,8 @@ struct armada_ovl_plane_properties {
 	uint16_t contrast;
 	uint16_t saturation;
 	uint32_t colorkey_mode;
+	uint32_t csc;
+	bool csc_set;
 };
 
 struct armada_ovl_plane {
@@ -252,6 +254,10 @@ armada_ovl_plane_update(struct drm_plane *plane, struct drm_crtc *crtc,
 	if (!dcrtc->plane) {
 		dcrtc->plane = plane;
 		armada_ovl_update_attr(&dplane->prop, dcrtc);
+		if (dplane->prop.csc_set) {
+			armada_drm_crtc_set_yuv_colorimetry(dcrtc, dplane->prop.csc);
+			dplane->prop.csc_set = false;
+		}
 	}
 
 	/* Queue it for update on the next interrupt if we are enabled */
@@ -332,11 +338,21 @@ static int armada_ovl_plane_set_property(struct drm_plane *plane,
 	} else if (property == priv->saturation_prop) {
 		dplane->prop.saturation = val;
 		update_attr = true;
+	} else if (property == priv->iturbt709_prop) {
+		dplane->prop.csc = val ? CFG_CSC_YUV_CCIR709 :
+					 CFG_CSC_YUV_CCIR601;
+		dplane->prop.csc_set = true;
 	}
 
-	if (update_attr && dplane->base.base.crtc)
-		armada_ovl_update_attr(&dplane->prop,
-				       drm_to_armada_crtc(dplane->base.base.crtc));
+	if (plane->crtc) {
+		struct armada_crtc *dcrtc = drm_to_armada_crtc(plane->crtc);
+		if (update_attr)
+			armada_ovl_update_attr(&dplane->prop, dcrtc);
+		if (dplane->prop.csc_set) {
+			armada_drm_crtc_set_yuv_colorimetry(dcrtc, dplane->prop.csc);
+			dplane->prop.csc_set = false;
+		}
+	}
 
 	return 0;
 }
@@ -407,6 +423,8 @@ static int armada_overlay_create_properties(struct drm_device *dev)
 				"contrast", 0, 0x7fff);
 	priv->saturation_prop = drm_property_create_range(dev, 0,
 				"saturation", 0, 0x7fff);
+	priv->iturbt709_prop = drm_property_create_bool(dev, 0,
+				"iturbt_709");
 
 	if (!priv->colorkey_prop)
 		return -ENOMEM;
@@ -475,6 +493,7 @@ int armada_overlay_plane_create(struct drm_device *dev, unsigned long crtcs)
 				   dplane->prop.contrast);
 	drm_object_attach_property(mobj, priv->saturation_prop,
 				   dplane->prop.saturation);
+	drm_object_attach_property(mobj, priv->iturbt709_prop, false);
 
 	return 0;
 }
Signature · 6 lines
2.7.4

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Signature · 7 lines
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
author maintainer
#33 lore raw

Re: [PATCH 25/25] drm/armada: add iturbt_709 plane property to control YUV colorspace

Quoted 9 lines · Daniel Vetter
Quoted 6 lines · Russell King

Add the defacto-standard "iturbt_709" property to the overlay plane to control the YUV to RGB colorspace conversion. This is mutually exclusive with the CSC_YUV CRTC property - the last property to be set determines the resulting colorspace conversion.

Can you pls provide a link to the corresponding userspace code using this?

http://git.armlinux.org.uk/cgit/xf86-video-armada.git/commit/?h=unstable-devel&id=7282633efd1c702a770b046505d7330fed028de0

Signature · 7 lines
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
#34 lore raw

Re: [PATCH 25/25] drm/armada: add iturbt_709 plane property to control YUV colorspace

In reply to Russell King
Quoted 4 lines · Russell King

Add the defacto-standard "iturbt_709" property to the overlay plane to control the YUV to RGB colorspace conversion. This is mutually exclusive with the CSC_YUV CRTC property - the last property to be set determines the resulting colorspace conversion.

See https://patchwork.freedesktop.org/patch/158920/ by Jyri.

For the correspoding i915 implementation I cooked up see:
https://patchwork.freedesktop.org/series/25505/

Quoted 176 lines
---
 drivers/gpu/drm/armada/armada_crtc.c    | 26 +++++++++++++++++++++++---
 drivers/gpu/drm/armada/armada_crtc.h    |  2 ++
 drivers/gpu/drm/armada/armada_drm.h     |  1 +
 drivers/gpu/drm/armada/armada_overlay.c | 25 ++++++++++++++++++++++---
 4 files changed, 48 insertions(+), 6 deletions(-)
drivers/gpu/drm/armada/armada_crtc.c +23 -3
index 8b66377a4890..1bf632bb8855 100644
@@ -535,12 +535,30 @@ static irqreturn_t armada_drm_irq(int irq, void *arg)
 	return IRQ_NONE;
 }
 
+void armada_drm_crtc_set_yuv_colorimetry(struct armada_crtc *dcrtc, u32 csc)
+{
+	armada_updatel(csc, CFG_CSC_YUV_CCIR709,
+		       dcrtc->base + LCD_SPU_IOPAD_CONTROL);
+
+	dcrtc->csc_yuv_ovl_mode = csc == CFG_CSC_YUV_CCIR709 ?
+				CSC_YUV_CCIR709 : CSC_YUV_CCIR601;
+}
+
 static uint32_t armada_drm_crtc_calculate_csc(struct armada_crtc *dcrtc)
 {
 	struct drm_display_mode *adj = &dcrtc->crtc.mode;
-	uint32_t val = 0;
+	u32 val = 0;
+	u8 csc_yuv_mode;
+
+	/*
+	 * If the iturbt_709 overlay plane property is used, it overrides
+	 * the CSC_YUV crtc property until the CSC_YUV property is set.
+	 */
+	csc_yuv_mode = dcrtc->csc_yuv_ovl_mode;
+	if (csc_yuv_mode == CSC_AUTO)
+		csc_yuv_mode = dcrtc->csc_yuv_mode;
 
-	if (dcrtc->csc_yuv_mode == CSC_YUV_CCIR709)
+	if (csc_yuv_mode == CSC_YUV_CCIR709)
 		val |= CFG_CSC_YUV_CCIR709;
 	if (dcrtc->csc_rgb_mode == CSC_RGB_STUDIO)
 		val |= CFG_CSC_RGB_STUDIO;
@@ -555,7 +573,7 @@ static uint32_t armada_drm_crtc_calculate_csc(struct armada_crtc *dcrtc)
 	if ((adj->hdisplay == 1280 && adj->vdisplay == 720 &&
 	     !(adj->flags & DRM_MODE_FLAG_INTERLACE)) ||
 	    (adj->hdisplay == 1920 && adj->vdisplay == 1080)) {
-		if (dcrtc->csc_yuv_mode == CSC_AUTO)
+		if (csc_yuv_mode == CSC_AUTO)
 			val |= CFG_CSC_YUV_CCIR709;
 	}
 
@@ -1094,6 +1112,7 @@ armada_drm_crtc_set_property(struct drm_crtc *crtc,
 
 	if (property == priv->csc_yuv_prop) {
 		dcrtc->csc_yuv_mode = val;
+		dcrtc->csc_yuv_ovl_mode = CSC_AUTO;
 		update_csc = true;
 	} else if (property == priv->csc_rgb_prop) {
 		dcrtc->csc_rgb_mode = val;
@@ -1396,6 +1415,7 @@ static int armada_drm_crtc_create(struct drm_device *drm, struct device *dev,
 	dcrtc->num = drm->mode_config.num_crtc;
 	dcrtc->clk = ERR_PTR(-EINVAL);
 	dcrtc->csc_yuv_mode = CSC_AUTO;
+	dcrtc->csc_yuv_ovl_mode = CSC_AUTO;
 	dcrtc->csc_rgb_mode = CSC_AUTO;
 	dcrtc->cfg_dumb_ctrl = DUMB24_RGB888_0;
 	dcrtc->spu_iopad_ctrl = CFG_VSCALE_LN_EN | CFG_IOPAD_DUMB24;
drivers/gpu/drm/armada/armada_crtc.h +2 -0
index 445829b8877a..5d70b4af20b7 100644
@@ -90,6 +90,7 @@ struct armada_crtc {
 	bool			interlaced;
 	bool			cursor_update;
 	uint8_t			csc_yuv_mode;
+	uint8_t			csc_yuv_ovl_mode;
 	uint8_t			csc_rgb_mode;
 
 	struct drm_plane	*plane;
@@ -113,6 +114,7 @@ struct armada_crtc {
 #define drm_to_armada_crtc(c) container_of(c, struct armada_crtc, crtc)
 
 void armada_drm_crtc_update_regs(struct armada_crtc *, struct armada_regs *);
+void armada_drm_crtc_set_yuv_colorimetry(struct armada_crtc *dcrtc, u32 csc);
 
 int armada_drm_plane_disable(struct drm_plane *plane,
 			     struct drm_modeset_acquire_ctx *ctx);
drivers/gpu/drm/armada/armada_drm.h +1 -0
index b064879ecdbd..45d5168d5748 100644
@@ -60,6 +60,7 @@ struct armada_private {
 	struct armada_crtc	*dcrtc[2];
 	struct drm_mm		linear; /* protected by linear_lock */
 	struct mutex		linear_lock;
+	struct drm_property	*iturbt709_prop;
 	struct drm_property	*csc_yuv_prop;
 	struct drm_property	*csc_rgb_prop;
 	struct drm_property	*colorkey_prop;
drivers/gpu/drm/armada/armada_overlay.c +22 -3
index 853f889e84f5..d7a9c79f0ada 100644
@@ -28,6 +28,8 @@ struct armada_ovl_plane_properties {
 	uint16_t contrast;
 	uint16_t saturation;
 	uint32_t colorkey_mode;
+	uint32_t csc;
+	bool csc_set;
 };
 
 struct armada_ovl_plane {
@@ -252,6 +254,10 @@ armada_ovl_plane_update(struct drm_plane *plane, struct drm_crtc *crtc,
 	if (!dcrtc->plane) {
 		dcrtc->plane = plane;
 		armada_ovl_update_attr(&dplane->prop, dcrtc);
+		if (dplane->prop.csc_set) {
+			armada_drm_crtc_set_yuv_colorimetry(dcrtc, dplane->prop.csc);
+			dplane->prop.csc_set = false;
+		}
 	}
 
 	/* Queue it for update on the next interrupt if we are enabled */
@@ -332,11 +338,21 @@ static int armada_ovl_plane_set_property(struct drm_plane *plane,
 	} else if (property == priv->saturation_prop) {
 		dplane->prop.saturation = val;
 		update_attr = true;
+	} else if (property == priv->iturbt709_prop) {
+		dplane->prop.csc = val ? CFG_CSC_YUV_CCIR709 :
+					 CFG_CSC_YUV_CCIR601;
+		dplane->prop.csc_set = true;
 	}
 
-	if (update_attr && dplane->base.base.crtc)
-		armada_ovl_update_attr(&dplane->prop,
-				       drm_to_armada_crtc(dplane->base.base.crtc));
+	if (plane->crtc) {
+		struct armada_crtc *dcrtc = drm_to_armada_crtc(plane->crtc);
+		if (update_attr)
+			armada_ovl_update_attr(&dplane->prop, dcrtc);
+		if (dplane->prop.csc_set) {
+			armada_drm_crtc_set_yuv_colorimetry(dcrtc, dplane->prop.csc);
+			dplane->prop.csc_set = false;
+		}
+	}
 
 	return 0;
 }
@@ -407,6 +423,8 @@ static int armada_overlay_create_properties(struct drm_device *dev)
 				"contrast", 0, 0x7fff);
 	priv->saturation_prop = drm_property_create_range(dev, 0,
 				"saturation", 0, 0x7fff);
+	priv->iturbt709_prop = drm_property_create_bool(dev, 0,
+				"iturbt_709");
 
 	if (!priv->colorkey_prop)
 		return -ENOMEM;
@@ -475,6 +493,7 @@ int armada_overlay_plane_create(struct drm_device *dev, unsigned long crtcs)
 				   dplane->prop.contrast);
 	drm_object_attach_property(mobj, priv->saturation_prop,
 				   dplane->prop.saturation);
+	drm_object_attach_property(mobj, priv->iturbt709_prop, false);
 
 	return 0;
 }
Signature · 6 lines
2.7.4

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Signature · 6 lines
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
#35 lore raw

Re: [PATCH 25/25] drm/armada: add iturbt_709 plane property to control YUV colorspace

In reply to Russell King

Hi Russell,

Quoted 4 lines · Russell King

Add the defacto-standard "iturbt_709" property to the overlay plane to control the YUV to RGB colorspace conversion. This is mutually exclusive with the CSC_YUV CRTC property - the last property to be set determines the resulting colorspace conversion.

I haven't seen this in other drivers - is it a 'defacto standard'? I prefer VIlle's choice of an explicit 601/709 enum, since that's more easily expandable. I do worry about the interaction with the CTM properties, but I also don't have a good answer as to what that would look like. I also worry that implementing the 'last-property-set' semantics might be difficult to implement in an atomic driver.

drivers/gpu/drm/armada/armada_overlay.c armada_ovl_plane_update(struct drm_plane *plane, struct drm_crtc *crtc, · line 261

Quoted 10 lines

struct armada_ovl_plane {

drivers/gpu/drm/armada/armada_overlay.c +4 -0
@@ -252,6 +254,10 @@ armada_ovl_plane_update(struct drm_plane *plane, struct drm_crtc *crtc,
        if (!dcrtc->plane) {
                dcrtc->plane = plane;
                armada_ovl_update_attr(&dplane->prop, dcrtc);
+               if (dplane->prop.csc_set) {
+                       armada_drm_crtc_set_yuv_colorimetry(dcrtc, dplane->prop.csc);
+                       dplane->prop.csc_set = false;
+               }
        }

Just trying to understand this a little better: setting this property on a plane results in a CRTC change. This is only implemented for the overlay ('video') plane, but the primary ('graphics') plane also supports YUV formats. Does this mean that the two planes have to have the same configuration wrt 601/709 if both are YUV? That could again get painful to express.

Cheers,
Daniel
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

author maintainer
#36 lore raw

Re: [PATCH 25/25] drm/armada: add iturbt_709 plane property to control YUV colorspace

Quoted 9 lines · Daniel Stone

Hi Russell,

Quoted 4 lines · Russell King

Add the defacto-standard "iturbt_709" property to the overlay plane to control the YUV to RGB colorspace conversion. This is mutually exclusive with the CSC_YUV CRTC property - the last property to be set determines the resulting colorspace conversion.

I haven't seen this in other drivers - is it a 'defacto standard'?

As far as I know, nothing for this is standardised - when I implemented this method, I did survey the drivers, and decided it was best to use what existing implementations were already using. I wrote this patch back in February, so memory is now hazy, but iirc, it came from:

drivers/gpu/drm/nouveau/dispnv04/overlay.c

Quoted 5 lines

I prefer VIlle's choice of an explicit 601/709 enum, since that's more easily expandable. I do worry about the interaction with the CTM properties, but I also don't have a good answer as to what that would look like. I also worry that implementing the 'last-property-set' semantics might be difficult to implement in an atomic driver.

Nothing really made use of the CRTC property, so its unlikely that there'll be a conflict. I suspect I could delete the CRTC property when eventually switching to atomic modeset.

Quoted 10 lines
Quoted 10 lines

struct armada_ovl_plane {

drivers/gpu/drm/armada/armada_overlay.c +4 -0
@@ -252,6 +254,10 @@ armada_ovl_plane_update(struct drm_plane *plane, struct drm_crtc *crtc,
        if (!dcrtc->plane) {
                dcrtc->plane = plane;
                armada_ovl_update_attr(&dplane->prop, dcrtc);
+               if (dplane->prop.csc_set) {
+                       armada_drm_crtc_set_yuv_colorimetry(dcrtc, dplane->prop.csc);
+                       dplane->prop.csc_set = false;
+               }
Quoted 8 lines

Review armada_ovl_plane_update(struct drm_plane *plane, struct drm_crtc *crtc, · line 261

Quoted 1 line
(quoted diff) +0 -0
        }

Just trying to understand this a little better: setting this property on a plane results in a CRTC change. This is only implemented for the overlay ('video') plane, but the primary ('graphics') plane also supports YUV formats. Does this mean that the two planes have to have the same configuration wrt 601/709 if both are YUV? That could again get painful to express.

The same is true of the saturation, brightness, contrast etc values. There is only one set of YUV to RGB conversion in the hardware block and its settings will be used for both.

However, practically I don't think YUV is ever used for the primary plane, so I could just drop those from the list of primary plane formats.

Signature · 7 lines
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
#37 lore raw

Re: [PATCH 25/25] drm/armada: add iturbt_709 plane property to control YUV colorspace

In reply to Daniel Stone
Quoted 9 lines · Daniel Stone

Hi Russell,

Quoted 4 lines · Russell King

Add the defacto-standard "iturbt_709" property to the overlay plane to control the YUV to RGB colorspace conversion. This is mutually exclusive with the CSC_YUV CRTC property - the last property to be set determines the resulting colorspace conversion.

I haven't seen this in other drivers - is it a 'defacto standard'? I

xf86-video-nv supported it, and I added it to nouveau as well when I ported YUV plane support. Some video players use the Xv property when available.

https://cgit.freedesktop.org/xorg/driver/xf86-video-nv/tree/src/nv_video.c#n128
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/nouveau/dispnv04/overlay.c?h=v4.15-rc3#n316

Cheers,

-ilia
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

#38 lore raw

Re: [PATCH 25/25] drm/armada: add iturbt_709 plane property to control YUV colorspace

Quoted 9 lines · Ilia Mirkin
Quoted 8 lines · Daniel Stone

Hi Russell,

Quoted 4 lines · Russell King

Add the defacto-standard "iturbt_709" property to the overlay plane to control the YUV to RGB colorspace conversion. This is mutually exclusive with the CSC_YUV CRTC property - the last property to be set determines the resulting colorspace conversion.

Quoted 8 lines
Quoted 1 line

I haven't seen this in other drivers - is it a 'defacto standard'? I

xf86-video-nv supported it, and I added it to nouveau as well when I ported YUV plane support. Some video players use the Xv property when available.

https://cgit.freedesktop.org/xorg/driver/xf86-video-nv/tree/src/nv_video.c#n128
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/nouveau/dispnv04/overlay.c?h=v4.15-rc3#n316

{XvSettable | XvGettable, 0, 1, "XV_ITURBT_709"}

Who came up with that and when? XV_COLORSPACE was the one semi-standard I know of.

Quoted 8 lines

Cheers,

-ilia
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Signature · 6 lines
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
author maintainer
#39 lore raw

Re: [PATCH 25/25] drm/armada: add iturbt_709 plane property to control YUV colorspace

Quoted 16 lines · Ville Syrjälä
Quoted 15 lines · Ilia Mirkin
Quoted 9 lines · Daniel Stone

Hi Russell,

Quoted 4 lines · Russell King

Add the defacto-standard "iturbt_709" property to the overlay plane to control the YUV to RGB colorspace conversion. This is mutually exclusive with the CSC_YUV CRTC property - the last property to be set determines the resulting colorspace conversion.

I haven't seen this in other drivers - is it a 'defacto standard'? I

xf86-video-nv supported it, and I added it to nouveau as well when I ported YUV plane support. Some video players use the Xv property when available.

Quoted 7 lines
Quoted 2 lines

https://cgit.freedesktop.org/xorg/driver/xf86-video-nv/tree/src/nv_video.c#n128
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/nouveau/dispnv04/overlay.c?h=v4.15-rc3#n316

   {XvSettable | XvGettable, 0, 1, "XV_ITURBT_709"}

Who came up with that and when? XV_COLORSPACE was the one semi-standard I know of.

I've no idea, and I was hoping that someone else would know - my use of it comes from research into what will make userspace work, not what standards may say.

XV_ITURBT_709 is already in-use in distro standard userspace programs:

# grep XV_ITURBT_709 /usr/lib/vlc/plugins/ -r
Binary file /usr/lib/vlc/plugins/video_output/libxcb_xv_plugin.so matches
# grep XV_ITURBT_709 /usr/lib/gstreamer-1.0/ -r
Binary file /usr/lib/gstreamer-1.0/libgstxvimagesink.so matches

but not XV_COLORSPACE:

# grep XV_COLORSPACE /usr/lib/vlc/plugins/ -r
# grep XV_COLORSPACE /usr/lib/gstreamer-1.0/ -r

So while XV_COLORSPACE may be some kind of standard, it seems that userspace has decided otherwise to go with a different name for this control.

Anyway, I'll drop this patch and send a pull request for the remainder to David.

Signature · 7 lines
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
author maintainer
#40 lore raw

Re: [PATCH 25/25] drm/armada: add iturbt_709 plane property to control YUV colorspace

Quoted 36 lines · Russell King - ARM Linux
Quoted 16 lines · Ville Syrjälä
Quoted 15 lines · Ilia Mirkin
Quoted 9 lines · Daniel Stone

Hi Russell,

Quoted 4 lines · Russell King

Add the defacto-standard "iturbt_709" property to the overlay plane to control the YUV to RGB colorspace conversion. This is mutually exclusive with the CSC_YUV CRTC property - the last property to be set determines the resulting colorspace conversion.

I haven't seen this in other drivers - is it a 'defacto standard'? I

xf86-video-nv supported it, and I added it to nouveau as well when I ported YUV plane support. Some video players use the Xv property when available.

Quoted 7 lines
Quoted 2 lines

https://cgit.freedesktop.org/xorg/driver/xf86-video-nv/tree/src/nv_video.c#n128
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/nouveau/dispnv04/overlay.c?h=v4.15-rc3#n316

   {XvSettable | XvGettable, 0, 1, "XV_ITURBT_709"}

Who came up with that and when? XV_COLORSPACE was the one semi-standard I know of.

I've no idea, and I was hoping that someone else would know - my use of it comes from research into what will make userspace work, not what standards may say.

XV_ITURBT_709 is already in-use in distro standard userspace programs:

# grep XV_ITURBT_709 /usr/lib/vlc/plugins/ -r
Binary file /usr/lib/vlc/plugins/video_output/libxcb_xv_plugin.so matches
# grep XV_ITURBT_709 /usr/lib/gstreamer-1.0/ -r
Binary file /usr/lib/gstreamer-1.0/libgstxvimagesink.so matches
Quoted 8 lines

but not XV_COLORSPACE:

# grep XV_COLORSPACE /usr/lib/vlc/plugins/ -r
# grep XV_COLORSPACE /usr/lib/gstreamer-1.0/ -r

So while XV_COLORSPACE may be some kind of standard, it seems that userspace has decided otherwise to go with a different name for this control.

Re-opening this discussion, since the above point was never replied to.

I can find no video players that make use of the "XV_COLORSPACE" property, but two that make use of the "XV_ITURBT_709" property.

It was added to gstreamer in 2014:
https://github.com/GStreamer/gst-plugins-base/commit/d99e270fc83278c309ec7cad20d75181d90b8722

and is present in VLC since 2016:
https://github.com/videolan/vlc/commit/8172a5470964550a1e5d6e2b7082650f932e6ce6

We seem to have the situation where some Xv backends implement "XV_COLORSPACE", others "XV_ITURBT_709" but players implement only "XV_ITURBT_709".

A DDX /could/ consider implementing both, but there is no way to notify Xv event listeners from a DDX's Xv backend that "the other" property has been changed - XvdiSendPortNotify() needs an XvPortPtr but the DDX has no access to that due to the xf86 layer on top hiding that, and there's nothing at xf86 level to allow that.

This doesn't seem to be a very productive situation, and certainly not useful for the user.

I think that the conclusion I'd come to given this is that 99.9% of people don't care about correct Xv colorimetry.

Signature · 7 lines
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 13.8Mbps down 630kbps up
According to speedtest.net: 13Mbps down 490kbps up
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
#41 lore raw

Re: [PATCH 25/25] drm/armada: add iturbt_709 plane property to control YUV colorspace

Quoted 42 lines · Russell King - ARM Linux
Quoted 41 lines · Russell King - ARM Linux
Quoted 16 lines · Ville Syrjälä
Quoted 15 lines · Ilia Mirkin
Quoted 9 lines · Daniel Stone

Hi Russell,

Quoted 4 lines · Russell King

Add the defacto-standard "iturbt_709" property to the overlay plane to control the YUV to RGB colorspace conversion. This is mutually exclusive with the CSC_YUV CRTC property - the last property to be set determines the resulting colorspace conversion.

I haven't seen this in other drivers - is it a 'defacto standard'? I

xf86-video-nv supported it, and I added it to nouveau as well when I ported YUV plane support. Some video players use the Xv property when available.

Quoted 7 lines
Quoted 2 lines

https://cgit.freedesktop.org/xorg/driver/xf86-video-nv/tree/src/nv_video.c#n128
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/nouveau/dispnv04/overlay.c?h=v4.15-rc3#n316

   {XvSettable | XvGettable, 0, 1, "XV_ITURBT_709"}

Who came up with that and when? XV_COLORSPACE was the one semi-standard I know of.

I've no idea, and I was hoping that someone else would know - my use of it comes from research into what will make userspace work, not what standards may say.

XV_ITURBT_709 is already in-use in distro standard userspace programs:

# grep XV_ITURBT_709 /usr/lib/vlc/plugins/ -r
Binary file /usr/lib/vlc/plugins/video_output/libxcb_xv_plugin.so matches
# grep XV_ITURBT_709 /usr/lib/gstreamer-1.0/ -r
Binary file /usr/lib/gstreamer-1.0/libgstxvimagesink.so matches

but not XV_COLORSPACE:

# grep XV_COLORSPACE /usr/lib/vlc/plugins/ -r
# grep XV_COLORSPACE /usr/lib/gstreamer-1.0/ -r
Quoted 8 lines
Quoted 3 lines

So while XV_COLORSPACE may be some kind of standard, it seems that userspace has decided otherwise to go with a different name for this control.

Re-opening this discussion, since the above point was never replied to.

I can find no video players that make use of the "XV_COLORSPACE" property, but two that make use of the "XV_ITURBT_709" property.

mpv supports both, and xvattr is of course one way to change it but requires manual user intervention.

I have a patch locally for gst xvimagesink to use XV_COLORSPACE as well. I wrote it when I posted the proposed XV_COLOR_RANGE attribute (and I wrote patches for that one too, for gst and mpv). Didn't bother posting any of these until I got some feedback on XV_COLOR_RANGE. No feedback was received though so I suppose everyone is OK with it.

Quoted 17 lines

It was added to gstreamer in 2014:
https://github.com/GStreamer/gst-plugins-base/commit/d99e270fc83278c309ec7cad20d75181d90b8722

and is present in VLC since 2016:
https://github.com/videolan/vlc/commit/8172a5470964550a1e5d6e2b7082650f932e6ce6

We seem to have the situation where some Xv backends implement "XV_COLORSPACE", others "XV_ITURBT_709" but players implement only "XV_ITURBT_709".

A DDX /could/ consider implementing both, but there is no way to notify Xv event listeners from a DDX's Xv backend that "the other" property has been changed - XvdiSendPortNotify() needs an XvPortPtr but the DDX has no access to that due to the xf86 layer on top hiding that, and there's nothing at xf86 level to allow that.

Quoted 5 lines

This doesn't seem to be a very productive situation, and certainly not useful for the user.

I think that the conclusion I'd come to given this is that 99.9% of people don't care about correct Xv colorimetry.

Also Xv usage is probably on the decline seeing as it's not used by browsers which is probably how many people watch videos these days.

Signature · 6 lines
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel