diff mbox series

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

Message ID 20180904121027.24031-9-laurent.pinchart+renesas@ideasonboard.com (mailing list archive)
State New, archived
Headers show
Series R-Car D3/E3 display support (with LVDS PLL) | expand

Commit Message

Laurent Pinchart Sept. 4, 2018, 12:10 p.m. UTC 
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.

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(-)

Comments

Jacopo Mondi Sept. 11, 2018, 3:46 p.m. UTC | #1 
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
>
Laurent Pinchart Sept. 13, 2018, 9:25 p.m. UTC | #2 
Hi Jacopo,

Thank you for the patch.

On Tuesday, 11 September 2018 18:46:53 EEST jacopo mondi wrote:
> 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?

It's already dynamic. We support dynamic routing of the DPAD (RGB) output on 
Gen2. On Gen3 all the SoCs supported so far could only route one CRTC to the 
DPAD output, so the routing wasn't very dynamic, but the register still had to 
be programmed accordingly. This patch reuses the existing dynamic routing that 
we use on Gen2 for Gen3.

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

Your analysis of the code flow is right. The reason why we don't write DEFR8 
directly is that changes to the register only take effect when the group is 
disabled (lovely hardware design, isn't it ?). The driver relies on the fact 
that changing the routing involves disabling and enabling CRTCs, but the 
mechanism is fragile, and it might even be buggy.

The RGB output on D3 and E3 isn't officially supported by this patch series, 
so I already plan to revisit the code later and hopefully clean it.

> > 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);
diff mbox series

Patch

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);