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 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?

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