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

Commit Message

Vidya Srinivas March 9, 2018, 8:48 a.m. UTC

From: Chandra Konduru <[e-mail]>

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 <[e-mail]>
Reviewed-by: Clinton Taylor <[e-mail]>
Signed-off-by: Chandra Konduru <[e-mail]>
Signed-off-by: Nabendu Maiti <[e-mail]>
Signed-off-by: Uma Shankar <[e-mail]>
Signed-off-by: Vidya Srinivas <[e-mail]>
---
 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(-)

Comments

Maarten Lankhorst March 14, 2018, 9:52 a.m. UTC | #1

Op 09-03-18 om 09:48 schreef Vidya Srinivas:
> From: Chandra Konduru <[e-mail]>
>
> 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 <[e-mail]>
> Reviewed-by: Clinton Taylor <[e-mail]>
> Signed-off-by: Chandra Konduru <[e-mail]>
> Signed-off-by: Nabendu Maiti <[e-mail]>
> Signed-off-by: Uma Shankar <[e-mail]>
> Signed-off-by: Vidya Srinivas <[e-mail]>
> ---
>  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?

This way we don't need to check in skl_update_scaler, and always do the right thing..

~Maarten

Maarten Lankhorst March 14, 2018, 10:25 a.m. UTC | #2

Op 14-03-18 om 10:52 schreef Maarten Lankhorst:
> Op 09-03-18 om 09:48 schreef Vidya Srinivas:
>> From: Chandra Konduru <[e-mail]>
>>
>> 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 <[e-mail]>
>> Reviewed-by: Clinton Taylor <[e-mail]>
>> Signed-off-by: Chandra Konduru <[e-mail]>
>> Signed-off-by: Nabendu Maiti <[e-mail]>
>> Signed-off-by: Uma Shankar <[e-mail]>
>> Signed-off-by: Vidya Srinivas <[e-mail]>
>> ---
>>  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

Vidya Srinivas March 14, 2018, 10:31 a.m. UTC | #3

> -----Original Message-----

> From: Maarten Lankhorst [mailto:[e-mail]]

> Sent: Wednesday, March 14, 2018 3:55 PM

> To: Srinivas, Vidya <[e-mail]>; intel-

> [e-mail]

> Cc: Syrjala, Ville <[e-mail]>; Lankhorst, Maarten

> <[e-mail]>

> 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 <[e-mail]>

> >>

> >> 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 <[e-mail]>

> >> Reviewed-by: Clinton Taylor <[e-mail]>

> >> Signed-off-by: Chandra Konduru <[e-mail]>

> >> Signed-off-by: Nabendu Maiti <[e-mail]>

> >> Signed-off-by: Uma Shankar <[e-mail]>

> >> Signed-off-by: Vidya Srinivas <[e-mail]>

> >> ---

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

Maarten Lankhorst March 14, 2018, 10:32 a.m. UTC | #4

Op 14-03-18 om 11:31 schreef Srinivas, Vidya:
>
>> -----Original Message-----
>> From: Maarten Lankhorst [mailto:[e-mail]]
>> Sent: Wednesday, March 14, 2018 3:55 PM
>> To: Srinivas, Vidya <[e-mail]>; intel-
>> [e-mail]
>> Cc: Syrjala, Ville <[e-mail]>; Lankhorst, Maarten
>> <[e-mail]>
>> 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 <[e-mail]>
>>>>
>>>> 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 <[e-mail]>
>>>> Reviewed-by: Clinton Taylor <[e-mail]>
>>>> Signed-off-by: Chandra Konduru <[e-mail]>
>>>> Signed-off-by: Nabendu Maiti <[e-mail]>
>>>> Signed-off-by: Uma Shankar <[e-mail]>
>>>> Signed-off-by: Vidya Srinivas <[e-mail]>
>>>> ---
>>>>  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..

Vidya Srinivas March 14, 2018, 10:36 a.m. UTC | #5

> -----Original Message-----

> From: Maarten Lankhorst [mailto:[e-mail]]

> Sent: Wednesday, March 14, 2018 4:03 PM

> To: Srinivas, Vidya <[e-mail]>; intel-

> [e-mail]

> Cc: Syrjala, Ville <[e-mail]>; Lankhorst, Maarten

> <[e-mail]>

> 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:[e-mail]]

> >> Sent: Wednesday, March 14, 2018 3:55 PM

> >> To: Srinivas, Vidya <[e-mail]>; intel-

> >> [e-mail]

> >> Cc: Syrjala, Ville <[e-mail]>; Lankhorst, Maarten

> >> <[e-mail]>

> >> 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 <[e-mail]>

> >>>>

> >>>> 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 <[e-mail]>

> >>>> Reviewed-by: Clinton Taylor <[e-mail]>

> >>>> Signed-off-by: Chandra Konduru <[e-mail]>

> >>>> Signed-off-by: Nabendu Maiti <[e-mail]>

> >>>> Signed-off-by: Uma Shankar <[e-mail]>

> >>>> Signed-off-by: Vidya Srinivas <[e-mail]>

> >>>> ---

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

Regards
Vidya

Ville Syrjälä March 14, 2018, 3:35 p.m. UTC | #6

On Wed, Mar 14, 2018 at 10:36:32AM +0000, Srinivas, Vidya wrote:
> 
> 
> > -----Original Message-----
> > From: Maarten Lankhorst [mailto:[e-mail]]
> > Sent: Wednesday, March 14, 2018 4:03 PM
> > To: Srinivas, Vidya <[e-mail]>; intel-
> > [e-mail]
> > Cc: Syrjala, Ville <[e-mail]>; Lankhorst, Maarten
> > <[e-mail]>
> > 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:[e-mail]]
> > >> Sent: Wednesday, March 14, 2018 3:55 PM
> > >> To: Srinivas, Vidya <[e-mail]>; intel-
> > >> [e-mail]
> > >> Cc: Syrjala, Ville <[e-mail]>; Lankhorst, Maarten
> > >> <[e-mail]>
> > >> 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 <[e-mail]>
> > >>>>
> > >>>> 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 <[e-mail]>
> > >>>> Reviewed-by: Clinton Taylor <[e-mail]>
> > >>>> Signed-off-by: Chandra Konduru <[e-mail]>
> > >>>> Signed-off-by: Nabendu Maiti <[e-mail]>
> > >>>> Signed-off-by: Uma Shankar <[e-mail]>
> > >>>> Signed-off-by: Vidya Srinivas <[e-mail]>
> > >>>> ---
> > >>>>  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?

Maarten Lankhorst March 14, 2018, 3:55 p.m. UTC | #7

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:[e-mail]]
>>> Sent: Wednesday, March 14, 2018 4:03 PM
>>> To: Srinivas, Vidya <[e-mail]>; intel-
>>> [e-mail]
>>> Cc: Syrjala, Ville <[e-mail]>; Lankhorst, Maarten
>>> <[e-mail]>
>>> 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:[e-mail]]
>>>>> Sent: Wednesday, March 14, 2018 3:55 PM
>>>>> To: Srinivas, Vidya <[e-mail]>; intel-
>>>>> [e-mail]
>>>>> Cc: Syrjala, Ville <[e-mail]>; Lankhorst, Maarten
>>>>> <[e-mail]>
>>>>> 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 <[e-mail]>
>>>>>>>
>>>>>>> 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 <[e-mail]>
>>>>>>> Reviewed-by: Clinton Taylor <[e-mail]>
>>>>>>> Signed-off-by: Chandra Konduru <[e-mail]>
>>>>>>> Signed-off-by: Nabendu Maiti <[e-mail]>
>>>>>>> Signed-off-by: Uma Shankar <[e-mail]>
>>>>>>> Signed-off-by: Vidya Srinivas <[e-mail]>
>>>>>>> ---
>>>>>>>  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?

Minimum width of 8 was based on SKL_MIN_SRC_W, since scaler is always enabled for NV12 that ends up being a restriction for the format.

Ville Syrjälä March 14, 2018, 5:08 p.m. UTC | #8

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:[e-mail]]
> >>> Sent: Wednesday, March 14, 2018 4:03 PM
> >>> To: Srinivas, Vidya <[e-mail]>; intel-
> >>> [e-mail]
> >>> Cc: Syrjala, Ville <[e-mail]>; Lankhorst, Maarten
> >>> <[e-mail]>
> >>> 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:[e-mail]]
> >>>>> Sent: Wednesday, March 14, 2018 3:55 PM
> >>>>> To: Srinivas, Vidya <[e-mail]>; intel-
> >>>>> [e-mail]
> >>>>> Cc: Syrjala, Ville <[e-mail]>; Lankhorst, Maarten
> >>>>> <[e-mail]>
> >>>>> 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 <[e-mail]>
> >>>>>>>
> >>>>>>> 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 <[e-mail]>
> >>>>>>> Reviewed-by: Clinton Taylor <[e-mail]>
> >>>>>>> Signed-off-by: Chandra Konduru <[e-mail]>
> >>>>>>> Signed-off-by: Nabendu Maiti <[e-mail]>
> >>>>>>> Signed-off-by: Uma Shankar <[e-mail]>
> >>>>>>> Signed-off-by: Vidya Srinivas <[e-mail]>
> >>>>>>> ---
> >>>>>>>  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.

Maarten Lankhorst March 14, 2018, 7:03 p.m. UTC | #9

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:[e-mail]]
>>>>> Sent: Wednesday, March 14, 2018 4:03 PM
>>>>> To: Srinivas, Vidya <[e-mail]>; intel-
>>>>> [e-mail]
>>>>> Cc: Syrjala, Ville <[e-mail]>; Lankhorst, Maarten
>>>>> <[e-mail]>
>>>>> 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:[e-mail]]
>>>>>>> Sent: Wednesday, March 14, 2018 3:55 PM
>>>>>>> To: Srinivas, Vidya <[e-mail]>; intel-
>>>>>>> [e-mail]
>>>>>>> Cc: Syrjala, Ville <[e-mail]>; Lankhorst, Maarten
>>>>>>> <[e-mail]>
>>>>>>> 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 <[e-mail]>
>>>>>>>>>
>>>>>>>>> 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 <[e-mail]>
>>>>>>>>> Reviewed-by: Clinton Taylor <[e-mail]>
>>>>>>>>> Signed-off-by: Chandra Konduru <[e-mail]>
>>>>>>>>> Signed-off-by: Nabendu Maiti <[e-mail]>
>>>>>>>>> Signed-off-by: Uma Shankar <[e-mail]>
>>>>>>>>> Signed-off-by: Vidya Srinivas <[e-mail]>
>>>>>>>>> ---
>>>>>>>>>  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

Vidya Srinivas March 15, 2018, 2:30 a.m. UTC | #10

> -----Original Message-----

> From: Maarten Lankhorst [mailto:[e-mail]]

> Sent: Thursday, March 15, 2018 12:34 AM

> To: Ville Syrjälä <[e-mail]>

> Cc: Srinivas, Vidya <[e-mail]>; intel-

> [e-mail]; Syrjala, Ville <[e-mail]>; Lankhorst,

> Maarten <[e-mail]>

> 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:[e-mail]]

> >>>>> Sent: Wednesday, March 14, 2018 4:03 PM

> >>>>> To: Srinivas, Vidya <[e-mail]>; intel-

> >>>>> [e-mail]

> >>>>> Cc: Syrjala, Ville <[e-mail]>; Lankhorst, Maarten

> >>>>> <[e-mail]>

> >>>>> 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:[e-mail]]

> >>>>>>> Sent: Wednesday, March 14, 2018 3:55 PM

> >>>>>>> To: Srinivas, Vidya <[e-mail]>; intel-

> >>>>>>> [e-mail]

> >>>>>>> Cc: Syrjala, Ville <[e-mail]>; Lankhorst, Maarten

> >>>>>>> <[e-mail]>

> >>>>>>> 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 <[e-mail]>

> >>>>>>>>>

> >>>>>>>>> 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 <[e-mail]>

