Re: [PATCH 1/2] drm/nouveau/kms/nv50-: Disable AUX bus for disconnected DP ports

[Dave Airlie]

On Fri, 5 Apr 2024 at 09:37, Lyude Paul <lyude@xxxxxxxxxx> wrote:
>
> GSP has its own state for keeping track of whether or not a given display
> connector is plugged in or not, and enforces this state on the driver. In
> particular, AUX transactions on a DisplayPort connector which GSP says is
> disconnected can never succeed - and can in some cases even cause
> unexpected timeouts, which can trickle up to cause other problems. A good
> example of this is runtime power management: where we can actually get
> stuck trying to resume the GPU if a userspace application like fwupd tries
> accessing a drm_aux_dev for a disconnected port. This was an issue I hit a
> few times with my Slimbook Executive 16 - where trying to offload something
> to the discrete GPU would wake it up, and then potentially cause it to
> timeout as fwupd tried to immediately access the dp_aux_dev nodes for
> nouveau.
>
> Likewise: we don't really have any cases I know of where we'd want to
> ignore this state and try an aux transaction anyway - and failing pointless
> aux transactions immediately can even speed things up. So - let's start
> enabling/disabling the aux bus in nouveau_dp_detect() to fix this. We
> enable the aux bus during connector probing, and leave it enabled if we
> discover something is actually on the connector. Otherwise, we just shut it
> off.
>
> This should fix some people's runtime PM issues (like myself), and also get
> rid of quite of a lot of GSP error spam in dmesg.
>
> Signed-off-by: Lyude Paul <lyude@xxxxxxxxxx>


For the two patches,

Reviewed-by: Dave Airlie <airlied@xxxxxxxxxx>

> ---
>  drivers/gpu/drm/nouveau/nouveau_dp.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/drivers/gpu/drm/nouveau/nouveau_dp.c b/drivers/gpu/drm/nouveau/nouveau_dp.c
> index fb06ee17d9e54..8b1be7dd64ebe 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_dp.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_dp.c
> @@ -232,6 +232,9 @@ nouveau_dp_detect(struct nouveau_connector *nv_connector,
>             dpcd[DP_DPCD_REV] != 0)
>                 return NOUVEAU_DP_SST;
>
> +       // Ensure that the aux bus is enabled for probing
> +       drm_dp_dpcd_set_powered(&nv_connector->aux, true);
> +
>         mutex_lock(&nv_encoder->dp.hpd_irq_lock);
>         if (mstm) {
>                 /* If we're not ready to handle MST state changes yet, just
> @@ -293,6 +296,13 @@ nouveau_dp_detect(struct nouveau_connector *nv_connector,
>         if (mstm && !mstm->suspended && ret != NOUVEAU_DP_MST)
>                 nv50_mstm_remove(mstm);
>
> +       /* GSP doesn't like when we try to do aux transactions on a port it considers disconnected,
> +        * and since we don't really have a usecase for that anyway - just disable the aux bus here
> +        * if we've decided the connector is disconnected
> +        */
> +       if (ret == NOUVEAU_DP_NONE)
> +               drm_dp_dpcd_set_powered(&nv_connector->aux, false);
> +
>         mutex_unlock(&nv_encoder->dp.hpd_irq_lock);
>         return ret;
>  }
> --
> 2.44.0
>