Re: [PATCH v2] drm/i915: pin sprite fb only if it changed

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

On Wed, Sep 10, 2014 at 12:03:17PM -0300, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> 
> Optimize code avoiding helding dev mutex if old fb and current fb
> are the same.
> 
> v2: take Ville's comments
> 	- move comment along with the pin_and_fence call
> 	- check for error before calling i915_gem_track_fb
> 	- move old_obj != obj to an upper if condition
> 
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

lgtm

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/intel_sprite.c | 34 +++++++++++++++++++---------------
>  1 file changed, 19 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index a4306cf..90bb45f 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -1038,21 +1038,24 @@ intel_commit_sprite_plane(struct drm_plane *plane,
>  	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);
> -
> -	/* Note that this will apply the VT-d workaround for scanouts,
> -	 * which is more restrictive than required for sprites. (The
> -	 * primary plane requires 256KiB alignment with 64 PTE padding,
> -	 * the sprite planes only require 128KiB alignment and 32 PTE padding.
> -	 */
> -	ret = intel_pin_and_fence_fb_obj(dev, obj, NULL);
>  
> -	i915_gem_track_fb(old_obj, obj,
> -			  INTEL_FRONTBUFFER_SPRITE(pipe));
> -	mutex_unlock(&dev->struct_mutex);
> +	if (old_obj != obj) {
> +		mutex_lock(&dev->struct_mutex);
>  
> -	if (ret)
> -		return ret;
> +		/* Note that this will apply the VT-d workaround for scanouts,
> +		 * which is more restrictive than required for sprites. (The
> +		 * primary plane requires 256KiB alignment with 64 PTE padding,
> +		 * the sprite planes only require 128KiB alignment and 32 PTE
> +		 * padding.
> +		 */
> +		ret = intel_pin_and_fence_fb_obj(dev, obj, NULL);
> +		if (ret == 0)
> +			i915_gem_track_fb(old_obj, obj,
> +					  INTEL_FRONTBUFFER_SPRITE(pipe));
> +		mutex_unlock(&dev->struct_mutex);
> +		if (ret)
> +			return ret;
> +	}
>  
>  	intel_plane->crtc_x = state->orig_dst.x1;
>  	intel_plane->crtc_y = state->orig_dst.y1;
> @@ -1099,14 +1102,15 @@ intel_commit_sprite_plane(struct drm_plane *plane,
>  	}
>  
>  	/* Unpin old obj after new one is active to avoid ugliness */
> -	if (old_obj) {
> +	if (old_obj && old_obj != obj) {
> +
>  		/*
>  		 * It's fairly common to simply update the position of
>  		 * an existing object.  In that case, we don't need to
>  		 * wait for vblank to avoid ugliness, we only need to
>  		 * do the pin & ref bookkeeping.
>  		 */
> -		if (old_obj != obj && intel_crtc->active)
> +		if (intel_crtc->active)
>  			intel_wait_for_vblank(dev, intel_crtc->pipe);
>  
>  		mutex_lock(&dev->struct_mutex);
> -- 
> 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