[PATCH] drm/edid: add CTA Video Format Data Block support

Jani Nikula jani.nikula at linux.intel.com
Wed Jul 31 08:36:39 UTC 2024


On Tue, 30 Jul 2024, Hamza Mahfooz <hamza.mahfooz at amd.com> wrote:
> Video Format Data Blocks (VFDBs) contain the necessary information that
> needs to be fed to the Optimized Video Timings (OVT) Algorithm.
> Also, we require OVT support to cover modes that aren't supported by
> earlier standards (e.g. CVT). So, parse all of the relevant VFDB data
> and feed it to the OVT Algorithm, to extract all of the missing OVT
> modes.

Is VFDB new to CTA-861-I? AFAICT the H version doesn't have it.

Is there any particular reason for the two step approach here? I mean
first allocating and storing the modes in drm_parse_cea_ext() and then
adding them in _drm_edid_connector_add_modes()? I think you could just
as well do everything in the latter, without the complications of
allocation. See e.g. add_cea_modes() which also iterates the CTA data
blocks. I think this would simplify everything considerably.

Please find some additional comments inline. I'll do more when I've got
hold of CTA-861-I.

BR,
Jani.

>
> Link: https://gitlab.freedesktop.org/drm/amd/-/issues/1442
> Suggested-by: Karol Herbst <kherbst at redhat.com>
> Signed-off-by: Hamza Mahfooz <hamza.mahfooz at amd.com>
> ---
>  drivers/gpu/drm/drm_edid.c  | 426 ++++++++++++++++++++++++++++++++++++
>  include/drm/drm_connector.h |  12 +
>  2 files changed, 438 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index f68a41eeb1fa..112a0070c4d5 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -31,6 +31,7 @@
>  #include <linux/bitfield.h>
>  #include <linux/byteorder/generic.h>
>  #include <linux/cec.h>
> +#include <linux/gcd.h>
>  #include <linux/hdmi.h>
>  #include <linux/i2c.h>
>  #include <linux/kernel.h>
> @@ -741,6 +742,93 @@ static const struct minimode extra_modes[] = {
>  	{ 2048, 1536, 60, 0 },
>  };
>  
> +struct cta_rid {
> +	u16 hactive;
> +	u16 vactive;
> +	u8 hratio;
> +	u8 vratio;
> +};
> +
> +/* CTA-861-I Table 11 - Resolution Identification (RID) */
> +static const struct cta_rid rids[] = {
> +	/* RID 0-9 */
> +	{ 0, 0, 0, 0 },
> +	{ 1280, 720, 16, 9 },
> +	{ 1280, 720, 64, 27 },
> +	{ 1680, 720, 64, 27 },
> +	{ 1920, 1080, 16, 9 },
> +	{ 1920, 1080, 64, 27 },
> +	{ 2560, 1080, 64, 27 },
> +	{ 3840, 1080, 32, 9 },
> +	{ 2560, 1440, 16, 9 },
> +	{ 3440, 1440, 64, 27 },
> +	/* RID 10-19 */
> +	{ 5120, 1440, 32, 9 },
> +	{ 3840, 2160, 16, 9 },
> +	{ 3840, 2160, 64, 27 },
> +	{ 5120, 2160, 64, 27 },
> +	{ 7680, 2160, 32, 9 },
> +	{ 5120, 2880, 16, 9 },
> +	{ 5120, 2880, 64, 27 },
> +	{ 6880, 2880, 64, 27 },
> +	{ 10240, 2880, 32, 9 },
> +	{ 7680, 4320, 16, 9 },
> +	/* RID 20-28 */
> +	{ 7680, 4320, 64, 27 },
> +	{ 10240, 4320, 64, 27 },
> +	{ 15360, 4320, 32, 9 },
> +	{ 11520, 6480, 16, 9 },
> +	{ 11520, 6480, 64, 27 },
> +	{ 15360, 6480, 64, 27 },
> +	{ 15360, 8640, 16, 9 },
> +	{ 15360, 8640, 64, 27 },
> +	{ 20480, 8640, 64, 27 },
> +};
> +
> +/* CTA-861-I Table 12 - AVI InfoFrame Video Format Frame Rate */
> +static const u16 cta_vf_fr[] = {
> +	/* Frame Rate 0-7 */
> +	0, 24, 25, 30, 48, 50, 60, 100,
> +	/* Frame Rate 8-15 */
> +	120, 144, 200, 240, 300, 360, 400, 480,
> +};
> +
> +/* CTA-861-I Table 13 - RID To VIC Mapping */
> +static const u8 rid_to_vic[][8] = {
> +	/* RID 0-9 */
> +	{},
> +	{ 60, 61, 62, 108, 19, 4, 41, 47 },
> +	{ 65, 66, 67, 109, 68, 69, 70, 71 },
> +	{ 79, 80, 81, 110, 82, 83, 84, 85 },
> +	{ 32, 33, 34, 111, 31, 16, 64, 63 },
> +	{ 72, 73, 74, 112, 75, 76, 77, 78 },
> +	{ 86, 87, 88, 113, 89, 90, 91, 92 },
> +	{},
> +	{},
> +	{},
> +	/* RID 10-19 */
> +	{},
> +	{ 93, 94, 95, 114, 96, 97, 117, 118 },
> +	{ 103, 104, 105, 116, 106, 107, 119, 120 },
> +	{ 121, 122, 123, 124, 125, 126, 127, 193 },
> +	{},
> +	{},
> +	{},
> +	{},
> +	{},
> +	{ 194, 195, 196, 197, 198, 199, 200, 201 },
> +	/* RID 20-28 */
> +	{ 202, 203, 204, 205, 206, 207, 208, 209 },
> +	{ 210, 211, 212, 213, 214, 215, 216, 217 },
> +	{},
> +	{},
> +	{},
> +	{},
> +	{},
> +	{},
> +	{},
> +};
> +
>  /*
>   * From CEA/CTA-861 spec.
>   *
> @@ -4140,6 +4228,7 @@ static int add_detailed_modes(struct drm_connector *connector,
>  #define CTA_DB_VIDEO			2
>  #define CTA_DB_VENDOR			3
>  #define CTA_DB_SPEAKER			4
> +#define CTA_DB_VIDEO_FORMAT		6
>  #define CTA_DB_EXTENDED_TAG		7
>  
>  /* CTA-861-H Table 62 - CTA Extended Tag Codes */
> @@ -4981,6 +5070,16 @@ struct cea_db {
>  	u8 data[];
>  } __packed;
>  
> +struct cta_vfd {
> +	u8 rid;
> +	u8 fr_fact;
> +	bool bfr50;
> +	bool fr24;
> +	bool bfr60;
> +	bool fr144;
> +	bool fr48;
> +};
> +
>  static int cea_db_tag(const struct cea_db *db)
>  {
>  	return db->tag_length >> 5;
> @@ -6018,6 +6117,307 @@ static void parse_cta_vdb(struct drm_connector *connector, const struct cea_db *
>  	}
>  }
>  
> +/* CTA-861 Video Format Descriptor (CTA VFD) */
> +static void parse_cta_vfd(const u8 *data, int vfd_len, struct cta_vfd *vfd)

