Nouveau - Aug 2018 - [PATCH v2] drm/nouveau: Fix nouveau_connector_ddc_detect()

It looks like that when we moved over to using
drm_connector_for_each_possible_encoder() in nouveau, that one rather
important part of this function got dropped by accident:

	/*          Right   v   here */
	for (i = 0; nv_encoder = NULL, i < DRM_CONNECTOR_MAX_ENCODER; i++) {
		int id = connector->encoder_ids[i];
		if (id == 0)
			break;

Since it's rather difficult to notice: the conditional in this loop is
actually:

	nv_encoder = NULL, i < DRM_CONNECTOR_MAX_ENCODER

Meaning that all early breaks result in nv_encoder keeping it's value,
otherwise nv_encoder = NULL. Ugh.

Since this got dropped, nouveau_connector_ddc_detect() now returns an
encoder for every single connector, regardless of whether or not it's
detected:

    [ 1780.056185] nouveau 0000:01:00.0: DRM: DDC responded, but no EDID for
DP-2

So: fix this to ensure we only return an encoder if we actually found
one, and clean up the rest of the function while we're at it since it's
nearly impossible to read properly.

Changes since v1:
- Don't skip ddc probing for LVDS if we can't switch DDC through
  vga-switcheroo, just do the DDC probing without calling
  vga_switcheroo_lock_ddc() - skeggsb

Signed-off-by: Lyude Paul <lyude at redhat.com>
Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
Fixes: ddba766dd07e ("drm/nouveau: Use
drm_connector_for_each_possible_encoder()")
---
 drivers/gpu/drm/nouveau/nouveau_connector.c | 49 ++++++++++++---------
 1 file changed, 28 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nouveau_connector.c
b/drivers/gpu/drm/nouveau/nouveau_connector.c
index 71c3bd8ff9f8..461111aaedc4 100644
--- a/drivers/gpu/drm/nouveau/nouveau_connector.c
+++ b/drivers/gpu/drm/nouveau/nouveau_connector.c
@@ -414,9 +414,10 @@ nouveau_connector_ddc_detect(struct drm_connector
*connector)
 	struct nouveau_connector *nv_connector = nouveau_connector(connector);
 	struct nouveau_drm *drm = nouveau_drm(dev);
 	struct nvkm_gpio *gpio = nvxx_gpio(&drm->client.device);
-	struct nouveau_encoder *nv_encoder = NULL;
+	struct nouveau_encoder *nv_encoder = NULL, *found = NULL;
 	struct drm_encoder *encoder;
-	int i, panel = -ENODEV;
+	int i, ret, panel = -ENODEV;
+	bool switcheroo_ddc = false;
 
 	/* eDP panels need powering on by us (if the VBIOS doesn't default it
 	 * to on) before doing any AUX channel transactions.  LVDS panel power
@@ -433,37 +434,43 @@ nouveau_connector_ddc_detect(struct drm_connector
*connector)
 	drm_connector_for_each_possible_encoder(connector, encoder, i) {
 		nv_encoder = nouveau_encoder(encoder);
 
-		if (nv_encoder->dcb->type == DCB_OUTPUT_DP) {
-			int ret = nouveau_dp_detect(nv_encoder);
+		switch (nv_encoder->dcb->type) {
+		case DCB_OUTPUT_DP:
+			ret = nouveau_dp_detect(nv_encoder);
 			if (ret == NOUVEAU_DP_MST)
 				return NULL;
-			if (ret == NOUVEAU_DP_SST)
-				break;
-		} else
-		if ((vga_switcheroo_handler_flags() &
-		     VGA_SWITCHEROO_CAN_SWITCH_DDC) &&
-		    nv_encoder->dcb->type == DCB_OUTPUT_LVDS &&
-		    nv_encoder->i2c) {
-			int ret;
-			vga_switcheroo_lock_ddc(dev->pdev);
-			ret = nvkm_probe_i2c(nv_encoder->i2c, 0x50);
-			vga_switcheroo_unlock_ddc(dev->pdev);
-			if (ret)
+			else if (ret == NOUVEAU_DP_SST)
+				found = nv_encoder;
+
+			break;
+		case DCB_OUTPUT_LVDS:
+			switcheroo_ddc = !!(vga_switcheroo_handler_flags() &
+					    VGA_SWITCHEROO_CAN_SWITCH_DDC);
+		/* fall-through */
+		default:
+			if (!nv_encoder->i2c)
 				break;
-		} else
-		if (nv_encoder->i2c) {
+
+			if (switcheroo_ddc)
+				vga_switcheroo_lock_ddc(dev->pdev);
 			if (nvkm_probe_i2c(nv_encoder->i2c, 0x50))
-				break;
+				found = nv_encoder;
+			if (switcheroo_ddc)
+				vga_switcheroo_unlock_ddc(dev->pdev);
+
+			break;
 		}
+		if (found)
+			break;
 	}
 
 	/* eDP panel not detected, restore panel power GPIO to previous
 	 * state to avoid confusing the SOR for other output types.
 	 */
