Re: [PATCH] drm/i915/skl: don't fail colorkey + scaler request

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

On Mon, May 18, 2015 at 04:18:44PM -0700, Chandra Konduru wrote:
> There is a mplayer video failure reported with xv.
> This is because there is a request to do both plane scaling
> and colorkey. Because skl hw doesn't support plane scaling
> and colorkey at the same time, request is failed which is expected
> behavior.
> 
> To make xv operate, this patch allows colorkey continue to work
> without using scaler. Then behavior would be similar to platforms
> without plane scaler support.
> 
> Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=90449
> ---
>  drivers/gpu/drm/i915/intel_display.c |   14 +++++++++-----
>  drivers/gpu/drm/i915/intel_sprite.c  |   31 +++++++++++++++++++++----------
>  2 files changed, 30 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 7ab75c0..fc5cef0 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4505,9 +4505,10 @@ skl_update_scaler_users(
>  	}
>  
>  	/* check colorkey */
> -	if (intel_plane && intel_plane->ckey.flags != I915_SET_COLORKEY_NONE) {
> -		DRM_DEBUG_KMS("PLANE:%d scaling with color key not allowed",
> -			intel_plane->base.base.id);
> +	if (WARN_ON(intel_plane &&
> +		intel_plane->ckey.flags != I915_SET_COLORKEY_NONE)) {
> +		DRM_DEBUG_KMS("PLANE:%d scaling %ux%u->%ux%u not allowed with colorkey",
> +			intel_plane->base.base.id, src_w, src_h, dst_w, dst_h);
>  		return -EINVAL;
>  	}
>  
> @@ -13049,8 +13050,11 @@ intel_check_primary_plane(struct drm_plane *plane,
>  		intel_atomic_get_crtc_state(state->base.state, intel_crtc) : NULL;
>  
>  	if (INTEL_INFO(dev)->gen >= 9) {
> -		min_scale = 1;
> -		max_scale = skl_max_scale(intel_crtc, crtc_state);
> +		/* use scaler when colorkey is not required */
> +		if (to_intel_plane(plane)->ckey.flags == I915_SET_COLORKEY_NONE) {
> +			min_scale = 1;
> +			max_scale = skl_max_scale(intel_crtc, crtc_state);
> +		}
>  		can_position = true;
>  	}
>  
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index 16d0e48..9004e47 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -775,7 +775,7 @@ intel_check_sprite_plane(struct drm_plane *plane,
>  	struct drm_rect *dst = &state->dst;
>  	const struct drm_rect *clip = &state->clip;
>  	int hscale, vscale;
> -	int max_scale, min_scale;
> +	int max_scale, min_scale, can_scale;

can_scale ought to be bool. Otherwise this looks good.
So with that bikeshed done this gets:
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Ps. Please use my @linux.intel.com address when cc:ing me on patches.
I try to shovel all patch traffic through that and use the @intel.com
address for other stuff.

>  	int pixel_size;
>  	int ret;
>  
> @@ -800,18 +800,29 @@ intel_check_sprite_plane(struct drm_plane *plane,
>  		return -EINVAL;
>  	}
>  
> +	/* setup can_scale, min_scale, max_scale */
> +	if (INTEL_INFO(dev)->gen >= 9) {
> +		/* use scaler when colorkey is not required */
> +		if (intel_plane->ckey.flags == I915_SET_COLORKEY_NONE) {
> +			can_scale = 1;
> +			min_scale = 1;
> +			max_scale = skl_max_scale(intel_crtc, crtc_state);
> +		} else {
> +			can_scale = 0;
> +			min_scale = DRM_PLANE_HELPER_NO_SCALING;
> +			max_scale = DRM_PLANE_HELPER_NO_SCALING;
> +		}
> +	} else {
> +		can_scale = intel_plane->can_scale;
> +		max_scale = intel_plane->max_downscale << 16;
> +		min_scale = intel_plane->can_scale ? 1 : (1 << 16);
> +	}
> +
>  	/*
>  	 * FIXME the following code does a bunch of fuzzy adjustments to the
>  	 * coordinates and sizes. We probably need some way to decide whether
>  	 * more strict checking should be done instead.
>  	 */
> -	max_scale = intel_plane->max_downscale << 16;
> -	min_scale = intel_plane->can_scale ? 1 : (1 << 16);
> -
> -	if (INTEL_INFO(dev)->gen >= 9) {
> -		min_scale = 1;
> -		max_scale = skl_max_scale(intel_crtc, crtc_state);
> -	}
>  
>  	drm_rect_rotate(src, fb->width << 16, fb->height << 16,
>  			state->base.rotation);
> @@ -882,7 +893,7 @@ intel_check_sprite_plane(struct drm_plane *plane,
>  			 * Must keep src and dst the
>  			 * same if we can't scale.
>  			 */
> -			if (!intel_plane->can_scale)
> +			if (!can_scale)
>  				crtc_w &= ~1;
>  
>  			if (crtc_w == 0)
> @@ -894,7 +905,7 @@ intel_check_sprite_plane(struct drm_plane *plane,
>  	if (state->visible && (src_w != crtc_w || src_h != crtc_h)) {
>  		unsigned int width_bytes;
>  
> -		WARN_ON(!intel_plane->can_scale);
> +		WARN_ON(!can_scale);
>  
>  		/* FIXME interlacing min height is 6 */
>  
> -- 
> 1.7.9.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx