Hi Ulrich,
On Tue, Jun 18, 2019 at 04:12:01PM +0200, Ulrich Hecht wrote:
> > On June 17, 2019 at 11:09 PM Laurent Pinchart wrote:
> >
> > Routing configuration for the DU is complex. Depending on the SoC
> > generation various routing options are available:
> >
> > - The VSP to DU routing is not available on Gen1, is configurable on
> > Gen2 and is fixed on Gen3. When configurable, the routing affects both
> > CRTC groups but is set in a register of the first CRTC group.
> > - The DU channel to DPAD output routing is explicitly configurable on
> > some SoCs of the Gen2 and Gen3 family. When configurable, the DPAD
> > outputs never offer routing options to CRTCs belonging to different
> > groups.
> > - On all SoCs the routing of DU channels to DU pin controllers (internal
> > output of the DU channels) can be swapped within a group. This feature
> > is only used on Gen1 to control routing of the DPAD1 output.
> >
> > Routing is thus handled at the group level, but for Gen2 hardware
> > requires configuration of the DPAD1 and VSPD1 routing in the first group
> > even when only the second group is enabled.
>
> That's "DPAD0 and VSPD1", isn't it?
Yes, my bad. Will fix.
> > Routing at the group level is currently configured when applying CRTC
> > configuration. Global routing is configured at the same time, and is
> > additionally configured by the plane setup code to set VSPD1 routing.
> > This results in code paths that are difficult to follow.
> >
> > Simplify the routing configuration by performing it all directly, based
> > on CRTC and CRTC group state. Group-level routing is moved to group
> > setup as it only depends on the group state and the state of the CRTCs
> > it contains. Global routing is moved to the commit tail handler, and
> > based on global DU state.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@xxxxxxxxxxxxxxxx>
> > ---
> > drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 3 +-
> > drivers/gpu/drm/rcar-du/rcar_du_drv.h | 1 -
> > drivers/gpu/drm/rcar-du/rcar_du_group.c | 154 ++++++++++++++++--------
> > drivers/gpu/drm/rcar-du/rcar_du_group.h | 3 +-
> > drivers/gpu/drm/rcar-du/rcar_du_kms.c | 16 +--
> > drivers/gpu/drm/rcar-du/rcar_du_plane.c | 10 +-
> > 6 files changed, 115 insertions(+), 72 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> > index ab5c288f9d09..f6ea19674a31 100644
> > --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> > +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> > @@ -695,9 +695,8 @@ int rcar_du_crtc_atomic_modeset(struct drm_device *dev,
> > !crtc_state->active)
> > continue;
> >
> > - /* Configure display timings and output routing. */
> > + /* Configure display timings. */
> > rcar_du_crtc_set_display_timing(rcrtc);
> > - rcar_du_group_set_routing(rcrtc->group);
> >
> > if (rcar_du_has(rcrtc->dev, RCAR_DU_FEATURE_VSP1_SOURCE))
> > rcar_du_vsp_modeset(rcrtc);
> > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.h b/drivers/gpu/drm/rcar-du/rcar_du_drv.h
> > index 0cc0984bf2ea..4e3a12496098 100644
> > --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.h
> > +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.h
> > @@ -92,7 +92,6 @@ struct rcar_du_device {
> > } props;
> >
> > unsigned int dpad0_source;
> > - unsigned int dpad1_source;
> > unsigned int vspd1_sink;
> > };
> >
> > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_group.c b/drivers/gpu/drm/rcar-du/rcar_du_group.c
> > index 7c9145778567..261baaf7c1f5 100644
> > --- a/drivers/gpu/drm/rcar-du/rcar_du_group.c
> > +++ b/drivers/gpu/drm/rcar-du/rcar_du_group.c
> > @@ -46,6 +46,10 @@ void rcar_du_group_write(struct rcar_du_group *rgrp, u32 reg, u32 data)
> > rcar_du_write(rgrp->dev, rgrp->mmio_offset + reg, data);
> > }
> >
> > +/* -----------------------------------------------------------------------------
> > + * Static Group Setup
> > + */
> > +
> > static void rcar_du_group_setup_pins(struct rcar_du_group *rgrp)
> > {
> > u32 defr6 = DEFR6_CODE;
> > @@ -59,37 +63,6 @@ static void rcar_du_group_setup_pins(struct rcar_du_group *rgrp)
> > rcar_du_group_write(rgrp, DEFR6, defr6);
> > }
> >
> > -static void rcar_du_group_setup_defr8(struct rcar_du_group *rgrp)
> > -{
> > - struct rcar_du_device *rcdu = rgrp->dev;
> > - u32 defr8 = DEFR8_CODE;
> > -
> > - if (rcdu->info->gen < 3) {
> > - defr8 |= DEFR8_DEFE8;
> > -
> > - /*
> > - * On Gen2 the DEFR8 register for the first group also controls
> > - * RGB output routing to DPAD0 and VSPD1 routing to DU0/1/2 for
> > - * DU instances that support it.
> > - */
> > - if (rgrp->index == 0) {
> > - defr8 |= DEFR8_DRGBS_DU(rcdu->dpad0_source);
> > - if (rgrp->dev->vspd1_sink == 2)
> > - defr8 |= DEFR8_VSCS;
> > - }
> > - } else {
> > - /*
> > - * On Gen3 VSPD routing can't be configured, and DPAD routing
> > - * is set in the group corresponding to the DPAD output (no Gen3
> > - * SoC has multiple DPAD sources belonging to separate groups).
> > - */
> > - if (rgrp->index == rcdu->dpad0_source / 2)
> > - defr8 |= DEFR8_DRGBS_DU(rcdu->dpad0_source);
> > - }
> > -
> > - rcar_du_group_write(rgrp, DEFR8, defr8);
> > -}
> > -
> > static void rcar_du_group_setup_didsr(struct rcar_du_group *rgrp)
> > {
> > struct rcar_du_device *rcdu = rgrp->dev;
> > @@ -153,10 +126,8 @@ static void rcar_du_group_setup(struct rcar_du_group *rgrp)
> >
> > rcar_du_group_setup_pins(rgrp);
> >
> > - if (rcdu->info->gen >= 2) {
> > - rcar_du_group_setup_defr8(rgrp);
> > + if (rcdu->info->gen >= 2)
> > rcar_du_group_setup_didsr(rgrp);
> > - }
> >
> > if (rcdu->info->gen >= 3)
> > rcar_du_group_write(rgrp, DEFR10, DEFR10_CODE | DEFR10_DEFE10);
> > @@ -174,6 +145,10 @@ static void rcar_du_group_setup(struct rcar_du_group *rgrp)
> > mutex_unlock(&rgrp->lock);
> > }
> >
> > +/* -----------------------------------------------------------------------------
> > + * Start & Stop
> > + */
> > +
> > static void __rcar_du_group_start_stop(struct rcar_du_group *rgrp, bool start)
> > {
> > struct rcar_du_device *rcdu = rgrp->dev;
> > @@ -229,26 +204,63 @@ void rcar_du_group_restart(struct rcar_du_group *rgrp)
> > __rcar_du_group_start_stop(rgrp, true);
> > }
> >
> > +/* -----------------------------------------------------------------------------
> > + * Input and Output Routing
> > + */
> > +
> > +static void rcar_du_group_setup_defr8(struct rcar_du_group *rgrp)
> > +{
> > + struct rcar_du_device *rcdu = rgrp->dev;
> > + u32 defr8 = DEFR8_CODE;
> > +
> > + if (rcdu->info->gen < 3) {
> > + defr8 |= DEFR8_DEFE8;
> > +
> > + /*
> > + * On Gen2 the DEFR8 register for the first group also controls
> > + * RGB output routing to DPAD0 and VSPD1 routing to DU0/1/2 for
> > + * DU instances that support it.
> > + */
> > + if (rgrp->index == 0) {
> > + defr8 |= DEFR8_DRGBS_DU(rcdu->dpad0_source);
> > + if (rgrp->dev->vspd1_sink == 2)
> > + defr8 |= DEFR8_VSCS;
> > + }
> > + } else {
> > + /*
> > + * On Gen3 VSPD routing can't be configured, and DPAD routing
> > + * is set in the group corresponding to the DPAD output (no Gen3
> > + * SoC has multiple DPAD sources belonging to separate groups).
> > + */
> > + if (rgrp->index == rcdu->dpad0_source / 2)
> > + defr8 |= DEFR8_DRGBS_DU(rcdu->dpad0_source);
> > + }
> > +
> > + rcar_du_group_write(rgrp, DEFR8, defr8);
> > +}
> > +
> > int rcar_du_set_dpad0_vsp1_routing(struct rcar_du_device *rcdu)
> > {
> > struct rcar_du_group *rgrp;
> > struct rcar_du_crtc *crtc;
> > - unsigned int index;
> > int ret;
> >
> > - if (rcdu->info->gen < 2)
> > + /*
> > + * Only Gen2 hardware has global routing not handled in the group that
> > + * holds the corresponding CRTCs.
> > + */
> > + if (rcdu->info->gen != 2)
> > return 0;
> >
> > /*
> > * RGB output routing to DPAD0 and VSP1D routing to DU0/1/2 are
>
> "VSPD1"?
Yes, will fix this too.
> > - * configured in the DEFR8 register of the first group on Gen2 and the
> > - * last group on Gen3. As this function can be called with the DU
> > - * channels of the corresponding CRTCs disabled, we need to enable the
> > - * group clock before accessing the register.
> > + * configured in the DEFR8 register of the first group on Gen2. As this
> > + * function can be called with the DU channels of the corresponding
> > + * CRTCs disabled, we need to enable the group clock before accessing
> > + * the register.
> > */
> > - index = rcdu->info->gen < 3 ? 0 : DIV_ROUND_UP(rcdu->num_crtcs, 2) - 1;
> > - rgrp = &rcdu->groups[index];
> > - crtc = &rcdu->crtcs[index * 2];
> > + rgrp = &rcdu->groups[0];
> > + crtc = &rcdu->crtcs[0];
> >
> > ret = clk_prepare_enable(crtc->clock);
> > if (ret < 0)
> > @@ -302,19 +314,33 @@ static void rcar_du_group_set_dpad_levels(struct rcar_du_group *rgrp)
> > rcar_du_group_write(rgrp, DOFLR, doflr);
> > }
> >
> > -int rcar_du_group_set_routing(struct rcar_du_group *rgrp)
> > +static void rcar_du_group_set_routing(struct rcar_du_group *rgrp)
> > {
> > struct rcar_du_device *rcdu = rgrp->dev;
> > u32 dorcr = rcar_du_group_read(rgrp, DORCR);
> > + bool sp1_to_pin2 = false;
> >
> > dorcr &= ~(DORCR_PG2T | DORCR_DK2S | DORCR_PG2D_MASK);
> >
> > /*
> > - * Set the DPAD1 pins sources. Select CRTC 0 if explicitly requested and
> > - * CRTC 1 in all other cases to avoid cloning CRTC 0 to DPAD0 and DPAD1
> > - * by default.
> > + * Configure the superposition processor to pin controller routing.
> > + * Hardcode the assignment, except on Gen1 where we use it to route the
> > + * DU channels to DPAD1. There we route CRTC 0 to DPAD1 if explicitly
> > + * requested, and CRTC 1 in all other cases to avoid cloning CRTC 0 to
> > + * DPAD0 and DPAD1 by default.
> > */
> > - if (rcdu->dpad1_source == rgrp->index * 2)
> > + if (rcdu->info->gen == 1 && rgrp->index == 0) {
> > + struct rcar_du_crtc_state *rstate;
> > + struct rcar_du_crtc *rcrtc;
> > +
> > + rcrtc = &rcdu->crtcs[0];
> > + rstate = to_rcar_crtc_state(rcrtc->crtc.state);
> > +
> > + if (rstate->outputs & BIT(RCAR_DU_OUTPUT_DPAD1))
> > + sp1_to_pin2 = true;
> > + }
> > +
> > + if (sp1_to_pin2)
> > dorcr |= DORCR_PG2D_DS1;
> > else
> > dorcr |= DORCR_PG2T | DORCR_DK2S | DORCR_PG2D_DS2;
> > @@ -323,7 +349,7 @@ int rcar_du_group_set_routing(struct rcar_du_group *rgrp)
> >
> > rcar_du_group_set_dpad_levels(rgrp);
> >
> > - return rcar_du_set_dpad0_vsp1_routing(rgrp->dev);
> > + rcar_du_group_setup_defr8(rgrp);
> > }
> >
> > /* -----------------------------------------------------------------------------
> > @@ -430,20 +456,36 @@ rcar_du_get_new_group_state(struct drm_atomic_state *state,
> > int rcar_du_group_atomic_check(struct drm_device *dev,
> > struct drm_atomic_state *state)
> > {
> > - struct drm_crtc_state *crtc_state;
> > + static const u32 dpad_mask = BIT(RCAR_DU_OUTPUT_DPAD1)
> > + | BIT(RCAR_DU_OUTPUT_DPAD0);
> > + struct drm_crtc_state *old_crtc_state;
> > + struct drm_crtc_state *new_crtc_state;
> > struct drm_crtc *crtc;
> > unsigned int i;
> >
> > - for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
> > + for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
> > struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
> > + struct rcar_du_crtc_state *old_rcrtc_state;
> > + struct rcar_du_crtc_state *new_rcrtc_state;
> > struct rcar_du_group_state *gstate;
> >
> > gstate = rcar_du_get_group_state(state, rcrtc->group);
> > if (IS_ERR(gstate))
> > return PTR_ERR(gstate);
> >
> > - if (crtc_state->active)
> > + if (new_crtc_state->active)
> > gstate->enabled = true;
> > +
> > + if (!new_crtc_state->active_changed &&
> > + !new_crtc_state->connectors_changed)
> > + continue;
> > +
> > + old_rcrtc_state = to_rcar_crtc_state(old_crtc_state);
> > + new_rcrtc_state = to_rcar_crtc_state(new_crtc_state);
> > +
> > + if ((old_rcrtc_state->outputs & dpad_mask) !=
> > + (new_rcrtc_state->outputs & dpad_mask))
> > + gstate->dpad_routing_changed = true;
> > }
> >
> > return 0;
> > @@ -463,8 +505,14 @@ void rcar_du_group_atomic_setup(struct drm_device *dev,
> > old_state = to_rcar_group_state(old_pstate);
> > new_state = to_rcar_group_state(new_pstate);
> >
> > - if (!old_state->enabled && new_state->enabled)
> > + if (!new_state->enabled)
> > + continue;
> > +
> > + if (!old_state->enabled)
> > rcar_du_group_setup(rgrp);
> > +
> > + if (!old_state->enabled || new_state->dpad_routing_changed)
> > + rcar_du_group_set_routing(rgrp);
> > }
> > }
> >
> > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_group.h b/drivers/gpu/drm/rcar-du/rcar_du_group.h
> > index 20efd2251ec4..b31b3bf8cb94 100644
> > --- a/drivers/gpu/drm/rcar-du/rcar_du_group.h
> > +++ b/drivers/gpu/drm/rcar-du/rcar_du_group.h
> > @@ -58,11 +58,13 @@ struct rcar_du_group {
> > * struct rcar_du_group_state - Driver-specific group state
> > * @state: base DRM private state
> > * @enabled: true if at least one CRTC in the group is enabled
> > + * @dpad_routing_changed: set if CRTC to DPAD output routing has changed
> > */
> > struct rcar_du_group_state {
> > struct drm_private_state state;
> >
> > bool enabled;
> > + bool dpad_routing_changed;
> > };
> >
> > #define to_rcar_group_state(s) \
> > @@ -73,7 +75,6 @@ void rcar_du_group_write(struct rcar_du_group *rgrp, u32 reg, u32 data);
> >
> > void rcar_du_group_start_stop(struct rcar_du_group *rgrp, bool start);
> > void rcar_du_group_restart(struct rcar_du_group *rgrp);
> > -int rcar_du_group_set_routing(struct rcar_du_group *rgrp);
> >
> > int rcar_du_set_dpad0_vsp1_routing(struct rcar_du_device *rcdu);
> >
> > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> > index 65396134fba1..778060bfd383 100644
> > --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> > +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> > @@ -393,14 +393,14 @@ static void rcar_du_atomic_commit_tail(struct drm_atomic_state *old_state)
> > struct rcar_du_device *rcdu = dev->dev_private;
> > struct drm_crtc_state *crtc_state;
> > struct drm_crtc *crtc;
> > + unsigned int vspd1_sink = rcdu->vspd1_sink;
> > + unsigned int dpad0_source = rcdu->dpad0_source;
> > unsigned int i;
> >
> > /*
> > - * Store RGB routing to DPAD0 and DPAD1, the hardware will be configured
> > - * when starting the CRTCs.
> > + * Store RGB routing to DPAD0, the hardware will be configured when
> > + * setting up the groups.
> > */
> > - rcdu->dpad1_source = -1;
> > -
> > for_each_new_crtc_in_state(old_state, crtc, crtc_state, i) {
> > struct rcar_du_crtc_state *rcrtc_state =
> > to_rcar_crtc_state(crtc_state);
> > @@ -408,9 +408,6 @@ static void rcar_du_atomic_commit_tail(struct drm_atomic_state *old_state)
> >
> > if (rcrtc_state->outputs & BIT(RCAR_DU_OUTPUT_DPAD0))
> > rcdu->dpad0_source = rcrtc->index;
> > -
> > - if (rcrtc_state->outputs & BIT(RCAR_DU_OUTPUT_DPAD1))
> > - rcdu->dpad1_source = rcrtc->index;
> > }
> >
> > /* Apply the atomic update. */
> > @@ -421,6 +418,11 @@ static void rcar_du_atomic_commit_tail(struct drm_atomic_state *old_state)
> > rcar_du_crtc_atomic_modeset(dev, old_state);
> > drm_atomic_helper_commit_planes(dev, old_state,
> > DRM_PLANE_COMMIT_ACTIVE_ONLY);
> > +
> > + if (rcdu->vspd1_sink != vspd1_sink ||
> > + rcdu->dpad0_source != dpad0_source)
> > + rcar_du_set_dpad0_vsp1_routing(rcdu);
> > +
> > drm_atomic_helper_commit_modeset_enables(dev, old_state);
> >
> > rcar_du_crtc_atomic_enter_standby(dev, old_state);
> > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_plane.c b/drivers/gpu/drm/rcar-du/rcar_du_plane.c
> > index c6430027169f..9bf32585dab3 100644
> > --- a/drivers/gpu/drm/rcar-du/rcar_du_plane.c
> > +++ b/drivers/gpu/drm/rcar-du/rcar_du_plane.c
> > @@ -552,14 +552,8 @@ void __rcar_du_plane_setup(struct rcar_du_group *rgrp,
> > if (rcdu->info->gen < 3)
> > rcar_du_plane_setup_scanout(rgrp, state);
> >
> > - if (state->source == RCAR_DU_PLANE_VSPD1) {
> > - unsigned int vspd1_sink = rgrp->index ? 2 : 0;
> > -
> > - if (rcdu->vspd1_sink != vspd1_sink) {
> > - rcdu->vspd1_sink = vspd1_sink;
> > - rcar_du_set_dpad0_vsp1_routing(rcdu);
> > - }
> > - }
> > + if (state->source == RCAR_DU_PLANE_VSPD1)
> > + rcdu->vspd1_sink = rgrp->index ? 2 : 0;
> > }
> >
> > int __rcar_du_plane_atomic_check(struct drm_plane *plane,
--
Regards,
Laurent Pinchart
