Re: [PATCH 08/16] drm: rcar-du: Enable configurable DPAD0 routing on Gen3

[jacopo mondi <jacopo@jmondi.org>]

[-- Attachment #1: Type: text/plain, Size: 4841 bytes --]

Hi Laurent,

On Tue, Sep 04, 2018 at 03:10:19PM +0300, Laurent Pinchart wrote:
> All Gen3 SoCs supported so far have a fixed association between DPAD0
> and DU channels, which led to hardcoding that association when writing
> the corresponding hardware register. The D3 and E3 will break that
> mechanism as DPAD0 can be dynamically connected to either DU0 or DU1.
>
> Make DPAD0 routing dynamic on Gen3. To ensure a valid hardware
> configuration when the DU starts without the RGB output enabled, DPAD0
> is associated at initialization time to the first DU channel that it can
> be connected to. This makes no change on Gen2 as all Gen2 SoCs can
> connected DPAD0 to DU0, which is the current implicit default value.
>
> As the DPAD0 source is always 0 when a single source is possible on
> Gen2, we can also simplify the Gen2 code in the same function to remove
> a conditional check.
>

Does this patch only prepares for routing to be mad dynamic or it is
already supported?

I am missing where those dynamic association happens, as I see the
possible dpad0 source being changed in 'rcar_du_crtc_route_output()'
which is in turn called by the mode set operation
'rcar_du_crtc_route_output()'.

But at the same time, I only see the routing register DEFR8 being
written by rcar_du_group_setup_defr8() called by the group route setup
operation rcar_du_group_set_routing() called at crtc setup time only.

Would the mode set operation on the encoder being supporsed to go to
the group's set_routing operation? Or does DEFR8 configuration happens
with a different call path?

Thanks
  j

> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
>  drivers/gpu/drm/rcar-du/rcar_du_group.c | 17 ++++++-----------
>  drivers/gpu/drm/rcar-du/rcar_du_kms.c   | 12 ++++++++++++
>  2 files changed, 18 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_group.c b/drivers/gpu/drm/rcar-du/rcar_du_group.c
> index 4c62841eff2f..f38703e7a10d 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_group.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_group.c
> @@ -56,8 +56,6 @@ static void rcar_du_group_setup_pins(struct rcar_du_group *rgrp)
>  static void rcar_du_group_setup_defr8(struct rcar_du_group *rgrp)
>  {
>  	struct rcar_du_device *rcdu = rgrp->dev;
> -	unsigned int possible_crtcs =
> -		rcdu->info->routes[RCAR_DU_OUTPUT_DPAD0].possible_crtcs;
>  	u32 defr8 = DEFR8_CODE;
>
>  	if (rcdu->info->gen < 3) {
> @@ -69,21 +67,18 @@ static void rcar_du_group_setup_defr8(struct rcar_du_group *rgrp)
>  		 * DU instances that support it.
>  		 */
>  		if (rgrp->index == 0) {
> -			if (possible_crtcs > 1)
> -				defr8 |= DEFR8_DRGBS_DU(rcdu->dpad0_source);
> +			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, but DPAD routing
> -		 * needs to be set despite having a single option available.
> +		 * 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).
>  		 */
> -		unsigned int rgb_crtc = ffs(possible_crtcs) - 1;
> -		struct rcar_du_crtc *crtc = &rcdu->crtcs[rgb_crtc];
> -
> -		if (crtc->index / 2 == rgrp->index)
> -			defr8 |= DEFR8_DRGBS_DU(crtc->index);
> +		if (rgrp->index == rcdu->dpad0_source / 2)
> +			defr8 |= DEFR8_DRGBS_DU(rcdu->dpad0_source);
>  	}
>
>  	rcar_du_group_write(rgrp, DEFR8, defr8);
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> index ed7fa3204892..bd01197700c5 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> @@ -501,6 +501,7 @@ int rcar_du_modeset_init(struct rcar_du_device *rcdu)
>  	struct drm_device *dev = rcdu->ddev;
>  	struct drm_encoder *encoder;
>  	struct drm_fbdev_cma *fbdev;
> +	unsigned int dpad0_sources;
>  	unsigned int num_encoders;
>  	unsigned int num_groups;
>  	unsigned int swindex;
> @@ -613,6 +614,17 @@ int rcar_du_modeset_init(struct rcar_du_device *rcdu)
>  		encoder->possible_clones = (1 << num_encoders) - 1;
>  	}
>
> +	/*
> +	 * Initialize the default DPAD0 source to the index of the first DU
> +	 * channel that can be connected to DPAD0. The exact value doesn't
> +	 * matter as it should be overwritten by mode setting for the RGB
> +	 * output, but it is nonetheless required to ensure a valid initial
> +	 * hardware configuration on Gen3 where DU0 can't always be connected to
> +	 * DPAD0.
> +	 */
> +	dpad0_sources = rcdu->info->routes[RCAR_DU_OUTPUT_DPAD0].possible_crtcs;
> +	rcdu->dpad0_source = ffs(dpad0_sources) - 1;
> +
>  	drm_mode_config_reset(dev);
>
>  	drm_kms_helper_poll_init(dev);
> --
> Regards,
>
> Laurent Pinchart
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]