[PATCH 00/19] wire acquire ctx through legacy modeset paths

[PATCH 00/19] wire acquire ctx through legacy modeset paths

[Daniel Vetter]

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(-)

-- 
2.11.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 01/19] drm: Wire up proper acquire ctx for plane functions

[Daniel Vetter]

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;
-- 
2.11.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 01/19] drm: Wire up proper acquire ctx for plane functions

[Harry Wentland]

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;


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 01/19] drm: Wire up proper acquire ctx for plane functions

[Daniel Vetter]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 01/19] drm: Wire up proper acquire ctx for plane functions

[Daniel Vetter]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 01/19] drm: Wire up proper acquire ctx for plane functions

[Daniel Vetter]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 01/19] drm: Wire up proper acquire ctx for plane functions

[Harry Wentland]

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
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 02/19] drm: Add acquire ctx parameter to ->update_plane

[Daniel Vetter]

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;
-- 
2.11.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 02/19] drm: Add acquire ctx parameter to ->update_plane

[Russell King - ARM Linux]

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.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 02/19] drm: Add acquire ctx parameter to ->update_plane

[Daniel Vetter]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 03/19] drm: drm_plane_force_disable is not for atomic drivers

[Daniel Vetter]

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) {
-- 
2.11.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 04/19] drm: Add acquire ctx parameter to ->plane_disable

[Daniel Vetter]

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;
 
-- 
2.11.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 05/19] drm/atomic-helper: remove backoff hack from disable/update_plane

[Daniel Vetter]

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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 06/19] drm/vmwgfx: Drop the cursor locking hack

[Daniel Vetter]

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;
 }
 
-- 
2.11.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 06/19] drm/vmwgfx: Drop the cursor locking hack

[Thomas Hellstrom]

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;
>  }
>  


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 06/19] drm/vmwgfx: Drop the cursor locking hack

[Daniel Vetter]

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;
> >  }
> >  
> 
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 06/19] drm/vmwgfx: Drop the cursor locking hack

[Daniel Vetter]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 06/19] drm/vmwgfx: Drop the cursor locking hack

[Thomas Hellstrom]

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



_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 06/19] drm/vmwgfx: Drop the cursor locking hack

[Daniel Vetter]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 06/19] drm/vmwgfx: Drop the cursor locking hack

[Thomas Hellstrom]

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



_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 06/19] drm/vmwgfx: Drop the cursor locking hack

[Daniel Vetter]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 06/19] drm/vmwgfx: Drop the cursor locking hack

[Michel Dänzer]

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.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 06/19] drm/vmwgfx: Drop the cursor locking hack

[Daniel Vetter]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 06/19] drm/vmwgfx: Drop the cursor locking hack

[Thomas Hellstrom]

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



_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 06/19] drm/vmwgfx: Drop the cursor locking hack

[Daniel Vetter]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 06/19] drm/vmwgfx: Drop the cursor locking hack

[Thomas Hellstrom]

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


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 07/19] drm/tegra: Don't use modeset_lock_crtc

[Daniel Vetter]

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;
 }
 
-- 
2.11.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 07/19] drm/tegra: Don't use modeset_lock_crtc

[Daniel Vetter]

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
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 08/19] drm/tilcdc: Drop calls to modeset_lock_crtc

[Daniel Vetter]

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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 08/19] drm/tilcdc: Drop calls to modeset_lock_crtc

[Tomi Valkeinen]

[-- Attachment #1.1.1: Type: text/plain, Size: 441 bytes --]

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


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 08/19] drm/tilcdc: Drop calls to modeset_lock_crtc

[Daniel Vetter]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 08/19] drm/tilcdc: Drop calls to modeset_lock_crtc

[Jyri Sarha]

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
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 09/19] drm: Make drm_modeset_lock_crtc internal

[Daniel Vetter]

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 *
-- 
2.11.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 10/19] drm: Roll out acquire context for the page_flip ioctl

[Daniel Vetter]

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;
 }
-- 
2.11.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 11/19] drm: Add acquire ctx parameter to ->page_flip(_target)

[Daniel Vetter]

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.com>t
---
 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:
-- 
2.11.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 12/19] drm/atomic-helper: remove backoff hack from page_flip

[Daniel Vetter]

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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 13/19] drm: simplify the locking in the GETCRTC ioctl

[Daniel Vetter]

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;
 }
-- 
2.11.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 13/19] drm: simplify the locking in the GETCRTC ioctl

[Harry Wentland]

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;
>  }


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 13/19] drm: simplify the locking in the GETCRTC ioctl

[Daniel Vetter]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] drm: simplify the locking in the GETCRTC ioctl

[Daniel Vetter]

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 =
-- 
2.11.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: simplify the locking in the GETCRTC ioctl

[Harry Wentland]

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 =
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] Revert unrelated part of "drm: simplify the locking in the GETCRTC ioctl"

[Maarten Lankhorst]

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 =

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] Revert unrelated part of "drm: simplify the locking in the GETCRTC ioctl"

[Pandiyan, Dhinakaran]

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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] Revert unrelated part of "drm: simplify the locking in the GETCRTC ioctl"

[Daniel Vetter]

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
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 14/19] drm: Remove drm_modeset_(un)lock_crtc

[Daniel Vetter]

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);
-- 
2.11.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 15/19] drm: Remove drm_modeset_legacy_acquire_ctx and crtc->acquire_ctx

[Daniel Vetter]

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);
-- 
2.11.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 16/19] drm: Restrict drm_mode_set_config_internal to non-atomic drivers

[Daniel Vetter]

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)
-- 
2.11.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 17/19] drm: Add explicit acquire ctx handling around ->set_config

[Daniel Vetter]

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;
 }
 
-- 
2.11.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 18/19] drm: Add acquire ctx parameter to ->set_config

[Daniel Vetter]

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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 18/19] drm: Add acquire ctx parameter to ->set_config

[Sinclair Yeh]

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
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 19/19] drm/atomic-helper: Remove the backoff hack from set_config

[Daniel Vetter]

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 */
-- 
2.11.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

✗ Fi.CI.BAT: warning for wire acquire ctx through legacy modeset paths

[Patchwork]

== Series Details ==

Series: wire acquire ctx through legacy modeset paths
URL   : https://patchwork.freedesktop.org/series/21710/
State : warning

== Summary ==

Series 21710v1 wire acquire ctx through legacy modeset paths
https://patchwork.freedesktop.org/api/1.0/series/21710/revisions/1/mbox/

Test drv_module_reload:
        Subgroup basic-reload-final:
                pass       -> DMESG-WARN (fi-skl-6770hq) fdo#100248
Test kms_busy:
        Subgroup basic-flip-default-a:
                pass       -> DMESG-WARN (fi-bdw-gvtdvm)
                pass       -> DMESG-WARN (fi-skl-gvtdvm)
                pass       -> DMESG-WARN (fi-kbl-7500u)
                pass       -> DMESG-WARN (fi-snb-2600)
                pass       -> DMESG-WARN (fi-skl-6700k)
                pass       -> DMESG-WARN (fi-ivb-3770)
                pass       -> DMESG-WARN (fi-skl-6260u)
                pass       -> DMESG-WARN (fi-bxt-j4205)
                pass       -> DMESG-WARN (fi-snb-2520m)
                pass       -> DMESG-WARN (fi-ivb-3520m)
                pass       -> DMESG-WARN (fi-byt-j1900)
                pass       -> DMESG-WARN (fi-skl-6700hq)
                pass       -> DMESG-WARN (fi-skl-6770hq)
                pass       -> DMESG-WARN (fi-bdw-5557u)
                pass       -> DMESG-WARN (fi-byt-n2820) fdo#100117
                pass       -> DMESG-WARN (fi-ilk-650)
                pass       -> DMESG-WARN (fi-hsw-4770)
                pass       -> DMESG-WARN (fi-hsw-4770r)
        Subgroup basic-flip-default-b:
                pass       -> DMESG-WARN (fi-bdw-gvtdvm)
                pass       -> DMESG-WARN (fi-skl-gvtdvm)
                pass       -> DMESG-WARN (fi-kbl-7500u)
                pass       -> DMESG-WARN (fi-snb-2600)
                pass       -> DMESG-WARN (fi-skl-6700k)
                pass       -> DMESG-WARN (fi-ivb-3770)
                pass       -> DMESG-WARN (fi-skl-6260u)
                pass       -> DMESG-WARN (fi-bxt-j4205) fdo#99728
                pass       -> DMESG-WARN (fi-snb-2520m)
                pass       -> DMESG-WARN (fi-ivb-3520m)
                pass       -> DMESG-WARN (fi-byt-j1900)
                pass       -> DMESG-WARN (fi-skl-6700hq)
                pass       -> DMESG-WARN (fi-skl-6770hq)
                pass       -> DMESG-WARN (fi-bdw-5557u)
                pass       -> DMESG-WARN (fi-byt-n2820)
                pass       -> DMESG-WARN (fi-ilk-650)
                pass       -> DMESG-WARN (fi-hsw-4770)
                pass       -> DMESG-WARN (fi-hsw-4770r)
        Subgroup basic-flip-default-c:
                pass       -> DMESG-WARN (fi-bdw-5557u)
                pass       -> DMESG-WARN (fi-skl-gvtdvm)
                pass       -> DMESG-WARN (fi-kbl-7500u)
                pass       -> DMESG-WARN (fi-bdw-gvtdvm)
                pass       -> DMESG-WARN (fi-skl-6700k)
                pass       -> DMESG-WARN (fi-ivb-3770)
                pass       -> DMESG-WARN (fi-skl-6260u)
                pass       -> DMESG-WARN (fi-bsw-n3050)
                pass       -> DMESG-WARN (fi-bxt-j4205) fdo#99728
                pass       -> DMESG-WARN (fi-skl-6700hq)
                pass       -> DMESG-WARN (fi-skl-6770hq)
                pass       -> DMESG-WARN (fi-ivb-3520m)
                pass       -> DMESG-WARN (fi-hsw-4770r)
                pass       -> DMESG-WARN (fi-hsw-4770)
