On Thu, Aug 13, 2026 at 04:50:16PM GMT, [email protected] wrote:
> 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.
>
Duplicate of memory leak from PATCH 4/8.
> > +
> > + 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.
Yes, this will be fixed in next version.
>
> > +
> > + 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.
Yes, this needs to be fixed as well.
>
> [ ... ]
>
> > 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.
Yes, since it is related to earlier comment from this email.
BR,
Michał
>
> --
> Sashiko AI review ·
> https://sashiko.dev/#/patchset/[email protected]?part=7
