Re: [PATCH v1 1/2] drm/i915/hdmi: Add debugfs support to force EDID reads over GPIO

["Naladala, Ramanaidu" <Ramanaidu.naladala@intel.com>]

Hi Jani,

Thanks for the review.

On 8/31/2026 7:19 PM, Jani Nikula wrote:
> On Wed, 26 Aug 2026, Naladala Ramanaidu <ramanaidu.naladala@intel.com> wrote:
>> Add an i915_hdmi_force_bit_banging debugfs control for HDMI
>> connectors to allow EDID reads to be forced over GPIO bit-banging.
> The primary question the commit message *must* answer is *why*.
>
> It takes me under a minute to look at the patch and deduce the *what*,
> and even that does not match the commit message. There is no support for
> actually forcing anything here, it's just the non-functional debugfs
> being added.
>
> I still have no clue why.
Sure. I'll fold this 2 patches into single patch and i will address it 
in the next revision.
>
>> Assisted-by: Claude:claude-opus-5
>> Signed-off-by: Naladala Ramanaidu <ramanaidu.naladala@intel.com>
>> ---
>>   .../drm/i915/display/intel_display_debugfs.c  |  1 +
>>   .../drm/i915/display/intel_display_types.h    |  3 ++
>>   drivers/gpu/drm/i915/display/intel_hdmi.c     | 51 +++++++++++++++++++
>>   drivers/gpu/drm/i915/display/intel_hdmi.h     |  2 +
>>   4 files changed, 57 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_display_debugfs.c b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
>> index 3e302f23f247..c991f224b4be 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display_debugfs.c
>> +++ b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
>> @@ -1335,6 +1335,7 @@ void intel_connector_debugfs_add(struct intel_connector *connector)
>>   	intel_dp_link_training_debugfs_add(connector);
>>   	intel_dp_link_caps_debugfs_add(connector);
>>   	intel_link_bw_connector_debugfs_add(connector);
>> +	intel_hdmi_connector_debugfs_add(connector);
>>   
>>   	if (DISPLAY_VER(display) >= 11 &&
>>   	    ((connector_type == DRM_MODE_CONNECTOR_DisplayPort && !connector->mst.dp) ||
>> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
>> index 20a07ea06b5e..58983e31c5ef 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
>> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
>> @@ -1689,6 +1689,9 @@ struct intel_hdmi {
>>   	} dp_dual_mode;
>>   	struct intel_connector *attached_connector;
>>   	struct cec_notifier *cec_notifier;
>> +
>> +	/* Debugfs knob to force EDID reads over GPIO bit-banging. */
> Is that comment helpful?
>
>> +	bool force_bit_banging;
>>   };
>>   
>>   struct intel_dp_mst_encoder;
>> diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c
>> index 9b637e38a1a5..38915f19d3e5 100644
>> --- a/drivers/gpu/drm/i915/display/intel_hdmi.c
>> +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
>> @@ -26,6 +26,7 @@
>>    *	Jesse Barnes <jesse.barnes@intel.com>
>>    */
>>   
>> +#include <linux/debugfs.h>
>>   #include <linux/delay.h>
>>   #include <linux/hdmi.h>
>>   #include <linux/i2c.h>
>> @@ -3112,6 +3113,56 @@ void intel_infoframe_init(struct intel_digital_port *dig_port)
>>   	}
>>   }
>>   
>> +static int i915_hdmi_force_bit_banging_show(void *data, u64 *val)
> Please don't use i915_ naming.
I will drop the  i915_  prefix. I will rename it in the next revision.
>
>> +{
>> +	struct intel_connector *connector = to_intel_connector(data);
>> +
>> +	*val = READ_ONCE(intel_attached_hdmi(connector)->force_bit_banging);
>> +
>> +	return 0;
>> +}
>> +
>> +static int i915_hdmi_force_bit_banging_write(void *data, u64 val)
>> +{
>> +	struct intel_connector *connector = to_intel_connector(data);
>> +	struct intel_display *display = to_intel_display(connector);
>> +	struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector);
>> +
>> +	if (val > 1)
>> +		return -EINVAL;
>> +
>> +	drm_dbg_kms(display->drm,
>> +		    "[CONNECTOR:%d:%s] %sabling forced GPIO bit-banging for EDID reads\n",
>> +		    connector->base.base.id, connector->base.name,
>> +		    val ? "en" : "dis");
> I see this cute "%sabling" and val ? "en" : "dis" but it's just too
> clever for its own good. str_enable_disable() is close enough.
Sure Jani. I will fix this in next revision.
>
>> +
>> +	WRITE_ONCE(intel_hdmi->force_bit_banging, val);
>> +
>> +	return 0;
>> +}
>> +DEFINE_DEBUGFS_ATTRIBUTE(i915_hdmi_force_bit_banging_fops,
>> +			 i915_hdmi_force_bit_banging_show,
>> +			 i915_hdmi_force_bit_banging_write, "%llu\n");
>> +
>> +/**
>> + * intel_hdmi_connector_debugfs_add - add HDMI specific connector debugfs files
>> + * @connector: pointer to a registered intel_connector
>> + *
>> + * Cleanup will be done by drm_connector_unregister() through a call to
>> + * drm_debugfs_connector_remove().
>> + */
>> +void intel_hdmi_connector_debugfs_add(struct intel_connector *connector)
>> +{
>> +	struct dentry *root = connector->base.debugfs_entry;
>> +
>> +	if (connector->base.connector_type != DRM_MODE_CONNECTOR_HDMIA &&
>> +	    connector->base.connector_type != DRM_MODE_CONNECTOR_HDMIB)
>> +		return;
>> +
>> +	debugfs_create_file("i915_hdmi_force_bit_banging", 0644, root,
>> +			    connector, &i915_hdmi_force_bit_banging_fops);
> Please no new i915_ prefixed naming for shared display code. Please just
> use intel_.
>
>> +}
>> +
>>   bool intel_hdmi_init_connector(struct intel_digital_port *dig_port,
>>   			       struct intel_connector *intel_connector)
>>   {
>> diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.h b/drivers/gpu/drm/i915/display/intel_hdmi.h
>> index c95ed37bcc0c..752cccf5ccee 100644
>> --- a/drivers/gpu/drm/i915/display/intel_hdmi.h
>> +++ b/drivers/gpu/drm/i915/display/intel_hdmi.h
>> @@ -78,4 +78,6 @@ void intel_hdmi_poll_for_scrambling_enable(const struct intel_crtc_state *crtc_s
>>   int intel_hdmi_sink_max_frl_rate(struct drm_connector *connector);
>>   int intel_hdmi_sink_dsc_max_frl_rate(struct drm_connector *connector);
>>   
>> +void intel_hdmi_connector_debugfs_add(struct intel_connector *connector);
>> +
>>   #endif /* __INTEL_HDMI_H__ */