> >>>>>>>>> Reviewed-by: Clinton Taylor <[e-mail]>

> >>>>>>>>> Signed-off-by: Chandra Konduru <[e-mail]>

> >>>>>>>>> Signed-off-by: Nabendu Maiti

> <[e-mail]>

> >>>>>>>>> Signed-off-by: Uma Shankar <[e-mail]>

> >>>>>>>>> Signed-off-by: Vidya Srinivas <[e-mail]>

> >>>>>>>>> ---

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

Vidya Srinivas March 15, 2018, 2:35 a.m. UTC | #11

> -----Original Message-----
> From: Ville Syrjälä [mailto:[e-mail]]
> Sent: Wednesday, March 14, 2018 9:06 PM
> To: Srinivas, Vidya <[e-mail]>
> Cc: Maarten Lankhorst <[e-mail]>; intel-
> [e-mail]; Syrjala, Ville <[e-mail]>; Lankhorst,
> Maarten <[e-mail]>
> 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:[e-mail]]
> > > Sent: Wednesday, March 14, 2018 4:03 PM
> > > To: Srinivas, Vidya <[e-mail]>; intel-
> > > [e-mail]
> > > Cc: Syrjala, Ville <[e-mail]>; Lankhorst, Maarten
> > > <[e-mail]>
> > > 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:[e-mail]]
> > > >> Sent: Wednesday, March 14, 2018 3:55 PM
> > > >> To: Srinivas, Vidya <[e-mail]>; intel-
> > > >> [e-mail]
> > > >> Cc: Syrjala, Ville <[e-mail]>; Lankhorst, Maarten
> > > >> <[e-mail]>
> > > >> 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 <[e-mail]>
> > > >>>>
> > > >>>> 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 <[e-mail]>
> > > >>>> Reviewed-by: Clinton Taylor <[e-mail]>
> > > >>>> Signed-off-by: Chandra Konduru <[e-mail]>
> > > >>>> Signed-off-by: Nabendu Maiti <[e-mail]>
> > > >>>> Signed-off-by: Uma Shankar <[e-mail]>
> > > >>>> Signed-off-by: Vidya Srinivas <[e-mail]>
> > > >>>> ---
> > > >>>>  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

Patch

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;
+
 	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;
+	}
+check_scaler_range:
+	if (src_w >= SKL_MIN_SRC_W || 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)
+		goto scaler_range_ok;
+
+failed_range:
+	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;
+
+scaler_range_ok:
 	/* mark this plane as a scaler user in crtc_state */
 	scaler_state->scaler_users |= (1 << scaler_user);
 	DRM_DEBUG_KMS("scaler_user index %u.%u: "
@@ -4798,9 +4821,10 @@  int skl_update_scaler_crtc(struct intel_crtc_state *state)
 	const struct drm_display_mode *adjusted_mode = &state->base.adjusted_mode;
 
 	return skl_update_scaler(state, !state->base.active, SKL_CRTC_INDEX,
-		&state->scaler_state.scaler_id,
-		state->pipe_src_w, state->pipe_src_h,
-		adjusted_mode->crtc_hdisplay, adjusted_mode->crtc_vdisplay);
+				 &state->scaler_state.scaler_id,
+				 state->pipe_src_w, state->pipe_src_h,
+				 adjusted_mode->crtc_hdisplay,
+				 adjusted_mode->crtc_vdisplay, false, 0);
 }
 
 /**
@@ -4829,7 +4853,8 @@  static int skl_update_scaler_plane(struct intel_crtc_state *crtc_state,
 				drm_rect_width(&plane_state->base.src) >> 16,
 				drm_rect_height(&plane_state->base.src) >> 16,
 				drm_rect_width(&plane_state->base.dst),
-				drm_rect_height(&plane_state->base.dst));
+				drm_rect_height(&plane_state->base.dst),
+				fb ? true : false, fb ? fb->format->format : 0);
 
 	if (ret || plane_state->scaler_id < 0)
 		return ret;
@@ -4855,6 +4880,7 @@  static int skl_update_scaler_plane(struct intel_crtc_state *crtc_state,
 	case DRM_FORMAT_YVYU:
 	case DRM_FORMAT_UYVY:
 	case DRM_FORMAT_VYUY:
+	case DRM_FORMAT_NV12:
 		break;
 	default:
 		DRM_DEBUG_KMS("[PLANE:%d:%s] FB:%d unsupported scaling format 0x%x\n",
@@ -12831,11 +12857,13 @@  intel_cleanup_plane_fb(struct drm_plane *plane,
 }
 
 int
-skl_max_scale(struct intel_crtc *intel_crtc, struct intel_crtc_state *crtc_state)
+skl_max_scale(struct intel_crtc *intel_crtc,
+	      struct intel_crtc_state *crtc_state,
+	      uint32_t pixel_format)
 {
 	struct drm_i915_private *dev_priv;
-	int max_scale;
-	int crtc_clock, max_dotclk;
+	int max_scale, mult;
+	int crtc_clock, max_dotclk, tmpclk1, tmpclk2;
 
 	if (!intel_crtc || !crtc_state->base.enable)
 		return DRM_PLANE_HELPER_NO_SCALING;
@@ -12857,8 +12885,10 @@  skl_max_scale(struct intel_crtc *intel_crtc, struct intel_crtc_state *crtc_state
 	 *            or
 	 *    cdclk/crtc_clock
 	 */
