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

Wed Mar 14 10:36:32 UTC 2018

> -----Original Message-----
> From: Maarten Lankhorst [mailto:maarten.lankhorst at linux.intel.com]
> Sent: Wednesday, March 14, 2018 4:03 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 11:31 schreef 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
> 
> For the framebuffer creation also require minimum width then, since it
> needs to be SKL_MIN_SRC_W too..

As such there is no restriction on width for YUV in Bspec. It only mentions
about the height.

Regards
Vidya