[v6,6/6] drm/i915: Add NV12 as supported format for sprite plane

Commit Message

Vidya Srinivas May 6, 2018, 5:44 p.m. UTC

From: Chandra Konduru <[e-mail]>

This patch adds NV12 to list of supported formats for sprite plane.

v2: Rebased (me)

v3: Review comments by Ville addressed
- Removed skl_plane_formats_with_nv12 and added
NV12 case in existing skl_plane_formats
- Added the 10bpc RGB formats

v4: Addressed review comments from Clinton A Taylor
"Why are we adding 10 bit RGB formats with the NV12 series patches?
Trying to set XR30 or AB30 results in error returned even though
the modes are advertised for the planes"
- Removed 10bit RGB formats added previously with NV12 series

v5: Missed the Tested-by/Reviewed-by in the previous series
Adding the same to commit message in this version.
Addressed review comments from Clinton A Taylor
"Why are we adding 10 bit RGB formats with the NV12 series patches?
Trying to set XR30 or AB30 results in error returned even though
the modes are advertised for the planes"
- Previous version has 10bit RGB format removed from VLV formats
by mistake. Fixing that in this version.
Removed 10bit RGB formats added previously with NV12 series
for SKL.

v6: Addressed review comments by Ville
Restricting the NV12 to BXT and PIPE A and B

v7: Rebased (me)

v8: Rebased (me)
Restricting NV12 changes to BXT and KBL
Restricting NV12 changes for plane 0 (overlay)

v9: Rebased (me)

v10: Addressed review comments from Maarten.
Adding NV12 to skl_plane_formats itself.

v11: Addressed review comments from Shashank Sharma

v12: Addressed review comments from Shashank Sharma
Made the condition in intel_sprite_plane_create
simple and easy to read as suggested.

v13: Adding reviewed by tag from Shashank Sharma
Addressed review comments from Juha-Pekka Heikkila
"NV12 not to be supported by SKL"

v14: Addressed review comments from Ville
Added skl_planar_formats to include NV12
and a check skl_plane_has_planar in sprite create
Added NV12 format to skl_mod_supported. These were
review comments from Kristian Høgsberg <[e-mail]>

v15: Added reviewed by from Juha-Pekka Heikkila

v16: Rebased the series

v17: Added all tiling under mod supported for NV12
Credits to Megha Aggarwal

v18: Added RB by Maarten and Kristian

Credits-to: Megha Aggarwal <[e-mail]>
Credits-to: Kristian Høgsberg <[e-mail]>
Reviewed-by: Kristian Høgsberg <[e-mail]>
Reviewed-by: Maarten Lankhorst <[e-mail]>
Tested-by: Clinton Taylor <[e-mail]>
Reviewed-by: Juha-Pekka Heikkila <[e-mail]>
Reviewed-by: Shashank Sharma <[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: Vidya Srinivas <[e-mail]>
---
 drivers/gpu/drm/i915/intel_sprite.c | 29 +++++++++++++++++++++++++++--
 1 file changed, 27 insertions(+), 2 deletions(-)

Comments

Maarten Lankhorst May 7, 2018, 8:07 a.m. UTC | #1

Op 06-05-18 om 19:44 schreef Vidya Srinivas:
> From: Chandra Konduru <[e-mail]>
>
> This patch adds NV12 to list of supported formats for sprite plane.
>
> v2: Rebased (me)
>
> v3: Review comments by Ville addressed
> - Removed skl_plane_formats_with_nv12 and added
> NV12 case in existing skl_plane_formats
> - Added the 10bpc RGB formats
>
> v4: Addressed review comments from Clinton A Taylor
> "Why are we adding 10 bit RGB formats with the NV12 series patches?
> Trying to set XR30 or AB30 results in error returned even though
> the modes are advertised for the planes"
> - Removed 10bit RGB formats added previously with NV12 series
>
> v5: Missed the Tested-by/Reviewed-by in the previous series
> Adding the same to commit message in this version.
> Addressed review comments from Clinton A Taylor
> "Why are we adding 10 bit RGB formats with the NV12 series patches?
> Trying to set XR30 or AB30 results in error returned even though
> the modes are advertised for the planes"
> - Previous version has 10bit RGB format removed from VLV formats
> by mistake. Fixing that in this version.
> Removed 10bit RGB formats added previously with NV12 series
> for SKL.
>
> v6: Addressed review comments by Ville
> Restricting the NV12 to BXT and PIPE A and B
>
> v7: Rebased (me)
>
> v8: Rebased (me)
> Restricting NV12 changes to BXT and KBL
> Restricting NV12 changes for plane 0 (overlay)
>
> v9: Rebased (me)
>
> v10: Addressed review comments from Maarten.
> Adding NV12 to skl_plane_formats itself.
>
> v11: Addressed review comments from Shashank Sharma
>
> v12: Addressed review comments from Shashank Sharma
> Made the condition in intel_sprite_plane_create
> simple and easy to read as suggested.
>
> v13: Adding reviewed by tag from Shashank Sharma
> Addressed review comments from Juha-Pekka Heikkila
> "NV12 not to be supported by SKL"
>
> v14: Addressed review comments from Ville
> Added skl_planar_formats to include NV12
> and a check skl_plane_has_planar in sprite create
> Added NV12 format to skl_mod_supported. These were
> review comments from Kristian Høgsberg <[e-mail]>
>
> v15: Added reviewed by from Juha-Pekka Heikkila
>
> v16: Rebased the series
>
> v17: Added all tiling under mod supported for NV12
> Credits to Megha Aggarwal
>
> v18: Added RB by Maarten and Kristian
>
> Credits-to: Megha Aggarwal <[e-mail]>
> Credits-to: Kristian Høgsberg <[e-mail]>
> Reviewed-by: Kristian Høgsberg <[e-mail]>
> Reviewed-by: Maarten Lankhorst <[e-mail]>
> Tested-by: Clinton Taylor <[e-mail]>
> Reviewed-by: Juha-Pekka Heikkila <[e-mail]>
> Reviewed-by: Shashank Sharma <[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: Vidya Srinivas <[e-mail]>
> ---
>  drivers/gpu/drm/i915/intel_sprite.c | 29 +++++++++++++++++++++++++++--
>  1 file changed, 27 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index c73553a..cdcae9e 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -1180,6 +1180,19 @@ static uint32_t skl_plane_formats[] = {
>  	DRM_FORMAT_VYUY,
>  };
>  
> +static uint32_t skl_planar_formats[] = {
> +	DRM_FORMAT_RGB565,
> +	DRM_FORMAT_ABGR8888,
> +	DRM_FORMAT_ARGB8888,
> +	DRM_FORMAT_XBGR8888,
> +	DRM_FORMAT_XRGB8888,
> +	DRM_FORMAT_YUYV,
> +	DRM_FORMAT_YVYU,
> +	DRM_FORMAT_UYVY,
> +	DRM_FORMAT_VYUY,
> +	DRM_FORMAT_NV12,
> +};
> +
>  static const uint64_t skl_plane_format_modifiers_noccs[] = {
>  	I915_FORMAT_MOD_Yf_TILED,
>  	I915_FORMAT_MOD_Y_TILED,
> @@ -1277,6 +1290,12 @@ static bool skl_mod_supported(uint32_t format, uint64_t modifier)
>  		if (modifier == I915_FORMAT_MOD_Yf_TILED)
>  			return true;
>  		/* fall through */
> +	case DRM_FORMAT_NV12:
> +		if (modifier == DRM_FORMAT_MOD_LINEAR ||
> +		    modifier == I915_FORMAT_MOD_X_TILED ||
> +		    modifier == I915_FORMAT_MOD_Y_TILED ||
> +		    modifier == I915_FORMAT_MOD_Yf_TILED)
> +			return true;
On patch 5 and 6: this is a tad overkill, just put it below DRM_FORMAT_VYUY and let it fall through. It's not different from the other formats. :)
Otherwise looks good, I'll wait until the changes from drm-misc-next are merged into drm-intel-next-queued then this series can be applied with the minor fixup.
>  	case DRM_FORMAT_C8:
>  		if (modifier == DRM_FORMAT_MOD_LINEAR ||
>  		    modifier == I915_FORMAT_MOD_X_TILED ||
> @@ -1373,8 +1392,14 @@ intel_sprite_plane_create(struct drm_i915_private *dev_priv,
>  		intel_plane->disable_plane = skl_disable_plane;
>  		intel_plane->get_hw_state = skl_plane_get_hw_state;
>  
> -		plane_formats = skl_plane_formats;
> -		num_plane_formats = ARRAY_SIZE(skl_plane_formats);
> +		if (skl_plane_has_planar(dev_priv, pipe,
> +					 PLANE_SPRITE0 + plane)) {
> +			plane_formats = skl_planar_formats;
> +			num_plane_formats = ARRAY_SIZE(skl_planar_formats);
> +		} else {
> +			plane_formats = skl_plane_formats;
> +			num_plane_formats = ARRAY_SIZE(skl_plane_formats);
> +		}
>  
>  		if (skl_plane_has_ccs(dev_priv, pipe, PLANE_SPRITE0 + plane))
>  			modifiers = skl_plane_format_modifiers_ccs;

Vidya Srinivas May 7, 2018, 8:11 a.m. UTC | #2

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

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

> Sent: Monday, May 7, 2018 1:38 PM

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

> [e-mail]

> Subject: Re: [Intel-gfx] [PATCH v6 6/6] drm/i915: Add NV12 as supported

> format for sprite plane

> 

> Op 06-05-18 om 19:44 schreef Vidya Srinivas:

> > From: Chandra Konduru <[e-mail]>

> >

> > This patch adds NV12 to list of supported formats for sprite plane.

> >

> > v2: Rebased (me)

> >

> > v3: Review comments by Ville addressed

> > - Removed skl_plane_formats_with_nv12 and added

> > NV12 case in existing skl_plane_formats

> > - Added the 10bpc RGB formats

> >

> > v4: Addressed review comments from Clinton A Taylor "Why are we

> adding

> > 10 bit RGB formats with the NV12 series patches?

> > Trying to set XR30 or AB30 results in error returned even though the

> > modes are advertised for the planes"

> > - Removed 10bit RGB formats added previously with NV12 series

> >

> > v5: Missed the Tested-by/Reviewed-by in the previous series Adding the

> > same to commit message in this version.

> > Addressed review comments from Clinton A Taylor "Why are we adding 10

> > bit RGB formats with the NV12 series patches?

> > Trying to set XR30 or AB30 results in error returned even though the

> > modes are advertised for the planes"

> > - Previous version has 10bit RGB format removed from VLV formats by

> > mistake. Fixing that in this version.

> > Removed 10bit RGB formats added previously with NV12 series for SKL.

> >

> > v6: Addressed review comments by Ville Restricting the NV12 to BXT and

> > PIPE A and B

> >

> > v7: Rebased (me)

> >

> > v8: Rebased (me)

> > Restricting NV12 changes to BXT and KBL Restricting NV12 changes for

> > plane 0 (overlay)

> >

> > v9: Rebased (me)

> >

> > v10: Addressed review comments from Maarten.

> > Adding NV12 to skl_plane_formats itself.

> >

> > v11: Addressed review comments from Shashank Sharma

> >

> > v12: Addressed review comments from Shashank Sharma Made the

> condition

> > in intel_sprite_plane_create simple and easy to read as suggested.

> >

> > v13: Adding reviewed by tag from Shashank Sharma Addressed review

> > comments from Juha-Pekka Heikkila

> > "NV12 not to be supported by SKL"

> >

> > v14: Addressed review comments from Ville Added skl_planar_formats to

> > include NV12 and a check skl_plane_has_planar in sprite create Added

> > NV12 format to skl_mod_supported. These were review comments from

> > Kristian Høgsberg <[e-mail]>

> >

> > v15: Added reviewed by from Juha-Pekka Heikkila

> >

> > v16: Rebased the series

> >

> > v17: Added all tiling under mod supported for NV12 Credits to Megha

> > Aggarwal

> >

> > v18: Added RB by Maarten and Kristian

> >

> > Credits-to: Megha Aggarwal <[e-mail]>

> > Credits-to: Kristian Høgsberg <[e-mail]>

> > Reviewed-by: Kristian Høgsberg <[e-mail]>

> > Reviewed-by: Maarten Lankhorst <[e-mail]>

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

> > Reviewed-by: Juha-Pekka Heikkila <[e-mail]>

> > Reviewed-by: Shashank Sharma <[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: Vidya Srinivas <[e-mail]>

> > ---

> >  drivers/gpu/drm/i915/intel_sprite.c | 29

> > +++++++++++++++++++++++++++--

> >  1 file changed, 27 insertions(+), 2 deletions(-)

> >

> > diff --git a/drivers/gpu/drm/i915/intel_sprite.c

> > b/drivers/gpu/drm/i915/intel_sprite.c

> > index c73553a..cdcae9e 100644

> > --- a/drivers/gpu/drm/i915/intel_sprite.c

> > +++ b/drivers/gpu/drm/i915/intel_sprite.c

> > @@ -1180,6 +1180,19 @@ static uint32_t skl_plane_formats[] = {

> >  	DRM_FORMAT_VYUY,

> >  };

> >

> > +static uint32_t skl_planar_formats[] = {

> > +	DRM_FORMAT_RGB565,

> > +	DRM_FORMAT_ABGR8888,

> > +	DRM_FORMAT_ARGB8888,

> > +	DRM_FORMAT_XBGR8888,

> > +	DRM_FORMAT_XRGB8888,

> > +	DRM_FORMAT_YUYV,

> > +	DRM_FORMAT_YVYU,

> > +	DRM_FORMAT_UYVY,

> > +	DRM_FORMAT_VYUY,

> > +	DRM_FORMAT_NV12,

> > +};

> > +

> >  static const uint64_t skl_plane_format_modifiers_noccs[] = {

> >  	I915_FORMAT_MOD_Yf_TILED,

> >  	I915_FORMAT_MOD_Y_TILED,

> > @@ -1277,6 +1290,12 @@ static bool skl_mod_supported(uint32_t

> format, uint64_t modifier)

> >  		if (modifier == I915_FORMAT_MOD_Yf_TILED)

> >  			return true;

> >  		/* fall through */

> > +	case DRM_FORMAT_NV12:

> > +		if (modifier == DRM_FORMAT_MOD_LINEAR ||

> > +		    modifier == I915_FORMAT_MOD_X_TILED ||

> > +		    modifier == I915_FORMAT_MOD_Y_TILED ||

> > +		    modifier == I915_FORMAT_MOD_Yf_TILED)

> > +			return true;

> On patch 5 and 6: this is a tad overkill, just put it below

> DRM_FORMAT_VYUY and let it fall through. It's not different from the other

> formats. :) Otherwise looks good, I'll wait until the changes from drm-misc-

> next are merged into drm-intel-next-queued then this series can be applied

> with the minor fixup.


Sure, thank you. I'll make the change. I did this to fix a review comment - which said all modifiers aren’t
Covered for NV12 - when we earlier had it under VYUV. Would that be still okay?

Regards
Vidya

> >  	case DRM_FORMAT_C8:

> >  		if (modifier == DRM_FORMAT_MOD_LINEAR ||

> >  		    modifier == I915_FORMAT_MOD_X_TILED || @@ -1373,8

> +1392,14 @@

> > intel_sprite_plane_create(struct drm_i915_private *dev_priv,

> >  		intel_plane->disable_plane = skl_disable_plane;

> >  		intel_plane->get_hw_state = skl_plane_get_hw_state;

> >

> > -		plane_formats = skl_plane_formats;

> > -		num_plane_formats = ARRAY_SIZE(skl_plane_formats);

> > +		if (skl_plane_has_planar(dev_priv, pipe,

> > +					 PLANE_SPRITE0 + plane)) {

> > +			plane_formats = skl_planar_formats;

> > +			num_plane_formats =

> ARRAY_SIZE(skl_planar_formats);

> > +		} else {

> > +			plane_formats = skl_plane_formats;

> > +			num_plane_formats =

> ARRAY_SIZE(skl_plane_formats);

> > +		}

> >

> >  		if (skl_plane_has_ccs(dev_priv, pipe, PLANE_SPRITE0 +

> plane))

> >  			modifiers = skl_plane_format_modifiers_ccs;

>

Maarten Lankhorst May 7, 2018, 8:13 a.m. UTC | #3

Op 07-05-18 om 10:11 schreef Srinivas, Vidya:
>
>> -----Original Message-----
>> From: Maarten Lankhorst [mailto:[e-mail]]
>> Sent: Monday, May 7, 2018 1:38 PM
>> To: Srinivas, Vidya <[e-mail]>; intel-
>> [e-mail]
>> Subject: Re: [Intel-gfx] [PATCH v6 6/6] drm/i915: Add NV12 as supported
>> format for sprite plane
>>
>> Op 06-05-18 om 19:44 schreef Vidya Srinivas:
>>> From: Chandra Konduru <[e-mail]>
>>>
>>> This patch adds NV12 to list of supported formats for sprite plane.
>>>
>>> v2: Rebased (me)
>>>
>>> v3: Review comments by Ville addressed
>>> - Removed skl_plane_formats_with_nv12 and added
>>> NV12 case in existing skl_plane_formats
>>> - Added the 10bpc RGB formats
>>>
>>> v4: Addressed review comments from Clinton A Taylor "Why are we
>> adding
>>> 10 bit RGB formats with the NV12 series patches?
>>> Trying to set XR30 or AB30 results in error returned even though the
>>> modes are advertised for the planes"
>>> - Removed 10bit RGB formats added previously with NV12 series
>>>
>>> v5: Missed the Tested-by/Reviewed-by in the previous series Adding the
>>> same to commit message in this version.
>>> Addressed review comments from Clinton A Taylor "Why are we adding 10
>>> bit RGB formats with the NV12 series patches?
>>> Trying to set XR30 or AB30 results in error returned even though the
>>> modes are advertised for the planes"
>>> - Previous version has 10bit RGB format removed from VLV formats by
>>> mistake. Fixing that in this version.
>>> Removed 10bit RGB formats added previously with NV12 series for SKL.
>>>
>>> v6: Addressed review comments by Ville Restricting the NV12 to BXT and
>>> PIPE A and B
>>>
>>> v7: Rebased (me)
>>>
>>> v8: Rebased (me)
>>> Restricting NV12 changes to BXT and KBL Restricting NV12 changes for
>>> plane 0 (overlay)
>>>
>>> v9: Rebased (me)
>>>
>>> v10: Addressed review comments from Maarten.
>>> Adding NV12 to skl_plane_formats itself.
>>>
>>> v11: Addressed review comments from Shashank Sharma
>>>
>>> v12: Addressed review comments from Shashank Sharma Made the
>> condition
>>> in intel_sprite_plane_create simple and easy to read as suggested.
>>>
>>> v13: Adding reviewed by tag from Shashank Sharma Addressed review
>>> comments from Juha-Pekka Heikkila
>>> "NV12 not to be supported by SKL"
>>>
>>> v14: Addressed review comments from Ville Added skl_planar_formats to
>>> include NV12 and a check skl_plane_has_planar in sprite create Added
>>> NV12 format to skl_mod_supported. These were review comments from
>>> Kristian Høgsberg <[e-mail]>
>>>
>>> v15: Added reviewed by from Juha-Pekka Heikkila
>>>
>>> v16: Rebased the series
>>>
>>> v17: Added all tiling under mod supported for NV12 Credits to Megha
>>> Aggarwal
>>>
>>> v18: Added RB by Maarten and Kristian
>>>
>>> Credits-to: Megha Aggarwal <[e-mail]>
>>> Credits-to: Kristian Høgsberg <[e-mail]>
>>> Reviewed-by: Kristian Høgsberg <[e-mail]>
>>> Reviewed-by: Maarten Lankhorst <[e-mail]>
>>> Tested-by: Clinton Taylor <[e-mail]>
>>> Reviewed-by: Juha-Pekka Heikkila <[e-mail]>
>>> Reviewed-by: Shashank Sharma <[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: Vidya Srinivas <[e-mail]>
>>> ---
>>>  drivers/gpu/drm/i915/intel_sprite.c | 29
>>> +++++++++++++++++++++++++++--
>>>  1 file changed, 27 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_sprite.c
>>> b/drivers/gpu/drm/i915/intel_sprite.c
>>> index c73553a..cdcae9e 100644
>>> --- a/drivers/gpu/drm/i915/intel_sprite.c
>>> +++ b/drivers/gpu/drm/i915/intel_sprite.c
>>> @@ -1180,6 +1180,19 @@ static uint32_t skl_plane_formats[] = {
>>>  	DRM_FORMAT_VYUY,
>>>  };
>>>
>>> +static uint32_t skl_planar_formats[] = {
>>> +	DRM_FORMAT_RGB565,
>>> +	DRM_FORMAT_ABGR8888,
>>> +	DRM_FORMAT_ARGB8888,
>>> +	DRM_FORMAT_XBGR8888,
>>> +	DRM_FORMAT_XRGB8888,
>>> +	DRM_FORMAT_YUYV,
>>> +	DRM_FORMAT_YVYU,
>>> +	DRM_FORMAT_UYVY,
>>> +	DRM_FORMAT_VYUY,
>>> +	DRM_FORMAT_NV12,
>>> +};
>>> +
>>>  static const uint64_t skl_plane_format_modifiers_noccs[] = {
>>>  	I915_FORMAT_MOD_Yf_TILED,
>>>  	I915_FORMAT_MOD_Y_TILED,
>>> @@ -1277,6 +1290,12 @@ static bool skl_mod_supported(uint32_t
>> format, uint64_t modifier)
>>>  		if (modifier == I915_FORMAT_MOD_Yf_TILED)
>>>  			return true;
>>>  		/* fall through */
>>> +	case DRM_FORMAT_NV12:
>>> +		if (modifier == DRM_FORMAT_MOD_LINEAR ||
>>> +		    modifier == I915_FORMAT_MOD_X_TILED ||
>>> +		    modifier == I915_FORMAT_MOD_Y_TILED ||
>>> +		    modifier == I915_FORMAT_MOD_Yf_TILED)
>>> +			return true;
>> On patch 5 and 6: this is a tad overkill, just put it below
>> DRM_FORMAT_VYUY and let it fall through. It's not different from the other
>> formats. :) Otherwise looks good, I'll wait until the changes from drm-misc-
>> next are merged into drm-intel-next-queued then this series can be applied
>> with the minor fixup.
> Sure, thank you. I'll make the change. I did this to fix a review comment - which said all modifiers aren’t
> Covered for NV12 - when we earlier had it under VYUV. Would that be still okay?
Seems only CCS isn't covered, so putting it there should be fine. :)

Vidya Srinivas May 7, 2018, 8:16 a.m. UTC | #4

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

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

> Sent: Monday, May 7, 2018 1:44 PM

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

> [e-mail]

> Subject: Re: [Intel-gfx] [PATCH v6 6/6] drm/i915: Add NV12 as supported

> format for sprite plane

> 

> Op 07-05-18 om 10:11 schreef Srinivas, Vidya:

> >

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

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

> >> Sent: Monday, May 7, 2018 1:38 PM

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

> >> [e-mail]

> >> Subject: Re: [Intel-gfx] [PATCH v6 6/6] drm/i915: Add NV12 as

> >> supported format for sprite plane

> >>

> >> Op 06-05-18 om 19:44 schreef Vidya Srinivas:

> >>> From: Chandra Konduru <[e-mail]>

> >>>

> >>> This patch adds NV12 to list of supported formats for sprite plane.

> >>>

> >>> v2: Rebased (me)

> >>>

> >>> v3: Review comments by Ville addressed

> >>> - Removed skl_plane_formats_with_nv12 and added

> >>> NV12 case in existing skl_plane_formats

> >>> - Added the 10bpc RGB formats

> >>>

> >>> v4: Addressed review comments from Clinton A Taylor "Why are we

> >> adding

> >>> 10 bit RGB formats with the NV12 series patches?

> >>> Trying to set XR30 or AB30 results in error returned even though the

> >>> modes are advertised for the planes"

> >>> - Removed 10bit RGB formats added previously with NV12 series

> >>>

> >>> v5: Missed the Tested-by/Reviewed-by in the previous series Adding

> >>> the same to commit message in this version.

> >>> Addressed review comments from Clinton A Taylor "Why are we adding

> >>> 10 bit RGB formats with the NV12 series patches?

> >>> Trying to set XR30 or AB30 results in error returned even though the

> >>> modes are advertised for the planes"

> >>> - Previous version has 10bit RGB format removed from VLV formats by

> >>> mistake. Fixing that in this version.

> >>> Removed 10bit RGB formats added previously with NV12 series for SKL.

> >>>

> >>> v6: Addressed review comments by Ville Restricting the NV12 to BXT

> >>> and PIPE A and B

> >>>

> >>> v7: Rebased (me)

> >>>

> >>> v8: Rebased (me)

> >>> Restricting NV12 changes to BXT and KBL Restricting NV12 changes for

> >>> plane 0 (overlay)

> >>>

> >>> v9: Rebased (me)

> >>>

> >>> v10: Addressed review comments from Maarten.

> >>> Adding NV12 to skl_plane_formats itself.

> >>>

> >>> v11: Addressed review comments from Shashank Sharma

> >>>

> >>> v12: Addressed review comments from Shashank Sharma Made the

> >> condition

> >>> in intel_sprite_plane_create simple and easy to read as suggested.

> >>>

> >>> v13: Adding reviewed by tag from Shashank Sharma Addressed review

> >>> comments from Juha-Pekka Heikkila

> >>> "NV12 not to be supported by SKL"

> >>>

> >>> v14: Addressed review comments from Ville Added skl_planar_formats

> >>> to include NV12 and a check skl_plane_has_planar in sprite create

> >>> Added

> >>> NV12 format to skl_mod_supported. These were review comments

> from

> >>> Kristian Høgsberg <[e-mail]>

> >>>

> >>> v15: Added reviewed by from Juha-Pekka Heikkila

> >>>

> >>> v16: Rebased the series

> >>>

> >>> v17: Added all tiling under mod supported for NV12 Credits to Megha

> >>> Aggarwal

> >>>

> >>> v18: Added RB by Maarten and Kristian

> >>>

> >>> Credits-to: Megha Aggarwal <[e-mail]>

> >>> Credits-to: Kristian Høgsberg <[e-mail]>

> >>> Reviewed-by: Kristian Høgsberg <[e-mail]>

> >>> Reviewed-by: Maarten Lankhorst <[e-mail]>

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

> >>> Reviewed-by: Juha-Pekka Heikkila <[e-mail]>

> >>> Reviewed-by: Shashank Sharma <[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: Vidya Srinivas <[e-mail]>

> >>> ---

> >>>  drivers/gpu/drm/i915/intel_sprite.c | 29

> >>> +++++++++++++++++++++++++++--

> >>>  1 file changed, 27 insertions(+), 2 deletions(-)

> >>>

> >>> diff --git a/drivers/gpu/drm/i915/intel_sprite.c

> >>> b/drivers/gpu/drm/i915/intel_sprite.c

> >>> index c73553a..cdcae9e 100644

> >>> --- a/drivers/gpu/drm/i915/intel_sprite.c

> >>> +++ b/drivers/gpu/drm/i915/intel_sprite.c

> >>> @@ -1180,6 +1180,19 @@ static uint32_t skl_plane_formats[] = {

> >>>  	DRM_FORMAT_VYUY,

> >>>  };

> >>>

> >>> +static uint32_t skl_planar_formats[] = {

> >>> +	DRM_FORMAT_RGB565,

> >>> +	DRM_FORMAT_ABGR8888,

> >>> +	DRM_FORMAT_ARGB8888,

> >>> +	DRM_FORMAT_XBGR8888,

> >>> +	DRM_FORMAT_XRGB8888,

> >>> +	DRM_FORMAT_YUYV,

> >>> +	DRM_FORMAT_YVYU,

> >>> +	DRM_FORMAT_UYVY,

> >>> +	DRM_FORMAT_VYUY,

> >>> +	DRM_FORMAT_NV12,

> >>> +};

> >>> +

> >>>  static const uint64_t skl_plane_format_modifiers_noccs[] = {

> >>>  	I915_FORMAT_MOD_Yf_TILED,

> >>>  	I915_FORMAT_MOD_Y_TILED,

> >>> @@ -1277,6 +1290,12 @@ static bool skl_mod_supported(uint32_t

> >> format, uint64_t modifier)

> >>>  		if (modifier == I915_FORMAT_MOD_Yf_TILED)

> >>>  			return true;

> >>>  		/* fall through */

> >>> +	case DRM_FORMAT_NV12:

> >>> +		if (modifier == DRM_FORMAT_MOD_LINEAR ||

> >>> +		    modifier == I915_FORMAT_MOD_X_TILED ||

> >>> +		    modifier == I915_FORMAT_MOD_Y_TILED ||

> >>> +		    modifier == I915_FORMAT_MOD_Yf_TILED)

> >>> +			return true;

> >> On patch 5 and 6: this is a tad overkill, just put it below

> >> DRM_FORMAT_VYUY and let it fall through. It's not different from the

> >> other formats. :) Otherwise looks good, I'll wait until the changes

> >> from drm-misc- next are merged into drm-intel-next-queued then this

> >> series can be applied with the minor fixup.

> > Sure, thank you. I'll make the change. I did this to fix a review

> > comment - which said all modifiers aren’t Covered for NV12 - when we

> earlier had it under VYUV. Would that be still okay?

> Seems only CCS isn't covered, so putting it there should be fine. :)

Oh okay - sure thank you. Will make the change and float the series.

Regards
Vidya

Vidya Srinivas May 7, 2018, 8:20 a.m. UTC | #5

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

> From: Srinivas, Vidya

> Sent: Monday, May 7, 2018 1:46 PM

> To: 'Maarten Lankhorst' <[e-mail]>; intel-

> [e-mail]

> Subject: RE: [Intel-gfx] [PATCH v6 6/6] drm/i915: Add NV12 as supported

> format for sprite plane

> 

> 

> 

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

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

> > Sent: Monday, May 7, 2018 1:44 PM

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

> > [e-mail]

> > Subject: Re: [Intel-gfx] [PATCH v6 6/6] drm/i915: Add NV12 as

> > supported format for sprite plane

> >

> > Op 07-05-18 om 10:11 schreef Srinivas, Vidya:

> > >

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

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

> > >> Sent: Monday, May 7, 2018 1:38 PM

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

> > >> [e-mail]

> > >> Subject: Re: [Intel-gfx] [PATCH v6 6/6] drm/i915: Add NV12 as

> > >> supported format for sprite plane

> > >>

> > >> Op 06-05-18 om 19:44 schreef Vidya Srinivas:

> > >>> From: Chandra Konduru <[e-mail]>

> > >>>

> > >>> This patch adds NV12 to list of supported formats for sprite plane.

> > >>>

> > >>> v2: Rebased (me)

> > >>>

> > >>> v3: Review comments by Ville addressed

> > >>> - Removed skl_plane_formats_with_nv12 and added

> > >>> NV12 case in existing skl_plane_formats

> > >>> - Added the 10bpc RGB formats

> > >>>

> > >>> v4: Addressed review comments from Clinton A Taylor "Why are we

> > >> adding

> > >>> 10 bit RGB formats with the NV12 series patches?

> > >>> Trying to set XR30 or AB30 results in error returned even though

> > >>> the modes are advertised for the planes"

> > >>> - Removed 10bit RGB formats added previously with NV12 series

> > >>>

> > >>> v5: Missed the Tested-by/Reviewed-by in the previous series Adding

> > >>> the same to commit message in this version.

> > >>> Addressed review comments from Clinton A Taylor "Why are we

> adding

> > >>> 10 bit RGB formats with the NV12 series patches?

> > >>> Trying to set XR30 or AB30 results in error returned even though

> > >>> the modes are advertised for the planes"

> > >>> - Previous version has 10bit RGB format removed from VLV formats

> > >>> by mistake. Fixing that in this version.

> > >>> Removed 10bit RGB formats added previously with NV12 series for

> SKL.

> > >>>

> > >>> v6: Addressed review comments by Ville Restricting the NV12 to BXT

> > >>> and PIPE A and B

> > >>>

> > >>> v7: Rebased (me)

> > >>>

> > >>> v8: Rebased (me)

> > >>> Restricting NV12 changes to BXT and KBL Restricting NV12 changes

> > >>> for plane 0 (overlay)

> > >>>

> > >>> v9: Rebased (me)

> > >>>

> > >>> v10: Addressed review comments from Maarten.

> > >>> Adding NV12 to skl_plane_formats itself.

> > >>>

> > >>> v11: Addressed review comments from Shashank Sharma

> > >>>

> > >>> v12: Addressed review comments from Shashank Sharma Made the

> > >> condition

> > >>> in intel_sprite_plane_create simple and easy to read as suggested.

> > >>>

> > >>> v13: Adding reviewed by tag from Shashank Sharma Addressed review

> > >>> comments from Juha-Pekka Heikkila

> > >>> "NV12 not to be supported by SKL"

> > >>>

> > >>> v14: Addressed review comments from Ville Added

> skl_planar_formats

> > >>> to include NV12 and a check skl_plane_has_planar in sprite create

> > >>> Added

> > >>> NV12 format to skl_mod_supported. These were review comments

> > from

> > >>> Kristian Høgsberg <[e-mail]>

> > >>>

> > >>> v15: Added reviewed by from Juha-Pekka Heikkila

> > >>>

> > >>> v16: Rebased the series

> > >>>

> > >>> v17: Added all tiling under mod supported for NV12 Credits to

> > >>> Megha Aggarwal

> > >>>

> > >>> v18: Added RB by Maarten and Kristian

> > >>>

> > >>> Credits-to: Megha Aggarwal <[e-mail]>

> > >>> Credits-to: Kristian Høgsberg <[e-mail]>

> > >>> Reviewed-by: Kristian Høgsberg <[e-mail]>

> > >>> Reviewed-by: Maarten Lankhorst

> <[e-mail]>

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

> > >>> Reviewed-by: Juha-Pekka Heikkila <[e-mail]>

> > >>> Reviewed-by: Shashank Sharma <[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: Vidya Srinivas <[e-mail]>

> > >>> ---

> > >>>  drivers/gpu/drm/i915/intel_sprite.c | 29

> > >>> +++++++++++++++++++++++++++--

> > >>>  1 file changed, 27 insertions(+), 2 deletions(-)

> > >>>

> > >>> diff --git a/drivers/gpu/drm/i915/intel_sprite.c

> > >>> b/drivers/gpu/drm/i915/intel_sprite.c

> > >>> index c73553a..cdcae9e 100644

> > >>> --- a/drivers/gpu/drm/i915/intel_sprite.c

> > >>> +++ b/drivers/gpu/drm/i915/intel_sprite.c

> > >>> @@ -1180,6 +1180,19 @@ static uint32_t skl_plane_formats[] = {

> > >>>  	DRM_FORMAT_VYUY,

> > >>>  };

> > >>>

> > >>> +static uint32_t skl_planar_formats[] = {

> > >>> +	DRM_FORMAT_RGB565,

> > >>> +	DRM_FORMAT_ABGR8888,

> > >>> +	DRM_FORMAT_ARGB8888,

> > >>> +	DRM_FORMAT_XBGR8888,

> > >>> +	DRM_FORMAT_XRGB8888,

> > >>> +	DRM_FORMAT_YUYV,

> > >>> +	DRM_FORMAT_YVYU,

> > >>> +	DRM_FORMAT_UYVY,

> > >>> +	DRM_FORMAT_VYUY,

> > >>> +	DRM_FORMAT_NV12,

> > >>> +};

> > >>> +

> > >>>  static const uint64_t skl_plane_format_modifiers_noccs[] = {

> > >>>  	I915_FORMAT_MOD_Yf_TILED,

> > >>>  	I915_FORMAT_MOD_Y_TILED,

> > >>> @@ -1277,6 +1290,12 @@ static bool skl_mod_supported(uint32_t

> > >> format, uint64_t modifier)

> > >>>  		if (modifier == I915_FORMAT_MOD_Yf_TILED)

> > >>>  			return true;

> > >>>  		/* fall through */

> > >>> +	case DRM_FORMAT_NV12:

> > >>> +		if (modifier == DRM_FORMAT_MOD_LINEAR ||

> > >>> +		    modifier == I915_FORMAT_MOD_X_TILED ||

> > >>> +		    modifier == I915_FORMAT_MOD_Y_TILED ||

> > >>> +		    modifier == I915_FORMAT_MOD_Yf_TILED)

> > >>> +			return true;

> > >> On patch 5 and 6: this is a tad overkill, just put it below

> > >> DRM_FORMAT_VYUY and let it fall through. It's not different from

> > >> the other formats. :) Otherwise looks good, I'll wait until the

> > >> changes from drm-misc- next are merged into drm-intel-next-queued

> > >> then this series can be applied with the minor fixup.

> > > Sure, thank you. I'll make the change. I did this to fix a review

> > > comment - which said all modifiers aren’t Covered for NV12 - when we

> > earlier had it under VYUV. Would that be still okay?

> > Seems only CCS isn't covered, so putting it there should be fine. :)

> Oh okay - sure thank you. Will make the change and float the series.

Sorry to ask - under VYUV only Yf_TILED returns true. Don’t we need to
add the other modifiers like MOD_LINEAR, MOD_X_TILED and MOD_Y_TILED?
> 

> Regards

> Vidya

Maarten Lankhorst May 7, 2018, 8:25 a.m. UTC | #6

Op 07-05-18 om 10:20 schreef Srinivas, Vidya:
>
>> -----Original Message-----
>> From: Srinivas, Vidya
>> Sent: Monday, May 7, 2018 1:46 PM
>> To: 'Maarten Lankhorst' <[e-mail]>; intel-
>> [e-mail]
>> Subject: RE: [Intel-gfx] [PATCH v6 6/6] drm/i915: Add NV12 as supported
>> format for sprite plane
>>
>>
>>
>>> -----Original Message-----
>>> From: Maarten Lankhorst [mailto:[e-mail]]
>>> Sent: Monday, May 7, 2018 1:44 PM
>>> To: Srinivas, Vidya <[e-mail]>; intel-
>>> [e-mail]
>>> Subject: Re: [Intel-gfx] [PATCH v6 6/6] drm/i915: Add NV12 as
>>> supported format for sprite plane
>>>
>>> Op 07-05-18 om 10:11 schreef Srinivas, Vidya:
>>>>> -----Original Message-----
>>>>> From: Maarten Lankhorst [mailto:[e-mail]]
>>>>> Sent: Monday, May 7, 2018 1:38 PM
>>>>> To: Srinivas, Vidya <[e-mail]>; intel-
>>>>> [e-mail]
>>>>> Subject: Re: [Intel-gfx] [PATCH v6 6/6] drm/i915: Add NV12 as
>>>>> supported format for sprite plane
>>>>>
>>>>> Op 06-05-18 om 19:44 schreef Vidya Srinivas:
>>>>>> From: Chandra Konduru <[e-mail]>
>>>>>>
>>>>>> This patch adds NV12 to list of supported formats for sprite plane.
>>>>>>
>>>>>> v2: Rebased (me)
>>>>>>
>>>>>> v3: Review comments by Ville addressed
>>>>>> - Removed skl_plane_formats_with_nv12 and added
>>>>>> NV12 case in existing skl_plane_formats
>>>>>> - Added the 10bpc RGB formats
>>>>>>
>>>>>> v4: Addressed review comments from Clinton A Taylor "Why are we
>>>>> adding
>>>>>> 10 bit RGB formats with the NV12 series patches?
>>>>>> Trying to set XR30 or AB30 results in error returned even though
>>>>>> the modes are advertised for the planes"
>>>>>> - Removed 10bit RGB formats added previously with NV12 series
>>>>>>
>>>>>> v5: Missed the Tested-by/Reviewed-by in the previous series Adding
>>>>>> the same to commit message in this version.
>>>>>> Addressed review comments from Clinton A Taylor "Why are we
>> adding
>>>>>> 10 bit RGB formats with the NV12 series patches?
>>>>>> Trying to set XR30 or AB30 results in error returned even though
>>>>>> the modes are advertised for the planes"
>>>>>> - Previous version has 10bit RGB format removed from VLV formats
>>>>>> by mistake. Fixing that in this version.
>>>>>> Removed 10bit RGB formats added previously with NV12 series for
>> SKL.
>>>>>> v6: Addressed review comments by Ville Restricting the NV12 to BXT
>>>>>> and PIPE A and B
>>>>>>
>>>>>> v7: Rebased (me)
>>>>>>
>>>>>> v8: Rebased (me)
>>>>>> Restricting NV12 changes to BXT and KBL Restricting NV12 changes
>>>>>> for plane 0 (overlay)
>>>>>>
>>>>>> v9: Rebased (me)
>>>>>>
>>>>>> v10: Addressed review comments from Maarten.
>>>>>> Adding NV12 to skl_plane_formats itself.
>>>>>>
>>>>>> v11: Addressed review comments from Shashank Sharma
>>>>>>
>>>>>> v12: Addressed review comments from Shashank Sharma Made the
>>>>> condition
>>>>>> in intel_sprite_plane_create simple and easy to read as suggested.
>>>>>>
>>>>>> v13: Adding reviewed by tag from Shashank Sharma Addressed review
>>>>>> comments from Juha-Pekka Heikkila
>>>>>> "NV12 not to be supported by SKL"
>>>>>>
>>>>>> v14: Addressed review comments from Ville Added
>> skl_planar_formats
>>>>>> to include NV12 and a check skl_plane_has_planar in sprite create
>>>>>> Added
>>>>>> NV12 format to skl_mod_supported. These were review comments
>>> from
>>>>>> Kristian Høgsberg <[e-mail]>
>>>>>>
>>>>>> v15: Added reviewed by from Juha-Pekka Heikkila
>>>>>>
>>>>>> v16: Rebased the series
>>>>>>
>>>>>> v17: Added all tiling under mod supported for NV12 Credits to
>>>>>> Megha Aggarwal
>>>>>>
>>>>>> v18: Added RB by Maarten and Kristian
>>>>>>
>>>>>> Credits-to: Megha Aggarwal <[e-mail]>
>>>>>> Credits-to: Kristian Høgsberg <[e-mail]>
>>>>>> Reviewed-by: Kristian Høgsberg <[e-mail]>
>>>>>> Reviewed-by: Maarten Lankhorst
>> <[e-mail]>
>>>>>> Tested-by: Clinton Taylor <[e-mail]>
>>>>>> Reviewed-by: Juha-Pekka Heikkila <[e-mail]>
>>>>>> Reviewed-by: Shashank Sharma <[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: Vidya Srinivas <[e-mail]>
>>>>>> ---
>>>>>>  drivers/gpu/drm/i915/intel_sprite.c | 29
>>>>>> +++++++++++++++++++++++++++--
>>>>>>  1 file changed, 27 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/i915/intel_sprite.c
>>>>>> b/drivers/gpu/drm/i915/intel_sprite.c
>>>>>> index c73553a..cdcae9e 100644
>>>>>> --- a/drivers/gpu/drm/i915/intel_sprite.c
>>>>>> +++ b/drivers/gpu/drm/i915/intel_sprite.c
>>>>>> @@ -1180,6 +1180,19 @@ static uint32_t skl_plane_formats[] = {
>>>>>>  	DRM_FORMAT_VYUY,
>>>>>>  };
>>>>>>
>>>>>> +static uint32_t skl_planar_formats[] = {
>>>>>> +	DRM_FORMAT_RGB565,
>>>>>> +	DRM_FORMAT_ABGR8888,
>>>>>> +	DRM_FORMAT_ARGB8888,
>>>>>> +	DRM_FORMAT_XBGR8888,
>>>>>> +	DRM_FORMAT_XRGB8888,
>>>>>> +	DRM_FORMAT_YUYV,
>>>>>> +	DRM_FORMAT_YVYU,
>>>>>> +	DRM_FORMAT_UYVY,
>>>>>> +	DRM_FORMAT_VYUY,
>>>>>> +	DRM_FORMAT_NV12,
>>>>>> +};
>>>>>> +
>>>>>>  static const uint64_t skl_plane_format_modifiers_noccs[] = {
>>>>>>  	I915_FORMAT_MOD_Yf_TILED,
>>>>>>  	I915_FORMAT_MOD_Y_TILED,
>>>>>> @@ -1277,6 +1290,12 @@ static bool skl_mod_supported(uint32_t
>>>>> format, uint64_t modifier)
>>>>>>  		if (modifier == I915_FORMAT_MOD_Yf_TILED)
>>>>>>  			return true;
>>>>>>  		/* fall through */
>>>>>> +	case DRM_FORMAT_NV12:
>>>>>> +		if (modifier == DRM_FORMAT_MOD_LINEAR ||
>>>>>> +		    modifier == I915_FORMAT_MOD_X_TILED ||
>>>>>> +		    modifier == I915_FORMAT_MOD_Y_TILED ||
>>>>>> +		    modifier == I915_FORMAT_MOD_Yf_TILED)
>>>>>> +			return true;
>>>>> On patch 5 and 6: this is a tad overkill, just put it below
>>>>> DRM_FORMAT_VYUY and let it fall through. It's not different from
>>>>> the other formats. :) Otherwise looks good, I'll wait until the
>>>>> changes from drm-misc- next are merged into drm-intel-next-queued
>>>>> then this series can be applied with the minor fixup.
>>>> Sure, thank you. I'll make the change. I did this to fix a review
>>>> comment - which said all modifiers aren’t Covered for NV12 - when we
>>> earlier had it under VYUV. Would that be still okay?
>>> Seems only CCS isn't covered, so putting it there should be fine. :)
>> Oh okay - sure thank you. Will make the change and float the series.
> Sorry to ask - under VYUV only Yf_TILED returns true. Don’t we need to
> add the other modifiers like MOD_LINEAR, MOD_X_TILED and MOD_Y_TILED?
>> Regards
>> Vidya

This is a C switch, unless there is a break it will continue, so it works as intended. :)

For example format UYVY if modifier == Yf_TILED_CCS, it will ignore all 
the lines with case and default, and continue to the end of the switch 
statement, or until it hits a 'break' so the below:


	case DRM_FORMAT_UYVY:
	case DRM_FORMAT_VYUY:
		if (modifier == I915_FORMAT_MOD_Yf_TILED)
			return true;
		/* fall through */
	case DRM_FORMAT_C8:
		if (modifier == DRM_FORMAT_MOD_LINEAR ||
		    modifier == I915_FORMAT_MOD_X_TILED ||
		    modifier == I915_FORMAT_MOD_Y_TILED)
			return true;
		/* fall through */
	default:
		return false;
}

for UYVY ends up being the same as:

if (format == DRM_FORMAT_UYVY) {
	if (modifier == I915_FORMAT_MOD_Yf_TILED)
		return true;
	if (modifier == DRM_FORMAT_MOD_LINEAR ||
	    modifier == I915_FORMAT_MOD_X_TILED ||
	    modifier == I915_FORMAT_MOD_Y_TILED)
		return true;
	return false;
}

Maarten Lankhorst May 7, 2018, 8:28 a.m. UTC | #7

Op 07-05-18 om 10:29 schreef Srinivas, Vidya:
>
>> -----Original Message-----
>> From: Maarten Lankhorst [mailto:[e-mail]]
>> Sent: Monday, May 7, 2018 1:55 PM
>> To: Srinivas, Vidya <[e-mail]>; intel-
>> [e-mail]
>> Subject: Re: [Intel-gfx] [PATCH v6 6/6] drm/i915: Add NV12 as supported
>> format for sprite plane
>>
>> Op 07-05-18 om 10:20 schreef Srinivas, Vidya:
>>>> -----Original Message-----
>>>> From: Srinivas, Vidya
>>>> Sent: Monday, May 7, 2018 1:46 PM
>>>> To: 'Maarten Lankhorst' <[e-mail]>; intel-
>>>> [e-mail]
>>>> Subject: RE: [Intel-gfx] [PATCH v6 6/6] drm/i915: Add NV12 as
>>>> supported format for sprite plane
>>>>
>>>>
>>>>
>>>>> -----Original Message-----
>>>>> From: Maarten Lankhorst [mailto:[e-mail]]
>>>>> Sent: Monday, May 7, 2018 1:44 PM
>>>>> To: Srinivas, Vidya <[e-mail]>; intel-
>>>>> [e-mail]
>>>>> Subject: Re: [Intel-gfx] [PATCH v6 6/6] drm/i915: Add NV12 as
>>>>> supported format for sprite plane
>>>>>
>>>>> Op 07-05-18 om 10:11 schreef Srinivas, Vidya:
>>>>>>> -----Original Message-----
>>>>>>> From: Maarten Lankhorst [mailto:[e-mail]]
>>>>>>> Sent: Monday, May 7, 2018 1:38 PM
>>>>>>> To: Srinivas, Vidya <[e-mail]>; intel-
>>>>>>> [e-mail]
>>>>>>> Subject: Re: [Intel-gfx] [PATCH v6 6/6] drm/i915: Add NV12 as
>>>>>>> supported format for sprite plane
>>>>>>>
>>>>>>> Op 06-05-18 om 19:44 schreef Vidya Srinivas:
>>>>>>>> From: Chandra Konduru <[e-mail]>
>>>>>>>>
>>>>>>>> This patch adds NV12 to list of supported formats for sprite plane.
>>>>>>>>
>>>>>>>> v2: Rebased (me)
>>>>>>>>
>>>>>>>> v3: Review comments by Ville addressed
>>>>>>>> - Removed skl_plane_formats_with_nv12 and added
>>>>>>>> NV12 case in existing skl_plane_formats
>>>>>>>> - Added the 10bpc RGB formats
>>>>>>>>
>>>>>>>> v4: Addressed review comments from Clinton A Taylor "Why are we
>>>>>>> adding
>>>>>>>> 10 bit RGB formats with the NV12 series patches?
>>>>>>>> Trying to set XR30 or AB30 results in error returned even though
>>>>>>>> the modes are advertised for the planes"
>>>>>>>> - Removed 10bit RGB formats added previously with NV12 series
>>>>>>>>
>>>>>>>> v5: Missed the Tested-by/Reviewed-by in the previous series
>>>>>>>> Adding the same to commit message in this version.
>>>>>>>> Addressed review comments from Clinton A Taylor "Why are we
>>>> adding
>>>>>>>> 10 bit RGB formats with the NV12 series patches?
>>>>>>>> Trying to set XR30 or AB30 results in error returned even though
>>>>>>>> the modes are advertised for the planes"
>>>>>>>> - Previous version has 10bit RGB format removed from VLV formats
>>>>>>>> by mistake. Fixing that in this version.
>>>>>>>> Removed 10bit RGB formats added previously with NV12 series for
>>>> SKL.
>>>>>>>> v6: Addressed review comments by Ville Restricting the NV12 to
>>>>>>>> BXT and PIPE A and B
>>>>>>>>
>>>>>>>> v7: Rebased (me)
>>>>>>>>
>>>>>>>> v8: Rebased (me)
>>>>>>>> Restricting NV12 changes to BXT and KBL Restricting NV12 changes
>>>>>>>> for plane 0 (overlay)
>>>>>>>>
>>>>>>>> v9: Rebased (me)
>>>>>>>>
>>>>>>>> v10: Addressed review comments from Maarten.
>>>>>>>> Adding NV12 to skl_plane_formats itself.
>>>>>>>>
>>>>>>>> v11: Addressed review comments from Shashank Sharma
>>>>>>>>
>>>>>>>> v12: Addressed review comments from Shashank Sharma Made the
>>>>>>> condition
>>>>>>>> in intel_sprite_plane_create simple and easy to read as suggested.
>>>>>>>>
>>>>>>>> v13: Adding reviewed by tag from Shashank Sharma Addressed
>> review
>>>>>>>> comments from Juha-Pekka Heikkila
>>>>>>>> "NV12 not to be supported by SKL"
>>>>>>>>
>>>>>>>> v14: Addressed review comments from Ville Added
>>>> skl_planar_formats
>>>>>>>> to include NV12 and a check skl_plane_has_planar in sprite create
>>>>>>>> Added
>>>>>>>> NV12 format to skl_mod_supported. These were review comments
>>>>> from
>>>>>>>> Kristian Høgsberg <[e-mail]>
>>>>>>>>
>>>>>>>> v15: Added reviewed by from Juha-Pekka Heikkila
>>>>>>>>
>>>>>>>> v16: Rebased the series
>>>>>>>>
>>>>>>>> v17: Added all tiling under mod supported for NV12 Credits to
>>>>>>>> Megha Aggarwal
>>>>>>>>
>>>>>>>> v18: Added RB by Maarten and Kristian
>>>>>>>>
>>>>>>>> Credits-to: Megha Aggarwal <[e-mail]>
>>>>>>>> Credits-to: Kristian Høgsberg <[e-mail]>
>>>>>>>> Reviewed-by: Kristian Høgsberg <[e-mail]>
>>>>>>>> Reviewed-by: Maarten Lankhorst
>>>> <[e-mail]>
>>>>>>>> Tested-by: Clinton Taylor <[e-mail]>
>>>>>>>> Reviewed-by: Juha-Pekka Heikkila <[e-mail]>
>>>>>>>> Reviewed-by: Shashank Sharma <[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: Vidya Srinivas <[e-mail]>
>>>>>>>> ---
>>>>>>>>  drivers/gpu/drm/i915/intel_sprite.c | 29
>>>>>>>> +++++++++++++++++++++++++++--
>>>>>>>>  1 file changed, 27 insertions(+), 2 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/gpu/drm/i915/intel_sprite.c
>>>>>>>> b/drivers/gpu/drm/i915/intel_sprite.c
>>>>>>>> index c73553a..cdcae9e 100644
>>>>>>>> --- a/drivers/gpu/drm/i915/intel_sprite.c
>>>>>>>> +++ b/drivers/gpu/drm/i915/intel_sprite.c
>>>>>>>> @@ -1180,6 +1180,19 @@ static uint32_t skl_plane_formats[] = {
>>>>>>>>  	DRM_FORMAT_VYUY,
>>>>>>>>  };
>>>>>>>>
>>>>>>>> +static uint32_t skl_planar_formats[] = {
>>>>>>>> +	DRM_FORMAT_RGB565,
>>>>>>>> +	DRM_FORMAT_ABGR8888,
>>>>>>>> +	DRM_FORMAT_ARGB8888,
>>>>>>>> +	DRM_FORMAT_XBGR8888,
>>>>>>>> +	DRM_FORMAT_XRGB8888,
>>>>>>>> +	DRM_FORMAT_YUYV,
>>>>>>>> +	DRM_FORMAT_YVYU,
>>>>>>>> +	DRM_FORMAT_UYVY,
>>>>>>>> +	DRM_FORMAT_VYUY,
>>>>>>>> +	DRM_FORMAT_NV12,
>>>>>>>> +};
>>>>>>>> +
>>>>>>>>  static const uint64_t skl_plane_format_modifiers_noccs[] = {
>>>>>>>>  	I915_FORMAT_MOD_Yf_TILED,
>>>>>>>>  	I915_FORMAT_MOD_Y_TILED,
>>>>>>>> @@ -1277,6 +1290,12 @@ static bool skl_mod_supported(uint32_t
>>>>>>> format, uint64_t modifier)
>>>>>>>>  		if (modifier == I915_FORMAT_MOD_Yf_TILED)
>>>>>>>>  			return true;
>>>>>>>>  		/* fall through */
>>>>>>>> +	case DRM_FORMAT_NV12:
>>>>>>>> +		if (modifier == DRM_FORMAT_MOD_LINEAR ||
>>>>>>>> +		    modifier == I915_FORMAT_MOD_X_TILED ||
>>>>>>>> +		    modifier == I915_FORMAT_MOD_Y_TILED ||
>>>>>>>> +		    modifier == I915_FORMAT_MOD_Yf_TILED)
>>>>>>>> +			return true;
>>>>>>> On patch 5 and 6: this is a tad overkill, just put it below
>>>>>>> DRM_FORMAT_VYUY and let it fall through. It's not different from
>>>>>>> the other formats. :) Otherwise looks good, I'll wait until the
>>>>>>> changes from drm-misc- next are merged into drm-intel-next-queued
>>>>>>> then this series can be applied with the minor fixup.
>>>>>> Sure, thank you. I'll make the change. I did this to fix a review
>>>>>> comment - which said all modifiers aren’t Covered for NV12 - when
>>>>>> we
>>>>> earlier had it under VYUV. Would that be still okay?
>>>>> Seems only CCS isn't covered, so putting it there should be fine. :)
>>>> Oh okay - sure thank you. Will make the change and float the series.
>>> Sorry to ask - under VYUV only Yf_TILED returns true. Don’t we need to
>>> add the other modifiers like MOD_LINEAR, MOD_X_TILED and
>> MOD_Y_TILED?
>>>> Regards
>>>> Vidya
>> This is a C switch, unless there is a break it will continue, so it works as
>> intended. :)
>>
>> For example format UYVY if modifier == Yf_TILED_CCS, it will ignore all the
>> lines with case and default, and continue to the end of the switch
>> statement, or until it hits a 'break' so the below:
> I am so sorry, I did not notice there was no break :( Got confused. Thank you.
Yeah, in general that's what the 'fall through' comment is for, to make it clear that the lack of break is intentional and not a bug. :)

Vidya Srinivas May 7, 2018, 8:29 a.m. UTC | #8

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

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

> Sent: Monday, May 7, 2018 1:55 PM

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

> [e-mail]

> Subject: Re: [Intel-gfx] [PATCH v6 6/6] drm/i915: Add NV12 as supported

> format for sprite plane

> 

> Op 07-05-18 om 10:20 schreef Srinivas, Vidya:

> >

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

> >> From: Srinivas, Vidya

> >> Sent: Monday, May 7, 2018 1:46 PM

> >> To: 'Maarten Lankhorst' <[e-mail]>; intel-

> >> [e-mail]

> >> Subject: RE: [Intel-gfx] [PATCH v6 6/6] drm/i915: Add NV12 as

> >> supported format for sprite plane

> >>

> >>

> >>

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

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

> >>> Sent: Monday, May 7, 2018 1:44 PM

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

> >>> [e-mail]

> >>> Subject: Re: [Intel-gfx] [PATCH v6 6/6] drm/i915: Add NV12 as

> >>> supported format for sprite plane

> >>>

> >>> Op 07-05-18 om 10:11 schreef Srinivas, Vidya:

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

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

> >>>>> Sent: Monday, May 7, 2018 1:38 PM

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

> >>>>> [e-mail]

> >>>>> Subject: Re: [Intel-gfx] [PATCH v6 6/6] drm/i915: Add NV12 as

> >>>>> supported format for sprite plane

> >>>>>

> >>>>> Op 06-05-18 om 19:44 schreef Vidya Srinivas:

> >>>>>> From: Chandra Konduru <[e-mail]>

> >>>>>>

> >>>>>> This patch adds NV12 to list of supported formats for sprite plane.

> >>>>>>

> >>>>>> v2: Rebased (me)

> >>>>>>

> >>>>>> v3: Review comments by Ville addressed

> >>>>>> - Removed skl_plane_formats_with_nv12 and added

> >>>>>> NV12 case in existing skl_plane_formats

> >>>>>> - Added the 10bpc RGB formats

> >>>>>>

> >>>>>> v4: Addressed review comments from Clinton A Taylor "Why are we

> >>>>> adding

> >>>>>> 10 bit RGB formats with the NV12 series patches?

> >>>>>> Trying to set XR30 or AB30 results in error returned even though

> >>>>>> the modes are advertised for the planes"

> >>>>>> - Removed 10bit RGB formats added previously with NV12 series

> >>>>>>

> >>>>>> v5: Missed the Tested-by/Reviewed-by in the previous series

> >>>>>> Adding the same to commit message in this version.

> >>>>>> Addressed review comments from Clinton A Taylor "Why are we

> >> adding

> >>>>>> 10 bit RGB formats with the NV12 series patches?

> >>>>>> Trying to set XR30 or AB30 results in error returned even though

> >>>>>> the modes are advertised for the planes"

> >>>>>> - Previous version has 10bit RGB format removed from VLV formats

> >>>>>> by mistake. Fixing that in this version.

> >>>>>> Removed 10bit RGB formats added previously with NV12 series for

> >> SKL.

> >>>>>> v6: Addressed review comments by Ville Restricting the NV12 to

> >>>>>> BXT and PIPE A and B

> >>>>>>

> >>>>>> v7: Rebased (me)

> >>>>>>

> >>>>>> v8: Rebased (me)

> >>>>>> Restricting NV12 changes to BXT and KBL Restricting NV12 changes

> >>>>>> for plane 0 (overlay)

> >>>>>>

> >>>>>> v9: Rebased (me)

> >>>>>>

> >>>>>> v10: Addressed review comments from Maarten.

> >>>>>> Adding NV12 to skl_plane_formats itself.

> >>>>>>

> >>>>>> v11: Addressed review comments from Shashank Sharma

> >>>>>>

> >>>>>> v12: Addressed review comments from Shashank Sharma Made the

> >>>>> condition

> >>>>>> in intel_sprite_plane_create simple and easy to read as suggested.

> >>>>>>

> >>>>>> v13: Adding reviewed by tag from Shashank Sharma Addressed

> review

> >>>>>> comments from Juha-Pekka Heikkila

> >>>>>> "NV12 not to be supported by SKL"

> >>>>>>

> >>>>>> v14: Addressed review comments from Ville Added

> >> skl_planar_formats

> >>>>>> to include NV12 and a check skl_plane_has_planar in sprite create

> >>>>>> Added

> >>>>>> NV12 format to skl_mod_supported. These were review comments

> >>> from

> >>>>>> Kristian Høgsberg <[e-mail]>

> >>>>>>

> >>>>>> v15: Added reviewed by from Juha-Pekka Heikkila

> >>>>>>

> >>>>>> v16: Rebased the series

> >>>>>>

> >>>>>> v17: Added all tiling under mod supported for NV12 Credits to

> >>>>>> Megha Aggarwal

> >>>>>>

> >>>>>> v18: Added RB by Maarten and Kristian

> >>>>>>

> >>>>>> Credits-to: Megha Aggarwal <[e-mail]>

> >>>>>> Credits-to: Kristian Høgsberg <[e-mail]>

> >>>>>> Reviewed-by: Kristian Høgsberg <[e-mail]>

> >>>>>> Reviewed-by: Maarten Lankhorst

> >> <[e-mail]>

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

> >>>>>> Reviewed-by: Juha-Pekka Heikkila <[e-mail]>

> >>>>>> Reviewed-by: Shashank Sharma <[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: Vidya Srinivas <[e-mail]>

> >>>>>> ---

> >>>>>>  drivers/gpu/drm/i915/intel_sprite.c | 29

> >>>>>> +++++++++++++++++++++++++++--

> >>>>>>  1 file changed, 27 insertions(+), 2 deletions(-)

> >>>>>>

> >>>>>> diff --git a/drivers/gpu/drm/i915/intel_sprite.c

> >>>>>> b/drivers/gpu/drm/i915/intel_sprite.c

> >>>>>> index c73553a..cdcae9e 100644

> >>>>>> --- a/drivers/gpu/drm/i915/intel_sprite.c

> >>>>>> +++ b/drivers/gpu/drm/i915/intel_sprite.c

> >>>>>> @@ -1180,6 +1180,19 @@ static uint32_t skl_plane_formats[] = {

> >>>>>>  	DRM_FORMAT_VYUY,

> >>>>>>  };

> >>>>>>

> >>>>>> +static uint32_t skl_planar_formats[] = {

> >>>>>> +	DRM_FORMAT_RGB565,

> >>>>>> +	DRM_FORMAT_ABGR8888,

> >>>>>> +	DRM_FORMAT_ARGB8888,

> >>>>>> +	DRM_FORMAT_XBGR8888,

> >>>>>> +	DRM_FORMAT_XRGB8888,

> >>>>>> +	DRM_FORMAT_YUYV,

> >>>>>> +	DRM_FORMAT_YVYU,

> >>>>>> +	DRM_FORMAT_UYVY,

> >>>>>> +	DRM_FORMAT_VYUY,

> >>>>>> +	DRM_FORMAT_NV12,

> >>>>>> +};

> >>>>>> +

> >>>>>>  static const uint64_t skl_plane_format_modifiers_noccs[] = {

> >>>>>>  	I915_FORMAT_MOD_Yf_TILED,

> >>>>>>  	I915_FORMAT_MOD_Y_TILED,

> >>>>>> @@ -1277,6 +1290,12 @@ static bool skl_mod_supported(uint32_t

> >>>>> format, uint64_t modifier)

> >>>>>>  		if (modifier == I915_FORMAT_MOD_Yf_TILED)

> >>>>>>  			return true;

> >>>>>>  		/* fall through */

> >>>>>> +	case DRM_FORMAT_NV12:

> >>>>>> +		if (modifier == DRM_FORMAT_MOD_LINEAR ||

> >>>>>> +		    modifier == I915_FORMAT_MOD_X_TILED ||

> >>>>>> +		    modifier == I915_FORMAT_MOD_Y_TILED ||

> >>>>>> +		    modifier == I915_FORMAT_MOD_Yf_TILED)

> >>>>>> +			return true;

> >>>>> On patch 5 and 6: this is a tad overkill, just put it below

> >>>>> DRM_FORMAT_VYUY and let it fall through. It's not different from

> >>>>> the other formats. :) Otherwise looks good, I'll wait until the

> >>>>> changes from drm-misc- next are merged into drm-intel-next-queued

> >>>>> then this series can be applied with the minor fixup.

> >>>> Sure, thank you. I'll make the change. I did this to fix a review

> >>>> comment - which said all modifiers aren’t Covered for NV12 - when

> >>>> we

> >>> earlier had it under VYUV. Would that be still okay?

> >>> Seems only CCS isn't covered, so putting it there should be fine. :)

> >> Oh okay - sure thank you. Will make the change and float the series.

> > Sorry to ask - under VYUV only Yf_TILED returns true. Don’t we need to

> > add the other modifiers like MOD_LINEAR, MOD_X_TILED and

> MOD_Y_TILED?

> >> Regards

> >> Vidya

> 

> This is a C switch, unless there is a break it will continue, so it works as

> intended. :)

> 

> For example format UYVY if modifier == Yf_TILED_CCS, it will ignore all the

> lines with case and default, and continue to the end of the switch

> statement, or until it hits a 'break' so the below:

I am so sorry, I did not notice there was no break :( Got confused. Thank you.

Regards
Vidya
> 

> 

> 	case DRM_FORMAT_UYVY:

> 	case DRM_FORMAT_VYUY:

> 		if (modifier == I915_FORMAT_MOD_Yf_TILED)

> 			return true;

> 		/* fall through */

> 	case DRM_FORMAT_C8:

> 		if (modifier == DRM_FORMAT_MOD_LINEAR ||

> 		    modifier == I915_FORMAT_MOD_X_TILED ||

> 		    modifier == I915_FORMAT_MOD_Y_TILED)

> 			return true;

> 		/* fall through */

> 	default:

> 		return false;

> }

> 

> for UYVY ends up being the same as:

> 

> if (format == DRM_FORMAT_UYVY) {

> 	if (modifier == I915_FORMAT_MOD_Yf_TILED)

> 		return true;

> 	if (modifier == DRM_FORMAT_MOD_LINEAR ||

> 	    modifier == I915_FORMAT_MOD_X_TILED ||

> 	    modifier == I915_FORMAT_MOD_Y_TILED)

> 		return true;

> 	return false;

> }

Vidya Srinivas May 7, 2018, 8:34 a.m. UTC | #9

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

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

> Sent: Monday, May 7, 2018 1:59 PM

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

> [e-mail]

> Subject: Re: [Intel-gfx] [PATCH v6 6/6] drm/i915: Add NV12 as supported

> format for sprite plane

> 

> Op 07-05-18 om 10:29 schreef Srinivas, Vidya:

> >

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

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

> >> Sent: Monday, May 7, 2018 1:55 PM

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

> >> [e-mail]

> >> Subject: Re: [Intel-gfx] [PATCH v6 6/6] drm/i915: Add NV12 as

> >> supported format for sprite plane

> >>

> >> Op 07-05-18 om 10:20 schreef Srinivas, Vidya:

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

> >>>> From: Srinivas, Vidya

> >>>> Sent: Monday, May 7, 2018 1:46 PM

> >>>> To: 'Maarten Lankhorst' <[e-mail]>; intel-

> >>>> [e-mail]

> >>>> Subject: RE: [Intel-gfx] [PATCH v6 6/6] drm/i915: Add NV12 as

> >>>> supported format for sprite plane

> >>>>

> >>>>

> >>>>

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

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

> >>>>> Sent: Monday, May 7, 2018 1:44 PM

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

> >>>>> [e-mail]

> >>>>> Subject: Re: [Intel-gfx] [PATCH v6 6/6] drm/i915: Add NV12 as

> >>>>> supported format for sprite plane

> >>>>>

> >>>>> Op 07-05-18 om 10:11 schreef Srinivas, Vidya:

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

> >>>>>>> From: Maarten Lankhorst

> >>>>>>> [mailto:[e-mail]]

> >>>>>>> Sent: Monday, May 7, 2018 1:38 PM

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

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

> >>>>>>> Subject: Re: [Intel-gfx] [PATCH v6 6/6] drm/i915: Add NV12 as

> >>>>>>> supported format for sprite plane

> >>>>>>>

> >>>>>>> Op 06-05-18 om 19:44 schreef Vidya Srinivas:

> >>>>>>>> From: Chandra Konduru <[e-mail]>

> >>>>>>>>

> >>>>>>>> This patch adds NV12 to list of supported formats for sprite plane.

> >>>>>>>>

> >>>>>>>> v2: Rebased (me)

> >>>>>>>>

> >>>>>>>> v3: Review comments by Ville addressed

> >>>>>>>> - Removed skl_plane_formats_with_nv12 and added

> >>>>>>>> NV12 case in existing skl_plane_formats

> >>>>>>>> - Added the 10bpc RGB formats

> >>>>>>>>

> >>>>>>>> v4: Addressed review comments from Clinton A Taylor "Why are

> we

> >>>>>>> adding

> >>>>>>>> 10 bit RGB formats with the NV12 series patches?

> >>>>>>>> Trying to set XR30 or AB30 results in error returned even

> >>>>>>>> though the modes are advertised for the planes"

> >>>>>>>> - Removed 10bit RGB formats added previously with NV12 series

> >>>>>>>>

> >>>>>>>> v5: Missed the Tested-by/Reviewed-by in the previous series

> >>>>>>>> Adding the same to commit message in this version.

> >>>>>>>> Addressed review comments from Clinton A Taylor "Why are we

> >>>> adding

> >>>>>>>> 10 bit RGB formats with the NV12 series patches?

> >>>>>>>> Trying to set XR30 or AB30 results in error returned even

> >>>>>>>> though the modes are advertised for the planes"

> >>>>>>>> - Previous version has 10bit RGB format removed from VLV

> >>>>>>>> formats by mistake. Fixing that in this version.

> >>>>>>>> Removed 10bit RGB formats added previously with NV12 series

> for

> >>>> SKL.

> >>>>>>>> v6: Addressed review comments by Ville Restricting the NV12 to

> >>>>>>>> BXT and PIPE A and B

> >>>>>>>>

> >>>>>>>> v7: Rebased (me)

> >>>>>>>>

> >>>>>>>> v8: Rebased (me)

> >>>>>>>> Restricting NV12 changes to BXT and KBL Restricting NV12

> >>>>>>>> changes for plane 0 (overlay)

> >>>>>>>>

> >>>>>>>> v9: Rebased (me)

> >>>>>>>>

> >>>>>>>> v10: Addressed review comments from Maarten.

> >>>>>>>> Adding NV12 to skl_plane_formats itself.

> >>>>>>>>

> >>>>>>>> v11: Addressed review comments from Shashank Sharma

> >>>>>>>>

> >>>>>>>> v12: Addressed review comments from Shashank Sharma Made

> the

> >>>>>>> condition

> >>>>>>>> in intel_sprite_plane_create simple and easy to read as

> suggested.

> >>>>>>>>

> >>>>>>>> v13: Adding reviewed by tag from Shashank Sharma Addressed

> >> review

> >>>>>>>> comments from Juha-Pekka Heikkila

> >>>>>>>> "NV12 not to be supported by SKL"

> >>>>>>>>

> >>>>>>>> v14: Addressed review comments from Ville Added

> >>>> skl_planar_formats

> >>>>>>>> to include NV12 and a check skl_plane_has_planar in sprite

> >>>>>>>> create Added

> >>>>>>>> NV12 format to skl_mod_supported. These were review

> comments

> >>>>> from

> >>>>>>>> Kristian Høgsberg <[e-mail]>

> >>>>>>>>

> >>>>>>>> v15: Added reviewed by from Juha-Pekka Heikkila

> >>>>>>>>

> >>>>>>>> v16: Rebased the series

> >>>>>>>>

> >>>>>>>> v17: Added all tiling under mod supported for NV12 Credits to

> >>>>>>>> Megha Aggarwal

> >>>>>>>>

> >>>>>>>> v18: Added RB by Maarten and Kristian

> >>>>>>>>

> >>>>>>>> Credits-to: Megha Aggarwal <[e-mail]>

> >>>>>>>> Credits-to: Kristian Høgsberg <[e-mail]>

> >>>>>>>> Reviewed-by: Kristian Høgsberg <[e-mail]>

> >>>>>>>> Reviewed-by: Maarten Lankhorst

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

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

> >>>>>>>> Reviewed-by: Juha-Pekka Heikkila

> <[e-mail]>

> >>>>>>>> Reviewed-by: Shashank Sharma <[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: Vidya Srinivas <[e-mail]>

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

> >>>>>>>>  drivers/gpu/drm/i915/intel_sprite.c | 29

> >>>>>>>> +++++++++++++++++++++++++++--

> >>>>>>>>  1 file changed, 27 insertions(+), 2 deletions(-)

> >>>>>>>>

> >>>>>>>> diff --git a/drivers/gpu/drm/i915/intel_sprite.c

> >>>>>>>> b/drivers/gpu/drm/i915/intel_sprite.c

> >>>>>>>> index c73553a..cdcae9e 100644

> >>>>>>>> --- a/drivers/gpu/drm/i915/intel_sprite.c

> >>>>>>>> +++ b/drivers/gpu/drm/i915/intel_sprite.c

> >>>>>>>> @@ -1180,6 +1180,19 @@ static uint32_t skl_plane_formats[] =

> {

> >>>>>>>>  	DRM_FORMAT_VYUY,

> >>>>>>>>  };

> >>>>>>>>

> >>>>>>>> +static uint32_t skl_planar_formats[] = {

> >>>>>>>> +	DRM_FORMAT_RGB565,

> >>>>>>>> +	DRM_FORMAT_ABGR8888,

> >>>>>>>> +	DRM_FORMAT_ARGB8888,

> >>>>>>>> +	DRM_FORMAT_XBGR8888,

> >>>>>>>> +	DRM_FORMAT_XRGB8888,

> >>>>>>>> +	DRM_FORMAT_YUYV,

> >>>>>>>> +	DRM_FORMAT_YVYU,

> >>>>>>>> +	DRM_FORMAT_UYVY,

> >>>>>>>> +	DRM_FORMAT_VYUY,

> >>>>>>>> +	DRM_FORMAT_NV12,

> >>>>>>>> +};

> >>>>>>>> +

> >>>>>>>>  static const uint64_t skl_plane_format_modifiers_noccs[] = {

> >>>>>>>>  	I915_FORMAT_MOD_Yf_TILED,

> >>>>>>>>  	I915_FORMAT_MOD_Y_TILED,

> >>>>>>>> @@ -1277,6 +1290,12 @@ static bool

> skl_mod_supported(uint32_t

> >>>>>>> format, uint64_t modifier)

> >>>>>>>>  		if (modifier == I915_FORMAT_MOD_Yf_TILED)

> >>>>>>>>  			return true;

> >>>>>>>>  		/* fall through */

> >>>>>>>> +	case DRM_FORMAT_NV12:

> >>>>>>>> +		if (modifier == DRM_FORMAT_MOD_LINEAR ||

> >>>>>>>> +		    modifier == I915_FORMAT_MOD_X_TILED ||

> >>>>>>>> +		    modifier == I915_FORMAT_MOD_Y_TILED ||

> >>>>>>>> +		    modifier == I915_FORMAT_MOD_Yf_TILED)

> >>>>>>>> +			return true;

> >>>>>>> On patch 5 and 6: this is a tad overkill, just put it below

> >>>>>>> DRM_FORMAT_VYUY and let it fall through. It's not different from

> >>>>>>> the other formats. :) Otherwise looks good, I'll wait until the

> >>>>>>> changes from drm-misc- next are merged into

> >>>>>>> drm-intel-next-queued then this series can be applied with the

> minor fixup.

> >>>>>> Sure, thank you. I'll make the change. I did this to fix a review

> >>>>>> comment - which said all modifiers aren’t Covered for NV12 - when

> >>>>>> we

> >>>>> earlier had it under VYUV. Would that be still okay?

> >>>>> Seems only CCS isn't covered, so putting it there should be fine.

> >>>>> :)

> >>>> Oh okay - sure thank you. Will make the change and float the series.

> >>> Sorry to ask - under VYUV only Yf_TILED returns true. Don’t we need

> >>> to add the other modifiers like MOD_LINEAR, MOD_X_TILED and

> >> MOD_Y_TILED?

> >>>> Regards

> >>>> Vidya

> >> This is a C switch, unless there is a break it will continue, so it

> >> works as intended. :)

> >>

> >> For example format UYVY if modifier == Yf_TILED_CCS, it will ignore

> >> all the lines with case and default, and continue to the end of the

> >> switch statement, or until it hits a 'break' so the below:

> > I am so sorry, I did not notice there was no break :( Got confused. Thank

> you.

> Yeah, in general that's what the 'fall through' comment is for, to make it

> clear that the lack of break is intentional and not a bug. :)

Oh okay, thank you so much. I did not know about that. I have made the change
and refloated the series. Kindly have a check. https://patchwork.freedesktop.org/series/41674/
Rev 5.

Regards
Vidya

Maarten Lankhorst May 7, 2018, 8:38 a.m. UTC | #10

Op 07-05-18 om 10:34 schreef Srinivas, Vidya:
>
>> -----Original Message-----
>> From: Maarten Lankhorst [mailto:[e-mail]]
>> Sent: Monday, May 7, 2018 1:59 PM
>> To: Srinivas, Vidya <[e-mail]>; intel-
>> [e-mail]
>> Subject: Re: [Intel-gfx] [PATCH v6 6/6] drm/i915: Add NV12 as supported
>> format for sprite plane
>>
>> Op 07-05-18 om 10:29 schreef Srinivas, Vidya:
>>>> -----Original Message-----
>>>> From: Maarten Lankhorst [mailto:[e-mail]]
>>>> Sent: Monday, May 7, 2018 1:55 PM
>>>> To: Srinivas, Vidya <[e-mail]>; intel-
>>>> [e-mail]
>>>> Subject: Re: [Intel-gfx] [PATCH v6 6/6] drm/i915: Add NV12 as
>>>> supported format for sprite plane
>>>>
>>>> Op 07-05-18 om 10:20 schreef Srinivas, Vidya:
>>>>>> -----Original Message-----
>>>>>> From: Srinivas, Vidya
>>>>>> Sent: Monday, May 7, 2018 1:46 PM
>>>>>> To: 'Maarten Lankhorst' <[e-mail]>; intel-
>>>>>> [e-mail]
>>>>>> Subject: RE: [Intel-gfx] [PATCH v6 6/6] drm/i915: Add NV12 as
>>>>>> supported format for sprite plane
>>>>>>
>>>>>>
>>>>>>
>>>>>>> -----Original Message-----
>>>>>>> From: Maarten Lankhorst [mailto:[e-mail]]
>>>>>>> Sent: Monday, May 7, 2018 1:44 PM
>>>>>>> To: Srinivas, Vidya <[e-mail]>; intel-
>>>>>>> [e-mail]
>>>>>>> Subject: Re: [Intel-gfx] [PATCH v6 6/6] drm/i915: Add NV12 as
>>>>>>> supported format for sprite plane
>>>>>>>
>>>>>>> Op 07-05-18 om 10:11 schreef Srinivas, Vidya:
>>>>>>>>> -----Original Message-----
>>>>>>>>> From: Maarten Lankhorst
>>>>>>>>> [mailto:[e-mail]]
>>>>>>>>> Sent: Monday, May 7, 2018 1:38 PM
>>>>>>>>> To: Srinivas, Vidya <[e-mail]>; intel-
>>>>>>>>> [e-mail]
>>>>>>>>> Subject: Re: [Intel-gfx] [PATCH v6 6/6] drm/i915: Add NV12 as
>>>>>>>>> supported format for sprite plane
>>>>>>>>>
>>>>>>>>> Op 06-05-18 om 19:44 schreef Vidya Srinivas:
>>>>>>>>>> From: Chandra Konduru <[e-mail]>
>>>>>>>>>>
>>>>>>>>>> This patch adds NV12 to list of supported formats for sprite plane.
>>>>>>>>>>
>>>>>>>>>> v2: Rebased (me)
>>>>>>>>>>
>>>>>>>>>> v3: Review comments by Ville addressed
>>>>>>>>>> - Removed skl_plane_formats_with_nv12 and added
>>>>>>>>>> NV12 case in existing skl_plane_formats
>>>>>>>>>> - Added the 10bpc RGB formats
>>>>>>>>>>
>>>>>>>>>> v4: Addressed review comments from Clinton A Taylor "Why are
>> we
>>>>>>>>> adding
>>>>>>>>>> 10 bit RGB formats with the NV12 series patches?
>>>>>>>>>> Trying to set XR30 or AB30 results in error returned even
>>>>>>>>>> though the modes are advertised for the planes"
>>>>>>>>>> - Removed 10bit RGB formats added previously with NV12 series
>>>>>>>>>>
>>>>>>>>>> v5: Missed the Tested-by/Reviewed-by in the previous series
>>>>>>>>>> Adding the same to commit message in this version.
>>>>>>>>>> Addressed review comments from Clinton A Taylor "Why are we
>>>>>> adding
>>>>>>>>>> 10 bit RGB formats with the NV12 series patches?
>>>>>>>>>> Trying to set XR30 or AB30 results in error returned even
>>>>>>>>>> though the modes are advertised for the planes"
>>>>>>>>>> - Previous version has 10bit RGB format removed from VLV
>>>>>>>>>> formats by mistake. Fixing that in this version.
>>>>>>>>>> Removed 10bit RGB formats added previously with NV12 series
>> for
>>>>>> SKL.
>>>>>>>>>> v6: Addressed review comments by Ville Restricting the NV12 to
>>>>>>>>>> BXT and PIPE A and B
>>>>>>>>>>
>>>>>>>>>> v7: Rebased (me)
>>>>>>>>>>
>>>>>>>>>> v8: Rebased (me)
>>>>>>>>>> Restricting NV12 changes to BXT and KBL Restricting NV12
>>>>>>>>>> changes for plane 0 (overlay)
>>>>>>>>>>
>>>>>>>>>> v9: Rebased (me)
>>>>>>>>>>
>>>>>>>>>> v10: Addressed review comments from Maarten.
>>>>>>>>>> Adding NV12 to skl_plane_formats itself.
>>>>>>>>>>
>>>>>>>>>> v11: Addressed review comments from Shashank Sharma
>>>>>>>>>>
>>>>>>>>>> v12: Addressed review comments from Shashank Sharma Made
>> the
>>>>>>>>> condition
>>>>>>>>>> in intel_sprite_plane_create simple and easy to read as
>> suggested.
>>>>>>>>>> v13: Adding reviewed by tag from Shashank Sharma Addressed
>>>> review
>>>>>>>>>> comments from Juha-Pekka Heikkila
>>>>>>>>>> "NV12 not to be supported by SKL"
>>>>>>>>>>
>>>>>>>>>> v14: Addressed review comments from Ville Added
>>>>>> skl_planar_formats
>>>>>>>>>> to include NV12 and a check skl_plane_has_planar in sprite
>>>>>>>>>> create Added
>>>>>>>>>> NV12 format to skl_mod_supported. These were review
>> comments
>>>>>>> from
>>>>>>>>>> Kristian Høgsberg <[e-mail]>
>>>>>>>>>>
>>>>>>>>>> v15: Added reviewed by from Juha-Pekka Heikkila
>>>>>>>>>>
>>>>>>>>>> v16: Rebased the series
>>>>>>>>>>
>>>>>>>>>> v17: Added all tiling under mod supported for NV12 Credits to
>>>>>>>>>> Megha Aggarwal
>>>>>>>>>>
>>>>>>>>>> v18: Added RB by Maarten and Kristian
>>>>>>>>>>
>>>>>>>>>> Credits-to: Megha Aggarwal <[e-mail]>
>>>>>>>>>> Credits-to: Kristian Høgsberg <[e-mail]>
>>>>>>>>>> Reviewed-by: Kristian Høgsberg <[e-mail]>
>>>>>>>>>> Reviewed-by: Maarten Lankhorst
>>>>>> <[e-mail]>
>>>>>>>>>> Tested-by: Clinton Taylor <[e-mail]>
>>>>>>>>>> Reviewed-by: Juha-Pekka Heikkila
>> <[e-mail]>
>>>>>>>>>> Reviewed-by: Shashank Sharma <[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: Vidya Srinivas <[e-mail]>
>>>>>>>>>> ---
>>>>>>>>>>  drivers/gpu/drm/i915/intel_sprite.c | 29
>>>>>>>>>> +++++++++++++++++++++++++++--
>>>>>>>>>>  1 file changed, 27 insertions(+), 2 deletions(-)
>>>>>>>>>>
>>>>>>>>>> diff --git a/drivers/gpu/drm/i915/intel_sprite.c
>>>>>>>>>> b/drivers/gpu/drm/i915/intel_sprite.c
>>>>>>>>>> index c73553a..cdcae9e 100644
>>>>>>>>>> --- a/drivers/gpu/drm/i915/intel_sprite.c
>>>>>>>>>> +++ b/drivers/gpu/drm/i915/intel_sprite.c
>>>>>>>>>> @@ -1180,6 +1180,19 @@ static uint32_t skl_plane_formats[] =
>> {
>>>>>>>>>>  	DRM_FORMAT_VYUY,
>>>>>>>>>>  };
>>>>>>>>>>
>>>>>>>>>> +static uint32_t skl_planar_formats[] = {
>>>>>>>>>> +	DRM_FORMAT_RGB565,
>>>>>>>>>> +	DRM_FORMAT_ABGR8888,
>>>>>>>>>> +	DRM_FORMAT_ARGB8888,
>>>>>>>>>> +	DRM_FORMAT_XBGR8888,
>>>>>>>>>> +	DRM_FORMAT_XRGB8888,
>>>>>>>>>> +	DRM_FORMAT_YUYV,
>>>>>>>>>> +	DRM_FORMAT_YVYU,
>>>>>>>>>> +	DRM_FORMAT_UYVY,
>>>>>>>>>> +	DRM_FORMAT_VYUY,
>>>>>>>>>> +	DRM_FORMAT_NV12,
>>>>>>>>>> +};
>>>>>>>>>> +
>>>>>>>>>>  static const uint64_t skl_plane_format_modifiers_noccs[] = {
>>>>>>>>>>  	I915_FORMAT_MOD_Yf_TILED,
>>>>>>>>>>  	I915_FORMAT_MOD_Y_TILED,
>>>>>>>>>> @@ -1277,6 +1290,12 @@ static bool
>> skl_mod_supported(uint32_t
>>>>>>>>> format, uint64_t modifier)
>>>>>>>>>>  		if (modifier == I915_FORMAT_MOD_Yf_TILED)
>>>>>>>>>>  			return true;
>>>>>>>>>>  		/* fall through */
>>>>>>>>>> +	case DRM_FORMAT_NV12:
>>>>>>>>>> +		if (modifier == DRM_FORMAT_MOD_LINEAR ||
>>>>>>>>>> +		    modifier == I915_FORMAT_MOD_X_TILED ||
>>>>>>>>>> +		    modifier == I915_FORMAT_MOD_Y_TILED ||
>>>>>>>>>> +		    modifier == I915_FORMAT_MOD_Yf_TILED)
>>>>>>>>>> +			return true;
>>>>>>>>> On patch 5 and 6: this is a tad overkill, just put it below
>>>>>>>>> DRM_FORMAT_VYUY and let it fall through. It's not different from
>>>>>>>>> the other formats. :) Otherwise looks good, I'll wait until the
>>>>>>>>> changes from drm-misc- next are merged into
>>>>>>>>> drm-intel-next-queued then this series can be applied with the
>> minor fixup.
>>>>>>>> Sure, thank you. I'll make the change. I did this to fix a review
>>>>>>>> comment - which said all modifiers aren’t Covered for NV12 - when
>>>>>>>> we
>>>>>>> earlier had it under VYUV. Would that be still okay?
>>>>>>> Seems only CCS isn't covered, so putting it there should be fine.
>>>>>>> :)
>>>>>> Oh okay - sure thank you. Will make the change and float the series.
>>>>> Sorry to ask - under VYUV only Yf_TILED returns true. Don’t we need
>>>>> to add the other modifiers like MOD_LINEAR, MOD_X_TILED and
>>>> MOD_Y_TILED?
>>>>>> Regards
>>>>>> Vidya
>>>> This is a C switch, unless there is a break it will continue, so it
>>>> works as intended. :)
>>>>
>>>> For example format UYVY if modifier == Yf_TILED_CCS, it will ignore
>>>> all the lines with case and default, and continue to the end of the
>>>> switch statement, or until it hits a 'break' so the below:
>>> I am so sorry, I did not notice there was no break :( Got confused. Thank
>> you.
>> Yeah, in general that's what the 'fall through' comment is for, to make it
>> clear that the lack of break is intentional and not a bug. :)
> Oh okay, thank you so much. I did not know about that. I have made the change
> and refloated the series. Kindly have a check. https://patchwork.freedesktop.org/series/41674/
> Rev 5.
>
> Regards
> Vidya

Ack, looking good.

Now we only have to wait for the drm-misc-next backmerge, then I will commit this.

~Maarten

Vidya Srinivas May 7, 2018, 8:41 a.m. UTC | #11

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

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

> Sent: Monday, May 7, 2018 2:08 PM

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

> [e-mail]

> Subject: Re: [Intel-gfx] [PATCH v6 6/6] drm/i915: Add NV12 as supported

> format for sprite plane

> 

> Op 07-05-18 om 10:34 schreef Srinivas, Vidya:

> >

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

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

> >> Sent: Monday, May 7, 2018 1:59 PM

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

> >> [e-mail]

> >> Subject: Re: [Intel-gfx] [PATCH v6 6/6] drm/i915: Add NV12 as

> >> supported format for sprite plane

> >>

> >> Op 07-05-18 om 10:29 schreef Srinivas, Vidya:

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

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

> >>>> Sent: Monday, May 7, 2018 1:55 PM

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

> >>>> [e-mail]

> >>>> Subject: Re: [Intel-gfx] [PATCH v6 6/6] drm/i915: Add NV12 as

> >>>> supported format for sprite plane

> >>>>

> >>>> Op 07-05-18 om 10:20 schreef Srinivas, Vidya:

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

> >>>>>> From: Srinivas, Vidya

> >>>>>> Sent: Monday, May 7, 2018 1:46 PM

> >>>>>> To: 'Maarten Lankhorst' <[e-mail]>;

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

> >>>>>> Subject: RE: [Intel-gfx] [PATCH v6 6/6] drm/i915: Add NV12 as

> >>>>>> supported format for sprite plane

> >>>>>>

> >>>>>>

> >>>>>>

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

> >>>>>>> From: Maarten Lankhorst

> >>>>>>> [mailto:[e-mail]]

> >>>>>>> Sent: Monday, May 7, 2018 1:44 PM

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

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

> >>>>>>> Subject: Re: [Intel-gfx] [PATCH v6 6/6] drm/i915: Add NV12 as

> >>>>>>> supported format for sprite plane

> >>>>>>>

> >>>>>>> Op 07-05-18 om 10:11 schreef Srinivas, Vidya:

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

> >>>>>>>>> From: Maarten Lankhorst

> >>>>>>>>> [mailto:[e-mail]]

> >>>>>>>>> Sent: Monday, May 7, 2018 1:38 PM

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

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

> >>>>>>>>> Subject: Re: [Intel-gfx] [PATCH v6 6/6] drm/i915: Add NV12 as

> >>>>>>>>> supported format for sprite plane

> >>>>>>>>>

> >>>>>>>>> Op 06-05-18 om 19:44 schreef Vidya Srinivas:

> >>>>>>>>>> From: Chandra Konduru <[e-mail]>

> >>>>>>>>>>

> >>>>>>>>>> This patch adds NV12 to list of supported formats for sprite

> plane.

> >>>>>>>>>>

> >>>>>>>>>> v2: Rebased (me)

> >>>>>>>>>>

> >>>>>>>>>> v3: Review comments by Ville addressed

> >>>>>>>>>> - Removed skl_plane_formats_with_nv12 and added

> >>>>>>>>>> NV12 case in existing skl_plane_formats

> >>>>>>>>>> - Added the 10bpc RGB formats

> >>>>>>>>>>

> >>>>>>>>>> v4: Addressed review comments from Clinton A Taylor "Why

> are

> >> we

> >>>>>>>>> adding

> >>>>>>>>>> 10 bit RGB formats with the NV12 series patches?

> >>>>>>>>>> Trying to set XR30 or AB30 results in error returned even

> >>>>>>>>>> though the modes are advertised for the planes"

> >>>>>>>>>> - Removed 10bit RGB formats added previously with NV12

> series

> >>>>>>>>>>

> >>>>>>>>>> v5: Missed the Tested-by/Reviewed-by in the previous series

> >>>>>>>>>> Adding the same to commit message in this version.

> >>>>>>>>>> Addressed review comments from Clinton A Taylor "Why are

> we

> >>>>>> adding

> >>>>>>>>>> 10 bit RGB formats with the NV12 series patches?

> >>>>>>>>>> Trying to set XR30 or AB30 results in error returned even

> >>>>>>>>>> though the modes are advertised for the planes"

> >>>>>>>>>> - Previous version has 10bit RGB format removed from VLV

> >>>>>>>>>> formats by mistake. Fixing that in this version.

> >>>>>>>>>> Removed 10bit RGB formats added previously with NV12 series

> >> for

> >>>>>> SKL.

> >>>>>>>>>> v6: Addressed review comments by Ville Restricting the NV12

> >>>>>>>>>> to BXT and PIPE A and B

> >>>>>>>>>>

> >>>>>>>>>> v7: Rebased (me)

> >>>>>>>>>>

> >>>>>>>>>> v8: Rebased (me)

> >>>>>>>>>> Restricting NV12 changes to BXT and KBL Restricting NV12

> >>>>>>>>>> changes for plane 0 (overlay)

> >>>>>>>>>>

> >>>>>>>>>> v9: Rebased (me)

> >>>>>>>>>>

> >>>>>>>>>> v10: Addressed review comments from Maarten.

> >>>>>>>>>> Adding NV12 to skl_plane_formats itself.

> >>>>>>>>>>

> >>>>>>>>>> v11: Addressed review comments from Shashank Sharma

> >>>>>>>>>>

> >>>>>>>>>> v12: Addressed review comments from Shashank Sharma Made

> >> the

> >>>>>>>>> condition

> >>>>>>>>>> in intel_sprite_plane_create simple and easy to read as

> >> suggested.

> >>>>>>>>>> v13: Adding reviewed by tag from Shashank Sharma Addressed

> >>>> review

> >>>>>>>>>> comments from Juha-Pekka Heikkila

> >>>>>>>>>> "NV12 not to be supported by SKL"

> >>>>>>>>>>

> >>>>>>>>>> v14: Addressed review comments from Ville Added

> >>>>>> skl_planar_formats

> >>>>>>>>>> to include NV12 and a check skl_plane_has_planar in sprite

> >>>>>>>>>> create Added

> >>>>>>>>>> NV12 format to skl_mod_supported. These were review

> >> comments

> >>>>>>> from

> >>>>>>>>>> Kristian Høgsberg <[e-mail]>

> >>>>>>>>>>

> >>>>>>>>>> v15: Added reviewed by from Juha-Pekka Heikkila

> >>>>>>>>>>

> >>>>>>>>>> v16: Rebased the series

> >>>>>>>>>>

> >>>>>>>>>> v17: Added all tiling under mod supported for NV12 Credits to

> >>>>>>>>>> Megha Aggarwal

> >>>>>>>>>>

> >>>>>>>>>> v18: Added RB by Maarten and Kristian

> >>>>>>>>>>

> >>>>>>>>>> Credits-to: Megha Aggarwal <[e-mail]>

> >>>>>>>>>> Credits-to: Kristian Høgsberg <[e-mail]>

> >>>>>>>>>> Reviewed-by: Kristian Høgsberg <[e-mail]>

> >>>>>>>>>> Reviewed-by: Maarten Lankhorst

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

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

> >>>>>>>>>> Reviewed-by: Juha-Pekka Heikkila

> >> <[e-mail]>

> >>>>>>>>>> Reviewed-by: Shashank Sharma <[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: Vidya Srinivas <[e-mail]>

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

> >>>>>>>>>>  drivers/gpu/drm/i915/intel_sprite.c | 29

> >>>>>>>>>> +++++++++++++++++++++++++++--

> >>>>>>>>>>  1 file changed, 27 insertions(+), 2 deletions(-)

> >>>>>>>>>>

> >>>>>>>>>> diff --git a/drivers/gpu/drm/i915/intel_sprite.c

> >>>>>>>>>> b/drivers/gpu/drm/i915/intel_sprite.c

> >>>>>>>>>> index c73553a..cdcae9e 100644

> >>>>>>>>>> --- a/drivers/gpu/drm/i915/intel_sprite.c

> >>>>>>>>>> +++ b/drivers/gpu/drm/i915/intel_sprite.c

> >>>>>>>>>> @@ -1180,6 +1180,19 @@ static uint32_t skl_plane_formats[]

> =

> >> {

> >>>>>>>>>>  	DRM_FORMAT_VYUY,

> >>>>>>>>>>  };

> >>>>>>>>>>

> >>>>>>>>>> +static uint32_t skl_planar_formats[] = {

> >>>>>>>>>> +	DRM_FORMAT_RGB565,

> >>>>>>>>>> +	DRM_FORMAT_ABGR8888,

> >>>>>>>>>> +	DRM_FORMAT_ARGB8888,

> >>>>>>>>>> +	DRM_FORMAT_XBGR8888,

> >>>>>>>>>> +	DRM_FORMAT_XRGB8888,

> >>>>>>>>>> +	DRM_FORMAT_YUYV,

> >>>>>>>>>> +	DRM_FORMAT_YVYU,

> >>>>>>>>>> +	DRM_FORMAT_UYVY,

> >>>>>>>>>> +	DRM_FORMAT_VYUY,

> >>>>>>>>>> +	DRM_FORMAT_NV12,

> >>>>>>>>>> +};

> >>>>>>>>>> +

> >>>>>>>>>>  static const uint64_t skl_plane_format_modifiers_noccs[] = {

> >>>>>>>>>>  	I915_FORMAT_MOD_Yf_TILED,

> >>>>>>>>>>  	I915_FORMAT_MOD_Y_TILED,

> >>>>>>>>>> @@ -1277,6 +1290,12 @@ static bool

> >> skl_mod_supported(uint32_t

> >>>>>>>>> format, uint64_t modifier)

> >>>>>>>>>>  		if (modifier == I915_FORMAT_MOD_Yf_TILED)

> >>>>>>>>>>  			return true;

> >>>>>>>>>>  		/* fall through */

> >>>>>>>>>> +	case DRM_FORMAT_NV12:

> >>>>>>>>>> +		if (modifier == DRM_FORMAT_MOD_LINEAR ||

> >>>>>>>>>> +		    modifier == I915_FORMAT_MOD_X_TILED ||

> >>>>>>>>>> +		    modifier == I915_FORMAT_MOD_Y_TILED ||

> >>>>>>>>>> +		    modifier == I915_FORMAT_MOD_Yf_TILED)

> >>>>>>>>>> +			return true;

> >>>>>>>>> On patch 5 and 6: this is a tad overkill, just put it below

> >>>>>>>>> DRM_FORMAT_VYUY and let it fall through. It's not different

> >>>>>>>>> from the other formats. :) Otherwise looks good, I'll wait

> >>>>>>>>> until the changes from drm-misc- next are merged into

> >>>>>>>>> drm-intel-next-queued then this series can be applied with the

> >> minor fixup.

> >>>>>>>> Sure, thank you. I'll make the change. I did this to fix a

> >>>>>>>> review comment - which said all modifiers aren’t Covered for

> >>>>>>>> NV12 - when we

> >>>>>>> earlier had it under VYUV. Would that be still okay?

> >>>>>>> Seems only CCS isn't covered, so putting it there should be fine.

> >>>>>>> :)

> >>>>>> Oh okay - sure thank you. Will make the change and float the series.

> >>>>> Sorry to ask - under VYUV only Yf_TILED returns true. Don’t we

> >>>>> need to add the other modifiers like MOD_LINEAR, MOD_X_TILED and

> >>>> MOD_Y_TILED?

> >>>>>> Regards

> >>>>>> Vidya

> >>>> This is a C switch, unless there is a break it will continue, so it

> >>>> works as intended. :)

> >>>>

> >>>> For example format UYVY if modifier == Yf_TILED_CCS, it will ignore

> >>>> all the lines with case and default, and continue to the end of the

> >>>> switch statement, or until it hits a 'break' so the below:

> >>> I am so sorry, I did not notice there was no break :( Got confused.

> >>> Thank

> >> you.

> >> Yeah, in general that's what the 'fall through' comment is for, to

> >> make it clear that the lack of break is intentional and not a bug. :)

> > Oh okay, thank you so much. I did not know about that. I have made the

> > change and refloated the series. Kindly have a check.

> > https://patchwork.freedesktop.org/series/41674/

> > Rev 5.

> >

> > Regards

> > Vidya

> 

> Ack, looking good.

> 

> Now we only have to wait for the drm-misc-next backmerge, then I will

> commit this.


:) Sure thank you :)

> 

> ~Maarten

Patch

diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index c73553a..cdcae9e 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -1180,6 +1180,19 @@  static uint32_t skl_plane_formats[] = {
 	DRM_FORMAT_VYUY,
 };
 
+static uint32_t skl_planar_formats[] = {
+	DRM_FORMAT_RGB565,
+	DRM_FORMAT_ABGR8888,
+	DRM_FORMAT_ARGB8888,
+	DRM_FORMAT_XBGR8888,
+	DRM_FORMAT_XRGB8888,
+	DRM_FORMAT_YUYV,
+	DRM_FORMAT_YVYU,
+	DRM_FORMAT_UYVY,
+	DRM_FORMAT_VYUY,
+	DRM_FORMAT_NV12,
+};
+
 static const uint64_t skl_plane_format_modifiers_noccs[] = {
 	I915_FORMAT_MOD_Yf_TILED,
 	I915_FORMAT_MOD_Y_TILED,
@@ -1277,6 +1290,12 @@  static bool skl_mod_supported(uint32_t format, uint64_t modifier)
 		if (modifier == I915_FORMAT_MOD_Yf_TILED)
 			return true;
 		/* fall through */
+	case DRM_FORMAT_NV12:
+		if (modifier == DRM_FORMAT_MOD_LINEAR ||
+		    modifier == I915_FORMAT_MOD_X_TILED ||
+		    modifier == I915_FORMAT_MOD_Y_TILED ||
+		    modifier == I915_FORMAT_MOD_Yf_TILED)
+			return true;
 	case DRM_FORMAT_C8:
 		if (modifier == DRM_FORMAT_MOD_LINEAR ||
 		    modifier == I915_FORMAT_MOD_X_TILED ||
@@ -1373,8 +1392,14 @@  intel_sprite_plane_create(struct drm_i915_private *dev_priv,
 		intel_plane->disable_plane = skl_disable_plane;
 		intel_plane->get_hw_state = skl_plane_get_hw_state;
 
-		plane_formats = skl_plane_formats;
-		num_plane_formats = ARRAY_SIZE(skl_plane_formats);
+		if (skl_plane_has_planar(dev_priv, pipe,
+					 PLANE_SPRITE0 + plane)) {
+			plane_formats = skl_planar_formats;
+			num_plane_formats = ARRAY_SIZE(skl_planar_formats);
+		} else {
+			plane_formats = skl_plane_formats;
+			num_plane_formats = ARRAY_SIZE(skl_plane_formats);
+		}
 
 		if (skl_plane_has_ccs(dev_priv, pipe, PLANE_SPRITE0 + plane))
 			modifiers = skl_plane_format_modifiers_ccs;