diff mbox

[RESEND-CI,v4,04/15] drm/edid: parse YCBCR 420 videomodes from EDID

Message ID 1498041253-16426-5-git-send-email-shashank.sharma@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Sharma, Shashank June 21, 2017, 10:34 a.m. UTC
HDMI 2.0 spec adds support for YCBCR420 sub-sampled output.
CEA-861-F adds two new blocks in EDID's CEA extension blocks,
to provide information about sink's YCBCR420 output capabilities.

These blocks are:

- YCBCR420vdb(YCBCR 420 video data block):
This block contains VICs of video modes, which can be sopported only
in YCBCR420 output mode (Not in RGB/YCBCR444/422. Its like a normal
SVD block, valid for YCBCR420 modes only.

- YCBCR420cmdb(YCBCR 420 capability map data block):
This block gives information about video modes which can support
YCBCR420 output mode also (along with RGB,YCBCR444/422 etc) This
block contains a bitmap index of normal svd videomodes, which can
support YCBCR420 output too.
So if bit 0 from first vcb byte is set, first video mode in the svd
list can support YCBCR420 output too. Bit 1 means second video mode
from svd list can support YCBCR420 output too, and so on.

This patch adds two bitmaps in display's hdmi_info structure, one each
for VCB and VDB modes. If the source is HDMI 2.0 capable, this patch
adds:
- VDB modes (YCBCR 420 only modes) in connector's mode list, also makes
  an entry in the vdb_bitmap per vic.
- VCB modes (YCBCR 420 also modes) only entry in the vcb_bitmap.

Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
Cc: Jose Abreu <joabreu@synopsys.com>
Cc: Emil Velikov <emil.l.velikov@gmail.com>

V2: Addressed
    Review comments from Emil:
    - Use 1ULL<<i instead of 1<<i to make sure the output is 64bit.
    - Use the suggested method for updating dbmap.
    - Add documentation for YCBCR420_vcb_map to fix kbuild warning.

    Review comments from Ville:
    - Do not expose the YCBCR420 flags in uabi layer, keep it internal.
    - Save a map of YCBCR420 modes for future reference.
    - Check db length before trying to parse extended tag.
    - Add a warning if there are > 64 modes in capability map block.
    - Use y420cmdb in function names and macros while dealing with vcb
      to be aligned with spec.
    - Move the display information parsing block ahead of mode parsing
      blocks.

V3: Addressed design/review comments from Ville
    - Do not add flags in video modes, else we have to expose them to user
    - There should not be a UABI change, and kernel should detect the
      choice of the output based on type of mode, and the bitmaps.
    - Use standard bitops from kernel bitmap header, instead of calculating
      bit positions manually.

V4: Addressed review comments from Ville:
    - s/ycbcr_420_vdb/y420vdb
    - s/ycbcr_420_vcb/y420cmdb
    - Be less verbose on description of do_y420vdb_modes
    - Move newmode variable in the loop scope.
    - Use svd_to_vic() to get a VIC, instead of 0x7f
    - Remove bitmap description for CMDB modes & VDB modes
    - Dont add connector->ycbcr_420_allowed check for cmdb modes
    - Remove 'len' variable, in is_y420cmdb function, which is used
      only once
    - Add length check in is_y420vdb function
    - Remove unnecessary if (!db) check in function parse_y420cmdb_bitmap
    - Do not add print about YCBCR 420 modes
    - Fix indentation in few places
    - Move ycbcr420_dc_modes in next patch, where its used
    - Add a separate patch for movement of drm_add_display_info()

Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
---
 drivers/gpu/drm/drm_edid.c  | 157 +++++++++++++++++++++++++++++++++++++++++++-
 include/drm/drm_connector.h |  20 ++++++
 2 files changed, 174 insertions(+), 3 deletions(-)

Comments

Ville Syrjälä June 27, 2017, 11:52 a.m. UTC | #1
On Wed, Jun 21, 2017 at 04:04:02PM +0530, Shashank Sharma wrote:
> HDMI 2.0 spec adds support for YCBCR420 sub-sampled output.
> CEA-861-F adds two new blocks in EDID's CEA extension blocks,
> to provide information about sink's YCBCR420 output capabilities.
> 
> These blocks are:
> 
> - YCBCR420vdb(YCBCR 420 video data block):
> This block contains VICs of video modes, which can be sopported only
> in YCBCR420 output mode (Not in RGB/YCBCR444/422. Its like a normal
> SVD block, valid for YCBCR420 modes only.
> 
> - YCBCR420cmdb(YCBCR 420 capability map data block):
> This block gives information about video modes which can support
> YCBCR420 output mode also (along with RGB,YCBCR444/422 etc) This
> block contains a bitmap index of normal svd videomodes, which can
> support YCBCR420 output too.
> So if bit 0 from first vcb byte is set, first video mode in the svd
> list can support YCBCR420 output too. Bit 1 means second video mode
> from svd list can support YCBCR420 output too, and so on.
> 
> This patch adds two bitmaps in display's hdmi_info structure, one each
> for VCB and VDB modes. If the source is HDMI 2.0 capable, this patch
> adds:
> - VDB modes (YCBCR 420 only modes) in connector's mode list, also makes
>   an entry in the vdb_bitmap per vic.
> - VCB modes (YCBCR 420 also modes) only entry in the vcb_bitmap.
> 
> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> Cc: Jose Abreu <joabreu@synopsys.com>
> Cc: Emil Velikov <emil.l.velikov@gmail.com>
> 
> V2: Addressed
>     Review comments from Emil:
>     - Use 1ULL<<i instead of 1<<i to make sure the output is 64bit.
>     - Use the suggested method for updating dbmap.
>     - Add documentation for YCBCR420_vcb_map to fix kbuild warning.
> 
>     Review comments from Ville:
>     - Do not expose the YCBCR420 flags in uabi layer, keep it internal.
>     - Save a map of YCBCR420 modes for future reference.
>     - Check db length before trying to parse extended tag.
>     - Add a warning if there are > 64 modes in capability map block.
>     - Use y420cmdb in function names and macros while dealing with vcb
>       to be aligned with spec.
>     - Move the display information parsing block ahead of mode parsing
>       blocks.
> 
> V3: Addressed design/review comments from Ville
>     - Do not add flags in video modes, else we have to expose them to user
>     - There should not be a UABI change, and kernel should detect the
>       choice of the output based on type of mode, and the bitmaps.
>     - Use standard bitops from kernel bitmap header, instead of calculating
>       bit positions manually.
> 
> V4: Addressed review comments from Ville:
>     - s/ycbcr_420_vdb/y420vdb
>     - s/ycbcr_420_vcb/y420cmdb
>     - Be less verbose on description of do_y420vdb_modes
>     - Move newmode variable in the loop scope.
>     - Use svd_to_vic() to get a VIC, instead of 0x7f
>     - Remove bitmap description for CMDB modes & VDB modes
>     - Dont add connector->ycbcr_420_allowed check for cmdb modes
>     - Remove 'len' variable, in is_y420cmdb function, which is used
>       only once
>     - Add length check in is_y420vdb function
>     - Remove unnecessary if (!db) check in function parse_y420cmdb_bitmap
>     - Do not add print about YCBCR 420 modes
>     - Fix indentation in few places
>     - Move ycbcr420_dc_modes in next patch, where its used
>     - Add a separate patch for movement of drm_add_display_info()
> 
> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> ---
>  drivers/gpu/drm/drm_edid.c  | 157 +++++++++++++++++++++++++++++++++++++++++++-
>  include/drm/drm_connector.h |  20 ++++++
>  2 files changed, 174 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index e2d1b30..8c7e73b 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -2781,7 +2781,9 @@ add_detailed_modes(struct drm_connector *connector, struct edid *edid,
>  #define VIDEO_BLOCK     0x02
>  #define VENDOR_BLOCK    0x03
>  #define SPEAKER_BLOCK	0x04
> -#define VIDEO_CAPABILITY_BLOCK	0x07
> +#define VIDEO_CAPABILITY_BLOCK 0x07
> +#define VIDEO_DATA_BLOCK_420	0x0E
> +#define VIDEO_CAP_BLOCK_Y420CMDB 0x0F
>  #define EDID_BASIC_AUDIO	(1 << 6)
>  #define EDID_CEA_YCRCB444	(1 << 5)
>  #define EDID_CEA_YCRCB422	(1 << 4)
> @@ -3153,15 +3155,79 @@ drm_display_mode_from_vic_index(struct drm_connector *connector,
>  	return newmode;
>  }
>  
> +/*
> + * do_y420vdb_modes - Parse YCBCR 420 only modes
> + * @connector: connector corresponding to the HDMI sink
> + * @svds: start of the data block of CEA YCBCR 420 VDB
> + * @len: length of the CEA YCBCR 420 VDB
> + *
> + * Parse the CEA-861-F YCBCR 420 Video Data Block (Y420VDB)
> + * which contains modes which can be supported in YCBCR 420
> + * output format only.
> + */
> +static int do_y420vdb_modes(struct drm_connector *connector,
> +				   const u8 *svds, u8 svds_len)
> +{
> +	int modes = 0, i;
> +	struct drm_device *dev = connector->dev;
> +	struct drm_display_info *info = &connector->display_info;
> +	struct drm_hdmi_info *hdmi = &info->hdmi;
> +
> +	for (i = 0; i < svds_len; i++) {
> +		u8 vic = svd_to_vic(svds[i]);
> +		struct drm_display_mode *newmode;
> +
> +		newmode = drm_mode_duplicate(dev, &edid_cea_modes[vic]);
> +		if (!newmode)
> +			break;
> +		bitmap_set(hdmi->y420_vdb_modes, vic, 1);
> +		drm_mode_probed_add(connector, newmode);
> +		modes++;
> +	}
> +
> +	if (modes > 0)
> +		info->color_formats |= DRM_COLOR_FORMAT_YCRCB420;
> +	return modes;
> +}
> +
> +/*
> + * drm_add_cmdb_modes - Add a YCBCR 420 mode into bitmap
> + * @connector: connector corresponding to the HDMI sink
> + * @vic: CEA vic for the video mode to be added in the map
> + *
> + * Makes an entry for a videomode in the YCBCR 420 bitmap
> + */
> +static void
> +drm_add_cmdb_modes(struct drm_connector *connector, u8 svd)
> +{
> +	u8 vic = svd_to_vic(svd);
> +	struct drm_hdmi_info *hdmi = &connector->display_info.hdmi;
> +
> +	bitmap_set(hdmi->y420_cmdb_modes, vic, 1);
> +}
> +
>  static int
>  do_cea_modes(struct drm_connector *connector, const u8 *db, u8 len)
>  {
>  	int i, modes = 0;
> +	struct drm_hdmi_info *hdmi = &connector->display_info.hdmi;
>  
>  	for (i = 0; i < len; i++) {
>  		struct drm_display_mode *mode;
>  		mode = drm_display_mode_from_vic_index(connector, db, len, i);
>  		if (mode) {
> +			/*
> +			 * YCBCR420 capability block contains a bitmap which
> +			 * gives the index of CEA modes from CEA VDB, which
> +			 * can support YCBCR 420 sampling output also (apart
> +			 * from RGB/YCBCR444 etc).
> +			 * For example, if the bit 0 in bitmap is set,
> +			 * first mode in VDB can support YCBCR420 output too.
> +			 * Add YCBCR420 modes only if sink is HDMI 2.0 capable.
> +			 */
> +			if (test_bit(i, &hdmi->y420_cmdb_map))
> +				drm_add_cmdb_modes(connector, db[i]);
> +
>  			drm_mode_probed_add(connector, mode);
>  			modes++;
>  		}
> @@ -3437,6 +3503,12 @@ do_hdmi_vsdb_modes(struct drm_connector *connector, const u8 *db, u8 len,
>  }
>  
>  static int
> +cea_db_extended_tag(const u8 *db)
> +{
> +	return db[1];
> +}
> +
> +static int
>  cea_db_payload_len(const u8 *db)
>  {
>  	return db[0] & 0x1f;
> @@ -3497,9 +3569,78 @@ static bool cea_db_is_hdmi_forum_vsdb(const u8 *db)
>  	return oui == HDMI_FORUM_IEEE_OUI;
>  }
>  
> +static bool cea_db_is_y420cmdb(const u8 *db)
> +{
> +
> +	if (cea_db_tag(db) != VIDEO_CAPABILITY_BLOCK)

Still would like to see that patch to clean up the current extended tag
usage...

> +		return false;
> +
> +	if (!cea_db_payload_len(db))
> +		return false;
> +
> +	if (cea_db_extended_tag(db) != VIDEO_CAP_BLOCK_Y420CMDB)
> +		return false;
> +
> +	return true;
> +}
> +
> +static bool cea_db_is_y420vdb(const u8 *db)
> +{
> +	if (cea_db_tag(db) != VIDEO_CAPABILITY_BLOCK)
> +		return false;
> +
> +	if (!cea_db_payload_len(db))
> +		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)
>  
> +static void drm_parse_y420cmdb_bitmap(struct drm_connector *connector,
> +				     const u8 *db)
> +{
> +	struct drm_display_info *info = &connector->display_info;
> +	struct drm_hdmi_info *hdmi = &info->hdmi;
> +	u8 map_len = cea_db_payload_len(db) - 1;
> +	u8 count;
> +	u64 map = 0;
> +
> +	if (map_len == 0) {
> +		/* All CEA modes support ycbcr420 sampling also.*/
> +		hdmi->y420_cmdb_map = U64_MAX;
> +		info->color_formats |= DRM_COLOR_FORMAT_YCRCB420;
> +		return;
> +	}
> +
> +	/*
> +	 * This map indicates which of the existing CEA block modes
> +	 * from VDB can support YCBCR420 output too. So if bit=0 is
> +	 * set, first mode from VDB can support YCBCR420 output too.
> +	 * We will parse and keep this map, before parsing VDB itself
> +	 * to avoid going through the same block again and again.
> +	 *
> +	 * Spec is not clear about max possible size of this block.
> +	 * Clamping max bitmap block size at 8 bytes. Every byte can
> +	 * address 8 CEA modes, in this way this map can address
> +	 * 8*8 = first 64 SVDs.
> +	 */
> +	if (WARN_ON_ONCE(map_len > 8))
> +		map_len = 8;
> +
> +	for (count = 0; count < map_len; count++)
> +		map |= (u64)db[2 + count] << (8 * count);
> +
> +	if (map)
> +		info->color_formats |= DRM_COLOR_FORMAT_YCRCB420;
> +
> +	hdmi->y420_cmdb_map = map;
> +}
> +
>  static int
>  add_cea_modes(struct drm_connector *connector, struct edid *edid)
>  {
> @@ -3522,10 +3663,17 @@ 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_y420vdb(db) &&
> +				   connector->ycbcr_420_allowed) {
> +				const u8 *vdb420 = &db[2];
> +
> +				/* Add 4:2:0(only) modes present in EDID */
> +				modes += do_y420vdb_modes(connector,
> +							  vdb420,
> +							  dbl - 1);
>  			}
>  		}
>  	}
> @@ -4206,6 +4354,8 @@ static void drm_parse_cea_ext(struct drm_connector *connector,
>  			drm_parse_hdmi_vsdb_video(connector, db);
>  		if (cea_db_is_hdmi_forum_vsdb(db))
>  			drm_parse_hdmi_forum_vsdb(connector, db);
> +		if (cea_db_is_y420cmdb(db))
> +			drm_parse_y420cmdb_bitmap(connector, db);
>  	}
>  }
>  
> @@ -4460,6 +4610,7 @@ int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid)
>  	num_modes += add_cea_modes(connector, edid);
>  	num_modes += add_alternate_cea_modes(connector, edid);
>  	num_modes += add_displayid_detailed_modes(connector, edid);
> +
>  	if (edid->features & DRM_EDID_FEATURE_DEFAULT_GTF)
>  		num_modes += add_inferred_modes(connector, edid);
>  
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 7493fd3..ce2212d 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -137,6 +137,25 @@ struct drm_scdc {
>  struct drm_hdmi_info {
>  	/** @scdc: sink's scdc support and capabilities */
>  	struct drm_scdc scdc;
> +
> +	/**
> +	 * @y420_vdb_modes: bitmap of modes which can support ycbcr420
> +	 * output only (not normal RGB/YCBCR444/422 outputs). There are total
> +	 * 107 VICs defined by CEA-861-F spec, so the size is 128 bits to map
> +	 * upto 128 VICs;
> +	 */
> +	unsigned long y420_vdb_modes[BITS_TO_LONGS(128)];
> +
> +	/**
> +	 * @y420_cmdb_modes: bitmap of modes which can support ycbcr420
> +	 * output also, along with normal HDMI outputs. There are total 107
> +	 * VICs defined by CEA-861-F spec, so the size is 128 bits to map upto
> +	 * 128 VICs;
> +	 */
> +	unsigned long y420_cmdb_modes[BITS_TO_LONGS(128)];
> +
> +	/** @y420_cmdb_map: bitmap of SVD index, to extraxt vcb modes */
> +	unsigned long y420_cmdb_map;

u64, otherwise it's busted on 32bit machines.

Mostl this looks pretty good. I think the main problem is the lack of
mode validation that would reject the 4:2:0 only modes if not supported
by the driver/device. I guess that's in a later patch? If so we need to
reorder the series a bit to avoid regeressions.

>  };
>  
>  /**
> @@ -200,6 +219,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<<3)
>  
>  	/**
>  	 * @color_formats: HDMI Color formats, selects between RGB and YCrCb
> -- 
> 2.7.4
Sharma, Shashank June 30, 2017, 5:17 a.m. UTC | #2
Regards

Shashank


On 6/27/2017 5:22 PM, Ville Syrjälä wrote:
> On Wed, Jun 21, 2017 at 04:04:02PM +0530, Shashank Sharma wrote:
>> HDMI 2.0 spec adds support for YCBCR420 sub-sampled output.
>> CEA-861-F adds two new blocks in EDID's CEA extension blocks,
>> to provide information about sink's YCBCR420 output capabilities.
>>
>> These blocks are:
>>
>> - YCBCR420vdb(YCBCR 420 video data block):
>> This block contains VICs of video modes, which can be sopported only
>> in YCBCR420 output mode (Not in RGB/YCBCR444/422. Its like a normal
>> SVD block, valid for YCBCR420 modes only.
>>
>> - YCBCR420cmdb(YCBCR 420 capability map data block):
>> This block gives information about video modes which can support
>> YCBCR420 output mode also (along with RGB,YCBCR444/422 etc) This
>> block contains a bitmap index of normal svd videomodes, which can
>> support YCBCR420 output too.
>> So if bit 0 from first vcb byte is set, first video mode in the svd
>> list can support YCBCR420 output too. Bit 1 means second video mode
>> from svd list can support YCBCR420 output too, and so on.
>>
>> This patch adds two bitmaps in display's hdmi_info structure, one each
>> for VCB and VDB modes. If the source is HDMI 2.0 capable, this patch
>> adds:
>> - VDB modes (YCBCR 420 only modes) in connector's mode list, also makes
>>    an entry in the vdb_bitmap per vic.
>> - VCB modes (YCBCR 420 also modes) only entry in the vcb_bitmap.
>>
>> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
>> Cc: Jose Abreu <joabreu@synopsys.com>
>> Cc: Emil Velikov <emil.l.velikov@gmail.com>
>>
>> V2: Addressed
>>      Review comments from Emil:
>>      - Use 1ULL<<i instead of 1<<i to make sure the output is 64bit.
>>      - Use the suggested method for updating dbmap.
>>      - Add documentation for YCBCR420_vcb_map to fix kbuild warning.
>>
>>      Review comments from Ville:
>>      - Do not expose the YCBCR420 flags in uabi layer, keep it internal.
>>      - Save a map of YCBCR420 modes for future reference.
>>      - Check db length before trying to parse extended tag.
>>      - Add a warning if there are > 64 modes in capability map block.
>>      - Use y420cmdb in function names and macros while dealing with vcb
>>        to be aligned with spec.
>>      - Move the display information parsing block ahead of mode parsing
>>        blocks.
>>
>> V3: Addressed design/review comments from Ville
>>      - Do not add flags in video modes, else we have to expose them to user
>>      - There should not be a UABI change, and kernel should detect the
>>        choice of the output based on type of mode, and the bitmaps.
>>      - Use standard bitops from kernel bitmap header, instead of calculating
>>        bit positions manually.
>>
>> V4: Addressed review comments from Ville:
>>      - s/ycbcr_420_vdb/y420vdb
>>      - s/ycbcr_420_vcb/y420cmdb
>>      - Be less verbose on description of do_y420vdb_modes
>>      - Move newmode variable in the loop scope.
>>      - Use svd_to_vic() to get a VIC, instead of 0x7f
>>      - Remove bitmap description for CMDB modes & VDB modes
>>      - Dont add connector->ycbcr_420_allowed check for cmdb modes
>>      - Remove 'len' variable, in is_y420cmdb function, which is used
>>        only once
>>      - Add length check in is_y420vdb function
>>      - Remove unnecessary if (!db) check in function parse_y420cmdb_bitmap
>>      - Do not add print about YCBCR 420 modes
>>      - Fix indentation in few places
>>      - Move ycbcr420_dc_modes in next patch, where its used
>>      - Add a separate patch for movement of drm_add_display_info()
>>
>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
>> ---
>>   drivers/gpu/drm/drm_edid.c  | 157 +++++++++++++++++++++++++++++++++++++++++++-
>>   include/drm/drm_connector.h |  20 ++++++
>>   2 files changed, 174 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>> index e2d1b30..8c7e73b 100644
>> --- a/drivers/gpu/drm/drm_edid.c
>> +++ b/drivers/gpu/drm/drm_edid.c
>> @@ -2781,7 +2781,9 @@ add_detailed_modes(struct drm_connector *connector, struct edid *edid,
>>   #define VIDEO_BLOCK     0x02
>>   #define VENDOR_BLOCK    0x03
>>   #define SPEAKER_BLOCK	0x04
>> -#define VIDEO_CAPABILITY_BLOCK	0x07
>> +#define VIDEO_CAPABILITY_BLOCK 0x07
>> +#define VIDEO_DATA_BLOCK_420	0x0E
>> +#define VIDEO_CAP_BLOCK_Y420CMDB 0x0F
>>   #define EDID_BASIC_AUDIO	(1 << 6)
>>   #define EDID_CEA_YCRCB444	(1 << 5)
>>   #define EDID_CEA_YCRCB422	(1 << 4)
>> @@ -3153,15 +3155,79 @@ drm_display_mode_from_vic_index(struct drm_connector *connector,
>>   	return newmode;
>>   }
>>   
>> +/*
>> + * do_y420vdb_modes - Parse YCBCR 420 only modes
>> + * @connector: connector corresponding to the HDMI sink
>> + * @svds: start of the data block of CEA YCBCR 420 VDB
>> + * @len: length of the CEA YCBCR 420 VDB
>> + *
>> + * Parse the CEA-861-F YCBCR 420 Video Data Block (Y420VDB)
>> + * which contains modes which can be supported in YCBCR 420
>> + * output format only.
>> + */
>> +static int do_y420vdb_modes(struct drm_connector *connector,
>> +				   const u8 *svds, u8 svds_len)
>> +{
>> +	int modes = 0, i;
>> +	struct drm_device *dev = connector->dev;
>> +	struct drm_display_info *info = &connector->display_info;
>> +	struct drm_hdmi_info *hdmi = &info->hdmi;
>> +
>> +	for (i = 0; i < svds_len; i++) {
>> +		u8 vic = svd_to_vic(svds[i]);
>> +		struct drm_display_mode *newmode;
>> +
>> +		newmode = drm_mode_duplicate(dev, &edid_cea_modes[vic]);
>> +		if (!newmode)
>> +			break;
>> +		bitmap_set(hdmi->y420_vdb_modes, vic, 1);
>> +		drm_mode_probed_add(connector, newmode);
>> +		modes++;
>> +	}
>> +
>> +	if (modes > 0)
>> +		info->color_formats |= DRM_COLOR_FORMAT_YCRCB420;
>> +	return modes;
>> +}
>> +
>> +/*
>> + * drm_add_cmdb_modes - Add a YCBCR 420 mode into bitmap
>> + * @connector: connector corresponding to the HDMI sink
>> + * @vic: CEA vic for the video mode to be added in the map
>> + *
>> + * Makes an entry for a videomode in the YCBCR 420 bitmap
>> + */
>> +static void
>> +drm_add_cmdb_modes(struct drm_connector *connector, u8 svd)
>> +{
>> +	u8 vic = svd_to_vic(svd);
>> +	struct drm_hdmi_info *hdmi = &connector->display_info.hdmi;
>> +
>> +	bitmap_set(hdmi->y420_cmdb_modes, vic, 1);
>> +}
>> +
>>   static int
>>   do_cea_modes(struct drm_connector *connector, const u8 *db, u8 len)
>>   {
>>   	int i, modes = 0;
>> +	struct drm_hdmi_info *hdmi = &connector->display_info.hdmi;
>>   
>>   	for (i = 0; i < len; i++) {
>>   		struct drm_display_mode *mode;
>>   		mode = drm_display_mode_from_vic_index(connector, db, len, i);
>>   		if (mode) {
>> +			/*
>> +			 * YCBCR420 capability block contains a bitmap which
>> +			 * gives the index of CEA modes from CEA VDB, which
>> +			 * can support YCBCR 420 sampling output also (apart
>> +			 * from RGB/YCBCR444 etc).
>> +			 * For example, if the bit 0 in bitmap is set,
>> +			 * first mode in VDB can support YCBCR420 output too.
>> +			 * Add YCBCR420 modes only if sink is HDMI 2.0 capable.
>> +			 */
>> +			if (test_bit(i, &hdmi->y420_cmdb_map))
>> +				drm_add_cmdb_modes(connector, db[i]);
>> +
>>   			drm_mode_probed_add(connector, mode);
>>   			modes++;
>>   		}
>> @@ -3437,6 +3503,12 @@ do_hdmi_vsdb_modes(struct drm_connector *connector, const u8 *db, u8 len,
>>   }
>>   
>>   static int
>> +cea_db_extended_tag(const u8 *db)
>> +{
>> +	return db[1];
>> +}
>> +
>> +static int
>>   cea_db_payload_len(const u8 *db)
>>   {
>>   	return db[0] & 0x1f;
>> @@ -3497,9 +3569,78 @@ static bool cea_db_is_hdmi_forum_vsdb(const u8 *db)
>>   	return oui == HDMI_FORUM_IEEE_OUI;
>>   }
>>   
>> +static bool cea_db_is_y420cmdb(const u8 *db)
>> +{
>> +
>> +	if (cea_db_tag(db) != VIDEO_CAPABILITY_BLOCK)
> Still would like to see that patch to clean up the current extended tag
> usage...
Yep, its on the way, running a bit loaded.
>> +		return false;
>> +
>> +	if (!cea_db_payload_len(db))
>> +		return false;
>> +
>> +	if (cea_db_extended_tag(db) != VIDEO_CAP_BLOCK_Y420CMDB)
>> +		return false;
>> +
>> +	return true;
>> +}
>> +
>> +static bool cea_db_is_y420vdb(const u8 *db)
>> +{
>> +	if (cea_db_tag(db) != VIDEO_CAPABILITY_BLOCK)
>> +		return false;
>> +
>> +	if (!cea_db_payload_len(db))
>> +		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)
>>   
>> +static void drm_parse_y420cmdb_bitmap(struct drm_connector *connector,
>> +				     const u8 *db)
>> +{
>> +	struct drm_display_info *info = &connector->display_info;
>> +	struct drm_hdmi_info *hdmi = &info->hdmi;
>> +	u8 map_len = cea_db_payload_len(db) - 1;
>> +	u8 count;
>> +	u64 map = 0;
>> +
>> +	if (map_len == 0) {
>> +		/* All CEA modes support ycbcr420 sampling also.*/
>> +		hdmi->y420_cmdb_map = U64_MAX;
>> +		info->color_formats |= DRM_COLOR_FORMAT_YCRCB420;
>> +		return;
>> +	}
>> +
>> +	/*
>> +	 * This map indicates which of the existing CEA block modes
>> +	 * from VDB can support YCBCR420 output too. So if bit=0 is
>> +	 * set, first mode from VDB can support YCBCR420 output too.
>> +	 * We will parse and keep this map, before parsing VDB itself
>> +	 * to avoid going through the same block again and again.
>> +	 *
>> +	 * Spec is not clear about max possible size of this block.
>> +	 * Clamping max bitmap block size at 8 bytes. Every byte can
>> +	 * address 8 CEA modes, in this way this map can address
>> +	 * 8*8 = first 64 SVDs.
>> +	 */
>> +	if (WARN_ON_ONCE(map_len > 8))
>> +		map_len = 8;
>> +
>> +	for (count = 0; count < map_len; count++)
>> +		map |= (u64)db[2 + count] << (8 * count);
>> +
>> +	if (map)
>> +		info->color_formats |= DRM_COLOR_FORMAT_YCRCB420;
>> +
>> +	hdmi->y420_cmdb_map = map;
>> +}
>> +
>>   static int
>>   add_cea_modes(struct drm_connector *connector, struct edid *edid)
>>   {
>> @@ -3522,10 +3663,17 @@ 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_y420vdb(db) &&
>> +				   connector->ycbcr_420_allowed) {
>> +				const u8 *vdb420 = &db[2];
>> +
>> +				/* Add 4:2:0(only) modes present in EDID */
>> +				modes += do_y420vdb_modes(connector,
>> +							  vdb420,
>> +							  dbl - 1);
>>   			}
>>   		}
>>   	}
>> @@ -4206,6 +4354,8 @@ static void drm_parse_cea_ext(struct drm_connector *connector,
>>   			drm_parse_hdmi_vsdb_video(connector, db);
>>   		if (cea_db_is_hdmi_forum_vsdb(db))
>>   			drm_parse_hdmi_forum_vsdb(connector, db);
>> +		if (cea_db_is_y420cmdb(db))
>> +			drm_parse_y420cmdb_bitmap(connector, db);
>>   	}
>>   }
>>   
>> @@ -4460,6 +4610,7 @@ int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid)
>>   	num_modes += add_cea_modes(connector, edid);
>>   	num_modes += add_alternate_cea_modes(connector, edid);
>>   	num_modes += add_displayid_detailed_modes(connector, edid);
>> +
>>   	if (edid->features & DRM_EDID_FEATURE_DEFAULT_GTF)
>>   		num_modes += add_inferred_modes(connector, edid);
>>   
>> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
>> index 7493fd3..ce2212d 100644
>> --- a/include/drm/drm_connector.h
>> +++ b/include/drm/drm_connector.h
>> @@ -137,6 +137,25 @@ struct drm_scdc {
>>   struct drm_hdmi_info {
>>   	/** @scdc: sink's scdc support and capabilities */
>>   	struct drm_scdc scdc;
>> +
>> +	/**
>> +	 * @y420_vdb_modes: bitmap of modes which can support ycbcr420
>> +	 * output only (not normal RGB/YCBCR444/422 outputs). There are total
>> +	 * 107 VICs defined by CEA-861-F spec, so the size is 128 bits to map
>> +	 * upto 128 VICs;
>> +	 */
>> +	unsigned long y420_vdb_modes[BITS_TO_LONGS(128)];
>> +
>> +	/**
>> +	 * @y420_cmdb_modes: bitmap of modes which can support ycbcr420
>> +	 * output also, along with normal HDMI outputs. There are total 107
>> +	 * VICs defined by CEA-861-F spec, so the size is 128 bits to map upto
>> +	 * 128 VICs;
>> +	 */
>> +	unsigned long y420_cmdb_modes[BITS_TO_LONGS(128)];
>> +
>> +	/** @y420_cmdb_map: bitmap of SVD index, to extraxt vcb modes */
>> +	unsigned long y420_cmdb_map;
> u64, otherwise it's busted on 32bit machines.
Got it.
>
> Mostl this looks pretty good. I think the main problem is the lack of
> mode validation that would reject the 4:2:0 only modes if not supported
> by the driver/device. I guess that's in a later patch?
Yep, its the next patch in the series.
>   If so we need to
> reorder the series a bit to avoid regeressions.
That patch uses parameters added in this patch, to detect the 4:2:0 only 
modes. So cant make it before this patch.
Let me see what best can I do on the reordering.

- Shashank
>>   };
>>   
>>   /**
>> @@ -200,6 +219,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<<3)
>>   
>>   	/**
>>   	 * @color_formats: HDMI Color formats, selects between RGB and YCrCb
>> -- 
>> 2.7.4
Ville Syrjälä June 30, 2017, 11:58 a.m. UTC | #3
On Fri, Jun 30, 2017 at 10:47:48AM +0530, Sharma, Shashank wrote:
> Regards
> 
> Shashank
> 
> 
> On 6/27/2017 5:22 PM, Ville Syrjälä wrote:
> > On Wed, Jun 21, 2017 at 04:04:02PM +0530, Shashank Sharma wrote:
> >> HDMI 2.0 spec adds support for YCBCR420 sub-sampled output.
> >> CEA-861-F adds two new blocks in EDID's CEA extension blocks,
> >> to provide information about sink's YCBCR420 output capabilities.
> >>
> >> These blocks are:
> >>
> >> - YCBCR420vdb(YCBCR 420 video data block):
> >> This block contains VICs of video modes, which can be sopported only
> >> in YCBCR420 output mode (Not in RGB/YCBCR444/422. Its like a normal
> >> SVD block, valid for YCBCR420 modes only.
> >>
> >> - YCBCR420cmdb(YCBCR 420 capability map data block):
> >> This block gives information about video modes which can support
> >> YCBCR420 output mode also (along with RGB,YCBCR444/422 etc) This
> >> block contains a bitmap index of normal svd videomodes, which can
> >> support YCBCR420 output too.
> >> So if bit 0 from first vcb byte is set, first video mode in the svd
> >> list can support YCBCR420 output too. Bit 1 means second video mode
> >> from svd list can support YCBCR420 output too, and so on.
> >>
> >> This patch adds two bitmaps in display's hdmi_info structure, one each
> >> for VCB and VDB modes. If the source is HDMI 2.0 capable, this patch
> >> adds:
> >> - VDB modes (YCBCR 420 only modes) in connector's mode list, also makes
> >>    an entry in the vdb_bitmap per vic.
> >> - VCB modes (YCBCR 420 also modes) only entry in the vcb_bitmap.
> >>
> >> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> >> Cc: Jose Abreu <joabreu@synopsys.com>
> >> Cc: Emil Velikov <emil.l.velikov@gmail.com>
> >>
> >> V2: Addressed
> >>      Review comments from Emil:
> >>      - Use 1ULL<<i instead of 1<<i to make sure the output is 64bit.
> >>      - Use the suggested method for updating dbmap.
> >>      - Add documentation for YCBCR420_vcb_map to fix kbuild warning.
> >>
> >>      Review comments from Ville:
> >>      - Do not expose the YCBCR420 flags in uabi layer, keep it internal.
> >>      - Save a map of YCBCR420 modes for future reference.
> >>      - Check db length before trying to parse extended tag.
> >>      - Add a warning if there are > 64 modes in capability map block.
> >>      - Use y420cmdb in function names and macros while dealing with vcb
> >>        to be aligned with spec.
> >>      - Move the display information parsing block ahead of mode parsing
> >>        blocks.
> >>
> >> V3: Addressed design/review comments from Ville
> >>      - Do not add flags in video modes, else we have to expose them to user
> >>      - There should not be a UABI change, and kernel should detect the
> >>        choice of the output based on type of mode, and the bitmaps.
> >>      - Use standard bitops from kernel bitmap header, instead of calculating
> >>        bit positions manually.
> >>
> >> V4: Addressed review comments from Ville:
> >>      - s/ycbcr_420_vdb/y420vdb
> >>      - s/ycbcr_420_vcb/y420cmdb
> >>      - Be less verbose on description of do_y420vdb_modes
> >>      - Move newmode variable in the loop scope.
> >>      - Use svd_to_vic() to get a VIC, instead of 0x7f
> >>      - Remove bitmap description for CMDB modes & VDB modes
> >>      - Dont add connector->ycbcr_420_allowed check for cmdb modes
> >>      - Remove 'len' variable, in is_y420cmdb function, which is used
> >>        only once
> >>      - Add length check in is_y420vdb function
> >>      - Remove unnecessary if (!db) check in function parse_y420cmdb_bitmap
> >>      - Do not add print about YCBCR 420 modes
> >>      - Fix indentation in few places
> >>      - Move ycbcr420_dc_modes in next patch, where its used
> >>      - Add a separate patch for movement of drm_add_display_info()
> >>
> >> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> >> ---
> >>   drivers/gpu/drm/drm_edid.c  | 157 +++++++++++++++++++++++++++++++++++++++++++-
> >>   include/drm/drm_connector.h |  20 ++++++
> >>   2 files changed, 174 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> >> index e2d1b30..8c7e73b 100644
> >> --- a/drivers/gpu/drm/drm_edid.c
> >> +++ b/drivers/gpu/drm/drm_edid.c
> >> @@ -2781,7 +2781,9 @@ add_detailed_modes(struct drm_connector *connector, struct edid *edid,
> >>   #define VIDEO_BLOCK     0x02
> >>   #define VENDOR_BLOCK    0x03
> >>   #define SPEAKER_BLOCK	0x04
> >> -#define VIDEO_CAPABILITY_BLOCK	0x07
> >> +#define VIDEO_CAPABILITY_BLOCK 0x07
> >> +#define VIDEO_DATA_BLOCK_420	0x0E
> >> +#define VIDEO_CAP_BLOCK_Y420CMDB 0x0F
> >>   #define EDID_BASIC_AUDIO	(1 << 6)
> >>   #define EDID_CEA_YCRCB444	(1 << 5)
> >>   #define EDID_CEA_YCRCB422	(1 << 4)
> >> @@ -3153,15 +3155,79 @@ drm_display_mode_from_vic_index(struct drm_connector *connector,
> >>   	return newmode;
> >>   }
> >>   
> >> +/*
> >> + * do_y420vdb_modes - Parse YCBCR 420 only modes
> >> + * @connector: connector corresponding to the HDMI sink
> >> + * @svds: start of the data block of CEA YCBCR 420 VDB
> >> + * @len: length of the CEA YCBCR 420 VDB
> >> + *
> >> + * Parse the CEA-861-F YCBCR 420 Video Data Block (Y420VDB)
> >> + * which contains modes which can be supported in YCBCR 420
> >> + * output format only.
> >> + */
> >> +static int do_y420vdb_modes(struct drm_connector *connector,
> >> +				   const u8 *svds, u8 svds_len)
> >> +{
> >> +	int modes = 0, i;
> >> +	struct drm_device *dev = connector->dev;
> >> +	struct drm_display_info *info = &connector->display_info;
> >> +	struct drm_hdmi_info *hdmi = &info->hdmi;
> >> +
> >> +	for (i = 0; i < svds_len; i++) {
> >> +		u8 vic = svd_to_vic(svds[i]);
> >> +		struct drm_display_mode *newmode;
> >> +
> >> +		newmode = drm_mode_duplicate(dev, &edid_cea_modes[vic]);
> >> +		if (!newmode)
> >> +			break;
> >> +		bitmap_set(hdmi->y420_vdb_modes, vic, 1);
> >> +		drm_mode_probed_add(connector, newmode);
> >> +		modes++;
> >> +	}
> >> +
> >> +	if (modes > 0)
> >> +		info->color_formats |= DRM_COLOR_FORMAT_YCRCB420;
> >> +	return modes;
> >> +}
> >> +
> >> +/*
> >> + * drm_add_cmdb_modes - Add a YCBCR 420 mode into bitmap
> >> + * @connector: connector corresponding to the HDMI sink
> >> + * @vic: CEA vic for the video mode to be added in the map
> >> + *
> >> + * Makes an entry for a videomode in the YCBCR 420 bitmap
> >> + */
> >> +static void
> >> +drm_add_cmdb_modes(struct drm_connector *connector, u8 svd)
> >> +{
> >> +	u8 vic = svd_to_vic(svd);
> >> +	struct drm_hdmi_info *hdmi = &connector->display_info.hdmi;
> >> +
> >> +	bitmap_set(hdmi->y420_cmdb_modes, vic, 1);
> >> +}
> >> +
> >>   static int
> >>   do_cea_modes(struct drm_connector *connector, const u8 *db, u8 len)
> >>   {
> >>   	int i, modes = 0;
> >> +	struct drm_hdmi_info *hdmi = &connector->display_info.hdmi;
> >>   
> >>   	for (i = 0; i < len; i++) {
> >>   		struct drm_display_mode *mode;
> >>   		mode = drm_display_mode_from_vic_index(connector, db, len, i);
> >>   		if (mode) {
> >> +			/*
> >> +			 * YCBCR420 capability block contains a bitmap which
> >> +			 * gives the index of CEA modes from CEA VDB, which
> >> +			 * can support YCBCR 420 sampling output also (apart
> >> +			 * from RGB/YCBCR444 etc).
> >> +			 * For example, if the bit 0 in bitmap is set,
> >> +			 * first mode in VDB can support YCBCR420 output too.
> >> +			 * Add YCBCR420 modes only if sink is HDMI 2.0 capable.
> >> +			 */
> >> +			if (test_bit(i, &hdmi->y420_cmdb_map))
> >> +				drm_add_cmdb_modes(connector, db[i]);
> >> +
> >>   			drm_mode_probed_add(connector, mode);
> >>   			modes++;
> >>   		}
> >> @@ -3437,6 +3503,12 @@ do_hdmi_vsdb_modes(struct drm_connector *connector, const u8 *db, u8 len,
> >>   }
> >>   
> >>   static int
> >> +cea_db_extended_tag(const u8 *db)
> >> +{
> >> +	return db[1];
> >> +}
> >> +
> >> +static int
> >>   cea_db_payload_len(const u8 *db)
> >>   {
> >>   	return db[0] & 0x1f;
> >> @@ -3497,9 +3569,78 @@ static bool cea_db_is_hdmi_forum_vsdb(const u8 *db)
> >>   	return oui == HDMI_FORUM_IEEE_OUI;
> >>   }
> >>   
> >> +static bool cea_db_is_y420cmdb(const u8 *db)
> >> +{
> >> +
> >> +	if (cea_db_tag(db) != VIDEO_CAPABILITY_BLOCK)
> > Still would like to see that patch to clean up the current extended tag
> > usage...
> Yep, its on the way, running a bit loaded.
> >> +		return false;
> >> +
> >> +	if (!cea_db_payload_len(db))
> >> +		return false;
> >> +
> >> +	if (cea_db_extended_tag(db) != VIDEO_CAP_BLOCK_Y420CMDB)
> >> +		return false;
> >> +
> >> +	return true;
> >> +}
> >> +
> >> +static bool cea_db_is_y420vdb(const u8 *db)
> >> +{
> >> +	if (cea_db_tag(db) != VIDEO_CAPABILITY_BLOCK)
> >> +		return false;
> >> +
> >> +	if (!cea_db_payload_len(db))
> >> +		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)
> >>   
> >> +static void drm_parse_y420cmdb_bitmap(struct drm_connector *connector,
> >> +				     const u8 *db)
> >> +{
> >> +	struct drm_display_info *info = &connector->display_info;
> >> +	struct drm_hdmi_info *hdmi = &info->hdmi;
> >> +	u8 map_len = cea_db_payload_len(db) - 1;
> >> +	u8 count;
> >> +	u64 map = 0;
> >> +
> >> +	if (map_len == 0) {
> >> +		/* All CEA modes support ycbcr420 sampling also.*/
> >> +		hdmi->y420_cmdb_map = U64_MAX;
> >> +		info->color_formats |= DRM_COLOR_FORMAT_YCRCB420;
> >> +		return;
> >> +	}
> >> +
> >> +	/*
> >> +	 * This map indicates which of the existing CEA block modes
> >> +	 * from VDB can support YCBCR420 output too. So if bit=0 is
> >> +	 * set, first mode from VDB can support YCBCR420 output too.
> >> +	 * We will parse and keep this map, before parsing VDB itself
> >> +	 * to avoid going through the same block again and again.
> >> +	 *
> >> +	 * Spec is not clear about max possible size of this block.
> >> +	 * Clamping max bitmap block size at 8 bytes. Every byte can
> >> +	 * address 8 CEA modes, in this way this map can address
> >> +	 * 8*8 = first 64 SVDs.
> >> +	 */
> >> +	if (WARN_ON_ONCE(map_len > 8))
> >> +		map_len = 8;
> >> +
> >> +	for (count = 0; count < map_len; count++)
> >> +		map |= (u64)db[2 + count] << (8 * count);
> >> +
> >> +	if (map)
> >> +		info->color_formats |= DRM_COLOR_FORMAT_YCRCB420;
> >> +
> >> +	hdmi->y420_cmdb_map = map;
> >> +}
> >> +
> >>   static int
> >>   add_cea_modes(struct drm_connector *connector, struct edid *edid)
> >>   {
> >> @@ -3522,10 +3663,17 @@ 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_y420vdb(db) &&
> >> +				   connector->ycbcr_420_allowed) {
> >> +				const u8 *vdb420 = &db[2];
> >> +
> >> +				/* Add 4:2:0(only) modes present in EDID */
> >> +				modes += do_y420vdb_modes(connector,
> >> +							  vdb420,
> >> +							  dbl - 1);
> >>   			}
> >>   		}
> >>   	}
> >> @@ -4206,6 +4354,8 @@ static void drm_parse_cea_ext(struct drm_connector *connector,
> >>   			drm_parse_hdmi_vsdb_video(connector, db);
> >>   		if (cea_db_is_hdmi_forum_vsdb(db))
> >>   			drm_parse_hdmi_forum_vsdb(connector, db);
> >> +		if (cea_db_is_y420cmdb(db))
> >> +			drm_parse_y420cmdb_bitmap(connector, db);
> >>   	}
> >>   }
> >>   
> >> @@ -4460,6 +4610,7 @@ int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid)
> >>   	num_modes += add_cea_modes(connector, edid);
> >>   	num_modes += add_alternate_cea_modes(connector, edid);
> >>   	num_modes += add_displayid_detailed_modes(connector, edid);
> >> +
> >>   	if (edid->features & DRM_EDID_FEATURE_DEFAULT_GTF)
> >>   		num_modes += add_inferred_modes(connector, edid);
> >>   
> >> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> >> index 7493fd3..ce2212d 100644
> >> --- a/include/drm/drm_connector.h
> >> +++ b/include/drm/drm_connector.h
> >> @@ -137,6 +137,25 @@ struct drm_scdc {
> >>   struct drm_hdmi_info {
> >>   	/** @scdc: sink's scdc support and capabilities */
> >>   	struct drm_scdc scdc;
> >> +
> >> +	/**
> >> +	 * @y420_vdb_modes: bitmap of modes which can support ycbcr420
> >> +	 * output only (not normal RGB/YCBCR444/422 outputs). There are total
> >> +	 * 107 VICs defined by CEA-861-F spec, so the size is 128 bits to map
> >> +	 * upto 128 VICs;
> >> +	 */
> >> +	unsigned long y420_vdb_modes[BITS_TO_LONGS(128)];
> >> +
> >> +	/**
> >> +	 * @y420_cmdb_modes: bitmap of modes which can support ycbcr420
> >> +	 * output also, along with normal HDMI outputs. There are total 107
> >> +	 * VICs defined by CEA-861-F spec, so the size is 128 bits to map upto
> >> +	 * 128 VICs;
> >> +	 */
> >> +	unsigned long y420_cmdb_modes[BITS_TO_LONGS(128)];
> >> +
> >> +	/** @y420_cmdb_map: bitmap of SVD index, to extraxt vcb modes */
> >> +	unsigned long y420_cmdb_map;
> > u64, otherwise it's busted on 32bit machines.
> Got it.
> >
> > Mostl this looks pretty good. I think the main problem is the lack of
> > mode validation that would reject the 4:2:0 only modes if not supported
> > by the driver/device. I guess that's in a later patch?
> Yep, its the next patch in the series.

The only thing the next patch does is set up the y420_dc_modes, but it
doesn't use that for anything, and neither does it filter anything. I
can't find any mode filtering patch actually.

> >   If so we need to
> > reorder the series a bit to avoid regeressions.
> That patch uses parameters added in this patch, to detect the 4:2:0 only 
> modes. So cant make it before this patch.
> Let me see what best can I do on the reordering.
> 
> - Shashank
> >>   };
> >>   
> >>   /**
> >> @@ -200,6 +219,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<<3)
> >>   
> >>   	/**
> >>   	 * @color_formats: HDMI Color formats, selects between RGB and YCrCb
> >> -- 
> >> 2.7.4
Sharma, Shashank June 30, 2017, 12:08 p.m. UTC | #4
Regards

Shashank


On 6/30/2017 5:28 PM, Ville Syrjälä wrote:
> On Fri, Jun 30, 2017 at 10:47:48AM +0530, Sharma, Shashank wrote:
>> Regards
>>
>> Shashank
>>
>>
>> On 6/27/2017 5:22 PM, Ville Syrjälä wrote:
>>> On Wed, Jun 21, 2017 at 04:04:02PM +0530, Shashank Sharma wrote:
>>>> HDMI 2.0 spec adds support for YCBCR420 sub-sampled output.
>>>> CEA-861-F adds two new blocks in EDID's CEA extension blocks,
>>>> to provide information about sink's YCBCR420 output capabilities.
>>>>
>>>> These blocks are:
>>>>
>>>> - YCBCR420vdb(YCBCR 420 video data block):
>>>> This block contains VICs of video modes, which can be sopported only
>>>> in YCBCR420 output mode (Not in RGB/YCBCR444/422. Its like a normal
>>>> SVD block, valid for YCBCR420 modes only.
>>>>
>>>> - YCBCR420cmdb(YCBCR 420 capability map data block):
>>>> This block gives information about video modes which can support
>>>> YCBCR420 output mode also (along with RGB,YCBCR444/422 etc) This
>>>> block contains a bitmap index of normal svd videomodes, which can
>>>> support YCBCR420 output too.
>>>> So if bit 0 from first vcb byte is set, first video mode in the svd
>>>> list can support YCBCR420 output too. Bit 1 means second video mode
>>>> from svd list can support YCBCR420 output too, and so on.
>>>>
>>>> This patch adds two bitmaps in display's hdmi_info structure, one each
>>>> for VCB and VDB modes. If the source is HDMI 2.0 capable, this patch
>>>> adds:
>>>> - VDB modes (YCBCR 420 only modes) in connector's mode list, also makes
>>>>     an entry in the vdb_bitmap per vic.
>>>> - VCB modes (YCBCR 420 also modes) only entry in the vcb_bitmap.
>>>>
>>>> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
>>>> Cc: Jose Abreu <joabreu@synopsys.com>
>>>> Cc: Emil Velikov <emil.l.velikov@gmail.com>
>>>>
>>>> V2: Addressed
>>>>       Review comments from Emil:
>>>>       - Use 1ULL<<i instead of 1<<i to make sure the output is 64bit.
>>>>       - Use the suggested method for updating dbmap.
>>>>       - Add documentation for YCBCR420_vcb_map to fix kbuild warning.
>>>>
>>>>       Review comments from Ville:
>>>>       - Do not expose the YCBCR420 flags in uabi layer, keep it internal.
>>>>       - Save a map of YCBCR420 modes for future reference.
>>>>       - Check db length before trying to parse extended tag.
>>>>       - Add a warning if there are > 64 modes in capability map block.
>>>>       - Use y420cmdb in function names and macros while dealing with vcb
>>>>         to be aligned with spec.
>>>>       - Move the display information parsing block ahead of mode parsing
>>>>         blocks.
>>>>
>>>> V3: Addressed design/review comments from Ville
>>>>       - Do not add flags in video modes, else we have to expose them to user
>>>>       - There should not be a UABI change, and kernel should detect the
>>>>         choice of the output based on type of mode, and the bitmaps.
>>>>       - Use standard bitops from kernel bitmap header, instead of calculating
>>>>         bit positions manually.
>>>>
>>>> V4: Addressed review comments from Ville:
>>>>       - s/ycbcr_420_vdb/y420vdb
>>>>       - s/ycbcr_420_vcb/y420cmdb
>>>>       - Be less verbose on description of do_y420vdb_modes
>>>>       - Move newmode variable in the loop scope.
>>>>       - Use svd_to_vic() to get a VIC, instead of 0x7f
>>>>       - Remove bitmap description for CMDB modes & VDB modes
>>>>       - Dont add connector->ycbcr_420_allowed check for cmdb modes
>>>>       - Remove 'len' variable, in is_y420cmdb function, which is used
>>>>         only once
>>>>       - Add length check in is_y420vdb function
>>>>       - Remove unnecessary if (!db) check in function parse_y420cmdb_bitmap
>>>>       - Do not add print about YCBCR 420 modes
>>>>       - Fix indentation in few places
>>>>       - Move ycbcr420_dc_modes in next patch, where its used
>>>>       - Add a separate patch for movement of drm_add_display_info()
>>>>
>>>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
>>>> ---
>>>>    drivers/gpu/drm/drm_edid.c  | 157 +++++++++++++++++++++++++++++++++++++++++++-
>>>>    include/drm/drm_connector.h |  20 ++++++
>>>>    2 files changed, 174 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>>>> index e2d1b30..8c7e73b 100644
>>>> --- a/drivers/gpu/drm/drm_edid.c
>>>> +++ b/drivers/gpu/drm/drm_edid.c
>>>> @@ -2781,7 +2781,9 @@ add_detailed_modes(struct drm_connector *connector, struct edid *edid,
>>>>    #define VIDEO_BLOCK     0x02
>>>>    #define VENDOR_BLOCK    0x03
>>>>    #define SPEAKER_BLOCK	0x04
>>>> -#define VIDEO_CAPABILITY_BLOCK	0x07
>>>> +#define VIDEO_CAPABILITY_BLOCK 0x07
>>>> +#define VIDEO_DATA_BLOCK_420	0x0E
>>>> +#define VIDEO_CAP_BLOCK_Y420CMDB 0x0F
>>>>    #define EDID_BASIC_AUDIO	(1 << 6)
>>>>    #define EDID_CEA_YCRCB444	(1 << 5)
>>>>    #define EDID_CEA_YCRCB422	(1 << 4)
>>>> @@ -3153,15 +3155,79 @@ drm_display_mode_from_vic_index(struct drm_connector *connector,
>>>>    	return newmode;
>>>>    }
>>>>    
>>>> +/*
>>>> + * do_y420vdb_modes - Parse YCBCR 420 only modes
>>>> + * @connector: connector corresponding to the HDMI sink
>>>> + * @svds: start of the data block of CEA YCBCR 420 VDB
>>>> + * @len: length of the CEA YCBCR 420 VDB
>>>> + *
>>>> + * Parse the CEA-861-F YCBCR 420 Video Data Block (Y420VDB)
>>>> + * which contains modes which can be supported in YCBCR 420
>>>> + * output format only.
>>>> + */
>>>> +static int do_y420vdb_modes(struct drm_connector *connector,
>>>> +				   const u8 *svds, u8 svds_len)
>>>> +{
>>>> +	int modes = 0, i;
>>>> +	struct drm_device *dev = connector->dev;
>>>> +	struct drm_display_info *info = &connector->display_info;
>>>> +	struct drm_hdmi_info *hdmi = &info->hdmi;
>>>> +
>>>> +	for (i = 0; i < svds_len; i++) {
>>>> +		u8 vic = svd_to_vic(svds[i]);
>>>> +		struct drm_display_mode *newmode;
>>>> +
>>>> +		newmode = drm_mode_duplicate(dev, &edid_cea_modes[vic]);
>>>> +		if (!newmode)
>>>> +			break;
>>>> +		bitmap_set(hdmi->y420_vdb_modes, vic, 1);
>>>> +		drm_mode_probed_add(connector, newmode);
>>>> +		modes++;
>>>> +	}
>>>> +
>>>> +	if (modes > 0)
>>>> +		info->color_formats |= DRM_COLOR_FORMAT_YCRCB420;
>>>> +	return modes;
>>>> +}
>>>> +
>>>> +/*
>>>> + * drm_add_cmdb_modes - Add a YCBCR 420 mode into bitmap
>>>> + * @connector: connector corresponding to the HDMI sink
>>>> + * @vic: CEA vic for the video mode to be added in the map
>>>> + *
>>>> + * Makes an entry for a videomode in the YCBCR 420 bitmap
>>>> + */
>>>> +static void
>>>> +drm_add_cmdb_modes(struct drm_connector *connector, u8 svd)
>>>> +{
>>>> +	u8 vic = svd_to_vic(svd);
>>>> +	struct drm_hdmi_info *hdmi = &connector->display_info.hdmi;
>>>> +
>>>> +	bitmap_set(hdmi->y420_cmdb_modes, vic, 1);
>>>> +}
>>>> +
>>>>    static int
>>>>    do_cea_modes(struct drm_connector *connector, const u8 *db, u8 len)
>>>>    {
>>>>    	int i, modes = 0;
>>>> +	struct drm_hdmi_info *hdmi = &connector->display_info.hdmi;
>>>>    
>>>>    	for (i = 0; i < len; i++) {
>>>>    		struct drm_display_mode *mode;
>>>>    		mode = drm_display_mode_from_vic_index(connector, db, len, i);
>>>>    		if (mode) {
>>>> +			/*
>>>> +			 * YCBCR420 capability block contains a bitmap which
>>>> +			 * gives the index of CEA modes from CEA VDB, which
>>>> +			 * can support YCBCR 420 sampling output also (apart
>>>> +			 * from RGB/YCBCR444 etc).
>>>> +			 * For example, if the bit 0 in bitmap is set,
>>>> +			 * first mode in VDB can support YCBCR420 output too.
>>>> +			 * Add YCBCR420 modes only if sink is HDMI 2.0 capable.
>>>> +			 */
>>>> +			if (test_bit(i, &hdmi->y420_cmdb_map))
>>>> +				drm_add_cmdb_modes(connector, db[i]);
>>>> +
>>>>    			drm_mode_probed_add(connector, mode);
>>>>    			modes++;
>>>>    		}
>>>> @@ -3437,6 +3503,12 @@ do_hdmi_vsdb_modes(struct drm_connector *connector, const u8 *db, u8 len,
>>>>    }
>>>>    
>>>>    static int
>>>> +cea_db_extended_tag(const u8 *db)
>>>> +{
>>>> +	return db[1];
>>>> +}
>>>> +
>>>> +static int
>>>>    cea_db_payload_len(const u8 *db)
>>>>    {
>>>>    	return db[0] & 0x1f;
>>>> @@ -3497,9 +3569,78 @@ static bool cea_db_is_hdmi_forum_vsdb(const u8 *db)
>>>>    	return oui == HDMI_FORUM_IEEE_OUI;
>>>>    }
>>>>    
>>>> +static bool cea_db_is_y420cmdb(const u8 *db)
>>>> +{
>>>> +
>>>> +	if (cea_db_tag(db) != VIDEO_CAPABILITY_BLOCK)
>>> Still would like to see that patch to clean up the current extended tag
>>> usage...
>> Yep, its on the way, running a bit loaded.
>>>> +		return false;
>>>> +
>>>> +	if (!cea_db_payload_len(db))
>>>> +		return false;
>>>> +
>>>> +	if (cea_db_extended_tag(db) != VIDEO_CAP_BLOCK_Y420CMDB)
>>>> +		return false;
>>>> +
>>>> +	return true;
>>>> +}
>>>> +
>>>> +static bool cea_db_is_y420vdb(const u8 *db)
>>>> +{
>>>> +	if (cea_db_tag(db) != VIDEO_CAPABILITY_BLOCK)
>>>> +		return false;
>>>> +
>>>> +	if (!cea_db_payload_len(db))
>>>> +		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)
>>>>    
>>>> +static void drm_parse_y420cmdb_bitmap(struct drm_connector *connector,
>>>> +				     const u8 *db)
>>>> +{
>>>> +	struct drm_display_info *info = &connector->display_info;
>>>> +	struct drm_hdmi_info *hdmi = &info->hdmi;
>>>> +	u8 map_len = cea_db_payload_len(db) - 1;
>>>> +	u8 count;
>>>> +	u64 map = 0;
>>>> +
>>>> +	if (map_len == 0) {
>>>> +		/* All CEA modes support ycbcr420 sampling also.*/
>>>> +		hdmi->y420_cmdb_map = U64_MAX;
>>>> +		info->color_formats |= DRM_COLOR_FORMAT_YCRCB420;
>>>> +		return;
>>>> +	}
>>>> +
>>>> +	/*
>>>> +	 * This map indicates which of the existing CEA block modes
>>>> +	 * from VDB can support YCBCR420 output too. So if bit=0 is
>>>> +	 * set, first mode from VDB can support YCBCR420 output too.
>>>> +	 * We will parse and keep this map, before parsing VDB itself
>>>> +	 * to avoid going through the same block again and again.
>>>> +	 *
>>>> +	 * Spec is not clear about max possible size of this block.
>>>> +	 * Clamping max bitmap block size at 8 bytes. Every byte can
>>>> +	 * address 8 CEA modes, in this way this map can address
>>>> +	 * 8*8 = first 64 SVDs.
>>>> +	 */
>>>> +	if (WARN_ON_ONCE(map_len > 8))
>>>> +		map_len = 8;
>>>> +
>>>> +	for (count = 0; count < map_len; count++)
>>>> +		map |= (u64)db[2 + count] << (8 * count);
>>>> +
>>>> +	if (map)
>>>> +		info->color_formats |= DRM_COLOR_FORMAT_YCRCB420;
>>>> +
>>>> +	hdmi->y420_cmdb_map = map;
>>>> +}
>>>> +
>>>>    static int
>>>>    add_cea_modes(struct drm_connector *connector, struct edid *edid)
>>>>    {
>>>> @@ -3522,10 +3663,17 @@ 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_y420vdb(db) &&
>>>> +				   connector->ycbcr_420_allowed) {
>>>> +				const u8 *vdb420 = &db[2];
>>>> +
>>>> +				/* Add 4:2:0(only) modes present in EDID */
>>>> +				modes += do_y420vdb_modes(connector,
>>>> +							  vdb420,
>>>> +							  dbl - 1);
>>>>    			}
>>>>    		}
>>>>    	}
>>>> @@ -4206,6 +4354,8 @@ static void drm_parse_cea_ext(struct drm_connector *connector,
>>>>    			drm_parse_hdmi_vsdb_video(connector, db);
>>>>    		if (cea_db_is_hdmi_forum_vsdb(db))
>>>>    			drm_parse_hdmi_forum_vsdb(connector, db);
>>>> +		if (cea_db_is_y420cmdb(db))
>>>> +			drm_parse_y420cmdb_bitmap(connector, db);
>>>>    	}
>>>>    }
>>>>    
>>>> @@ -4460,6 +4610,7 @@ int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid)
>>>>    	num_modes += add_cea_modes(connector, edid);
>>>>    	num_modes += add_alternate_cea_modes(connector, edid);
>>>>    	num_modes += add_displayid_detailed_modes(connector, edid);
>>>> +
>>>>    	if (edid->features & DRM_EDID_FEATURE_DEFAULT_GTF)
>>>>    		num_modes += add_inferred_modes(connector, edid);
>>>>    
>>>> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
>>>> index 7493fd3..ce2212d 100644
>>>> --- a/include/drm/drm_connector.h
>>>> +++ b/include/drm/drm_connector.h
>>>> @@ -137,6 +137,25 @@ struct drm_scdc {
>>>>    struct drm_hdmi_info {
>>>>    	/** @scdc: sink's scdc support and capabilities */
>>>>    	struct drm_scdc scdc;
>>>> +
>>>> +	/**
>>>> +	 * @y420_vdb_modes: bitmap of modes which can support ycbcr420
>>>> +	 * output only (not normal RGB/YCBCR444/422 outputs). There are total
>>>> +	 * 107 VICs defined by CEA-861-F spec, so the size is 128 bits to map
>>>> +	 * upto 128 VICs;
>>>> +	 */
>>>> +	unsigned long y420_vdb_modes[BITS_TO_LONGS(128)];
>>>> +
>>>> +	/**
>>>> +	 * @y420_cmdb_modes: bitmap of modes which can support ycbcr420
>>>> +	 * output also, along with normal HDMI outputs. There are total 107
>>>> +	 * VICs defined by CEA-861-F spec, so the size is 128 bits to map upto
>>>> +	 * 128 VICs;
>>>> +	 */
>>>> +	unsigned long y420_cmdb_modes[BITS_TO_LONGS(128)];
>>>> +
>>>> +	/** @y420_cmdb_map: bitmap of SVD index, to extraxt vcb modes */
>>>> +	unsigned long y420_cmdb_map;
>>> u64, otherwise it's busted on 32bit machines.
>> Got it.
>>> Mostl this looks pretty good. I think the main problem is the lack of
>>> mode validation that would reject the 4:2:0 only modes if not supported
>>> by the driver/device. I guess that's in a later patch?
>> Yep, its the next patch in the series.
> The only thing the next patch does is set up the y420_dc_modes, but it
> doesn't use that for anything, and neither does it filter anything. I
> can't find any mode filtering patch actually.
Perhaps I misunderstood your question. If you are asking for the patch, 
in which you asked to remove the
420_only modes, at connector_probe level (instead of blocking the read 
of 420_only modes based on connector->y420_allowed),
its being prepared now. It should be available with V5.

- Shashank
>>>    If so we need to
>>> reorder the series a bit to avoid regeressions.
>> That patch uses parameters added in this patch, to detect the 4:2:0 only
>> modes. So cant make it before this patch.
>> Let me see what best can I do on the reordering.
>>
>> - Shashank
>>>>    };
>>>>    
>>>>    /**
>>>> @@ -200,6 +219,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<<3)
>>>>    
>>>>    	/**
>>>>    	 * @color_formats: HDMI Color formats, selects between RGB and YCrCb
>>>> -- 
>>>> 2.7.4
diff mbox

Patch

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index e2d1b30..8c7e73b 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -2781,7 +2781,9 @@  add_detailed_modes(struct drm_connector *connector, struct edid *edid,
 #define VIDEO_BLOCK     0x02
 #define VENDOR_BLOCK    0x03
 #define SPEAKER_BLOCK	0x04
-#define VIDEO_CAPABILITY_BLOCK	0x07
+#define VIDEO_CAPABILITY_BLOCK 0x07
+#define VIDEO_DATA_BLOCK_420	0x0E
+#define VIDEO_CAP_BLOCK_Y420CMDB 0x0F
 #define EDID_BASIC_AUDIO	(1 << 6)
 #define EDID_CEA_YCRCB444	(1 << 5)
 #define EDID_CEA_YCRCB422	(1 << 4)
@@ -3153,15 +3155,79 @@  drm_display_mode_from_vic_index(struct drm_connector *connector,
 	return newmode;
 }
 
+/*
+ * do_y420vdb_modes - Parse YCBCR 420 only modes
+ * @connector: connector corresponding to the HDMI sink
+ * @svds: start of the data block of CEA YCBCR 420 VDB
+ * @len: length of the CEA YCBCR 420 VDB
+ *
+ * Parse the CEA-861-F YCBCR 420 Video Data Block (Y420VDB)
+ * which contains modes which can be supported in YCBCR 420
+ * output format only.
+ */
+static int do_y420vdb_modes(struct drm_connector *connector,
+				   const u8 *svds, u8 svds_len)
+{
+	int modes = 0, i;
+	struct drm_device *dev = connector->dev;
+	struct drm_display_info *info = &connector->display_info;
+	struct drm_hdmi_info *hdmi = &info->hdmi;
+
+	for (i = 0; i < svds_len; i++) {
+		u8 vic = svd_to_vic(svds[i]);
+		struct drm_display_mode *newmode;
+
+		newmode = drm_mode_duplicate(dev, &edid_cea_modes[vic]);
+		if (!newmode)
+			break;
+		bitmap_set(hdmi->y420_vdb_modes, vic, 1);
+		drm_mode_probed_add(connector, newmode);
+		modes++;
+	}
+
+	if (modes > 0)
+		info->color_formats |= DRM_COLOR_FORMAT_YCRCB420;
+	return modes;
+}
+
+/*
+ * drm_add_cmdb_modes - Add a YCBCR 420 mode into bitmap
+ * @connector: connector corresponding to the HDMI sink
+ * @vic: CEA vic for the video mode to be added in the map
+ *
+ * Makes an entry for a videomode in the YCBCR 420 bitmap
+ */
+static void
+drm_add_cmdb_modes(struct drm_connector *connector, u8 svd)
+{
+	u8 vic = svd_to_vic(svd);
+	struct drm_hdmi_info *hdmi = &connector->display_info.hdmi;
+
+	bitmap_set(hdmi->y420_cmdb_modes, vic, 1);
+}
+
 static int
 do_cea_modes(struct drm_connector *connector, const u8 *db, u8 len)
 {
 	int i, modes = 0;
+	struct drm_hdmi_info *hdmi = &connector->display_info.hdmi;
 
 	for (i = 0; i < len; i++) {
 		struct drm_display_mode *mode;
 		mode = drm_display_mode_from_vic_index(connector, db, len, i);
 		if (mode) {
+			/*
+			 * YCBCR420 capability block contains a bitmap which
+			 * gives the index of CEA modes from CEA VDB, which
+			 * can support YCBCR 420 sampling output also (apart
+			 * from RGB/YCBCR444 etc).
+			 * For example, if the bit 0 in bitmap is set,
+			 * first mode in VDB can support YCBCR420 output too.
+			 * Add YCBCR420 modes only if sink is HDMI 2.0 capable.
+			 */
+			if (test_bit(i, &hdmi->y420_cmdb_map))
+				drm_add_cmdb_modes(connector, db[i]);
+
 			drm_mode_probed_add(connector, mode);
 			modes++;
 		}
@@ -3437,6 +3503,12 @@  do_hdmi_vsdb_modes(struct drm_connector *connector, const u8 *db, u8 len,
 }
 
 static int
+cea_db_extended_tag(const u8 *db)
+{
+	return db[1];
+}
+
+static int
 cea_db_payload_len(const u8 *db)
 {
 	return db[0] & 0x1f;
@@ -3497,9 +3569,78 @@  static bool cea_db_is_hdmi_forum_vsdb(const u8 *db)
 	return oui == HDMI_FORUM_IEEE_OUI;
 }
 
+static bool cea_db_is_y420cmdb(const u8 *db)
+{
+
+	if (cea_db_tag(db) != VIDEO_CAPABILITY_BLOCK)
+		return false;
+
+	if (!cea_db_payload_len(db))
+		return false;
+
+	if (cea_db_extended_tag(db) != VIDEO_CAP_BLOCK_Y420CMDB)
+		return false;
+
+	return true;
+}
+
+static bool cea_db_is_y420vdb(const u8 *db)
+{
+	if (cea_db_tag(db) != VIDEO_CAPABILITY_BLOCK)
+		return false;
+
+	if (!cea_db_payload_len(db))
+		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)
 
+static void drm_parse_y420cmdb_bitmap(struct drm_connector *connector,
+				     const u8 *db)
+{
+	struct drm_display_info *info = &connector->display_info;
+	struct drm_hdmi_info *hdmi = &info->hdmi;
+	u8 map_len = cea_db_payload_len(db) - 1;
+	u8 count;
+	u64 map = 0;
+
+	if (map_len == 0) {
+		/* All CEA modes support ycbcr420 sampling also.*/
+		hdmi->y420_cmdb_map = U64_MAX;
+		info->color_formats |= DRM_COLOR_FORMAT_YCRCB420;
+		return;
+	}
+
+	/*
+	 * This map indicates which of the existing CEA block modes
+	 * from VDB can support YCBCR420 output too. So if bit=0 is
+	 * set, first mode from VDB can support YCBCR420 output too.
+	 * We will parse and keep this map, before parsing VDB itself
+	 * to avoid going through the same block again and again.
+	 *
+	 * Spec is not clear about max possible size of this block.
+	 * Clamping max bitmap block size at 8 bytes. Every byte can
+	 * address 8 CEA modes, in this way this map can address
+	 * 8*8 = first 64 SVDs.
+	 */
+	if (WARN_ON_ONCE(map_len > 8))
+		map_len = 8;
+
+	for (count = 0; count < map_len; count++)
+		map |= (u64)db[2 + count] << (8 * count);
+
+	if (map)
+		info->color_formats |= DRM_COLOR_FORMAT_YCRCB420;
+
+	hdmi->y420_cmdb_map = map;
+}
+
 static int
 add_cea_modes(struct drm_connector *connector, struct edid *edid)
 {
@@ -3522,10 +3663,17 @@  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_y420vdb(db) &&
+				   connector->ycbcr_420_allowed) {
+				const u8 *vdb420 = &db[2];
+
+				/* Add 4:2:0(only) modes present in EDID */
+				modes += do_y420vdb_modes(connector,
+							  vdb420,
+							  dbl - 1);
 			}
 		}
 	}
@@ -4206,6 +4354,8 @@  static void drm_parse_cea_ext(struct drm_connector *connector,
 			drm_parse_hdmi_vsdb_video(connector, db);
 		if (cea_db_is_hdmi_forum_vsdb(db))
 			drm_parse_hdmi_forum_vsdb(connector, db);
+		if (cea_db_is_y420cmdb(db))
+			drm_parse_y420cmdb_bitmap(connector, db);
 	}
 }
 
