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

[Maarten Lankhorst <maarten.lankhorst@linux.intel.com>]

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@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.
>
There's not much of a usecase for a 8x16 fb though, might as well limit it to 16x16. :)

~Maarten

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx