| Message ID | 20240404233736.7946-2-lyude@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | nouveau: GSP DP aux fixes | expand |
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 >
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; }
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> --- drivers/gpu/drm/nouveau/nouveau_dp.c | 10 ++++++++++ 1 file changed, 10 insertions(+)