@@ -4460,6 +4610,7 @@  int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid)
 	num_modes += add_cea_modes(connector, edid);
 	num_modes += add_alternate_cea_modes(connector, edid);
 	num_modes += add_displayid_detailed_modes(connector, edid);
+
 	if (edid->features & DRM_EDID_FEATURE_DEFAULT_GTF)
 		num_modes += add_inferred_modes(connector, edid);
 
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 7493fd3..ce2212d 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -137,6 +137,25 @@  struct drm_scdc {
 struct drm_hdmi_info {
 	/** @scdc: sink's scdc support and capabilities */
 	struct drm_scdc scdc;
+
+	/**
+	 * @y420_vdb_modes: bitmap of modes which can support ycbcr420
+	 * output only (not normal RGB/YCBCR444/422 outputs). There are total
+	 * 107 VICs defined by CEA-861-F spec, so the size is 128 bits to map
+	 * upto 128 VICs;
+	 */
+	unsigned long y420_vdb_modes[BITS_TO_LONGS(128)];
+
+	/**
+	 * @y420_cmdb_modes: bitmap of modes which can support ycbcr420
+	 * output also, along with normal HDMI outputs. There are total 107
+	 * VICs defined by CEA-861-F spec, so the size is 128 bits to map upto
+	 * 128 VICs;
+	 */
+	unsigned long y420_cmdb_modes[BITS_TO_LONGS(128)];
+
+	/** @y420_cmdb_map: bitmap of SVD index, to extraxt vcb modes */
+	unsigned long y420_cmdb_map;
 };
 
 /**
@@ -200,6 +219,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<<3)
 
 	/**
 	 * @color_formats: HDMI Color formats, selects between RGB and YCrCb