Re: [PATCH 6/9] drm/rcar-du: Add support for r8a779h0

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Laurent,

On 03/12/2024 12:48, Laurent Pinchart wrote:
On Tue, Dec 03, 2024 at 11:22:15AM +0200, Tomi Valkeinen wrote:
On 03/12/2024 10:56, Laurent Pinchart wrote:
On Tue, Dec 03, 2024 at 10:01:40AM +0200, Tomi Valkeinen wrote:
From: Tomi Valkeinen <tomi.valkeinen+renesas@xxxxxxxxxxxxxxxx>

Add support for r8a779h0. It is very similar to r8a779g0, but has only
one output.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@xxxxxxxxxxxxxxxx>
---
   drivers/gpu/drm/renesas/rcar-du/rcar_du_drv.c   | 19 +++++++++++++++++++
   drivers/gpu/drm/renesas/rcar-du/rcar_du_drv.h   |  1 +
   drivers/gpu/drm/renesas/rcar-du/rcar_du_group.c | 16 ++++++++++------
   3 files changed, 30 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/renesas/rcar-du/rcar_du_drv.c b/drivers/gpu/drm/renesas/rcar-du/rcar_du_drv.c
index fb719d9aff10..afbc74e18cce 100644
--- a/drivers/gpu/drm/renesas/rcar-du/rcar_du_drv.c
+++ b/drivers/gpu/drm/renesas/rcar-du/rcar_du_drv.c
@@ -545,6 +545,24 @@ static const struct rcar_du_device_info rcar_du_r8a779g0_info = {
   	.dsi_clk_mask =  BIT(1) | BIT(0),
   };
