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

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

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' <maarten.lankhorst@linux.intel.com>; intel-
>> gfx@lists.freedesktop.org
>> Subject: RE: [Intel-gfx] [PATCH v6 6/6] drm/i915: Add NV12 as supported
>> format for sprite plane
>>
>>
>>
>>> -----Original Message-----
>>> From: Maarten Lankhorst [mailto:maarten.lankhorst@linux.intel.com]
>>> Sent: Monday, May 7, 2018 1:44 PM
>>> To: Srinivas, Vidya <vidya.srinivas@intel.com>; intel-
>>> gfx@lists.freedesktop.org
>>> 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:maarten.lankhorst@linux.intel.com]
>>>>> Sent: Monday, May 7, 2018 1:38 PM
>>>>> To: Srinivas, Vidya <vidya.srinivas@intel.com>; intel-
>>>>> gfx@lists.freedesktop.org
>>>>> 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 <chandra.konduru@intel.com>
>>>>>>
>>>>>> 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 <hoegsberg@gmail.com>
>>>>>>
>>>>>> 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 <megha.aggarwal@intel.com>
>>>>>> Credits-to: Kristian Høgsberg <hoegsberg@gmail.com>
>>>>>> Reviewed-by: Kristian Høgsberg <hoegsberg@gmail.com>
>>>>>> Reviewed-by: Maarten Lankhorst
>> <maarten.lankhorst@linux.intel.com>
>>>>>> Tested-by: Clinton Taylor <clinton.a.taylor@intel.com>
>>>>>> Reviewed-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
>>>>>> Reviewed-by: Shashank Sharma <shashank.sharma@intel.com>
>>>>>> Reviewed-by: Clinton Taylor <clinton.a.taylor@intel.com>
>>>>>> Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
>>>>>> Signed-off-by: Nabendu Maiti <nabendu.bikash.maiti@intel.com>
>>>>>> Signed-off-by: Vidya Srinivas <vidya.srinivas@intel.com>
>>>>>> ---
>>>>>>  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;
}

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