Re: [PATCH v2 06/14] drm/edid: parse YCBCR420 videomodes from EDID

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Thu, Jul 13, 2017 at 09:03:12PM +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()
> 
> V5: Addressed review comments from Ville:
>     - Add the patch which cleans up the current EXTENDED_TAG usage
>     - Make y420_cmdb_map u64
>     - Do not block ycbcr420 modes while parsing the EDID, rather
>       add a separate helper function to prune ycbcr420-only modes from
>       connector's probed modes.
> 
> V6: Rebase
> V7: Move this patch after the 420_only validation patch (Ville)
> 
> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> ---
>  drivers/gpu/drm/drm_edid.c  | 148 +++++++++++++++++++++++++++++++++++++++++++-
>  include/drm/drm_connector.h |  12 ++++
>  2 files changed, 158 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 96eee5a..b86afb9 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -2783,6 +2783,8 @@ add_detailed_modes(struct drm_connector *connector, struct edid *edid,
>  #define SPEAKER_BLOCK	0x04
>  #define USE_EXTENDED_TAG 0x07
>  #define EXT_VIDEO_CAPABILITY_BLOCK 0x00
> +#define EXT_VIDEO_DATA_BLOCK_420	0x0E
> +#define EXT_VIDEO_CAP_BLOCK_Y420CMDB 0x0F
>  #define EDID_BASIC_AUDIO	(1 << 6)
>  #define EDID_CEA_YCRCB444	(1 << 5)
>  #define EDID_CEA_YCRCB422	(1 << 4)
> @@ -3155,15 +3157,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;
> +

Hmm. Looks like here we will need the drm_valid_cea_vic() check
because the vic is coming from an untrusted source.

> +		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;

And maybe here. It's not actually needed since we know it's valid
because drm_display_mode_from_vic_index() returned a mode, but since
that's not immediately obvious from here we might want to have the
check. But if you disagree feel free to leave it out.

At some point I think we might want to reorganize
drm_display_mode_from_vic_index() such that we actually pass it the
vic instead. Then we could also pass the vic here directly.

> +
> +	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 (hdmi->y420_cmdb_map & (1 << i))

I think this needs to be something like
if (i < 64 && hdmi->y420_cmdb_map & (1ULL << i))

With those changes I think this part should be good to go.

> +				drm_add_cmdb_modes(connector, db[i]);
> +
>  			drm_mode_probed_add(connector, mode);
>  			modes++;
>  		}
> @@ -3505,9 +3571,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) != USE_EXTENDED_TAG)
> +		return false;
> +
> +	if (!cea_db_payload_len(db))
> +		return false;
> +
> +	if (cea_db_extended_tag(db) != EXT_VIDEO_CAP_BLOCK_Y420CMDB)
> +		return false;
> +
> +	return true;
> +}
> +
> +static bool cea_db_is_y420vdb(const u8 *db)
> +{
> +	if (cea_db_tag(db) != USE_EXTENDED_TAG)
> +		return false;
> +
> +	if (!cea_db_payload_len(db))
> +		return false;
> +
> +	if (cea_db_extended_tag(db) != EXT_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)
>  {
> @@ -3530,10 +3665,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_y420vdb(db)) {
> +				const u8 *vdb420 = &db[2];
> +
> +				/* Add 4:2:0(only) modes present in EDID */
> +				modes += do_y420vdb_modes(connector,
> +							  vdb420,
> +							  dbl - 1);
>  			}
>  		}
>  	}
> @@ -4216,6 +4357,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);
>  	}
>  }
>  
> @@ -4477,6 +4620,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 26dd3eb..225e092 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -143,6 +143,17 @@ struct drm_hdmi_info {
>  	 * 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 */
> +	u64 y420_cmdb_map;
>  };
>  
>  /**
> @@ -206,6 +217,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ä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel