Re: [PATCH 03/11] drm: parse ycbcr 420 vdb block

["Sharma, Shashank" <shashank.sharma@intel.com>]

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@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@linux.intel.com>
>>>> Signed-off-by: Jose Abreu <joabreu@synopsys.com>
>>>> Signed-off-by: Shashank Sharma <shashank.sharma@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 ?
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.
Even in the current implementation, I have been using only the 
YCBCR420_MASK to identify support.

- Shashank
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel