| Message ID | 20170522135945.28831-1-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
2017-05-22 16:59 GMT+03:00 Daniel Vetter <daniel.vetter@ffwll.ch>: > I failed to properly onion-wrap the unwind code: We acquire the vblank > reference before we start with the wait-wound locking dance, hence we > must make sure we retry before we drop the reference. Oops. > > v2: The vblank_put must be after the frambuffer_put (Michel). I suck at > unwrapping code that doesn't use separate labels for each stage, but > checks each pointer first ... While re-reading everything I also > realized that we must clean up the fb refcounts, and specifically > plane->old_fb before we drop the locks, either in the final unlocking, > or in the w/w retry path. Hence the correct fix is to drop the > vblank_put to the very bottom. > > Fixes: 29dc0d1de182 ("drm: Roll out acquire context for the page_flip ioctl") > Cc: Harry Wentland <harry.wentland@amd.com> > Cc: Daniel Vetter <daniel.vetter@intel.com> > Cc: Jani Nikula <jani.nikula@linux.intel.com> > Cc: Sean Paul <seanpaul@chromium.org> > Cc: David Airlie <airlied@linux.ie> > Cc: dri-devel@lists.freedesktop.org > Reported-by: Tommi Rantala <tt.rantala@gmail.com> > Cc: Tommi Rantala <tt.rantala@gmail.com> > Cc: Michel Dänzer <michel@daenzer.net> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> Thanks, confirmed that this fixes the warning that I was seeing! Tested-by: Tommi Rantala <tt.rantala@gmail.com> > --- > drivers/gpu/drm/drm_plane.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c > index fedd4d60d9cd..5dc8c4350602 100644 > --- a/drivers/gpu/drm/drm_plane.c > +++ b/drivers/gpu/drm/drm_plane.c > @@ -948,8 +948,6 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev, > } > > out: > - if (ret && crtc->funcs->page_flip_target) > - drm_crtc_vblank_put(crtc); > if (fb) > drm_framebuffer_put(fb); > if (crtc->primary->old_fb) > @@ -964,5 +962,8 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev, > drm_modeset_drop_locks(&ctx); > drm_modeset_acquire_fini(&ctx); > > + if (ret && crtc->funcs->page_flip_target) > + drm_crtc_vblank_put(crtc); > + > return ret; > } > -- > 2.11.0 >
On 22/05/17 10:59 PM, Daniel Vetter wrote: > I failed to properly onion-wrap the unwind code: We acquire the vblank > reference before we start with the wait-wound locking dance, hence we > must make sure we retry before we drop the reference. Oops. > > v2: The vblank_put must be after the frambuffer_put (Michel). I suck at > unwrapping code that doesn't use separate labels for each stage, but > checks each pointer first ... While re-reading everything I also > realized that we must clean up the fb refcounts, and specifically > plane->old_fb before we drop the locks, either in the final unlocking, > or in the w/w retry path. Hence the correct fix is to drop the > vblank_put to the very bottom. > > Fixes: 29dc0d1de182 ("drm: Roll out acquire context for the page_flip ioctl") > Cc: Harry Wentland <harry.wentland@amd.com> > Cc: Daniel Vetter <daniel.vetter@intel.com> > Cc: Jani Nikula <jani.nikula@linux.intel.com> > Cc: Sean Paul <seanpaul@chromium.org> > Cc: David Airlie <airlied@linux.ie> > Cc: dri-devel@lists.freedesktop.org > Reported-by: Tommi Rantala <tt.rantala@gmail.com> > Cc: Tommi Rantala <tt.rantala@gmail.com> > Cc: Michel Dänzer <michel@daenzer.net> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > --- > drivers/gpu/drm/drm_plane.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c > index fedd4d60d9cd..5dc8c4350602 100644 > --- a/drivers/gpu/drm/drm_plane.c > +++ b/drivers/gpu/drm/drm_plane.c > @@ -948,8 +948,6 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev, > } > > out: > - if (ret && crtc->funcs->page_flip_target) > - drm_crtc_vblank_put(crtc); > if (fb) > drm_framebuffer_put(fb); > if (crtc->primary->old_fb) > @@ -964,5 +962,8 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev, > drm_modeset_drop_locks(&ctx); > drm_modeset_acquire_fini(&ctx); > > + if (ret && crtc->funcs->page_flip_target) > + drm_crtc_vblank_put(crtc); > + > return ret; > } > Reviewed-by: Michel Dänzer <michel.daenzer@amd.com>
diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c index fedd4d60d9cd..5dc8c4350602 100644 --- a/drivers/gpu/drm/drm_plane.c +++ b/drivers/gpu/drm/drm_plane.c @@ -948,8 +948,6 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev, } out: - if (ret && crtc->funcs->page_flip_target) - drm_crtc_vblank_put(crtc); if (fb) drm_framebuffer_put(fb); if (crtc->primary->old_fb) @@ -964,5 +962,8 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev, drm_modeset_drop_locks(&ctx); drm_modeset_acquire_fini(&ctx); + if (ret && crtc->funcs->page_flip_target) + drm_crtc_vblank_put(crtc); + return ret; }
I failed to properly onion-wrap the unwind code: We acquire the vblank reference before we start with the wait-wound locking dance, hence we must make sure we retry before we drop the reference. Oops. v2: The vblank_put must be after the frambuffer_put (Michel). I suck at unwrapping code that doesn't use separate labels for each stage, but checks each pointer first ... While re-reading everything I also realized that we must clean up the fb refcounts, and specifically plane->old_fb before we drop the locks, either in the final unlocking, or in the w/w retry path. Hence the correct fix is to drop the vblank_put to the very bottom. Fixes: 29dc0d1de182 ("drm: Roll out acquire context for the page_flip ioctl") Cc: Harry Wentland <harry.wentland@amd.com> Cc: Daniel Vetter <daniel.vetter@intel.com> Cc: Jani Nikula <jani.nikula@linux.intel.com> Cc: Sean Paul <seanpaul@chromium.org> Cc: David Airlie <airlied@linux.ie> Cc: dri-devel@lists.freedesktop.org Reported-by: Tommi Rantala <tt.rantala@gmail.com> Cc: Tommi Rantala <tt.rantala@gmail.com> Cc: Michel Dänzer <michel@daenzer.net> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> --- drivers/gpu/drm/drm_plane.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)