* [PATCH 0/2] drm/nouveau: CRTC Runtime PM ref tracking fixes
@ 2019-08-07 21:32 Lyude Paul
2019-08-07 21:33 ` [PATCH 1/2] drm/nouveau/dispnv04: Grab/put runtime PM refs on DPMS on/off Lyude Paul
2019-08-07 21:33 ` [PATCH 2/2] drm/nouveau/dispnv50: Fix runtime PM ref tracking for non-blocking modesets Lyude Paul
0 siblings, 2 replies; 6+ messages in thread
From: Lyude Paul @ 2019-08-07 21:32 UTC (permalink / raw)
To: nouveau
Cc: Daniel Vetter, David Airlie, linux-kernel, dri-devel, Ben Skeggs,
Lyude Paul, Laurent Pinchart, Karol Herbst, Ilia Mirkin
Just some runtime PM fixes for some much less noticeable runtime PM ref
tracking issues that I got reminded of when fixing some unrelated issues
with nouveau.
Lyude Paul (2):
drm/nouveau/dispnv04: Grab/put runtime PM refs on DPMS on/off
drm/nouveau/dispnv50: Fix runtime PM ref tracking for non-blocking
modesets
drivers/gpu/drm/nouveau/dispnv04/crtc.c | 18 +++---------
drivers/gpu/drm/nouveau/dispnv50/disp.c | 38 +++++++++++--------------
drivers/gpu/drm/nouveau/nouveau_drv.h | 3 --
3 files changed, 21 insertions(+), 38 deletions(-)
--
2.21.0
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH 1/2] drm/nouveau/dispnv04: Grab/put runtime PM refs on DPMS on/off 2019-08-07 21:32 [PATCH 0/2] drm/nouveau: CRTC Runtime PM ref tracking fixes Lyude Paul @ 2019-08-07 21:33 ` Lyude Paul 2019-08-07 21:55 ` Daniel Vetter 2019-08-07 21:33 ` [PATCH 2/2] drm/nouveau/dispnv50: Fix runtime PM ref tracking for non-blocking modesets Lyude Paul 1 sibling, 1 reply; 6+ messages in thread From: Lyude Paul @ 2019-08-07 21:33 UTC (permalink / raw) To: nouveau; +Cc: Ben Skeggs, David Airlie, Daniel Vetter, dri-devel, linux-kernel The code claims to grab a runtime PM ref when at least one CRTC is active, but that's not actually the case as we grab a runtime PM ref whenever a CRTC is enabled regardless of it's DPMS state. Meaning that we can end up keeping the GPU awake when there are no screens enabled, something we don't really want to do. Note that we fixed this same issue for nv50 a while ago in: commit e5d54f193572 ("drm/nouveau/drm/nouveau: Fix runtime PM leak in nv50_disp_atomic_commit()") Since we're about to remove nouveau_drm->have_disp_power_ref in the next commit, let's also simplify the RPM code here while we're at it: grab a ref during a modeset, grab additional RPM refs for each CRTC enabled by said modeset, and drop an RPM ref for each CRTC disabled by said modeset. This allows us to keep the GPU awake whenever screens are turned on, without needing to use nouveau_drm->have_disp_power_ref. Signed-off-by: Lyude Paul <lyude@redhat.com> --- drivers/gpu/drm/nouveau/dispnv04/crtc.c | 18 ++++-------------- 1 file changed, 4 insertions(+), 14 deletions(-) diff --git a/drivers/gpu/drm/nouveau/dispnv04/crtc.c b/drivers/gpu/drm/nouveau/dispnv04/crtc.c index f22f01020625..08ad8e3b9cd2 100644 --- a/drivers/gpu/drm/nouveau/dispnv04/crtc.c +++ b/drivers/gpu/drm/nouveau/dispnv04/crtc.c @@ -183,6 +183,10 @@ nv_crtc_dpms(struct drm_crtc *crtc, int mode) return; nv_crtc->last_dpms = mode; + if (mode == DRM_MODE_DPMS_ON) + pm_runtime_get_noresume(dev->dev); + else + pm_runtime_put_noidle(dev->dev); if (nv_two_heads(dev)) NVSetOwner(dev, nv_crtc->index); @@ -1045,7 +1049,6 @@ nouveau_crtc_set_config(struct drm_mode_set *set, dev = set->crtc->dev; - /* get a pm reference here */ ret = pm_runtime_get_sync(dev->dev); if (ret < 0 && ret != -EACCES) return ret; @@ -1061,19 +1064,6 @@ nouveau_crtc_set_config(struct drm_mode_set *set, } pm_runtime_mark_last_busy(dev->dev); - /* if we have active crtcs and we don't have a power ref, - take the current one */ - if (active && !drm->have_disp_power_ref) { - drm->have_disp_power_ref = true; - return ret; - } - /* if we have no active crtcs, then drop the power ref - we got before */ - if (!active && drm->have_disp_power_ref) { - pm_runtime_put_autosuspend(dev->dev); - drm->have_disp_power_ref = false; - } - /* drop the power reference we got coming in here */ pm_runtime_put_autosuspend(dev->dev); return ret; } -- 2.21.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] drm/nouveau/dispnv04: Grab/put runtime PM refs on DPMS on/off 2019-08-07 21:33 ` [PATCH 1/2] drm/nouveau/dispnv04: Grab/put runtime PM refs on DPMS on/off Lyude Paul @ 2019-08-07 21:55 ` Daniel Vetter 2019-08-07 23:06 ` Ilia Mirkin 0 siblings, 1 reply; 6+ messages in thread From: Daniel Vetter @ 2019-08-07 21:55 UTC (permalink / raw) To: Lyude Paul Cc: nouveau, Ben Skeggs, David Airlie, Daniel Vetter, dri-devel, linux-kernel On Wed, Aug 07, 2019 at 05:33:00PM -0400, Lyude Paul wrote: > The code claims to grab a runtime PM ref when at least one CRTC is > active, but that's not actually the case as we grab a runtime PM ref > whenever a CRTC is enabled regardless of it's DPMS state. Meaning that > we can end up keeping the GPU awake when there are no screens enabled, > something we don't really want to do. > > Note that we fixed this same issue for nv50 a while ago in: > > commit e5d54f193572 ("drm/nouveau/drm/nouveau: Fix runtime PM leak in > nv50_disp_atomic_commit()") > > Since we're about to remove nouveau_drm->have_disp_power_ref in the next > commit, let's also simplify the RPM code here while we're at it: grab a > ref during a modeset, grab additional RPM refs for each CRTC enabled by > said modeset, and drop an RPM ref for each CRTC disabled by said > modeset. This allows us to keep the GPU awake whenever screens are > turned on, without needing to use nouveau_drm->have_disp_power_ref. > > Signed-off-by: Lyude Paul <lyude@redhat.com> > --- > drivers/gpu/drm/nouveau/dispnv04/crtc.c | 18 ++++-------------- > 1 file changed, 4 insertions(+), 14 deletions(-) > > diff --git a/drivers/gpu/drm/nouveau/dispnv04/crtc.c b/drivers/gpu/drm/nouveau/dispnv04/crtc.c > index f22f01020625..08ad8e3b9cd2 100644 > --- a/drivers/gpu/drm/nouveau/dispnv04/crtc.c > +++ b/drivers/gpu/drm/nouveau/dispnv04/crtc.c > @@ -183,6 +183,10 @@ nv_crtc_dpms(struct drm_crtc *crtc, int mode) > return; > > nv_crtc->last_dpms = mode; > + if (mode == DRM_MODE_DPMS_ON) > + pm_runtime_get_noresume(dev->dev); > + else > + pm_runtime_put_noidle(dev->dev); it's after we filter out duplicate operations, so that part looks good. But not all of nouveau's legacy helper crtc callbacks go throuh ->dpms I think: nv_crtc_disable doesn't, and crtc helpers use that in preference over ->dpms in some cases. I think the only way to actually hit that path is if you switch an active connector from an active CRTC to an inactive one. This implicitly disables the crtc (well, should, nv_crtc_disable doesn't seem to shut down hw), and I think would leak your runtime PM reference here. At least temporarily. No idea how to best fix that. Aside from "use atomic" :-) Cheers, Daniel > > if (nv_two_heads(dev)) > NVSetOwner(dev, nv_crtc->index); > @@ -1045,7 +1049,6 @@ nouveau_crtc_set_config(struct drm_mode_set *set, > > dev = set->crtc->dev; > > - /* get a pm reference here */ > ret = pm_runtime_get_sync(dev->dev); > if (ret < 0 && ret != -EACCES) > return ret; > @@ -1061,19 +1064,6 @@ nouveau_crtc_set_config(struct drm_mode_set *set, > } > > pm_runtime_mark_last_busy(dev->dev); > - /* if we have active crtcs and we don't have a power ref, > - take the current one */ > - if (active && !drm->have_disp_power_ref) { > - drm->have_disp_power_ref = true; > - return ret; > - } > - /* if we have no active crtcs, then drop the power ref > - we got before */ > - if (!active && drm->have_disp_power_ref) { > - pm_runtime_put_autosuspend(dev->dev); > - drm->have_disp_power_ref = false; > - } > - /* drop the power reference we got coming in here */ > pm_runtime_put_autosuspend(dev->dev); > return ret; > } > -- > 2.21.0 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] drm/nouveau/dispnv04: Grab/put runtime PM refs on DPMS on/off 2019-08-07 21:55 ` Daniel Vetter @ 2019-08-07 23:06 ` Ilia Mirkin 2019-08-07 23:09 ` Lyude Paul 0 siblings, 1 reply; 6+ messages in thread From: Ilia Mirkin @ 2019-08-07 23:06 UTC (permalink / raw) To: Lyude Paul, nouveau, Ben Skeggs, David Airlie, dri-devel, LKML Cc: Daniel Vetter On Wed, Aug 7, 2019 at 5:55 PM Daniel Vetter <daniel@ffwll.ch> wrote: > > On Wed, Aug 07, 2019 at 05:33:00PM -0400, Lyude Paul wrote: > > The code claims to grab a runtime PM ref when at least one CRTC is > > active, but that's not actually the case as we grab a runtime PM ref > > whenever a CRTC is enabled regardless of it's DPMS state. Meaning that > > we can end up keeping the GPU awake when there are no screens enabled, > > something we don't really want to do. > > > > Note that we fixed this same issue for nv50 a while ago in: > > > > commit e5d54f193572 ("drm/nouveau/drm/nouveau: Fix runtime PM leak in > > nv50_disp_atomic_commit()") > > > > Since we're about to remove nouveau_drm->have_disp_power_ref in the next > > commit, let's also simplify the RPM code here while we're at it: grab a > > ref during a modeset, grab additional RPM refs for each CRTC enabled by > > said modeset, and drop an RPM ref for each CRTC disabled by said > > modeset. This allows us to keep the GPU awake whenever screens are > > turned on, without needing to use nouveau_drm->have_disp_power_ref. > > > > Signed-off-by: Lyude Paul <lyude@redhat.com> > > --- > > drivers/gpu/drm/nouveau/dispnv04/crtc.c | 18 ++++-------------- > > 1 file changed, 4 insertions(+), 14 deletions(-) > > > > diff --git a/drivers/gpu/drm/nouveau/dispnv04/crtc.c b/drivers/gpu/drm/nouveau/dispnv04/crtc.c > > index f22f01020625..08ad8e3b9cd2 100644 > > --- a/drivers/gpu/drm/nouveau/dispnv04/crtc.c > > +++ b/drivers/gpu/drm/nouveau/dispnv04/crtc.c > > @@ -183,6 +183,10 @@ nv_crtc_dpms(struct drm_crtc *crtc, int mode) > > return; > > > > nv_crtc->last_dpms = mode; > > + if (mode == DRM_MODE_DPMS_ON) > > + pm_runtime_get_noresume(dev->dev); > > + else > > + pm_runtime_put_noidle(dev->dev); > > it's after we filter out duplicate operations, so that part looks good. > But not all of nouveau's legacy helper crtc callbacks go throuh ->dpms I > think: nv_crtc_disable doesn't, and crtc helpers use that in preference > over ->dpms in some cases. > > I think the only way to actually hit that path is if you switch an active > connector from an active CRTC to an inactive one. This implicitly disables > the crtc (well, should, nv_crtc_disable doesn't seem to shut down hw), and > I think would leak your runtime PM reference here. At least temporarily. > > No idea how to best fix that. Aside from "use atomic" :-) Not sure if this is relevant to the discussion at hand, but I'd like to point out that dispnv04 is for pre-nv50 things, which definitely didn't support any kind of ACPI-based runtime suspend. -ilia ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] drm/nouveau/dispnv04: Grab/put runtime PM refs on DPMS on/off 2019-08-07 23:06 ` Ilia Mirkin @ 2019-08-07 23:09 ` Lyude Paul 0 siblings, 0 replies; 6+ messages in thread From: Lyude Paul @ 2019-08-07 23:09 UTC (permalink / raw) To: Ilia Mirkin, nouveau, Ben Skeggs, David Airlie, dri-devel, LKML Cc: Daniel Vetter On Wed, 2019-08-07 at 19:06 -0400, Ilia Mirkin wrote: > On Wed, Aug 7, 2019 at 5:55 PM Daniel Vetter <daniel@ffwll.ch> wrote: > > On Wed, Aug 07, 2019 at 05:33:00PM -0400, Lyude Paul wrote: > > > The code claims to grab a runtime PM ref when at least one CRTC is > > > active, but that's not actually the case as we grab a runtime PM ref > > > whenever a CRTC is enabled regardless of it's DPMS state. Meaning that > > > we can end up keeping the GPU awake when there are no screens enabled, > > > something we don't really want to do. > > > > > > Note that we fixed this same issue for nv50 a while ago in: > > > > > > commit e5d54f193572 ("drm/nouveau/drm/nouveau: Fix runtime PM leak in > > > nv50_disp_atomic_commit()") > > > > > > Since we're about to remove nouveau_drm->have_disp_power_ref in the next > > > commit, let's also simplify the RPM code here while we're at it: grab a > > > ref during a modeset, grab additional RPM refs for each CRTC enabled by > > > said modeset, and drop an RPM ref for each CRTC disabled by said > > > modeset. This allows us to keep the GPU awake whenever screens are > > > turned on, without needing to use nouveau_drm->have_disp_power_ref. > > > > > > Signed-off-by: Lyude Paul <lyude@redhat.com> > > > --- > > > drivers/gpu/drm/nouveau/dispnv04/crtc.c | 18 ++++-------------- > > > 1 file changed, 4 insertions(+), 14 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/nouveau/dispnv04/crtc.c > > > b/drivers/gpu/drm/nouveau/dispnv04/crtc.c > > > index f22f01020625..08ad8e3b9cd2 100644 > > > --- a/drivers/gpu/drm/nouveau/dispnv04/crtc.c > > > +++ b/drivers/gpu/drm/nouveau/dispnv04/crtc.c > > > @@ -183,6 +183,10 @@ nv_crtc_dpms(struct drm_crtc *crtc, int mode) > > > return; > > > > > > nv_crtc->last_dpms = mode; > > > + if (mode == DRM_MODE_DPMS_ON) > > > + pm_runtime_get_noresume(dev->dev); > > > + else > > > + pm_runtime_put_noidle(dev->dev); > > > > it's after we filter out duplicate operations, so that part looks good. > > But not all of nouveau's legacy helper crtc callbacks go throuh ->dpms I > > think: nv_crtc_disable doesn't, and crtc helpers use that in preference > > over ->dpms in some cases. > > > > I think the only way to actually hit that path is if you switch an active > > connector from an active CRTC to an inactive one. This implicitly disables > > the crtc (well, should, nv_crtc_disable doesn't seem to shut down hw), and > > I think would leak your runtime PM reference here. At least temporarily. > > > > No idea how to best fix that. Aside from "use atomic" :-) > > Not sure if this is relevant to the discussion at hand, but I'd like > to point out that dispnv04 is for pre-nv50 things, which definitely > didn't support any kind of ACPI-based runtime suspend. I thought it was really suspicious that such an old chipset had any kind of runtime PM, but didn't question the code lol. I guess a more appropriate patch would be to just remove runtime PM support entirely for pre-nv50. Will respin soon and do this. > > -ilia ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 2/2] drm/nouveau/dispnv50: Fix runtime PM ref tracking for non-blocking modesets 2019-08-07 21:32 [PATCH 0/2] drm/nouveau: CRTC Runtime PM ref tracking fixes Lyude Paul 2019-08-07 21:33 ` [PATCH 1/2] drm/nouveau/dispnv04: Grab/put runtime PM refs on DPMS on/off Lyude Paul @ 2019-08-07 21:33 ` Lyude Paul 1 sibling, 0 replies; 6+ messages in thread From: Lyude Paul @ 2019-08-07 21:33 UTC (permalink / raw) To: nouveau Cc: Ben Skeggs, David Airlie, Daniel Vetter, Karol Herbst, Laurent Pinchart, Ilia Mirkin, dri-devel, linux-kernel This is something that got noticed a while ago back when I was fixing a large number of runtime PM related issues in nouveau, but never got fixed: https://patchwork.freedesktop.org/series/46815/#rev7 It's not safe to iterate the entire list of CRTCs in nv50_disp_atomic_commit(), as we could be doing a non-blocking modeset on one CRTC in parallel with one or more other CRTCs. Likewise, this means it's also not safe to do so in order to track runtime PM state. While this code is certainly wrong, so far the only issues I've seen this cause in the wild is the occasional PM ref unbalance after an atomic check failure + module reloading (since the PCI device will outlive nouveau in such scenarios). So, do this far more elegantly: grab a runtime PM ref across the modeset and commit tail, then grab/put references for each CRTC enable/disable. This also ends up being much simpler then the previous broken solution we had. Finally, since we've removed all it's users: get rid of nouveau_drm->have_disp_power_ref. Signed-off-by: Lyude Paul <lyude@redhat.com> --- drivers/gpu/drm/nouveau/dispnv50/disp.c | 38 +++++++++++-------------- drivers/gpu/drm/nouveau/nouveau_drv.h | 3 -- 2 files changed, 17 insertions(+), 24 deletions(-) diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c index 126703816794..659e6fa645cb 100644 --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c @@ -1826,8 +1826,11 @@ nv50_disp_atomic_commit_tail(struct drm_atomic_state *state) NV_ATOMIC(drm, "%s: clr %04x (set %04x)\n", crtc->name, asyh->clr.mask, asyh->set.mask); - if (old_crtc_state->active && !new_crtc_state->active) + + if (old_crtc_state->active && !new_crtc_state->active) { + pm_runtime_put_noidle(dev->dev); drm_crtc_vblank_off(crtc); + } if (asyh->clr.mask) { nv50_head_flush_clr(head, asyh, atom->flush_disable); @@ -1913,8 +1916,10 @@ nv50_disp_atomic_commit_tail(struct drm_atomic_state *state) } if (new_crtc_state->active) { - if (!old_crtc_state->active) + if (!old_crtc_state->active) { drm_crtc_vblank_on(crtc); + pm_runtime_get_noresume(dev->dev); + } if (new_crtc_state->event) drm_crtc_vblank_get(crtc); } @@ -1979,6 +1984,10 @@ nv50_disp_atomic_commit_tail(struct drm_atomic_state *state) drm_atomic_helper_cleanup_planes(dev, state); drm_atomic_helper_commit_cleanup_done(state); drm_atomic_state_put(state); + + /* Drop the RPM ref we got from nv50_disp_atomic_commit() */ + pm_runtime_mark_last_busy(dev->dev); + pm_runtime_put_autosuspend(dev->dev); } static void @@ -1993,11 +2002,8 @@ static int nv50_disp_atomic_commit(struct drm_device *dev, struct drm_atomic_state *state, bool nonblock) { - struct nouveau_drm *drm = nouveau_drm(dev); struct drm_plane_state *new_plane_state; struct drm_plane *plane; - struct drm_crtc *crtc; - bool active = false; int ret, i; ret = pm_runtime_get_sync(dev->dev); @@ -2034,27 +2040,17 @@ nv50_disp_atomic_commit(struct drm_device *dev, drm_atomic_state_get(state); + /* + * Grab another RPM ref for the commit tail, which will release the + * ref when it's finished + */ + pm_runtime_get_noresume(dev->dev); + if (nonblock) queue_work(system_unbound_wq, &state->commit_work); else nv50_disp_atomic_commit_tail(state); - drm_for_each_crtc(crtc, dev) { - if (crtc->state->active) { - if (!drm->have_disp_power_ref) { - drm->have_disp_power_ref = true; - return 0; - } - active = true; - break; - } - } - - if (!active && drm->have_disp_power_ref) { - pm_runtime_put_autosuspend(dev->dev); - drm->have_disp_power_ref = false; - } - err_cleanup: if (ret) drm_atomic_helper_cleanup_planes(dev, state); diff --git a/drivers/gpu/drm/nouveau/nouveau_drv.h b/drivers/gpu/drm/nouveau/nouveau_drv.h index aae035816383..411352dd5390 100644 --- a/drivers/gpu/drm/nouveau/nouveau_drv.h +++ b/drivers/gpu/drm/nouveau/nouveau_drv.h @@ -204,9 +204,6 @@ struct nouveau_drm { /* led management */ struct nouveau_led *led; - /* display power reference */ - bool have_disp_power_ref; - struct dev_pm_domain vga_pm_domain; struct nouveau_svm *svm; -- 2.21.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-08-07 23:09 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-08-07 21:32 [PATCH 0/2] drm/nouveau: CRTC Runtime PM ref tracking fixes Lyude Paul 2019-08-07 21:33 ` [PATCH 1/2] drm/nouveau/dispnv04: Grab/put runtime PM refs on DPMS on/off Lyude Paul 2019-08-07 21:55 ` Daniel Vetter 2019-08-07 23:06 ` Ilia Mirkin 2019-08-07 23:09 ` Lyude Paul 2019-08-07 21:33 ` [PATCH 2/2] drm/nouveau/dispnv50: Fix runtime PM ref tracking for non-blocking modesets Lyude Paul
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).