Re: [PATCH] drm: edid: HDMI 2.0 HF-VSDB block parsing

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

On Wed, Aug 10, 2016 at 04:29:21PM +0100, Jose Abreu wrote:
> Adds parsing for HDMI 2.0 'HDMI Forum Vendor
> Specific Data Block'. This block is present in
> some HDMI 2.0 EDID's and gives information about
> scrambling support, SCDC, 3D Views, and others.
> 
> Parsed parameters are stored in drm_connector
> structure.
> 
> Signed-off-by: Jose Abreu <joabreu@synopsys.com>
> Cc: Carlos Palminha <palminha@synopsys.com>
> Cc: David Airlie <airlied@linux.ie>
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: dri-devel@lists.freedesktop.org
> Cc: linux-kernel@vger.kernel.org

Thierry not in cc? Is this based on his SCDC work [1] or did we have
multiple people implementing the same thing, partially at least?

[1] https://lists.freedesktop.org/archives/dri-devel/2015-September/090929.html

> ---
>  drivers/gpu/drm/drm_edid.c | 49 ++++++++++++++++++++++++++++++++++++++++++++++
>  include/drm/drm_crtc.h     | 12 ++++++++++++
>  include/drm/drm_edid.h     |  5 +++++
>  include/linux/hdmi.h       |  1 +
>  4 files changed, 67 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 7df26d4..bcacf11 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -3160,6 +3160,21 @@ static bool cea_db_is_hdmi_vsdb(const u8 *db)
>  	return hdmi_id == HDMI_IEEE_OUI;
>  }
>  
> +static bool cea_db_is_hdmi_hf_vsdb(const u8 *db)

IIRC I asked Thierry to spell out the hdmi forum to avoid having this
look like alphabet soup. But now that I've read the spec, I do see it
being referred as hdmi-hf there as well, so I suppose it's fine.

> +{
> +	int hdmi_id;
> +
> +	if (cea_db_tag(db) != VENDOR_BLOCK)
> +		return false;
> +
> +	if (cea_db_payload_len(db) < 7)
> +		return false;
> +
> +	hdmi_id = db[1] | (db[2] << 8) | (db[3] << 16);
> +
> +	return hdmi_id == HDMI_IEEE_OUI_HF;
> +}
> +
>  #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)
>  
> @@ -3287,6 +3302,37 @@ parse_hdmi_vsdb(struct drm_connector *connector, const u8 *db)
>  }
>  
>  static void
> +parse_hdmi_hf_vsdb(struct drm_connector *connector, const u8 *db)
> +{
> +	u8 len = cea_db_payload_len(db);
> +
> +	if (len < 7)
> +		return;
> +
> +	if (db[4] != 1)
> +		return; /* invalid version */
> +
> +	connector->max_tmds_char = db[5] * 5;

We already have the max_tmds_clock. Also I was just trying move some of
the junk out from the connector into display_info [2].

[2] https://lists.freedesktop.org/archives/dri-devel/2016-August/114634.html

> +	connector->scdc_present = db[6] & (1 << 7);
> +	connector->rr_capable = db[6] & (1 << 6);
> +	connector->flags_3d = db[6] & 0x7;
> +	connector->supports_scramble = connector->scdc_present &&
> +			(db[6] & (1 << 3));

Do we have actual users for all these? I don't like adding stuff into
core structures just for the purposes of immediately printing it out.

For the stereo flags, I guess someone would have to figure put how they
relate to the 3d flags in the other vsdb, and how to convert to drm mode
flags.

> +
> +	DRM_DEBUG_KMS("HDMI v2: max TMDS char %d, "
> +			"scdc %s, "
> +			"rr %s, "
> +			"3D flags 0x%x, "
> +			"scramble %s\n",
> +			connector->max_tmds_char,
> +			connector->scdc_present ? "available" : "not available",
> +			connector->rr_capable ? "capable" : "not capable",
> +			connector->flags_3d,
> +			connector->supports_scramble ?
> +				"supported" : "not supported");
> +}
> +
> +static void
>  monitor_name(struct detailed_timing *t, void *data)
>  {
>  	if (t->data.other_data.type == EDID_DETAIL_MONITOR_NAME)
> @@ -3403,6 +3449,9 @@ void drm_edid_to_eld(struct drm_connector *connector, struct edid *edid)
>  				/* HDMI Vendor-Specific Data Block */
>  				if (cea_db_is_hdmi_vsdb(db))
>  					parse_hdmi_vsdb(connector, db);
> +				/* HDMI Forum Vendor-Specific Data Block */
> +				else if (cea_db_is_hdmi_hf_vsdb(db))
> +					parse_hdmi_hf_vsdb(connector, db);

Wasn't there some priority scheme between these two? Or was it just
about the infoframes? I'd have to dig through the spec to find out.

>  				break;
>  			default:
>  				break;
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index b618b50..eca57d2 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -1279,6 +1279,11 @@ struct drm_encoder {
>   * @audio_latency: audio latency info from ELD, if found
>   * @null_edid_counter: track sinks that give us all zeros for the EDID
>   * @bad_edid_counter: track sinks that give us an EDID with invalid checksum
> + * @max_tmds_char: indicates the maximum TMDS Character Rate supported
> + * @scdc_present: when set the sink supports SCDC functionality
> + * @rr_capable: when set the sink is capable of initiating an SCDC read request
> + * @supports_scramble: when set the sink supports less than 340Mcsc scrambling
> + * @flags_3d: 3D view(s) supported by the sink, see drm_edid.h (DRM_EDID_3D_*)
>   * @edid_corrupt: indicates whether the last read EDID was corrupt
>   * @debugfs_entry: debugfs directory for this connector
>   * @state: current atomic state for this connector
> @@ -1377,6 +1382,13 @@ struct drm_connector {
>  	int null_edid_counter; /* needed to workaround some HW bugs where we get all 0s */
>  	unsigned bad_edid_counter;
>  
> +	/* EDID bits HDMI 2.0 */
> +	int max_tmds_char;	/* in Mcsc */
> +	bool scdc_present;
> +	bool rr_capable;
> +	bool supports_scramble;
> +	int flags_3d;
> +
>  	/* Flag for raw EDID header corruption - used in Displayport
>  	 * compliance testing - * Displayport Link CTS Core 1.2 rev1.1 4.2.2.6
>  	 */
> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> index 919933d..35d39f0 100644
> --- a/include/drm/drm_edid.h
> +++ b/include/drm/drm_edid.h
> @@ -266,6 +266,11 @@ struct detailed_timing {
>  
>  #define DRM_ELD_CEA_SAD(mnl, sad)	(20 + (mnl) + 3 * (sad))
>  
> +/* HDMI 2.0 */
> +#define DRM_EDID_3D_INDEPENDENT_VIEW	(1 << 2)
> +#define DRM_EDID_3D_DUAL_VIEW		(1 << 1)
> +#define DRM_EDID_3D_OSD_DISPARITY	(1 << 0)
> +
>  struct edid {
>  	u8 header[8];
>  	/* Vendor & product info */
> diff --git a/include/linux/hdmi.h b/include/linux/hdmi.h
> index e974420..6e23409 100644
> --- a/include/linux/hdmi.h
> +++ b/include/linux/hdmi.h
> @@ -35,6 +35,7 @@ enum hdmi_infoframe_type {
>  };
>  
>  #define HDMI_IEEE_OUI 0x000c03
> +#define HDMI_IEEE_OUI_HF	0xc45dd8
>  #define HDMI_INFOFRAME_HEADER_SIZE  4
>  #define HDMI_AVI_INFOFRAME_SIZE    13
>  #define HDMI_SPD_INFOFRAME_SIZE    25
> -- 
> 2.1.4
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel OTC