* [PATCH] drm: Fixup locking for universal cursor planes
@ 2014-09-12 15:07 Daniel Vetter
2014-09-12 17:48 ` Matt Roper
2014-09-14 15:57 ` David Herrmann
0 siblings, 2 replies; 5+ messages in thread
From: Daniel Vetter @ 2014-09-12 15:07 UTC (permalink / raw)
To: Intel Graphics Development; +Cc: Daniel Vetter, Daniel Vetter, David Herrmann
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);
+
+ 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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] drm: Fixup locking for universal cursor planes
2014-09-12 15:07 [PATCH] drm: Fixup locking for universal cursor planes Daniel Vetter
@ 2014-09-12 17:48 ` Matt Roper
2014-09-14 15:57 ` David Herrmann
1 sibling, 0 replies; 5+ messages in thread
From: Matt Roper @ 2014-09-12 17:48 UTC (permalink / raw)
To: Daniel Vetter; +Cc: Daniel Vetter, Intel Graphics Development, David Herrmann
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
>
--
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] drm: Fixup locking for universal cursor planes
2014-09-12 15:07 [PATCH] drm: Fixup locking for universal cursor planes Daniel Vetter
2014-09-12 17:48 ` Matt Roper
@ 2014-09-14 15:57 ` David Herrmann
2014-09-15 6:57 ` Daniel Vetter
1 sibling, 1 reply; 5+ messages in thread
From: David Herrmann @ 2014-09-14 15:57 UTC (permalink / raw)
To: Daniel Vetter; +Cc: Daniel Vetter, Intel Graphics Development
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
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] drm: Fixup locking for universal cursor planes
2014-09-14 15:57 ` David Herrmann
@ 2014-09-15 6:57 ` Daniel Vetter
0 siblings, 0 replies; 5+ messages in thread
From: Daniel Vetter @ 2014-09-15 6:57 UTC (permalink / raw)
To: David Herrmann; +Cc: Daniel Vetter, Intel Graphics Development
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
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH] drm: Fixup locking for universal cursor planes
@ 2014-09-15 11:44 Daniel Vetter
0 siblings, 0 replies; 5+ messages in thread
From: Daniel Vetter @ 2014-09-15 11:44 UTC (permalink / raw)
To: DRI Development; +Cc: Daniel Vetter, Daniel Vetter, Pallavi G
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>
Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
Tested-by: Matt Roper <matthew.d.roper@intel.com>
Reviewed-by: David Herrmann <dh.herrmann@gmail.com>
Signed-off-by: Daniel Vetter <daniel.vetter@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 2ec08e9269bd..4b8a262a8eb2 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
^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2014-09-15 11:43 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-09-12 15:07 [PATCH] drm: Fixup locking for universal cursor planes Daniel Vetter
2014-09-12 17:48 ` Matt Roper
2014-09-14 15:57 ` David Herrmann
2014-09-15 6:57 ` Daniel Vetter
-- strict thread matches above, loose matches on Subject: below --
2014-09-15 11:44 Daniel Vetter
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.