[PATCH v2 6/6] drm/i915: Do not do fb src adjustments for NV12

[Maarten Lankhorst]

Hmm, more thought about src adjustments...

Op 13-04-18 om 07:22 schreef Vidya Srinivas:
> 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
>
> Credits-to: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> Signed-off-by: Vidya Srinivas <vidya.srinivas at intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 11 +++++++++++
>  drivers/gpu/drm/i915/intel_sprite.c  | 15 +++++++++++++++
>  2 files changed, 26 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index bc83f10..f64708f 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12978,6 +12978,10 @@ intel_check_primary_plane(struct intel_plane *plane,
>  	bool can_position = false;
>  	int ret;
>  	uint32_t pixel_format = 0;
> +	struct drm_plane_state *plane_state = &state->base;
> +	struct drm_rect *src = &plane_state->src;
> +
> +	*src = drm_plane_state_src(plane_state);
>  
>  	if (INTEL_GEN(dev_priv) >= 9) {
>  		/* use scaler when colorkey is not required */
> @@ -13001,6 +13005,13 @@ intel_check_primary_plane(struct intel_plane *plane,
>  	if (!state->base.fb)
>  		return 0;
>  
> +	if (pixel_format == DRM_FORMAT_NV12) {
> +		src->x1 = (((src->x1 >> 16)/4)*4) << 16;
> +		src->x2 = (((src->x2 >> 16)/4)*4) << 16;
> +		src->y1 = (((src->y1 >> 16)/4)*4) << 16;
> +		src->y2 = (((src->y2 >> 16)/4)*4) << 16;
> +	}

Lets reject non multiple of 4 coordinates for plane_state's src_x and src_y,
and before adjustment also reject non multiple of 4 src_w/src_h.fter that we
can also reject clipping to the right/bottom edge of the screen, if the
pipe_src_w/h is not a multiple of 4. That way an application trying to go
beyond the screen.

skl_check_plane_surface could do all the fixups and is allowed to return an
error code, so we could put all SKL specific NV12 handling in there.
It doesn't matter that we calculate potentially illegal values, if we never
program them and return -EINVAL there. If it turns out we've been too paranoid
we could relax the condition.

This would mean unified handling for NV12 for primary and sprite. :)

>  	if (INTEL_GEN(dev_priv) >= 9) {
>  		ret = skl_check_plane_surface(crtc_state, state);
>  		if (ret)
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index 8b7947d..c1dd85e 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -1035,11 +1035,20 @@ intel_check_sprite_plane(struct intel_plane *plane,
>  			return vscale;
>  		}
>  
> +		if (fb->format->format == DRM_FORMAT_NV12) {
> +			if (src->x2 >> 16 == 16)
> +				src->x2 = 16 << 16;
> +			if (src->y2 >> 16 == 16)
> +				src->y2 = 16 << 16;
This part doesn't look required, the magic below for the nv12 format is similar.. Just conditionally call adjust_size for format != NV12. :)
> +			goto nv12_min_no_clip;
> +		}
> +
>  		/* 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));
>  
> +nv12_min_no_clip:
>  		drm_rect_rotate_inv(src, fb->width << 16, fb->height << 16,
>  				    state->base.rotation);
>  
> @@ -1105,6 +1114,12 @@ intel_check_sprite_plane(struct intel_plane *plane,
>  		src->x2 = (src_x + src_w) << 16;
>  		src->y1 = src_y << 16;
>  		src->y2 = (src_y + src_h) << 16;
> +		if (fb->format->format == DRM_FORMAT_NV12) {
> +			src->x1 = (((src->x1 >> 16)/4)*4) << 16;
> +			src->x2 = (((src->x2 >> 16)/4)*4) << 16;
> +			src->y1 = (((src->y1 >> 16)/4)*4) << 16;
> +			src->y2 = (((src->y2 >> 16)/4)*4) << 16;
> +		}