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

[Srinivas, Vidya]

> -----Original Message-----
> From: Ville Syrjälä [mailto:ville.syrjala at linux.intel.com]
> Sent: Wednesday, March 14, 2018 9:06 PM
> To: Srinivas, Vidya <vidya.srinivas at intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>; intel-
> gfx at lists.freedesktop.org; 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
> 
> On Wed, Mar 14, 2018 at 10:36:32AM +0000, Srinivas, Vidya wrote:
> >
> >
> > > -----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.
> 
> Do remember to consider 90/270 degree rotation. That's still supported with
> NV12 is it not?
> 

It is supported. Thank you. As suggested, will check both min width and height for NV12
as > 16

Regards
Vidya

> --
> Ville Syrjälä
> Intel OTC