Received-SPF: pass (google.com: domain of linux-kernel+bounces-135972-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Precedence: bulk
MIME-Version: 1.0
References: <20240404233736.7946-1-lyude@redhat.com> <20240404233736.7946-2-lyude@redhat.com>
In-Reply-To: <20240404233736.7946-2-lyude@redhat.com>
From: Dave Airlie <airlied@gmail.com>
Date: Tue, 9 Apr 2024 08:28:11 +1000
Message-ID: <CAPM=9twXXx9c9O7GFBHMhANOuMqt_tUKr97-2cPCwzZoJFtH0g@mail.gmail.com>
Subject: Re: [PATCH 1/2] drm/nouveau/kms/nv50-: Disable AUX bus for
 disconnected DP ports
To: Lyude Paul <lyude@redhat.com>
Cc: nouveau@lists.freedesktop.org, dri-devel@lists.freedesktop.org, 
	Karol Herbst <kherbst@redhat.com>, Danilo Krummrich <dakr@redhat.com>, Daniel Vetter <daniel@ffwll.ch>, 
	open list <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"

On Fri, 5 Apr 2024 at 09:37, Lyude Paul <lyude@redhat.com> 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@redhat.com>


For the two patches,

Reviewed-by: Dave Airlie <airlied@redhat.com>

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