Test kms_cursor_legacy:
        Subgroup basic-busy-flip-before-cursor-legacy:
                pass       -> DMESG-WARN (fi-byt-j1900)
                pass       -> DMESG-WARN (fi-byt-n2820)
                pass       -> DMESG-WARN (fi-bsw-n3050)
        Subgroup basic-flip-after-cursor-legacy:
                pass       -> DMESG-WARN (fi-byt-j1900)
                pass       -> DMESG-WARN (fi-byt-n2820)
                pass       -> DMESG-WARN (fi-bsw-n3050)
        Subgroup basic-flip-after-cursor-varying-size:
                pass       -> DMESG-WARN (fi-byt-j1900)
                pass       -> DMESG-WARN (fi-byt-n2820) fdo#100094
                pass       -> DMESG-WARN (fi-bsw-n3050)
        Subgroup basic-flip-before-cursor-legacy:
                pass       -> DMESG-WARN (fi-byt-j1900)
                pass       -> DMESG-WARN (fi-byt-n2820) fdo#100094
                pass       -> DMESG-WARN (fi-bsw-n3050)
        Subgroup basic-flip-before-cursor-varying-size:
                pass       -> DMESG-WARN (fi-byt-j1900)
                pass       -> DMESG-WARN (fi-byt-n2820) fdo#100094
                pass       -> DMESG-WARN (fi-bsw-n3050)
Test kms_flip:
        Subgroup basic-flip-vs-dpms:
                pass       -> DMESG-WARN (fi-snb-2600)
                pass       -> DMESG-WARN (fi-bxt-j4205)
                pass       -> DMESG-WARN (fi-kbl-7500u) fdo#100179
                pass       -> DMESG-WARN (fi-bdw-5557u)
                pass       -> DMESG-WARN (fi-skl-6700k)
                pass       -> DMESG-WARN (fi-ivb-3770)
                pass       -> DMESG-WARN (fi-skl-6260u)
                pass       -> DMESG-WARN (fi-snb-2520m)
                pass       -> DMESG-WARN (fi-hsw-4770r)
                pass       -> DMESG-WARN (fi-byt-j1900)
                pass       -> DMESG-WARN (fi-skl-6700hq)
                pass       -> DMESG-WARN (fi-skl-6770hq)
                pass       -> DMESG-WARN (fi-ivb-3520m)
                pass       -> DMESG-WARN (fi-byt-n2820)
                pass       -> DMESG-WARN (fi-ilk-650)
                pass       -> DMESG-WARN (fi-hsw-4770)
        Subgroup basic-flip-vs-modeset:
WARNING: Long output truncated

b1ecc49ac71e484430083e5b3b7cd75d4976a465 drm-tip: 2017y-03m-23d-07h-40m-49s UTC integration manifest
99a3f73 drm: Add explicit acquire ctx handling around ->set_config
d8d4c9a drm: Restrict drm_mode_set_config_internal to non-atomic drivers
e797375 drm: Remove drm_modeset_legacy_acquire_ctx and crtc->acquire_ctx
52537eb drm: Remove drm_modeset_(un)lock_crtc
f1fe6c2 drm: simplify the locking in the GETCRTC ioctl
4f61441 drm/atomic-helper: remove backoff hack from page_flip
1f4aff40 drm: Add acquire ctx parameter to ->page_flip(_target)
abe7a2c drm: Roll out acquire context for the page_flip ioctl
6b1fe44 drm: Make drm_modeset_lock_crtc internal
4a86e4c drm/tilcdc: Drop calls to modeset_lock_crtc
f6e61314 drm/tegra: Don't use modeset_lock_crtc
59be23e drm/vmwgfx: Drop the cursor locking hack
930bf17 drm/atomic-helper: remove backoff hack from disable/update_plane
bf4c74f drm: Add acquire ctx parameter to ->plane_disable
d795236 drm: drm_plane_force_disable is not for atomic drivers
79d5911 drm: Add acquire ctx parameter to ->update_plane
8e10b2d drm: Wire up proper acquire ctx for plane functions

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4268/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 00/19] wire acquire ctx through legacy modeset paths

