| Message ID | 1410534452-22565-1-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Sep 12, 2014 at 05:07:32PM +0200, Daniel Vetter wrote: > Bunch of things amiss: > - Updating crtc->cursor_x/y was done without any locking. Spotted by > David Herrmann. > - Dereferencing crtc->cursor->fb was using the wrong lock, should take > the crtc lock. > - Grabbing _all_ modeset locks torpedoes the reason why we added > fine-grained locks originally: Cursor updates shouldn't stall on > background stuff like probing outputs. > > Best is to just grab the crtc lock around everything and drop all the > other locking. The only issue is that we can't switch planes between > crtcs with that, so make sure that never happens when someone uses > universal plane helpers. This shouldn't be a possible regression ever > since legacy ioctls also only grabbed the crtc lock, so switching > crtcs was never possible for the underlying plane object. And i915 > (the only user of universal cursors thus far) has fixed cursor->crtc > links. > > Cc: David Herrmann <dh.herrmann@gmail.com> > Cc: Pallavi G<pallavi.g@intel.com> > Cc: Matt Roper <matthew.d.roper@intel.com> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > > -- > Totally untested, but should aim in the right direction. Roughly. > > Cheers, Daniel Looks good to me. I also ran this through the igt tests kms_plane, kms_universal_plane, and kms_cursor_crc to make sure that we didn't accidentally break something and they all ran fine. So Reviewed-by: Matt Roper <matthew.d.roper@intel.com> and Tested-by: Matt Roper <matthew.d.roper@intel.com> > --- > drivers/gpu/drm/drm_crtc.c | 51 ++++++++++++++++++++++++++++++---------------- > 1 file changed, 34 insertions(+), 17 deletions(-) > > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c > index b1271a8d8ce7..8c1870114a07 100644 > --- a/drivers/gpu/drm/drm_crtc.c > +++ b/drivers/gpu/drm/drm_crtc.c > @@ -2262,21 +2262,19 @@ out: > * > * src_{x,y,w,h} are provided in 16.16 fixed point format > */ > -static int setplane_internal(struct drm_plane *plane, > - struct drm_crtc *crtc, > - struct drm_framebuffer *fb, > - int32_t crtc_x, int32_t crtc_y, > - uint32_t crtc_w, uint32_t crtc_h, > - /* src_{x,y,w,h} values are 16.16 fixed point */ > - uint32_t src_x, uint32_t src_y, > - uint32_t src_w, uint32_t src_h) > +static int __setplane_internal(struct drm_plane *plane, > + struct drm_crtc *crtc, > + struct drm_framebuffer *fb, > + int32_t crtc_x, int32_t crtc_y, > + uint32_t crtc_w, uint32_t crtc_h, > + /* src_{x,y,w,h} values are 16.16 fixed point */ > + uint32_t src_x, uint32_t src_y, > + uint32_t src_w, uint32_t src_h) > { > - struct drm_device *dev = plane->dev; > int ret = 0; > unsigned int fb_width, fb_height; > int i; > > - drm_modeset_lock_all(dev); > /* No fb means shut it down */ > if (!fb) { > plane->old_fb = plane->fb; > @@ -2344,10 +2342,28 @@ out: > if (plane->old_fb) > drm_framebuffer_unreference(plane->old_fb); > plane->old_fb = NULL; > - drm_modeset_unlock_all(dev); > > return ret; > +} > > +static int setplane_internal(struct drm_plane *plane, > + struct drm_crtc *crtc, > + struct drm_framebuffer *fb, > + int32_t crtc_x, int32_t crtc_y, > + uint32_t crtc_w, uint32_t crtc_h, > + /* src_{x,y,w,h} values are 16.16 fixed point */ > + uint32_t src_x, uint32_t src_y, > + uint32_t src_w, uint32_t src_h) > +{ > + int ret; > + > + drm_modeset_lock_all(plane->dev); > + ret = __setplane_internal(plane, crtc, fb, > + crtc_x, crtc_y, crtc_w, crtc_h, > + src_x, src_y, src_w, src_h); > + drm_modeset_unlock_all(plane->dev); > + > + return ret; > } > > /** > @@ -2713,6 +2729,7 @@ static int drm_mode_cursor_universal(struct drm_crtc *crtc, > int ret = 0; > > BUG_ON(!crtc->cursor); > + WARN_ON(crtc->cursor->crtc != crtc && crtc->cursor->crtc != NULL); > > /* > * Obtain fb we'll be using (either new or existing) and take an extra > @@ -2732,11 +2749,9 @@ static int drm_mode_cursor_universal(struct drm_crtc *crtc, > fb = NULL; > } > } else { > - mutex_lock(&dev->mode_config.mutex); > fb = crtc->cursor->fb; > if (fb) > drm_framebuffer_reference(fb); > - mutex_unlock(&dev->mode_config.mutex); > } > > if (req->flags & DRM_MODE_CURSOR_MOVE) { > @@ -2758,7 +2773,7 @@ static int drm_mode_cursor_universal(struct drm_crtc *crtc, > * setplane_internal will take care of deref'ing either the old or new > * framebuffer depending on success. > */ > - ret = setplane_internal(crtc->cursor, crtc, fb, > + ret = __setplane_internal(crtc->cursor, crtc, fb, > crtc_x, crtc_y, crtc_w, crtc_h, > 0, 0, src_w, src_h); > > @@ -2794,10 +2809,12 @@ static int drm_mode_cursor_common(struct drm_device *dev, > * If this crtc has a universal cursor plane, call that plane's update > * handler rather than using legacy cursor handlers. > */ > - if (crtc->cursor) > - return drm_mode_cursor_universal(crtc, req, file_priv); > - > drm_modeset_lock_crtc(crtc); > + if (crtc->cursor) { > + ret = drm_mode_cursor_universal(crtc, req, file_priv); > + goto out; > + } > + > if (req->flags & DRM_MODE_CURSOR_BO) { > if (!crtc->funcs->cursor_set && !crtc->funcs->cursor_set2) { > ret = -ENXIO; > -- > 2.0.1 >
Hi On Fri, Sep 12, 2014 at 5:07 PM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > Bunch of things amiss: > - Updating crtc->cursor_x/y was done without any locking. Spotted by > David Herrmann. > - Dereferencing crtc->cursor->fb was using the wrong lock, should take > the crtc lock. > - Grabbing _all_ modeset locks torpedoes the reason why we added > fine-grained locks originally: Cursor updates shouldn't stall on > background stuff like probing outputs. > > Best is to just grab the crtc lock around everything and drop all the > other locking. The only issue is that we can't switch planes between > crtcs with that, so make sure that never happens when someone uses > universal plane helpers. This shouldn't be a possible regression ever > since legacy ioctls also only grabbed the crtc lock, so switching > crtcs was never possible for the underlying plane object. And i915 > (the only user of universal cursors thus far) has fixed cursor->crtc > links. > > Cc: David Herrmann <dh.herrmann@gmail.com> > Cc: Pallavi G<pallavi.g@intel.com> > Cc: Matt Roper <matthew.d.roper@intel.com> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > > -- > Totally untested, but should aim in the right direction. Roughly. > > Cheers, Daniel > --- > drivers/gpu/drm/drm_crtc.c | 51 ++++++++++++++++++++++++++++++---------------- > 1 file changed, 34 insertions(+), 17 deletions(-) > > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c > index b1271a8d8ce7..8c1870114a07 100644 > --- a/drivers/gpu/drm/drm_crtc.c > +++ b/drivers/gpu/drm/drm_crtc.c > @@ -2262,21 +2262,19 @@ out: > * > * src_{x,y,w,h} are provided in 16.16 fixed point format > */ > -static int setplane_internal(struct drm_plane *plane, > - struct drm_crtc *crtc, > - struct drm_framebuffer *fb, > - int32_t crtc_x, int32_t crtc_y, > - uint32_t crtc_w, uint32_t crtc_h, > - /* src_{x,y,w,h} values are 16.16 fixed point */ > - uint32_t src_x, uint32_t src_y, > - uint32_t src_w, uint32_t src_h) > +static int __setplane_internal(struct drm_plane *plane, > + struct drm_crtc *crtc, > + struct drm_framebuffer *fb, > + int32_t crtc_x, int32_t crtc_y, > + uint32_t crtc_w, uint32_t crtc_h, > + /* src_{x,y,w,h} values are 16.16 fixed point */ > + uint32_t src_x, uint32_t src_y, > + uint32_t src_w, uint32_t src_h) > { > - struct drm_device *dev = plane->dev; > int ret = 0; > unsigned int fb_width, fb_height; > int i; > > - drm_modeset_lock_all(dev); > /* No fb means shut it down */ > if (!fb) { > plane->old_fb = plane->fb; > @@ -2344,10 +2342,28 @@ out: > if (plane->old_fb) > drm_framebuffer_unreference(plane->old_fb); > plane->old_fb = NULL; > - drm_modeset_unlock_all(dev); > > return ret; > +} > > +static int setplane_internal(struct drm_plane *plane, > + struct drm_crtc *crtc, > + struct drm_framebuffer *fb, > + int32_t crtc_x, int32_t crtc_y, > + uint32_t crtc_w, uint32_t crtc_h, > + /* src_{x,y,w,h} values are 16.16 fixed point */ > + uint32_t src_x, uint32_t src_y, > + uint32_t src_w, uint32_t src_h) > +{ > + int ret; > + > + drm_modeset_lock_all(plane->dev); > + ret = __setplane_internal(plane, crtc, fb, > + crtc_x, crtc_y, crtc_w, crtc_h, > + src_x, src_y, src_w, src_h); > + drm_modeset_unlock_all(plane->dev); Might be confusing to call into drivers with different locks held depending on the plane. But on the other hand, I think drivers should just assume only the involved CRTCs are locked, so it sounds fine. With atomic we get the same behavior, presumably. Reviewed-by: David Herrmann <dh.herrmann@gmail.com> Thanks David > + > + return ret; > } > > /** > @@ -2713,6 +2729,7 @@ static int drm_mode_cursor_universal(struct drm_crtc *crtc, > int ret = 0; > > BUG_ON(!crtc->cursor); > + WARN_ON(crtc->cursor->crtc != crtc && crtc->cursor->crtc != NULL); > > /* > * Obtain fb we'll be using (either new or existing) and take an extra > @@ -2732,11 +2749,9 @@ static int drm_mode_cursor_universal(struct drm_crtc *crtc, > fb = NULL; > } > } else { > - mutex_lock(&dev->mode_config.mutex); > fb = crtc->cursor->fb; > if (fb) > drm_framebuffer_reference(fb); > - mutex_unlock(&dev->mode_config.mutex); > } > > if (req->flags & DRM_MODE_CURSOR_MOVE) { > @@ -2758,7 +2773,7 @@ static int drm_mode_cursor_universal(struct drm_crtc *crtc, > * setplane_internal will take care of deref'ing either the old or new > * framebuffer depending on success. > */ > - ret = setplane_internal(crtc->cursor, crtc, fb, > + ret = __setplane_internal(crtc->cursor, crtc, fb, > crtc_x, crtc_y, crtc_w, crtc_h, > 0, 0, src_w, src_h); > > @@ -2794,10 +2809,12 @@ static int drm_mode_cursor_common(struct drm_device *dev, > * If this crtc has a universal cursor plane, call that plane's update > * handler rather than using legacy cursor handlers. > */ > - if (crtc->cursor) > - return drm_mode_cursor_universal(crtc, req, file_priv); > - > drm_modeset_lock_crtc(crtc); > + if (crtc->cursor) { > + ret = drm_mode_cursor_universal(crtc, req, file_priv); > + goto out; > + } > + > if (req->flags & DRM_MODE_CURSOR_BO) { > if (!crtc->funcs->cursor_set && !crtc->funcs->cursor_set2) { > ret = -ENXIO; > -- > 2.0.1 >
On Sun, Sep 14, 2014 at 5:57 PM, David Herrmann <dh.herrmann@gmail.com> wrote: > Might be confusing to call into drivers with different locks held > depending on the plane. But on the other hand, I think drivers should > just assume only the involved CRTCs are locked, so it sounds fine. > With atomic we get the same behavior, presumably. Yeah, this will actually more closely resemeble atomic, where we only grab crtc locks for plane updates (both old&new crtc for each plane). -Daniel
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index b1271a8d8ce7..8c1870114a07 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -2262,21 +2262,19 @@ out: * * src_{x,y,w,h} are provided in 16.16 fixed point format */ -static int setplane_internal(struct drm_plane *plane, - struct drm_crtc *crtc, - struct drm_framebuffer *fb, - int32_t crtc_x, int32_t crtc_y, - uint32_t crtc_w, uint32_t crtc_h, - /* src_{x,y,w,h} values are 16.16 fixed point */ - uint32_t src_x, uint32_t src_y, - uint32_t src_w, uint32_t src_h) +static int __setplane_internal(struct drm_plane *plane, + struct drm_crtc *crtc, + struct drm_framebuffer *fb, + int32_t crtc_x, int32_t crtc_y, + uint32_t crtc_w, uint32_t crtc_h, + /* src_{x,y,w,h} values are 16.16 fixed point */ + uint32_t src_x, uint32_t src_y, + uint32_t src_w, uint32_t src_h) { - struct drm_device *dev = plane->dev; int ret = 0; unsigned int fb_width, fb_height; int i; - drm_modeset_lock_all(dev); /* No fb means shut it down */ if (!fb) { plane->old_fb = plane->fb; @@ -2344,10 +2342,28 @@ out: if (plane->old_fb) drm_framebuffer_unreference(plane->old_fb); plane->old_fb = NULL; - drm_modeset_unlock_all(dev); return ret; +} +static int setplane_internal(struct drm_plane *plane, + struct drm_crtc *crtc, + struct drm_framebuffer *fb, + int32_t crtc_x, int32_t crtc_y, + uint32_t crtc_w, uint32_t crtc_h, + /* src_{x,y,w,h} values are 16.16 fixed point */ + uint32_t src_x, uint32_t src_y, + uint32_t src_w, uint32_t src_h) +{ + int ret; + + drm_modeset_lock_all(plane->dev); + ret = __setplane_internal(plane, crtc, fb, + crtc_x, crtc_y, crtc_w, crtc_h, + src_x, src_y, src_w, src_h); + drm_modeset_unlock_all(plane->dev); + + return ret; } /** @@ -2713,6 +2729,7 @@ static int drm_mode_cursor_universal(struct drm_crtc *crtc, int ret = 0; BUG_ON(!crtc->cursor); + WARN_ON(crtc->cursor->crtc != crtc && crtc->cursor->crtc != NULL); /* * Obtain fb we'll be using (either new or existing) and take an extra @@ -2732,11 +2749,9 @@ static int drm_mode_cursor_universal(struct drm_crtc *crtc, fb = NULL; } } else { - mutex_lock(&dev->mode_config.mutex); fb = crtc->cursor->fb; if (fb) drm_framebuffer_reference(fb); - mutex_unlock(&dev->mode_config.mutex); } if (req->flags & DRM_MODE_CURSOR_MOVE) { @@ -2758,7 +2773,7 @@ static int drm_mode_cursor_universal(struct drm_crtc *crtc, * setplane_internal will take care of deref'ing either the old or new * framebuffer depending on success. */ - ret = setplane_internal(crtc->cursor, crtc, fb, + ret = __setplane_internal(crtc->cursor, crtc, fb, crtc_x, crtc_y, crtc_w, crtc_h, 0, 0, src_w, src_h); @@ -2794,10 +2809,12 @@ static int drm_mode_cursor_common(struct drm_device *dev, * If this crtc has a universal cursor plane, call that plane's update * handler rather than using legacy cursor handlers. */ - if (crtc->cursor) - return drm_mode_cursor_universal(crtc, req, file_priv); - drm_modeset_lock_crtc(crtc); + if (crtc->cursor) { + ret = drm_mode_cursor_universal(crtc, req, file_priv); + goto out; + } + if (req->flags & DRM_MODE_CURSOR_BO) { if (!crtc->funcs->cursor_set && !crtc->funcs->cursor_set2) { ret = -ENXIO;
Bunch of things amiss: - Updating crtc->cursor_x/y was done without any locking. Spotted by David Herrmann. - Dereferencing crtc->cursor->fb was using the wrong lock, should take the crtc lock. - Grabbing _all_ modeset locks torpedoes the reason why we added fine-grained locks originally: Cursor updates shouldn't stall on background stuff like probing outputs. Best is to just grab the crtc lock around everything and drop all the other locking. The only issue is that we can't switch planes between crtcs with that, so make sure that never happens when someone uses universal plane helpers. This shouldn't be a possible regression ever since legacy ioctls also only grabbed the crtc lock, so switching crtcs was never possible for the underlying plane object. And i915 (the only user of universal cursors thus far) has fixed cursor->crtc links. Cc: David Herrmann <dh.herrmann@gmail.com> Cc: Pallavi G<pallavi.g@intel.com> Cc: Matt Roper <matthew.d.roper@intel.com> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> -- Totally untested, but should aim in the right direction. Roughly. Cheers, Daniel --- drivers/gpu/drm/drm_crtc.c | 51 ++++++++++++++++++++++++++++++---------------- 1 file changed, 34 insertions(+), 17 deletions(-)