+static const struct rcar_du_device_info rcar_du_r8a779h0_info = {
+	.gen = 4,
+	.features = RCAR_DU_FEATURE_CRTC_IRQ
+		  | RCAR_DU_FEATURE_VSP1_SOURCE
+		  | RCAR_DU_FEATURE_NO_BLENDING
+		  | RCAR_DU_FEATURE_NO_DPTSR,
+	.channels_mask = BIT(0),
+	.routes = {
+		/* R8A779H0 has one MIPI DSI output. */
+		[RCAR_DU_OUTPUT_DSI0] = {
+			.possible_crtcs = BIT(0),
+			.port = 0,
+		},
+	},
+	.num_rpf = 5,
+	.dsi_clk_mask = BIT(0),
+};
+
   static const struct of_device_id rcar_du_of_table[] = {
   	{ .compatible = "renesas,du-r8a7742", .data = &rcar_du_r8a7790_info },
   	{ .compatible = "renesas,du-r8a7743", .data = &rzg1_du_r8a7743_info },
@@ -571,6 +589,7 @@ static const struct of_device_id rcar_du_of_table[] = {
   	{ .compatible = "renesas,du-r8a77995", .data = &rcar_du_r8a7799x_info },
   	{ .compatible = "renesas,du-r8a779a0", .data = &rcar_du_r8a779a0_info },
   	{ .compatible = "renesas,du-r8a779g0", .data = &rcar_du_r8a779g0_info },
+	{ .compatible = "renesas,du-r8a779h0", .data = &rcar_du_r8a779h0_info },
   	{ }
   };
diff --git a/drivers/gpu/drm/renesas/rcar-du/rcar_du_drv.h b/drivers/gpu/drm/renesas/rcar-du/rcar_du_drv.h
index 5cfa2bb7ad93..d7004f76f735 100644
--- a/drivers/gpu/drm/renesas/rcar-du/rcar_du_drv.h
+++ b/drivers/gpu/drm/renesas/rcar-du/rcar_du_drv.h
@@ -32,6 +32,7 @@ struct rcar_du_device;
   #define RCAR_DU_FEATURE_INTERLACED	BIT(3)	/* HW supports interlaced */
   #define RCAR_DU_FEATURE_TVM_SYNC	BIT(4)	/* Has TV switch/sync modes */
   #define RCAR_DU_FEATURE_NO_BLENDING	BIT(5)	/* PnMR.SPIM does not have ALP nor EOR bits */
+#define RCAR_DU_FEATURE_NO_DPTSR	BIT(6)  /* V4M does not have DPTSR */

Do we need a quirk ? At first glance it seems the DPTSR register is only
used for DU instances that have two channels, so a check on the number
of channels should be enough ?

What do you mean with "DPTSR register is only used for DU instances that
have two channels"? The upstream code sets it for all SoCs, doesn't it,
without any checks?

DPTSR is one of those registers that controls features shared between
channels, in this specific case plane assignment to DU channels. The
default register value (i.e. all 0's) splits resources between the
channels. For DU groups with a single channel, there's no need for
resource assignment. Logically speaking, the all 0's register value as
documented in instances that have two channels would assign all the
resources that exist in the single-channel group to the single channel.
When computing the DPTSR value, the driver will (or at least should)
therefore always come up with 0x00000000. Writing that to the register
should be a no-op.

It's not clear if the register is present or not when the group has a
single channel. Some datasheets document the register is not being
applicable. Writing to it has never caused issues, so we may be dealing
with the hardware just ignoring writes to a non-implemented register, or
the register may be there, with only 0x00000000 being a meaningful
value. This being said, some people are concerned about writes to
registers that are not documented as present, as they could possibly
cause issues. Safety certification of the driver could be impacted.
We've updated the DU driver over the past few years to avoid those
writes for this reason.

TL;DR: yes, the DU driver writes to DPTSR for DU groups with a single
channel, but that seem it could be wrong, and we could fix it for all
single-channel groups in one go without introducing this feature bit. I
can test a patch on a M3 board that has a single channel in the second
group.

Do you have docs for r8a77970? Is the register there?

Do you want me to change the series to use the number of channels here, or shall we go with the current version and change it later if we're confident that the change works?

 Tomi

Most of the SoCs seem to have two channels, but r8a77970 has one.
However, I don't have docs for that one. It could be that it does not
have DPTSR register, and indeed we could use the num_crtcs > 1 check there.

   #define RCAR_DU_QUIRK_ALIGN_128B	BIT(0)	/* Align pitches to 128 bytes */
diff --git a/drivers/gpu/drm/renesas/rcar-du/rcar_du_group.c b/drivers/gpu/drm/renesas/rcar-du/rcar_du_group.c
index 2ccd2581f544..132d930670eb 100644
--- a/drivers/gpu/drm/renesas/rcar-du/rcar_du_group.c
+++ b/drivers/gpu/drm/renesas/rcar-du/rcar_du_group.c
@@ -107,10 +107,12 @@ static void rcar_du_group_setup_didsr(struct rcar_du_group *rgrp)
   		 */
   		rcrtc = rcdu->crtcs;
   		num_crtcs = rcdu->num_crtcs;
-	} else if (rcdu->info->gen >= 3 && rgrp->num_crtcs > 1) {
+	} else if ((rcdu->info->gen == 3 && rgrp->num_crtcs > 1) ||
+		   rcdu->info->gen == 4) {
   		/*
   		 * On Gen3 dot clocks are setup through per-group registers,
   		 * only available when the group has two channels.
+		 * On Gen4 the registers are there for single channel too.
   		 */
   		rcrtc = &rcdu->crtcs[rgrp->index * 2];
   		num_crtcs = rgrp->num_crtcs;
@@ -185,11 +187,13 @@ static void rcar_du_group_setup(struct rcar_du_group *rgrp)
   		dorcr |= DORCR_PG1T | DORCR_DK1S | DORCR_PG1D_DS1;
   	rcar_du_group_write(rgrp, DORCR, dorcr);
- /* Apply planes to CRTCs association. */
-	mutex_lock(&rgrp->lock);
-	rcar_du_group_write(rgrp, DPTSR, (rgrp->dptsr_planes << 16) |
-			    rgrp->dptsr_planes);
-	mutex_unlock(&rgrp->lock);
+	if (!rcar_du_has(rcdu, RCAR_DU_FEATURE_NO_DPTSR)) {
+		/* Apply planes to CRTCs association. */
+		mutex_lock(&rgrp->lock);
+		rcar_du_group_write(rgrp, DPTSR, (rgrp->dptsr_planes << 16) |
+				    rgrp->dptsr_planes);
+		mutex_unlock(&rgrp->lock);
+	}
   }
/*





[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux