Hi all,
This is something I kinda had on my todo list ever since atomic landed. The legacy_backoff() hack really doesn't work if you need to acquire additional locks, which does restrict drivers in how they handle and protect legacy paths, and kinda forces us to be overzealous with taking locks for legacy paths, just in case.
This patch set here fixes this, with 2 huge exceptions: - get/set_property calls aren't fixed. The locking in there is a mess and needs some serious attention. My goal would be that for atomic we take no lock at all, and entirely rely upon the magic of drm_modeset_lock and atomic to just grab the minimal set required to update a property.
- fbdev emulation helpers. It abused the modeset_lock_all bkl as its own lock, which prevents us from pushing it down just around the (atomic) modeset calls, and hence from switching over to handling the acquire context in an explicit fashion. Thierry started to fix this with the addition of proper locking for fbdev emulation, but it needs a pile more work.
Survived light testing with full ww mutex debugging, I'll rely on CI to catch the remaining mixups :-)
Cheers, Daniel
Daniel Vetter (19): drm: Wire up proper acquire ctx for plane functions drm: Add acquire ctx parameter to ->update_plane drm: drm_plane_force_disable is not for atomic drivers drm: Add acquire ctx parameter to ->plane_disable drm/atomic-helper: remove backoff hack from disable/update_plane drm/vmwgfx: Drop the cursor locking hack drm/tegra: Don't use modeset_lock_crtc drm/tilcdc: Drop calls to modeset_lock_crtc drm: Make drm_modeset_lock_crtc internal drm: Roll out acquire context for the page_flip ioctl drm: Add acquire ctx parameter to ->page_flip(_target) drm/atomic-helper: remove backoff hack from page_flip drm: simplify the locking in the GETCRTC ioctl drm: Remove drm_modeset_(un)lock_crtc drm: Remove drm_modeset_legacy_acquire_ctx and crtc->acquire_ctx drm: Restrict drm_mode_set_config_internal to non-atomic drivers drm: Add explicit acquire ctx handling around ->set_config drm: Add acquire ctx parameter to ->set_config drm/atomic-helper: Remove the backoff hack from set_config
drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 8 +- drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h | 6 +- drivers/gpu/drm/armada/armada_crtc.c | 3 +- drivers/gpu/drm/armada/armada_overlay.c | 6 +- drivers/gpu/drm/bochs/bochs_kms.c | 3 +- drivers/gpu/drm/drm_atomic.c | 14 --- drivers/gpu/drm/drm_atomic_helper.c | 132 +++++----------------------- drivers/gpu/drm/drm_crtc.c | 61 ++++++++----- drivers/gpu/drm/drm_crtc_helper.c | 4 +- drivers/gpu/drm/drm_modeset_lock.c | 102 --------------------- drivers/gpu/drm/drm_plane.c | 87 ++++++++++++++---- drivers/gpu/drm/drm_plane_helper.c | 11 ++- drivers/gpu/drm/gma500/gma_display.c | 7 +- drivers/gpu/drm/gma500/gma_display.h | 3 +- drivers/gpu/drm/i915/intel_display.c | 9 +- drivers/gpu/drm/i915/intel_pipe_crc.c | 2 +- drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c | 8 +- drivers/gpu/drm/nouveau/dispnv04/crtc.c | 7 +- drivers/gpu/drm/nouveau/dispnv04/overlay.c | 18 ++-- drivers/gpu/drm/nouveau/nouveau_display.c | 3 +- drivers/gpu/drm/nouveau/nouveau_display.h | 4 +- drivers/gpu/drm/radeon/radeon_display.c | 8 +- drivers/gpu/drm/shmobile/shmob_drm_crtc.c | 3 +- drivers/gpu/drm/shmobile/shmob_drm_plane.c | 8 +- drivers/gpu/drm/tegra/dc.c | 8 +- drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 12 +-- drivers/gpu/drm/udl/udl_modeset.c | 3 +- drivers/gpu/drm/vc4/vc4_crtc.c | 5 +- drivers/gpu/drm/vc4/vc4_plane.c | 6 +- drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 25 ------ drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c | 3 +- drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c | 6 +- drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c | 6 +- include/drm/drm_atomic_helper.h | 15 ++-- include/drm/drm_crtc.h | 18 ++-- include/drm/drm_crtc_helper.h | 3 +- include/drm/drm_modeset_lock.h | 5 -- include/drm/drm_plane.h | 7 +- include/drm/drm_plane_helper.h | 6 +- 39 files changed, 265 insertions(+), 380 deletions(-)
This is just prep work to get an acquire ctx into every place where we call ->update_plane or ->disable_plane.
v2: Keep the hidden acquire_ctx pointers valid while transitioning.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/drm_plane.c | 52 ++++++++++++++++++++++++++++++++++++--------- 1 file changed, 42 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c index a22e76837065..0d04888172a7 100644 --- a/drivers/gpu/drm/drm_plane.c +++ b/drivers/gpu/drm/drm_plane.c @@ -457,7 +457,8 @@ static int __setplane_internal(struct drm_plane *plane, 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) + uint32_t src_w, uint32_t src_h, + struct drm_modeset_acquire_ctx *ctx) { int ret = 0;
@@ -537,13 +538,26 @@ static int setplane_internal(struct drm_plane *plane, uint32_t src_x, uint32_t src_y, uint32_t src_w, uint32_t src_h) { + struct drm_modeset_acquire_ctx ctx; int ret;
- drm_modeset_lock_all(plane->dev); + drm_modeset_acquire_init(&ctx, 0); +retry: + ret = drm_modeset_lock_all_ctx(plane->dev, &ctx); + if (ret) + goto fail; + plane->dev->mode_config.acquire_ctx = &ctx; 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); + src_x, src_y, src_w, src_h, &ctx); + +fail: + if (ret == -EDEADLK) { + drm_modeset_backoff(&ctx); + goto retry; + } + drm_modeset_drop_locks(&ctx); + drm_modeset_acquire_fini(&ctx);
return ret; } @@ -613,11 +627,22 @@ static int drm_mode_cursor_universal(struct drm_crtc *crtc, int32_t crtc_x, crtc_y; uint32_t crtc_w = 0, crtc_h = 0; uint32_t src_w = 0, src_h = 0; + struct drm_modeset_acquire_ctx ctx; int ret = 0;
BUG_ON(!crtc->cursor); WARN_ON(crtc->cursor->crtc != crtc && crtc->cursor->crtc != NULL);
+ drm_modeset_acquire_init(&ctx, 0); +retry: + ret = drm_modeset_lock(&crtc->mutex, &ctx); + if (ret) + goto fail; + ret = drm_modeset_lock(&crtc->cursor->mutex, &ctx); + if (ret) + goto fail; + crtc->acquire_ctx = &ctx; + /* * Obtain fb we'll be using (either new or existing) and take an extra * reference to it if fb != null. setplane will take care of dropping @@ -662,7 +687,7 @@ static int drm_mode_cursor_universal(struct drm_crtc *crtc, */ ret = __setplane_internal(crtc->cursor, crtc, fb, crtc_x, crtc_y, crtc_w, crtc_h, - 0, 0, src_w, src_h); + 0, 0, src_w, src_h, &ctx);
/* Update successful; save new cursor position, if necessary */ if (ret == 0 && req->flags & DRM_MODE_CURSOR_MOVE) { @@ -670,6 +695,15 @@ static int drm_mode_cursor_universal(struct drm_crtc *crtc, crtc->cursor_y = req->y; }
+fail: + if (ret == -EDEADLK) { + drm_modeset_backoff(&ctx); + goto retry; + } + + drm_modeset_drop_locks(&ctx); + drm_modeset_acquire_fini(&ctx); + return ret; }
@@ -696,12 +730,10 @@ 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. */ - drm_modeset_lock_crtc(crtc, crtc->cursor); - if (crtc->cursor) { - ret = drm_mode_cursor_universal(crtc, req, file_priv); - goto out; - } + if (crtc->cursor) + return drm_mode_cursor_universal(crtc, req, file_priv);
+ drm_modeset_lock_crtc(crtc, crtc->cursor); if (req->flags & DRM_MODE_CURSOR_BO) { if (!crtc->funcs->cursor_set && !crtc->funcs->cursor_set2) { ret = -ENXIO;
On Wednesday, March 22, 2017 10:50:40 PM EDT Daniel Vetter wrote:
This is just prep work to get an acquire ctx into every place where we call ->update_plane or ->disable_plane.
v2: Keep the hidden acquire_ctx pointers valid while transitioning.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com
drivers/gpu/drm/drm_plane.c | 52 ++++++++++++++++++++++++++++++++++++--------- 1 file changed, 42 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c index a22e76837065..0d04888172a7 100644 --- a/drivers/gpu/drm/drm_plane.c +++ b/drivers/gpu/drm/drm_plane.c @@ -457,7 +457,8 @@ static int __setplane_internal(struct drm_plane *plane, 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)
uint32_t src_w, uint32_t src_h,struct drm_modeset_acquire_ctx *ctx){ int ret = 0;
@@ -537,13 +538,26 @@ static int setplane_internal(struct drm_plane *plane, uint32_t src_x, uint32_t src_y, uint32_t src_w, uint32_t src_h) {
- struct drm_modeset_acquire_ctx ctx; int ret;
- drm_modeset_lock_all(plane->dev);
- drm_modeset_acquire_init(&ctx, 0);
+retry:
- ret = drm_modeset_lock_all_ctx(plane->dev, &ctx);
- if (ret)
goto fail;- plane->dev->mode_config.acquire_ctx = &ctx; 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);
src_x, src_y, src_w, src_h, &ctx);+fail:
if (ret == -EDEADLK) {
drm_modeset_backoff(&ctx);goto retry;}
drm_modeset_drop_locks(&ctx);
drm_modeset_acquire_fini(&ctx);
return ret;
} @@ -613,11 +627,22 @@ static int drm_mode_cursor_universal(struct drm_crtc *crtc, int32_t crtc_x, crtc_y; uint32_t crtc_w = 0, crtc_h = 0; uint32_t src_w = 0, src_h = 0;
struct drm_modeset_acquire_ctx ctx; int ret = 0;
BUG_ON(!crtc->cursor); WARN_ON(crtc->cursor->crtc != crtc && crtc->cursor->crtc != NULL);
drm_modeset_acquire_init(&ctx, 0);
+retry:
- ret = drm_modeset_lock(&crtc->mutex, &ctx);
- if (ret)
goto fail;- ret = drm_modeset_lock(&crtc->cursor->mutex, &ctx);
- if (ret)
goto fail;- crtc->acquire_ctx = &ctx;
- /*
- Obtain fb we'll be using (either new or existing) and take an extra
- reference to it if fb != null. setplane will take care of dropping
@@ -662,7 +687,7 @@ static int drm_mode_cursor_universal(struct drm_crtc *crtc, */ ret = __setplane_internal(crtc->cursor, crtc, fb, crtc_x, crtc_y, crtc_w, crtc_h,
0, 0, src_w, src_h);
0, 0, src_w, src_h, &ctx);/* Update successful; save new cursor position, if necessary */ if (ret == 0 && req->flags & DRM_MODE_CURSOR_MOVE) {
@@ -670,6 +695,15 @@ static int drm_mode_cursor_universal(struct drm_crtc *crtc, crtc->cursor_y = req->y; }
+fail:
- if (ret == -EDEADLK) {
drm_modeset_backoff(&ctx);goto retry;- }
- drm_modeset_drop_locks(&ctx);
- drm_modeset_acquire_fini(&ctx);
- return ret;
}
@@ -696,12 +730,10 @@ 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. */
- drm_modeset_lock_crtc(crtc, crtc->cursor);
- if (crtc->cursor) {
ret = drm_mode_cursor_universal(crtc, req, file_priv);goto out;- }
if (crtc->cursor)
return drm_mode_cursor_universal(crtc, req, file_priv);drm_modeset_lock_crtc(crtc, crtc->cursor);
Why are you moving the lock? Shouldn't it still be covering the call to drm_mode_cursor_universal?
Harry
if (req->flags & DRM_MODE_CURSOR_BO) { if (!crtc->funcs->cursor_set && !crtc->funcs->cursor_set2) { ret = -ENXIO;
On Mon, Mar 27, 2017 at 04:12:05PM -0400, Harry Wentland wrote:
On Wednesday, March 22, 2017 10:50:40 PM EDT Daniel Vetter wrote:
This is just prep work to get an acquire ctx into every place where we call ->update_plane or ->disable_plane.
v2: Keep the hidden acquire_ctx pointers valid while transitioning.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com
drivers/gpu/drm/drm_plane.c | 52 ++++++++++++++++++++++++++++++++++++--------- 1 file changed, 42 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c index a22e76837065..0d04888172a7 100644 --- a/drivers/gpu/drm/drm_plane.c +++ b/drivers/gpu/drm/drm_plane.c @@ -457,7 +457,8 @@ static int __setplane_internal(struct drm_plane *plane, 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)
uint32_t src_w, uint32_t src_h,struct drm_modeset_acquire_ctx *ctx){ int ret = 0;
@@ -537,13 +538,26 @@ static int setplane_internal(struct drm_plane *plane, uint32_t src_x, uint32_t src_y, uint32_t src_w, uint32_t src_h) {
- struct drm_modeset_acquire_ctx ctx; int ret;
- drm_modeset_lock_all(plane->dev);
- drm_modeset_acquire_init(&ctx, 0);
+retry:
- ret = drm_modeset_lock_all_ctx(plane->dev, &ctx);
- if (ret)
goto fail;- plane->dev->mode_config.acquire_ctx = &ctx; 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);
src_x, src_y, src_w, src_h, &ctx);+fail:
if (ret == -EDEADLK) {
drm_modeset_backoff(&ctx);goto retry;}
drm_modeset_drop_locks(&ctx);
drm_modeset_acquire_fini(&ctx);
return ret;
} @@ -613,11 +627,22 @@ static int drm_mode_cursor_universal(struct drm_crtc *crtc, int32_t crtc_x, crtc_y; uint32_t crtc_w = 0, crtc_h = 0; uint32_t src_w = 0, src_h = 0;
struct drm_modeset_acquire_ctx ctx; int ret = 0;
BUG_ON(!crtc->cursor); WARN_ON(crtc->cursor->crtc != crtc && crtc->cursor->crtc != NULL);
drm_modeset_acquire_init(&ctx, 0);
+retry:
- ret = drm_modeset_lock(&crtc->mutex, &ctx);
- if (ret)
goto fail;- ret = drm_modeset_lock(&crtc->cursor->mutex, &ctx);
- if (ret)
goto fail;- crtc->acquire_ctx = &ctx;
- /*
- Obtain fb we'll be using (either new or existing) and take an extra
- reference to it if fb != null. setplane will take care of dropping
@@ -662,7 +687,7 @@ static int drm_mode_cursor_universal(struct drm_crtc *crtc, */ ret = __setplane_internal(crtc->cursor, crtc, fb, crtc_x, crtc_y, crtc_w, crtc_h,
0, 0, src_w, src_h);
0, 0, src_w, src_h, &ctx);/* Update successful; save new cursor position, if necessary */ if (ret == 0 && req->flags & DRM_MODE_CURSOR_MOVE) {
@@ -670,6 +695,15 @@ static int drm_mode_cursor_universal(struct drm_crtc *crtc, crtc->cursor_y = req->y; }
+fail:
- if (ret == -EDEADLK) {
drm_modeset_backoff(&ctx);goto retry;- }
- drm_modeset_drop_locks(&ctx);
- drm_modeset_acquire_fini(&ctx);
- return ret;
}
@@ -696,12 +730,10 @@ 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. */
- drm_modeset_lock_crtc(crtc, crtc->cursor);
- if (crtc->cursor) {
ret = drm_mode_cursor_universal(crtc, req, file_priv);goto out;- }
if (crtc->cursor)
return drm_mode_cursor_universal(crtc, req, file_priv);drm_modeset_lock_crtc(crtc, crtc->cursor);
Why are you moving the lock? Shouldn't it still be covering the call to drm_mode_cursor_universal?
It would deadlock, because the new acquire ctx dance for universal planes also takes the same locks as this helper?
But I just realized that crtc->cursor is NULL here, I can simplify this some more. -Daniel
On Tue, Mar 28, 2017 at 08:23:53AM +0200, Daniel Vetter wrote:
On Mon, Mar 27, 2017 at 04:12:05PM -0400, Harry Wentland wrote:
On Wednesday, March 22, 2017 10:50:40 PM EDT Daniel Vetter wrote:
This is just prep work to get an acquire ctx into every place where we call ->update_plane or ->disable_plane.
v2: Keep the hidden acquire_ctx pointers valid while transitioning.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com
drivers/gpu/drm/drm_plane.c | 52 ++++++++++++++++++++++++++++++++++++--------- 1 file changed, 42 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c index a22e76837065..0d04888172a7 100644 --- a/drivers/gpu/drm/drm_plane.c +++ b/drivers/gpu/drm/drm_plane.c @@ -457,7 +457,8 @@ static int __setplane_internal(struct drm_plane *plane, 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)
uint32_t src_w, uint32_t src_h,struct drm_modeset_acquire_ctx *ctx){ int ret = 0;
@@ -537,13 +538,26 @@ static int setplane_internal(struct drm_plane *plane, uint32_t src_x, uint32_t src_y, uint32_t src_w, uint32_t src_h) {
- struct drm_modeset_acquire_ctx ctx; int ret;
- drm_modeset_lock_all(plane->dev);
- drm_modeset_acquire_init(&ctx, 0);
+retry:
- ret = drm_modeset_lock_all_ctx(plane->dev, &ctx);
- if (ret)
goto fail;- plane->dev->mode_config.acquire_ctx = &ctx; 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);
src_x, src_y, src_w, src_h, &ctx);+fail:
if (ret == -EDEADLK) {
drm_modeset_backoff(&ctx);goto retry;}
drm_modeset_drop_locks(&ctx);
drm_modeset_acquire_fini(&ctx);
return ret;
} @@ -613,11 +627,22 @@ static int drm_mode_cursor_universal(struct drm_crtc *crtc, int32_t crtc_x, crtc_y; uint32_t crtc_w = 0, crtc_h = 0; uint32_t src_w = 0, src_h = 0;
struct drm_modeset_acquire_ctx ctx; int ret = 0;
BUG_ON(!crtc->cursor); WARN_ON(crtc->cursor->crtc != crtc && crtc->cursor->crtc != NULL);
drm_modeset_acquire_init(&ctx, 0);
+retry:
- ret = drm_modeset_lock(&crtc->mutex, &ctx);
- if (ret)
goto fail;- ret = drm_modeset_lock(&crtc->cursor->mutex, &ctx);
- if (ret)
goto fail;- crtc->acquire_ctx = &ctx;
- /*
- Obtain fb we'll be using (either new or existing) and take an extra
- reference to it if fb != null. setplane will take care of dropping
@@ -662,7 +687,7 @@ static int drm_mode_cursor_universal(struct drm_crtc *crtc, */ ret = __setplane_internal(crtc->cursor, crtc, fb, crtc_x, crtc_y, crtc_w, crtc_h,
0, 0, src_w, src_h);
0, 0, src_w, src_h, &ctx);/* Update successful; save new cursor position, if necessary */ if (ret == 0 && req->flags & DRM_MODE_CURSOR_MOVE) {
@@ -670,6 +695,15 @@ static int drm_mode_cursor_universal(struct drm_crtc *crtc, crtc->cursor_y = req->y; }
+fail:
- if (ret == -EDEADLK) {
drm_modeset_backoff(&ctx);goto retry;- }
- drm_modeset_drop_locks(&ctx);
- drm_modeset_acquire_fini(&ctx);
- return ret;
}
@@ -696,12 +730,10 @@ 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. */
- drm_modeset_lock_crtc(crtc, crtc->cursor);
- if (crtc->cursor) {
ret = drm_mode_cursor_universal(crtc, req, file_priv);goto out;- }
if (crtc->cursor)
return drm_mode_cursor_universal(crtc, req, file_priv);drm_modeset_lock_crtc(crtc, crtc->cursor);
Why are you moving the lock? Shouldn't it still be covering the call to drm_mode_cursor_universal?
It would deadlock, because the new acquire ctx dance for universal planes also takes the same locks as this helper?
But I just realized that crtc->cursor is NULL here, I can simplify this some more.
I changed my mind :-) The end result is that the locking gets shuffled a bit more, and the acquire ctx dance moves all the way up to this function here. But that means more wiring up of ctx, so kinda would likee to keep it seperate.
Also note that crtc->cursor is invariant, so no locking needed for that. -Daniel
On Tue, Mar 28, 2017 at 08:23:53AM +0200, Daniel Vetter wrote:
On Mon, Mar 27, 2017 at 04:12:05PM -0400, Harry Wentland wrote:
On Wednesday, March 22, 2017 10:50:40 PM EDT Daniel Vetter wrote:
This is just prep work to get an acquire ctx into every place where we call ->update_plane or ->disable_plane.
v2: Keep the hidden acquire_ctx pointers valid while transitioning.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com
drivers/gpu/drm/drm_plane.c | 52 ++++++++++++++++++++++++++++++++++++--------- 1 file changed, 42 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c index a22e76837065..0d04888172a7 100644 --- a/drivers/gpu/drm/drm_plane.c +++ b/drivers/gpu/drm/drm_plane.c @@ -457,7 +457,8 @@ static int __setplane_internal(struct drm_plane *plane, 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)
uint32_t src_w, uint32_t src_h,struct drm_modeset_acquire_ctx *ctx){ int ret = 0;
@@ -537,13 +538,26 @@ static int setplane_internal(struct drm_plane *plane, uint32_t src_x, uint32_t src_y, uint32_t src_w, uint32_t src_h) {
- struct drm_modeset_acquire_ctx ctx; int ret;
- drm_modeset_lock_all(plane->dev);
- drm_modeset_acquire_init(&ctx, 0);
+retry:
- ret = drm_modeset_lock_all_ctx(plane->dev, &ctx);
- if (ret)
goto fail;- plane->dev->mode_config.acquire_ctx = &ctx; 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);
src_x, src_y, src_w, src_h, &ctx);+fail:
if (ret == -EDEADLK) {
drm_modeset_backoff(&ctx);goto retry;}
drm_modeset_drop_locks(&ctx);
drm_modeset_acquire_fini(&ctx);
return ret;
} @@ -613,11 +627,22 @@ static int drm_mode_cursor_universal(struct drm_crtc *crtc, int32_t crtc_x, crtc_y; uint32_t crtc_w = 0, crtc_h = 0; uint32_t src_w = 0, src_h = 0;
struct drm_modeset_acquire_ctx ctx; int ret = 0;
BUG_ON(!crtc->cursor); WARN_ON(crtc->cursor->crtc != crtc && crtc->cursor->crtc != NULL);
drm_modeset_acquire_init(&ctx, 0);
+retry:
- ret = drm_modeset_lock(&crtc->mutex, &ctx);
- if (ret)
goto fail;- ret = drm_modeset_lock(&crtc->cursor->mutex, &ctx);
- if (ret)
goto fail;- crtc->acquire_ctx = &ctx;
- /*
- Obtain fb we'll be using (either new or existing) and take an extra
- reference to it if fb != null. setplane will take care of dropping
@@ -662,7 +687,7 @@ static int drm_mode_cursor_universal(struct drm_crtc *crtc, */ ret = __setplane_internal(crtc->cursor, crtc, fb, crtc_x, crtc_y, crtc_w, crtc_h,
0, 0, src_w, src_h);
0, 0, src_w, src_h, &ctx);/* Update successful; save new cursor position, if necessary */ if (ret == 0 && req->flags & DRM_MODE_CURSOR_MOVE) {
@@ -670,6 +695,15 @@ static int drm_mode_cursor_universal(struct drm_crtc *crtc, crtc->cursor_y = req->y; }
+fail:
- if (ret == -EDEADLK) {
drm_modeset_backoff(&ctx);goto retry;- }
- drm_modeset_drop_locks(&ctx);
- drm_modeset_acquire_fini(&ctx);
- return ret;
}
@@ -696,12 +730,10 @@ 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. */
- drm_modeset_lock_crtc(crtc, crtc->cursor);
- if (crtc->cursor) {
ret = drm_mode_cursor_universal(crtc, req, file_priv);goto out;- }
if (crtc->cursor)
return drm_mode_cursor_universal(crtc, req, file_priv);drm_modeset_lock_crtc(crtc, crtc->cursor);
Why are you moving the lock? Shouldn't it still be covering the call to drm_mode_cursor_universal?
It would deadlock, because the new acquire ctx dance for universal planes also takes the same locks as this helper?
But I just realized that crtc->cursor is NULL here, I can simplify this some more.
Specifically this gets reshuffled in patch 14, but that is blocked on the vmwgfx story for now. -Daniel
On 2017-03-28 03:02 AM, Daniel Vetter wrote:
On Tue, Mar 28, 2017 at 08:23:53AM +0200, Daniel Vetter wrote:
On Mon, Mar 27, 2017 at 04:12:05PM -0400, Harry Wentland wrote:
On Wednesday, March 22, 2017 10:50:40 PM EDT Daniel Vetter wrote:
This is just prep work to get an acquire ctx into every place where we call ->update_plane or ->disable_plane.
v2: Keep the hidden acquire_ctx pointers valid while transitioning.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com
drivers/gpu/drm/drm_plane.c | 52 ++++++++++++++++++++++++++++++++++++--------- 1 file changed, 42 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c index a22e76837065..0d04888172a7 100644 --- a/drivers/gpu/drm/drm_plane.c +++ b/drivers/gpu/drm/drm_plane.c @@ -457,7 +457,8 @@ static int __setplane_internal(struct drm_plane *plane, 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)
uint32_t src_w, uint32_t src_h,struct drm_modeset_acquire_ctx *ctx){ int ret = 0;
@@ -537,13 +538,26 @@ static int setplane_internal(struct drm_plane *plane, uint32_t src_x, uint32_t src_y, uint32_t src_w, uint32_t src_h) {
- struct drm_modeset_acquire_ctx ctx; int ret;
- drm_modeset_lock_all(plane->dev);
- drm_modeset_acquire_init(&ctx, 0);
+retry:
- ret = drm_modeset_lock_all_ctx(plane->dev, &ctx);
- if (ret)
goto fail;- plane->dev->mode_config.acquire_ctx = &ctx; 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);
src_x, src_y, src_w, src_h, &ctx);+fail:
if (ret == -EDEADLK) {
drm_modeset_backoff(&ctx);goto retry;}
drm_modeset_drop_locks(&ctx);
drm_modeset_acquire_fini(&ctx);
return ret;
} @@ -613,11 +627,22 @@ static int drm_mode_cursor_universal(struct drm_crtc *crtc, int32_t crtc_x, crtc_y; uint32_t crtc_w = 0, crtc_h = 0; uint32_t src_w = 0, src_h = 0;
struct drm_modeset_acquire_ctx ctx; int ret = 0;
BUG_ON(!crtc->cursor); WARN_ON(crtc->cursor->crtc != crtc && crtc->cursor->crtc != NULL);
drm_modeset_acquire_init(&ctx, 0);
+retry:
- ret = drm_modeset_lock(&crtc->mutex, &ctx);
- if (ret)
goto fail;- ret = drm_modeset_lock(&crtc->cursor->mutex, &ctx);
- if (ret)
goto fail;- crtc->acquire_ctx = &ctx;
- /*
- Obtain fb we'll be using (either new or existing) and take an extra
- reference to it if fb != null. setplane will take care of dropping
@@ -662,7 +687,7 @@ static int drm_mode_cursor_universal(struct drm_crtc *crtc, */ ret = __setplane_internal(crtc->cursor, crtc, fb, crtc_x, crtc_y, crtc_w, crtc_h,
0, 0, src_w, src_h);
0, 0, src_w, src_h, &ctx);/* Update successful; save new cursor position, if necessary */ if (ret == 0 && req->flags & DRM_MODE_CURSOR_MOVE) {
@@ -670,6 +695,15 @@ static int drm_mode_cursor_universal(struct drm_crtc *crtc, crtc->cursor_y = req->y; }
+fail:
- if (ret == -EDEADLK) {
drm_modeset_backoff(&ctx);goto retry;- }
- drm_modeset_drop_locks(&ctx);
- drm_modeset_acquire_fini(&ctx);
- return ret;
}
@@ -696,12 +730,10 @@ 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. */
- drm_modeset_lock_crtc(crtc, crtc->cursor);
- if (crtc->cursor) {
ret = drm_mode_cursor_universal(crtc, req, file_priv);goto out;- }
if (crtc->cursor)
return drm_mode_cursor_universal(crtc, req, file_priv);drm_modeset_lock_crtc(crtc, crtc->cursor);
Why are you moving the lock? Shouldn't it still be covering the call to drm_mode_cursor_universal?
It would deadlock, because the new acquire ctx dance for universal planes also takes the same locks as this helper?
But I just realized that crtc->cursor is NULL here, I can simplify this some more.
Specifically this gets reshuffled in patch 14, but that is blocked on the vmwgfx story for now.
Ah, right, the lock moves down into drm_mode_cursor_universal. I don't know how I missed that.
Reviewed-by: Harry Wentland harry.wentland@amd.com
Harry
-Daniel
Just rolling it out, no code change here.
Cc: Ben Skeggs bskeggs@redhat.com Cc: Russell King linux@armlinux.org.uk Cc: Rob Clark robdclark@gmail.com Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com Cc: Eric Anholt eric@anholt.net Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/armada/armada_overlay.c | 3 ++- drivers/gpu/drm/drm_atomic_helper.c | 4 +++- drivers/gpu/drm/drm_plane.c | 2 +- drivers/gpu/drm/drm_plane_helper.c | 4 +++- drivers/gpu/drm/i915/intel_display.c | 5 +++-- drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c | 8 +++++--- drivers/gpu/drm/nouveau/dispnv04/overlay.c | 6 ++++-- drivers/gpu/drm/shmobile/shmob_drm_plane.c | 3 ++- drivers/gpu/drm/vc4/vc4_plane.c | 6 ++++-- include/drm/drm_atomic_helper.h | 3 ++- include/drm/drm_plane.h | 4 +++- include/drm/drm_plane_helper.h | 3 ++- 12 files changed, 34 insertions(+), 17 deletions(-)
diff --git a/drivers/gpu/drm/armada/armada_overlay.c b/drivers/gpu/drm/armada/armada_overlay.c index 34cb73d0db77..b54fd8cbd3a6 100644 --- a/drivers/gpu/drm/armada/armada_overlay.c +++ b/drivers/gpu/drm/armada/armada_overlay.c @@ -94,7 +94,8 @@ static int armada_ovl_plane_update(struct drm_plane *plane, struct drm_crtc *crtc, struct drm_framebuffer *fb, int crtc_x, int crtc_y, unsigned crtc_w, unsigned crtc_h, - uint32_t src_x, uint32_t src_y, uint32_t src_w, uint32_t src_h) + uint32_t src_x, uint32_t src_y, uint32_t src_w, uint32_t src_h, + struct drm_modeset_acquire_ctx *ctx) { struct armada_ovl_plane *dplane = drm_to_armada_ovl_plane(plane); struct armada_crtc *dcrtc = drm_to_armada_crtc(crtc); diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 278d72aa012f..8ef7c808670b 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -2078,6 +2078,7 @@ EXPORT_SYMBOL(drm_atomic_helper_swap_state); * @src_y: y offset of @fb for panning * @src_w: width of source rectangle in @fb * @src_h: height of source rectangle in @fb + * @ctx: lock acquire context * * Provides a default plane update handler using the atomic driver interface. * @@ -2090,7 +2091,8 @@ int drm_atomic_helper_update_plane(struct drm_plane *plane, int crtc_x, int crtc_y, unsigned int crtc_w, unsigned int crtc_h, uint32_t src_x, uint32_t src_y, - uint32_t src_w, uint32_t src_h) + uint32_t src_w, uint32_t src_h, + struct drm_modeset_acquire_ctx *ctx) { struct drm_atomic_state *state; struct drm_plane_state *plane_state; diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c index 0d04888172a7..8a4e75929810 100644 --- a/drivers/gpu/drm/drm_plane.c +++ b/drivers/gpu/drm/drm_plane.c @@ -510,7 +510,7 @@ static int __setplane_internal(struct drm_plane *plane, plane->old_fb = plane->fb; ret = plane->funcs->update_plane(plane, crtc, fb, crtc_x, crtc_y, crtc_w, crtc_h, - src_x, src_y, src_w, src_h); + src_x, src_y, src_w, src_h, ctx); if (!ret) { plane->crtc = crtc; plane->fb = fb; diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c index de1ac5e08f4d..2339879f775d 100644 --- a/drivers/gpu/drm/drm_plane_helper.c +++ b/drivers/gpu/drm/drm_plane_helper.c @@ -275,6 +275,7 @@ EXPORT_SYMBOL(drm_plane_helper_check_update); * @src_y: y offset of @fb for panning * @src_w: width of source rectangle in @fb * @src_h: height of source rectangle in @fb + * @ctx: lock acquire context, not used here * * Provides a default plane update handler for primary planes. This is handler * is called in response to a userspace SetPlane operation on the plane with a @@ -303,7 +304,8 @@ int drm_primary_helper_update(struct drm_plane *plane, struct drm_crtc *crtc, int crtc_x, int crtc_y, unsigned int crtc_w, unsigned int crtc_h, uint32_t src_x, uint32_t src_y, - uint32_t src_w, uint32_t src_h) + uint32_t src_w, uint32_t src_h, + struct drm_modeset_acquire_ctx *ctx) { struct drm_mode_set set = { .crtc = crtc, diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 010e5ddb198a..e27ea89efd67 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -13426,7 +13426,8 @@ intel_legacy_cursor_update(struct drm_plane *plane, int crtc_x, int crtc_y, unsigned int crtc_w, unsigned int crtc_h, uint32_t src_x, uint32_t src_y, - uint32_t src_w, uint32_t src_h) + uint32_t src_w, uint32_t src_h, + struct drm_modeset_acquire_ctx *ctx) { struct drm_i915_private *dev_priv = to_i915(crtc->dev); int ret; @@ -13539,7 +13540,7 @@ intel_legacy_cursor_update(struct drm_plane *plane, slow: return drm_atomic_helper_update_plane(plane, crtc, fb, crtc_x, crtc_y, crtc_w, crtc_h, - src_x, src_y, src_w, src_h); + src_x, src_y, src_w, src_h, ctx); }
static const struct drm_plane_funcs intel_cursor_plane_funcs = { diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c index 0ffb8affef35..60a5451ae0b9 100644 --- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c +++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c @@ -37,7 +37,8 @@ static int mdp5_update_cursor_plane_legacy(struct drm_plane *plane, int crtc_x, int crtc_y, unsigned int crtc_w, unsigned int crtc_h, uint32_t src_x, uint32_t src_y, - uint32_t src_w, uint32_t src_h); + uint32_t src_w, uint32_t src_h, + struct drm_modeset_acquire_ctx *ctx);
static void set_scanout_locked(struct drm_plane *plane, struct drm_framebuffer *fb); @@ -886,7 +887,8 @@ static int mdp5_update_cursor_plane_legacy(struct drm_plane *plane, int crtc_x, int crtc_y, unsigned int crtc_w, unsigned int crtc_h, uint32_t src_x, uint32_t src_y, - uint32_t src_w, uint32_t src_h) + uint32_t src_w, uint32_t src_h, + struct drm_modeset_acquire_ctx *ctx) { struct drm_plane_state *plane_state, *new_plane_state; struct mdp5_plane_state *mdp5_pstate; @@ -954,7 +956,7 @@ static int mdp5_update_cursor_plane_legacy(struct drm_plane *plane, slow: return drm_atomic_helper_update_plane(plane, crtc, fb, crtc_x, crtc_y, crtc_w, crtc_h, - src_x, src_y, src_w, src_h); + src_x, src_y, src_w, src_h, ctx); }
enum mdp5_pipe mdp5_plane_pipe(struct drm_plane *plane) diff --git a/drivers/gpu/drm/nouveau/dispnv04/overlay.c b/drivers/gpu/drm/nouveau/dispnv04/overlay.c index 5319f2a7f24d..2d90e7898ec8 100644 --- a/drivers/gpu/drm/nouveau/dispnv04/overlay.c +++ b/drivers/gpu/drm/nouveau/dispnv04/overlay.c @@ -94,7 +94,8 @@ nv10_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, struct drm_framebuffer *fb, int crtc_x, int crtc_y, unsigned int crtc_w, unsigned int crtc_h, uint32_t src_x, uint32_t src_y, - uint32_t src_w, uint32_t src_h) + uint32_t src_w, uint32_t src_h, + struct drm_modeset_acquire_ctx *ctx) { struct nouveau_drm *drm = nouveau_drm(plane->dev); struct nvif_object *dev = &drm->client.device.object; @@ -345,7 +346,8 @@ nv04_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, struct drm_framebuffer *fb, int crtc_x, int crtc_y, unsigned int crtc_w, unsigned int crtc_h, uint32_t src_x, uint32_t src_y, - uint32_t src_w, uint32_t src_h) + uint32_t src_w, uint32_t src_h, + struct drm_modeset_acquire_ctx *ctx) { struct nvif_object *dev = &nouveau_drm(plane->dev)->client.device.object; struct nouveau_plane *nv_plane = diff --git a/drivers/gpu/drm/shmobile/shmob_drm_plane.c b/drivers/gpu/drm/shmobile/shmob_drm_plane.c index 2023a93cee2b..9a3c8ddfe198 100644 --- a/drivers/gpu/drm/shmobile/shmob_drm_plane.c +++ b/drivers/gpu/drm/shmobile/shmob_drm_plane.c @@ -177,7 +177,8 @@ shmob_drm_plane_update(struct drm_plane *plane, struct drm_crtc *crtc, struct drm_framebuffer *fb, int crtc_x, int crtc_y, unsigned int crtc_w, unsigned int crtc_h, uint32_t src_x, uint32_t src_y, - uint32_t src_w, uint32_t src_h) + uint32_t src_w, uint32_t src_h, + struct drm_modeset_acquire_ctx *ctx) { struct shmob_drm_plane *splane = to_shmob_plane(plane); struct shmob_drm_device *sdev = plane->dev->dev_private; diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c index 0f4564beb017..d34cd5393a9b 100644 --- a/drivers/gpu/drm/vc4/vc4_plane.c +++ b/drivers/gpu/drm/vc4/vc4_plane.c @@ -756,7 +756,8 @@ vc4_update_plane(struct drm_plane *plane, int crtc_x, int crtc_y, unsigned int crtc_w, unsigned int crtc_h, uint32_t src_x, uint32_t src_y, - uint32_t src_w, uint32_t src_h) + uint32_t src_w, uint32_t src_h, + struct drm_modeset_acquire_ctx *ctx) { struct drm_plane_state *plane_state; struct vc4_plane_state *vc4_state; @@ -817,7 +818,8 @@ vc4_update_plane(struct drm_plane *plane, crtc_x, crtc_y, crtc_w, crtc_h, src_x, src_y, - src_w, src_h); + src_w, src_h, + ctx); }
static const struct drm_plane_funcs vc4_plane_funcs = { diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h index 969f7237f1fc..62ac6053721d 100644 --- a/include/drm/drm_atomic_helper.h +++ b/include/drm/drm_atomic_helper.h @@ -94,7 +94,8 @@ int drm_atomic_helper_update_plane(struct drm_plane *plane, int crtc_x, int crtc_y, unsigned int crtc_w, unsigned int crtc_h, uint32_t src_x, uint32_t src_y, - uint32_t src_w, uint32_t src_h); + uint32_t src_w, uint32_t src_h, + struct drm_modeset_acquire_ctx *ctx); int drm_atomic_helper_disable_plane(struct drm_plane *plane); int __drm_atomic_helper_disable_plane(struct drm_plane *plane, struct drm_plane_state *plane_state); diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h index 31da9f0c4ad2..1076fe150cdf 100644 --- a/include/drm/drm_plane.h +++ b/include/drm/drm_plane.h @@ -29,6 +29,7 @@
struct drm_crtc; struct drm_printer; +struct drm_modeset_acquire_ctx;
/** * struct drm_plane_state - mutable plane state @@ -184,7 +185,8 @@ struct drm_plane_funcs { int crtc_x, int crtc_y, unsigned int crtc_w, unsigned int crtc_h, uint32_t src_x, uint32_t src_y, - uint32_t src_w, uint32_t src_h); + uint32_t src_w, uint32_t src_h, + struct drm_modeset_acquire_ctx *ctx);
/** * @disable_plane: diff --git a/include/drm/drm_plane_helper.h b/include/drm/drm_plane_helper.h index c18959685c06..ea219423d72b 100644 --- a/include/drm/drm_plane_helper.h +++ b/include/drm/drm_plane_helper.h @@ -61,7 +61,8 @@ int drm_primary_helper_update(struct drm_plane *plane, int crtc_x, int crtc_y, unsigned int crtc_w, unsigned int crtc_h, uint32_t src_x, uint32_t src_y, - uint32_t src_w, uint32_t src_h); + uint32_t src_w, uint32_t src_h, + struct drm_modeset_acquire_ctx *ctx); int drm_primary_helper_disable(struct drm_plane *plane); void drm_primary_helper_destroy(struct drm_plane *plane); extern const struct drm_plane_funcs drm_primary_helper_funcs;
On Wed, Mar 22, 2017 at 10:50:41PM +0100, Daniel Vetter wrote:
diff --git a/drivers/gpu/drm/armada/armada_overlay.c b/drivers/gpu/drm/armada/armada_overlay.c index 34cb73d0db77..b54fd8cbd3a6 100644 --- a/drivers/gpu/drm/armada/armada_overlay.c +++ b/drivers/gpu/drm/armada/armada_overlay.c @@ -94,7 +94,8 @@ static int armada_ovl_plane_update(struct drm_plane *plane, struct drm_crtc *crtc, struct drm_framebuffer *fb, int crtc_x, int crtc_y, unsigned crtc_w, unsigned crtc_h,
- uint32_t src_x, uint32_t src_y, uint32_t src_w, uint32_t src_h)
- uint32_t src_x, uint32_t src_y, uint32_t src_w, uint32_t src_h,
- struct drm_modeset_acquire_ctx *ctx)
I'm rather unhappy that we're ending up with a function taking soo many arguments.
Most of these have to be stacked on ARM, and I'm guessing most architectures end up doing something similar. Is there a reason why we don't pass pointers to drm_rect's or maybe even consider passing the drm_plane_state structure in?
I've found that, when cleaning up these code paths in armada, that storing all the parameters into a drm_plane_state and then validating it with drm_plane_helper_check_state() is by way the simplest solution, and of course, it's forward-compatible with atomic modeset.
On Wed, Mar 22, 2017 at 11:03:41PM +0000, Russell King - ARM Linux wrote:
On Wed, Mar 22, 2017 at 10:50:41PM +0100, Daniel Vetter wrote:
diff --git a/drivers/gpu/drm/armada/armada_overlay.c b/drivers/gpu/drm/armada/armada_overlay.c index 34cb73d0db77..b54fd8cbd3a6 100644 --- a/drivers/gpu/drm/armada/armada_overlay.c +++ b/drivers/gpu/drm/armada/armada_overlay.c @@ -94,7 +94,8 @@ static int armada_ovl_plane_update(struct drm_plane *plane, struct drm_crtc *crtc, struct drm_framebuffer *fb, int crtc_x, int crtc_y, unsigned crtc_w, unsigned crtc_h,
- uint32_t src_x, uint32_t src_y, uint32_t src_w, uint32_t src_h)
- uint32_t src_x, uint32_t src_y, uint32_t src_w, uint32_t src_h,
- struct drm_modeset_acquire_ctx *ctx)
I'm rather unhappy that we're ending up with a function taking soo many arguments.
Most of these have to be stacked on ARM, and I'm guessing most architectures end up doing something similar. Is there a reason why we don't pass pointers to drm_rect's or maybe even consider passing the drm_plane_state structure in?
I've found that, when cleaning up these code paths in armada, that storing all the parameters into a drm_plane_state and then validating it with drm_plane_helper_check_state() is by way the simplest solution, and of course, it's forward-compatible with atomic modeset.
Yeah, we could do that, there's not many plane_update implementations left. But it wouldn't help for this case here, because acquire context is in drm_atomic_state, not in the individual per-object state structs.
There's no reason this isn't the case except organically grown and no one bothered to improve it. I'll be happy to review such patches, but probably won't ever get around to typing them. -Daniel
This way I can explain why it'll be fine to pass a NULL acquire ctx here in the next patch.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/drm_plane.c | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c index 8a4e75929810..67119332c441 100644 --- a/drivers/gpu/drm/drm_plane.c +++ b/drivers/gpu/drm/drm_plane.c @@ -277,6 +277,12 @@ EXPORT_SYMBOL(drm_plane_from_index); * * Used when the plane's current framebuffer is destroyed, * and when restoring fbdev mode. + * + * Note that this function is not suitable for atomic drivers, since it doesn't + * wire through the lock acquisition context properly and hence can't handle + * retries or driver private locks. You probably want to use + * drm_atomic_helper_disable_plane() or + * drm_atomic_helper_disable_planes_on_crtc() instead. */ void drm_plane_force_disable(struct drm_plane *plane) { @@ -285,6 +291,8 @@ void drm_plane_force_disable(struct drm_plane *plane) if (!plane->fb) return;
+ WARN_ON(drm_drv_uses_atomic_modeset(plane->dev)); + plane->old_fb = plane->fb; ret = plane->funcs->disable_plane(plane); if (ret) {
Nouveau had a few direct calls to ->disable_plane, I replaced those with drm_plane_force_disable. Same story for shmob.
Otherwise no code changes.
Cc: Ben Skeggs bskeggs@redhat.com Cc: Russell King linux@armlinux.org.uk Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/armada/armada_overlay.c | 3 ++- drivers/gpu/drm/drm_atomic_helper.c | 4 +++- drivers/gpu/drm/drm_plane.c | 4 ++-- drivers/gpu/drm/drm_plane_helper.c | 5 +++-- drivers/gpu/drm/nouveau/dispnv04/overlay.c | 12 +++++++----- drivers/gpu/drm/shmobile/shmob_drm_plane.c | 5 +++-- include/drm/drm_atomic_helper.h | 3 ++- include/drm/drm_plane.h | 3 ++- include/drm/drm_plane_helper.h | 3 ++- 9 files changed, 26 insertions(+), 16 deletions(-)
diff --git a/drivers/gpu/drm/armada/armada_overlay.c b/drivers/gpu/drm/armada/armada_overlay.c index b54fd8cbd3a6..424e465ff407 100644 --- a/drivers/gpu/drm/armada/armada_overlay.c +++ b/drivers/gpu/drm/armada/armada_overlay.c @@ -258,7 +258,8 @@ armada_ovl_plane_update(struct drm_plane *plane, struct drm_crtc *crtc, return 0; }
-static int armada_ovl_plane_disable(struct drm_plane *plane) +static int armada_ovl_plane_disable(struct drm_plane *plane, + struct drm_modeset_acquire_ctx *ctx) { struct armada_ovl_plane *dplane = drm_to_armada_ovl_plane(plane); struct drm_framebuffer *fb; diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 8ef7c808670b..1142075032a2 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -2152,13 +2152,15 @@ EXPORT_SYMBOL(drm_atomic_helper_update_plane); /** * drm_atomic_helper_disable_plane - Helper for primary plane disable using * atomic * @plane: plane to disable + * @ctx: lock acquire context * * Provides a default plane disable handler using the atomic driver interface. * * RETURNS: * Zero on success, error code on failure */ -int drm_atomic_helper_disable_plane(struct drm_plane *plane) +int drm_atomic_helper_disable_plane(struct drm_plane *plane, + struct drm_modeset_acquire_ctx *ctx) { struct drm_atomic_state *state; struct drm_plane_state *plane_state; diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c index 67119332c441..526e74b548b2 100644 --- a/drivers/gpu/drm/drm_plane.c +++ b/drivers/gpu/drm/drm_plane.c @@ -294,7 +294,7 @@ void drm_plane_force_disable(struct drm_plane *plane) WARN_ON(drm_drv_uses_atomic_modeset(plane->dev));
plane->old_fb = plane->fb; - ret = plane->funcs->disable_plane(plane); + ret = plane->funcs->disable_plane(plane, NULL); if (ret) { DRM_ERROR("failed to disable plane with busy fb\n"); plane->old_fb = NULL; @@ -473,7 +473,7 @@ static int __setplane_internal(struct drm_plane *plane, /* No fb means shut it down */ if (!fb) { plane->old_fb = plane->fb; - ret = plane->funcs->disable_plane(plane); + ret = plane->funcs->disable_plane(plane, ctx); if (!ret) { plane->crtc = NULL; plane->fb = NULL; diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c index 2339879f775d..775e94c30368 100644 --- a/drivers/gpu/drm/drm_plane_helper.c +++ b/drivers/gpu/drm/drm_plane_helper.c @@ -349,7 +349,7 @@ int drm_primary_helper_update(struct drm_plane *plane, struct drm_crtc *crtc, * provides their own disable function, this will just * wind up returning -EINVAL to userspace. */ - return plane->funcs->disable_plane(plane); + return plane->funcs->disable_plane(plane, ctx);
/* Find current connectors for CRTC */ num_connectors = get_connectors_for_crtc(crtc, NULL, 0); @@ -398,7 +398,8 @@ EXPORT_SYMBOL(drm_primary_helper_update); * RETURNS: * Unconditionally returns -EINVAL. */ -int drm_primary_helper_disable(struct drm_plane *plane) +int drm_primary_helper_disable(struct drm_plane *plane, + struct drm_modeset_acquire_ctx *ctx) { return -EINVAL; } diff --git a/drivers/gpu/drm/nouveau/dispnv04/overlay.c b/drivers/gpu/drm/nouveau/dispnv04/overlay.c index 2d90e7898ec8..e54944d23268 100644 --- a/drivers/gpu/drm/nouveau/dispnv04/overlay.c +++ b/drivers/gpu/drm/nouveau/dispnv04/overlay.c @@ -173,7 +173,8 @@ nv10_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, }
static int -nv10_disable_plane(struct drm_plane *plane) +nv10_disable_plane(struct drm_plane *plane, + struct drm_modeset_acquire_ctx *ctx) { struct nvif_object *dev = &nouveau_drm(plane->dev)->client.device.object; struct nouveau_plane *nv_plane = @@ -191,7 +192,7 @@ nv10_disable_plane(struct drm_plane *plane) static void nv_destroy_plane(struct drm_plane *plane) { - plane->funcs->disable_plane(plane); + drm_plane_force_disable(plane); drm_plane_cleanup(plane); kfree(plane); } @@ -332,7 +333,7 @@ nv10_overlay_init(struct drm_device *device)
plane->set_params = nv10_set_params; nv10_set_params(plane); - nv10_disable_plane(&plane->base); + drm_plane_force_disable(&plane->base); return; cleanup: drm_plane_cleanup(&plane->base); @@ -427,7 +428,8 @@ nv04_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, }
static int -nv04_disable_plane(struct drm_plane *plane) +nv04_disable_plane(struct drm_plane *plane, + struct drm_modeset_acquire_ctx *ctx) { struct nvif_object *dev = &nouveau_drm(plane->dev)->client.device.object; struct nouveau_plane *nv_plane = @@ -485,7 +487,7 @@ nv04_overlay_init(struct drm_device *device) drm_object_attach_property(&plane->base.base, plane->props.brightness, plane->brightness);
- nv04_disable_plane(&plane->base); + drm_plane_force_disable(&plane->base); return; cleanup: drm_plane_cleanup(&plane->base); diff --git a/drivers/gpu/drm/shmobile/shmob_drm_plane.c b/drivers/gpu/drm/shmobile/shmob_drm_plane.c index 9a3c8ddfe198..97f6e4a3eb0d 100644 --- a/drivers/gpu/drm/shmobile/shmob_drm_plane.c +++ b/drivers/gpu/drm/shmobile/shmob_drm_plane.c @@ -209,7 +209,8 @@ shmob_drm_plane_update(struct drm_plane *plane, struct drm_crtc *crtc, return 0; }
-static int shmob_drm_plane_disable(struct drm_plane *plane) +static int shmob_drm_plane_disable(struct drm_plane *plane, + struct drm_modeset_acquire_ctx *ctx) { struct shmob_drm_plane *splane = to_shmob_plane(plane); struct shmob_drm_device *sdev = plane->dev->dev_private; @@ -222,7 +223,7 @@ static int shmob_drm_plane_disable(struct drm_plane *plane)
static void shmob_drm_plane_destroy(struct drm_plane *plane) { - shmob_drm_plane_disable(plane); + drm_plane_force_disable(plane); drm_plane_cleanup(plane); }
diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h index 62ac6053721d..73554fff086a 100644 --- a/include/drm/drm_atomic_helper.h +++ b/include/drm/drm_atomic_helper.h @@ -96,7 +96,8 @@ int drm_atomic_helper_update_plane(struct drm_plane *plane, uint32_t src_x, uint32_t src_y, uint32_t src_w, uint32_t src_h, struct drm_modeset_acquire_ctx *ctx); -int drm_atomic_helper_disable_plane(struct drm_plane *plane); +int drm_atomic_helper_disable_plane(struct drm_plane *plane, + struct drm_modeset_acquire_ctx *ctx); int __drm_atomic_helper_disable_plane(struct drm_plane *plane, struct drm_plane_state *plane_state); int drm_atomic_helper_set_config(struct drm_mode_set *set); diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h index 1076fe150cdf..c2eae851fa30 100644 --- a/include/drm/drm_plane.h +++ b/include/drm/drm_plane.h @@ -203,7 +203,8 @@ struct drm_plane_funcs { * * 0 on success or a negative error code on failure. */ - int (*disable_plane)(struct drm_plane *plane); + int (*disable_plane)(struct drm_plane *plane, + struct drm_modeset_acquire_ctx *ctx);
/** * @destroy: diff --git a/include/drm/drm_plane_helper.h b/include/drm/drm_plane_helper.h index ea219423d72b..7c8a00ceadb7 100644 --- a/include/drm/drm_plane_helper.h +++ b/include/drm/drm_plane_helper.h @@ -63,7 +63,8 @@ int drm_primary_helper_update(struct drm_plane *plane, uint32_t src_x, uint32_t src_y, uint32_t src_w, uint32_t src_h, struct drm_modeset_acquire_ctx *ctx); -int drm_primary_helper_disable(struct drm_plane *plane); +int drm_primary_helper_disable(struct drm_plane *plane, + struct drm_modeset_acquire_ctx *ctx); void drm_primary_helper_destroy(struct drm_plane *plane); extern const struct drm_plane_funcs drm_primary_helper_funcs;
We can now properly retry at the top level, yay!
v2: Also remove the temporary acquire_ctx hack again, no longer needed!
Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/drm_atomic_helper.c | 49 ++----------------------------------- drivers/gpu/drm/drm_plane.c | 2 -- 2 files changed, 2 insertions(+), 49 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 1142075032a2..845378c92ccb 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -2102,8 +2102,7 @@ int drm_atomic_helper_update_plane(struct drm_plane *plane, if (!state) return -ENOMEM;
- state->acquire_ctx = drm_modeset_legacy_acquire_ctx(crtc); -retry: + state->acquire_ctx = ctx; plane_state = drm_atomic_get_plane_state(state, plane); if (IS_ERR(plane_state)) { ret = PTR_ERR(plane_state); @@ -2128,24 +2127,8 @@ int drm_atomic_helper_update_plane(struct drm_plane *plane,
ret = drm_atomic_commit(state); fail: - if (ret == -EDEADLK) - goto backoff; - drm_atomic_state_put(state); return ret; - -backoff: - drm_atomic_state_clear(state); - drm_atomic_legacy_backoff(state); - - /* - * Someone might have exchanged the framebuffer while we dropped locks - * in the backoff code. We need to fix up the fb refcount tracking the - * core does for us. - */ - plane->old_fb = plane->fb; - - goto retry; } EXPORT_SYMBOL(drm_atomic_helper_update_plane);
@@ -2166,23 +2149,11 @@ int drm_atomic_helper_disable_plane(struct drm_plane *plane, struct drm_plane_state *plane_state; int ret = 0;
- /* - * FIXME: Without plane->crtc set we can't get at the implicit legacy - * acquire context. The real fix will be to wire the acquire ctx through - * everywhere we need it, but meanwhile prevent chaos by just skipping - * this noop. The critical case is the cursor ioctls which a) only grab - * crtc/cursor-plane locks (so we need the crtc to get at the right - * acquire context) and b) can try to disable the plane multiple times. - */ - if (!plane->crtc) - return 0; - state = drm_atomic_state_alloc(plane->dev); if (!state) return -ENOMEM;
- state->acquire_ctx = drm_modeset_legacy_acquire_ctx(plane->crtc); -retry: + state->acquire_ctx = ctx; plane_state = drm_atomic_get_plane_state(state, plane); if (IS_ERR(plane_state)) { ret = PTR_ERR(plane_state); @@ -2198,24 +2169,8 @@ int drm_atomic_helper_disable_plane(struct drm_plane *plane,
ret = drm_atomic_commit(state); fail: - if (ret == -EDEADLK) - goto backoff; - drm_atomic_state_put(state); return ret; - -backoff: - drm_atomic_state_clear(state); - drm_atomic_legacy_backoff(state); - - /* - * Someone might have exchanged the framebuffer while we dropped locks - * in the backoff code. We need to fix up the fb refcount tracking the - * core does for us. - */ - plane->old_fb = plane->fb; - - goto retry; } EXPORT_SYMBOL(drm_atomic_helper_disable_plane);
diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c index 526e74b548b2..8535dc17db7e 100644 --- a/drivers/gpu/drm/drm_plane.c +++ b/drivers/gpu/drm/drm_plane.c @@ -554,7 +554,6 @@ static int setplane_internal(struct drm_plane *plane, ret = drm_modeset_lock_all_ctx(plane->dev, &ctx); if (ret) goto fail; - plane->dev->mode_config.acquire_ctx = &ctx; ret = __setplane_internal(plane, crtc, fb, crtc_x, crtc_y, crtc_w, crtc_h, src_x, src_y, src_w, src_h, &ctx); @@ -649,7 +648,6 @@ static int drm_mode_cursor_universal(struct drm_crtc *crtc, ret = drm_modeset_lock(&crtc->cursor->mutex, &ctx); if (ret) goto fail; - crtc->acquire_ctx = &ctx;
/* * Obtain fb we'll be using (either new or existing) and take an extra
It's been around forever, no one bothered to address the FIXME, so I presume it's all fine.
Cc: Sinclair Yeh syeh@vmware.com Cc: Thomas Hellstrom thellstrom@vmware.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 25 ------------------------- 1 file changed, 25 deletions(-)
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c index d492d57d5309..424b3fc57203 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c @@ -148,15 +148,6 @@ int vmw_du_crtc_cursor_set2(struct drm_crtc *crtc, struct drm_file *file_priv, s32 hotspot_x, hotspot_y; int ret;
- /* - * FIXME: Unclear whether there's any global state touched by the - * cursor_set function, especially vmw_cursor_update_position looks - * suspicious. For now take the easy route and reacquire all locks. We - * can do this since the caller in the drm core doesn't check anything - * which is protected by any looks. - */ - drm_modeset_unlock_crtc(crtc); - drm_modeset_lock_all(dev_priv->dev); hotspot_x = hot_x + du->hotspot_x; hotspot_y = hot_y + du->hotspot_y;
@@ -224,9 +215,6 @@ int vmw_du_crtc_cursor_set2(struct drm_crtc *crtc, struct drm_file *file_priv, }
out: - drm_modeset_unlock_all(dev_priv->dev); - drm_modeset_lock_crtc(crtc, crtc->cursor); - return ret; }
@@ -239,25 +227,12 @@ int vmw_du_crtc_cursor_move(struct drm_crtc *crtc, int x, int y) du->cursor_x = x + du->set_gui_x; du->cursor_y = y + du->set_gui_y;
- /* - * FIXME: Unclear whether there's any global state touched by the - * cursor_set function, especially vmw_cursor_update_position looks - * suspicious. For now take the easy route and reacquire all locks. We - * can do this since the caller in the drm core doesn't check anything - * which is protected by any looks. - */ - drm_modeset_unlock_crtc(crtc); - drm_modeset_lock_all(dev_priv->dev); - vmw_cursor_update_position(dev_priv, shown, du->cursor_x + du->hotspot_x + du->core_hotspot_x, du->cursor_y + du->hotspot_y + du->core_hotspot_y);
- drm_modeset_unlock_all(dev_priv->dev); - drm_modeset_lock_crtc(crtc, crtc->cursor); - return 0; }
On 03/22/2017 10:50 PM, Daniel Vetter wrote:
It's been around forever, no one bothered to address the FIXME, so I presume it's all fine.
Cc: Sinclair Yeh syeh@vmware.com Cc: Thomas Hellstrom thellstrom@vmware.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com
NAK. We need to properly address this. Probably as part of the atomic update. /Thomas
drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 25 ------------------------- 1 file changed, 25 deletions(-)
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c index d492d57d5309..424b3fc57203 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c @@ -148,15 +148,6 @@ int vmw_du_crtc_cursor_set2(struct drm_crtc *crtc, struct drm_file *file_priv, s32 hotspot_x, hotspot_y; int ret;
- /*
* FIXME: Unclear whether there's any global state touched by the* cursor_set function, especially vmw_cursor_update_position looks* suspicious. For now take the easy route and reacquire all locks. We* can do this since the caller in the drm core doesn't check anything* which is protected by any looks.*/- drm_modeset_unlock_crtc(crtc);
- drm_modeset_lock_all(dev_priv->dev); hotspot_x = hot_x + du->hotspot_x; hotspot_y = hot_y + du->hotspot_y;
@@ -224,9 +215,6 @@ int vmw_du_crtc_cursor_set2(struct drm_crtc *crtc, struct drm_file *file_priv, }
out:
- drm_modeset_unlock_all(dev_priv->dev);
- drm_modeset_lock_crtc(crtc, crtc->cursor);
- return ret;
}
@@ -239,25 +227,12 @@ int vmw_du_crtc_cursor_move(struct drm_crtc *crtc, int x, int y) du->cursor_x = x + du->set_gui_x; du->cursor_y = y + du->set_gui_y;
/*
* FIXME: Unclear whether there's any global state touched by the* cursor_set function, especially vmw_cursor_update_position looks* suspicious. For now take the easy route and reacquire all locks. We* can do this since the caller in the drm core doesn't check anything* which is protected by any looks.*/drm_modeset_unlock_crtc(crtc);
drm_modeset_lock_all(dev_priv->dev);
vmw_cursor_update_position(dev_priv, shown, du->cursor_x + du->hotspot_x + du->core_hotspot_x, du->cursor_y + du->hotspot_y + du->core_hotspot_y);
drm_modeset_unlock_all(dev_priv->dev);
drm_modeset_lock_crtc(crtc, crtc->cursor);
return 0;
}
On Thu, Mar 23, 2017 at 07:22:31AM +0100, Thomas Hellstrom wrote:
On 03/22/2017 10:50 PM, Daniel Vetter wrote:
It's been around forever, no one bothered to address the FIXME, so I presume it's all fine.
Cc: Sinclair Yeh syeh@vmware.com Cc: Thomas Hellstrom thellstrom@vmware.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com
NAK. We need to properly address this. Probably as part of the atomic update.
So could someone with vmwgfx understanding explain this? Note that the FIXME was originally added by me years ago, because I wasn't sure (only about 90%) that this is safe, and was essentially pleading for a vmwgfx expert to review this?
Since it didn't happen I presume it's not that terribly and probably safe ...
I'm still 90% sure that this is correct, but I'd love for a vmwgfx to audit it. Replying with a NAK is kinda not the response I was hoping for (and yes I guess I should have explained what's going on here better, but it's just a git blame of the FIXME comment away).
Thanks,
Daniel
/Thomas
drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 25 ------------------------- 1 file changed, 25 deletions(-)
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c index d492d57d5309..424b3fc57203 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c @@ -148,15 +148,6 @@ int vmw_du_crtc_cursor_set2(struct drm_crtc *crtc, struct drm_file *file_priv, s32 hotspot_x, hotspot_y; int ret;
- /*
* FIXME: Unclear whether there's any global state touched by the* cursor_set function, especially vmw_cursor_update_position looks* suspicious. For now take the easy route and reacquire all locks. We* can do this since the caller in the drm core doesn't check anything* which is protected by any looks.*/- drm_modeset_unlock_crtc(crtc);
- drm_modeset_lock_all(dev_priv->dev); hotspot_x = hot_x + du->hotspot_x; hotspot_y = hot_y + du->hotspot_y;
@@ -224,9 +215,6 @@ int vmw_du_crtc_cursor_set2(struct drm_crtc *crtc, struct drm_file *file_priv, }
out:
- drm_modeset_unlock_all(dev_priv->dev);
- drm_modeset_lock_crtc(crtc, crtc->cursor);
- return ret;
}
@@ -239,25 +227,12 @@ int vmw_du_crtc_cursor_move(struct drm_crtc *crtc, int x, int y) du->cursor_x = x + du->set_gui_x; du->cursor_y = y + du->set_gui_y;
/*
* FIXME: Unclear whether there's any global state touched by the* cursor_set function, especially vmw_cursor_update_position looks* suspicious. For now take the easy route and reacquire all locks. We* can do this since the caller in the drm core doesn't check anything* which is protected by any looks.*/drm_modeset_unlock_crtc(crtc);
drm_modeset_lock_all(dev_priv->dev);
vmw_cursor_update_position(dev_priv, shown, du->cursor_x + du->hotspot_x + du->core_hotspot_x, du->cursor_y + du->hotspot_y + du->core_hotspot_y);
drm_modeset_unlock_all(dev_priv->dev);
drm_modeset_lock_crtc(crtc, crtc->cursor);
return 0;
}
On Thu, Mar 23, 2017 at 08:28:32AM +0100, Daniel Vetter wrote:
On Thu, Mar 23, 2017 at 07:22:31AM +0100, Thomas Hellstrom wrote:
On 03/22/2017 10:50 PM, Daniel Vetter wrote:
It's been around forever, no one bothered to address the FIXME, so I presume it's all fine.
Cc: Sinclair Yeh syeh@vmware.com Cc: Thomas Hellstrom thellstrom@vmware.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com
NAK. We need to properly address this. Probably as part of the atomic update.
So could someone with vmwgfx understanding explain this? Note that the FIXME was originally added by me years ago, because I wasn't sure (only about 90%) that this is safe, and was essentially pleading for a vmwgfx expert to review this?
Since it didn't happen I presume it's not that terribly and probably safe ...
I'm still 90% sure that this is correct, but I'd love for a vmwgfx to audit it. Replying with a NAK is kinda not the response I was hoping for (and yes I guess I should have explained what's going on here better, but it's just a git blame of the FIXME comment away).
Bit more context even: This lock dropping dance is _not_ safe from a drm core perspective. But when I've done the original kms locking rework the tradeoff between upsetting core state a bit and totally breaking vmwgfx leaned towards not breaking vmwgfx. And iirc you or Syeh promised to look at this and then either remove the FIXME, maybe with a vmwgfx lock/unlock added if there's a gap (I looked, didn't find one, but I don't understand vmwgfx in details really).
Thanks, Daniel
Hi, Daniel,
On 03/23/2017 08:31 AM, Daniel Vetter wrote:
On Thu, Mar 23, 2017 at 08:28:32AM +0100, Daniel Vetter wrote:
On Thu, Mar 23, 2017 at 07:22:31AM +0100, Thomas Hellstrom wrote:
On 03/22/2017 10:50 PM, Daniel Vetter wrote:
It's been around forever, no one bothered to address the FIXME, so I presume it's all fine.
Cc: Sinclair Yeh syeh@vmware.com Cc: Thomas Hellstrom thellstrom@vmware.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com
NAK. We need to properly address this. Probably as part of the atomic update.
So could someone with vmwgfx understanding explain this? Note that the FIXME was originally added by me years ago, because I wasn't sure (only about 90%) that this is safe, and was essentially pleading for a vmwgfx expert to review this?
Since it didn't happen I presume it's not that terribly and probably safe ...
I'm still 90% sure that this is correct, but I'd love for a vmwgfx to audit it. Replying with a NAK is kinda not the response I was hoping for (and yes I guess I should have explained what's going on here better, but it's just a git blame of the FIXME comment away).
So the code has been left in place because it works. Altering it now will create unnecessary merge conflicts with the atomic code, and the change isn't tested and audited which means we need to drop focus from what we're doing and audit and test code that isn't going to be used anyway for not apparent reason? But otoh put in the below context there indeed is a reason.
From a quick audit of the existing code it seems like at least vmw_cursor_update_position is touching global device state so I think at a minimum we need to take a spinlock in that function. Otherwise it seems to be safe.
But I prefer if we can do that as part of the atomic update?
Thanks, Thomas
Bit more context even: This lock dropping dance is _not_ safe from a drm core perspective. But when I've done the original kms locking rework the tradeoff between upsetting core state a bit and totally breaking vmwgfx leaned towards not breaking vmwgfx. And iirc you or Syeh promised to look at this and then either remove the FIXME, maybe with a vmwgfx lock/unlock added if there's a gap (I looked, didn't find one, but I don't understand vmwgfx in details really).
Thanks, Daniel
On Thu, Mar 23, 2017 at 09:35:25AM +0100, Thomas Hellstrom wrote:
Hi, Daniel,
On 03/23/2017 08:31 AM, Daniel Vetter wrote:
On Thu, Mar 23, 2017 at 08:28:32AM +0100, Daniel Vetter wrote:
On Thu, Mar 23, 2017 at 07:22:31AM +0100, Thomas Hellstrom wrote:
On 03/22/2017 10:50 PM, Daniel Vetter wrote:
It's been around forever, no one bothered to address the FIXME, so I presume it's all fine.
Cc: Sinclair Yeh syeh@vmware.com Cc: Thomas Hellstrom thellstrom@vmware.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com
NAK. We need to properly address this. Probably as part of the atomic update.
So could someone with vmwgfx understanding explain this? Note that the FIXME was originally added by me years ago, because I wasn't sure (only about 90%) that this is safe, and was essentially pleading for a vmwgfx expert to review this?
Since it didn't happen I presume it's not that terribly and probably safe ...
I'm still 90% sure that this is correct, but I'd love for a vmwgfx to audit it. Replying with a NAK is kinda not the response I was hoping for (and yes I guess I should have explained what's going on here better, but it's just a git blame of the FIXME comment away).
So the code has been left in place because it works. Altering it now will create unnecessary merge conflicts with the atomic code, and the change isn't tested and audited which means we need to drop focus from what we're doing and audit and test code that isn't going to be used anyway for not apparent reason? But otoh put in the below context there indeed is a reason.
From a quick audit of the existing code it seems like at least vmw_cursor_update_position is touching global device state so I think at a minimum we need to take a spinlock in that function. Otherwise it seems to be safe.
Note that you're holding the crtc lock already, which gives you exclusion against concurrent page_flips, mode_sets and property changes. Note also that page_flips themselves also only hold the crtc lock, so you can run multiple page_flips in parallel on different crtc (iirc vmwgfx has multiple crtc, if not this discussion is entirely moot).
tbh I'd be surprised if my patch really breaks something that hasn't been a pre-existing issue for a long time. The original commit which added this FIXME comment is from 2012. Note also that because it's a hack, you already have a pretty a real race with the core drm state keeping, and no one seems to have hit that either.
I mean I can dig through vmwgfx code and do the audit, but it'll take a few hours and vmwgfx is it's own world, so much harder to understand (for me).
But I prefer if we can do that as part of the atomic update?
When does that vmwgfx atomic happen? -Daniel
On 03/23/2017 11:10 AM, Daniel Vetter wrote:
On Thu, Mar 23, 2017 at 09:35:25AM +0100, Thomas Hellstrom wrote:
Hi, Daniel,
On 03/23/2017 08:31 AM, Daniel Vetter wrote:
On Thu, Mar 23, 2017 at 08:28:32AM +0100, Daniel Vetter wrote:
On Thu, Mar 23, 2017 at 07:22:31AM +0100, Thomas Hellstrom wrote:
On 03/22/2017 10:50 PM, Daniel Vetter wrote:
It's been around forever, no one bothered to address the FIXME, so I presume it's all fine.
Cc: Sinclair Yeh syeh@vmware.com Cc: Thomas Hellstrom thellstrom@vmware.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com
NAK. We need to properly address this. Probably as part of the atomic update.
So could someone with vmwgfx understanding explain this? Note that the FIXME was originally added by me years ago, because I wasn't sure (only about 90%) that this is safe, and was essentially pleading for a vmwgfx expert to review this?
Since it didn't happen I presume it's not that terribly and probably safe ...
I'm still 90% sure that this is correct, but I'd love for a vmwgfx to audit it. Replying with a NAK is kinda not the response I was hoping for (and yes I guess I should have explained what's going on here better, but it's just a git blame of the FIXME comment away).
So the code has been left in place because it works. Altering it now will create unnecessary merge conflicts with the atomic code, and the change isn't tested and audited which means we need to drop focus from what we're doing and audit and test code that isn't going to be used anyway for not apparent reason? But otoh put in the below context there indeed is a reason.
From a quick audit of the existing code it seems like at least vmw_cursor_update_position is touching global device state so I think at a minimum we need to take a spinlock in that function. Otherwise it seems to be safe.
Note that you're holding the crtc lock already, which gives you exclusion against concurrent page_flips, mode_sets and property changes. Note also that page_flips themselves also only hold the crtc lock, so you can run multiple page_flips in parallel on different crtc (iirc vmwgfx has multiple crtc, if not this discussion is entirely moot).
tbh I'd be surprised if my patch really breaks something that hasn't been a pre-existing issue for a long time. The original commit which added this FIXME comment is from 2012. Note also that because it's a hack, you already have a pretty a real race with the core drm state keeping, and no one seems to have hit that either.
I mean I can dig through vmwgfx code and do the audit, but it'll take a few hours and vmwgfx is it's own world, so much harder to understand (for me).
I'm thinking of the situation when someone would call a cursor_set ioctl in parallell for two crtcs at the same time and race writing the position registers? Note that the device has only a single global cursor. Admittedly the effects of a race would probably be small, but I'd rather see it being properly protected.
But I prefer if we can do that as part of the atomic update?
When does that vmwgfx atomic happen? -Daniel
We're targeting 4.12, which means the code that is currently under testing will need to be sent out for review pretty soon. It's already in our standalone testing repo at
git://git.freedesktop.org/git/mesa/vmwgfx
but the cursor code hasn't been fixed in that repo yet.
BTW is this blocking some other core drm work you're doing?
Thanks,
/Thomas
On Thu, Mar 23, 2017 at 11:32:49AM +0100, Thomas Hellstrom wrote:
On 03/23/2017 11:10 AM, Daniel Vetter wrote:
On Thu, Mar 23, 2017 at 09:35:25AM +0100, Thomas Hellstrom wrote:
Hi, Daniel,
On 03/23/2017 08:31 AM, Daniel Vetter wrote:
On Thu, Mar 23, 2017 at 08:28:32AM +0100, Daniel Vetter wrote:
On Thu, Mar 23, 2017 at 07:22:31AM +0100, Thomas Hellstrom wrote:
On 03/22/2017 10:50 PM, Daniel Vetter wrote: > It's been around forever, no one bothered to address the FIXME, so I > presume it's all fine. > > Cc: Sinclair Yeh syeh@vmware.com > Cc: Thomas Hellstrom thellstrom@vmware.com > Signed-off-by: Daniel Vetter daniel.vetter@intel.com NAK. We need to properly address this. Probably as part of the atomic update.
So could someone with vmwgfx understanding explain this? Note that the FIXME was originally added by me years ago, because I wasn't sure (only about 90%) that this is safe, and was essentially pleading for a vmwgfx expert to review this?
Since it didn't happen I presume it's not that terribly and probably safe ...
I'm still 90% sure that this is correct, but I'd love for a vmwgfx to audit it. Replying with a NAK is kinda not the response I was hoping for (and yes I guess I should have explained what's going on here better, but it's just a git blame of the FIXME comment away).
So the code has been left in place because it works. Altering it now will create unnecessary merge conflicts with the atomic code, and the change isn't tested and audited which means we need to drop focus from what we're doing and audit and test code that isn't going to be used anyway for not apparent reason? But otoh put in the below context there indeed is a reason.
From a quick audit of the existing code it seems like at least vmw_cursor_update_position is touching global device state so I think at a minimum we need to take a spinlock in that function. Otherwise it seems to be safe.
Note that you're holding the crtc lock already, which gives you exclusion against concurrent page_flips, mode_sets and property changes. Note also that page_flips themselves also only hold the crtc lock, so you can run multiple page_flips in parallel on different crtc (iirc vmwgfx has multiple crtc, if not this discussion is entirely moot).
tbh I'd be surprised if my patch really breaks something that hasn't been a pre-existing issue for a long time. The original commit which added this FIXME comment is from 2012. Note also that because it's a hack, you already have a pretty a real race with the core drm state keeping, and no one seems to have hit that either.
I mean I can dig through vmwgfx code and do the audit, but it'll take a few hours and vmwgfx is it's own world, so much harder to understand (for me).
I'm thinking of the situation when someone would call a cursor_set ioctl in parallell for two crtcs at the same time and race writing the position registers? Note that the device has only a single global cursor. Admittedly the effects of a race would probably be small, but I'd rather see it being properly protected.
Hm, didn't realize you only have 1 cursor for everything together. In that case you indeed have a problem. Not sure why that didn't come up 4 years ago with the original patch, would be pretty easy to add a quite mutex in v2 ... Since read-only global state is perfectly fine, having the crtc lock gives you a read-only global state lock (for legacy drivers at least, not for atomic).
But I prefer if we can do that as part of the atomic update?
When does that vmwgfx atomic happen?
We're targeting 4.12, which means the code that is currently under testing will need to be sent out for review pretty soon. It's already in our standalone testing repo at
git://git.freedesktop.org/git/mesa/vmwgfx
Deadline is in 2 weeks for 4.12 feature work, per the discussion we've had after the 4.11 merge window fallout with Linus. You pretty much have to submit the patches now to have a reasonable chance of them landing in time. Since vmwgfx has traditionally been the odd kms driver out I'd really like to give the new atomic code at least a quick read-through, to make sure it's aligned as much as possible with the other 20+ atomic drivers.
but the cursor code hasn't been fixed in that repo yet.
Well if you switched to universal planes it's pretty easy to fix with the acquire ctx and grabbing mode_config.connection_mutex. Without that you can just add a global cursor mutex (equally few lines) to patch it up.
BTW is this blocking some other core drm work you're doing?
Just removing lock_crtc and preventing abuse from spreading. Somehow both tegra and tilcdc starting using it in places it was definitely not meant for. vmwgfx (with this FIXME here) was the only legit user of this function. So not high priority really, but something that'd be really nice to remove from the exported set of functions to prevent future misuse by new drivers.
Thanks, Daniel
On 23/03/17 07:32 PM, Thomas Hellstrom wrote:
On 03/23/2017 11:10 AM, Daniel Vetter wrote:
On Thu, Mar 23, 2017 at 09:35:25AM +0100, Thomas Hellstrom wrote:
On 03/23/2017 08:31 AM, Daniel Vetter wrote:
On Thu, Mar 23, 2017 at 08:28:32AM +0100, Daniel Vetter wrote:
On Thu, Mar 23, 2017 at 07:22:31AM +0100, Thomas Hellstrom wrote:
On 03/22/2017 10:50 PM, Daniel Vetter wrote: > It's been around forever, no one bothered to address the FIXME, so I > presume it's all fine. > > Cc: Sinclair Yeh syeh@vmware.com > Cc: Thomas Hellstrom thellstrom@vmware.com > Signed-off-by: Daniel Vetter daniel.vetter@intel.com NAK. We need to properly address this. Probably as part of the atomic update.
So could someone with vmwgfx understanding explain this? Note that the FIXME was originally added by me years ago, because I wasn't sure (only about 90%) that this is safe, and was essentially pleading for a vmwgfx expert to review this?
Since it didn't happen I presume it's not that terribly and probably safe ...
I'm still 90% sure that this is correct, but I'd love for a vmwgfx to audit it. Replying with a NAK is kinda not the response I was hoping for (and yes I guess I should have explained what's going on here better, but it's just a git blame of the FIXME comment away).
So the code has been left in place because it works. Altering it now will create unnecessary merge conflicts with the atomic code, and the change isn't tested and audited which means we need to drop focus from what we're doing and audit and test code that isn't going to be used anyway for not apparent reason? But otoh put in the below context there indeed is a reason.
From a quick audit of the existing code it seems like at least vmw_cursor_update_position is touching global device state so I think at a minimum we need to take a spinlock in that function. Otherwise it seems to be safe.
Note that you're holding the crtc lock already, which gives you exclusion against concurrent page_flips, mode_sets and property changes. Note also that page_flips themselves also only hold the crtc lock, so you can run multiple page_flips in parallel on different crtc (iirc vmwgfx has multiple crtc, if not this discussion is entirely moot).
tbh I'd be surprised if my patch really breaks something that hasn't been a pre-existing issue for a long time. The original commit which added this FIXME comment is from 2012. Note also that because it's a hack, you already have a pretty a real race with the core drm state keeping, and no one seems to have hit that either.
I mean I can dig through vmwgfx code and do the audit, but it'll take a few hours and vmwgfx is it's own world, so much harder to understand (for me).
I'm thinking of the situation when someone would call a cursor_set ioctl in parallell for two crtcs at the same time and race writing the position registers? Note that the device has only a single global cursor. Admittedly the effects of a race would probably be small, but I'd rather see it being properly protected.
Indeed, as long as userspace uses cursor positions (and images) on each CRTC which are consistent with a single cursor in a single framebuffer, it shouldn't matter in which order they write the registers. And if the per-CRTC positions aren't consistent like that, locking won't help either.
Strictly speaking, the (virtual) hardware is too limited to support the legacy KMS cursor API. AFAIR e.g. weston at least used to make use of HW cursors for other surfaces, not sure that's currently the case though.
We discussed this quickly on irc, transcribing.
On Mon, Mar 27, 2017 at 5:01 AM, Michel Dänzer michel@daenzer.net wrote:
Strictly speaking, the (virtual) hardware is too limited to support the legacy KMS cursor API. AFAIR e.g. weston at least used to make use of HW cursors for other surfaces, not sure that's currently the case though.
That was disabled again because of lack of atomic (together with all overlay support if your driver isn't atomic). But atomic/universal planes allows us to at least model vmwgfx correctly. For each crtc we'd have one primary plane, but only one global cursor plane that we attach to the cursor slot of each crtc. Then universal/atomic aware userspace could realize that there's only 1 cursor plane and make sure it's not over-used. -Daniel
On 03/27/2017 08:28 AM, Daniel Vetter wrote:
We discussed this quickly on irc, transcribing.
On Mon, Mar 27, 2017 at 5:01 AM, Michel Dänzer michel@daenzer.net wrote:
Strictly speaking, the (virtual) hardware is too limited to support the legacy KMS cursor API. AFAIR e.g. weston at least used to make use of HW cursors for other surfaces, not sure that's currently the case though.
That was disabled again because of lack of atomic (together with all overlay support if your driver isn't atomic). But atomic/universal planes allows us to at least model vmwgfx correctly. For each crtc we'd have one primary plane, but only one global cursor plane that we attach to the cursor slot of each crtc. Then universal/atomic aware userspace could realize that there's only 1 cursor plane and make sure it's not over-used.
That sounds encouraging. In practice we haven't really seen any problems because most users use vmware tools, which places the outputs in such a way that the cursor location visually coincides for all crtcs. The problem starts if someone would override tools and try to clone the contents across crtcs. The vmware xorg driver has some logic to try to detect such situations and fall back to software cursors, and possibly we might have to, at some point, implement software cursor composition in the kernel, but for now we live with the potential possibilty that users will see the cursor jumping across the screens..
/Thomas
-Daniel
On Mon, Mar 27, 2017 at 10:31:51AM +0200, Thomas Hellstrom wrote:
On 03/27/2017 08:28 AM, Daniel Vetter wrote:
We discussed this quickly on irc, transcribing.
On Mon, Mar 27, 2017 at 5:01 AM, Michel Dänzer michel@daenzer.net wrote:
Strictly speaking, the (virtual) hardware is too limited to support the legacy KMS cursor API. AFAIR e.g. weston at least used to make use of HW cursors for other surfaces, not sure that's currently the case though.
That was disabled again because of lack of atomic (together with all overlay support if your driver isn't atomic). But atomic/universal planes allows us to at least model vmwgfx correctly. For each crtc we'd have one primary plane, but only one global cursor plane that we attach to the cursor slot of each crtc. Then universal/atomic aware userspace could realize that there's only 1 cursor plane and make sure it's not over-used.
That sounds encouraging. In practice we haven't really seen any problems because most users use vmware tools, which places the outputs in such a way that the cursor location visually coincides for all crtcs. The problem starts if someone would override tools and try to clone the contents across crtcs. The vmware xorg driver has some logic to try to detect such situations and fall back to software cursors, and possibly we might have to, at some point, implement software cursor composition in the kernel, but for now we live with the potential possibilty that users will see the cursor jumping across the screens..
Ok, I've pulled in the series, except this patch plus the few cleanups that depend upon it. I'll respin this as soon as vmwgfx atomic has landed, with either a local mutex (if you still have more sw cursor planes than real ones) or no changes (if your universal cursor code is fixed to only have one cursor for the entire device instance).
Thanks, Daniel
On 03/29/2017 10:00 AM, Daniel Vetter wrote:
On Mon, Mar 27, 2017 at 10:31:51AM +0200, Thomas Hellstrom wrote:
On 03/27/2017 08:28 AM, Daniel Vetter wrote:
We discussed this quickly on irc, transcribing.
On Mon, Mar 27, 2017 at 5:01 AM, Michel Dänzer michel@daenzer.net wrote:
Strictly speaking, the (virtual) hardware is too limited to support the legacy KMS cursor API. AFAIR e.g. weston at least used to make use of HW cursors for other surfaces, not sure that's currently the case though.
That was disabled again because of lack of atomic (together with all overlay support if your driver isn't atomic). But atomic/universal planes allows us to at least model vmwgfx correctly. For each crtc we'd have one primary plane, but only one global cursor plane that we attach to the cursor slot of each crtc. Then universal/atomic aware userspace could realize that there's only 1 cursor plane and make sure it's not over-used.
That sounds encouraging. In practice we haven't really seen any problems because most users use vmware tools, which places the outputs in such a way that the cursor location visually coincides for all crtcs. The problem starts if someone would override tools and try to clone the contents across crtcs. The vmware xorg driver has some logic to try to detect such situations and fall back to software cursors, and possibly we might have to, at some point, implement software cursor composition in the kernel, but for now we live with the potential possibilty that users will see the cursor jumping across the screens..
Ok, I've pulled in the series, except this patch plus the few cleanups that depend upon it. I'll respin this as soon as vmwgfx atomic has landed, with either a local mutex (if you still have more sw cursor planes than real ones) or no changes (if your universal cursor code is fixed to only have one cursor for the entire device instance).
Thanks, Daniel
Thanks,
In the patch series we have added a local spinlock (cursor_lock) to protect from concurrent register access.
/Thomas
Yes the help text is unhelpful, but atomic drivers should never use this. Just grab the lock without context or anything.
Also an aside: Checking ->active like this doesn't protect against nonblocking commits, this is rather bogus.
Cc: Thierry Reding thierry.reding@gmail.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/tegra/dc.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c index 0db5d5a8d3b9..95b373f739f2 100644 --- a/drivers/gpu/drm/tegra/dc.c +++ b/drivers/gpu/drm/tegra/dc.c @@ -1382,7 +1382,7 @@ static int tegra_dc_show_regs(struct seq_file *s, void *data) struct tegra_dc *dc = node->info_ent->data; int err = 0;
- drm_modeset_lock_crtc(&dc->base, NULL); + drm_modeset_lock(&dc->base.mutex, NULL);
if (!dc->base.state->active) { err = -EBUSY; @@ -1609,7 +1609,7 @@ static int tegra_dc_show_regs(struct seq_file *s, void *data) #undef DUMP_REG
unlock: - drm_modeset_unlock_crtc(&dc->base); + drm_modeset_unlock(&dc->base.mutex); return err; }
@@ -1620,7 +1620,7 @@ static int tegra_dc_show_crc(struct seq_file *s, void *data) int err = 0; u32 value;
- drm_modeset_lock_crtc(&dc->base, NULL); + drm_modeset_lock(&dc->base.mutex, NULL);
if (!dc->base.state->active) { err = -EBUSY; @@ -1640,7 +1640,7 @@ static int tegra_dc_show_crc(struct seq_file *s, void *data) tegra_dc_writel(dc, 0, DC_COM_CRC_CONTROL);
unlock: - drm_modeset_unlock_crtc(&dc->base); + drm_modeset_unlock(&dc->base.mutex); return err; }
On Wed, Mar 22, 2017 at 10:50:46PM +0100, Daniel Vetter wrote:
Yes the help text is unhelpful, but atomic drivers should never use this. Just grab the lock without context or anything.
Also an aside: Checking ->active like this doesn't protect against nonblocking commits, this is rather bogus.
Cc: Thierry Reding thierry.reding@gmail.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com
Applied to drm-misc with Thierry's irc-ack. -Daniel
drivers/gpu/drm/tegra/dc.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c index 0db5d5a8d3b9..95b373f739f2 100644 --- a/drivers/gpu/drm/tegra/dc.c +++ b/drivers/gpu/drm/tegra/dc.c @@ -1382,7 +1382,7 @@ static int tegra_dc_show_regs(struct seq_file *s, void *data) struct tegra_dc *dc = node->info_ent->data; int err = 0;
- drm_modeset_lock_crtc(&dc->base, NULL);
drm_modeset_lock(&dc->base.mutex, NULL);
if (!dc->base.state->active) { err = -EBUSY;
@@ -1609,7 +1609,7 @@ static int tegra_dc_show_regs(struct seq_file *s, void *data) #undef DUMP_REG
unlock:
- drm_modeset_unlock_crtc(&dc->base);
- drm_modeset_unlock(&dc->base.mutex); return err;
}
@@ -1620,7 +1620,7 @@ static int tegra_dc_show_crc(struct seq_file *s, void *data) int err = 0; u32 value;
- drm_modeset_lock_crtc(&dc->base, NULL);
drm_modeset_lock(&dc->base.mutex, NULL);
if (!dc->base.state->active) { err = -EBUSY;
@@ -1640,7 +1640,7 @@ static int tegra_dc_show_crc(struct seq_file *s, void *data) tegra_dc_writel(dc, 0, DC_COM_CRC_CONTROL);
unlock:
- drm_modeset_unlock_crtc(&dc->base);
- drm_modeset_unlock(&dc->base.mutex); return err;
}
-- 2.11.0
Again this is an internal helper, not the official way to lock a crtc.
Cc: Jyri Sarha jsarha@ti.com Cc: Tomi Valkeinen tomi.valkeinen@ti.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c index c92faa8f7560..afd2a7b2aff7 100644 --- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c +++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c @@ -579,7 +579,7 @@ static void tilcdc_crtc_recover_work(struct work_struct *work)
dev_info(crtc->dev->dev, "%s: Reset CRTC", __func__);
- drm_modeset_lock_crtc(crtc, NULL); + drm_modeset_lock(&crtc->mutex, NULL);
if (!tilcdc_crtc_is_on(crtc)) goto out; @@ -587,7 +587,7 @@ static void tilcdc_crtc_recover_work(struct work_struct *work) tilcdc_crtc_disable(crtc); tilcdc_crtc_enable(crtc); out: - drm_modeset_unlock_crtc(crtc); + drm_modeset_unlock(&crtc->mutex); }
static void tilcdc_crtc_destroy(struct drm_crtc *crtc) @@ -595,9 +595,9 @@ static void tilcdc_crtc_destroy(struct drm_crtc *crtc) struct tilcdc_crtc *tilcdc_crtc = to_tilcdc_crtc(crtc); struct tilcdc_drm_private *priv = crtc->dev->dev_private;
- drm_modeset_lock_crtc(crtc, NULL); + drm_modeset_lock(&crtc->mutex, NULL); tilcdc_crtc_disable(crtc); - drm_modeset_unlock_crtc(crtc); + drm_modeset_unlock(&crtc->mutex);
flush_workqueue(priv->wq);
@@ -856,7 +856,7 @@ void tilcdc_crtc_update_clk(struct drm_crtc *crtc) struct tilcdc_drm_private *priv = dev->dev_private; struct tilcdc_crtc *tilcdc_crtc = to_tilcdc_crtc(crtc);
- drm_modeset_lock_crtc(crtc, NULL); + drm_modeset_lock(&crtc->mutex, NULL); if (tilcdc_crtc->lcd_fck_rate != clk_get_rate(priv->clk)) { if (tilcdc_crtc_is_on(crtc)) { pm_runtime_get_sync(dev->dev); @@ -868,7 +868,7 @@ void tilcdc_crtc_update_clk(struct drm_crtc *crtc) pm_runtime_put_sync(dev->dev); } } - drm_modeset_unlock_crtc(crtc); + drm_modeset_unlock(&crtc->mutex); }
#define SYNC_LOST_COUNT_LIMIT 50
On 22/03/17 23:50, Daniel Vetter wrote:
Again this is an internal helper, not the official way to lock a crtc.
Cc: Jyri Sarha jsarha@ti.com Cc: Tomi Valkeinen tomi.valkeinen@ti.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com
drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
Reviewed-by: Tomi Valkeinen tomi.valkeinen@ti.com
Tomi
On Fri, Mar 24, 2017 at 11:46:53AM +0200, Tomi Valkeinen wrote:
On 22/03/17 23:50, Daniel Vetter wrote:
Again this is an internal helper, not the official way to lock a crtc.
Cc: Jyri Sarha jsarha@ti.com Cc: Tomi Valkeinen tomi.valkeinen@ti.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com
drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
Reviewed-by: Tomi Valkeinen tomi.valkeinen@ti.com
Pushed to drm-misc, thanks for the review. -Daniel
On 03/22/17 23:50, Daniel Vetter wrote:
Again this is an internal helper, not the official way to lock a crtc.
Cc: Jyri Sarha jsarha@ti.com Cc: Tomi Valkeinen tomi.valkeinen@ti.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com
Acked-by: Jyri Sarha jsarha@ti.com
drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c index c92faa8f7560..afd2a7b2aff7 100644 --- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c +++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c @@ -579,7 +579,7 @@ static void tilcdc_crtc_recover_work(struct work_struct *work)
dev_info(crtc->dev->dev, "%s: Reset CRTC", __func__);
- drm_modeset_lock_crtc(crtc, NULL);
drm_modeset_lock(&crtc->mutex, NULL);
if (!tilcdc_crtc_is_on(crtc)) goto out;
@@ -587,7 +587,7 @@ static void tilcdc_crtc_recover_work(struct work_struct *work) tilcdc_crtc_disable(crtc); tilcdc_crtc_enable(crtc); out:
- drm_modeset_unlock_crtc(crtc);
- drm_modeset_unlock(&crtc->mutex);
}
static void tilcdc_crtc_destroy(struct drm_crtc *crtc) @@ -595,9 +595,9 @@ static void tilcdc_crtc_destroy(struct drm_crtc *crtc) struct tilcdc_crtc *tilcdc_crtc = to_tilcdc_crtc(crtc); struct tilcdc_drm_private *priv = crtc->dev->dev_private;
- drm_modeset_lock_crtc(crtc, NULL);
- drm_modeset_lock(&crtc->mutex, NULL); tilcdc_crtc_disable(crtc);
- drm_modeset_unlock_crtc(crtc);
drm_modeset_unlock(&crtc->mutex);
flush_workqueue(priv->wq);
@@ -856,7 +856,7 @@ void tilcdc_crtc_update_clk(struct drm_crtc *crtc) struct tilcdc_drm_private *priv = dev->dev_private; struct tilcdc_crtc *tilcdc_crtc = to_tilcdc_crtc(crtc);
- drm_modeset_lock_crtc(crtc, NULL);
- drm_modeset_lock(&crtc->mutex, NULL); if (tilcdc_crtc->lcd_fck_rate != clk_get_rate(priv->clk)) { if (tilcdc_crtc_is_on(crtc)) { pm_runtime_get_sync(dev->dev);
@@ -868,7 +868,7 @@ void tilcdc_crtc_update_clk(struct drm_crtc *crtc) pm_runtime_put_sync(dev->dev); } }
- drm_modeset_unlock_crtc(crtc);
- drm_modeset_unlock(&crtc->mutex);
}
#define SYNC_LOST_COUNT_LIMIT 50
This is only for legacy paths that need to grab the crtc/plane lock combo. If you want to lock a crtc, just use drm_modeset_lock().
Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/drm_crtc_internal.h | 3 +++ drivers/gpu/drm/drm_modeset_lock.c | 14 -------------- include/drm/drm_modeset_lock.h | 2 -- 3 files changed, 3 insertions(+), 16 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc_internal.h b/drivers/gpu/drm/drm_crtc_internal.h index 8c04275cf226..de1047530e07 100644 --- a/drivers/gpu/drm/drm_crtc_internal.h +++ b/drivers/gpu/drm/drm_crtc_internal.h @@ -61,6 +61,9 @@ int drm_mode_getresources(struct drm_device *dev, void *data, struct drm_file *file_priv);
+/* drm_modeset_lock.c */ +void drm_modeset_lock_crtc(struct drm_crtc *crtc, + struct drm_plane *plane); /* drm_dumb_buffers.c */ /* IOCTLs */ int drm_mode_create_dumb_ioctl(struct drm_device *dev, diff --git a/drivers/gpu/drm/drm_modeset_lock.c b/drivers/gpu/drm/drm_modeset_lock.c index bf60f2645e55..c94eff9d7544 100644 --- a/drivers/gpu/drm/drm_modeset_lock.c +++ b/drivers/gpu/drm/drm_modeset_lock.c @@ -148,19 +148,6 @@ void drm_modeset_unlock_all(struct drm_device *dev) } EXPORT_SYMBOL(drm_modeset_unlock_all);
-/** - * drm_modeset_lock_crtc - lock crtc with hidden acquire ctx for a plane update - * @crtc: DRM CRTC - * @plane: DRM plane to be updated on @crtc - * - * This function locks the given crtc and plane (which should be either the - * primary or cursor plane) using a hidden acquire context. This is necessary so - * that drivers internally using the atomic interfaces can grab further locks - * with the lock acquire context. - * - * Note that @plane can be NULL, e.g. when the cursor support hasn't yet been - * converted to universal planes yet. - */ void drm_modeset_lock_crtc(struct drm_crtc *crtc, struct drm_plane *plane) { @@ -205,7 +192,6 @@ void drm_modeset_lock_crtc(struct drm_crtc *crtc, goto retry; } } -EXPORT_SYMBOL(drm_modeset_lock_crtc);
/** * drm_modeset_legacy_acquire_ctx - find acquire ctx for legacy ioctls diff --git a/include/drm/drm_modeset_lock.h b/include/drm/drm_modeset_lock.h index 96d39fbd12ca..88d35bfc9cd8 100644 --- a/include/drm/drm_modeset_lock.h +++ b/include/drm/drm_modeset_lock.h @@ -121,8 +121,6 @@ struct drm_plane;
void drm_modeset_lock_all(struct drm_device *dev); void drm_modeset_unlock_all(struct drm_device *dev); -void drm_modeset_lock_crtc(struct drm_crtc *crtc, - struct drm_plane *plane); void drm_modeset_unlock_crtc(struct drm_crtc *crtc); void drm_warn_on_modeset_not_all_locked(struct drm_device *dev); struct drm_modeset_acquire_ctx *
Again just prep work.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/drm_plane.c | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c index 8535dc17db7e..62e833ffeffd 100644 --- a/drivers/gpu/drm/drm_plane.c +++ b/drivers/gpu/drm/drm_plane.c @@ -803,6 +803,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev, struct drm_framebuffer *fb = NULL; struct drm_pending_vblank_event *e = NULL; u32 target_vblank = page_flip->sequence; + struct drm_modeset_acquire_ctx ctx; int ret = -EINVAL;
if (!drm_core_check_feature(dev, DRIVER_MODESET)) @@ -866,7 +867,16 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev, return -EINVAL; }
- drm_modeset_lock_crtc(crtc, crtc->primary); + drm_modeset_acquire_init(&ctx, 0); +retry: + ret = drm_modeset_lock(&crtc->mutex, &ctx); + if (ret) + goto out; + ret = drm_modeset_lock(&crtc->cursor->mutex, &ctx); + if (ret) + goto out; + crtc->acquire_ctx = &ctx; + if (crtc->primary->fb == NULL) { /* The framebuffer is currently unbound, presumably * due to a hotplug event, that userspace has not @@ -944,7 +954,14 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev, if (crtc->primary->old_fb) drm_framebuffer_put(crtc->primary->old_fb); crtc->primary->old_fb = NULL; - drm_modeset_unlock_crtc(crtc); + + if (ret == -EDEADLK) { + drm_modeset_backoff(&ctx); + goto retry; + } + + drm_modeset_drop_locks(&ctx); + drm_modeset_acquire_fini(&ctx);
return ret; }
Again just going through the motions, no functional changes in here.
Cc: Gerd Hoffmann kraxel@redhat.com Cc: Ben Skeggs bskeggs@redhat.com Cc: Russell King linux@armlinux.org.uk Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com Cc: Eric Anholt eric@anholt.net Cc: Alex Deucher alexander.deucher@amd.com Cc: Christian König christian.koenig@amd.com Signed-off-by: Daniel Vetter daniel.vetter@intel.comt --- drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 3 ++- drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h | 3 ++- drivers/gpu/drm/armada/armada_crtc.c | 3 ++- drivers/gpu/drm/bochs/bochs_kms.c | 3 ++- drivers/gpu/drm/drm_atomic_helper.c | 8 ++++++-- drivers/gpu/drm/drm_plane.c | 6 ++++-- drivers/gpu/drm/nouveau/nouveau_display.c | 3 ++- drivers/gpu/drm/nouveau/nouveau_display.h | 3 ++- drivers/gpu/drm/radeon/radeon_display.c | 3 ++- drivers/gpu/drm/shmobile/shmob_drm_crtc.c | 3 ++- drivers/gpu/drm/udl/udl_modeset.c | 3 ++- drivers/gpu/drm/vc4/vc4_crtc.c | 5 +++-- drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c | 3 ++- drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c | 3 ++- include/drm/drm_atomic_helper.h | 6 ++++-- include/drm/drm_crtc.h | 6 ++++-- 16 files changed, 43 insertions(+), 21 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c index 39fc388f222a..7b4fe91d3aec 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c @@ -311,7 +311,8 @@ int amdgpu_crtc_page_flip_target(struct drm_crtc *crtc, struct drm_framebuffer *fb, struct drm_pending_vblank_event *event, uint32_t page_flip_flags, - uint32_t target) + uint32_t target, + struct drm_modeset_acquire_ctx *ctx) { struct amdgpu_bo *new_abo; struct amdgpu_flip_work *work; diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h index 6b8f766a6a35..d19b803ba509 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h @@ -597,7 +597,8 @@ int amdgpu_crtc_set_config(struct drm_mode_set *set); int amdgpu_crtc_page_flip_target(struct drm_crtc *crtc, struct drm_framebuffer *fb, struct drm_pending_vblank_event *event, - uint32_t page_flip_flags, uint32_t target); + uint32_t page_flip_flags, uint32_t target, + struct drm_modeset_acquire_ctx *ctx); void amdgpu_crtc_cleanup_flip_ctx(struct amdgpu_flip_work *work, struct amdgpu_bo *new_abo); int amdgpu_crtc_prepare_flip(struct drm_crtc *crtc, diff --git a/drivers/gpu/drm/armada/armada_crtc.c b/drivers/gpu/drm/armada/armada_crtc.c index 1341e0b9368a..4fe19fde84f9 100644 --- a/drivers/gpu/drm/armada/armada_crtc.c +++ b/drivers/gpu/drm/armada/armada_crtc.c @@ -1027,7 +1027,8 @@ static void armada_drm_crtc_destroy(struct drm_crtc *crtc) * and a mode_set. */ static int armada_drm_crtc_page_flip(struct drm_crtc *crtc, - struct drm_framebuffer *fb, struct drm_pending_vblank_event *event, uint32_t page_flip_flags) + struct drm_framebuffer *fb, struct drm_pending_vblank_event *event, uint32_t page_flip_flags, + struct drm_modeset_acquire_ctx *ctx) { struct armada_crtc *dcrtc = drm_to_armada_crtc(crtc); struct armada_frame_work *work; diff --git a/drivers/gpu/drm/bochs/bochs_kms.c b/drivers/gpu/drm/bochs/bochs_kms.c index d5e63eff357b..6a91e62da2f4 100644 --- a/drivers/gpu/drm/bochs/bochs_kms.c +++ b/drivers/gpu/drm/bochs/bochs_kms.c @@ -96,7 +96,8 @@ static void bochs_crtc_commit(struct drm_crtc *crtc) static int bochs_crtc_page_flip(struct drm_crtc *crtc, struct drm_framebuffer *fb, struct drm_pending_vblank_event *event, - uint32_t page_flip_flags) + uint32_t page_flip_flags, + struct drm_modeset_acquire_ctx *ctx) { struct bochs_device *bochs = container_of(crtc, struct bochs_device, crtc); diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 845378c92ccb..0e65ce4497b3 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -2860,6 +2860,7 @@ static int page_flip_common( * @fb: DRM framebuffer * @event: optional DRM event to signal upon completion * @flags: flip flags for non-vblank sync'ed updates + * @ctx: lock acquisition context * * Provides a default &drm_crtc_funcs.page_flip implementation * using the atomic driver interface. @@ -2873,7 +2874,8 @@ static int page_flip_common( int drm_atomic_helper_page_flip(struct drm_crtc *crtc, struct drm_framebuffer *fb, struct drm_pending_vblank_event *event, - uint32_t flags) + uint32_t flags, + struct drm_modeset_acquire_ctx *ctx) { struct drm_plane *plane = crtc->primary; struct drm_atomic_state *state; @@ -2921,6 +2923,7 @@ EXPORT_SYMBOL(drm_atomic_helper_page_flip); * @event: optional DRM event to signal upon completion * @flags: flip flags for non-vblank sync'ed updates * @target: specifying the target vblank period when the flip to take effect + * @ctx: lock acquisition context * * Provides a default &drm_crtc_funcs.page_flip_target implementation. * Similar to drm_atomic_helper_page_flip() with extra parameter to specify @@ -2934,7 +2937,8 @@ int drm_atomic_helper_page_flip_target( struct drm_framebuffer *fb, struct drm_pending_vblank_event *event, uint32_t flags, - uint32_t target) + uint32_t target, + struct drm_modeset_acquire_ctx *ctx) { struct drm_plane *plane = crtc->primary; struct drm_atomic_state *state; diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c index 62e833ffeffd..373e980d698d 100644 --- a/drivers/gpu/drm/drm_plane.c +++ b/drivers/gpu/drm/drm_plane.c @@ -932,9 +932,11 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev, if (crtc->funcs->page_flip_target) ret = crtc->funcs->page_flip_target(crtc, fb, e, page_flip->flags, - target_vblank); + target_vblank, + &ctx); else - ret = crtc->funcs->page_flip(crtc, fb, e, page_flip->flags); + ret = crtc->funcs->page_flip(crtc, fb, e, page_flip->flags, + &ctx); if (ret) { if (page_flip->flags & DRM_MODE_PAGE_FLIP_EVENT) drm_event_cancel_free(dev, &e->base); diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c b/drivers/gpu/drm/nouveau/nouveau_display.c index b5a92386cc8e..d4468d12ee73 100644 --- a/drivers/gpu/drm/nouveau/nouveau_display.c +++ b/drivers/gpu/drm/nouveau/nouveau_display.c @@ -769,7 +769,8 @@ nouveau_page_flip_emit(struct nouveau_channel *chan,
int nouveau_crtc_page_flip(struct drm_crtc *crtc, struct drm_framebuffer *fb, - struct drm_pending_vblank_event *event, u32 flags) + struct drm_pending_vblank_event *event, u32 flags, + struct drm_modeset_acquire_ctx *ctx) { const int swap_interval = (flags & DRM_MODE_PAGE_FLIP_ASYNC) ? 0 : 1; struct drm_device *dev = crtc->dev; diff --git a/drivers/gpu/drm/nouveau/nouveau_display.h b/drivers/gpu/drm/nouveau/nouveau_display.h index b76b65c7c8da..b28426e02adf 100644 --- a/drivers/gpu/drm/nouveau/nouveau_display.h +++ b/drivers/gpu/drm/nouveau/nouveau_display.h @@ -74,7 +74,8 @@ bool nouveau_display_scanoutpos(struct drm_device *, unsigned int,
int nouveau_crtc_page_flip(struct drm_crtc *crtc, struct drm_framebuffer *fb, struct drm_pending_vblank_event *event, - uint32_t page_flip_flags); + uint32_t page_flip_flags, + struct drm_modeset_acquire_ctx *ctx); int nouveau_finish_page_flip(struct nouveau_channel *, struct nouveau_page_flip_state *);
diff --git a/drivers/gpu/drm/radeon/radeon_display.c b/drivers/gpu/drm/radeon/radeon_display.c index aea8b62835a4..31020db573d5 100644 --- a/drivers/gpu/drm/radeon/radeon_display.c +++ b/drivers/gpu/drm/radeon/radeon_display.c @@ -485,7 +485,8 @@ static int radeon_crtc_page_flip_target(struct drm_crtc *crtc, struct drm_framebuffer *fb, struct drm_pending_vblank_event *event, uint32_t page_flip_flags, - uint32_t target) + uint32_t target, + struct drm_modeset_acquire_ctx *ctx) { struct drm_device *dev = crtc->dev; struct radeon_device *rdev = dev->dev_private; diff --git a/drivers/gpu/drm/shmobile/shmob_drm_crtc.c b/drivers/gpu/drm/shmobile/shmob_drm_crtc.c index 5fcabc04f307..e7738939a86d 100644 --- a/drivers/gpu/drm/shmobile/shmob_drm_crtc.c +++ b/drivers/gpu/drm/shmobile/shmob_drm_crtc.c @@ -449,7 +449,8 @@ void shmob_drm_crtc_finish_page_flip(struct shmob_drm_crtc *scrtc) static int shmob_drm_crtc_page_flip(struct drm_crtc *crtc, struct drm_framebuffer *fb, struct drm_pending_vblank_event *event, - uint32_t page_flip_flags) + uint32_t page_flip_flags, + struct drm_modeset_acquire_ctx *ctx) { struct shmob_drm_crtc *scrtc = to_shmob_crtc(crtc); struct drm_device *dev = scrtc->crtc.dev; diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c index f2b2481cad52..5bcae7649795 100644 --- a/drivers/gpu/drm/udl/udl_modeset.c +++ b/drivers/gpu/drm/udl/udl_modeset.c @@ -361,7 +361,8 @@ static void udl_crtc_destroy(struct drm_crtc *crtc) static int udl_crtc_page_flip(struct drm_crtc *crtc, struct drm_framebuffer *fb, struct drm_pending_vblank_event *event, - uint32_t page_flip_flags) + uint32_t page_flip_flags, + struct drm_modeset_acquire_ctx *ctx) { struct udl_framebuffer *ufb = to_udl_fb(fb); struct drm_device *dev = crtc->dev; diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c index e871862a21b8..9a12750ec996 100644 --- a/drivers/gpu/drm/vc4/vc4_crtc.c +++ b/drivers/gpu/drm/vc4/vc4_crtc.c @@ -793,12 +793,13 @@ static int vc4_async_page_flip(struct drm_crtc *crtc, static int vc4_page_flip(struct drm_crtc *crtc, struct drm_framebuffer *fb, struct drm_pending_vblank_event *event, - uint32_t flags) + uint32_t flags, + struct drm_modeset_acquire_ctx *ctx) { if (flags & DRM_MODE_PAGE_FLIP_ASYNC) return vc4_async_page_flip(crtc, fb, event, flags); else - return drm_atomic_helper_page_flip(crtc, fb, event, flags); + return drm_atomic_helper_page_flip(crtc, fb, event, flags, ctx); }
static struct drm_crtc_state *vc4_crtc_duplicate_state(struct drm_crtc *crtc) diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c b/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c index d4268efc37d2..53cf3be7a902 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c @@ -395,7 +395,8 @@ static int vmw_sou_crtc_set_config(struct drm_mode_set *set) static int vmw_sou_crtc_page_flip(struct drm_crtc *crtc, struct drm_framebuffer *fb, struct drm_pending_vblank_event *event, - uint32_t flags) + uint32_t flags, + struct drm_modeset_acquire_ctx *ctx) { struct vmw_private *dev_priv = vmw_priv(crtc->dev); struct drm_framebuffer *old_fb = crtc->primary->fb; diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c index b27cd18ee66a..85e12309cb71 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c @@ -649,7 +649,8 @@ static int vmw_stdu_crtc_set_config(struct drm_mode_set *set) static int vmw_stdu_crtc_page_flip(struct drm_crtc *crtc, struct drm_framebuffer *new_fb, struct drm_pending_vblank_event *event, - uint32_t flags) + uint32_t flags, + struct drm_modeset_acquire_ctx *ctx)
{ struct vmw_private *dev_priv = vmw_priv(crtc->dev); diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h index 73554fff086a..9675cacb72a3 100644 --- a/include/drm/drm_atomic_helper.h +++ b/include/drm/drm_atomic_helper.h @@ -125,13 +125,15 @@ int drm_atomic_helper_connector_set_property(struct drm_connector *connector, int drm_atomic_helper_page_flip(struct drm_crtc *crtc, struct drm_framebuffer *fb, struct drm_pending_vblank_event *event, - uint32_t flags); + uint32_t flags, + struct drm_modeset_acquire_ctx *ctx); int drm_atomic_helper_page_flip_target( struct drm_crtc *crtc, struct drm_framebuffer *fb, struct drm_pending_vblank_event *event, uint32_t flags, - uint32_t target); + uint32_t target, + struct drm_modeset_acquire_ctx *ctx); int drm_atomic_helper_connector_dpms(struct drm_connector *connector, int mode); struct drm_encoder * diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 24dcb121bad4..fe42a57393df 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -405,7 +405,8 @@ struct drm_crtc_funcs { int (*page_flip)(struct drm_crtc *crtc, struct drm_framebuffer *fb, struct drm_pending_vblank_event *event, - uint32_t flags); + uint32_t flags, + struct drm_modeset_acquire_ctx *ctx);
/** * @page_flip_target: @@ -423,7 +424,8 @@ struct drm_crtc_funcs { int (*page_flip_target)(struct drm_crtc *crtc, struct drm_framebuffer *fb, struct drm_pending_vblank_event *event, - uint32_t flags, uint32_t target); + uint32_t flags, uint32_t target, + struct drm_modeset_acquire_ctx *ctx);
/** * @set_property:
Yay, we can now properly retry in case of deadlocks or whatever!
Also don't forget to remove the transitional crtc->acquire_ctx assignment again.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/drm_atomic_helper.c | 40 ++----------------------------------- drivers/gpu/drm/drm_plane.c | 1 - 2 files changed, 2 insertions(+), 39 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 0e65ce4497b3..c0ec763ec538 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -2885,34 +2885,16 @@ int drm_atomic_helper_page_flip(struct drm_crtc *crtc, if (!state) return -ENOMEM;
- state->acquire_ctx = drm_modeset_legacy_acquire_ctx(crtc); + state->acquire_ctx = ctx;
-retry: ret = page_flip_common(state, crtc, fb, event, flags); if (ret != 0) goto fail;
ret = drm_atomic_nonblocking_commit(state); - fail: - if (ret == -EDEADLK) - goto backoff; - drm_atomic_state_put(state); return ret; - -backoff: - drm_atomic_state_clear(state); - drm_atomic_legacy_backoff(state); - - /* - * Someone might have exchanged the framebuffer while we dropped locks - * in the backoff code. We need to fix up the fb refcount tracking the - * core does for us. - */ - plane->old_fb = plane->fb; - - goto retry; } EXPORT_SYMBOL(drm_atomic_helper_page_flip);
@@ -2949,9 +2931,8 @@ int drm_atomic_helper_page_flip_target( if (!state) return -ENOMEM;
- state->acquire_ctx = drm_modeset_legacy_acquire_ctx(crtc); + state->acquire_ctx = ctx;
-retry: ret = page_flip_common(state, crtc, fb, event, flags); if (ret != 0) goto fail; @@ -2964,26 +2945,9 @@ int drm_atomic_helper_page_flip_target( crtc_state->target_vblank = target;
ret = drm_atomic_nonblocking_commit(state); - fail: - if (ret == -EDEADLK) - goto backoff; - drm_atomic_state_put(state); return ret; - -backoff: - drm_atomic_state_clear(state); - drm_atomic_legacy_backoff(state); - - /* - * Someone might have exchanged the framebuffer while we dropped locks - * in the backoff code. We need to fix up the fb refcount tracking the - * core does for us. - */ - plane->old_fb = plane->fb; - - goto retry; } EXPORT_SYMBOL(drm_atomic_helper_page_flip_target);
diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c index 373e980d698d..ec3e2e757800 100644 --- a/drivers/gpu/drm/drm_plane.c +++ b/drivers/gpu/drm/drm_plane.c @@ -875,7 +875,6 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev, ret = drm_modeset_lock(&crtc->cursor->mutex, &ctx); if (ret) goto out; - crtc->acquire_ctx = &ctx;
if (crtc->primary->fb == NULL) { /* The framebuffer is currently unbound, presumably
No need to grab both plane and crtc locks at the same time, we can do them one after the other. If userspace races it'll get what it deserves either way.
This removes another user of drm_modeset_lock_crtc. There's only one left.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/drm_crtc.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 660b4c8715de..df1ff0b8818b 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -406,16 +406,18 @@ int drm_mode_getcrtc(struct drm_device *dev, if (!crtc) return -ENOENT;
- drm_modeset_lock_crtc(crtc, crtc->primary); crtc_resp->gamma_size = crtc->gamma_size;
+ drm_modeset_lock(&crtc->primary->mutex, NULL); if (crtc->primary->state && crtc->primary->state->fb) crtc_resp->fb_id = crtc->primary->state->fb->base.id; else if (!crtc->primary->state && crtc->primary->fb) crtc_resp->fb_id = crtc->primary->fb->base.id; else crtc_resp->fb_id = 0; + drm_modeset_unlock(&crtc->primary->mutex);
+ drm_modeset_lock(&crtc->mutex, NULL); if (crtc->state) { crtc_resp->x = crtc->primary->state->src_x >> 16; crtc_resp->y = crtc->primary->state->src_y >> 16; @@ -437,7 +439,7 @@ int drm_mode_getcrtc(struct drm_device *dev, crtc_resp->mode_valid = 0; } } - drm_modeset_unlock_crtc(crtc); + drm_modeset_unlock(&crtc->mutex);
return 0; }
On Wednesday, March 22, 2017 10:50:52 PM EDT Daniel Vetter wrote:
No need to grab both plane and crtc locks at the same time, we can do them one after the other. If userspace races it'll get what it deserves either way.
This removes another user of drm_modeset_lock_crtc. There's only one left.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com
drivers/gpu/drm/drm_crtc.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 660b4c8715de..df1ff0b8818b 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -406,16 +406,18 @@ int drm_mode_getcrtc(struct drm_device *dev, if (!crtc) return -ENOENT;
- drm_modeset_lock_crtc(crtc, crtc->primary); crtc_resp->gamma_size = crtc->gamma_size;
drm_modeset_lock(&crtc->primary->mutex, NULL); if (crtc->primary->state && crtc->primary->state->fb) crtc_resp->fb_id = crtc->primary->state->fb->base.id; else if (!crtc->primary->state && crtc->primary->fb) crtc_resp->fb_id = crtc->primary->fb->base.id; else crtc_resp->fb_id = 0;
drm_modeset_unlock(&crtc->primary->mutex);
drm_modeset_lock(&crtc->mutex, NULL); if (crtc->state) { crtc_resp->x = crtc->primary->state->src_x >> 16; crtc_resp->y = crtc->primary->state->src_y >> 16;
What about crtc->primary here? Shouldn't that also be locked with the crtc-
primary->mutex or do we treat state differently? Still not 100% on the exact
locking semantics.
Harry
@@ -437,7 +439,7 @@ int drm_mode_getcrtc(struct drm_device *dev, crtc_resp->mode_valid = 0; } }
- drm_modeset_unlock_crtc(crtc);
drm_modeset_unlock(&crtc->mutex);
return 0;
}
On Mon, Mar 27, 2017 at 08:13:17PM -0400, Harry Wentland wrote:
On Wednesday, March 22, 2017 10:50:52 PM EDT Daniel Vetter wrote:
No need to grab both plane and crtc locks at the same time, we can do them one after the other. If userspace races it'll get what it deserves either way.
This removes another user of drm_modeset_lock_crtc. There's only one left.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com
drivers/gpu/drm/drm_crtc.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 660b4c8715de..df1ff0b8818b 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -406,16 +406,18 @@ int drm_mode_getcrtc(struct drm_device *dev, if (!crtc) return -ENOENT;
- drm_modeset_lock_crtc(crtc, crtc->primary); crtc_resp->gamma_size = crtc->gamma_size;
drm_modeset_lock(&crtc->primary->mutex, NULL); if (crtc->primary->state && crtc->primary->state->fb) crtc_resp->fb_id = crtc->primary->state->fb->base.id; else if (!crtc->primary->state && crtc->primary->fb) crtc_resp->fb_id = crtc->primary->fb->base.id; else crtc_resp->fb_id = 0;
drm_modeset_unlock(&crtc->primary->mutex);
drm_modeset_lock(&crtc->mutex, NULL); if (crtc->state) { crtc_resp->x = crtc->primary->state->src_x >> 16; crtc_resp->y = crtc->primary->state->src_y >> 16;
What about crtc->primary here? Shouldn't that also be locked with the crtc-
primary->mutex or do we treat state differently? Still not 100% on the exact
locking semantics.
I misread the code, I thought we only access crtc->state here. I'll shuffle this around so that crtc->primary->state is properly protected, that's indeed a bug. -Daniel
No need to grab both plane and crtc locks at the same time, we can do them one after the other. If userspace races it'll get what it deserves either way.
This removes another user of drm_modeset_lock_crtc. There's only one left.
v2: Make sure all access to primary->state is properly protected (Harry).
Cc: Harry Wentland harry.wentland@amd.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/drm_crtc.c | 11 ++++++++--- drivers/gpu/drm/i915/intel_display.c | 5 +++++ 2 files changed, 13 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 660b4c8715de..55b3da2e2a82 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -406,9 +406,9 @@ int drm_mode_getcrtc(struct drm_device *dev, if (!crtc) return -ENOENT;
- drm_modeset_lock_crtc(crtc, crtc->primary); crtc_resp->gamma_size = crtc->gamma_size;
+ drm_modeset_lock(&crtc->primary->mutex, NULL); if (crtc->primary->state && crtc->primary->state->fb) crtc_resp->fb_id = crtc->primary->state->fb->base.id; else if (!crtc->primary->state && crtc->primary->fb) @@ -416,9 +416,14 @@ int drm_mode_getcrtc(struct drm_device *dev, else crtc_resp->fb_id = 0;
- if (crtc->state) { + if (crtc->primary->state) { crtc_resp->x = crtc->primary->state->src_x >> 16; crtc_resp->y = crtc->primary->state->src_y >> 16; + } + drm_modeset_unlock(&crtc->primary->mutex); + + drm_modeset_lock(&crtc->mutex, NULL); + if (crtc->state) { if (crtc->state->enable) { drm_mode_convert_to_umode(&crtc_resp->mode, &crtc->state->mode); crtc_resp->mode_valid = 1; @@ -437,7 +442,7 @@ int drm_mode_getcrtc(struct drm_device *dev, crtc_resp->mode_valid = 0; } } - drm_modeset_unlock_crtc(crtc); + drm_modeset_unlock(&crtc->mutex);
return 0; } diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 68cded453882..43dbad62786e 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -12463,6 +12463,11 @@ static int intel_atomic_check(struct drm_device *dev, ret = drm_atomic_helper_check_modeset(dev, state); if (ret) return ret; + /* enocder->atomic_check might upgrade some crtc to a full modeset */ + ret = drm_atomic_helper_check_modeset(dev, state); + if (ret) + return ret; +
for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, crtc_state, i) { struct intel_crtc_state *pipe_config =
Reviewed-by: Harry Wentland harry.wentland@amd.com
Harry
On 2017-03-28 03:01 AM, Daniel Vetter wrote:
No need to grab both plane and crtc locks at the same time, we can do them one after the other. If userspace races it'll get what it deserves either way.
This removes another user of drm_modeset_lock_crtc. There's only one left.
v2: Make sure all access to primary->state is properly protected (Harry).
Cc: Harry Wentland harry.wentland@amd.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com
drivers/gpu/drm/drm_crtc.c | 11 ++++++++--- drivers/gpu/drm/i915/intel_display.c | 5 +++++ 2 files changed, 13 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 660b4c8715de..55b3da2e2a82 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -406,9 +406,9 @@ int drm_mode_getcrtc(struct drm_device *dev, if (!crtc) return -ENOENT;
- drm_modeset_lock_crtc(crtc, crtc->primary); crtc_resp->gamma_size = crtc->gamma_size;
- drm_modeset_lock(&crtc->primary->mutex, NULL); if (crtc->primary->state && crtc->primary->state->fb) crtc_resp->fb_id = crtc->primary->state->fb->base.id; else if (!crtc->primary->state && crtc->primary->fb)
@@ -416,9 +416,14 @@ int drm_mode_getcrtc(struct drm_device *dev, else crtc_resp->fb_id = 0;
- if (crtc->state) {
- if (crtc->primary->state) { crtc_resp->x = crtc->primary->state->src_x >> 16; crtc_resp->y = crtc->primary->state->src_y >> 16;
- }
- drm_modeset_unlock(&crtc->primary->mutex);
- drm_modeset_lock(&crtc->mutex, NULL);
- if (crtc->state) { if (crtc->state->enable) { drm_mode_convert_to_umode(&crtc_resp->mode, &crtc->state->mode); crtc_resp->mode_valid = 1;
@@ -437,7 +442,7 @@ int drm_mode_getcrtc(struct drm_device *dev, crtc_resp->mode_valid = 0; } }
- drm_modeset_unlock_crtc(crtc);
drm_modeset_unlock(&crtc->mutex);
return 0;
} diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 68cded453882..43dbad62786e 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -12463,6 +12463,11 @@ static int intel_atomic_check(struct drm_device *dev, ret = drm_atomic_helper_check_modeset(dev, state); if (ret) return ret;
/* enocder->atomic_check might upgrade some crtc to a full modeset */
ret = drm_atomic_helper_check_modeset(dev, state);
if (ret)
return ret;for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, crtc_state, i) { struct intel_crtc_state *pipe_config =
Op 28-03-17 om 09:01 schreef Daniel Vetter:
<snip> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 68cded453882..43dbad62786e 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -12463,6 +12463,11 @@ static int intel_atomic_check(struct drm_device *dev, ret = drm_atomic_helper_check_modeset(dev, state); if (ret) return ret; + /* enocder->atomic_check might upgrade some crtc to a full modeset */ + ret = drm_atomic_helper_check_modeset(dev, state); + if (ret) + return ret; +
for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, crtc_state, i) { struct intel_crtc_state *pipe_config =
I know this patch has been applied, but this hunk is completely unrelated.
Can I get a R-B on reverting it?
---->8---- v2 of the commit 2c77bb29d398 ("drm: simplify the locking in the GETCRTC ioctl") accidentally introduced a unrelated change in intel_display.c, revert the unrelated change.
Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com Fixes: 2c77bb29d398 ("drm: simplify the locking in the GETCRTC ioctl") --- diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index baa8d836c8e7..c45694abda5b 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -12478,11 +12478,6 @@ static int intel_atomic_check(struct drm_device *dev, ret = drm_atomic_helper_check_modeset(dev, state); if (ret) return ret; - /* enocder->atomic_check might upgrade some crtc to a full modeset */ - ret = drm_atomic_helper_check_modeset(dev, state); - if (ret) - return ret; -
for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, crtc_state, i) { struct intel_crtc_state *pipe_config =
On Thu, 2017-03-30 at 09:36 +0200, Maarten Lankhorst wrote:
Op 28-03-17 om 09:01 schreef Daniel Vetter:
<snip> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 68cded453882..43dbad62786e 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -12463,6 +12463,11 @@ static int intel_atomic_check(struct drm_device *dev, ret = drm_atomic_helper_check_modeset(dev, state); if (ret) return ret; + /* enocder->atomic_check might upgrade some crtc to a full modeset */ + ret = drm_atomic_helper_check_modeset(dev, state); + if (ret) + return ret; +
for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, crtc_state, i) { struct intel_crtc_state *pipe_config =
I know this patch has been applied, but this hunk is completely unrelated.
Can I get a R-B on reverting it?
---->8---- v2 of the commit 2c77bb29d398 ("drm: simplify the locking in the GETCRTC ioctl") accidentally introduced a unrelated change in intel_display.c, revert the unrelated change.
Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com Fixes: 2c77bb29d398 ("drm: simplify the locking in the GETCRTC ioctl")
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index baa8d836c8e7..c45694abda5b 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -12478,11 +12478,6 @@ static int intel_atomic_check(struct drm_device *dev, ret = drm_atomic_helper_check_modeset(dev, state); if (ret) return ret;
- /* enocder->atomic_check might upgrade some crtc to a full modeset */
- ret = drm_atomic_helper_check_modeset(dev, state);
Noticed this while testing my driver-private object series. drm_atomic_helper_check_modeset->atomic_release() getting called twice within an atomic_check() broke the vcpi slots bookkeeping I had and ironically exposed a bug in my code.
Reported-by: Dhinakaran Pandiyan dhinakaran.pandiyan@intel.com Reviewed-by: Dhinakaran Pandiyan dhinakaran.pandiyan@intel.com
if (ret)
return ret;for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, crtc_state, i) { struct intel_crtc_state *pipe_config =
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Thu, Mar 30, 2017 at 07:48:45AM +0000, Pandiyan, Dhinakaran wrote:
On Thu, 2017-03-30 at 09:36 +0200, Maarten Lankhorst wrote:
Op 28-03-17 om 09:01 schreef Daniel Vetter:
<snip> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 68cded453882..43dbad62786e 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -12463,6 +12463,11 @@ static int intel_atomic_check(struct drm_device *dev, ret = drm_atomic_helper_check_modeset(dev, state); if (ret) return ret; + /* enocder->atomic_check might upgrade some crtc to a full modeset */ + ret = drm_atomic_helper_check_modeset(dev, state); + if (ret) + return ret; +
for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, crtc_state, i) { struct intel_crtc_state *pipe_config =
I know this patch has been applied, but this hunk is completely unrelated.
Can I get a R-B on reverting it?
---->8---- v2 of the commit 2c77bb29d398 ("drm: simplify the locking in the GETCRTC ioctl") accidentally introduced a unrelated change in intel_display.c, revert the unrelated change.
Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com Fixes: 2c77bb29d398 ("drm: simplify the locking in the GETCRTC ioctl")
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index baa8d836c8e7..c45694abda5b 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -12478,11 +12478,6 @@ static int intel_atomic_check(struct drm_device *dev, ret = drm_atomic_helper_check_modeset(dev, state); if (ret) return ret;
- /* enocder->atomic_check might upgrade some crtc to a full modeset */
- ret = drm_atomic_helper_check_modeset(dev, state);
Noticed this while testing my driver-private object series. drm_atomic_helper_check_modeset->atomic_release() getting called twice within an atomic_check() broke the vcpi slots bookkeeping I had and ironically exposed a bug in my code.
Reported-by: Dhinakaran Pandiyan dhinakaran.pandiyan@intel.com Reviewed-by: Dhinakaran Pandiyan dhinakaran.pandiyan@intel.com
Oh derp this is embarrassing :( Thanks for the quick fix, applied. -Daniel
if (ret)
return ret;for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, crtc_state, i) { struct intel_crtc_state *pipe_config =
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
The last user, the cursor ioctl, can just open-code this too. We simply have to move the acquire ctx dance from the universal function up into the top-level ioctl handler.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/drm_crtc_internal.h | 3 -- drivers/gpu/drm/drm_modeset_lock.c | 67 ------------------------------------- drivers/gpu/drm/drm_plane.c | 49 +++++++++++++-------------- include/drm/drm_modeset_lock.h | 1 - 4 files changed, 24 insertions(+), 96 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc_internal.h b/drivers/gpu/drm/drm_crtc_internal.h index de1047530e07..8c04275cf226 100644 --- a/drivers/gpu/drm/drm_crtc_internal.h +++ b/drivers/gpu/drm/drm_crtc_internal.h @@ -61,9 +61,6 @@ int drm_mode_getresources(struct drm_device *dev, void *data, struct drm_file *file_priv);
-/* drm_modeset_lock.c */ -void drm_modeset_lock_crtc(struct drm_crtc *crtc, - struct drm_plane *plane); /* drm_dumb_buffers.c */ /* IOCTLs */ int drm_mode_create_dumb_ioctl(struct drm_device *dev, diff --git a/drivers/gpu/drm/drm_modeset_lock.c b/drivers/gpu/drm/drm_modeset_lock.c index c94eff9d7544..c3ca6b859236 100644 --- a/drivers/gpu/drm/drm_modeset_lock.c +++ b/drivers/gpu/drm/drm_modeset_lock.c @@ -148,51 +148,6 @@ void drm_modeset_unlock_all(struct drm_device *dev) } EXPORT_SYMBOL(drm_modeset_unlock_all);
-void drm_modeset_lock_crtc(struct drm_crtc *crtc, - struct drm_plane *plane) -{ - struct drm_modeset_acquire_ctx *ctx; - int ret; - - ctx = kzalloc(sizeof(*ctx), GFP_KERNEL); - if (WARN_ON(!ctx)) - return; - - drm_modeset_acquire_init(ctx, 0); - -retry: - ret = drm_modeset_lock(&crtc->mutex, ctx); - if (ret) - goto fail; - - if (plane) { - ret = drm_modeset_lock(&plane->mutex, ctx); - if (ret) - goto fail; - - if (plane->crtc) { - ret = drm_modeset_lock(&plane->crtc->mutex, ctx); - if (ret) - goto fail; - } - } - - WARN_ON(crtc->acquire_ctx); - - /* now we hold the locks, so now that it is safe, stash the - * ctx for drm_modeset_unlock_crtc(): - */ - crtc->acquire_ctx = ctx; - - return; - -fail: - if (ret == -EDEADLK) { - drm_modeset_backoff(ctx); - goto retry; - } -} - /** * drm_modeset_legacy_acquire_ctx - find acquire ctx for legacy ioctls * @crtc: drm crtc @@ -215,28 +170,6 @@ drm_modeset_legacy_acquire_ctx(struct drm_crtc *crtc) EXPORT_SYMBOL(drm_modeset_legacy_acquire_ctx);
/** - * drm_modeset_unlock_crtc - drop crtc lock - * @crtc: drm crtc - * - * This drops the crtc lock acquire with drm_modeset_lock_crtc() and all other - * locks acquired through the hidden context. - */ -void drm_modeset_unlock_crtc(struct drm_crtc *crtc) -{ - struct drm_modeset_acquire_ctx *ctx = crtc->acquire_ctx; - - if (WARN_ON(!ctx)) - return; - - crtc->acquire_ctx = NULL; - drm_modeset_drop_locks(ctx); - drm_modeset_acquire_fini(ctx); - - kfree(ctx); -} -EXPORT_SYMBOL(drm_modeset_unlock_crtc); - -/** * drm_warn_on_modeset_not_all_locked - check that all modeset locks are locked * @dev: device * diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c index ec3e2e757800..ec62221d64a9 100644 --- a/drivers/gpu/drm/drm_plane.c +++ b/drivers/gpu/drm/drm_plane.c @@ -620,7 +620,8 @@ int drm_mode_setplane(struct drm_device *dev, void *data,
static int drm_mode_cursor_universal(struct drm_crtc *crtc, struct drm_mode_cursor2 *req, - struct drm_file *file_priv) + struct drm_file *file_priv, + struct drm_modeset_acquire_ctx *ctx) { struct drm_device *dev = crtc->dev; struct drm_framebuffer *fb = NULL; @@ -634,21 +635,11 @@ static int drm_mode_cursor_universal(struct drm_crtc *crtc, int32_t crtc_x, crtc_y; uint32_t crtc_w = 0, crtc_h = 0; uint32_t src_w = 0, src_h = 0; - struct drm_modeset_acquire_ctx ctx; int ret = 0;
BUG_ON(!crtc->cursor); WARN_ON(crtc->cursor->crtc != crtc && crtc->cursor->crtc != NULL);
- drm_modeset_acquire_init(&ctx, 0); -retry: - ret = drm_modeset_lock(&crtc->mutex, &ctx); - if (ret) - goto fail; - ret = drm_modeset_lock(&crtc->cursor->mutex, &ctx); - if (ret) - goto fail; - /* * Obtain fb we'll be using (either new or existing) and take an extra * reference to it if fb != null. setplane will take care of dropping @@ -693,7 +684,7 @@ static int drm_mode_cursor_universal(struct drm_crtc *crtc, */ ret = __setplane_internal(crtc->cursor, crtc, fb, crtc_x, crtc_y, crtc_w, crtc_h, - 0, 0, src_w, src_h, &ctx); + 0, 0, src_w, src_h, ctx);
/* Update successful; save new cursor position, if necessary */ if (ret == 0 && req->flags & DRM_MODE_CURSOR_MOVE) { @@ -701,15 +692,6 @@ static int drm_mode_cursor_universal(struct drm_crtc *crtc, crtc->cursor_y = req->y; }
-fail: - if (ret == -EDEADLK) { - drm_modeset_backoff(&ctx); - goto retry; - } - - drm_modeset_drop_locks(&ctx); - drm_modeset_acquire_fini(&ctx); - return ret; }
@@ -718,6 +700,7 @@ static int drm_mode_cursor_common(struct drm_device *dev, struct drm_file *file_priv) { struct drm_crtc *crtc; + struct drm_modeset_acquire_ctx ctx; int ret = 0;
if (!drm_core_check_feature(dev, DRIVER_MODESET)) @@ -732,14 +715,24 @@ static int drm_mode_cursor_common(struct drm_device *dev, return -ENOENT; }
+ drm_modeset_acquire_init(&ctx, 0); +retry: + ret = drm_modeset_lock(&crtc->mutex, &ctx); + if (ret) + goto out; + ret = drm_modeset_lock(&crtc->cursor->mutex, &ctx); + if (ret) + goto out; + /* * 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); + if (crtc->cursor) { + ret = drm_mode_cursor_universal(crtc, req, file_priv, &ctx); + goto out; + }
- drm_modeset_lock_crtc(crtc, crtc->cursor); if (req->flags & DRM_MODE_CURSOR_BO) { if (!crtc->funcs->cursor_set && !crtc->funcs->cursor_set2) { ret = -ENXIO; @@ -763,7 +756,13 @@ static int drm_mode_cursor_common(struct drm_device *dev, } } out: - drm_modeset_unlock_crtc(crtc); + if (ret == -EDEADLK) { + drm_modeset_backoff(&ctx); + goto retry; + } + + drm_modeset_drop_locks(&ctx); + drm_modeset_acquire_fini(&ctx);
return ret;
diff --git a/include/drm/drm_modeset_lock.h b/include/drm/drm_modeset_lock.h index 88d35bfc9cd8..2bb065bf0593 100644 --- a/include/drm/drm_modeset_lock.h +++ b/include/drm/drm_modeset_lock.h @@ -121,7 +121,6 @@ struct drm_plane;
void drm_modeset_lock_all(struct drm_device *dev); void drm_modeset_unlock_all(struct drm_device *dev); -void drm_modeset_unlock_crtc(struct drm_crtc *crtc); void drm_warn_on_modeset_not_all_locked(struct drm_device *dev); struct drm_modeset_acquire_ctx * drm_modeset_legacy_acquire_ctx(struct drm_crtc *crtc);
With all the callers of drm_modeset_lock_crtc gone, and all the places it was formerly used properly wiring the acquire ctx through, we can remove this.
The only hidden context magic we still have is now the global one.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/drm_atomic.c | 14 -------------- drivers/gpu/drm/drm_atomic_helper.c | 4 ++-- drivers/gpu/drm/drm_modeset_lock.c | 21 --------------------- drivers/gpu/drm/i915/intel_display.c | 4 ++-- drivers/gpu/drm/i915/intel_pipe_crc.c | 2 +- include/drm/drm_crtc.h | 9 --------- include/drm/drm_modeset_lock.h | 2 -- 7 files changed, 5 insertions(+), 51 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 9b892af7811a..345310213820 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -1516,19 +1516,9 @@ EXPORT_SYMBOL(drm_atomic_add_affected_planes); void drm_atomic_legacy_backoff(struct drm_atomic_state *state) { struct drm_device *dev = state->dev; - unsigned crtc_mask = 0; - struct drm_crtc *crtc; int ret; bool global = false;
- drm_for_each_crtc(crtc, dev) { - if (crtc->acquire_ctx != state->acquire_ctx) - continue; - - crtc_mask |= drm_crtc_mask(crtc); - crtc->acquire_ctx = NULL; - } - if (WARN_ON(dev->mode_config.acquire_ctx == state->acquire_ctx)) { global = true;
@@ -1542,10 +1532,6 @@ void drm_atomic_legacy_backoff(struct drm_atomic_state *state) if (ret) goto retry;
- drm_for_each_crtc(crtc, dev) - if (drm_crtc_mask(crtc) & crtc_mask) - crtc->acquire_ctx = state->acquire_ctx; - if (global) dev->mode_config.acquire_ctx = state->acquire_ctx; } diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index c0ec763ec538..08d10abcece0 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -2289,7 +2289,7 @@ int drm_atomic_helper_set_config(struct drm_mode_set *set) return -ENOMEM;
state->legacy_set_config = true; - state->acquire_ctx = drm_modeset_legacy_acquire_ctx(crtc); + state->acquire_ctx = crtc->dev->mode_config.acquire_ctx; retry: ret = __drm_atomic_helper_set_config(set, state); if (ret != 0) @@ -2990,7 +2990,7 @@ int drm_atomic_helper_connector_dpms(struct drm_connector *connector, if (!state) return -ENOMEM;
- state->acquire_ctx = drm_modeset_legacy_acquire_ctx(crtc); + state->acquire_ctx = crtc->dev->mode_config.acquire_ctx; retry: crtc_state = drm_atomic_get_crtc_state(state, crtc); if (IS_ERR(crtc_state)) { diff --git a/drivers/gpu/drm/drm_modeset_lock.c b/drivers/gpu/drm/drm_modeset_lock.c index c3ca6b859236..64ef09a6cccb 100644 --- a/drivers/gpu/drm/drm_modeset_lock.c +++ b/drivers/gpu/drm/drm_modeset_lock.c @@ -149,27 +149,6 @@ void drm_modeset_unlock_all(struct drm_device *dev) EXPORT_SYMBOL(drm_modeset_unlock_all);
/** - * drm_modeset_legacy_acquire_ctx - find acquire ctx for legacy ioctls - * @crtc: drm crtc - * - * Legacy ioctl operations like cursor updates or page flips only have per-crtc - * locking, and store the acquire ctx in the corresponding crtc. All other - * legacy operations take all locks and use a global acquire context. This - * function grabs the right one. - */ -struct drm_modeset_acquire_ctx * -drm_modeset_legacy_acquire_ctx(struct drm_crtc *crtc) -{ - if (crtc->acquire_ctx) - return crtc->acquire_ctx; - - WARN_ON(!crtc->dev->mode_config.acquire_ctx); - - return crtc->dev->mode_config.acquire_ctx; -} -EXPORT_SYMBOL(drm_modeset_legacy_acquire_ctx); - -/** * drm_warn_on_modeset_not_all_locked - check that all modeset locks are locked * @dev: device * diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index e27ea89efd67..84abff3f43d9 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -10715,7 +10715,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc, state = drm_atomic_state_alloc(dev); if (!state) return -ENOMEM; - state->acquire_ctx = drm_modeset_legacy_acquire_ctx(crtc); + state->acquire_ctx = dev->mode_config.acquire_ctx;
retry: plane_state = drm_atomic_get_plane_state(state, primary); @@ -13075,7 +13075,7 @@ void intel_crtc_restore_mode(struct drm_crtc *crtc) return; }
- state->acquire_ctx = drm_modeset_legacy_acquire_ctx(crtc); + state->acquire_ctx = crtc->dev->mode_config.acquire_ctx;
retry: crtc_state = drm_atomic_get_crtc_state(state, crtc); diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c b/drivers/gpu/drm/i915/intel_pipe_crc.c index 9fd9c70baeed..206ee4f0150e 100644 --- a/drivers/gpu/drm/i915/intel_pipe_crc.c +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c @@ -522,7 +522,7 @@ static void hsw_trans_edp_pipe_A_crc_wa(struct drm_i915_private *dev_priv, goto unlock; }
- state->acquire_ctx = drm_modeset_legacy_acquire_ctx(&crtc->base); + state->acquire_ctx = crtc->base.dev->mode_config.acquire_ctx; pipe_config = intel_atomic_get_crtc_state(state, crtc); if (IS_ERR(pipe_config)) { ret = PTR_ERR(pipe_config); diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index fe42a57393df..bca3330d8085 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -771,15 +771,6 @@ struct drm_crtc { */ spinlock_t commit_lock;
- /** - * @acquire_ctx: - * - * Per-CRTC implicit acquire context used by atomic drivers for legacy - * IOCTLs, so that atomic drivers can get at the locking acquire - * context. - */ - struct drm_modeset_acquire_ctx *acquire_ctx; - #ifdef CONFIG_DEBUG_FS /** * @debugfs_entry: diff --git a/include/drm/drm_modeset_lock.h b/include/drm/drm_modeset_lock.h index 2bb065bf0593..4b27c2bb955c 100644 --- a/include/drm/drm_modeset_lock.h +++ b/include/drm/drm_modeset_lock.h @@ -122,8 +122,6 @@ struct drm_plane; void drm_modeset_lock_all(struct drm_device *dev); void drm_modeset_unlock_all(struct drm_device *dev); void drm_warn_on_modeset_not_all_locked(struct drm_device *dev); -struct drm_modeset_acquire_ctx * -drm_modeset_legacy_acquire_ctx(struct drm_crtc *crtc);
int drm_modeset_lock_all_ctx(struct drm_device *dev, struct drm_modeset_acquire_ctx *ctx);
This is another case where we really can't reconstruct a acquire ctx in any useful fashion because all the callers are legacy drivers. So like drm_plane_force_disable simply restrict it to non-atomic drivers so that it's clear we're ok with passing a NULL ctx.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/drm_crtc.c | 34 +++++++++++++++++++++------------- 1 file changed, 21 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index df1ff0b8818b..05447492483f 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -444,18 +444,7 @@ int drm_mode_getcrtc(struct drm_device *dev, return 0; }
-/** - * drm_mode_set_config_internal - helper to call &drm_mode_config_funcs.set_config - * @set: modeset config to set - * - * This is a little helper to wrap internal calls to the - * &drm_mode_config_funcs.set_config driver interface. The only thing it adds is - * correct refcounting dance. - * - * Returns: - * Zero on success, negative errno on failure. - */ -int drm_mode_set_config_internal(struct drm_mode_set *set) +static int __drm_mode_set_config_internal(struct drm_mode_set *set) { struct drm_crtc *crtc = set->crtc; struct drm_framebuffer *fb; @@ -488,6 +477,25 @@ int drm_mode_set_config_internal(struct drm_mode_set *set)
return ret; } +/** + * drm_mode_set_config_internal - helper to call &drm_mode_config_funcs.set_config + * @set: modeset config to set + * + * This is a little helper to wrap internal calls to the + * &drm_mode_config_funcs.set_config driver interface. The only thing it adds is + * correct refcounting dance. + * + * This should only be used by non-atomic legacy drivers. + * + * Returns: + * Zero on success, negative errno on failure. + */ +int drm_mode_set_config_internal(struct drm_mode_set *set) +{ + WARN_ON(drm_drv_uses_atomic_modeset(set->crtc->dev)); + + return __drm_mode_set_config_internal(set); +} EXPORT_SYMBOL(drm_mode_set_config_internal);
/** @@ -685,7 +693,7 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data, set.connectors = connector_set; set.num_connectors = crtc_req->count_connectors; set.fb = fb; - ret = drm_mode_set_config_internal(&set); + ret = __drm_mode_set_config_internal(&set);
out: if (fb)
Just the groundwork to have something to feed into ->set_config. Again we need a temporary hack to still fill out the legacy ctx in mode_config.acquire_ctx.
Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/drm_crtc.c | 26 +++++++++++++++++++------- 1 file changed, 19 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 05447492483f..bfaa0e769ea6 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -444,7 +444,8 @@ int drm_mode_getcrtc(struct drm_device *dev, return 0; }
-static int __drm_mode_set_config_internal(struct drm_mode_set *set) +static int __drm_mode_set_config_internal(struct drm_mode_set *set, + struct drm_modeset_acquire_ctx *ctx) { struct drm_crtc *crtc = set->crtc; struct drm_framebuffer *fb; @@ -494,7 +495,7 @@ int drm_mode_set_config_internal(struct drm_mode_set *set) { WARN_ON(drm_drv_uses_atomic_modeset(set->crtc->dev));
- return __drm_mode_set_config_internal(set); + return __drm_mode_set_config_internal(set, NULL); } EXPORT_SYMBOL(drm_mode_set_config_internal);
@@ -551,6 +552,7 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data, struct drm_display_mode *mode = NULL; struct drm_mode_set set; uint32_t __user *set_connectors_ptr; + struct drm_modeset_acquire_ctx ctx; int ret; int i;
@@ -564,15 +566,19 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data, if (crtc_req->x & 0xffff0000 || crtc_req->y & 0xffff0000) return -ERANGE;
- drm_modeset_lock_all(dev); crtc = drm_crtc_find(dev, crtc_req->crtc_id); if (!crtc) { DRM_DEBUG_KMS("Unknown CRTC ID %d\n", crtc_req->crtc_id); - ret = -ENOENT; - goto out; + return -ENOENT; } DRM_DEBUG_KMS("[CRTC:%d:%s]\n", crtc->base.id, crtc->name);
+ drm_modeset_acquire_init(&ctx, 0); +retry: + ret = drm_modeset_lock_all_ctx(crtc->dev, &ctx); + if (ret) + goto out; + dev->mode_config.acquire_ctx = &ctx; if (crtc_req->mode_valid) { /* If we have a mode we need a framebuffer. */ /* If we pass -1, set the mode with the currently bound fb */ @@ -693,7 +699,7 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data, set.connectors = connector_set; set.num_connectors = crtc_req->count_connectors; set.fb = fb; - ret = __drm_mode_set_config_internal(&set); + ret = __drm_mode_set_config_internal(&set, &ctx);
out: if (fb) @@ -707,7 +713,13 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data, } kfree(connector_set); drm_mode_destroy(dev, mode); - drm_modeset_unlock_all(dev); + if (ret == -EDEADLK) { + drm_modeset_backoff(&ctx); + goto retry; + } + drm_modeset_drop_locks(&ctx); + drm_modeset_acquire_fini(&ctx); + return ret; }
Surprisingly a lot of legacy drivers roll their own, for runtime pm and because vmwgfx.
Also make nouveau's set_config static while at it.
Cc: Sinclair Yeh syeh@vmware.com Cc: Thomas Hellstrom thellstrom@vmware.com Cc: Ben Skeggs bskeggs@redhat.com Cc: Patrik Jakobsson patrik.r.jakobsson@gmail.com Cc: Alex Deucher alexander.deucher@amd.com Cc: Christian König christian.koenig@amd.com
Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 5 +++-- drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h | 3 ++- drivers/gpu/drm/drm_atomic_helper.c | 4 +++- drivers/gpu/drm/drm_crtc.c | 2 +- drivers/gpu/drm/drm_crtc_helper.c | 4 +++- drivers/gpu/drm/drm_plane_helper.c | 2 +- drivers/gpu/drm/gma500/gma_display.c | 7 ++++--- drivers/gpu/drm/gma500/gma_display.h | 3 ++- drivers/gpu/drm/nouveau/dispnv04/crtc.c | 7 ++++--- drivers/gpu/drm/nouveau/nouveau_display.h | 1 - drivers/gpu/drm/radeon/radeon_display.c | 5 +++-- drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c | 3 ++- drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c | 3 ++- drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c | 3 ++- include/drm/drm_atomic_helper.h | 3 ++- include/drm/drm_crtc.h | 3 ++- include/drm/drm_crtc_helper.h | 3 ++- 17 files changed, 38 insertions(+), 23 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c index 7b4fe91d3aec..ce15721cadda 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c @@ -333,7 +333,8 @@ int amdgpu_crtc_page_flip_target(struct drm_crtc *crtc, return 0; }
-int amdgpu_crtc_set_config(struct drm_mode_set *set) +int amdgpu_crtc_set_config(struct drm_mode_set *set, + struct drm_modeset_acquire_ctx *ctx) { struct drm_device *dev; struct amdgpu_device *adev; @@ -350,7 +351,7 @@ int amdgpu_crtc_set_config(struct drm_mode_set *set) if (ret < 0) return ret;
- ret = drm_crtc_helper_set_config(set); + ret = drm_crtc_helper_set_config(set, ctx);
list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) if (crtc->enabled) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h index d19b803ba509..20d6522fd7b4 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h @@ -593,7 +593,8 @@ int amdgpu_align_pitch(struct amdgpu_device *adev, int width, int bpp, bool tile /* amdgpu_display.c */ void amdgpu_print_display_setup(struct drm_device *dev); int amdgpu_modeset_create_props(struct amdgpu_device *adev); -int amdgpu_crtc_set_config(struct drm_mode_set *set); +int amdgpu_crtc_set_config(struct drm_mode_set *set, + struct drm_modeset_acquire_ctx *ctx); int amdgpu_crtc_page_flip_target(struct drm_crtc *crtc, struct drm_framebuffer *fb, struct drm_pending_vblank_event *event, diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 08d10abcece0..b502e2809ebd 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -2266,6 +2266,7 @@ static int update_output_state(struct drm_atomic_state *state, /** * drm_atomic_helper_set_config - set a new config from userspace * @set: mode set configuration + * @ctx: lock acquisition context * * Provides a default crtc set_config handler using the atomic driver interface. * @@ -2278,7 +2279,8 @@ static int update_output_state(struct drm_atomic_state *state, * Returns: * Returns 0 on success, negative errno numbers on failure. */ -int drm_atomic_helper_set_config(struct drm_mode_set *set) +int drm_atomic_helper_set_config(struct drm_mode_set *set, + struct drm_modeset_acquire_ctx *ctx) { struct drm_atomic_state *state; struct drm_crtc *crtc = set->crtc; diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index bfaa0e769ea6..3fe1ec23c87e 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -462,7 +462,7 @@ static int __drm_mode_set_config_internal(struct drm_mode_set *set,
fb = set->fb;
- ret = crtc->funcs->set_config(set); + ret = crtc->funcs->set_config(set, ctx); if (ret == 0) { crtc->primary->crtc = crtc; crtc->primary->fb = fb; diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c index 8aa8c1084121..4afdf7902eda 100644 --- a/drivers/gpu/drm/drm_crtc_helper.c +++ b/drivers/gpu/drm/drm_crtc_helper.c @@ -476,6 +476,7 @@ drm_crtc_helper_disable(struct drm_crtc *crtc) /** * drm_crtc_helper_set_config - set a new config from userspace * @set: mode set configuration + * @ctx: lock acquire context, not used here * * The drm_crtc_helper_set_config() helper function implements the of * &drm_crtc_funcs.set_config callback for drivers using the legacy CRTC @@ -510,7 +511,8 @@ drm_crtc_helper_disable(struct drm_crtc *crtc) * Returns: * Returns 0 on success, negative errno numbers on failure. */ -int drm_crtc_helper_set_config(struct drm_mode_set *set) +int drm_crtc_helper_set_config(struct drm_mode_set *set, + struct drm_modeset_acquire_ctx *ctx) { struct drm_device *dev; struct drm_crtc **save_encoder_crtcs, *new_crtc; diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c index 775e94c30368..b84a295230fc 100644 --- a/drivers/gpu/drm/drm_plane_helper.c +++ b/drivers/gpu/drm/drm_plane_helper.c @@ -371,7 +371,7 @@ int drm_primary_helper_update(struct drm_plane *plane, struct drm_crtc *crtc, * drm_mode_setplane() already handles the basic refcounting for the * framebuffers involved in this operation. */ - ret = crtc->funcs->set_config(&set); + ret = crtc->funcs->set_config(&set, ctx);
kfree(connector_list); return ret; diff --git a/drivers/gpu/drm/gma500/gma_display.c b/drivers/gpu/drm/gma500/gma_display.c index d1c5642b1c1e..93ff46535c04 100644 --- a/drivers/gpu/drm/gma500/gma_display.c +++ b/drivers/gpu/drm/gma500/gma_display.c @@ -514,17 +514,18 @@ void gma_crtc_destroy(struct drm_crtc *crtc) kfree(gma_crtc); }
-int gma_crtc_set_config(struct drm_mode_set *set) +int gma_crtc_set_config(struct drm_mode_set *set, + struct drm_modeset_acquire_ctx *ctx) { struct drm_device *dev = set->crtc->dev; struct drm_psb_private *dev_priv = dev->dev_private; int ret;
if (!dev_priv->rpm_enabled) - return drm_crtc_helper_set_config(set); + return drm_crtc_helper_set_config(set, ctx);
pm_runtime_forbid(&dev->pdev->dev); - ret = drm_crtc_helper_set_config(set); + ret = drm_crtc_helper_set_config(set, ctx); pm_runtime_allow(&dev->pdev->dev);
return ret; diff --git a/drivers/gpu/drm/gma500/gma_display.h b/drivers/gpu/drm/gma500/gma_display.h index e72dd08b701b..166e608923db 100644 --- a/drivers/gpu/drm/gma500/gma_display.h +++ b/drivers/gpu/drm/gma500/gma_display.h @@ -79,7 +79,8 @@ extern void gma_crtc_prepare(struct drm_crtc *crtc); extern void gma_crtc_commit(struct drm_crtc *crtc); extern void gma_crtc_disable(struct drm_crtc *crtc); extern void gma_crtc_destroy(struct drm_crtc *crtc); -extern int gma_crtc_set_config(struct drm_mode_set *set); +extern int gma_crtc_set_config(struct drm_mode_set *set, + struct drm_modeset_acquire_ctx *ctx);
extern void gma_crtc_save(struct drm_crtc *crtc); extern void gma_crtc_restore(struct drm_crtc *crtc); diff --git a/drivers/gpu/drm/nouveau/dispnv04/crtc.c b/drivers/gpu/drm/nouveau/dispnv04/crtc.c index ab7b69c11d40..43ab560de7f9 100644 --- a/drivers/gpu/drm/nouveau/dispnv04/crtc.c +++ b/drivers/gpu/drm/nouveau/dispnv04/crtc.c @@ -1031,8 +1031,9 @@ nv04_crtc_cursor_move(struct drm_crtc *crtc, int x, int y) return 0; }
-int -nouveau_crtc_set_config(struct drm_mode_set *set) +static int +nouveau_crtc_set_config(struct drm_mode_set *set, + struct drm_modeset_acquire_ctx *ctx) { struct drm_device *dev; struct nouveau_drm *drm; @@ -1049,7 +1050,7 @@ nouveau_crtc_set_config(struct drm_mode_set *set) if (ret < 0 && ret != -EACCES) return ret;
- ret = drm_crtc_helper_set_config(set); + ret = drm_crtc_helper_set_config(set, ctx);
drm = nouveau_drm(dev);
diff --git a/drivers/gpu/drm/nouveau/nouveau_display.h b/drivers/gpu/drm/nouveau/nouveau_display.h index b28426e02adf..201aec2ea5b8 100644 --- a/drivers/gpu/drm/nouveau/nouveau_display.h +++ b/drivers/gpu/drm/nouveau/nouveau_display.h @@ -86,7 +86,6 @@ int nouveau_display_dumb_map_offset(struct drm_file *, struct drm_device *,
void nouveau_hdmi_mode_set(struct drm_encoder *, struct drm_display_mode *);
-int nouveau_crtc_set_config(struct drm_mode_set *set); #ifdef CONFIG_DRM_NOUVEAU_BACKLIGHT extern int nouveau_backlight_init(struct drm_device *); extern void nouveau_backlight_exit(struct drm_device *); diff --git a/drivers/gpu/drm/radeon/radeon_display.c b/drivers/gpu/drm/radeon/radeon_display.c index 31020db573d5..146297a702ab 100644 --- a/drivers/gpu/drm/radeon/radeon_display.c +++ b/drivers/gpu/drm/radeon/radeon_display.c @@ -624,7 +624,8 @@ static int radeon_crtc_page_flip_target(struct drm_crtc *crtc, }
static int -radeon_crtc_set_config(struct drm_mode_set *set) +radeon_crtc_set_config(struct drm_mode_set *set, + struct drm_modeset_acquire_ctx *ctx) { struct drm_device *dev; struct radeon_device *rdev; @@ -641,7 +642,7 @@ radeon_crtc_set_config(struct drm_mode_set *set) if (ret < 0) return ret;
- ret = drm_crtc_helper_set_config(set); + ret = drm_crtc_helper_set_config(set, ctx);
list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) if (crtc->enabled) diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c b/drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c index 3806148e1bdb..08a66f0db2ec 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c @@ -208,7 +208,8 @@ static int vmw_ldu_add_active(struct vmw_private *vmw_priv, return 0; }
-static int vmw_ldu_crtc_set_config(struct drm_mode_set *set) +static int vmw_ldu_crtc_set_config(struct drm_mode_set *set, + struct drm_modeset_acquire_ctx *ctx) { struct vmw_private *dev_priv; struct vmw_legacy_display_unit *ldu; diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c b/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c index 53cf3be7a902..e9d3c4b92df7 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c @@ -248,7 +248,8 @@ static int vmw_sou_backing_alloc(struct vmw_private *dev_priv, return ret; }
-static int vmw_sou_crtc_set_config(struct drm_mode_set *set) +static int vmw_sou_crtc_set_config(struct drm_mode_set *set, + struct drm_modeset_acquire_ctx *ctx) { struct vmw_private *dev_priv; struct vmw_screen_object_unit *sou; diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c index 85e12309cb71..b2c9d6ce7ce4 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c @@ -509,7 +509,8 @@ static int vmw_stdu_bind_fb(struct vmw_private *dev_priv, * RETURNS: * 0 on success, error code otherwise */ -static int vmw_stdu_crtc_set_config(struct drm_mode_set *set) +static int vmw_stdu_crtc_set_config(struct drm_mode_set *set, + struct drm_modeset_acquire_ctx *ctx) { struct vmw_private *dev_priv; struct vmw_framebuffer *vfb; diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h index 9675cacb72a3..fd395dc050ee 100644 --- a/include/drm/drm_atomic_helper.h +++ b/include/drm/drm_atomic_helper.h @@ -100,7 +100,8 @@ int drm_atomic_helper_disable_plane(struct drm_plane *plane, struct drm_modeset_acquire_ctx *ctx); int __drm_atomic_helper_disable_plane(struct drm_plane *plane, struct drm_plane_state *plane_state); -int drm_atomic_helper_set_config(struct drm_mode_set *set); +int drm_atomic_helper_set_config(struct drm_mode_set *set, + struct drm_modeset_acquire_ctx *ctx); int __drm_atomic_helper_set_config(struct drm_mode_set *set, struct drm_atomic_state *state);
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index bca3330d8085..352558e62cfa 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -347,7 +347,8 @@ struct drm_crtc_funcs { * * 0 on success or a negative error code on failure. */ - int (*set_config)(struct drm_mode_set *set); + int (*set_config)(struct drm_mode_set *set, + struct drm_modeset_acquire_ctx *ctx);
/** * @page_flip: diff --git a/include/drm/drm_crtc_helper.h b/include/drm/drm_crtc_helper.h index 7506a60df8b1..43505c7b2b3f 100644 --- a/include/drm/drm_crtc_helper.h +++ b/include/drm/drm_crtc_helper.h @@ -44,7 +44,8 @@ #include <drm/drm_modeset_helper.h>
void drm_helper_disable_unused_functions(struct drm_device *dev); -int drm_crtc_helper_set_config(struct drm_mode_set *set); +int drm_crtc_helper_set_config(struct drm_mode_set *set, + struct drm_modeset_acquire_ctx *ctx); bool drm_crtc_helper_set_mode(struct drm_crtc *crtc, struct drm_display_mode *mode, int x, int y,
I missed this one, and looks like it's already in. So a belated: Reviewed-by: Sinclair Yeh syeh@vmware.com
for the vmwgfx part
On Wed, Mar 22, 2017 at 10:50:57PM +0100, Daniel Vetter wrote:
Surprisingly a lot of legacy drivers roll their own, for runtime pm and because vmwgfx.
Also make nouveau's set_config static while at it.
Cc: Sinclair Yeh syeh@vmware.com Cc: Thomas Hellstrom thellstrom@vmware.com Cc: Ben Skeggs bskeggs@redhat.com Cc: Patrik Jakobsson patrik.r.jakobsson@gmail.com Cc: Alex Deucher alexander.deucher@amd.com Cc: Christian König christian.koenig@amd.com
Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch
drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 5 +++-- drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h | 3 ++- drivers/gpu/drm/drm_atomic_helper.c | 4 +++- drivers/gpu/drm/drm_crtc.c | 2 +- drivers/gpu/drm/drm_crtc_helper.c | 4 +++- drivers/gpu/drm/drm_plane_helper.c | 2 +- drivers/gpu/drm/gma500/gma_display.c | 7 ++++--- drivers/gpu/drm/gma500/gma_display.h | 3 ++- drivers/gpu/drm/nouveau/dispnv04/crtc.c | 7 ++++--- drivers/gpu/drm/nouveau/nouveau_display.h | 1 - drivers/gpu/drm/radeon/radeon_display.c | 5 +++-- drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c | 3 ++- drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c | 3 ++- drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c | 3 ++- include/drm/drm_atomic_helper.h | 3 ++- include/drm/drm_crtc.h | 3 ++- include/drm/drm_crtc_helper.h | 3 ++- 17 files changed, 38 insertions(+), 23 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c index 7b4fe91d3aec..ce15721cadda 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c @@ -333,7 +333,8 @@ int amdgpu_crtc_page_flip_target(struct drm_crtc *crtc, return 0; }
-int amdgpu_crtc_set_config(struct drm_mode_set *set) +int amdgpu_crtc_set_config(struct drm_mode_set *set,
struct drm_modeset_acquire_ctx *ctx){ struct drm_device *dev; struct amdgpu_device *adev; @@ -350,7 +351,7 @@ int amdgpu_crtc_set_config(struct drm_mode_set *set) if (ret < 0) return ret;
- ret = drm_crtc_helper_set_config(set);
ret = drm_crtc_helper_set_config(set, ctx);
list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) if (crtc->enabled)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h index d19b803ba509..20d6522fd7b4 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h @@ -593,7 +593,8 @@ int amdgpu_align_pitch(struct amdgpu_device *adev, int width, int bpp, bool tile /* amdgpu_display.c */ void amdgpu_print_display_setup(struct drm_device *dev); int amdgpu_modeset_create_props(struct amdgpu_device *adev); -int amdgpu_crtc_set_config(struct drm_mode_set *set); +int amdgpu_crtc_set_config(struct drm_mode_set *set,
struct drm_modeset_acquire_ctx *ctx);int amdgpu_crtc_page_flip_target(struct drm_crtc *crtc, struct drm_framebuffer *fb, struct drm_pending_vblank_event *event, diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 08d10abcece0..b502e2809ebd 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -2266,6 +2266,7 @@ static int update_output_state(struct drm_atomic_state *state, /**
- drm_atomic_helper_set_config - set a new config from userspace
- @set: mode set configuration
- @ctx: lock acquisition context
- Provides a default crtc set_config handler using the atomic driver interface.
@@ -2278,7 +2279,8 @@ static int update_output_state(struct drm_atomic_state *state,
- Returns:
- Returns 0 on success, negative errno numbers on failure.
*/ -int drm_atomic_helper_set_config(struct drm_mode_set *set) +int drm_atomic_helper_set_config(struct drm_mode_set *set,
struct drm_modeset_acquire_ctx *ctx){ struct drm_atomic_state *state; struct drm_crtc *crtc = set->crtc; diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index bfaa0e769ea6..3fe1ec23c87e 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -462,7 +462,7 @@ static int __drm_mode_set_config_internal(struct drm_mode_set *set,
fb = set->fb;
- ret = crtc->funcs->set_config(set);
- ret = crtc->funcs->set_config(set, ctx); if (ret == 0) { crtc->primary->crtc = crtc; crtc->primary->fb = fb;
diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c index 8aa8c1084121..4afdf7902eda 100644 --- a/drivers/gpu/drm/drm_crtc_helper.c +++ b/drivers/gpu/drm/drm_crtc_helper.c @@ -476,6 +476,7 @@ drm_crtc_helper_disable(struct drm_crtc *crtc) /**
- drm_crtc_helper_set_config - set a new config from userspace
- @set: mode set configuration
- @ctx: lock acquire context, not used here
- The drm_crtc_helper_set_config() helper function implements the of
- &drm_crtc_funcs.set_config callback for drivers using the legacy CRTC
@@ -510,7 +511,8 @@ drm_crtc_helper_disable(struct drm_crtc *crtc)
- Returns:
- Returns 0 on success, negative errno numbers on failure.
*/ -int drm_crtc_helper_set_config(struct drm_mode_set *set) +int drm_crtc_helper_set_config(struct drm_mode_set *set,
struct drm_modeset_acquire_ctx *ctx){ struct drm_device *dev; struct drm_crtc **save_encoder_crtcs, *new_crtc; diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c index 775e94c30368..b84a295230fc 100644 --- a/drivers/gpu/drm/drm_plane_helper.c +++ b/drivers/gpu/drm/drm_plane_helper.c @@ -371,7 +371,7 @@ int drm_primary_helper_update(struct drm_plane *plane, struct drm_crtc *crtc, * drm_mode_setplane() already handles the basic refcounting for the * framebuffers involved in this operation. */
- ret = crtc->funcs->set_config(&set);
ret = crtc->funcs->set_config(&set, ctx);
kfree(connector_list); return ret;
diff --git a/drivers/gpu/drm/gma500/gma_display.c b/drivers/gpu/drm/gma500/gma_display.c index d1c5642b1c1e..93ff46535c04 100644 --- a/drivers/gpu/drm/gma500/gma_display.c +++ b/drivers/gpu/drm/gma500/gma_display.c @@ -514,17 +514,18 @@ void gma_crtc_destroy(struct drm_crtc *crtc) kfree(gma_crtc); }
-int gma_crtc_set_config(struct drm_mode_set *set) +int gma_crtc_set_config(struct drm_mode_set *set,
struct drm_modeset_acquire_ctx *ctx){ struct drm_device *dev = set->crtc->dev; struct drm_psb_private *dev_priv = dev->dev_private; int ret;
if (!dev_priv->rpm_enabled)
return drm_crtc_helper_set_config(set);
return drm_crtc_helper_set_config(set, ctx);pm_runtime_forbid(&dev->pdev->dev);
- ret = drm_crtc_helper_set_config(set);
ret = drm_crtc_helper_set_config(set, ctx); pm_runtime_allow(&dev->pdev->dev);
return ret;
diff --git a/drivers/gpu/drm/gma500/gma_display.h b/drivers/gpu/drm/gma500/gma_display.h index e72dd08b701b..166e608923db 100644 --- a/drivers/gpu/drm/gma500/gma_display.h +++ b/drivers/gpu/drm/gma500/gma_display.h @@ -79,7 +79,8 @@ extern void gma_crtc_prepare(struct drm_crtc *crtc); extern void gma_crtc_commit(struct drm_crtc *crtc); extern void gma_crtc_disable(struct drm_crtc *crtc); extern void gma_crtc_destroy(struct drm_crtc *crtc); -extern int gma_crtc_set_config(struct drm_mode_set *set); +extern int gma_crtc_set_config(struct drm_mode_set *set,
struct drm_modeset_acquire_ctx *ctx);extern void gma_crtc_save(struct drm_crtc *crtc); extern void gma_crtc_restore(struct drm_crtc *crtc); diff --git a/drivers/gpu/drm/nouveau/dispnv04/crtc.c b/drivers/gpu/drm/nouveau/dispnv04/crtc.c index ab7b69c11d40..43ab560de7f9 100644 --- a/drivers/gpu/drm/nouveau/dispnv04/crtc.c +++ b/drivers/gpu/drm/nouveau/dispnv04/crtc.c @@ -1031,8 +1031,9 @@ nv04_crtc_cursor_move(struct drm_crtc *crtc, int x, int y) return 0; }
-int -nouveau_crtc_set_config(struct drm_mode_set *set) +static int +nouveau_crtc_set_config(struct drm_mode_set *set,
struct drm_modeset_acquire_ctx *ctx){ struct drm_device *dev; struct nouveau_drm *drm; @@ -1049,7 +1050,7 @@ nouveau_crtc_set_config(struct drm_mode_set *set) if (ret < 0 && ret != -EACCES) return ret;
- ret = drm_crtc_helper_set_config(set);
ret = drm_crtc_helper_set_config(set, ctx);
drm = nouveau_drm(dev);
diff --git a/drivers/gpu/drm/nouveau/nouveau_display.h b/drivers/gpu/drm/nouveau/nouveau_display.h index b28426e02adf..201aec2ea5b8 100644 --- a/drivers/gpu/drm/nouveau/nouveau_display.h +++ b/drivers/gpu/drm/nouveau/nouveau_display.h @@ -86,7 +86,6 @@ int nouveau_display_dumb_map_offset(struct drm_file *, struct drm_device *,
void nouveau_hdmi_mode_set(struct drm_encoder *, struct drm_display_mode *);
-int nouveau_crtc_set_config(struct drm_mode_set *set); #ifdef CONFIG_DRM_NOUVEAU_BACKLIGHT extern int nouveau_backlight_init(struct drm_device *); extern void nouveau_backlight_exit(struct drm_device *); diff --git a/drivers/gpu/drm/radeon/radeon_display.c b/drivers/gpu/drm/radeon/radeon_display.c index 31020db573d5..146297a702ab 100644 --- a/drivers/gpu/drm/radeon/radeon_display.c +++ b/drivers/gpu/drm/radeon/radeon_display.c @@ -624,7 +624,8 @@ static int radeon_crtc_page_flip_target(struct drm_crtc *crtc, }
static int -radeon_crtc_set_config(struct drm_mode_set *set) +radeon_crtc_set_config(struct drm_mode_set *set,
struct drm_modeset_acquire_ctx *ctx){ struct drm_device *dev; struct radeon_device *rdev; @@ -641,7 +642,7 @@ radeon_crtc_set_config(struct drm_mode_set *set) if (ret < 0) return ret;
- ret = drm_crtc_helper_set_config(set);
ret = drm_crtc_helper_set_config(set, ctx);
list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) if (crtc->enabled)
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c b/drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c index 3806148e1bdb..08a66f0db2ec 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c @@ -208,7 +208,8 @@ static int vmw_ldu_add_active(struct vmw_private *vmw_priv, return 0; }
-static int vmw_ldu_crtc_set_config(struct drm_mode_set *set) +static int vmw_ldu_crtc_set_config(struct drm_mode_set *set,
struct drm_modeset_acquire_ctx *ctx){ struct vmw_private *dev_priv; struct vmw_legacy_display_unit *ldu; diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c b/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c index 53cf3be7a902..e9d3c4b92df7 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c @@ -248,7 +248,8 @@ static int vmw_sou_backing_alloc(struct vmw_private *dev_priv, return ret; }
-static int vmw_sou_crtc_set_config(struct drm_mode_set *set) +static int vmw_sou_crtc_set_config(struct drm_mode_set *set,
struct drm_modeset_acquire_ctx *ctx){ struct vmw_private *dev_priv; struct vmw_screen_object_unit *sou; diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c index 85e12309cb71..b2c9d6ce7ce4 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c @@ -509,7 +509,8 @@ static int vmw_stdu_bind_fb(struct vmw_private *dev_priv,
- RETURNS:
- 0 on success, error code otherwise
*/ -static int vmw_stdu_crtc_set_config(struct drm_mode_set *set) +static int vmw_stdu_crtc_set_config(struct drm_mode_set *set,
struct drm_modeset_acquire_ctx *ctx){ struct vmw_private *dev_priv; struct vmw_framebuffer *vfb; diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h index 9675cacb72a3..fd395dc050ee 100644 --- a/include/drm/drm_atomic_helper.h +++ b/include/drm/drm_atomic_helper.h @@ -100,7 +100,8 @@ int drm_atomic_helper_disable_plane(struct drm_plane *plane, struct drm_modeset_acquire_ctx *ctx); int __drm_atomic_helper_disable_plane(struct drm_plane *plane, struct drm_plane_state *plane_state); -int drm_atomic_helper_set_config(struct drm_mode_set *set); +int drm_atomic_helper_set_config(struct drm_mode_set *set,
struct drm_modeset_acquire_ctx *ctx);int __drm_atomic_helper_set_config(struct drm_mode_set *set, struct drm_atomic_state *state);
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index bca3330d8085..352558e62cfa 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -347,7 +347,8 @@ struct drm_crtc_funcs { * * 0 on success or a negative error code on failure. */
- int (*set_config)(struct drm_mode_set *set);
int (*set_config)(struct drm_mode_set *set,
struct drm_modeset_acquire_ctx *ctx);/**
- @page_flip:
diff --git a/include/drm/drm_crtc_helper.h b/include/drm/drm_crtc_helper.h index 7506a60df8b1..43505c7b2b3f 100644 --- a/include/drm/drm_crtc_helper.h +++ b/include/drm/drm_crtc_helper.h @@ -44,7 +44,8 @@ #include <drm/drm_modeset_helper.h>
void drm_helper_disable_unused_functions(struct drm_device *dev); -int drm_crtc_helper_set_config(struct drm_mode_set *set); +int drm_crtc_helper_set_config(struct drm_mode_set *set,
struct drm_modeset_acquire_ctx *ctx);bool drm_crtc_helper_set_mode(struct drm_crtc *crtc, struct drm_display_mode *mode, int x, int y, -- 2.11.0
Another one bites the dust.
Again let's not forget to remove the temporary hidden acquire_ctx assignment, now that we pass this all around explicitly it can go away again.
Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/drm_atomic_helper.c | 21 ++------------------- drivers/gpu/drm/drm_crtc.c | 1 - 2 files changed, 2 insertions(+), 20 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index b502e2809ebd..2df4827305d4 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -2291,32 +2291,15 @@ int drm_atomic_helper_set_config(struct drm_mode_set *set, return -ENOMEM;
state->legacy_set_config = true; - state->acquire_ctx = crtc->dev->mode_config.acquire_ctx; -retry: + state->acquire_ctx = ctx; ret = __drm_atomic_helper_set_config(set, state); if (ret != 0) - goto fail; + return ret;
ret = drm_atomic_commit(state); -fail: - if (ret == -EDEADLK) - goto backoff;
drm_atomic_state_put(state); return ret; - -backoff: - drm_atomic_state_clear(state); - drm_atomic_legacy_backoff(state); - - /* - * Someone might have exchanged the framebuffer while we dropped locks - * in the backoff code. We need to fix up the fb refcount tracking the - * core does for us. - */ - crtc->primary->old_fb = crtc->primary->fb; - - goto retry; } EXPORT_SYMBOL(drm_atomic_helper_set_config);
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 3fe1ec23c87e..46e5b97a8c87 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -578,7 +578,6 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data, ret = drm_modeset_lock_all_ctx(crtc->dev, &ctx); if (ret) goto out; - dev->mode_config.acquire_ctx = &ctx; if (crtc_req->mode_valid) { /* If we have a mode we need a framebuffer. */ /* If we pass -1, set the mode with the currently bound fb */
Patches 2-5, 9-12, 14-19: Reviewed-by: Harry Wentland harry.wentland@amd.com Patches 1,13: Questions in response to those patches Patches 6-8: Acked-by: Harry Wentland harry.wentland@amd.com
Looks like a nice cleanup.
Harry
On Wednesday, March 22, 2017 10:50:39 PM EDT Daniel Vetter wrote:
Hi all,
This is something I kinda had on my todo list ever since atomic landed. The legacy_backoff() hack really doesn't work if you need to acquire additional locks, which does restrict drivers in how they handle and protect legacy paths, and kinda forces us to be overzealous with taking locks for legacy paths, just in case.
This patch set here fixes this, with 2 huge exceptions:
- get/set_property calls aren't fixed. The locking in there is a mess and
needs some serious attention. My goal would be that for atomic we take no lock at all, and entirely rely upon the magic of drm_modeset_lock and atomic to just grab the minimal set required to update a property.
- fbdev emulation helpers. It abused the modeset_lock_all bkl as its own
lock, which prevents us from pushing it down just around the (atomic) modeset calls, and hence from switching over to handling the acquire context in an explicit fashion. Thierry started to fix this with the addition of proper locking for fbdev emulation, but it needs a pile more work.
Survived light testing with full ww mutex debugging, I'll rely on CI to catch the remaining mixups :-)
Cheers, Daniel
Daniel Vetter (19): drm: Wire up proper acquire ctx for plane functions drm: Add acquire ctx parameter to ->update_plane drm: drm_plane_force_disable is not for atomic drivers drm: Add acquire ctx parameter to ->plane_disable drm/atomic-helper: remove backoff hack from disable/update_plane drm/vmwgfx: Drop the cursor locking hack drm/tegra: Don't use modeset_lock_crtc drm/tilcdc: Drop calls to modeset_lock_crtc drm: Make drm_modeset_lock_crtc internal drm: Roll out acquire context for the page_flip ioctl drm: Add acquire ctx parameter to ->page_flip(_target) drm/atomic-helper: remove backoff hack from page_flip drm: simplify the locking in the GETCRTC ioctl drm: Remove drm_modeset_(un)lock_crtc drm: Remove drm_modeset_legacy_acquire_ctx and crtc->acquire_ctx drm: Restrict drm_mode_set_config_internal to non-atomic drivers drm: Add explicit acquire ctx handling around ->set_config drm: Add acquire ctx parameter to ->set_config drm/atomic-helper: Remove the backoff hack from set_config
drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 8 +- drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h | 6 +- drivers/gpu/drm/armada/armada_crtc.c | 3 +- drivers/gpu/drm/armada/armada_overlay.c | 6 +- drivers/gpu/drm/bochs/bochs_kms.c | 3 +- drivers/gpu/drm/drm_atomic.c | 14 --- drivers/gpu/drm/drm_atomic_helper.c | 132 +++++----------------------- drivers/gpu/drm/drm_crtc.c | 61 ++++++++----- drivers/gpu/drm/drm_crtc_helper.c | 4 +- drivers/gpu/drm/drm_modeset_lock.c | 102 --------------------- drivers/gpu/drm/drm_plane.c | 87 ++++++++++++++---- drivers/gpu/drm/drm_plane_helper.c | 11 ++- drivers/gpu/drm/gma500/gma_display.c | 7 +- drivers/gpu/drm/gma500/gma_display.h | 3 +- drivers/gpu/drm/i915/intel_display.c | 9 +- drivers/gpu/drm/i915/intel_pipe_crc.c | 2 +- drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c | 8 +- drivers/gpu/drm/nouveau/dispnv04/crtc.c | 7 +- drivers/gpu/drm/nouveau/dispnv04/overlay.c | 18 ++-- drivers/gpu/drm/nouveau/nouveau_display.c | 3 +- drivers/gpu/drm/nouveau/nouveau_display.h | 4 +- drivers/gpu/drm/radeon/radeon_display.c | 8 +- drivers/gpu/drm/shmobile/shmob_drm_crtc.c | 3 +- drivers/gpu/drm/shmobile/shmob_drm_plane.c | 8 +- drivers/gpu/drm/tegra/dc.c | 8 +- drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 12 +-- drivers/gpu/drm/udl/udl_modeset.c | 3 +- drivers/gpu/drm/vc4/vc4_crtc.c | 5 +- drivers/gpu/drm/vc4/vc4_plane.c | 6 +- drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 25 ------ drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c | 3 +- drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c | 6 +- drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c | 6 +- include/drm/drm_atomic_helper.h | 15 ++-- include/drm/drm_crtc.h | 18 ++-- include/drm/drm_crtc_helper.h | 3 +- include/drm/drm_modeset_lock.h | 5 -- include/drm/drm_plane.h | 7 +- include/drm/drm_plane_helper.h | 6 +- 39 files changed, 265 insertions(+), 380 deletions(-)
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Daniel Vetter -
Harry Wentland -
Jyri Sarha -
Maarten Lankhorst -
Michel Dänzer -
Pandiyan, Dhinakaran -
Russell King - ARM Linux -
Sinclair Yeh -
Thomas Hellstrom -
Tomi Valkeinen