[Harry Wentland]

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 mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

✓ Fi.CI.BAT: success for wire acquire ctx through legacy modeset paths (rev2)

[Patchwork]

== Series Details ==

Series: wire acquire ctx through legacy modeset paths (rev2)
URL   : https://patchwork.freedesktop.org/series/21710/
State : success

== Summary ==

Series 21710v2 wire acquire ctx through legacy modeset paths
https://patchwork.freedesktop.org/api/1.0/series/21710/revisions/2/mbox/

Test gem_exec_flush:
        Subgroup basic-batch-kernel-default-uc:
                pass       -> FAIL       (fi-snb-2600) fdo#100007
Test gem_exec_suspend:
        Subgroup basic-s4-devices:
                pass       -> DMESG-WARN (fi-bxt-t5700) fdo#100125

fdo#100007 https://bugs.freedesktop.org/show_bug.cgi?id=100007
fdo#100125 https://bugs.freedesktop.org/show_bug.cgi?id=100125

fi-bdw-5557u     total:278  pass:267  dwarn:0   dfail:0   fail:0   skip:11  time: 458s
fi-bdw-gvtdvm    total:278  pass:256  dwarn:8   dfail:0   fail:0   skip:14  time: 454s
fi-bsw-n3050     total:278  pass:239  dwarn:0   dfail:0   fail:0   skip:39  time: 589s
fi-bxt-j4205     total:278  pass:259  dwarn:0   dfail:0   fail:0   skip:19  time: 549s
fi-bxt-t5700     total:278  pass:257  dwarn:1   dfail:0   fail:0   skip:20  time: 586s
fi-byt-j1900     total:278  pass:251  dwarn:0   dfail:0   fail:0   skip:27  time: 507s
fi-byt-n2820     total:278  pass:247  dwarn:0   dfail:0   fail:0   skip:31  time: 504s
fi-hsw-4770      total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time: 435s
fi-hsw-4770r     total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time: 443s
fi-ilk-650       total:278  pass:228  dwarn:0   dfail:0   fail:0   skip:50  time: 438s
fi-ivb-3520m     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 512s
fi-ivb-3770      total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 517s
fi-kbl-7500u     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 485s
fi-kbl-7560u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time: 596s
fi-skl-6260u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time: 476s
fi-skl-6700hq    total:278  pass:261  dwarn:0   dfail:0   fail:0   skip:17  time: 602s
fi-skl-6700k     total:278  pass:256  dwarn:4   dfail:0   fail:0   skip:18  time: 482s
fi-skl-6770hq    total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time: 525s
fi-snb-2520m     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time: 556s
fi-snb-2600      total:278  pass:248  dwarn:0   dfail:0   fail:1   skip:29  time: 425s
fi-skl-gvtdvm failed to collect. IGT log at Patchwork_4321/fi-skl-gvtdvm/igt.log

c3ca5620be419c7e20c39d07f6cf7d1cb7c3bfc8 drm-tip: 2017y-03m-27d-20h-31m-03s UTC integration manifest
833d003 drm/atomic-helper: Remove the backoff hack from set_config
b0c8256 drm: Add acquire ctx parameter to ->set_config
f717eae drm: Add explicit acquire ctx handling around ->set_config
5ceb73d drm: Restrict drm_mode_set_config_internal to non-atomic drivers
3775d3a drm: Remove drm_modeset_legacy_acquire_ctx and crtc->acquire_ctx
c4340bc drm: Remove drm_modeset_(un)lock_crtc
799f4b3 drm: simplify the locking in the GETCRTC ioctl
adfabe0 drm/atomic-helper: remove backoff hack from page_flip
5bc3f14 drm: Add acquire ctx parameter to ->page_flip(_target)
e7818c5 drm: Roll out acquire context for the page_flip ioctl
1cf1c67 drm: Make drm_modeset_lock_crtc internal
41a330f drm/vmwgfx: Drop the cursor locking hack
9906efb drm/atomic-helper: remove backoff hack from disable/update_plane
28e9f7d drm: Add acquire ctx parameter to ->plane_disable
6bf89e2 drm: drm_plane_force_disable is not for atomic drivers
bf2007c drm: Add acquire ctx parameter to ->update_plane
17bf3da drm: Wire up proper acquire ctx for plane functions

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4321/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx