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@xxxxxxxxxx>
> Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> 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@xxxxxxxxxxxxxxx>
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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
