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

Message ID 20170322215058.8671-2-daniel.vetter@ffwll.ch
State New
Headers show

Commit Message

Daniel Vetter March 22, 2017, 9:50 p.m. UTC
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(-)

Comments

Harry Wentland March 27, 2017, 8:12 p.m. UTC | #1
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;
Daniel Vetter March 28, 2017, 6:23 a.m. UTC | #2
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 March 28, 2017, 6:46 a.m. UTC | #3
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 March 28, 2017, 7:02 a.m. UTC | #4
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
Harry Wentland March 28, 2017, 2:48 p.m. UTC | #5
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
>

Patch
diff mbox

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;