| Message ID | 20241205-rcar-gh-dsi-v2-2-42471851df86@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: Add DSI/DP support for Renesas r8a779h0 V4M and grey-hawk board | expand |
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
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
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 --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); + } } /*