diff mbox

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

Message ID 1491583168-20042-4-git-send-email-shashank.sharma@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Sharma, Shashank April 7, 2017, 4:39 p.m. UTC
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(-)

Comments

Ville Syrjala May 8, 2017, 4:24 p.m. UTC | #1
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?
Sharma, Shashank May 8, 2017, 4:41 p.m. UTC | #2
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.

- Shashank
Ville Syrjala May 8, 2017, 5:09 p.m. UTC | #3
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.

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. 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).
Sharma, Shashank May 9, 2017, 8:34 a.m. UTC | #4
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
Ville Syrjala May 9, 2017, 3:28 p.m. UTC | #5
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@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 ?

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.

> 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.

> Even in the current implementation, I have been using only the 
> YCBCR420_MASK to identify support.
> 
> - Shashank
Sharma, Shashank May 10, 2017, 5:01 a.m. UTC | #6
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@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 ?
> 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
diff mbox

Patch

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 8d98687..64d8e2e 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -2777,6 +2777,7 @@  add_detailed_modes(struct drm_connector *connector, struct edid *edid,
 #define VENDOR_BLOCK    0x03
 #define SPEAKER_BLOCK	0x04
 #define VIDEO_CAPABILITY_BLOCK	0x07
+#define VIDEO_DATA_BLOCK_420	0x0E
 #define EDID_BASIC_AUDIO	(1 << 6)
 #define EDID_CEA_YCRCB444	(1 << 5)
 #define EDID_CEA_YCRCB422	(1 << 4)
@@ -3278,6 +3279,32 @@  static int add_3d_struct_modes(struct drm_connector *connector, u16 structure,
 	return modes;
 }
 
+/* Modes which can be supported in ycbcr 420 format only */
+static int do_420_vdb_modes(struct drm_connector *connector, const u8 *svds,
+		u8 svds_len)
+{
+	int modes = 0, i;
+	struct drm_device *dev = connector->dev;
+	struct drm_display_mode *newmode;
+	struct drm_display_info *info = &connector->display_info;
+
+	for (i = 0; i < svds_len; i++) {
+		u8 vic = svds[i] & 0x7f;
+
+		newmode = drm_mode_duplicate(dev, &edid_cea_modes[vic]);
+		if (!newmode)
+			break;
+
+		newmode->flags |= DRM_MODE_FLAG_420_ONLY;
+		drm_mode_probed_add(connector, newmode);
+		modes++;
+	}
+
+	if (modes > 0)
+		info->color_formats |= DRM_COLOR_FORMAT_YCRCB420;
+	return modes;
+}
+
 /*
  * do_hdmi_vsdb_modes - Parse the HDMI Vendor Specific data block
  * @connector: connector corresponding to the HDMI sink
@@ -3434,6 +3461,12 @@  cea_db_tag(const u8 *db)
 }
 
 static int
+cea_db_extended_tag(const u8 *db)
+{
+	return db[1];
+}
+
+static int
 cea_revision(const u8 *cea)
 {
 	return cea[1];
@@ -3482,6 +3515,17 @@  static bool cea_db_is_hdmi_forum_vsdb(const u8 *db)
 	return oui == HDMI_FORUM_IEEE_OUI;
 }
 
+static bool cea_db_is_hdmi_vdb420(const u8 *db)
+{
+	if (cea_db_tag(db) != VIDEO_CAPABILITY_BLOCK)
+		return false;
+
+	if (cea_db_extended_tag(db) != VIDEO_DATA_BLOCK_420)
+		return false;
+
+	return true;
+}
+
 #define for_each_cea_db(cea, i, start, end) \
 	for ((i) = (start); (i) < (end) && (i) + cea_db_payload_len(&(cea)[(i)]) < (end); (i) += cea_db_payload_len(&(cea)[(i)]) + 1)
 
@@ -3507,10 +3551,16 @@  add_cea_modes(struct drm_connector *connector, struct edid *edid)
 				video = db + 1;
 				video_len = dbl;
 				modes += do_cea_modes(connector, video, dbl);
-			}
-			else if (cea_db_is_hdmi_vsdb(db)) {
+			} else if (cea_db_is_hdmi_vsdb(db)) {
 				hdmi = db;
 				hdmi_len = dbl;
+			} else if (cea_db_is_hdmi_vdb420(db)) {
+				const u8 *vdb420 = &db[2];
+				u8 vdb420_len = dbl - 1;
+
+				/* Add 4:2:0(only) modes present in EDID */
+				modes += do_420_vdb_modes(connector, vdb420,
+							  vdb420_len);
 			}
 		}
 	}
diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index f2493b9..10432f3 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -987,6 +987,10 @@  bool drm_mode_equal_no_clocks(const struct drm_display_mode *mode1, const struct
 	    (mode2->flags & DRM_MODE_FLAG_3D_MASK))
 		return false;
 
+	if ((mode1->flags & DRM_MODE_FLAG_420_MASK) !=
+	    (mode2->flags & DRM_MODE_FLAG_420_MASK))
+		return false;
+
 	return drm_mode_equal_no_clocks_no_stereo(mode1, mode2);
 }
 EXPORT_SYMBOL(drm_mode_equal_no_clocks);
@@ -1005,6 +1009,9 @@  EXPORT_SYMBOL(drm_mode_equal_no_clocks);
 bool drm_mode_equal_no_clocks_no_stereo(const struct drm_display_mode *mode1,
 					const struct drm_display_mode *mode2)
 {
+	unsigned int flags_mask =
+		~(DRM_MODE_FLAG_3D_MASK | DRM_MODE_FLAG_420_MASK);
+
 	if (mode1->hdisplay == mode2->hdisplay &&
 	    mode1->hsync_start == mode2->hsync_start &&
 	    mode1->hsync_end == mode2->hsync_end &&
@@ -1015,8 +1022,7 @@  bool drm_mode_equal_no_clocks_no_stereo(const struct drm_display_mode *mode1,
 	    mode1->vsync_end == mode2->vsync_end &&
 	    mode1->vtotal == mode2->vtotal &&
 	    mode1->vscan == mode2->vscan &&
-	    (mode1->flags & ~DRM_MODE_FLAG_3D_MASK) ==
-	     (mode2->flags & ~DRM_MODE_FLAG_3D_MASK))
+	    (mode1->flags & flags_mask) == (mode2->flags & flags_mask))
 		return true;
 
 	return false;
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)
 
 /* Picture aspect ratio options */
 #define DRM_MODE_PICTURE_ASPECT_NONE		0