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/dri-devel/attachments/20161219/b9038b63/attachment-0001.html>
