Re: [PATCH 04/11] drm: parse ycbcr420 vcb block

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

On Fri, Apr 07, 2017 at 07:39:21PM +0300, Shashank Sharma wrote:
> 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.
> 
> One of these new blocks is: ycbcr420 vcb
> - 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-vcb in the DRM layer.
> This block contains a bitmap about which mode, from among the list of
> normal svd videomodes, can support ycbcr420 output too.
> 
> So if bit 0 from first vcb byte is set, means first video mode in the
> svd list, can be supported in ycbcr420 output too. Bit 2 means second
> video mode from svd list, and so on.
> 
> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> Cc: Jose Abreu <joabreu@synopsys.com>
> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> ---
>  drivers/gpu/drm/drm_edid.c  | 80 +++++++++++++++++++++++++++++++++++++++++++--
>  include/drm/drm_connector.h |  1 +
>  2 files changed, 79 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 64d8e2e..d01b7df 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -2778,6 +2778,7 @@ add_detailed_modes(struct drm_connector *connector, struct edid *edid,
>  #define SPEAKER_BLOCK	0x04
>  #define VIDEO_CAPABILITY_BLOCK	0x07
>  #define VIDEO_DATA_BLOCK_420	0x0E
> +#define VIDEO_CAP_BLOCK_420	0x0F
>  #define EDID_BASIC_AUDIO	(1 << 6)
>  #define EDID_CEA_YCRCB444	(1 << 5)
>  #define EDID_CEA_YCRCB422	(1 << 4)
> @@ -3143,11 +3144,21 @@ static int
>  do_cea_modes(struct drm_connector *connector, const u8 *db, u8 len)
>  {
>  	int i, modes = 0;
> +	u64 hdmi_420_cap_map = connector->display_info.hdmi.ycbcr420_vcb_map;
>  
>  	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 such CEA modes in VDB, which can
> +			 * support ycbcr420 sampling output also.
> +			 * For example, if the bit 0 in bitmap is set,
> +			 * first mode in VDB can support ycbcr420 output too.
> +			 */
> +			if (hdmi_420_cap_map & (1 << i))

We'd need to make sure 'len' is <=64. Not sure if there's any point in
making it possible to have more than 64 VDBs. If there is, then the standard
bitops.h stuff could be used to make the map longer quite trivially.

Whatever limit we choose I think we should print some kind of warning
to indicate that we've exceeded whatever arbitrary limit we chose. And
I definitely think it should be a warning level at least to make sure
we get a bug report about it.

> +				mode->flags |= DRM_MODE_FLAG_420;
>  			drm_mode_probed_add(connector, mode);
>  			modes++;
>  		}
> @@ -3526,9 +3537,64 @@ static bool cea_db_is_hdmi_vdb420(const u8 *db)
>  	return true;
>  }
>  
> +static bool cea_db_is_hdmi_vcb420(const u8 *db)

To keep with the spec terminology this should probably be called
cea_db_is_y420cmdb(). Same for the define and other functions dealing
witrh this block.

> +{
> +	if (cea_db_tag(db) != VIDEO_CAPABILITY_BLOCK)
> +		return false;
> +

We need to make sure the payload is long enough to actuall contain the
extended tag.

> +	if (cea_db_extended_tag(db) != VIDEO_CAP_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_vcb_420_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 (!db)
> +		return;

Is that possible somehow?

> +
> +	if (map_len == 0) {
> +		/* All CEA modes support ycbcr420 sampling also.*/
> +		hdmi->ycbcr420_vcb_map = U64_MAX;
> +		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 (map_len > 8)
> +		map_len = 8;
> +
> +	for (count = 0; count < map_len; count++)
> +		map = (db[2 + count] << 8 * count) | map;
> +
> +	if (map) {
> +		DRM_DEBUG_KMS("Sink supports ycbcr 420\n");
> +		info->color_formats |= DRM_COLOR_FORMAT_YCRCB420;
> +	}
> +
> +	hdmi->ycbcr420_vcb_map = map;
> +}
> +
>  static int
>  add_cea_modes(struct drm_connector *connector, struct edid *edid)
>  {
> @@ -3561,6 +3627,8 @@ add_cea_modes(struct drm_connector *connector, struct edid *edid)
>  				/* Add 4:2:0(only) modes present in EDID */
>  				modes += do_420_vdb_modes(connector, vdb420,
>  							  vdb420_len);
> +			} else if (cea_db_is_hdmi_vcb420(db)) {
> +				drm_parse_vcb_420_bitmap(connector, db);
>  			}
>  		}
>  	}
> @@ -4241,6 +4309,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_hdmi_vcb420(db))
> +			drm_parse_vcb_420_bitmap(connector, db);
>  	}
>  }
>  
> @@ -4492,6 +4562,14 @@ int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid)
>  	num_modes += add_cvt_modes(connector, edid);
>  	num_modes += add_standard_modes(connector, edid);
>  	num_modes += add_established_modes(connector, edid);
> +
> +	/*
> +	 * CEA-861-F adds ycbcr capability map block, for HDMI 2.0 sinks.
> +	 * To avoid multiple parsing of same block, lets get the sink info
> +	 * before parsing CEA modes.
> +	 */
> +	drm_add_display_info(connector, edid);

Is there any reason why this can't be moved to be called before any
modes are added? Having it in the middle like this feels wrong.

> +
>  	num_modes += add_cea_modes(connector, edid);
>  	num_modes += add_alternate_cea_modes(connector, edid);
>  	num_modes += add_displayid_detailed_modes(connector, edid);
> @@ -4501,8 +4579,6 @@ int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid)
>  	if (quirks & (EDID_QUIRK_PREFER_LARGE_60 | EDID_QUIRK_PREFER_LARGE_75))
>  		edid_fixup_preferred(connector, quirks);
>  
> -	drm_add_display_info(connector, edid);
> -
>  	if (quirks & EDID_QUIRK_FORCE_6BPC)
>  		connector->display_info.bpc = 6;
>  
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index cef76b2..dbfa6a1 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -136,6 +136,7 @@ struct drm_scdc {
>  struct drm_hdmi_info {
>  	/** @scdc: sink's scdc support and capabilities */
>  	struct drm_scdc scdc;
> +	u64 ycbcr420_vcb_map;
>  };
>  
>  /**
> -- 
> 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