Re: [PATCH v3 10/10] drm: rcar-du: Centralise routing configuration in commit tail handler

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Thank you for your patch.

> On June 17, 2019 at 11:09 PM Laurent Pinchart <laurent.pinchart+renesas@xxxxxxxxxxxxxxxx> 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?

> 
> 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"?

> -	 * 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
>

CU
Uli



[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux