Re: [PATCH v4 6/6] drm/i915: Add skl_check_nv12_surface for NV12

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

On Wed, Apr 18, 2018 at 09:38:13AM +0530, Vidya Srinivas wrote:
> From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> 
> We skip src trunction/adjustments for
> NV12 case and handle the sizes directly.
> Without this, pipe fifo underruns are seen on APL/KBL.
> 
> v2: For NV12, making the src coordinates multiplier of 4
> 
> v3: Moving all the src coords handling code for NV12
> to skl_check_nv12_surface
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Signed-off-by: Vidya Srinivas <vidya.srinivas@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 39 ++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_sprite.c  | 15 ++++++++++----
>  2 files changed, 50 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 925402e..b8dbaca 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3118,6 +3118,42 @@ static int skl_check_main_surface(const struct intel_crtc_state *crtc_state,
>  	return 0;
>  }
>  
> +static int
> +skl_check_nv12_surface(const struct intel_crtc_state *crtc_state,
> +		       struct intel_plane_state *plane_state)
> +{
> +	int crtc_x2 = plane_state->base.crtc_x + plane_state->base.crtc_w;
> +	int crtc_y2 = plane_state->base.crtc_y + plane_state->base.crtc_h;
> +
> +	if (((plane_state->base.src_x >> 16) % 4) != 0 ||
> +	    ((plane_state->base.src_y >> 16) % 4) != 0 ||
> +	    ((plane_state->base.src_w >> 16) % 4) != 0 ||
> +	    ((plane_state->base.src_h >> 16) % 4) != 0) {
> +		DRM_DEBUG_KMS("src coords must be multiple of 4 for NV12\n");
> +		return -EINVAL;
> +	}

I don't really see why we should check these. The clipped coordinates
are what matters.

> +
> +	/* Clipping would cause a 1-3 pixel gap at the edge of the screen? */
> +	if ((crtc_x2 > crtc_state->pipe_src_w && crtc_state->pipe_src_w % 4) ||
> +	    (crtc_y2 > crtc_state->pipe_src_h && crtc_state->pipe_src_h % 4)) {
> +		DRM_DEBUG_KMS("It's not possible to clip %u,%u to %u,%u\n",
> +			      crtc_x2, crtc_y2,
> +			      crtc_state->pipe_src_w, crtc_state->pipe_src_h);
> +		return -EINVAL;
> +	}

Why should we care? The current code already plays it fast and loose
and allows the dst rectangle to shrink to accomodate the hw limits.
If we want to change that we should change it universally.

> +
> +	plane_state->base.src.x1 =
> +		DIV_ROUND_CLOSEST(plane_state->base.src.x1, 1 << 18) << 18;
> +	plane_state->base.src.x2 =
> +		DIV_ROUND_CLOSEST(plane_state->base.src.x2, 1 << 18) << 18;
> +	plane_state->base.src.y1 =
> +		DIV_ROUND_CLOSEST(plane_state->base.src.y1, 1 << 18) << 18;
> +	plane_state->base.src.y2 =
> +		DIV_ROUND_CLOSEST(plane_state->base.src.y2, 1 << 18) << 18;

Since this can now increase the size of the source rectangle our
scaling factor checks are no longer 100% valid. We might end up with
a scaling factor that is too high.

I don't really like any of these "let's make NV12 behave special"
tricks. We should make the code behave the same way for all pixel
formats instead of adding format specific hacks.

> +
> +	return 0;
> +}
> +
>  static int skl_check_nv12_aux_surface(struct intel_plane_state *plane_state)
>  {
>  	const struct drm_framebuffer *fb = plane_state->base.fb;
> @@ -3201,6 +3237,9 @@ int skl_check_plane_surface(const struct intel_crtc_state *crtc_state,
>  	 * the main surface setup depends on it.
>  	 */
>  	if (fb->format->format == DRM_FORMAT_NV12) {
> +		ret = skl_check_nv12_surface(crtc_state, plane_state);
> +		if (ret)
> +			return ret;
>  		ret = skl_check_nv12_aux_surface(plane_state);
>  		if (ret)
>  			return ret;
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index 8b7947d..f9985fb 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -1035,10 +1035,17 @@ intel_check_sprite_plane(struct intel_plane *plane,
>  			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));
> +		if (fb->format->format != DRM_FORMAT_NV12) {
> +			/*
> +			 * 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_rotate_inv(src, fb->width << 16, fb->height << 16,
>  				    state->base.rotation);
> -- 
> 2.7.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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