[Intel-gfx] [PATCH] drm: parse hf-vsdb

Sharma, Shashank shashank.sharma at intel.com
Mon Dec 19 14:19:39 UTC 2016


Thanks for the review, Thierry. My comments inline.

Regards

Shashank


On 12/5/2016 10:29 PM, Thierry Reding wrote:
> On Tue, Nov 29, 2016 at 08:24:39PM +0530, Shashank Sharma wrote:
>> HDMI 2.0 / CEA-861-F specs define a new CEA extension data block,
>> called hdmi-forum vendor specific data block (HF-VSDB). This block
>> contains information about sink's support for HDMI 2.0 compliant
>> features. These features are:
>>      - Deep color YUV 420 support and BPC
>>      - 3D flags for
>>          - OSD Displarity
>>          - Dual view signaling
>>          - independent view signaling
>>      - SCDC support
>>      - Max TMDS char rate
>>      - Scrambling support
>>
>> This patch adds a parser function for this block, and add flags to
>> indicate support for new features, in drm_display_info structure.
>>
>> Signed-off-by: Shashank Sharma <shashank.sharma at intel.com>
>> ---
>>   drivers/gpu/drm/drm_edid.c  | 73 +++++++++++++++++++++++++++++++++++++++++++++
>>   include/drm/drm_connector.h | 48 +++++++++++++++++++++++++++++
>>   include/drm/drm_edid.h      |  5 ++++
>>   include/linux/hdmi.h        |  1 +
>>   4 files changed, 127 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>> index 336be31..b18bfe0 100644
>> --- a/drivers/gpu/drm/drm_edid.c
>> +++ b/drivers/gpu/drm/drm_edid.c
>> @@ -3223,6 +3223,27 @@ static int add_3d_struct_modes(struct drm_connector *connector, u16 structure,
>>   	return 0;
>>   }
>>   
>> +static bool cea_db_is_hf_vsdb(const u8 *db)
>> +{
>> +	u8 len;
>> +	int hfvsdb_id;
>> +
>> +	if (cea_db_tag(db) != VENDOR_BLOCK)
>> +		return false;
>> +
>> +	len = cea_db_payload_len(db);
>> +	if (len < 7 || len > 31)
> The second part of the check is unnecessary because there is no way that
> cea_db_payload_len() will return a number larger than 31.
Agree. Will remove this check.
>> +		return false;
>> +
>> +	/* version should be 1 */
>> +	if (db[4]  != 1)
> There's an extra space before !=.
Oh, I am surprised that checkpatch dint complaint. Will check this out.
> Also I'm not sure this is something
> that we need to worry about. Future versions of the HF VSDB are likely
> to be backwards compatible, so this seems like an unnecessary
> restriction.
Agree.
>
>> +		return false;
>> +
>> +	hfvsdb_id = db[1] | (db[2] << 8) | (db[3] << 16);
>> +
>> +	return hfvsdb_id == HDMI_IEEE_OUI_HFVSDB;
>> +}
>> +
>>   static bool cea_db_is_hdmi_vsdb(const u8 *db)
>>   {
>>   	int hdmi_id;
>> @@ -3767,6 +3788,56 @@ bool drm_rgb_quant_range_selectable(struct edid *edid)
>>   }
>>   EXPORT_SYMBOL(drm_rgb_quant_range_selectable);
>>   
>> +static void drm_parse_yuv420_deep_color_info(struct drm_connector *connector,
>> +						const u8 *db)
>> +{
>> +	struct drm_display_info *info = &connector->display_info;
>> +
>> +	if (db[7] & DRM_EDID_YUV420_DC_48)
>> +		info->edid_yuv420_dc_modes |= DRM_EDID_YUV420_DC_48;
>> +	if (db[7] & DRM_EDID_YUV420_DC_36)
>> +		info->edid_yuv420_dc_modes |= DRM_EDID_YUV420_DC_36;
>> +	if (db[7] & DRM_EDID_YUV420_DC_30)
>> +		info->edid_yuv420_dc_modes |= DRM_EDID_YUV420_DC_30;
>> +
>> +	if (!info->edid_yuv420_dc_modes) {
>> +		DRM_DEBUG("%s: No YUV 420 deep color support in sink.\n",
>> +			  connector->name);
>> +		return;
>> +	}
>> +}
>> +
>> +static void
>> +drm_parse_hf_vsdb(struct drm_connector *connector, const u8 *db)
>> +{
>> +	struct drm_display_info *info = &connector->display_info;
>> +
>> +	if (db[5]) {
>> +		/*
>> +		 * If the sink supplies max tmds char rate in db,
>> +		 * the actual max tmds rate = db[5] * 5Mhz.
>> +		 */
>> +		info->max_tmds_clock = db[5] * 5000;
>> +		DRM_DEBUG_KMS("HF-VSDB: max TMDS clock %d kHz\n",
>> +		info->max_tmds_clock);
>> +	}
>> +
>> +	if (db[6] & DRM_HFVSDB_SCDC_SUPPORT)
>> +		info->scdc_supported = true;
>> +	if (db[6] & DRM_HFVSDB_SCDC_RR_CAP)
>> +		info->scdc_rr_cap = true;
>> +	if (db[6] & DRM_HFVSDB_SCRAMBLING)
>> +		info->scrambling = true;
>> +	if (db[6] & DRM_HFVSDB_INDEPENDENT_VIEW)
>> +		info->independent_view_3d = true;
>> +	if (db[6] & DRM_HFVSDB_DUAL_VIEW)
>> +		info->dual_view_3d = true;
>> +	if (db[6] & DRM_HFVSDB_3D_OSD)
>> +		info->osd_disparity_3d = true;
>> +
>> +	drm_parse_yuv420_deep_color_info(connector, db);
>> +}
>> +
>>   static void drm_parse_hdmi_deep_color_info(struct drm_connector *connector,
>>   					   const u8 *hdmi)
>>   {
>> @@ -3881,6 +3952,8 @@ static void drm_parse_cea_ext(struct drm_connector *connector,
>>   
>>   		if (cea_db_is_hdmi_vsdb(db))
>>   			drm_parse_hdmi_vsdb_video(connector, db);
>> +		if (cea_db_is_hf_vsdb(db))
>> +			drm_parse_hf_vsdb(connector, db);
>>   	}
>>   }
>>   
>> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
>> index 34f9741..d011dd5 100644
>> --- a/include/drm/drm_connector.h
>> +++ b/include/drm/drm_connector.h
>> @@ -167,6 +167,46 @@ struct drm_display_info {
>>   	 */
>>   	u32 bus_flags;
>>   
>> +#define DRM_HFVSDB_SCDC_SUPPORT	(1<<7)
>> +#define DRM_HFVSDB_SCDC_RR_CAP		(1<<6)
>> +#define DRM_HFVSDB_SCRAMBLING		(1<<3)
>> +#define DRM_HFVSDB_INDEPENDENT_VIEW	(1<<2)
>> +#define DRM_HFVSDB_DUAL_VIEW		(1<<1)
>> +#define DRM_HFVSDB_3D_OSD		(1<<0)
> This looks to me like the wrong place for these defines. They should
> probably go into include/drm/drm_edid.h instead.
I saw few other defines in this same structure, like 
DRM_COLOR_FORMAT_RGB444 etc, so thought this would
be the right place.
>
>> +
>> +	/**
>> +	 * @scdc_supported: Sink supports SCDC functionality.
>> +	 */
>> +	bool scdc_supported;
>> +
>> +	/**
>> +	 * @scdc_rr_cap: Sink has SCDC read request capability.
>> +	 */
>> +	bool scdc_rr_cap;
>> +
>> +	/**
>> +	 * @scrambling: Sync supports scrambling for <=340 Mcsc TMDS
>> +	 * char rates. Above 340 Mcsc rates, scrambling is always reqd.
>> +	 */
>> +	bool scrambling;
>> +
>> +	/**
>> +	 * @independent_view_3d: Sink supports 3d independent view signaling
>> +	 * in HF-VSIF.
>> +	 */
>> +	bool independent_view_3d;
>> +
>> +	/**
>> +	 * @dual_view_3d: Sink supports 3d dual view signaling in HF-VSIF.
>> +	 */
>> +	bool dual_view_3d;
>> +
>> +	/**
>> +	 * @osd_disparity_3d: Sink supports 3d osd disparity indication
>> +	 * in HF-VSIF.
>> +	 */
>> +	bool osd_disparity_3d;
>> +
>>   	/**
>>   	 * @max_tmds_clock: Maximum TMDS clock rate supported by the
>>   	 * sink in kHz. 0 means undefined.
>> @@ -185,6 +225,14 @@ struct drm_display_info {
>>   	u8 edid_hdmi_dc_modes;
>>   
>>   	/**
>> +	 * @edid_yuv420_dc_modes: bpc for deep color yuv420 encoding.
>> +	 * various sinks can support 10/12/16 bit per channel deep
>> +	 * color encoding. edid_yuv420_dc_modes = 0 means sink doesn't
>> +	 * support deep color yuv420 encoding.
>> +	 */
>> +	u8 edid_yuv420_dc_modes;
> Why not store the additional formats in the edid_hdmi_dc_modes field?
Now, as per our discussion in mail thread, I will create nested 
drm_hdmi_info within drm_display_info and add it there.
>
>> +
>> +	/**
>>   	 * @cea_rev: CEA revision of the HDMI sink.
>>   	 */
>>   	u8 cea_rev;
>> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
>> index 38eabf6..5df8b9c 100644
>> --- a/include/drm/drm_edid.h
>> +++ b/include/drm/drm_edid.h
>> @@ -212,6 +212,11 @@ struct detailed_timing {
>>   #define DRM_EDID_HDMI_DC_30               (1 << 4)
>>   #define DRM_EDID_HDMI_DC_Y444             (1 << 3)
>>   
>> +/* YUV 420 deep color modes */
>> +#define DRM_EDID_YUV420_DC_48		  (1 << 6)
>> +#define DRM_EDID_YUV420_DC_36		  (1 << 5)
>> +#define DRM_EDID_YUV420_DC_30		  (1 << 5)
> 36- and 30-bit depths have the same value. That probably wasn't
> intended.
Oops, thanks for pointing this out.
> Thierry

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/intel-gfx/attachments/20161219/b9038b63/attachment-0001.html>


More information about the Intel-gfx mailing list