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

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

On Tue, Sep 02, 2014 at 04:23:47PM -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.
> 
> v2: use the new struct intel_plane_state to store information
> between check and commit stages.
> 
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> ---
>  drivers/gpu/drm/i915/intel_sprite.c | 236 ++++++++++++++++++++++--------------
>  1 file changed, 146 insertions(+), 90 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index 4cbe286..062eca2 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -845,57 +845,28 @@ 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 *pstate)

pstate looks a bit funny. Makes me think on cpu pstates. Maybe just call
it 'state' everywhere?

>  {
> -	struct drm_device *dev = plane->dev;
> -	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	struct intel_crtc *intel_crtc = to_intel_crtc(pstate->crtc);
>  	struct intel_plane *intel_plane = to_intel_plane(plane);
> -	enum pipe pipe = intel_crtc->pipe;
> +	struct drm_framebuffer *fb = pstate->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 = pstate->crtc_x;
> +	int crtc_y = pstate->crtc_y;
> +	int crtc_w = pstate->crtc_w;
> +	int crtc_h = pstate->crtc_h;
> +	int src_x = pstate->src_x;
> +	int src_y = pstate->src_y;
> +	int src_w = pstate->src_w;
> +	int src_h = pstate->src_h;

AFAICS we don't need to initialize this stuff here. We already populate
it all before we need it further down. So I think the only thing we need
is add a bit of code to write the src_? coordinates back into
pstate->src (at the end of check() like you already do for the dst
coordinates). To avoid warning about unitialized variables that
probably needs to be wrapped in an 'if (visible)' check.

> +	struct drm_rect *src = &pstate->src;
> +	struct drm_rect *dst = &pstate->dst;
> +	struct drm_rect *clip = &pstate->clip;

Please keep 'clip' const here to prevent silly accidents.

>  	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 +898,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);
> +	pstate->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 (pstate->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) src_x ||
> +			src->y1 < (int) src_y ||
> +			src->x2 > (int) (src_x + src_w) ||
> +			src->y2 > (int) (src_y + src_h));
>  
>  		/*
>  		 * Hardware doesn't handle subpixel coordinates.
> @@ -983,10 +954,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,12 +971,12 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>  				crtc_w &= ~1;
>  
>  			if (crtc_w == 0)
> -				visible = false;
> +				pstate->visible = false;
>  		}
>  	}
>  
>  	/* Check size restrictions when scaling */
> -	if (visible && (src_w != crtc_w || src_h != crtc_h)) {
> +	if (pstate->visible && (src_w != crtc_w || src_h != crtc_h)) {
>  		unsigned int width_bytes;
>  
>  		WARN_ON(!intel_plane->can_scale);
> @@ -1013,10 +984,10 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>  		/* FIXME interlacing min height is 6 */
>  
>  		if (crtc_w < 3 || crtc_h < 3)
> -			visible = false;
> +			pstate->visible = false;
>  
>  		if (src_w < 3 || src_h < 3)
> -			visible = false;
> +			pstate->visible = false;
>  
>  		width_bytes = ((src_x * pixel_size) & 63) + src_w * pixel_size;
>  
> @@ -1027,17 +998,55 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>  		}
>  	}
>  
> -	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;
> +
> +	pstate->src_x = src_x;
> +	pstate->src_y = src_y;
> +	pstate->src_w = src_w;
> +	pstate->src_h = src_h;
> +	pstate->crtc_x = crtc_x;
> +	pstate->crtc_y = crtc_y;
> +	pstate->crtc_w = crtc_w;
> +	pstate->crtc_x = crtc_h;
> +
> +	return 0;
> +}
> +
> +static int
> +intel_commit_sprite_plane(struct drm_plane *plane,
> +			  struct intel_plane_state *pstate)
> +{
> +	struct drm_device *dev = plane->dev;
> +	struct drm_crtc *crtc = pstate->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 = pstate->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 = pstate->crtc_x;
> +	int crtc_y = pstate->crtc_y;
> +	int crtc_w = pstate->crtc_w;
> +	int crtc_h = pstate->crtc_h;
> +	int src_x = pstate->src_x;
> +	int src_y = pstate->src_y;
> +	int src_w = pstate->src_w;
> +	int src_h = pstate->src_h;

As I mentioned on the previous patch I don't think we need the
coordinates stored twice in the plane config.

Just do:
 crtc_x = pstate->dst.x1;
 ...
 crtc_w = drm_rect_width(&pstate->dst);
 ...

etc.

> +	struct drm_rect *dst = &pstate->dst;
> +	struct drm_rect *clip = &pstate->clip;
> +	int ret;
> +	bool primary_enabled;
>  
>  	/*
>  	 * 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 && !pstate->visible && intel_crtc->active);
>  
>  	mutex_lock(&dev->struct_mutex);
>  
> @@ -1055,14 +1064,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 = pstate->orig_dst.x1;
> +	intel_plane->crtc_y = pstate->orig_dst.y1;
> +	intel_plane->crtc_w = drm_rect_width(&pstate->orig_dst);
> +	intel_plane->crtc_h = drm_rect_height(&pstate->orig_dst);
> +	intel_plane->src_x = pstate->orig_src.x1;
> +	intel_plane->src_y = pstate->orig_src.y1;
> +	intel_plane->src_w = drm_rect_width(&pstate->orig_src);
> +	intel_plane->src_h = drm_rect_height(&pstate->orig_src);
>  	intel_plane->obj = obj;
>  
>  	if (intel_crtc->active) {
> @@ -1076,7 +1085,7 @@ 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 (pstate->visible)
>  			intel_plane->update_plane(plane, crtc, fb, obj,
>  						  crtc_x, crtc_y, crtc_w, crtc_h,
>  						  src_x, src_y, src_w, src_h);
> @@ -1109,6 +1118,53 @@ 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 pstate;
> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	int ret;
> +
> +	pstate.crtc = crtc;
> +	pstate.fb = fb;
> +	pstate.crtc_x = crtc_x;
> +	pstate.crtc_y = crtc_y;
> +	pstate.crtc_w = crtc_w;
> +	pstate.crtc_h = crtc_h;
> +	pstate.src_x = src_x;
> +	pstate.src_y = src_y;
> +	pstate.src_w = src_w;
> +	pstate.src_h = src_h;
> +	pstate.src.x1 = src_x,
> +	pstate.src.x2 = src_x + src_w,
> +	pstate.src.y1 = src_y,
> +	pstate.src.y2 = src_y + src_h,
> +	pstate.dst.x1 = crtc_x,
> +	pstate.dst.x2 = crtc_x + crtc_w,
> +	pstate.dst.y1 = crtc_y,
> +	pstate.dst.y2 = crtc_y + crtc_h,
> +	pstate.clip.x2 = intel_crtc->active ? intel_crtc->config.pipe_src_w : 0,
> +	pstate.clip.y2 = intel_crtc->active ? intel_crtc->config.pipe_src_h : 0,

You need to initialize clip x1 and y1 too since pstate isn't zero
initialized.

> +	pstate.orig_src.x1 = src_x,
> +	pstate.orig_src.x2 = src_x + src_w,
> +	pstate.orig_src.y1 = src_y,
> +	pstate.orig_src.y2 = src_y + src_h,
> +	pstate.orig_dst.x1 = crtc_x,
> +	pstate.orig_dst.x2 = crtc_x + crtc_w,
> +	pstate.orig_dst.y1 = crtc_y,
> +	pstate.orig_dst.y2 = crtc_y + crtc_h,

s/,/;/ in a lots of places here.

But I think we can simplify this a bit like so:

pstate.orig.src... = ..;
pstate.orig.dst... = ..;

pstate.src = pstate.orig.src;
pstate.dst = pstate.orig.dst;

> +
> +	ret = intel_check_sprite_plane(plane, &pstate);
> +	if (ret)
> +		return ret;
> +
> +	return intel_commit_sprite_plane(plane, &pstate);
> +}
> +
> +static int
>  intel_disable_plane(struct drm_plane *plane)
>  {
>  	struct drm_device *dev = plane->dev;
> -- 
> 1.9.3
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel OTC