Re: [PATCH -v3 2/4] drm/i915: split intel_update_plane into check() and commit()

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Sep 03, 2014 at 05:10:16PM -0300, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> 
> Due to the upcoming atomic modesetting feature we need to separate
> some update functions into a check step that can fail and a commit
> step that should, ideally, never fail.
> 
> This commit splits intel_update_plane() and its commit part can still
> fail due to the fb pinning procedure.

Crap. I wrote a reply and somehow it seems to have gotten lost before I
sent it out. I'll try to recall it all again...

> 
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> ---
>  drivers/gpu/drm/i915/intel_sprite.c | 253 +++++++++++++++++++++---------------
>  1 file changed, 150 insertions(+), 103 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index 07a74ef..7b0d1a9 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -845,57 +845,24 @@ static bool colorkey_enabled(struct intel_plane *intel_plane)
>  }
>  
>  static int
> -intel_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)
> +intel_check_sprite_plane(struct drm_plane *plane,
> +			 struct intel_plane_state *state)
>  {
> -	struct drm_device *dev = plane->dev;
> -	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	struct intel_crtc *intel_crtc = to_intel_crtc(state->crtc);
>  	struct intel_plane *intel_plane = to_intel_plane(plane);
> -	enum pipe pipe = intel_crtc->pipe;
> +	struct drm_framebuffer *fb = state->fb;
>  	struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
>  	struct drm_i915_gem_object *obj = intel_fb->obj;
> -	struct drm_i915_gem_object *old_obj = intel_plane->obj;
> -	int ret;
> -	bool primary_enabled;
> -	bool visible;
> +	int crtc_x, crtc_y;
> +	unsigned int crtc_w, crtc_h;
> +	uint32_t src_x, src_y, src_w, src_h;
> +	struct drm_rect *src = &state->src;
> +	struct drm_rect *dst = &state->dst;
> +	struct drm_rect *orig_src = &state->orig_src;
> +	const struct drm_rect *clip = &state->clip;
>  	int hscale, vscale;
>  	int max_scale, min_scale;
>  	int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
> -	struct drm_rect src = {
> -		/* sample coordinates in 16.16 fixed point */
> -		.x1 = src_x,
> -		.x2 = src_x + src_w,
> -		.y1 = src_y,
> -		.y2 = src_y + src_h,
> -	};
> -	struct drm_rect dst = {
> -		/* integer pixels */
> -		.x1 = crtc_x,
> -		.x2 = crtc_x + crtc_w,
> -		.y1 = crtc_y,
> -		.y2 = crtc_y + crtc_h,
> -	};
> -	const struct drm_rect clip = {
> -		.x2 = intel_crtc->active ? intel_crtc->config.pipe_src_w : 0,
> -		.y2 = intel_crtc->active ? intel_crtc->config.pipe_src_h : 0,
> -	};
> -	const struct {
> -		int crtc_x, crtc_y;
> -		unsigned int crtc_w, crtc_h;
> -		uint32_t src_x, src_y, src_w, src_h;
> -	} orig = {
> -		.crtc_x = crtc_x,
> -		.crtc_y = crtc_y,
> -		.crtc_w = crtc_w,
> -		.crtc_h = crtc_h,
> -		.src_x = src_x,
> -		.src_y = src_y,
> -		.src_w = src_w,
> -		.src_h = src_h,
> -	};
>  
>  	/* Don't modify another pipe's plane */
>  	if (intel_plane->pipe != intel_crtc->pipe) {
> @@ -927,55 +894,55 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>  	max_scale = intel_plane->max_downscale << 16;
>  	min_scale = intel_plane->can_scale ? 1 : (1 << 16);
>  
> -	drm_rect_rotate(&src, fb->width << 16, fb->height << 16,
> +	drm_rect_rotate(src, fb->width << 16, fb->height << 16,
>  			intel_plane->rotation);
>  
> -	hscale = drm_rect_calc_hscale_relaxed(&src, &dst, min_scale, max_scale);
> +	hscale = drm_rect_calc_hscale_relaxed(src, dst, min_scale, max_scale);
>  	BUG_ON(hscale < 0);
>  
> -	vscale = drm_rect_calc_vscale_relaxed(&src, &dst, min_scale, max_scale);
> +	vscale = drm_rect_calc_vscale_relaxed(src, dst, min_scale, max_scale);
>  	BUG_ON(vscale < 0);
>  
> -	visible = drm_rect_clip_scaled(&src, &dst, &clip, hscale, vscale);
> +	state->visible =  drm_rect_clip_scaled(src, dst, clip, hscale, vscale);
>  
> -	crtc_x = dst.x1;
> -	crtc_y = dst.y1;
> -	crtc_w = drm_rect_width(&dst);
> -	crtc_h = drm_rect_height(&dst);
> +	crtc_x = dst->x1;
> +	crtc_y = dst->y1;
> +	crtc_w = drm_rect_width(dst);
> +	crtc_h = drm_rect_height(dst);
>  
> -	if (visible) {
> +	if (state->visible) {
>  		/* check again in case clipping clamped the results */
> -		hscale = drm_rect_calc_hscale(&src, &dst, min_scale, max_scale);
> +		hscale = drm_rect_calc_hscale(src, dst, min_scale, max_scale);
>  		if (hscale < 0) {
>  			DRM_DEBUG_KMS("Horizontal scaling factor out of limits\n");
> -			drm_rect_debug_print(&src, true);
> -			drm_rect_debug_print(&dst, false);
> +			drm_rect_debug_print(src, true);
> +			drm_rect_debug_print(dst, false);
>  
>  			return hscale;
>  		}
>  
> -		vscale = drm_rect_calc_vscale(&src, &dst, min_scale, max_scale);
> +		vscale = drm_rect_calc_vscale(src, dst, min_scale, max_scale);
>  		if (vscale < 0) {
>  			DRM_DEBUG_KMS("Vertical scaling factor out of limits\n");
> -			drm_rect_debug_print(&src, true);
> -			drm_rect_debug_print(&dst, false);
> +			drm_rect_debug_print(src, true);
> +			drm_rect_debug_print(dst, false);
>  
>  			return vscale;
>  		}
>  
>  		/* Make the source viewport size an exact multiple of the scaling factors. */
> -		drm_rect_adjust_size(&src,
> -				     drm_rect_width(&dst) * hscale - drm_rect_width(&src),
> -				     drm_rect_height(&dst) * vscale - drm_rect_height(&src));
> +		drm_rect_adjust_size(src,
> +				     drm_rect_width(dst) * hscale - drm_rect_width(src),
> +				     drm_rect_height(dst) * vscale - drm_rect_height(src));
>  
> -		drm_rect_rotate_inv(&src, fb->width << 16, fb->height << 16,
> +		drm_rect_rotate_inv(src, fb->width << 16, fb->height << 16,
>  				    intel_plane->rotation);
>  
>  		/* sanity check to make sure the src viewport wasn't enlarged */
> -		WARN_ON(src.x1 < (int) src_x ||
> -			src.y1 < (int) src_y ||
> -			src.x2 > (int) (src_x + src_w) ||
> -			src.y2 > (int) (src_y + src_h));
> +		WARN_ON(src->x1 < (int) orig_src->x1 ||
> +			src->y1 < (int) orig_src->y1 ||
> +			src->x2 > (int) orig_src->x2 ||
> +			src->y2 > (int) orig_src->y2);
>  
>  		/*
>  		 * Hardware doesn't handle subpixel coordinates.
> @@ -983,10 +950,10 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>  		 * increase the source viewport size, because that could
>  		 * push the downscaling factor out of bounds.
>  		 */
> -		src_x = src.x1 >> 16;
> -		src_w = drm_rect_width(&src) >> 16;
> -		src_y = src.y1 >> 16;
> -		src_h = drm_rect_height(&src) >> 16;
> +		src_x = src->x1 >> 16;
> +		src_w = drm_rect_width(src) >> 16;
> +		src_y = src->y1 >> 16;
> +		src_h = drm_rect_height(src) >> 16;
>  
>  		if (format_is_yuv(fb->pixel_format)) {
>  			src_x &= ~1;
> @@ -1000,44 +967,74 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>  				crtc_w &= ~1;
>  
>  			if (crtc_w == 0)
> -				visible = false;
> +				state->visible = false;
>  		}
> -	}
>  
> -	/* Check size restrictions when scaling */
> -	if (visible && (src_w != crtc_w || src_h != crtc_h)) {

I'd leave the code structure as is here to avoid changing the
indentation. It just makes it harder to review.

Also in theory the "Source dimensions exceed hardware limits" check
might get triggered for an invisible plane. I think that should be
impossible given fugded scaling coordinates we do, but better safe
than sorry.

> -		unsigned int width_bytes;
> +		/* Check size restrictions when scaling */
> +		if ((src_w != crtc_w || src_h != crtc_h)) {
> +			unsigned int width_bytes;
>  
> -		WARN_ON(!intel_plane->can_scale);
> +			WARN_ON(!intel_plane->can_scale);
>  
> -		/* FIXME interlacing min height is 6 */
> +			/* FIXME interlacing min height is 6 */
>  
> -		if (crtc_w < 3 || crtc_h < 3)
> -			visible = false;
> +			if (crtc_w < 3 || crtc_h < 3)
> +				state->visible = false;
>  
> -		if (src_w < 3 || src_h < 3)
> -			visible = false;
> +			if (src_w < 3 || src_h < 3)
> +				state->visible = false;
>  
> -		width_bytes = ((src_x * pixel_size) & 63) + src_w * pixel_size;
> +			width_bytes = ((src_x * pixel_size) & 63) +
> +						src_w * pixel_size;
>  
> -		if (src_w > 2048 || src_h > 2048 ||
> -		    width_bytes > 4096 || fb->pitches[0] > 4096) {
> -			DRM_DEBUG_KMS("Source dimensions exceed hardware limits\n");
> -			return -EINVAL;
> +			if (src_w > 2048 || src_h > 2048 ||
> +			    width_bytes > 4096 || fb->pitches[0] > 4096) {
> +				DRM_DEBUG_KMS("Source dimensions exceed hardware limits\n");
> +				return -EINVAL;
> +			}
>  		}
> +
> +		src->x1 = src_x;
> +		src->x2 = src_x + src_w;
> +		src->y1 = src_y;
> +		src->y2 = src_y + src_h;
> +
> +		dst->x1 = crtc_x;
> +		dst->x2 = crtc_x + crtc_w;
> +		dst->y1 = crtc_y;
> +		dst->y2 = crtc_y + crtc_h;
>  	}
>  
> -	dst.x1 = crtc_x;
> -	dst.x2 = crtc_x + crtc_w;
> -	dst.y1 = crtc_y;
> -	dst.y2 = crtc_y + crtc_h;

We need to keep the dst coordinate assignment outside the 'if(visible)'
block to keep the primary_enabled calculation happy in commit().

And now I have nagging feeling I spotted something else when I wrote my
original reply. But now I can't spot anything else. So let's hope that's
just one of those "did I remember to turn off the stove/lock the door?" 
type of feelings.

> +	return 0;
> +}
> +
> +static int
> +intel_commit_sprite_plane(struct drm_plane *plane,
> +			  struct intel_plane_state *state)
> +{
> +	struct drm_device *dev = plane->dev;
> +	struct drm_crtc *crtc = state->crtc;
> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	struct intel_plane *intel_plane = to_intel_plane(plane);
> +	enum pipe pipe = intel_crtc->pipe;
> +	struct drm_framebuffer *fb = state->fb;
> +	struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
> +	struct drm_i915_gem_object *obj = intel_fb->obj;
> +	struct drm_i915_gem_object *old_obj = intel_plane->obj;
> +	int crtc_x, crtc_y;
> +	unsigned int crtc_w, crtc_h;
> +	uint32_t src_x, src_y, src_w, src_h;
> +	struct drm_rect *dst = &state->dst;
> +	const struct drm_rect *clip = &state->clip;
> +	bool primary_enabled;
> +	int ret;
>  
>  	/*
>  	 * If the sprite is completely covering the primary plane,
>  	 * we can disable the primary and save power.
>  	 */
> -	primary_enabled = !drm_rect_equals(&dst, &clip) || colorkey_enabled(intel_plane);
> -	WARN_ON(!primary_enabled && !visible && intel_crtc->active);
> +	primary_enabled = !drm_rect_equals(dst, clip) || colorkey_enabled(intel_plane);
> +	WARN_ON(!primary_enabled && !state->visible && intel_crtc->active);
>  
>  	mutex_lock(&dev->struct_mutex);
>  
> @@ -1055,14 +1052,14 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>  	if (ret)
>  		return ret;
>  
> -	intel_plane->crtc_x = orig.crtc_x;
> -	intel_plane->crtc_y = orig.crtc_y;
> -	intel_plane->crtc_w = orig.crtc_w;
> -	intel_plane->crtc_h = orig.crtc_h;
> -	intel_plane->src_x = orig.src_x;
> -	intel_plane->src_y = orig.src_y;
> -	intel_plane->src_w = orig.src_w;
> -	intel_plane->src_h = orig.src_h;
> +	intel_plane->crtc_x = state->orig_dst.x1;
> +	intel_plane->crtc_y = state->orig_dst.y1;
> +	intel_plane->crtc_w = drm_rect_width(&state->orig_dst);
> +	intel_plane->crtc_h = drm_rect_height(&state->orig_dst);
> +	intel_plane->src_x = state->orig_src.x1;
> +	intel_plane->src_y = state->orig_src.y1;
> +	intel_plane->src_w = drm_rect_width(&state->orig_src);
> +	intel_plane->src_h = drm_rect_height(&state->orig_src);
>  	intel_plane->obj = obj;
>  
>  	if (intel_crtc->active) {
> @@ -1076,12 +1073,22 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>  		if (primary_was_enabled && !primary_enabled)
>  			intel_pre_disable_primary(crtc);
>  
> -		if (visible)
> +		if (state->visible) {
> +			crtc_x = state->dst.x1;
> +			crtc_y = state->dst.x2;
> +			crtc_w = drm_rect_width(&state->dst);
> +			crtc_h = drm_rect_height(&state->dst);
> +			src_x = state->src.x1;
> +			src_y = state->src.y1;
> +			src_w = drm_rect_width(&state->src);
> +			src_h = drm_rect_height(&state->src);
>  			intel_plane->update_plane(plane, crtc, fb, obj,
>  						  crtc_x, crtc_y, crtc_w, crtc_h,
>  						  src_x, src_y, src_w, src_h);
> -		else
> +		} else {
>  			intel_plane->disable_plane(plane, crtc);
> +		}
> +
>  
>  		intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_SPRITE(pipe));
>  
> @@ -1109,6 +1116,46 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>  }
>  
>  static int
> +intel_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)
> +{
> +	struct intel_plane_state state;
> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	int ret;
> +
> +	state.crtc = crtc;
> +	state.fb = fb;
> +
> +	/* sample coordinates in 16.16 fixed point */
> +	state.src.x1 = src_x;
> +	state.src.x2 = src_x + src_w;
> +	state.src.y1 = src_y;
> +	state.src.y2 = src_y + src_h;
> +
> +	/* integer pixels */
> +	state.dst.x1 = crtc_x;
> +	state.dst.x2 = crtc_x + crtc_w;
> +	state.dst.y1 = crtc_y;
> +	state.dst.y2 = crtc_y + crtc_h;
> +
> +	state.clip.x1 = 0;
> +	state.clip.y1 = 0;
> +	state.clip.x2 = intel_crtc->active ? intel_crtc->config.pipe_src_w : 0;
> +	state.clip.y2 = intel_crtc->active ? intel_crtc->config.pipe_src_h : 0;
> +	state.orig_src = state.src;
> +	state.orig_dst = state.dst;
> +
> +	ret = intel_check_sprite_plane(plane, &state);
> +	if (ret)
> +		return ret;
> +
> +	return intel_commit_sprite_plane(plane, &state);
> +}
> +
> +static int
>  intel_disable_plane(struct drm_plane *plane)
>  {
>  	struct drm_device *dev = plane->dev;
> -- 
> 1.9.3

-- 
Ville Syrjälä
Intel OTC