[Intel-gfx] [PATCH] drm: Fixup locking for universal cursor planes

[Matt Roper]

On Fri, Sep 12, 2014 at 05:07:32PM +0200, Daniel Vetter wrote:
> Bunch of things amiss:
> - Updating crtc->cursor_x/y was done without any locking. Spotted by
>   David Herrmann.
> - Dereferencing crtc->cursor->fb was using the wrong lock, should take
>   the crtc lock.
> - Grabbing _all_ modeset locks torpedoes the reason why we added
>   fine-grained locks originally: Cursor updates shouldn't stall on
>   background stuff like probing outputs.
> 
> Best is to just grab the crtc lock around everything and drop all the
> other locking. The only issue is that we can't switch planes between
> crtcs with that, so make sure that never happens when someone uses
> universal plane helpers. This shouldn't be a possible regression ever
> since legacy ioctls also only grabbed the crtc lock, so switching
> crtcs was never possible for the underlying plane object. And i915
> (the only user of universal cursors thus far) has fixed cursor->crtc
> links.
> 
> Cc: David Herrmann <dh.herrmann at gmail.com>
> Cc: Pallavi G<pallavi.g at intel.com>
> Cc: Matt Roper <matthew.d.roper at intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>
> 
> --
> Totally untested, but should aim in the right direction. Roughly.
> 
> Cheers, Daniel

Looks good to me.  I also ran this through the igt tests kms_plane,
kms_universal_plane, and kms_cursor_crc to make sure that we didn't
accidentally break something and they all ran fine.

So

Reviewed-by: Matt Roper <matthew.d.roper at intel.com>

and

Tested-by: Matt Roper <matthew.d.roper at intel.com>



> ---
>  drivers/gpu/drm/drm_crtc.c | 51 ++++++++++++++++++++++++++++++----------------
>  1 file changed, 34 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index b1271a8d8ce7..8c1870114a07 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -2262,21 +2262,19 @@ out:
>   *
>   * src_{x,y,w,h} are provided in 16.16 fixed point format
>   */
> -static int setplane_internal(struct drm_plane *plane,
> -			     struct drm_crtc *crtc,
> -			     struct drm_framebuffer *fb,
> -			     int32_t crtc_x, int32_t crtc_y,
> -			     uint32_t crtc_w, uint32_t crtc_h,
> -			     /* src_{x,y,w,h} values are 16.16 fixed point */
> -			     uint32_t src_x, uint32_t src_y,
> -			     uint32_t src_w, uint32_t src_h)
> +static int __setplane_internal(struct drm_plane *plane,
> +			       struct drm_crtc *crtc,
> +			       struct drm_framebuffer *fb,
> +			       int32_t crtc_x, int32_t crtc_y,
> +			       uint32_t crtc_w, uint32_t crtc_h,
> +			       /* src_{x,y,w,h} values are 16.16 fixed point */
> +			       uint32_t src_x, uint32_t src_y,
> +			       uint32_t src_w, uint32_t src_h)
>  {
> -	struct drm_device *dev = plane->dev;
>  	int ret = 0;
>  	unsigned int fb_width, fb_height;
>  	int i;
>  
> -	drm_modeset_lock_all(dev);
>  	/* No fb means shut it down */
>  	if (!fb) {
>  		plane->old_fb = plane->fb;
> @@ -2344,10 +2342,28 @@ out:
>  	if (plane->old_fb)
>  		drm_framebuffer_unreference(plane->old_fb);
>  	plane->old_fb = NULL;
> -	drm_modeset_unlock_all(dev);
>  
>  	return ret;
> +}
>  
> +static int setplane_internal(struct drm_plane *plane,
> +			     struct drm_crtc *crtc,
> +			     struct drm_framebuffer *fb,
> +			     int32_t crtc_x, int32_t crtc_y,
> +			     uint32_t crtc_w, uint32_t crtc_h,
> +			     /* src_{x,y,w,h} values are 16.16 fixed point */
> +			     uint32_t src_x, uint32_t src_y,
> +			     uint32_t src_w, uint32_t src_h)
> +{
> +	int ret;
> +
> +	drm_modeset_lock_all(plane->dev);
> +	ret = __setplane_internal(plane, crtc, fb,
> +				  crtc_x, crtc_y, crtc_w, crtc_h,
> +				  src_x, src_y, src_w, src_h);
> +	drm_modeset_unlock_all(plane->dev);
> +
> +	return ret;
>  }
>  
>  /**
> @@ -2713,6 +2729,7 @@ static int drm_mode_cursor_universal(struct drm_crtc *crtc,
>  	int ret = 0;
>  
>  	BUG_ON(!crtc->cursor);
> +	WARN_ON(crtc->cursor->crtc != crtc && crtc->cursor->crtc != NULL);
>  
>  	/*
>  	 * Obtain fb we'll be using (either new or existing) and take an extra
> @@ -2732,11 +2749,9 @@ static int drm_mode_cursor_universal(struct drm_crtc *crtc,
>  			fb = NULL;
>  		}
>  	} else {
> -		mutex_lock(&dev->mode_config.mutex);
>  		fb = crtc->cursor->fb;
>  		if (fb)
>  			drm_framebuffer_reference(fb);
> -		mutex_unlock(&dev->mode_config.mutex);
>  	}
>  
>  	if (req->flags & DRM_MODE_CURSOR_MOVE) {
> @@ -2758,7 +2773,7 @@ static int drm_mode_cursor_universal(struct drm_crtc *crtc,
>  	 * setplane_internal will take care of deref'ing either the old or new
>  	 * framebuffer depending on success.
>  	 */
> -	ret = setplane_internal(crtc->cursor, crtc, fb,
> +	ret = __setplane_internal(crtc->cursor, crtc, fb,
>  				crtc_x, crtc_y, crtc_w, crtc_h,
>  				0, 0, src_w, src_h);
>  
> @@ -2794,10 +2809,12 @@ static int drm_mode_cursor_common(struct drm_device *dev,
>  	 * If this crtc has a universal cursor plane, call that plane's update
>  	 * handler rather than using legacy cursor handlers.
>  	 */
> -	if (crtc->cursor)
> -		return drm_mode_cursor_universal(crtc, req, file_priv);
> -
>  	drm_modeset_lock_crtc(crtc);
> +	if (crtc->cursor) {
> +		ret = drm_mode_cursor_universal(crtc, req, file_priv);
> +		goto out;
> +	}
> +
>  	if (req->flags & DRM_MODE_CURSOR_BO) {
>  		if (!crtc->funcs->cursor_set && !crtc->funcs->cursor_set2) {
>  			ret = -ENXIO;
> -- 
> 2.0.1
> 

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795