diff mbox series

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

Message ID 20241205-rcar-gh-dsi-v2-2-42471851df86@ideasonboard.com (mailing list archive)
State Superseded
Delegated to: Kieran Bingham
Headers show
Series drm: Add DSI/DP support for Renesas r8a779h0 V4M and grey-hawk board | expand

Commit Message

Tomi Valkeinen Dec. 5, 2024, 1:44 p.m. UTC 
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(-)

Comments

Geert Uytterhoeven Dec. 5, 2024, 2:16 p.m. UTC | #1 
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
Tomi Valkeinen Dec. 5, 2024, 4:08 p.m. UTC | #2 
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
Laurent Pinchart Dec. 5, 2024, 5:34 p.m. UTC | #3 
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.
diff mbox series

Patch

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