-	max_scale = min((1 << 16) * 3 - 1,
-			(1 << 8) * ((max_dotclk << 8) / crtc_clock));
+	mult = pixel_format == DRM_FORMAT_NV12 ? 2 : 3;
+	tmpclk1 = (1 << 16) * mult - 1;
+	tmpclk2 = (1 << 8) * ((max_dotclk << 8) / crtc_clock);
+	max_scale = min(tmpclk1, tmpclk2);
 
 	return max_scale;
 }
@@ -12874,12 +12904,16 @@  intel_check_primary_plane(struct intel_plane *plane,
 	int max_scale = DRM_PLANE_HELPER_NO_SCALING;
 	bool can_position = false;
 	int ret;
+	uint32_t pixel_format = 0;
 
 	if (INTEL_GEN(dev_priv) >= 9) {
 		/* use scaler when colorkey is not required */
 		if (!state->ckey.flags) {
 			min_scale = 1;
-			max_scale = skl_max_scale(to_intel_crtc(crtc), crtc_state);
+			if (state->base.fb)
+				pixel_format = state->base.fb->format->format;
+			max_scale = skl_max_scale(to_intel_crtc(crtc),
+						  crtc_state, pixel_format);
 		}
 		can_position = true;
 	}
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 483f6ce..dadfa09 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -548,6 +548,7 @@  struct intel_initial_plane_config {
 #define SKL_MAX_DST_W 4096
 #define SKL_MIN_DST_H 8
 #define SKL_MAX_DST_H 4096
+#define SKL_MIN_YUV_420_SRC_H 16
 
 struct intel_scaler {
 	int in_use;
@@ -1592,7 +1593,8 @@  void intel_mode_from_pipe_config(struct drm_display_mode *mode,
 				 struct intel_crtc_state *pipe_config);
 
 int skl_update_scaler_crtc(struct intel_crtc_state *crtc_state);
-int skl_max_scale(struct intel_crtc *crtc, struct intel_crtc_state *crtc_state);
+int skl_max_scale(struct intel_crtc *crtc, struct intel_crtc_state *crtc_state,
+		  uint32_t pixel_format);
 
 static inline u32 intel_plane_ggtt_offset(const struct intel_plane_state *state)
 {
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 0652e58..a6e4ea5 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -974,7 +974,8 @@  intel_check_sprite_plane(struct intel_plane *plane,
 		if (!state->ckey.flags) {
 			can_scale = 1;
 			min_scale = 1;
-			max_scale = skl_max_scale(crtc, crtc_state);
+			max_scale = skl_max_scale(crtc, crtc_state,
+						  fb->format->format);
 		} else {
 			can_scale = 0;
 			min_scale = DRM_PLANE_HELPER_NO_SCALING;