[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: Thursday, March 15, 2018 12:34 AM
> To: Ville Syrjälä <ville.syrjala at linux.intel.com>
> Cc: Srinivas, Vidya <vidya.srinivas at 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
> 
> Op 14-03-18 om 18:08 schreef Ville Syrjälä:
> > On Wed, Mar 14, 2018 at 04:55:08PM +0100, Maarten Lankhorst wrote:
> >> Op 14-03-18 om 16:35 schreef Ville Syrjälä:
> >>> 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?
> >>>
> >> Should we also force a minimum width of 16 then?
> > Possibly. I think the scaler should be operating on the rotated data,
> > so from the scaler POV we probably need to consider the rotated
> > coordinates when figuring out the width vs. height limits. But I
> > didn't actually check the code which way we program the scaler input size.
> >
> There's not much of a usecase for a 8x16 fb though, might as well limit it to
> 16x16. :)
> 
> ~Maarten

Yeah :) Thank you. Will make the change and float the patch.

Regards
Vidya