Re: [PATCH 2/2] drm/edid: Parse the HDMI CEA block and look for 4k modes

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

On Wed, Aug 07, 2013 at 07:39:14PM +0100, Damien Lespiau wrote:
> HDMI 1.4 adds 4 "4k x 2k" modes in the the CEA vendor specific block.
> 
> With this commit, we now parse this block and expose the 4k modes that
> we find there.
> 
> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> Tested-by: Cancan Feng <cancan.feng@intel.com>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=67030
> ---
>  drivers/gpu/drm/drm_edid.c | 115 +++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 100 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 51342c4..b43d64f 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -931,6 +931,36 @@ static const struct drm_display_mode edid_cea_modes[] = {
>  	 .vrefresh = 100, },
>  };
>  
> +/*
> + * HDMI 1.4 4k modes.
> + */
> +static const struct drm_display_mode edid_4k_modes[] = {
> +	/* 1 - 3840x2160@30Hz */
> +	{ DRM_MODE("3840x2160", DRM_MODE_TYPE_DRIVER, 297000,
> +		   3840, 4016, 4104, 4400, 0,
> +		   2160, 2168, 2178, 2250, 0,
> +		   DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC),
> +	  .vrefresh = 30, },
> +	/* 2 - 3840x2160@25Hz */
> +	{ DRM_MODE("3840x2160", DRM_MODE_TYPE_DRIVER, 297000,
> +		   3840, 4896, 4984, 5280, 0,
> +		   2160, 2168, 2178, 2250, 0,
> +		   DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC),
> +	  .vrefresh = 25, },
> +	/* 3 - 3840x2160@24Hz */
> +	{ DRM_MODE("3840x2160", DRM_MODE_TYPE_DRIVER, 297000,
> +		   3840, 5116, 5204, 5500, 0,
> +		   2160, 2168, 2178, 2250, 0,
> +		   DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC),
> +	  .vrefresh = 24, },
> +	/* 4 - 4096x2160@24Hz (SMPTE) */
> +	{ DRM_MODE("3840x2160", DRM_MODE_TYPE_DRIVER, 297000,
                    ^^^^
> +		   4096, 5116, 5204, 5500, 0,
> +		   2160, 2168, 2178, 2250, 0,
> +		   DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC),
> +	  .vrefresh = 24, },

We should also add the 1000/1001 variants for #1 and #3. But we
don't want drm_match_cea_mode() to return the HDMI VIC, so a bit of
gymnastics are going to be required. I guess we want some kind of
drm_match_hdmi_mode() since we should also send the HDMI VIC in the
HDMI infoframe (if and when someone adds support for such a thing).

Also the mode #4 isn't supposed to have a 23.97Hz variant, so if we
want to utilize cea_mode_alternate_clock() for drm_match_hdmi_mode()
we need to add an exception there.

> +};
> +
>  /*** DDC fetch and block validation ***/
>  
>  static const u8 edid_header[] = {
> @@ -2465,6 +2495,59 @@ do_cea_modes(struct drm_connector *connector, u8 *db, u8 len)
>  	return modes;
>  }
>  
> +static int do_hdmi_vsdb_modes(struct drm_connector *connector, u8 *db, u8 len)
> +{

Could be 'const u8 *db'

> +	struct drm_device *dev = connector->dev;
> +	int modes = 0, offset = 0, i;
> +	u8 vic_len;
> +
> +	if (len < 8)
> +		goto out;

We skip the 'tag/len' byte for do_cea_modes(), but here we include it.
The resulting indexing is maybe a bit easier to compare w/ the spec,
but one must always keep in mind that the payload length doesn't include
byte 0. Not sure if we should just increase len by 1, or skip the tag
byte here too. Or maybe just add a comment explaining the situation.

> +
> +	/* no HDMI_Video_Present */
> +	if (!(db[8] & (1 << 5)))
> +		goto out;
> +
> +	/* Latency_Fields_Present */
> +	if (db[8] & (1 << 7))
> +		offset += 2;
> +
> +	/* I_Latency_Fields_Present */
> +	if (db[8] & (1 << 6))
> +		offset += 2;
> +

Maybe do the final 'offset += 2' already here? It would mean we could
say '8 + offset' below, which would somewhat nicely match the fact that
there are 8 fixed bytes in the payload, and the rest are shifty.

> +	/* the declared length is not long enough for the 2 first bytes
> +	 * of additional video format capabilities */
> +	if (len < (10 + offset))
> +		goto out;
> +
> +	vic_len =  db[10 + offset] >> 5;
> +	offset += 2;
> +
> +	for (i = 0; i < vic_len && len >= (9 + offset + i); i++) {
> +		struct drm_display_mode *newmode;
> +		u8 vic;
> +
> +		vic = db[9 + offset + i];
> +
> +		vic--; /* VICs start at 1 */
> +		if (vic >= ARRAY_SIZE(edid_4k_modes)) {
> +			DRM_ERROR("Unknow HDMI VIC: %d\n", vic);
> +			continue;
> +		}
> +
> +		newmode = drm_mode_duplicate(dev, &edid_4k_modes[vic]);
> +		if (!newmode)
> +			continue;
> +
> +		drm_mode_probed_add(connector, newmode);
> +		modes++;
> +	}
> +
> +out:
> +	return modes;
> +}
> +
>  static int
>  cea_db_payload_len(const u8 *db)
>  {
> @@ -2496,6 +2579,21 @@ cea_db_offsets(const u8 *cea, int *start, int *end)
>  	return 0;
>  }
>  
> +static bool cea_db_is_hdmi_vsdb(const u8 *db)
> +{
> +	int hdmi_id;
> +
> +	if (cea_db_tag(db) != VENDOR_BLOCK)
> +		return false;
> +
> +	if (cea_db_payload_len(db) < 5)
> +		return false;
> +
> +	hdmi_id = db[1] | (db[2] << 8) | (db[3] << 16);
> +
> +	return hdmi_id == HDMI_IDENTIFIER;
> +}
> +
>  #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)
>  
> @@ -2518,6 +2616,8 @@ add_cea_modes(struct drm_connector *connector, struct edid *edid)
>  
>  			if (cea_db_tag(db) == VIDEO_BLOCK)
>  				modes += do_cea_modes(connector, db + 1, dbl);
> +			else if (cea_db_is_hdmi_vsdb(db))
> +				modes += do_hdmi_vsdb_modes(connector, db, dbl);

>  		}
>  	}
>  
> @@ -2570,21 +2670,6 @@ monitor_name(struct detailed_timing *t, void *data)
>  		*(u8 **)data = t->data.other_data.data.str.str;
>  }
>  
> -static bool cea_db_is_hdmi_vsdb(const u8 *db)
> -{
> -	int hdmi_id;
> -
> -	if (cea_db_tag(db) != VENDOR_BLOCK)
> -		return false;
> -
> -	if (cea_db_payload_len(db) < 5)
> -		return false;
> -
> -	hdmi_id = db[1] | (db[2] << 8) | (db[3] << 16);
> -
> -	return hdmi_id == HDMI_IDENTIFIER;
> -}
> -
>  /**
>   * drm_edid_to_eld - build ELD from EDID
>   * @connector: connector corresponding to the HDMI/DP sink
> -- 
> 1.8.3.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel OTC