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

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

Regards

Shashank


On 5/8/2017 10:28 PM, Ville Syrjälä wrote:
> 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.
I know, among the monitors I have tested with (ACER and SAMSUNG) I have 
found only 1 or 2 modes in the capability map.
But as there is no limit of a 420_VBD block length from the spec, so 
thought it would be good to assume some safe limit.
>
> 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.
Sure, I can add a warning, for the cases where we cross 64 modes.
>> +				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.
Got it.
>> +{
>> +	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.
Dint I check that in the caller ? anyways, I will make sure there is a 
check.
>> +	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?
Bad, corrupt EDID extension from a bad monitor ;-) ?
>
>> +
>> +	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.
In fact I was also thinking about moving it to the start, due to the 
look and feel of the code, then though it would be better to have 
minimum changes in the routine.
But sure, I would be happy to move it.

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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx