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

[Juha-Pekka Heikkila]

On 22.02.2018 04:39, Srinivas, Vidya wrote:
> 
> 
>> -----Original Message-----
>> From: Juha-Pekka Heikkila [mailto:juhapekka.heikkila at gmail.com]
>> Sent: Wednesday, February 21, 2018 7:52 PM
>> To: Srinivas, Vidya <vidya.srinivas at intel.com>; intel-
>> gfx at 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 at 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 at intel.com>
>>> Reviewed-by: Clinton Taylor <clinton.a.taylor at intel.com>
>>> Reviewed-by: Shashank Sharma <shashank.sharma at intel.com>
>>> Signed-off-by: Chandra Konduru <chandra.konduru at intel.com>
>>> Signed-off-by: Nabendu Maiti <nabendu.bikash.maiti at intel.com>
>>> Signed-off-by: Vidya Srinivas <vidya.srinivas at 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
>>>
>