Hi Ankit,
On 8/31/2026 4:45 PM, Nautiyal, Ankit K wrote:
On 8/26/2026 8:54 PM, Naladala Ramanaidu wrote:
Normally the EDID is read over GMBUS, and GPIO bit-banging is only used
if that read fails. Now the debugfs flag is checked first. If it is set,
the DDC adapter is put in bit-banging mode before the read and put back
after it.
The flag is read once with READ_ONCE(), so the enable and the restore
always match even if the flag changes at the same time. The GMBUS retry
is not needed in this case, because the read already used GPIO.
Assisted-by: Claude:claude-opus-5
Signed-off-by: Naladala Ramanaidu <[email protected]>
---
drivers/gpu/drm/i915/display/intel_hdmi.c | 12 +++++++++++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c
b/drivers/gpu/drm/i915/display/intel_hdmi.c
index 38915f19d3e5..462ccfc840d4 100644
--- a/drivers/gpu/drm/i915/display/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
@@ -2555,13 +2555,23 @@ intel_hdmi_set_edid(struct drm_connector
*_connector)
struct i2c_adapter *ddc = connector->base.ddc;
struct ref_tracker *wakeref;
const struct drm_edid *drm_edid;
+ bool force_bit_banging = READ_ONCE(intel_hdmi->force_bit_banging);
Please drop the READ_ONCE(). No other display debugfs knob uses it.
force_dsc_en and family all are plain reads. Also, we can re-structure
the below calls to avoid using the local variable altogther.
Sure. I will address it in the next revision.
bool connected = false;
wakeref = intel_display_power_get(display, POWER_DOMAIN_GMBUS);
+ if (force_bit_banging) {
+ drm_dbg_kms(display->drm,
+ "[CONNECTOR:%d:%s] HDMI EDID read forced to GPIO
bit-banging\n",
+ connector->base.base.id, connector->base.name);
+ intel_gmbus_force_bit(ddc, true);
We can perhaps pull the force_bit/read/force_bit sequence into a helper:
static const struct drm_edid *
read_edid_with_gpio_bit_banging(struct intel_connector *connector)
{
struct i2c_adapter *ddc = connector->base.ddc;
const struct drm_edid *drm_edid;
intel_gmbus_force_bit(ddc, true);
drm_edid = drm_edid_read_ddc(&connector->base, ddc);
intel_gmbus_force_bit(ddc, false);
return drm_edid;
}
This part will then become:
if (intel_hdmi->force_bit_banging) {
drm_dbg_kms(...);
drm_edid = read_edid_with_gpio_bit_banging(connector);
} else {
drm_edid = drm_edid_read_ddc(&connector->base, ddc);
if (!drm_edid && !intel_gmbus_is_forced_bit(ddc)) {
drm_dbg_kms(...);
drm_edid = read_edid_with_gpio_bit_banging(connector);
}
}
This keeps force_bit(true) -> read edid -> force_bit(false) together
in one place, which is easier to follow.
It also covers the existing fallback below, which open-codes the same
three lines.
This also takes care of the concern in the commit message. The enable
and the restore sit in the same helper and no longer depend on the
flag, so they always
match on their own. We don't need READ_ONCE() to keep them in sync.
Sure. I will check and update in the next revision.
+ }
+
drm_edid = drm_edid_read_ddc(&connector->base, ddc);
- if (!drm_edid && !intel_gmbus_is_forced_bit(ddc)) {
+ if (force_bit_banging)
+ intel_gmbus_force_bit(ddc, false);
In case of force bit-banging failure, I was earlier thinking of
falling back to the original path.
However, on second thought it makes sense to let the case fail.
I think this should be documented in the commit message, and we can
drop the READ_ONCE() paragraph from it.
Since we are not falling back here, please also add a drm_dbg_kms()
when the forced read returns NULL. Otherwise the failure is silent.
Sure. I will check and address it in next revision.
Regards,
Ankit
+ else if (!drm_edid && !intel_gmbus_is_forced_bit(ddc)) {
drm_dbg_kms(display->drm,
"HDMI GMBUS EDID read failed, retry using GPIO
bit-banging\n");
intel_gmbus_force_bit(ddc, true);
