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

[Srinivas, Vidya]

> -----Original Message-----
> From: Maarten Lankhorst [mailto:maarten.lankhorst at linux.intel.com]
> Sent: Wednesday, March 14, 2018 3:55 PM
> To: Srinivas, Vidya <vidya.srinivas at intel.com>; intel-
> gfx at lists.freedesktop.org
> Cc: Syrjala, Ville <ville.syrjala at intel.com>; Lankhorst, Maarten
> <maarten.lankhorst at intel.com>
> Subject: Re: [Intel-gfx] [PATCH v13 12/17] drm/i915: Upscale scaler max scale
> for NV12
> 
> Op 14-03-18 om 10:52 schreef Maarten Lankhorst:
> > Op 09-03-18 om 09:48 schreef Vidya Srinivas:
> >> From: Chandra Konduru <chandra.konduru at 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 at intel.com>
> >> Reviewed-by: Clinton Taylor <clinton.a.taylor at intel.com>
> >> Signed-off-by: Chandra Konduru <chandra.konduru at intel.com>
> >> Signed-off-by: Nabendu Maiti <nabendu.bikash.maiti at intel.com>
> >> Signed-off-by: Uma Shankar <uma.shankar at intel.com>
> >> Signed-off-by: Vidya Srinivas <vidya.srinivas at 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?
> Hm we should probably reject this in that place anyway, but since src_h >=
> SKL_MIN_YUV_420_SRC_H implies src_h >= SKL_MIN_SRC_H we don't need
> special handling, and can just do if (pixel_format == NV12 && src_h >= 16)
> return -EINVAL; and keep the existing checks.
> 
> ~Maarten

Thank you, I will make this change and float the patch.

Regards
Vidya