Re: [PATCH v13 12/17] drm/i915: Upscale scaler max scale for NV12

[Maarten Lankhorst <maarten.lankhorst@linux.intel.com>]

Op 09-03-18 om 09:48 schreef Vidya Srinivas:
> From: Chandra Konduru <chandra.konduru@intel.com>
>
> This patch updates scaler max limit support for NV12
>
> v2: Rebased (me)
>
> v3: Rebased (me)
>
> v4: Missed the Tested-by/Reviewed-by in the previous series
> Adding the same to commit message in this version.
>
> v5: Addressed review comments from Ville and rebased
> - calculation of max_scale to be made
> less convoluted by splitting it up a bit
> - Indentation errors to be fixed in the series
>
> v6: Rebased (me)
> Fixed review comments from Paauwe, Bob J
> Previous version, where a split of calculation
> was done, was wrong. Fixed that issue here.
>
> v7: Rebased (me)
>
> v8: Rebased (me)
>
> v9: Rebased (me)
>
> v10: Rebased (me)
>
> v11: Addressed review comments from Shashank Sharma
> Alignment issues fixed.
> When call to skl_update_scaler is made, 0 was being
> sent instead of pixel_format.
> When crtc update scaler is called, we dont have the
> fb to derive the pixel format. Added the function
> parameter bool plane_scaler_check to account for this.
>
> v12: Fixed failure in IGT debugfs_test.
> fb is NULL in skl_update_scaler_plane
> Due to this, accessing fb->format caused failure.
> Patch checks fb before using.
>
> v13: In the previous version there was a flaw.
> In skl_update_scaler during plane_scaler_check
> if the format was non-NV12, it would set need_scaling
> to false. This could reset the previously set need_scaling
> from a previous condition check. Patch fixes this.
> Patch also adds minimum src height for YUV 420 formats
> to 16 (as defined in BSpec) and adds for checking this
> range.
>
> Tested-by: Clinton Taylor <clinton.a.taylor@intel.com>
> Reviewed-by: Clinton Taylor <clinton.a.taylor@intel.com>
> Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
> Signed-off-by: Nabendu Maiti <nabendu.bikash.maiti@intel.com>
> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
> Signed-off-by: Vidya Srinivas <vidya.srinivas@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 78 ++++++++++++++++++++++++++----------
>  drivers/gpu/drm/i915/intel_drv.h     |  4 +-
>  drivers/gpu/drm/i915/intel_sprite.c  |  3 +-
>  3 files changed, 61 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 34f7225..7fd8354 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3466,6 +3466,8 @@ static u32 skl_plane_ctl_format(uint32_t pixel_format)
>  		return PLANE_CTL_FORMAT_YUV422 | PLANE_CTL_YUV422_UYVY;
>  	case DRM_FORMAT_VYUY:
>  		return PLANE_CTL_FORMAT_YUV422 | PLANE_CTL_YUV422_VYUY;
> +	case DRM_FORMAT_NV12:
> +		return PLANE_CTL_FORMAT_NV12;
>  	default:
>  		MISSING_CASE(pixel_format);
>  	}
> @@ -4705,7 +4707,9 @@ static void cpt_verify_modeset(struct drm_device *dev, int pipe)
>  static int
>  skl_update_scaler(struct intel_crtc_state *crtc_state, bool force_detach,
>  		  unsigned int scaler_user, int *scaler_id,
> -		  int src_w, int src_h, int dst_w, int dst_h)
> +		  int src_w, int src_h, int dst_w, int dst_h,
> +		  bool plane_scaler_check,
> +		  uint32_t pixel_format)
>  {
>  	struct intel_crtc_scaler_state *scaler_state =
>  		&crtc_state->scaler_state;
> @@ -4723,6 +4727,10 @@ skl_update_scaler(struct intel_crtc_state *crtc_state, bool force_detach,
>  	 */
>  	need_scaling = src_w != dst_w || src_h != dst_h;
>  
> +	if (plane_scaler_check)
> +		if (pixel_format == DRM_FORMAT_NV12)
> +			need_scaling = true;
Seems redundant to add plane_scaler_check, if you can just check for scaler_user != SKL_CRTC_INDEX.
But since pixel_format is always 0 for crtc index, you can just check pixel_format == DRM_FORMAT_NV12 directly..

>  	if (crtc_state->ycbcr420 && scaler_user == SKL_CRTC_INDEX)
>  		need_scaling = true;
>  
> @@ -4763,17 +4771,32 @@ skl_update_scaler(struct intel_crtc_state *crtc_state, bool force_detach,
>  	}
>  
>  	/* range checks */
> -	if (src_w < SKL_MIN_SRC_W || src_h < SKL_MIN_SRC_H ||
> -		dst_w < SKL_MIN_DST_W || dst_h < SKL_MIN_DST_H ||
> -
> -		src_w > SKL_MAX_SRC_W || src_h > SKL_MAX_SRC_H ||
> -		dst_w > SKL_MAX_DST_W || dst_h > SKL_MAX_DST_H) {
> -		DRM_DEBUG_KMS("scaler_user index %u.%u: src %ux%u dst %ux%u "
> -			"size is out of scaler range\n",
> -			intel_crtc->pipe, scaler_user, src_w, src_h, dst_w, dst_h);
> -		return -EINVAL;
> -	}
> -
> +	if (plane_scaler_check && pixel_format == DRM_FORMAT_NV12) {
> +		if (src_h > SKL_MIN_YUV_420_SRC_H)
> +			goto check_scaler_range;
> +		else
> +			goto failed_range;
> +	} else {
> +		if (src_h >= SKL_MIN_SRC_H)
> +			goto check_scaler_range;
> +		else
> +			goto failed_range;
> +	}
Since nv12 always needs scaling, could we refuse to create NV12 fb's with height < 16 in intel_framebuffer_init?

This way we don't need to check in skl_update_scaler, and always do the right thing..

~Maarten
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx