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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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@linux.intel.com]
> >>> Sent: Wednesday, March 14, 2018 4:03 PM
> >>> To: Srinivas, Vidya <vidya.srinivas@intel.com>; intel-
> >>> gfx@lists.freedesktop.org
> >>> Cc: Syrjala, Ville <ville.syrjala@intel.com>; Lankhorst, Maarten
> >>> <maarten.lankhorst@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@linux.intel.com]
> >>>>> Sent: Wednesday, March 14, 2018 3:55 PM
> >>>>> To: Srinivas, Vidya <vidya.srinivas@intel.com>; intel-
> >>>>> gfx@lists.freedesktop.org
> >>>>> Cc: Syrjala, Ville <ville.syrjala@intel.com>; Lankhorst, Maarten
> >>>>> <maarten.lankhorst@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@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?
> >>>>> 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.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx