| Message ID | 20241203-rcar-gh-dsi-v1-6-738ae1a95d2a@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 |
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@ideasonboard.com> > > Add support for r8a779h0. It is very similar to r8a779g0, but has only > one output. > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com> > --- > 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 ? > > #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); > + } > } > > /*
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@ideasonboard.com> >> >> Add support for r8a779h0. It is very similar to r8a779g0, but has only >> one output. >> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com> >> --- >> 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); >> + } >> } >> >> /* >
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@ideasonboard.com> > >> > >> Add support for r8a779h0. It is very similar to r8a779g0, but has only > >> one output. > >> > >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com> > >> --- > >> 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. > 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); > >> + } > >> } > >> > >> /*
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@ideasonboard.com> >>>> >>>> Add support for r8a779h0. It is very similar to r8a779g0, but has only >>>> one output. >>>> >>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com> >>>> --- >>>> 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); >>>> + } >>>> } >>>> >>>> /* >
On Thu, Dec 05, 2024 at 07:41:09AM +0200, Tomi Valkeinen wrote: > 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@ideasonboard.com> > >>>> > >>>> Add support for r8a779h0. It is very similar to r8a779g0, but has only > >>>> one output. > >>>> > >>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com> > >>>> --- > >>>> 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? According to the Gen3 documentation the register isn't preent in V3M. > 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? The change is easy so I'd like to do it now. It should be split to a separate patch. I'll test it on Gen3 hardware right away. > >> 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); > >>>> + } > >>>> } > >>>> > >>>> /*
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 */ #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); + } } /*