[Intel-gfx] [PATCH 03/11] drm: parse ycbcr 420 vdb block

Wed May 10 05:01:34 UTC 2017

Regards

Shashank


On 5/9/2017 8:58 PM, Ville Syrjälä wrote:
> On Tue, May 09, 2017 at 02:04:55PM +0530, Sharma, Shashank wrote:
>> Regards
>>
>> Shashank
>>
>>
>> On 5/8/2017 10:39 PM, Ville Syrjälä wrote:
>>> On Mon, May 08, 2017 at 10:11:53PM +0530, Sharma, Shashank wrote:
>>>> Regards
>>>>
>>>> Shashank
>>>>
>>>>
>>>> On 5/8/2017 9:54 PM, Ville Syrjälä wrote:
>>>>> On Fri, Apr 07, 2017 at 07:39:20PM +0300, Shashank Sharma wrote:
>>>>>> From: Jose Abreu <jose.abreu at synopsys.com>
>>>>>>
>>>>>> HDMI 2.0 spec adds support for ycbcr420 subsampled output.
>>>>>> CEA-861-F adds two new blocks in EDID, to provide information about
>>>>>> sink's support for ycbcr420 output.
>>>>>>
>>>>>> These new blocks are:
>>>>>> - ycbcr420 video data (vdb) block: video modes which can be supported
>>>>>>      only in ycbcr420 output mode.
>>>>>> - ycbcr420 video capability data (vcb) block: video modes which can be
>>>>>>      support in ycbcr420 output mode also (along with RGB, YCBCR 444/422 etc)
>>>>>>
>>>>>> This patch adds parsing and handling of ycbcr420-vdb in the DRM
>>>>>> layer.
>>>>>>
>>>>>> This patch is a modified version of Jose's RFC patch:
>>>>>> https://patchwork.kernel.org/patch/9492327/
>>>>>> so the authorship is maintained.
>>>>>>
>>>>>> Cc: Ville Syrjala <ville.syrjala at linux.intel.com>
>>>>>> Signed-off-by: Jose Abreu <joabreu at synopsys.com>
>>>>>> Signed-off-by: Shashank Sharma <shashank.sharma at intel.com>
>>>>>> ---
>>>>>>     drivers/gpu/drm/drm_edid.c  | 54 +++++++++++++++++++++++++++++++++++++++++++--
>>>>>>     drivers/gpu/drm/drm_modes.c | 10 +++++++--
>>>>>>     include/drm/drm_connector.h |  1 +
>>>>>>     include/uapi/drm/drm_mode.h |  6 +++++
>>>>>>     4 files changed, 67 insertions(+), 4 deletions(-)
>>>>> <snip>
>>>>>> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
>>>>>> index 4eeda12..cef76b2 100644
>>>>>> --- a/include/drm/drm_connector.h
>>>>>> +++ b/include/drm/drm_connector.h
>>>>>> @@ -199,6 +199,7 @@ struct drm_display_info {
>>>>>>     #define DRM_COLOR_FORMAT_RGB444		(1<<0)
>>>>>>     #define DRM_COLOR_FORMAT_YCRCB444	(1<<1)
>>>>>>     #define DRM_COLOR_FORMAT_YCRCB422	(1<<2)
>>>>>> +#define DRM_COLOR_FORMAT_YCRCB420	(1<<2)
>>>>>>     
>>>>>>     	/**
>>>>>>     	 * @color_formats: HDMI Color formats, selects between RGB and YCrCb
>>>>>> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
>>>>>> index 8c67fc0..1e74d8e 100644
>>>>>> --- a/include/uapi/drm/drm_mode.h
>>>>>> +++ b/include/uapi/drm/drm_mode.h
>>>>>> @@ -84,6 +84,12 @@ extern "C" {
>>>>>>     #define  DRM_MODE_FLAG_3D_L_DEPTH_GFX_GFX_DEPTH	(6<<14)
>>>>>>     #define  DRM_MODE_FLAG_3D_TOP_AND_BOTTOM	(7<<14)
>>>>>>     #define  DRM_MODE_FLAG_3D_SIDE_BY_SIDE_HALF	(8<<14)
>>>>>> +/*
>>>>>> + * HDMI 2.0
>>>>>> + */
>>>>>> +#define DRM_MODE_FLAG_420_MASK			(0x03<<23)
>>>>>> +#define  DRM_MODE_FLAG_420			(1<<23)
>>>>>> +#define  DRM_MODE_FLAG_420_ONLY			(1<<24)
>>>>> Adding those would again break the uabi. We can't add new mode flags
>>>>> without some kind of client cap.
>>>>> But I think we agreed that new user
>>>>> space visible mode flags aren't needed, and instad we can keep it all
>>>>> internal?
>>>> Yep you are right, we had decided to keep it internal, and this whole
>>>> patch series is implemented in such a way only, to control everything
>>>> through the HDMI output property itself.
>>>> But may be I slightly misunderstood that we shouldn't add the flags bits
>>>> all together, and I added this flag to differentiate between YCBCR420
>>>> and notmal modes.
>>>> Can you please suggest me on:
>>>> - how to differentiate a YCBCR420 mode with normal mode ? I still need
>>>> to add a flag, but not expose it into uapi layer.
>>> I guess we can just tack on a few new bools to the end of
>>> drm_display_mode. And then when we get the mode passed in by the user
>>> we'll have to check whether that mode matches any CEA mode and
>>> then look up the correct YCbCr 4:2:0 mode for it.
>> seems good to me, I can add a is_ycbcr420 flag, and we need not to
>> bother about converting it to drm_mode_modeinfo as we are keeping it
>> internal.
>>> Hmm. Actually, that probably means that it isn't sufficient to
>>> actually store this information on the modes we have on the connector's
>>> mode list, because that list has been filtered and so may not actually
>>> have all the modes that were declared in the EDID.
>> I dint get this point,  Why do you think its not sufficient ? Do we need
>> to care about the modes which are getting rejected from the driver ?
> Yes, that was my worry. In general I don't think connector->modes should
> ever be used for mode validation or state computation. Eg. if fbdev
> handled the previus hotplug connector->modes will have been filtered
> based on the size of the fb_helper framebuffer, and then a new master
> might just blindly come in and blindly set a new mode without doing a
> full connector probe.
Its very valid point when it comes to resolution, but do you think fbdev 
will reject based on the flags (YCBCR420_only/also) .
I mean if I add YCBCR420 info as bools (like in your previous 
suggestion), would that alter fbdev selection ?
Nevertheless, a YCBCR420 bitmap seems to be a good idea already, just as 
soon as we can map it clearly with the hdmi_output property.
>> I guess they cant be applied anyways.  Do you think we will miss some of
>> the YCBCR modes due to mode filtering ?
>>> So I'm thinking we
>>> should perhaps make the bitmap parsed from the Y402CMDB index the
>>> full CEA mode list. That we can just lookup the matching VIC for
>>> the user provided mode and check whether the bit for that VIC
>>> indicates 4:2:0 support. And maybe we can handle Y420VDB in exactly
>>> the same way (ie. just a second bitmap). That would have the additional
>>> nice feature that the maximum length of those bitmaps is well defined
>>> (at most 256 VICs).
>>>
>> We can do this, but do we really need 2 bitmaps ? A YCBCR420 support is
>> same whether its coming from VCB or VDB, we just need a ORing of these
>> supports.
> We'll need to know if the mode is one of the "4:2:0 only" modes so that
> we'll automagically pick 4:2:0 whenever the user asks for this mode.
> And we'll need to know whether the mode is one of the "4:2:0 too" modes
> when the user asks for 4:2:0 for explicitly. The latter case of course
> needs a new property for that purpose.
The current design is as per out last discussion, where the hdmi_output 
property decides what would be the
possible output. So if userspace sets hdmi_output to YCBCR420, and sends 
modeset, we will just check if we can
support this mode in YCBCR420 in intel_hdmi_compute_ycbcr_config, and 
decide to accept or reject the modeset.
IF you can please have a look at the 8th patch in I915 layer 
(https://patchwork.freedesktop.org/patch/149107/ ) it would be easier
for us to discus if my approach needs any more changes there too.

As of now (only in current design) one property is serving the purpose, 
probably I need to fine tune this also.
- Shashank
>> Even in the current implementation, I have been using only the
>> YCBCR420_MASK to identify support.
>>
>> - Shashank