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

From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
To: "Srinivas, Vidya" <vidya.srinivas@intel.com>,
	"intel-gfx@lists.freedesktop.org"
	<intel-gfx@lists.freedesktop.org>
Subject: Re: [PATCH v6 6/6] drm/i915: Add NV12 as supported format for sprite plane
Date: Mon, 7 May 2018 10:28:37 +0200	[thread overview]
Message-ID: <be16ec8e-fd48-480c-3fae-d3bb2d8b7be7@linux.intel.com> (raw)
In-Reply-To: <F653A0A18852B74D88578FA2EB7094EAB6881467@BGSMSX108.gar.corp.intel.com>

Op 07-05-18 om 10:29 schreef Srinivas, Vidya:
>
>> -----Original Message-----
>> From: Maarten Lankhorst [mailto:maarten.lankhorst@linux.intel.com]
>> Sent: Monday, May 7, 2018 1:55 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: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:
> 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. :)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx