Re: [PATCH v11 7/8] drm/i915: override Combo's VS/PE when requested

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] EHL PHY override logic incorrectly applies 'low_vswing' settings to non-low_vswing eDP panels
- [Medium] Potential out-of-bounds read during VBT 'tables' parsing
- [Medium] JSL PHY override logic is reversed for HBR2 vs lower port clocks compared to intended behavior
- [Medium] Memory allocated for `devdata->entries` is leaked on early return in vspeo retrieval functions
--

--- Patch [7]: [PATCH v11 7/8] drm/i915: override Combo's VS/PE when requested ---
commit 3f316b5929dec8d0ea8bb76e4cb8983db3b9bf70
Author: Michał Grzelak <[email protected]>
drm/i915: override Combo's VS/PE when requested

This commit adds accessor functions for the Combo PHY to read requested
VS/PE tables from the VBT. It parses the requested table and transforms
the data to override the port's buffer settings based on platform (EHL/JSL)
and port capabilities.

> diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c
> --- a/drivers/gpu/drm/i915/display/intel_bios.c
> +++ b/drivers/gpu/drm/i915/display/intel_bios.c

[ ... ]

> +const struct intel_ddi_buf_trans *
> +intel_bios_get_ehl_combo_vspeo(const struct intel_bios_encoder_data *devdata,
> +			       bool has_dp, int port_clock, bool has_edp)
> +{

[ ... ]

> +	if (!validate_vspeo(devdata, has_dp))
> +		return NULL;

[Severity: Medium]
Does this early return (and the similar one in intel_bios_get_jsl_combo_vspeo)
cause a memory leak of the devdata->entries buffer?

During teardown in intel_bios_driver_remove(), the driver attempts to free
this memory using kfree(devdata->vspeo->entries). Since vspeo->entries is
assigned much later in this function, returning NULL here leaves
devdata->vspeo->entries uninitialized or NULL. This would cause the teardown
code to skip freeing the memory allocated to devdata->entries.

> +
> +	if (has_edp)
> +		idx = port_clock > 540000 ? 2 : 1;
> +	else
> +		idx = port_clock > 270000 ? 1 : 0;

[Severity: High]
The commit message indicates that for EHL, eDP configurations using low
vswing should behave differently from eDP configurations not using low
vswing. However, does this logic unconditionally apply the same table index
logic to all eDP configurations?

It seems intel_bios_get_ehl_combo_vspeo() does not receive or check a
low_vswing parameter (like the JSL equivalent does), applying the same
fallback logic to all eDP panels.

> +
> +	offset += idx * num_rows * num_columns;
> +
> +	for (level = 0; level < num_rows; level++) {
> +		u32 dw2_swing_sel = tables[offset];
> +		u32 dw7_n_scalar = tables[offset + 1];
> +		u32 dw4_cursor_coeff = tables[offset + 2];
> +		u32 dw4_post_cursor_2 = tables[offset + 3];
> +		u32 dw4_post_cursor_1 = tables[offset + 4];

[Severity: Medium]
Can this loop read out of bounds if the VBT is malformed?

The parsing calculation offset = idx * num_rows * num_columns followed by
direct accesses up to tables[offset + 4] doesn't appear to validate that
num_columns is at least 5, or that the overall VBT tables buffer is
sized adequately to support the calculated offset and rows.

[ ... ]

> +const struct intel_ddi_buf_trans *
> +intel_bios_get_jsl_combo_vspeo(const struct intel_bios_encoder_data *devdata,
> +			       bool has_dp, int port_clock, bool low_vswing_edp)
> +{

[ ... ]

> +	if (low_vswing_edp) {
> +		if (port_clock > 540000)
> +			idx = 0;
> +		else if (port_clock > 270000)
> +			idx = 1;
> +		else
> +			idx = 2;
> +	}

[Severity: Medium]
Is this table selection logic reversed compared to the commit message?

The commit message states: "When encoder supports HBR2 choose 3rd table.
When encoder supports modes lower than HBR2 choose 2nd table."

However, the code assigns idx = 1 (2nd table) for port_clock > 270000 (HBR2)
and idx = 2 (3rd table) for lower modes.

[ ... ]

> diff --git a/drivers/gpu/drm/i915/display/intel_ddi_buf_trans.c b/drivers/gpu/drm/i915/display/intel_ddi_buf_trans.c
> --- a/drivers/gpu/drm/i915/display/intel_ddi_buf_trans.c
> +++ b/drivers/gpu/drm/i915/display/intel_ddi_buf_trans.c

[ ... ]

> +static const struct intel_ddi_buf_trans *
> +ehl_get_combo_buf_trans_override(struct intel_encoder *encoder,
> +				 const struct intel_crtc_state *crtc_state,
> +				 int *n_entries)
> +{
> +	const struct intel_bios_encoder_data *devdata = encoder->devdata;
> +	bool has_edp, has_dp;
> +	int port_clock;
> +
> +	has_edp = intel_crtc_has_type(crtc_state, INTEL_OUTPUT_EDP);
> +	has_dp = intel_crtc_has_dp_encoder(crtc_state);
> +	port_clock = crtc_state->port_clock;
> +
> +	return intel_bios_get_ehl_combo_vspeo(devdata, has_dp, port_clock, has_edp);
> +}

[Severity: High]
Should this override function be checking use_edp_low_vswing(encoder)
similar to how it is checked in jsl_get_combo_buf_trans_override()?

The current implementation omits the low_vswing context entirely, passing
only has_edp down to intel_bios_get_ehl_combo_vspeo(), which prevents the
distinction between low vswing and normal eDP configurations from being made.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=7