<html> <head> <meta content="text/html; charset=windows-1252" http-equiv="Content-Type"> </head> <body bgcolor="#FFFFFF" text="#000000"> <p>Thanks for the review, Thierry. My comments inline. <br> </p> <p>Regards</p> <p>Shashank<br> </p> <br> <div class="moz-cite-prefix">On 12/5/2016 10:29 PM, Thierry Reding wrote:<br> </div> <blockquote cite="mid:20161205165936.GC21732@ulmo.ba.sec" type="cite"> <pre wrap="">On Tue, Nov 29, 2016 at 08:24:39PM +0530, Shashank Sharma wrote: </pre> <blockquote type="cite"> <pre wrap="">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 <a class="moz-txt-link-rfc2396E" href="mailto:shashank.sharma@intel.com"><shashank.sharma@intel.com></a> --- 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) </pre> </blockquote> <pre wrap=""> 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. </pre> </blockquote> Agree. Will remove this check. <br> <blockquote cite="mid:20161205165936.GC21732@ulmo.ba.sec" type="cite"> <pre wrap=""> </pre> <blockquote type="cite"> <pre wrap="">+ return false; + + /* version should be 1 */ + if (db[4] != 1) </pre> </blockquote> <pre wrap=""> There's an extra space before !=. </pre> </blockquote> Oh, I am surprised that checkpatch dint complaint. Will check this out. <br> <blockquote cite="mid:20161205165936.GC21732@ulmo.ba.sec" type="cite"> <pre wrap="">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.</pre> </blockquote> Agree. <br> <blockquote cite="mid:20161205165936.GC21732@ulmo.ba.sec" type="cite"> <pre wrap=""> </pre> <blockquote type="cite"> <pre wrap="">+ 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) </pre> </blockquote> <pre wrap=""> This looks to me like the wrong place for these defines. They should probably go into include/drm/drm_edid.h instead.</pre> </blockquote> I saw few other defines in this same structure, like DRM_COLOR_FORMAT_RGB444 etc, so thought this would<br> be the right place. <br> <blockquote cite="mid:20161205165936.GC21732@ulmo.ba.sec" type="cite"> <pre wrap=""> </pre> <blockquote type="cite"> <pre wrap="">+ + /** + * @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; </pre> </blockquote> <pre wrap=""> Why not store the additional formats in the edid_hdmi_dc_modes field?</pre> </blockquote> Now, as per our discussion in mail thread, I will create <meta http-equiv="Content-Type" content="text/html; charset=windows-1252"> <span style="font-size:11.0pt;font-family:"Calibri",sans-serif; mso-ascii-theme-font:minor-latin;mso-fareast-font-family:Calibri;mso-fareast-theme-font: minor-latin;mso-hansi-theme-font:minor-latin;mso-bidi-font-family:"Times New Roman"; mso-bidi-theme-font:minor-bidi;mso-ansi-language:EN-US;mso-fareast-language: EN-US;mso-bidi-language:AR-SA">nested drm_hdmi_info within drm_display_info and add it there. </span> <meta name="ProgId" content="Word.Document"> <meta name="Generator" content="Microsoft Word 15"> <meta name="Originator" content="Microsoft Word 15"> <link rel="File-List" href="file:///C:%5CUsers%5CSHARMASH%5CAppData%5CLocal%5CTemp%5Cmsohtmlclip1%5C01%5Cclip_filelist.xml">  <link rel="themeData" href="file:///C:%5CUsers%5CSHARMASH%5CAppData%5CLocal%5CTemp%5Cmsohtmlclip1%5C01%5Cclip_themedata.thmx"> <link rel="colorSchemeMapping" href="file:///C:%5CUsers%5CSHARMASH%5CAppData%5CLocal%5CTemp%5Cmsohtmlclip1%5C01%5Cclip_colorschememapping.xml">  <style>  </style> <blockquote cite="mid:20161205165936.GC21732@ulmo.ba.sec" type="cite"> <pre wrap=""> </pre> <blockquote type="cite"> <pre wrap="">+ + /** * @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) </pre> </blockquote> <pre wrap=""> 36- and 30-bit depths have the same value. That probably wasn't intended. </pre> </blockquote> Oops, thanks for pointing this out. <br> <blockquote cite="mid:20161205165936.GC21732@ulmo.ba.sec" type="cite"> <pre wrap=""> Thierry </pre> </blockquote> <br> </body> </html>