[v2] drm: Add DSI/DP support for Renesas r8a779h0 V4M and grey-hawk board

[PATCH v2 02/10] drm/rcar-du: Write DPTSR only if there are more than one crtc

Posted by Tomi Valkeinen 1 year ago

From: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>

Currently the driver always writes DPTSR when setting up the hardware.
However, the register is only meaningful when there are more than one
crtc, and the only SoC with one crtc, V3M, does not have the register
mentioned in its documentation.

So move the write behind a condition.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
---
 drivers/gpu/drm/renesas/rcar-du/rcar_du_group.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

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..0fbf6abbde6e 100644
--- a/drivers/gpu/drm/renesas/rcar-du/rcar_du_group.c
+++ b/drivers/gpu/drm/renesas/rcar-du/rcar_du_group.c
@@ -185,11 +185,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 (rgrp->num_crtcs > 1) {
+		/* 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);
+	}
 }
 
 /*

-- 
2.43.0

Re: [PATCH v2 02/10] drm/rcar-du: Write DPTSR only if there are more than one crtc

Posted by Geert Uytterhoeven 1 year ago

Hi Tomi,

CC Jacopo

On Thu, Dec 5, 2024 at 2:45 PM Tomi Valkeinen
<tomi.valkeinen@ideasonboard.com> wrote:
> From: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
>
> Currently the driver always writes DPTSR when setting up the hardware.
> However, the register is only meaningful when there are more than one
> crtc, and the only SoC with one crtc, V3M, does not have the register
> mentioned in its documentation.

R-Car V3H/V3H_2, too.

>
> So move the write behind a condition.
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
> ---
>  drivers/gpu/drm/renesas/rcar-du/rcar_du_group.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
>
> 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..0fbf6abbde6e 100644
> --- a/drivers/gpu/drm/renesas/rcar-du/rcar_du_group.c
> +++ b/drivers/gpu/drm/renesas/rcar-du/rcar_du_group.c
> @@ -185,11 +185,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 (rgrp->num_crtcs > 1) {
> +               /* 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);
> +       }

This is per group, not per DU, right?
The second group on R-Car M3-W/M3-W+ has a single channel, hence no
DPTSR2 register.
The second group on R-Car M3-N has a single channel, but it's actually
the second physical channel in the group, and thus does have DPTSR2.

And apparently we had this discussion before...
https://lore.kernel.org/all/CAMuHMdXxf4oePnyLvp84OhSa+wdehCNJBXnhjYO7-1VxpBJ7eQ@mail.gmail.com

Gr{oetje,eeting}s,

                        Geert


--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 02/10] drm/rcar-du: Write DPTSR only if there are more than one crtc

Posted by Tomi Valkeinen 1 year ago

Hi,

On 05/12/2024 16:16, Geert Uytterhoeven wrote:
> Hi Tomi,
> 
> CC Jacopo
> 
> On Thu, Dec 5, 2024 at 2:45 PM Tomi Valkeinen
> <tomi.valkeinen@ideasonboard.com> wrote:
>> From: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
>>
>> Currently the driver always writes DPTSR when setting up the hardware.
>> However, the register is only meaningful when there are more than one
>> crtc, and the only SoC with one crtc, V3M, does not have the register
>> mentioned in its documentation.
> 
> R-Car V3H/V3H_2, too.

Right... I was looking at the number of outputs, not the number of crtcs 
when going through the SoCs.

> 
>>
>> So move the write behind a condition.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
>> ---
>>   drivers/gpu/drm/renesas/rcar-du/rcar_du_group.c | 12 +++++++-----
>>   1 file changed, 7 insertions(+), 5 deletions(-)
>>
>> 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..0fbf6abbde6e 100644
>> --- a/drivers/gpu/drm/renesas/rcar-du/rcar_du_group.c
>> +++ b/drivers/gpu/drm/renesas/rcar-du/rcar_du_group.c
>> @@ -185,11 +185,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 (rgrp->num_crtcs > 1) {
>> +               /* 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);
>> +       }
> 
> This is per group, not per DU, right?
> The second group on R-Car M3-W/M3-W+ has a single channel, hence no
> DPTSR2 register.
> The second group on R-Car M3-N has a single channel, but it's actually
> the second physical channel in the group, and thus does have DPTSR2.

That logic does make sense. So that would be if (rgrp->channels_mask & 
BIT(1)) then write DPTSR? And probably add a comment in the code about this.

> And apparently we had this discussion before...
> https://lore.kernel.org/all/CAMuHMdXxf4oePnyLvp84OhSa+wdehCNJBXnhjYO7-1VxpBJ7eQ@mail.gmail.com

Somehow I hadn't even realized Jacopo had sent these before...

  Tomi

Re: [PATCH v2 02/10] drm/rcar-du: Write DPTSR only if there are more than one crtc

Posted by Laurent Pinchart 1 year ago

On Thu, Dec 05, 2024 at 06:08:24PM +0200, Tomi Valkeinen wrote:
> On 05/12/2024 16:16, Geert Uytterhoeven wrote:
> > Hi Tomi,
> > 
> > CC Jacopo
> > 
> > On Thu, Dec 5, 2024 at 2:45 PM Tomi Valkeinen wrote:
> >> From: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
> >>
> >> Currently the driver always writes DPTSR when setting up the hardware.
> >> However, the register is only meaningful when there are more than one
> >> crtc, and the only SoC with one crtc, V3M, does not have the register
> >> mentioned in its documentation.
> > 
> > R-Car V3H/V3H_2, too.
> 
> Right... I was looking at the number of outputs, not the number of crtcs 
> when going through the SoCs.
> 
> >> So move the write behind a condition.
> >>
> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
> >> ---
> >>   drivers/gpu/drm/renesas/rcar-du/rcar_du_group.c | 12 +++++++-----
> >>   1 file changed, 7 insertions(+), 5 deletions(-)
> >>
> >> 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..0fbf6abbde6e 100644
> >> --- a/drivers/gpu/drm/renesas/rcar-du/rcar_du_group.c
> >> +++ b/drivers/gpu/drm/renesas/rcar-du/rcar_du_group.c
> >> @@ -185,11 +185,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 (rgrp->num_crtcs > 1) {
> >> +               /* 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);
> >> +       }
> > 
> > This is per group, not per DU, right?
> > The second group on R-Car M3-W/M3-W+ has a single channel, hence no
> > DPTSR2 register.
> > The second group on R-Car M3-N has a single channel, but it's actually
> > the second physical channel in the group, and thus does have DPTSR2.
> 
> That logic does make sense. So that would be if (rgrp->channels_mask & 
> BIT(1)) then write DPTSR? And probably add a comment in the code about this.
> 
> > And apparently we had this discussion before...
> > https://lore.kernel.org/all/CAMuHMdXxf4oePnyLvp84OhSa+wdehCNJBXnhjYO7-1VxpBJ7eQ@mail.gmail.com
> 
> Somehow I hadn't even realized Jacopo had sent these before...

Oops...

I'll let Jacopo and you decide who will send an updated patch.

-- 
Regards,

Laurent Pinchart