diff mbox

[13/16] drm/i915: Add NV12 as supported format for primary plane

Message ID 1519208444-16665-14-git-send-email-vidya.srinivas@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Srinivas, Vidya Feb. 21, 2018, 10:20 a.m. UTC 
From: Chandra Konduru <chandra.konduru@intel.com>

This patch adds NV12 to list of supported formats for
primary plane

v2: Rebased (Chandra Konduru)

v3: Rebased (me)

v4: Review comments by Ville addressed
Removed the skl_primary_formats_with_nv12 and
added NV12 case in existing skl_primary_formats

v5: Rebased (me)

v6: Missed the Tested-by/Reviewed-by in the previous series
Adding the same to commit message in this version.

v7: Review comments by Ville addressed
	Restricting the NV12 for BXT and on PIPE A and B
Rebased (me)

v8: Rebased (me)
Modified restricting the NV12 support for both BXT and KBL.

v9: Rebased (me)

v10: Addressed review comments from Maarten.
	Adding NV12 inside skl_primary_formats itself.

v11: Adding Reviewed By tag from Shashank Sharma

Tested-by: Clinton Taylor <clinton.a.taylor@intel.com>
Reviewed-by: Clinton Taylor <clinton.a.taylor@intel.com>
Reviewed-by: Shashank Sharma <shashank.sharma@intel.com>
Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
Signed-off-by: Nabendu Maiti <nabendu.bikash.maiti@intel.com>
Signed-off-by: Vidya Srinivas <vidya.srinivas@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Juha-Pekka Heikkila Feb. 21, 2018, 2:22 p.m. UTC | #1 
On 21.02.2018 12:20, Vidya Srinivas wrote:
> From: Chandra Konduru <chandra.konduru@intel.com>
> 
> This patch adds NV12 to list of supported formats for
> primary plane
> 
> v2: Rebased (Chandra Konduru)
> 
> v3: Rebased (me)
> 
> v4: Review comments by Ville addressed
> Removed the skl_primary_formats_with_nv12 and
> added NV12 case in existing skl_primary_formats
> 
> v5: Rebased (me)
> 
> v6: Missed the Tested-by/Reviewed-by in the previous series
> Adding the same to commit message in this version.
> 
> v7: Review comments by Ville addressed
> 	Restricting the NV12 for BXT and on PIPE A and B
> Rebased (me)
> 
> v8: Rebased (me)
> Modified restricting the NV12 support for both BXT and KBL.
> 
> v9: Rebased (me)
> 
> v10: Addressed review comments from Maarten.
> 	Adding NV12 inside skl_primary_formats itself.
> 
> v11: Adding Reviewed By tag from Shashank Sharma
> 
> Tested-by: Clinton Taylor <clinton.a.taylor@intel.com>
> Reviewed-by: Clinton Taylor <clinton.a.taylor@intel.com>
> Reviewed-by: Shashank Sharma <shashank.sharma@intel.com>
> Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
> Signed-off-by: Nabendu Maiti <nabendu.bikash.maiti@intel.com>
> Signed-off-by: Vidya Srinivas <vidya.srinivas@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_display.c | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 142dfe0..1870366 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -86,6 +86,7 @@ static const uint32_t skl_primary_formats[] = {
>   	DRM_FORMAT_YVYU,
>   	DRM_FORMAT_UYVY,
>   	DRM_FORMAT_VYUY,
> +	DRM_FORMAT_NV12,
>   };
>   
>   static const uint64_t skl_format_modifiers_noccs[] = {
> @@ -13282,6 +13283,10 @@ intel_primary_plane_create(struct drm_i915_private *dev_priv, enum pipe pipe)
>   		intel_primary_formats = skl_primary_formats;
>   		num_formats = ARRAY_SIZE(skl_primary_formats);
>   
> +		if ((INTEL_GEN(dev_priv) == 9 && pipe == PIPE_C) &&
> +		    !IS_GEMINILAKE(dev_priv))
> +			num_formats -= 1;

This doesn't look future proof solution. This creates invisible 
dependency where it is required NV12 is last item in list of formats.

> +
>   		if (skl_plane_has_ccs(dev_priv, pipe, PLANE_PRIMARY))
>   			modifiers = skl_format_modifiers_ccs;
>   		else
>
Srinivas, Vidya Feb. 22, 2018, 2:39 a.m. UTC | #2 
> -----Original Message-----

> From: Juha-Pekka Heikkila [mailto:juhapekka.heikkila@gmail.com]

> Sent: Wednesday, February 21, 2018 7:52 PM

> To: Srinivas, Vidya <vidya.srinivas@intel.com>; intel-

> gfx@lists.freedesktop.org

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

> format for primary plane

> 

> On 21.02.2018 12:20, Vidya Srinivas wrote:

> > From: Chandra Konduru <chandra.konduru@intel.com>

> >

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

> >

> > v2: Rebased (Chandra Konduru)

> >

> > v3: Rebased (me)

> >

> > v4: Review comments by Ville addressed Removed the

> > skl_primary_formats_with_nv12 and added NV12 case in existing

> > skl_primary_formats

> >

> > v5: Rebased (me)

> >

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

> > same to commit message in this version.

> >

> > v7: Review comments by Ville addressed

> > 	Restricting the NV12 for BXT and on PIPE A and B Rebased (me)

> >

> > v8: Rebased (me)

> > Modified restricting the NV12 support for both BXT and KBL.

> >

> > v9: Rebased (me)

> >

> > v10: Addressed review comments from Maarten.

> > 	Adding NV12 inside skl_primary_formats itself.

> >

> > v11: Adding Reviewed By tag from Shashank Sharma

> >

> > Tested-by: Clinton Taylor <clinton.a.taylor@intel.com>

> > Reviewed-by: Clinton Taylor <clinton.a.taylor@intel.com>

> > Reviewed-by: Shashank Sharma <shashank.sharma@intel.com>

> > Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>

> > Signed-off-by: Nabendu Maiti <nabendu.bikash.maiti@intel.com>

> > Signed-off-by: Vidya Srinivas <vidya.srinivas@intel.com>

> > ---

> >   drivers/gpu/drm/i915/intel_display.c | 5 +++++

> >   1 file changed, 5 insertions(+)

> >

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

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

> > index 142dfe0..1870366 100644

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

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

> > @@ -86,6 +86,7 @@ static const uint32_t skl_primary_formats[] = {

> >   	DRM_FORMAT_YVYU,

> >   	DRM_FORMAT_UYVY,

> >   	DRM_FORMAT_VYUY,

> > +	DRM_FORMAT_NV12,

> >   };

> >

> >   static const uint64_t skl_format_modifiers_noccs[] = { @@ -13282,6

> > +13283,10 @@ intel_primary_plane_create(struct drm_i915_private

> *dev_priv, enum pipe pipe)

> >   		intel_primary_formats = skl_primary_formats;

> >   		num_formats = ARRAY_SIZE(skl_primary_formats);

> >

> > +		if ((INTEL_GEN(dev_priv) == 9 && pipe == PIPE_C) &&

> > +		    !IS_GEMINILAKE(dev_priv))

> > +			num_formats -= 1;

> 

> This doesn't look future proof solution. This creates invisible dependency

> where it is required NV12 is last item in list of formats.


Initially we had a different array for this. But as a part of one of the review
comments, I made this change.

Regards
Vidya

> 

> > +

> >   		if (skl_plane_has_ccs(dev_priv, pipe, PLANE_PRIMARY))

> >   			modifiers = skl_format_modifiers_ccs;

> >   		else

> >
Juha-Pekka Heikkila Feb. 22, 2018, 1:35 p.m. UTC | #3 
On 22.02.2018 04:39, Srinivas, Vidya wrote:
> 
> 
>> -----Original Message-----
>> From: Juha-Pekka Heikkila [mailto:juhapekka.heikkila@gmail.com]
>> Sent: Wednesday, February 21, 2018 7:52 PM
>> To: Srinivas, Vidya <vidya.srinivas@intel.com>; intel-
>> gfx@lists.freedesktop.org
>> Subject: Re: [Intel-gfx] [PATCH 13/16] drm/i915: Add NV12 as supported
>> format for primary plane
>>
>> On 21.02.2018 12:20, Vidya Srinivas wrote:
>>> From: Chandra Konduru <chandra.konduru@intel.com>
>>>
>>> This patch adds NV12 to list of supported formats for primary plane
>>>
>>> v2: Rebased (Chandra Konduru)
>>>
>>> v3: Rebased (me)
>>>
>>> v4: Review comments by Ville addressed Removed the
>>> skl_primary_formats_with_nv12 and added NV12 case in existing
>>> skl_primary_formats
>>>
>>> v5: Rebased (me)
>>>
>>> v6: Missed the Tested-by/Reviewed-by in the previous series Adding the
>>> same to commit message in this version.
>>>
>>> v7: Review comments by Ville addressed
>>> 	Restricting the NV12 for BXT and on PIPE A and B Rebased (me)
>>>
>>> v8: Rebased (me)
>>> Modified restricting the NV12 support for both BXT and KBL.
>>>
>>> v9: Rebased (me)
>>>
>>> v10: Addressed review comments from Maarten.
>>> 	Adding NV12 inside skl_primary_formats itself.
>>>
>>> v11: Adding Reviewed By tag from Shashank Sharma
>>>
>>> Tested-by: Clinton Taylor <clinton.a.taylor@intel.com>
>>> Reviewed-by: Clinton Taylor <clinton.a.taylor@intel.com>
>>> Reviewed-by: Shashank Sharma <shashank.sharma@intel.com>
>>> Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
>>> Signed-off-by: Nabendu Maiti <nabendu.bikash.maiti@intel.com>
>>> Signed-off-by: Vidya Srinivas <vidya.srinivas@intel.com>
>>> ---
>>>    drivers/gpu/drm/i915/intel_display.c | 5 +++++
>>>    1 file changed, 5 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_display.c
>>> b/drivers/gpu/drm/i915/intel_display.c
>>> index 142dfe0..1870366 100644
>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>> @@ -86,6 +86,7 @@ static const uint32_t skl_primary_formats[] = {
>>>    	DRM_FORMAT_YVYU,
>>>    	DRM_FORMAT_UYVY,
>>>    	DRM_FORMAT_VYUY,
>>> +	DRM_FORMAT_NV12,
>>>    };
>>>
>>>    static const uint64_t skl_format_modifiers_noccs[] = { @@ -13282,6
>>> +13283,10 @@ intel_primary_plane_create(struct drm_i915_private
>> *dev_priv, enum pipe pipe)
>>>    		intel_primary_formats = skl_primary_formats;
>>>    		num_formats = ARRAY_SIZE(skl_primary_formats);
>>>
>>> +		if ((INTEL_GEN(dev_priv) == 9 && pipe == PIPE_C) &&
>>> +		    !IS_GEMINILAKE(dev_priv))
>>> +			num_formats -= 1;
>>
>> This doesn't look future proof solution. This creates invisible dependency
>> where it is required NV12 is last item in list of formats.
> 
> Initially we had a different array for this. But as a part of one of the review
> comments, I made this change.

I did see Maarten's comment on your older patch. In my opinion having 
two lists would be more clear. Regardless of opinions on which is better 
you maybe anyway want to reconsider this piece of code as this expose 
NV12 also for Skylake platform.

> 
>>
>>> +
>>>    		if (skl_plane_has_ccs(dev_priv, pipe, PLANE_PRIMARY))
>>>    			modifiers = skl_format_modifiers_ccs;
>>>    		else
>>>
>
Srinivas, Vidya Feb. 23, 2018, 3:06 a.m. UTC | #4 
> -----Original Message-----

> From: Juha-Pekka Heikkila [mailto:juhapekka.heikkila@gmail.com]

> Sent: Thursday, February 22, 2018 7:06 PM

> To: Srinivas, Vidya <vidya.srinivas@intel.com>; intel-

> gfx@lists.freedesktop.org

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

> format for primary plane

> 

> On 22.02.2018 04:39, Srinivas, Vidya wrote:

> >

> >

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

> >> From: Juha-Pekka Heikkila [mailto:juhapekka.heikkila@gmail.com]

> >> Sent: Wednesday, February 21, 2018 7:52 PM

> >> To: Srinivas, Vidya <vidya.srinivas@intel.com>; intel-

> >> gfx@lists.freedesktop.org

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

> >> supported format for primary plane

> >>

> >> On 21.02.2018 12:20, Vidya Srinivas wrote:

> >>> From: Chandra Konduru <chandra.konduru@intel.com>

> >>>

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

> >>>

> >>> v2: Rebased (Chandra Konduru)

> >>>

> >>> v3: Rebased (me)

> >>>

> >>> v4: Review comments by Ville addressed Removed the

> >>> skl_primary_formats_with_nv12 and added NV12 case in existing

> >>> skl_primary_formats

> >>>

> >>> v5: Rebased (me)

> >>>

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

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

> >>>

> >>> v7: Review comments by Ville addressed

> >>> 	Restricting the NV12 for BXT and on PIPE A and B Rebased (me)

> >>>

> >>> v8: Rebased (me)

> >>> Modified restricting the NV12 support for both BXT and KBL.

> >>>

> >>> v9: Rebased (me)

> >>>

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

> >>> 	Adding NV12 inside skl_primary_formats itself.

> >>>

> >>> v11: Adding Reviewed By tag from Shashank Sharma

> >>>

> >>> Tested-by: Clinton Taylor <clinton.a.taylor@intel.com>

> >>> Reviewed-by: Clinton Taylor <clinton.a.taylor@intel.com>

> >>> Reviewed-by: Shashank Sharma <shashank.sharma@intel.com>

> >>> Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>

> >>> Signed-off-by: Nabendu Maiti <nabendu.bikash.maiti@intel.com>

> >>> Signed-off-by: Vidya Srinivas <vidya.srinivas@intel.com>

> >>> ---

> >>>    drivers/gpu/drm/i915/intel_display.c | 5 +++++

> >>>    1 file changed, 5 insertions(+)

> >>>

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

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

> >>> index 142dfe0..1870366 100644

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

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

> >>> @@ -86,6 +86,7 @@ static const uint32_t skl_primary_formats[] = {

> >>>    	DRM_FORMAT_YVYU,

> >>>    	DRM_FORMAT_UYVY,

> >>>    	DRM_FORMAT_VYUY,

> >>> +	DRM_FORMAT_NV12,

> >>>    };

> >>>

> >>>    static const uint64_t skl_format_modifiers_noccs[] = { @@

> >>> -13282,6

> >>> +13283,10 @@ intel_primary_plane_create(struct drm_i915_private

> >> *dev_priv, enum pipe pipe)

> >>>    		intel_primary_formats = skl_primary_formats;

> >>>    		num_formats = ARRAY_SIZE(skl_primary_formats);

> >>>

> >>> +		if ((INTEL_GEN(dev_priv) == 9 && pipe == PIPE_C) &&

> >>> +		    !IS_GEMINILAKE(dev_priv))

> >>> +			num_formats -= 1;

> >>

> >> This doesn't look future proof solution. This creates invisible

> >> dependency where it is required NV12 is last item in list of formats.

> >

> > Initially we had a different array for this. But as a part of one of

> > the review comments, I made this change.

> 

> I did see Maarten's comment on your older patch. In my opinion having two

> lists would be more clear. Regardless of opinions on which is better you

> maybe anyway want to reconsider this piece of code as this expose

> NV12 also for Skylake platform.


Maarten, could you please add your inputs as well?
I will change the patch according to what we decide.
Thank you.

Regards
Vidya
> 

> >

> >>

> >>> +

> >>>    		if (skl_plane_has_ccs(dev_priv, pipe, PLANE_PRIMARY))

> >>>    			modifiers = skl_format_modifiers_ccs;

> >>>    		else

> >>>

> >
Juha-Pekka Heikkila Feb. 23, 2018, 10:05 a.m. UTC | #5 
On 23.02.2018 05:06, Srinivas, Vidya wrote:
> 
> 
>> -----Original Message-----
>> From: Juha-Pekka Heikkila [mailto:juhapekka.heikkila@gmail.com]
>> Sent: Thursday, February 22, 2018 7:06 PM
>> To: Srinivas, Vidya <vidya.srinivas@intel.com>; intel-
>> gfx@lists.freedesktop.org
>> Subject: Re: [Intel-gfx] [PATCH 13/16] drm/i915: Add NV12 as supported
>> format for primary plane
>>
>> On 22.02.2018 04:39, Srinivas, Vidya wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: Juha-Pekka Heikkila [mailto:juhapekka.heikkila@gmail.com]
>>>> Sent: Wednesday, February 21, 2018 7:52 PM
>>>> To: Srinivas, Vidya <vidya.srinivas@intel.com>; intel-
>>>> gfx@lists.freedesktop.org
>>>> Subject: Re: [Intel-gfx] [PATCH 13/16] drm/i915: Add NV12 as
>>>> supported format for primary plane
>>>>
>>>> On 21.02.2018 12:20, Vidya Srinivas wrote:
>>>>> From: Chandra Konduru <chandra.konduru@intel.com>
>>>>>
>>>>> This patch adds NV12 to list of supported formats for primary plane
>>>>>
>>>>> v2: Rebased (Chandra Konduru)
>>>>>
>>>>> v3: Rebased (me)
>>>>>
>>>>> v4: Review comments by Ville addressed Removed the
>>>>> skl_primary_formats_with_nv12 and added NV12 case in existing
>>>>> skl_primary_formats
>>>>>
>>>>> v5: Rebased (me)
>>>>>
>>>>> v6: Missed the Tested-by/Reviewed-by in the previous series Adding
>>>>> the same to commit message in this version.
>>>>>
>>>>> v7: Review comments by Ville addressed
>>>>> 	Restricting the NV12 for BXT and on PIPE A and B Rebased (me)
>>>>>
>>>>> v8: Rebased (me)
>>>>> Modified restricting the NV12 support for both BXT and KBL.
>>>>>
>>>>> v9: Rebased (me)
>>>>>
>>>>> v10: Addressed review comments from Maarten.
>>>>> 	Adding NV12 inside skl_primary_formats itself.
>>>>>
>>>>> v11: Adding Reviewed By tag from Shashank Sharma
>>>>>
>>>>> Tested-by: Clinton Taylor <clinton.a.taylor@intel.com>
>>>>> Reviewed-by: Clinton Taylor <clinton.a.taylor@intel.com>
>>>>> Reviewed-by: Shashank Sharma <shashank.sharma@intel.com>
>>>>> Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
>>>>> Signed-off-by: Nabendu Maiti <nabendu.bikash.maiti@intel.com>
>>>>> Signed-off-by: Vidya Srinivas <vidya.srinivas@intel.com>
>>>>> ---
>>>>>     drivers/gpu/drm/i915/intel_display.c | 5 +++++
>>>>>     1 file changed, 5 insertions(+)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/i915/intel_display.c
>>>>> b/drivers/gpu/drm/i915/intel_display.c
>>>>> index 142dfe0..1870366 100644
>>>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>>>> @@ -86,6 +86,7 @@ static const uint32_t skl_primary_formats[] = {
>>>>>     	DRM_FORMAT_YVYU,
>>>>>     	DRM_FORMAT_UYVY,
>>>>>     	DRM_FORMAT_VYUY,
>>>>> +	DRM_FORMAT_NV12,
>>>>>     };
>>>>>
>>>>>     static const uint64_t skl_format_modifiers_noccs[] = { @@
>>>>> -13282,6
>>>>> +13283,10 @@ intel_primary_plane_create(struct drm_i915_private
>>>> *dev_priv, enum pipe pipe)
>>>>>     		intel_primary_formats = skl_primary_formats;
>>>>>     		num_formats = ARRAY_SIZE(skl_primary_formats);
>>>>>
>>>>> +		if ((INTEL_GEN(dev_priv) == 9 && pipe == PIPE_C) &&
>>>>> +		    !IS_GEMINILAKE(dev_priv))
>>>>> +			num_formats -= 1;
>>>>
>>>> This doesn't look future proof solution. This creates invisible
>>>> dependency where it is required NV12 is last item in list of formats.
>>>
>>> Initially we had a different array for this. But as a part of one of
>>> the review comments, I made this change.
>>
>> I did see Maarten's comment on your older patch. In my opinion having two
>> lists would be more clear. Regardless of opinions on which is better you
>> maybe anyway want to reconsider this piece of code as this expose
>> NV12 also for Skylake platform.
> 
> Maarten, could you please add your inputs as well?
> I will change the patch according to what we decide.
> Thank you.

If other's see it better to keep it in one list I'm ok for that. To me 
it just feel like accident waiting to happen if this list is changed at 
later time for different reasons.

My other comment about Skylake platform, you shouldn't enable NV12 for 
Skylake platform where its documented to have problems (I mean not all 
users of skl_* list but rule out probably something like 
IS_SKYLAKE(..)). For this reason this patch shouldn't be used as is. 
This same story applies for patch #14 of this series, NV12 shouldn't be 
enabled on Skylake platform.

>>>
>>>>
>>>>> +
>>>>>     		if (skl_plane_has_ccs(dev_priv, pipe, PLANE_PRIMARY))
>>>>>     			modifiers = skl_format_modifiers_ccs;
>>>>>     		else
>>>>>
>>>
>
Srinivas, Vidya Feb. 23, 2018, 10:08 a.m. UTC | #6 
> -----Original Message-----

> From: Juha-Pekka Heikkila [mailto:juhapekka.heikkila@gmail.com]

> Sent: Friday, February 23, 2018 3:35 PM

> To: Srinivas, Vidya <vidya.srinivas@intel.com>; intel-

> gfx@lists.freedesktop.org

> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

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

> format for primary plane

> 

> On 23.02.2018 05:06, Srinivas, Vidya wrote:

> >

> >

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

> >> From: Juha-Pekka Heikkila [mailto:juhapekka.heikkila@gmail.com]

> >> Sent: Thursday, February 22, 2018 7:06 PM

> >> To: Srinivas, Vidya <vidya.srinivas@intel.com>; intel-

> >> gfx@lists.freedesktop.org

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

> >> supported format for primary plane

> >>

> >> On 22.02.2018 04:39, Srinivas, Vidya wrote:

> >>>

> >>>

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

> >>>> From: Juha-Pekka Heikkila [mailto:juhapekka.heikkila@gmail.com]

> >>>> Sent: Wednesday, February 21, 2018 7:52 PM

> >>>> To: Srinivas, Vidya <vidya.srinivas@intel.com>; intel-

> >>>> gfx@lists.freedesktop.org

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

> >>>> supported format for primary plane

> >>>>

> >>>> On 21.02.2018 12:20, Vidya Srinivas wrote:

> >>>>> From: Chandra Konduru <chandra.konduru@intel.com>

> >>>>>

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

> >>>>> plane

> >>>>>

> >>>>> v2: Rebased (Chandra Konduru)

> >>>>>

> >>>>> v3: Rebased (me)

> >>>>>

> >>>>> v4: Review comments by Ville addressed Removed the

> >>>>> skl_primary_formats_with_nv12 and added NV12 case in existing

> >>>>> skl_primary_formats

> >>>>>

> >>>>> v5: Rebased (me)

> >>>>>

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

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

> >>>>>

> >>>>> v7: Review comments by Ville addressed

> >>>>> 	Restricting the NV12 for BXT and on PIPE A and B Rebased (me)

> >>>>>

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

> >>>>> Modified restricting the NV12 support for both BXT and KBL.

> >>>>>

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

> >>>>>

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

> >>>>> 	Adding NV12 inside skl_primary_formats itself.

> >>>>>

> >>>>> v11: Adding Reviewed By tag from Shashank Sharma

> >>>>>

> >>>>> Tested-by: Clinton Taylor <clinton.a.taylor@intel.com>

> >>>>> Reviewed-by: Clinton Taylor <clinton.a.taylor@intel.com>

> >>>>> Reviewed-by: Shashank Sharma <shashank.sharma@intel.com>

> >>>>> Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>

> >>>>> Signed-off-by: Nabendu Maiti <nabendu.bikash.maiti@intel.com>

> >>>>> Signed-off-by: Vidya Srinivas <vidya.srinivas@intel.com>

> >>>>> ---

> >>>>>     drivers/gpu/drm/i915/intel_display.c | 5 +++++

> >>>>>     1 file changed, 5 insertions(+)

> >>>>>

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

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

> >>>>> index 142dfe0..1870366 100644

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

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

> >>>>> @@ -86,6 +86,7 @@ static const uint32_t skl_primary_formats[] = {

> >>>>>     	DRM_FORMAT_YVYU,

> >>>>>     	DRM_FORMAT_UYVY,

> >>>>>     	DRM_FORMAT_VYUY,

> >>>>> +	DRM_FORMAT_NV12,

> >>>>>     };

> >>>>>

> >>>>>     static const uint64_t skl_format_modifiers_noccs[] = { @@

> >>>>> -13282,6

> >>>>> +13283,10 @@ intel_primary_plane_create(struct drm_i915_private

> >>>> *dev_priv, enum pipe pipe)

> >>>>>     		intel_primary_formats = skl_primary_formats;

> >>>>>     		num_formats = ARRAY_SIZE(skl_primary_formats);

> >>>>>

> >>>>> +		if ((INTEL_GEN(dev_priv) == 9 && pipe == PIPE_C)

> &&

> >>>>> +		    !IS_GEMINILAKE(dev_priv))

> >>>>> +			num_formats -= 1;

> >>>>

> >>>> This doesn't look future proof solution. This creates invisible

> >>>> dependency where it is required NV12 is last item in list of formats.

> >>>

> >>> Initially we had a different array for this. But as a part of one of

> >>> the review comments, I made this change.

> >>

> >> I did see Maarten's comment on your older patch. In my opinion having

> >> two lists would be more clear. Regardless of opinions on which is

> >> better you maybe anyway want to reconsider this piece of code as this

> >> expose

> >> NV12 also for Skylake platform.

> >

> > Maarten, could you please add your inputs as well?

> > I will change the patch according to what we decide.

> > Thank you.

> 

> If other's see it better to keep it in one list I'm ok for that. To me it just feel

> like accident waiting to happen if this list is changed at later time for different

> reasons.

> 

> My other comment about Skylake platform, you shouldn't enable NV12 for

> Skylake platform where its documented to have problems (I mean not all

> users of skl_* list but rule out probably something like IS_SKYLAKE(..)). For

> this reason this patch shouldn't be used as is.

> This same story applies for patch #14 of this series, NV12 shouldn't be

> enabled on Skylake platform.


Thank you. Sure, I will just wait for Maarten's input on the array.
For SKL, I will make the change in the patch and float.

Regards
Vidya

> 

> >>>

> >>>>

> >>>>> +

> >>>>>     		if (skl_plane_has_ccs(dev_priv, pipe,

> PLANE_PRIMARY))

> >>>>>     			modifiers = skl_format_modifiers_ccs;

> >>>>>     		else

> >>>>>

> >>>

> >
Ville Syrjälä March 5, 2018, 6:59 p.m. UTC | #7 
On Fri, Feb 23, 2018 at 10:08:25AM +0000, Srinivas, Vidya wrote:
> 
> 
> > -----Original Message-----
> > From: Juha-Pekka Heikkila [mailto:juhapekka.heikkila@gmail.com]
> > Sent: Friday, February 23, 2018 3:35 PM
> > To: Srinivas, Vidya <vidya.srinivas@intel.com>; intel-
> > gfx@lists.freedesktop.org
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Subject: Re: [Intel-gfx] [PATCH 13/16] drm/i915: Add NV12 as supported
> > format for primary plane
> > 
> > On 23.02.2018 05:06, Srinivas, Vidya wrote:
> > >
> > >
> > >> -----Original Message-----
> > >> From: Juha-Pekka Heikkila [mailto:juhapekka.heikkila@gmail.com]
> > >> Sent: Thursday, February 22, 2018 7:06 PM
> > >> To: Srinivas, Vidya <vidya.srinivas@intel.com>; intel-
> > >> gfx@lists.freedesktop.org
> > >> Subject: Re: [Intel-gfx] [PATCH 13/16] drm/i915: Add NV12 as
> > >> supported format for primary plane
> > >>
> > >> On 22.02.2018 04:39, Srinivas, Vidya wrote:
> > >>>
> > >>>
> > >>>> -----Original Message-----
> > >>>> From: Juha-Pekka Heikkila [mailto:juhapekka.heikkila@gmail.com]
> > >>>> Sent: Wednesday, February 21, 2018 7:52 PM
> > >>>> To: Srinivas, Vidya <vidya.srinivas@intel.com>; intel-
> > >>>> gfx@lists.freedesktop.org
> > >>>> Subject: Re: [Intel-gfx] [PATCH 13/16] drm/i915: Add NV12 as
> > >>>> supported format for primary plane
> > >>>>
> > >>>> On 21.02.2018 12:20, Vidya Srinivas wrote:
> > >>>>> From: Chandra Konduru <chandra.konduru@intel.com>
> > >>>>>
> > >>>>> This patch adds NV12 to list of supported formats for primary
> > >>>>> plane
> > >>>>>
> > >>>>> v2: Rebased (Chandra Konduru)
> > >>>>>
> > >>>>> v3: Rebased (me)
> > >>>>>
> > >>>>> v4: Review comments by Ville addressed Removed the
> > >>>>> skl_primary_formats_with_nv12 and added NV12 case in existing
> > >>>>> skl_primary_formats
> > >>>>>
> > >>>>> v5: Rebased (me)
> > >>>>>
> > >>>>> v6: Missed the Tested-by/Reviewed-by in the previous series Adding
> > >>>>> the same to commit message in this version.
> > >>>>>
> > >>>>> v7: Review comments by Ville addressed
> > >>>>> 	Restricting the NV12 for BXT and on PIPE A and B Rebased (me)
> > >>>>>
> > >>>>> v8: Rebased (me)
> > >>>>> Modified restricting the NV12 support for both BXT and KBL.
> > >>>>>
> > >>>>> v9: Rebased (me)
> > >>>>>
> > >>>>> v10: Addressed review comments from Maarten.
> > >>>>> 	Adding NV12 inside skl_primary_formats itself.
> > >>>>>
> > >>>>> v11: Adding Reviewed By tag from Shashank Sharma
> > >>>>>
> > >>>>> Tested-by: Clinton Taylor <clinton.a.taylor@intel.com>
> > >>>>> Reviewed-by: Clinton Taylor <clinton.a.taylor@intel.com>
> > >>>>> Reviewed-by: Shashank Sharma <shashank.sharma@intel.com>
> > >>>>> Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
> > >>>>> Signed-off-by: Nabendu Maiti <nabendu.bikash.maiti@intel.com>
> > >>>>> Signed-off-by: Vidya Srinivas <vidya.srinivas@intel.com>
> > >>>>> ---
> > >>>>>     drivers/gpu/drm/i915/intel_display.c | 5 +++++
> > >>>>>     1 file changed, 5 insertions(+)
> > >>>>>
> > >>>>> diff --git a/drivers/gpu/drm/i915/intel_display.c
> > >>>>> b/drivers/gpu/drm/i915/intel_display.c
> > >>>>> index 142dfe0..1870366 100644
> > >>>>> --- a/drivers/gpu/drm/i915/intel_display.c
> > >>>>> +++ b/drivers/gpu/drm/i915/intel_display.c
> > >>>>> @@ -86,6 +86,7 @@ static const uint32_t skl_primary_formats[] = {
> > >>>>>     	DRM_FORMAT_YVYU,
> > >>>>>     	DRM_FORMAT_UYVY,
> > >>>>>     	DRM_FORMAT_VYUY,
> > >>>>> +	DRM_FORMAT_NV12,
> > >>>>>     };
> > >>>>>
> > >>>>>     static const uint64_t skl_format_modifiers_noccs[] = { @@
> > >>>>> -13282,6
> > >>>>> +13283,10 @@ intel_primary_plane_create(struct drm_i915_private
> > >>>> *dev_priv, enum pipe pipe)
> > >>>>>     		intel_primary_formats = skl_primary_formats;
> > >>>>>     		num_formats = ARRAY_SIZE(skl_primary_formats);
> > >>>>>
> > >>>>> +		if ((INTEL_GEN(dev_priv) == 9 && pipe == PIPE_C)
> > &&
> > >>>>> +		    !IS_GEMINILAKE(dev_priv))
> > >>>>> +			num_formats -= 1;
> > >>>>
> > >>>> This doesn't look future proof solution. This creates invisible
> > >>>> dependency where it is required NV12 is last item in list of formats.
> > >>>
> > >>> Initially we had a different array for this. But as a part of one of
> > >>> the review comments, I made this change.
> > >>
> > >> I did see Maarten's comment on your older patch. In my opinion having
> > >> two lists would be more clear. Regardless of opinions on which is
> > >> better you maybe anyway want to reconsider this piece of code as this
> > >> expose
> > >> NV12 also for Skylake platform.
> > >
> > > Maarten, could you please add your inputs as well?
> > > I will change the patch according to what we decide.
> > > Thank you.
> > 
> > If other's see it better to keep it in one list I'm ok for that. To me it just feel
> > like accident waiting to happen if this list is changed at later time for different
> > reasons.
> > 
> > My other comment about Skylake platform, you shouldn't enable NV12 for
> > Skylake platform where its documented to have problems (I mean not all
> > users of skl_* list but rule out probably something like IS_SKYLAKE(..)). For
> > this reason this patch shouldn't be used as is.
> > This same story applies for patch #14 of this series, NV12 shouldn't be
> > enabled on Skylake platform.
> 
> Thank you. Sure, I will just wait for Maarten's input on the array.
> For SKL, I will make the change in the patch and float.

Pls add skl_plane_has_nv12() or something along those lines (to match
the skl_plane_has_ccs()).

Also https://patchwork.freedesktop.org/series/39390/ should make life a
bit easier for anyone dealing with SKL plane init code by removing this
silly code duplication we have going on currently.
Srinivas, Vidya March 7, 2018, 11:27 a.m. UTC | #8 
> -----Original Message-----
> From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> Sent: Tuesday, March 6, 2018 12:30 AM
> To: Srinivas, Vidya <vidya.srinivas@intel.com>
> Cc: juhapekka.heikkila@gmail.com; intel-gfx@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH 13/16] drm/i915: Add NV12 as supported
> format for primary plane
> 
> On Fri, Feb 23, 2018 at 10:08:25AM +0000, Srinivas, Vidya wrote:
> >
> >
> > > -----Original Message-----
> > > From: Juha-Pekka Heikkila [mailto:juhapekka.heikkila@gmail.com]
> > > Sent: Friday, February 23, 2018 3:35 PM
> > > To: Srinivas, Vidya <vidya.srinivas@intel.com>; intel-
> > > gfx@lists.freedesktop.org
> > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > Subject: Re: [Intel-gfx] [PATCH 13/16] drm/i915: Add NV12 as
> > > supported format for primary plane
> > >
> > > On 23.02.2018 05:06, Srinivas, Vidya wrote:
> > > >
> > > >
> > > >> -----Original Message-----
> > > >> From: Juha-Pekka Heikkila [mailto:juhapekka.heikkila@gmail.com]
> > > >> Sent: Thursday, February 22, 2018 7:06 PM
> > > >> To: Srinivas, Vidya <vidya.srinivas@intel.com>; intel-
> > > >> gfx@lists.freedesktop.org
> > > >> Subject: Re: [Intel-gfx] [PATCH 13/16] drm/i915: Add NV12 as
> > > >> supported format for primary plane
> > > >>
> > > >> On 22.02.2018 04:39, Srinivas, Vidya wrote:
> > > >>>
> > > >>>
> > > >>>> -----Original Message-----
> > > >>>> From: Juha-Pekka Heikkila [mailto:juhapekka.heikkila@gmail.com]
> > > >>>> Sent: Wednesday, February 21, 2018 7:52 PM
> > > >>>> To: Srinivas, Vidya <vidya.srinivas@intel.com>; intel-
> > > >>>> gfx@lists.freedesktop.org
> > > >>>> Subject: Re: [Intel-gfx] [PATCH 13/16] drm/i915: Add NV12 as
> > > >>>> supported format for primary plane
> > > >>>>
> > > >>>> On 21.02.2018 12:20, Vidya Srinivas wrote:
> > > >>>>> From: Chandra Konduru <chandra.konduru@intel.com>
> > > >>>>>
> > > >>>>> This patch adds NV12 to list of supported formats for primary
> > > >>>>> plane
> > > >>>>>
> > > >>>>> v2: Rebased (Chandra Konduru)
> > > >>>>>
> > > >>>>> v3: Rebased (me)
> > > >>>>>
> > > >>>>> v4: Review comments by Ville addressed Removed the
> > > >>>>> skl_primary_formats_with_nv12 and added NV12 case in existing
> > > >>>>> skl_primary_formats
> > > >>>>>
> > > >>>>> v5: Rebased (me)
> > > >>>>>
> > > >>>>> v6: Missed the Tested-by/Reviewed-by in the previous series
> > > >>>>> Adding the same to commit message in this version.
> > > >>>>>
> > > >>>>> v7: Review comments by Ville addressed
> > > >>>>> 	Restricting the NV12 for BXT and on PIPE A and B Rebased
> (me)
> > > >>>>>
> > > >>>>> v8: Rebased (me)
> > > >>>>> Modified restricting the NV12 support for both BXT and KBL.
> > > >>>>>
> > > >>>>> v9: Rebased (me)
> > > >>>>>
> > > >>>>> v10: Addressed review comments from Maarten.
> > > >>>>> 	Adding NV12 inside skl_primary_formats itself.
> > > >>>>>
> > > >>>>> v11: Adding Reviewed By tag from Shashank Sharma
> > > >>>>>
> > > >>>>> Tested-by: Clinton Taylor <clinton.a.taylor@intel.com>
> > > >>>>> Reviewed-by: Clinton Taylor <clinton.a.taylor@intel.com>
> > > >>>>> Reviewed-by: Shashank Sharma <shashank.sharma@intel.com>
> > > >>>>> Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
> > > >>>>> Signed-off-by: Nabendu Maiti <nabendu.bikash.maiti@intel.com>
> > > >>>>> Signed-off-by: Vidya Srinivas <vidya.srinivas@intel.com>
> > > >>>>> ---
> > > >>>>>     drivers/gpu/drm/i915/intel_display.c | 5 +++++
> > > >>>>>     1 file changed, 5 insertions(+)
> > > >>>>>
> > > >>>>> diff --git a/drivers/gpu/drm/i915/intel_display.c
> > > >>>>> b/drivers/gpu/drm/i915/intel_display.c
> > > >>>>> index 142dfe0..1870366 100644
> > > >>>>> --- a/drivers/gpu/drm/i915/intel_display.c
> > > >>>>> +++ b/drivers/gpu/drm/i915/intel_display.c
> > > >>>>> @@ -86,6 +86,7 @@ static const uint32_t skl_primary_formats[]
> = {
> > > >>>>>     	DRM_FORMAT_YVYU,
> > > >>>>>     	DRM_FORMAT_UYVY,
> > > >>>>>     	DRM_FORMAT_VYUY,
> > > >>>>> +	DRM_FORMAT_NV12,
> > > >>>>>     };
> > > >>>>>
> > > >>>>>     static const uint64_t skl_format_modifiers_noccs[] = { @@
> > > >>>>> -13282,6
> > > >>>>> +13283,10 @@ intel_primary_plane_create(struct
> > > >>>>> +drm_i915_private
> > > >>>> *dev_priv, enum pipe pipe)
> > > >>>>>     		intel_primary_formats = skl_primary_formats;
> > > >>>>>     		num_formats = ARRAY_SIZE(skl_primary_formats);
> > > >>>>>
> > > >>>>> +		if ((INTEL_GEN(dev_priv) == 9 && pipe == PIPE_C)
> > > &&
> > > >>>>> +		    !IS_GEMINILAKE(dev_priv))
> > > >>>>> +			num_formats -= 1;
> > > >>>>
> > > >>>> This doesn't look future proof solution. This creates invisible
> > > >>>> dependency where it is required NV12 is last item in list of
> formats.
> > > >>>
> > > >>> Initially we had a different array for this. But as a part of
> > > >>> one of the review comments, I made this change.
> > > >>
> > > >> I did see Maarten's comment on your older patch. In my opinion
> > > >> having two lists would be more clear. Regardless of opinions on
> > > >> which is better you maybe anyway want to reconsider this piece of
> > > >> code as this expose
> > > >> NV12 also for Skylake platform.
> > > >
> > > > Maarten, could you please add your inputs as well?
> > > > I will change the patch according to what we decide.
> > > > Thank you.
> > >
> > > If other's see it better to keep it in one list I'm ok for that. To
> > > me it just feel like accident waiting to happen if this list is
> > > changed at later time for different reasons.
> > >
> > > My other comment about Skylake platform, you shouldn't enable NV12
> > > for Skylake platform where its documented to have problems (I mean
> > > not all users of skl_* list but rule out probably something like
> > > IS_SKYLAKE(..)). For this reason this patch shouldn't be used as is.
> > > This same story applies for patch #14 of this series, NV12 shouldn't
> > > be enabled on Skylake platform.
> >
> > Thank you. Sure, I will just wait for Maarten's input on the array.
> > For SKL, I will make the change in the patch and float.
> 
> Pls add skl_plane_has_nv12() or something along those lines (to match the
> skl_plane_has_ccs()).
> 
> Also https://patchwork.freedesktop.org/series/39390/ should make life a bit
> easier for anyone dealing with SKL plane init code by removing this silly code
> duplication we have going on currently.
> 

Thank you. Will make the changes and float the patches.

Regards
Vidya

> --
> Ville Syrjälä
> Intel OTC
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 142dfe0..1870366 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -86,6 +86,7 @@  static const uint32_t skl_primary_formats[] = {
 	DRM_FORMAT_YVYU,
 	DRM_FORMAT_UYVY,
 	DRM_FORMAT_VYUY,
+	DRM_FORMAT_NV12,
 };
 
 static const uint64_t skl_format_modifiers_noccs[] = {
@@ -13282,6 +13283,10 @@  intel_primary_plane_create(struct drm_i915_private *dev_priv, enum pipe pipe)
 		intel_primary_formats = skl_primary_formats;
 		num_formats = ARRAY_SIZE(skl_primary_formats);
 
+		if ((INTEL_GEN(dev_priv) == 9 && pipe == PIPE_C) &&
+		    !IS_GEMINILAKE(dev_priv))
+			num_formats -= 1;
+
 		if (skl_plane_has_ccs(dev_priv, pipe, PLANE_PRIMARY))
 			modifiers = skl_format_modifiers_ccs;
 		else