From mboxrd@z Thu Jan 1 00:00:00 1970 From: Sean Paul Subject: Re: [PATCH v2 22/27] drm/nouveau: Don't grab runtime PM refs for HPD IRQs Date: Wed, 25 Sep 2019 16:06:56 -0400 Message-ID: <20190925200656.GN218215@art_vandelay> References: <20190903204645.25487-1-lyude@redhat.com> <20190903204645.25487-23-lyude@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: 8bit Return-path: Content-Disposition: inline In-Reply-To: <20190903204645.25487-23-lyude@redhat.com> Sender: linux-kernel-owner@vger.kernel.org To: Lyude Paul Cc: dri-devel@lists.freedesktop.org, nouveau@lists.freedesktop.org, amd-gfx@lists.freedesktop.org, Juston Li , Imre Deak , Ville =?iso-8859-1?Q?Syrj=E4l=E4?= , Harry Wentland , Daniel Vetter , Ben Skeggs , David Airlie , Daniel Vetter , linux-kernel@vger.kernel.org List-Id: amd-gfx.lists.freedesktop.org On Tue, Sep 03, 2019 at 04:46:00PM -0400, Lyude Paul wrote: > In order for suspend/resume reprobing to work, we need to be able to > perform sideband communications during suspend/resume, along with > runtime PM suspend/resume. In order to do so, we also need to make sure > that nouveau doesn't bother grabbing a runtime PM reference to do so, > since otherwise we'll start deadlocking runtime PM again. > > Note that we weren't able to do this before, because of the DP MST > helpers processing UP requests from topologies in the same context as > drm_dp_mst_hpd_irq() which would have caused us to open ourselves up to > receiving hotplug events and deadlocking with runtime suspend/resume. > Now that those requests are handled asynchronously, this change should > be completely safe. > > Cc: Juston Li > Cc: Imre Deak > Cc: Ville Syrjälä > Cc: Harry Wentland > Cc: Daniel Vetter > Signed-off-by: Lyude Paul Seems reasonable to me, but would feel better if a nouveau person confirmed Reviewed-by: Sean Paul > --- > drivers/gpu/drm/nouveau/nouveau_connector.c | 33 +++++++++++---------- > 1 file changed, 17 insertions(+), 16 deletions(-) > > diff --git a/drivers/gpu/drm/nouveau/nouveau_connector.c b/drivers/gpu/drm/nouveau/nouveau_connector.c > index 56871d34e3fb..f276918d3f3b 100644 > --- a/drivers/gpu/drm/nouveau/nouveau_connector.c > +++ b/drivers/gpu/drm/nouveau/nouveau_connector.c > @@ -1131,6 +1131,16 @@ nouveau_connector_hotplug(struct nvif_notify *notify) > const char *name = connector->name; > struct nouveau_encoder *nv_encoder; > int ret; > + bool plugged = (rep->mask != NVIF_NOTIFY_CONN_V0_UNPLUG); > + > + if (rep->mask & NVIF_NOTIFY_CONN_V0_IRQ) { > + NV_DEBUG(drm, "service %s\n", name); > + drm_dp_cec_irq(&nv_connector->aux); > + if ((nv_encoder = find_encoder(connector, DCB_OUTPUT_DP))) > + nv50_mstm_service(nv_encoder->dp.mstm); > + > + return NVIF_NOTIFY_KEEP; > + } > > ret = pm_runtime_get(drm->dev->dev); > if (ret == 0) { > @@ -1151,25 +1161,16 @@ nouveau_connector_hotplug(struct nvif_notify *notify) > return NVIF_NOTIFY_DROP; > } > > - if (rep->mask & NVIF_NOTIFY_CONN_V0_IRQ) { > - NV_DEBUG(drm, "service %s\n", name); > - drm_dp_cec_irq(&nv_connector->aux); > - if ((nv_encoder = find_encoder(connector, DCB_OUTPUT_DP))) > - nv50_mstm_service(nv_encoder->dp.mstm); > - } else { > - bool plugged = (rep->mask != NVIF_NOTIFY_CONN_V0_UNPLUG); > - > + if (!plugged) > + drm_dp_cec_unset_edid(&nv_connector->aux); > + NV_DEBUG(drm, "%splugged %s\n", plugged ? "" : "un", name); > + if ((nv_encoder = find_encoder(connector, DCB_OUTPUT_DP))) { > if (!plugged) > - drm_dp_cec_unset_edid(&nv_connector->aux); > - NV_DEBUG(drm, "%splugged %s\n", plugged ? "" : "un", name); > - if ((nv_encoder = find_encoder(connector, DCB_OUTPUT_DP))) { > - if (!plugged) > - nv50_mstm_remove(nv_encoder->dp.mstm); > - } > - > - drm_helper_hpd_irq_event(connector->dev); > + nv50_mstm_remove(nv_encoder->dp.mstm); > } > > + drm_helper_hpd_irq_event(connector->dev); > + > pm_runtime_mark_last_busy(drm->dev->dev); > pm_runtime_put_autosuspend(drm->dev->dev); > return NVIF_NOTIFY_KEEP; > -- > 2.21.0 > -- Sean Paul, Software Engineer, Google / Chromium OS