It's customary for the destination parameter to be first.

> +{
> +	vfd->rid = data[0] & 0x3f;
> +	vfd->bfr50 = data[0] >> 7;

Looks like data[0] & 0x80. But then I don't have the spec yet.

> +	vfd->fr24 = !!(data[0] & 0x40);
> +	vfd->bfr60 = vfd_len > 1 ? (data[1] >> 7) : 0x1;

Ditto. Why shift if you're only interested in the highest bit?

> +	vfd->fr144 = vfd_len > 1 ? !!(data[1] & 0x40) : 0x0;
> +	vfd->fr_fact = vfd_len > 1 ? (data[1] & 0x3f) : 0x3;
> +	vfd->fr48 = vfd_len > 2 ? !!(data[2] & 0x1) : 0x0;

All the !!'s are unnecessary for bool assignment.

> +}
> +
> +static bool vfd_has_fr(const struct cta_vfd *vfd, int rate_idx)
> +{
> +	static const u8 factors[6] = {

Unnecessary explicit arrays size.

> +		1, 2, 4, 8, 12, 16
> +	};
> +	u16 rate = cta_vf_fr[rate_idx];
> +	u16 factor = 0;
> +	unsigned int i;
> +
> +	switch (rate) {
> +	case 24:
> +		return vfd->fr24;
> +	case 48:
> +		return vfd->fr48;
> +	case 144:
> +		return vfd->fr144;
> +	}
> +
> +	if (!(rate % 25)) {
> +		if (!vfd->bfr50)
> +			return false;
> +
> +		factor = rate / 25;
> +	} else if (!(rate % 30)) {
> +		if (!vfd->bfr60)
> +			return false;
> +
> +		factor = rate / 30;
> +	}
> +
> +	for (i = 0; i < ARRAY_SIZE(factors); i++)
> +		if (factor == factors[i] && (vfd->fr_fact & (1 << i)))
> +			return true;
> +
> +	return false;
> +}
> +
> +#define OVT_PIXEL_CLOCK_GRANULARITY	1000		/* Hz */
> +#define OVT_MIN_HTOTAL_GRANULARITY	8		/* pixels */
> +#define OVT_MIN_VBLANK_DURATION	460000000	/* ps */
> +#define OVT_MIN_VBLANK_LINES		20
> +#define OVT_MIN_VSYNC_LEADING_EDGE	400		/* us */
> +#define OVT_MIN_VSYNC_LE_LINES		14
> +#define OVT_MIN_CLOCK_RATE_420		590000000	/* Hz */
> +#define OVT_PIXEL_FACTOR_420		2
> +#define OVT_MIN_HBLANK_444		80		/* pixels */
> +#define OVT_MIN_HBLANK_420		128		/* pixels */
> +#define OVT_MAX_CHUNK_RATE		650000000	/* Hz */
> +#define OVT_AUDIO_PACKET_RATE		195000		/* Hz */
> +#define OVT_AUDIO_PACKET_SIZE		32
> +#define OVT_LINE_OVERHEAD		32
> +#define OVT_HSYNC_WIDTH		32
> +#define OVT_VSYNC_WIDTH		8
> +
> +/* OVT Algorthim as specified in CTA-861-I */
> +static struct drm_display_mode *calculate_ovt_mode(const struct cta_rid *rid,
> +						   u16 vrate,
> +						   struct drm_device *dev)

