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@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.
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
>
>
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