-	if (!nv_encoder && panel == 0)
+	if (!found && panel == 0)
 		nvkm_gpio_set(gpio, 0, DCB_GPIO_PANEL_POWER, 0xff, panel);
 
-	return nv_encoder;
+	return found;
 }
 
 static struct nouveau_encoder *
-- 
2.17.1

On Thu, Aug 30, 2018 at 01:16:28PM -0400, Lyude Paul
wrote:
> It looks like that when we moved over to using
> drm_connector_for_each_possible_encoder() in nouveau, that one rather
> important part of this function got dropped by accident:
> 
> 	/*          Right   v   here */
> 	for (i = 0; nv_encoder = NULL, i < DRM_CONNECTOR_MAX_ENCODER; i++) {
> 		int id = connector->encoder_ids[i];
> 		if (id == 0)
> 			break;
> 
> Since it's rather difficult to notice: the conditional in this loop is
> actually:
> 
> 	nv_encoder = NULL, i < DRM_CONNECTOR_MAX_ENCODER
> 
> Meaning that all early breaks result in nv_encoder keeping it's value,
> otherwise nv_encoder = NULL. Ugh.
> 
> Since this got dropped, nouveau_connector_ddc_detect() now returns an
> encoder for every single connector, regardless of whether or not it's
> detected:
> 
>     [ 1780.056185] nouveau 0000:01:00.0: DRM: DDC responded, but no EDID
for DP-2
> 
> So: fix this to ensure we only return an encoder if we actually found
> one, and clean up the rest of the function while we're at it since
it's
> nearly impossible to read properly.
> 
> Changes since v1:
> - Don't skip ddc probing for LVDS if we can't switch DDC through
>   vga-switcheroo, just do the DDC probing without calling
>   vga_switcheroo_lock_ddc() - skeggsb
> 
> Signed-off-by: Lyude Paul <lyude at redhat.com>
> Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> Fixes: ddba766dd07e ("drm/nouveau: Use
drm_connector_for_each_possible_encoder()")
> ---
>  drivers/gpu/drm/nouveau/nouveau_connector.c | 49 ++++++++++++---------
>  1 file changed, 28 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/gpu/drm/nouveau/nouveau_connector.c
b/drivers/gpu/drm/nouveau/nouveau_connector.c
> index 71c3bd8ff9f8..461111aaedc4 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_connector.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_connector.c
> @@ -414,9 +414,10 @@ nouveau_connector_ddc_detect(struct drm_connector
*connector)
>  	struct nouveau_connector *nv_connector = nouveau_connector(connector);
>  	struct nouveau_drm *drm = nouveau_drm(dev);
>  	struct nvkm_gpio *gpio = nvxx_gpio(&drm->client.device);
> -	struct nouveau_encoder *nv_encoder = NULL;
> +	struct nouveau_encoder *nv_encoder = NULL, *found = NULL;
>  	struct drm_encoder *encoder;
> -	int i, panel = -ENODEV;
> +	int i, ret, panel = -ENODEV;
> +	bool switcheroo_ddc = false;
>  
>  	/* eDP panels need powering on by us (if the VBIOS doesn't default it
>  	 * to on) before doing any AUX channel transactions.  LVDS panel power
> @@ -433,37 +434,43 @@ nouveau_connector_ddc_detect(struct drm_connector
*connector)
>  	drm_connector_for_each_possible_encoder(connector, encoder, i) {
>  		nv_encoder = nouveau_encoder(encoder);
>  
> -		if (nv_encoder->dcb->type == DCB_OUTPUT_DP) {
> -			int ret = nouveau_dp_detect(nv_encoder);
> +		switch (nv_encoder->dcb->type) {
> +		case DCB_OUTPUT_DP:
> +			ret = nouveau_dp_detect(nv_encoder);
>  			if (ret == NOUVEAU_DP_MST)
>  				return NULL;
> -			if (ret == NOUVEAU_DP_SST)
> -				break;
> -		} else
> -		if ((vga_switcheroo_handler_flags() &
> -		     VGA_SWITCHEROO_CAN_SWITCH_DDC) &&
> -		    nv_encoder->dcb->type == DCB_OUTPUT_LVDS &&
> -		    nv_encoder->i2c) {
> -			int ret;
> -			vga_switcheroo_lock_ddc(dev->pdev);
> -			ret = nvkm_probe_i2c(nv_encoder->i2c, 0x50);
> -			vga_switcheroo_unlock_ddc(dev->pdev);
> -			if (ret)
> +			else if (ret == NOUVEAU_DP_SST)
> +				found = nv_encoder;
> +
> +			break;
> +		case DCB_OUTPUT_LVDS:
> +			switcheroo_ddc = !!(vga_switcheroo_handler_flags() &
> +					    VGA_SWITCHEROO_CAN_SWITCH_DDC);
> +		/* fall-through */
> +		default:
> +			if (!nv_encoder->i2c)
>  				break;
> -		} else
> -		if (nv_encoder->i2c) {
> +
> +			if (switcheroo_ddc)
> +				vga_switcheroo_lock_ddc(dev->pdev);
>  			if (nvkm_probe_i2c(nv_encoder->i2c, 0x50))
> -				break;
> +				found = nv_encoder;
> +			if (switcheroo_ddc)
> +				vga_switcheroo_unlock_ddc(dev->pdev);
> +
> +			break;
>  		}
> +		if (found)
> +			break;
The diff is rather messy due to the change to a switch, but after staring
at it for a while it does look correct to me.

Reviewed-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
and sorry for the mess.
>  	}
>  
>  	/* eDP panel not detected, restore panel power GPIO to previous
>  	 * state to avoid confusing the SOR for other output types.
>  	 */
> -	if (!nv_encoder && panel == 0)
> +	if (!found && panel == 0)
>  		nvkm_gpio_set(gpio, 0, DCB_GPIO_PANEL_POWER, 0xff, panel);
>  
> -	return nv_encoder;
> +	return found;
>  }
>  
>  static struct nouveau_encoder *
> -- 
> 2.17.1
-- 
Ville Syrjälä
Intel

