search for: have_disp_power_ref

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

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. Changes since v1: * Don't fix CRTC RPM code in dispnv04, because it's not actually doing anything in the first place. Just get rid of it. - imirkin Lyude Paul (2): drm/nouveau/dispnv04: Remove runtime PM drm/nouveau/dispnv50:

...ake 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 tur...

...gt; 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 when...

...pu/drm/nouveau/dispnv50/disp.c > @@ -1878,7 +1878,7 @@ nv50_disp_atomic_commit(struct drm_device *dev, > nv50_disp_atomic_commit_tail(state); > > drm_for_each_crtc(crtc, dev) { > - if (crtc->state->enable) { > + if (crtc->state->active) { > if (!drm->have_disp_power_ref) { > drm->have_disp_power_ref = true; > return 0; Somewhat tangential comment on this older patch, since you continue to dig around in the runtime PM area: Whenever a crtc is activated or deactivated in nouveau, we iterate over all crtcs and acquire a runtime PM if a crtc is ac...

...{ ret = drm_atomic_helper_wait_for_fences(dev, state, true); if (ret) - goto done; + goto err_cleanup; } for_each_plane_in_state(state, plane, plane_state, i) { @@ -4147,7 +4147,7 @@ nv50_disp_atomic_commit(struct drm_device *dev, if (crtc->state->enable) { if (!drm->have_disp_power_ref) { drm->have_disp_power_ref = true; - return ret; + return 0; } active = true; break; @@ -4158,7 +4158,10 @@ nv50_disp_atomic_commit(struct drm_device *dev, pm_runtime_put_autosuspend(dev->dev); drm->have_disp_power_ref = false; } + goto done; +err_cleanup:...

This is the latest version of https://patchwork.freedesktop.org/series/45862/ . One new patch has been added that also addresses some additional issues I found with pmops_runtime_idle that would stop nouveau from suspending the GPU when running under X. Additionally, "drm/nouveau: Fix runtime PM leak in nv50_disp_atomic_commit()" has had it's CC to stable removed. Lyude Paul (3):

...block) { ret = drm_atomic_helper_wait_for_fences(dev, state, true); if (ret) - goto done; + goto err_cleanup; } for_each_plane_in_state(state, plane, plane_state, i) { @@ -4162,7 +4162,7 @@ nv50_disp_atomic_commit(struct drm_devic if (crtc->state->enable) { if (!drm->have_disp_power_ref) { drm->have_disp_power_ref = true; - return ret; + return 0; } active = true; break; @@ -4174,6 +4174,9 @@ nv50_disp_atomic_commit(struct drm_devic drm->have_disp_power_ref = false; } +err_cleanup: + if (ret) + drm_atomic_helper_cleanup_planes(dev, state); don...

...block) { ret = drm_atomic_helper_wait_for_fences(dev, state, true); if (ret) - goto done; + goto err_cleanup; } for_each_plane_in_state(state, plane, plane_state, i) { @@ -4119,7 +4119,7 @@ nv50_disp_atomic_commit(struct drm_devic if (crtc->state->enable) { if (!drm->have_disp_power_ref) { drm->have_disp_power_ref = true; - return ret; + return 0; } active = true; break; @@ -4131,6 +4131,9 @@ nv50_disp_atomic_commit(struct drm_devic drm->have_disp_power_ref = false; } +err_cleanup: + if (ret) + drm_atomic_helper_cleanup_planes(dev, state); don...

...{ ret = drm_atomic_helper_wait_for_fences(dev, state, true); if (ret) - goto done; + goto err_cleanup; } for_each_plane_in_state(state, plane, plane_state, i) { @@ -4148,7 +4148,7 @@ nv50_disp_atomic_commit(struct drm_device *dev, if (crtc->state->enable) { if (!drm->have_disp_power_ref) { drm->have_disp_power_ref = true; - return ret; + return 0; } active = true; break; @@ -4160,6 +4160,9 @@ nv50_disp_atomic_commit(struct drm_device *dev, drm->have_disp_power_ref = false; } +err_cleanup: + if (ret) + drm_atomic_helper_cleanup_planes(dev, state...

...eau/dispnv50/disp.c +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c @@ -1878,7 +1878,7 @@ nv50_disp_atomic_commit(struct drm_device *dev, nv50_disp_atomic_commit_tail(state); drm_for_each_crtc(crtc, dev) { - if (crtc->state->enable) { + if (crtc->state->active) { if (!drm->have_disp_power_ref) { drm->have_disp_power_ref = true; return 0; -- 2.17.1

...uveau/nv50_display.c +++ b/drivers/gpu/drm/nouveau/nv50_display.c @@ -4198,7 +4198,7 @@ nv50_disp_atomic_commit(struct drm_device *dev, nv50_disp_atomic_commit_tail(state); drm_for_each_crtc(crtc, dev) { - if (crtc->state->enable) { + if (crtc->state->active) { if (!drm->have_disp_power_ref) { drm->have_disp_power_ref = true; return 0; -- 2.17.1

...y 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 kee...

One very easy to trigger runtime PM leak, along with a rare never before seen runtime PM leak! Lyude Paul (2): drm/nouveau: Fix runtime PM leak in drm_open() drm/nouveau: Fix runtime PM leak in nv50_disp_atomic_commit() drivers/gpu/drm/nouveau/dispnv50/disp.c | 2 +- drivers/gpu/drm/nouveau/nouveau_drm.c | 6 ++++-- 2 files changed, 5 insertions(+), 3 deletions(-) -- 2.17.1

...a drm_crtc structure is destroyed with drm_crtc_cleanup(), the DRM > core does not turn off the crtc first and neither do the drivers. With > nouveau, radeon and amdgpu, this causes a runtime pm ref to be leaked on > driver unload if at least one crtc was enabled. > > (See usage of have_disp_power_ref in nouveau_crtc_set_config(), > radeon_crtc_set_config() and amdgpu_crtc_set_config()). > > Fixes: 5addcf0a5f0f ("nouveau: add runtime PM support (v0.9)") > Cc: Dave Airlie <airlied at redhat.com> > Tested-by: Karol Herbst <nouveau at karolherbst.de> > Signe...

...crtc_cleanup(), the DRM >> > core does not turn off the crtc first and neither do the drivers. With >> > nouveau, radeon and amdgpu, this causes a runtime pm ref to be leaked on >> > driver unload if at least one crtc was enabled. >> > >> > (See usage of have_disp_power_ref in nouveau_crtc_set_config(), >> > radeon_crtc_set_config() and amdgpu_crtc_set_config()). >> > >> > Fixes: 5addcf0a5f0f ("nouveau: add runtime PM support (v0.9)") >> > Cc: Dave Airlie <airlied at redhat.com> >> > Tested-by: Karol Herbst...

...re does not turn off the crtc first and neither do the drivers. With > > >> > nouveau, radeon and amdgpu, this causes a runtime pm ref to be leaked on > > >> > driver unload if at least one crtc was enabled. > > >> > > > >> > (See usage of have_disp_power_ref in nouveau_crtc_set_config(), > > >> > radeon_crtc_set_config() and amdgpu_crtc_set_config()). > > >> > > > >> > Fixes: 5addcf0a5f0f ("nouveau: add runtime PM support (v0.9)") > > >> > Cc: Dave Airlie <airlied at redhat.com&gt...

...her do the drivers. With > > > > > > > nouveau, radeon and amdgpu, this causes a runtime pm ref to be leaked on > > > > > > > driver unload if at least one crtc was enabled. > > > > > > > > > > > > > > (See usage of have_disp_power_ref in nouveau_crtc_set_config(), > > > > > > > radeon_crtc_set_config() and amdgpu_crtc_set_config()). > > > > > > > > > > > > > > Fixes: 5addcf0a5f0f ("nouveau: add runtime PM support (v0.9)") > > > > > > >...

...; diff --git a/drivers/gpu/drm/nouveau/nouveau_drv.h b/drivers/gpu/drm/nouveau/nouveau_drv.h > index 0b2191fa96f7..e8d4203ddfb4 100644 > --- a/drivers/gpu/drm/nouveau/nouveau_drv.h > +++ b/drivers/gpu/drm/nouveau/nouveau_drv.h > @@ -212,6 +212,8 @@ struct nouveau_drm { > bool have_disp_power_ref; > > struct dev_pm_domain vga_pm_domain; > + > + struct task_struct *rpm_task; > }; > > static inline struct nouveau_drm * > @@ -231,6 +233,12 @@ int nouveau_pmops_suspend(struct device *); > int nouveau_pmops_resume(struct device *); > bool nouveau_p...

In preparation for runtime pm on muxed dual GPU laptops, I've fixed all runtime pm ref leaks I could find in nouveau, radeon and amdgpu. To ease reviewing, I've pushed this series to GitHub: https://github.com/l1k/linux/commits/drm_runpm_fixes_v1 @Alex Deucher: I do not have an AMD GPU so couldn't test this beyond verifying that it compiles. Please double-check the patches and test