Re: [PATCH v1 2/2] drm/i915/hdmi: Read EDID over GPIO when the debugfs flag is set

["Nautiyal, Ankit K" <ankit.k.nautiyal@intel.com>]

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 <ramanaidu.naladala@intel.com>
> ---
>   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.

>   	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.




> +	}
> +
>   	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.

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);