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

[Tomi Valkeinen <tomi.valkeinen@xxxxxxxxxxxxxxxx>]

On 03/12/2024 10:56, Laurent Pinchart wrote:
> Hi Tomi,
>
> Thank you for the patch.
>
> 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?

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.

 Tomi

> >   
> >   #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);
> > +	}
> >   }
> >   
> >   /*