Please pass around struct drm_connector if you have it, and as the
context parameter it should be first.

> +{
> +	u32 max_audio_packets_per_line;
> +	struct drm_display_mode *mode;
> +	u32 htotal_granularity_chunk;
> +	u32 resolution_granularity;
> +	u32 vtotal_granularity = 1;
> +	u64 min_pixel_clock_rate;
> +	u32 htotal_granularity;
> +	u32 max_vrate = vrate;
> +	u64 pixel_clock_rate;
> +	u64 max_active_time;
> +	u64 min_resolution;
> +	u32 vsync_position;
> +	u32 min_line_time;
> +	u32 min_line_rate;
> +	u32 min_hblank;
> +	u32 min_htotal;
> +	u32 min_vblank;
> +	u32 min_vtotal;
> +	u32 htotal;
> +	u32 vtotal;
> +	u32 h;
> +	u64 r;
> +	u32 v;

There's something wrong with *any* function that has this many local
variables.

> +
> +	/* step 1 */
> +	switch (vrate) {
> +	case 24:
> +	case 25:
> +		max_vrate = 30;
> +		fallthrough;
> +	case 30:
> +		vtotal_granularity = 20;
> +		break;
> +	case 48:
> +	case 50:
> +		max_vrate = 60;
> +		fallthrough;
> +	case 60:
> +		vtotal_granularity = 20;
> +		break;
> +	case 100:
> +		max_vrate = 120;
> +		fallthrough;
> +	case 120:
> +		vtotal_granularity = 5;
> +		break;
> +	case 200:
> +		max_vrate = 240;
> +		fallthrough;
> +	case 240:
> +		vtotal_granularity = 5;
> +		break;
> +	case 300:
> +		max_vrate = 360;
> +		fallthrough;
> +	case 360:
> +		vtotal_granularity = 5;
> +		break;
> +	case 400:
> +		max_vrate = 480;
> +		fallthrough;
> +	case 480:
> +		vtotal_granularity = 5;
> +		break;
> +	}
> +
> +	/* step 2 */
> +	max_active_time = ((u64)1000000000000 / (u64)max_vrate) -
> +		(u64)OVT_MIN_VBLANK_DURATION;
> +
> +	min_line_time = max_active_time / (u64)rid->vactive;
> +
> +	min_vblank = max_t(u64, (u64)OVT_MIN_VBLANK_LINES,
> +			   DIV64_U64_ROUND_UP(OVT_MIN_VBLANK_DURATION,
> +					      min_line_time));
> +
> +	min_vtotal = rid->vactive + min_vblank;
> +
> +	if (min_vtotal % vtotal_granularity)
> +		min_vtotal += vtotal_granularity - (min_vtotal %
> +						    vtotal_granularity);
> +
> +	/* step 3 */
> +	min_line_rate = max_vrate * min_vtotal;
> +
> +	max_audio_packets_per_line = DIV_ROUND_UP(OVT_AUDIO_PACKET_RATE,
> +						  min_line_rate);
> +
> +	/* step 4 */
> +	min_hblank = OVT_LINE_OVERHEAD + OVT_AUDIO_PACKET_SIZE *
> +		max_audio_packets_per_line;
> +
> +	min_htotal = rid->hactive + max(OVT_MIN_HBLANK_444, min_hblank);
> +
> +	min_pixel_clock_rate = max_vrate * min_htotal * min_vtotal;
> +
> +	htotal_granularity_chunk =
> +		roundup_pow_of_two(DIV_ROUND_UP(min_pixel_clock_rate,
> +						OVT_MAX_CHUNK_RATE));
> +
> +	htotal_granularity = max(OVT_MIN_HTOTAL_GRANULARITY,
> +				 htotal_granularity_chunk);
> +
> +	if (min_htotal % htotal_granularity)
> +		min_htotal += htotal_granularity - (min_htotal %
> +						    htotal_granularity);
> +
> +	resolution_granularity = OVT_PIXEL_CLOCK_GRANULARITY /
> +		gcd(OVT_PIXEL_CLOCK_GRANULARITY, max_vrate);
> +
> +	for (;;) {
> +		/* step 5 */
> +		min_resolution = 0;
> +		v = min_vtotal;
> +
> +		for (;;) {

Not a fan of loops without clear loop conditions, let alone two of them
nested! It's really hard to figure out this is guaranteed to stop.

> +			h = min_htotal;
> +			r = (u64)h * (u64)v;
> +
> +			if (min_resolution && r > min_resolution)
> +				break;
> +
> +			while (r % resolution_granularity ||
> +			       max_vrate * r / (h & ~(h - 1)) >
> +			       OVT_MAX_CHUNK_RATE) {
> +				h += htotal_granularity;
> +				r = (u64)h * (u64)v;
> +			}
> +
> +			if (!min_resolution || r < min_resolution) {
> +				htotal = h;
> +				vtotal = v;
> +				min_resolution = r;
> +			}
> +
> +			v += vtotal_granularity;
> +		}
> +
> +		pixel_clock_rate = max_vrate * min_resolution;
> +
> +		/* step 6 */
> +		min_htotal = rid->hactive + max(OVT_MIN_HBLANK_420,
> +						OVT_PIXEL_FACTOR_420 *
> +						min_hblank);
> +		if (pixel_clock_rate >= OVT_MIN_CLOCK_RATE_420 &&
> +		    htotal < min_htotal)
> +			continue;
> +
> +		break;
> +	}
> +
> +	/* step 7 */
> +	vtotal = vtotal * max_vrate / vrate;
> +
> +	/* step 8 */
> +	vsync_position = max(OVT_MIN_VSYNC_LE_LINES,
> +			     DIV64_U64_ROUND_UP((u64)OVT_MIN_VSYNC_LE_LINES *
> +						(u64)pixel_clock_rate,
> +						(u64)htotal * (u64)1000000));
> +
> +	mode = drm_mode_create(dev);
> +
> +	if (!mode)
> +		return NULL;
> +
> +	mode->clock = pixel_clock_rate / 1000;
> +	mode->hdisplay = rid->hactive;
> +	mode->hsync_start = htotal - OVT_HSYNC_WIDTH * 2;
> +	mode->hsync_end = mode->hsync_start + OVT_HSYNC_WIDTH;
> +	mode->htotal = htotal;
> +
> +	mode->vdisplay = rid->vactive;
> +	mode->vsync_start = vtotal - vsync_position;
> +	mode->vsync_end = mode->vsync_start + OVT_VSYNC_WIDTH;
> +	mode->vtotal = vtotal;
> +
> +	return mode;
> +}
> +
> +/* CTA-861 Video Format Data Block (CTA VFDB) */
> +static void parse_cta_vfdb(struct drm_connector *connector,
> +			   const struct cea_db *db)
> +{
> +	struct drm_display_info *info = &connector->display_info;
> +	int vfdb_len = cea_db_payload_len(db);
> +	int vfd_len = (db->data[0] & 0x3) + 1;

What if payload len is 0?

> +	struct drm_display_mode **modes;
> +	struct drm_display_mode *mode;
> +	struct cta_vfd vfd;
> +	int mode_index = 0;
> +	int i;
> +	int j;
> +
> +	if (!(vfdb_len - 1) || (vfdb_len - 1) % vfd_len)
> +		return;

Better to check for vfd_len < some minimum.

I'd usually not require the modulo is zero, just take as many whole
vfd's as there are, and ignore the rest.

> +
> +	modes = krealloc_array(info->ovt_modes, ((vfdb_len - 1) / vfd_len) *
> +			       (ARRAY_SIZE(cta_vf_fr) - 1),
> +			       sizeof(*info->ovt_modes), GFP_KERNEL);
> +

I really hope we can get rid of this.

> +	if (!modes)
> +		return;
> +
> +	for (i = 1; i < vfdb_len; i += vfd_len) {
> +		parse_cta_vfd(&db->data[i], vfd_len, &vfd);
> +
> +		if (!vfd.rid || vfd.rid >= ARRAY_SIZE(rids))
> +			continue;
> +
> +		for (j = 1; j < ARRAY_SIZE(cta_vf_fr); j++) {
> +			if (!vfd_has_fr(&vfd, j) ||
> +			    (cta_vf_fr[j] < 144 && rid_to_vic[vfd.rid][j - 1]))
> +				continue;
> +
> +			mode = calculate_ovt_mode(&rids[vfd.rid], cta_vf_fr[j],
> +						  connector->dev);
> +
> +			if (!mode)
> +				continue;
> +
> +			mode->height_mm = info->height_mm;
> +			mode->width_mm = info->width_mm;
> +
> +			info->ovt_modes[mode_index++] = mode;
> +		}
> +	}
> +
> +	info->num_ovt_modes = mode_index;
> +}
> +
>  /*
>   * Update y420_cmdb_modes based on previously parsed CTA VDB and Y420CMDB.
>   *
> @@ -6439,6 +6839,8 @@ static void drm_parse_cea_ext(struct drm_connector *connector,
>  			parse_cta_vdb(connector, db);
>  		else if (cea_db_tag(db) == CTA_DB_AUDIO)
>  			info->has_audio = true;
> +		else if (cea_db_tag(db) == CTA_DB_VIDEO_FORMAT)
> +			parse_cta_vfdb(connector, db);
>  	}
>  	cea_db_iter_end(&iter);
>  
> @@ -6585,6 +6987,7 @@ static void drm_update_mso(struct drm_connector *connector,
>  static void drm_reset_display_info(struct drm_connector *connector)
>  {
>  	struct drm_display_info *info = &connector->display_info;
> +	int i;
>  
>  	info->width_mm = 0;
>  	info->height_mm = 0;
> @@ -6611,6 +7014,13 @@ static void drm_reset_display_info(struct drm_connector *connector)
>  	info->mso_pixel_overlap = 0;
>  	info->max_dsc_bpp = 0;
>  
> +	for (i = 0; i < info->num_ovt_modes; i++)
> +		drm_mode_destroy(connector->dev, info->ovt_modes[i]);
> +
> +	kfree(info->ovt_modes);
> +	info->ovt_modes = NULL;
> +	info->num_ovt_modes = 0;
> +

I really hope we can get rid of this.

>  	kfree(info->vics);
>  	info->vics = NULL;
>  	info->vics_len = 0;
> @@ -6849,6 +7259,21 @@ static int add_displayid_detailed_modes(struct drm_connector *connector,
>  	return num_modes;
>  }
>  
> +static int add_ovt_modes(struct drm_connector *connector)
> +{
> +	struct drm_display_info *info = &connector->display_info;
> +	int i;
> +
> +	for (i = 0; i < info->num_ovt_modes; i++) {
> +		drm_mode_probed_add(connector, info->ovt_modes[i]);
> +		info->ovt_modes[i] = NULL;
> +	}
> +
> +	info->num_ovt_modes = 0;
> +
> +	return i;
> +}
> +
>  static int _drm_edid_connector_add_modes(struct drm_connector *connector,
>  					 const struct drm_edid *drm_edid)
>  {
> @@ -6872,6 +7297,7 @@ static int _drm_edid_connector_add_modes(struct drm_connector *connector,
>  	 *
>  	 * XXX order for additional mode types in extension blocks?
>  	 */
> +	num_modes += add_ovt_modes(connector);

Why first?

>  	num_modes += add_detailed_modes(connector, drm_edid);
>  	num_modes += add_cvt_modes(connector, drm_edid);
>  	num_modes += add_standard_modes(connector, drm_edid);
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 5ad735253413..35b5eb344ea8 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -829,6 +829,18 @@ struct drm_display_info {
>  	 */
>  	u32 max_dsc_bpp;
>  
> +	/**
> +	 * @ovt_modes: Array of @num_ovt_modes OVT modes. Internal to EDID
> +	 * parsing.
> +	 */
> +	struct drm_display_mode **ovt_modes;
> +
> +	/**
> +	 * @num_ovt_modes: Number of elements in @ovt_modes. Internal to EDID
> +	 * parsing.
> +	 */
> +	int num_ovt_modes;
> +

I really hope we can get rid of this.

>  	/**
>  	 * @vics: Array of vics_len VICs. Internal to EDID parsing.
>  	 */

-- 
Jani Nikula, Intel


More information about the amd-gfx mailing list