On Thu, 2018-08-30 at 20:36 +0300, Ville Syrjälä wrote:
> On Thu, Aug 30, 2018 at 01:16:28PM -0400, Lyude Paul wrote:
> > It looks like that when we moved over to using
> > drm_connector_for_each_possible_encoder() in nouveau, that one rather
> > important part of this function got dropped by accident:
> > 
> > 	/*          Right   v   here */
> > 	for (i = 0; nv_encoder = NULL, i < DRM_CONNECTOR_MAX_ENCODER; i++)
{
> > 		int id = connector->encoder_ids[i];
> > 		if (id == 0)
> > 			break;
> > 
> > Since it's rather difficult to notice: the conditional in this
loop is
> > actually:
> > 
> > 	nv_encoder = NULL, i < DRM_CONNECTOR_MAX_ENCODER
> > 
> > Meaning that all early breaks result in nv_encoder keeping it's
value,
> > otherwise nv_encoder = NULL. Ugh.
> > 
> > Since this got dropped, nouveau_connector_ddc_detect() now returns an
> > encoder for every single connector, regardless of whether or not
it's
> > detected:
> > 
> >     [ 1780.056185] nouveau 0000:01:00.0: DRM: DDC responded, but no
EDID for
> > DP-2
> > 
> > So: fix this to ensure we only return an encoder if we actually found
> > one, and clean up the rest of the function while we're at it since
it's
> > nearly impossible to read properly.
> > 
> > Changes since v1:
> > - Don't skip ddc probing for LVDS if we can't switch DDC
through
> >   vga-switcheroo, just do the DDC probing without calling
> >   vga_switcheroo_lock_ddc() - skeggsb
> > 
> > Signed-off-by: Lyude Paul <lyude at redhat.com>
> > Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > Fixes: ddba766dd07e ("drm/nouveau: Use
> > drm_connector_for_each_possible_encoder()")
> > ---
> >  drivers/gpu/drm/nouveau/nouveau_connector.c | 49
++++++++++++---------
> >  1 file changed, 28 insertions(+), 21 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/nouveau/nouveau_connector.c
> > b/drivers/gpu/drm/nouveau/nouveau_connector.c
> > index 71c3bd8ff9f8..461111aaedc4 100644
> > --- a/drivers/gpu/drm/nouveau/nouveau_connector.c
> > +++ b/drivers/gpu/drm/nouveau/nouveau_connector.c
> > @@ -414,9 +414,10 @@ nouveau_connector_ddc_detect(struct drm_connector
> > *connector)
> >  	struct nouveau_connector *nv_connector =
nouveau_connector(connector);
> >  	struct nouveau_drm *drm = nouveau_drm(dev);
> >  	struct nvkm_gpio *gpio = nvxx_gpio(&drm->client.device);
> > -	struct nouveau_encoder *nv_encoder = NULL;
> > +	struct nouveau_encoder *nv_encoder = NULL, *found = NULL;
> >  	struct drm_encoder *encoder;
> > -	int i, panel = -ENODEV;
> > +	int i, ret, panel = -ENODEV;
> > +	bool switcheroo_ddc = false;
> >  
> >  	/* eDP panels need powering on by us (if the VBIOS doesn't
default it
> >  	 * to on) before doing any AUX channel transactions.  LVDS panel
power
> > @@ -433,37 +434,43 @@ nouveau_connector_ddc_detect(struct
drm_connector
> > *connector)
> >  	drm_connector_for_each_possible_encoder(connector, encoder, i) {
> >  		nv_encoder = nouveau_encoder(encoder);
> >  
> > -		if (nv_encoder->dcb->type == DCB_OUTPUT_DP) {
> > -			int ret = nouveau_dp_detect(nv_encoder);
> > +		switch (nv_encoder->dcb->type) {
> > +		case DCB_OUTPUT_DP:
> > +			ret = nouveau_dp_detect(nv_encoder);
> >  			if (ret == NOUVEAU_DP_MST)
> >  				return NULL;
> > -			if (ret == NOUVEAU_DP_SST)
> > -				break;
> > -		} else
> > -		if ((vga_switcheroo_handler_flags() &
> > -		     VGA_SWITCHEROO_CAN_SWITCH_DDC) &&
> > -		    nv_encoder->dcb->type == DCB_OUTPUT_LVDS &&
> > -		    nv_encoder->i2c) {
> > -			int ret;
> > -			vga_switcheroo_lock_ddc(dev->pdev);
> > -			ret = nvkm_probe_i2c(nv_encoder->i2c, 0x50);
> > -			vga_switcheroo_unlock_ddc(dev->pdev);
> > -			if (ret)
> > +			else if (ret == NOUVEAU_DP_SST)
> > +				found = nv_encoder;
> > +
> > +			break;
> > +		case DCB_OUTPUT_LVDS:
> > +			switcheroo_ddc = !!(vga_switcheroo_handler_flags() &
> > +					    VGA_SWITCHEROO_CAN_SWITCH_DDC);
> > +		/* fall-through */
> > +		default:
> > +			if (!nv_encoder->i2c)
> >  				break;
> > -		} else
> > -		if (nv_encoder->i2c) {
> > +
> > +			if (switcheroo_ddc)
> > +				vga_switcheroo_lock_ddc(dev->pdev);
> >  			if (nvkm_probe_i2c(nv_encoder->i2c, 0x50))
> > -				break;
> > +				found = nv_encoder;
> > +			if (switcheroo_ddc)
> > +				vga_switcheroo_unlock_ddc(dev->pdev);
> > +
> > +			break;
> >  		}
> > +		if (found)
> > +			break;
> 
> The diff is rather messy due to the change to a switch, but after staring
> at it for a while it does look correct to me.
> 
> Reviewed-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> and sorry for the mess.
No problem! Honestly, if I was the one who made those changes I expect I would
have broken this same bit of code as well :P
> 
> >  	}
> >  
> >  	/* eDP panel not detected, restore panel power GPIO to previous
> >  	 * state to avoid confusing the SOR for other output types.
> >  	 */
> > -	if (!nv_encoder && panel == 0)
> > +	if (!found && panel == 0)
> >  		nvkm_gpio_set(gpio, 0, DCB_GPIO_PANEL_POWER, 0xff, panel);
> >  
> > -	return nv_encoder;
> > +	return found;
> >  }
> >  
> >  static struct nouveau_encoder *
> > -- 
> > 2.17.1
> 
>