Re: [PATCH v1 1/1] drm/i915: override LT's default Vswing / Pre-emphasis when requested

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] drm/i915/bios: missing VBT block 57 validation in intel_bios_get_lt_vspeo()
- [Medium] drm/i915/bios: 16-entry VSPEO table in intel_bios_get_lt_vspeo() warns on DP1.4
--

commit e0e2844e4f9495daf6ba15f7eba3dbda5ee9076e
Author: Michał Grzelak <michal.grzelak@linux.intel.com>

drm/i915: override LT's default Vswing / Pre-emphasis when requested

This commit adds Vswing / Pre-emphasis Override (VSPEO) support for LT PHYs
on NVL+ platforms by reading the requested override table from VBT block 57
in intel_bios_get_lt_vspeo() and hooking up xe3plpd_get_lt_buf_trans_override().

> diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c
> index 7b6a6f8c3cf19..3c1fc48ca2758 100644
> --- a/drivers/gpu/drm/i915/display/intel_bios.c
> +++ b/drivers/gpu/drm/i915/display/intel_bios.c
> @@ -3886,6 +3886,52 @@ validate_vspeo(const struct intel_bios_encoder_data *devdata, bool has_dp)
>  	return true;
>  }
>  
> +const struct intel_ddi_buf_trans *
> +intel_bios_get_lt_vspeo(const struct intel_bios_encoder_data *devdata,
> +			bool has_dp, bool is_uhbr, bool has_edp)
> +{
> +	struct intel_display *display = devdata->display;
> +	union intel_ddi_buf_trans_entry *entries = devdata->entries;
> +	struct intel_ddi_buf_trans *vspeo = devdata->vspeo;
> +	const u32 *tables = display->vbt.vspeo.tables;
> +	int num_columns = display->vbt.vspeo.num_columns;
> +	int num_rows = display->vbt.vspeo.num_rows;
> +	size_t offset = 0;
> +	int level, idx;
> +
> +	if (!validate_vspeo(devdata, has_dp))
> +		return NULL;
> +
> +	if (has_edp)
> +		idx = 0;
> +	else if (is_uhbr)
> +		idx = 2;
> +	else
> +		idx = 1;
> +
> +	offset += idx * num_rows * num_columns;
> +
> +	for (level = 0; level < num_rows; level++) {
> +		u32 main_cursor = tables[offset];
> +		u32 pre_cursor = tables[offset + 1];
> +		u32 post_cursor = tables[offset + 2];
> +		u32 txswing = tables[offset + 3];
> +		u32 txswing_level = tables[offset + 4];

[Severity: High]
Can this dereference an unpopulated or undersized VBT block 57 table in
intel_bios_get_lt_vspeo()?

If a child device sets child.use_vbt_vswing in VBT but BDB_VSWING_PREEMPH
(block 57) is absent, parse_vswing_preemph_override() returns early leaving
display->vbt.vspeo.tables as NULL and display->vbt.vspeo.num_rows as 0.
However, allocate_vswing_preemph_override() still allocates devdata->vspeo
and calls kzalloc_objs(*entries, 0), setting devdata->entries to
ZERO_SIZE_PTR and devdata->vspeo->num_entries to 0:

drivers/gpu/drm/i915/display/intel_bios.c:allocate_vswing_preemph_override() {
	int num_rows = devdata->display->vbt.vspeo.num_rows;
	...
	entries = kzalloc_objs(*entries, num_rows);
	if (!entries) {
		kfree(vspeo);
		return;
	}

	devdata->entries = entries;
	devdata->vspeo = vspeo;
	devdata->vspeo->num_entries = num_rows;
}

Because validate_vspeo() only checks devdata->vspeo and has_dp,
intel_bios_get_lt_vspeo() skips the loop when num_rows is 0 and returns vspeo
with entries = ZERO_SIZE_PTR and num_entries = 0 instead of returning NULL,
causing intel_lt_phy_set_signal_levels() to index off ZERO_SIZE_PTR.

Furthermore, when block 57 is present, bdb_blocks[] only enforces a minimum
size of sizeof(struct bdb_vswing_preemph), and neither
parse_vswing_preemph_override(), validate_vspeo(), nor
intel_bios_get_lt_vspeo() checks that idx < display->vbt.vspeo.num_tables,
num_columns >= 5, or that the block payload is large enough before reading
tables[offset] through tables[offset + 4].

Should parse_vswing_preemph_override() or validate_vspeo() verify that
display->vbt.vspeo.tables is present and that num_tables, num_columns, and
the block size cover the requested table entries?

> +
> +		entries[level].lt.main_cursor = main_cursor;
> +		entries[level].lt.pre_cursor = pre_cursor;
> +		entries[level].lt.post_cursor = post_cursor;
> +		entries[level].lt.txswing = txswing;
> +		entries[level].lt.txswing_level = txswing_level;
> +
> +		offset += num_columns;
> +	}
> +
> +	vspeo->entries = entries;
> +
> +	return vspeo;
> +}

[Severity: Medium]
Does returning vspeo without updating vspeo->num_entries trigger a
drm_WARN_ON in intel_ddi_dp_voltage_max() during non-UHBR eDP or DP 1.4 link
training?

On NVL+ (DISPLAY_VER(display) >= 14), vswing_preemph_num_rows() returns 16,
and allocate_vswing_preemph_override() initializes
devdata->vspeo->num_entries to 16. When xe3plpd_get_lt_buf_trans_override()
calls intel_bios_get_lt_vspeo() for a non-UHBR eDP (idx = 0) or DP 1.4
(idx = 1) link, vspeo->num_entries remains 16 (whereas the non-override
tables xe3plpd_lt_trans_edp and xe3plpd_lt_trans_dp14 have 10 entries).

During 8b/10b link training, intel_dp_get_lane_adjust_vswing_preemph() ->
intel_dp_phy_voltage_max() -> intel_ddi_dp_voltage_max() retrieves
n_entries = 16 from intel_ddi_buf_trans_get() and compares it against
ARRAY_SIZE(index_to_dp_signal_levels), which is 10:

drivers/gpu/drm/i915/display/intel_ddi.c:intel_ddi_dp_voltage_max() {
	...
	intel_ddi_buf_trans_get(encoder, crtc_state, &n_entries);

	if (drm_WARN_ON(display->drm, n_entries < 1))
		n_entries = 1;
	if (drm_WARN_ON(display->drm,
			n_entries > ARRAY_SIZE(index_to_dp_signal_levels)))
		n_entries = ARRAY_SIZE(index_to_dp_signal_levels);
}

Should intel_bios_get_lt_vspeo() set vspeo->num_entries to 10 when !is_uhbr
(and 16 when is_uhbr)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103001518.3478578-1-michal.grzelak@linux.intel.com?part=1