[PATCH] drm/display: hdmi: Do not read EDID on disconnected connectors

[PATCH] drm/display: hdmi: Do not read EDID on disconnected connectors

[Cristian Ciocaltea]

The recently introduced hotplug event handler in the HDMI Connector
framework attempts to unconditionally read the EDID data, leading to a
bunch of non-harmful, yet quite annoying DDC/I2C related errors being
reported.

Ensure the operation is performed only for connectors having the status
connected or unknown.

Fixes: ab716b74dc9d ("drm/display/hdmi: implement hotplug functions")
Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 drivers/gpu/drm/display/drm_hdmi_state_helper.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/display/drm_hdmi_state_helper.c b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
index 2691e8b3e480131ac6e4e4b74b24947be55694bd..8e4b30e09b53b84cfd36199d56db3221a00085b0 100644
--- a/drivers/gpu/drm/display/drm_hdmi_state_helper.c
+++ b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
@@ -786,8 +786,10 @@ drm_atomic_helper_connector_hdmi_update(struct drm_connector *connector,
 	const struct drm_edid *drm_edid;
 
 	if (status == connector_status_disconnected) {
+		drm_edid_connector_update(connector, NULL);
 		// TODO: also handle CEC and scramber, HDMI sink disconnected.
 		drm_connector_hdmi_audio_plugged_notify(connector, false);
+		return;
 	}
 
 	if (connector->hdmi.funcs->read_edid)

---
base-commit: 1854df7087be70ad54e24b2e308d7558ebea9f27
change-id: 20250110-hdmi-conn-edid-read-fix-178513c2b7ea

Re: [PATCH] drm/display: hdmi: Do not read EDID on disconnected connectors

[Dmitry Baryshkov]

On Sat, Jan 11, 2025 at 12:04:09AM +0200, Cristian Ciocaltea wrote:
> The recently introduced hotplug event handler in the HDMI Connector
> framework attempts to unconditionally read the EDID data, leading to a
> bunch of non-harmful, yet quite annoying DDC/I2C related errors being
> reported.
> 
> Ensure the operation is performed only for connectors having the status
> connected or unknown.
> 
> Fixes: ab716b74dc9d ("drm/display/hdmi: implement hotplug functions")
> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> ---
>  drivers/gpu/drm/display/drm_hdmi_state_helper.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/gpu/drm/display/drm_hdmi_state_helper.c b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
> index 2691e8b3e480131ac6e4e4b74b24947be55694bd..8e4b30e09b53b84cfd36199d56db3221a00085b0 100644
> --- a/drivers/gpu/drm/display/drm_hdmi_state_helper.c
> +++ b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
> @@ -786,8 +786,10 @@ drm_atomic_helper_connector_hdmi_update(struct drm_connector *connector,
>  	const struct drm_edid *drm_edid;
>  
>  	if (status == connector_status_disconnected) {
> +		drm_edid_connector_update(connector, NULL);
>  		// TODO: also handle CEC and scramber, HDMI sink disconnected.
>  		drm_connector_hdmi_audio_plugged_notify(connector, false);
> +		return;

I think, it should be other way around: plugged_notify before
drm_edid_connector_update(). At least that would follow current logic of
the function.

>  	}
>  
>  	if (connector->hdmi.funcs->read_edid)
> 
> ---
> base-commit: 1854df7087be70ad54e24b2e308d7558ebea9f27
> change-id: 20250110-hdmi-conn-edid-read-fix-178513c2b7ea
> 

-- 
With best wishes
Dmitry

Re: [PATCH] drm/display: hdmi: Do not read EDID on disconnected connectors

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1506 bytes --]

On Sat, Jan 11, 2025 at 12:04:09AM +0200, Cristian Ciocaltea wrote:
> The recently introduced hotplug event handler in the HDMI Connector
> framework attempts to unconditionally read the EDID data, leading to a
> bunch of non-harmful, yet quite annoying DDC/I2C related errors being
> reported.
> 
> Ensure the operation is performed only for connectors having the status
> connected or unknown.
> 
> Fixes: ab716b74dc9d ("drm/display/hdmi: implement hotplug functions")
> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> ---
>  drivers/gpu/drm/display/drm_hdmi_state_helper.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/gpu/drm/display/drm_hdmi_state_helper.c b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
> index 2691e8b3e480131ac6e4e4b74b24947be55694bd..8e4b30e09b53b84cfd36199d56db3221a00085b0 100644
> --- a/drivers/gpu/drm/display/drm_hdmi_state_helper.c
> +++ b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
> @@ -786,8 +786,10 @@ drm_atomic_helper_connector_hdmi_update(struct drm_connector *connector,
>  	const struct drm_edid *drm_edid;
>  
>  	if (status == connector_status_disconnected) {
> +		drm_edid_connector_update(connector, NULL);

Why is this needed? It's not mentionned in your commit log.

>  		// TODO: also handle CEC and scramber, HDMI sink disconnected.
>  		drm_connector_hdmi_audio_plugged_notify(connector, false);
> +		return;
>  	}
>  
>  	if (connector->hdmi.funcs->read_edid)

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH] drm/display: hdmi: Do not read EDID on disconnected connectors

[Cristian Ciocaltea]

Hi Dmitry,

On 1/13/25 11:16 AM, Dmitry Baryshkov wrote:
> On Sat, Jan 11, 2025 at 12:04:09AM +0200, Cristian Ciocaltea wrote:
>> The recently introduced hotplug event handler in the HDMI Connector
>> framework attempts to unconditionally read the EDID data, leading to a
>> bunch of non-harmful, yet quite annoying DDC/I2C related errors being
>> reported.
>>
>> Ensure the operation is performed only for connectors having the status
>> connected or unknown.
>>
>> Fixes: ab716b74dc9d ("drm/display/hdmi: implement hotplug functions")
>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
>> ---
>>  drivers/gpu/drm/display/drm_hdmi_state_helper.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/display/drm_hdmi_state_helper.c b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
>> index 2691e8b3e480131ac6e4e4b74b24947be55694bd..8e4b30e09b53b84cfd36199d56db3221a00085b0 100644
>> --- a/drivers/gpu/drm/display/drm_hdmi_state_helper.c
>> +++ b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
>> @@ -786,8 +786,10 @@ drm_atomic_helper_connector_hdmi_update(struct drm_connector *connector,
>>  	const struct drm_edid *drm_edid;
>>  
>>  	if (status == connector_status_disconnected) {
>> +		drm_edid_connector_update(connector, NULL);
>>  		// TODO: also handle CEC and scramber, HDMI sink disconnected.
>>  		drm_connector_hdmi_audio_plugged_notify(connector, false);
>> +		return;
> 
> I think, it should be other way around: plugged_notify before
> drm_edid_connector_update(). At least that would follow current logic of
> the function.

Yeah, I wasn't really sure about the order here. Will get this fixed in v2.

Thanks,
Cristian

Re: [PATCH] drm/display: hdmi: Do not read EDID on disconnected connectors

[Cristian Ciocaltea]

Hi Maxime,

On 1/13/25 11:35 AM, Maxime Ripard wrote:
> On Sat, Jan 11, 2025 at 12:04:09AM +0200, Cristian Ciocaltea wrote:
>> The recently introduced hotplug event handler in the HDMI Connector
>> framework attempts to unconditionally read the EDID data, leading to a
>> bunch of non-harmful, yet quite annoying DDC/I2C related errors being
>> reported.
>>
>> Ensure the operation is performed only for connectors having the status
>> connected or unknown.
>>
>> Fixes: ab716b74dc9d ("drm/display/hdmi: implement hotplug functions")
>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
>> ---
>>  drivers/gpu/drm/display/drm_hdmi_state_helper.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/display/drm_hdmi_state_helper.c b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
>> index 2691e8b3e480131ac6e4e4b74b24947be55694bd..8e4b30e09b53b84cfd36199d56db3221a00085b0 100644
>> --- a/drivers/gpu/drm/display/drm_hdmi_state_helper.c
>> +++ b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
>> @@ -786,8 +786,10 @@ drm_atomic_helper_connector_hdmi_update(struct drm_connector *connector,
>>  	const struct drm_edid *drm_edid;
>>  
>>  	if (status == connector_status_disconnected) {
>> +		drm_edid_connector_update(connector, NULL);
> 
> Why is this needed? It's not mentionned in your commit log.

The original implementation has it after reading the EDID, but I'm not
sure if we need the explicit reset in this case.

I was going to submit a new revision switching the order, as Dmitry
suggested, or should we simply drop it?

Thanks,
Cristian

> 
>>  		// TODO: also handle CEC and scramber, HDMI sink disconnected.
>>  		drm_connector_hdmi_audio_plugged_notify(connector, false);
>> +		return;
>>  	}
>>  
>>  	if (connector->hdmi.funcs->read_edid)
> 
> Maxime

Re: [PATCH] drm/display: hdmi: Do not read EDID on disconnected connectors

[Dmitry Baryshkov]

On Mon, 13 Jan 2025 at 14:00, Cristian Ciocaltea
<cristian.ciocaltea@collabora.com> wrote:
>
> Hi Maxime,
>
> On 1/13/25 11:35 AM, Maxime Ripard wrote:
> > On Sat, Jan 11, 2025 at 12:04:09AM +0200, Cristian Ciocaltea wrote:
> >> The recently introduced hotplug event handler in the HDMI Connector
> >> framework attempts to unconditionally read the EDID data, leading to a
> >> bunch of non-harmful, yet quite annoying DDC/I2C related errors being
> >> reported.
> >>
> >> Ensure the operation is performed only for connectors having the status
> >> connected or unknown.
> >>
> >> Fixes: ab716b74dc9d ("drm/display/hdmi: implement hotplug functions")
> >> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> >> ---
> >>  drivers/gpu/drm/display/drm_hdmi_state_helper.c | 2 ++
> >>  1 file changed, 2 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/display/drm_hdmi_state_helper.c b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
> >> index 2691e8b3e480131ac6e4e4b74b24947be55694bd..8e4b30e09b53b84cfd36199d56db3221a00085b0 100644
> >> --- a/drivers/gpu/drm/display/drm_hdmi_state_helper.c
> >> +++ b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
> >> @@ -786,8 +786,10 @@ drm_atomic_helper_connector_hdmi_update(struct drm_connector *connector,
> >>      const struct drm_edid *drm_edid;
> >>
> >>      if (status == connector_status_disconnected) {
> >> +            drm_edid_connector_update(connector, NULL);
> >
> > Why is this needed? It's not mentionned in your commit log.
>
> The original implementation has it after reading the EDID, but I'm not
> sure if we need the explicit reset in this case.
>
> I was going to submit a new revision switching the order, as Dmitry
> suggested, or should we simply drop it?

If the EDID is not available, it needs to be reset.

>
> Thanks,
> Cristian
>
> >
> >>              // TODO: also handle CEC and scramber, HDMI sink disconnected.
> >>              drm_connector_hdmi_audio_plugged_notify(connector, false);
> >> +            return;
> >>      }
> >>
> >>      if (connector->hdmi.funcs->read_edid)
> >
> > Maxime
>


-- 
With best wishes
Dmitry

Re: [PATCH] drm/display: hdmi: Do not read EDID on disconnected connectors

[Cristian Ciocaltea]

On 1/13/25 2:06 PM, Dmitry Baryshkov wrote:
> On Mon, 13 Jan 2025 at 14:00, Cristian Ciocaltea
> <cristian.ciocaltea@collabora.com> wrote:
>>
>> Hi Maxime,
>>
>> On 1/13/25 11:35 AM, Maxime Ripard wrote:
>>> On Sat, Jan 11, 2025 at 12:04:09AM +0200, Cristian Ciocaltea wrote:
>>>> The recently introduced hotplug event handler in the HDMI Connector
>>>> framework attempts to unconditionally read the EDID data, leading to a
>>>> bunch of non-harmful, yet quite annoying DDC/I2C related errors being
>>>> reported.
>>>>
>>>> Ensure the operation is performed only for connectors having the status
>>>> connected or unknown.
>>>>
>>>> Fixes: ab716b74dc9d ("drm/display/hdmi: implement hotplug functions")
>>>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
>>>> ---
>>>>  drivers/gpu/drm/display/drm_hdmi_state_helper.c | 2 ++
>>>>  1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/display/drm_hdmi_state_helper.c b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
>>>> index 2691e8b3e480131ac6e4e4b74b24947be55694bd..8e4b30e09b53b84cfd36199d56db3221a00085b0 100644
>>>> --- a/drivers/gpu/drm/display/drm_hdmi_state_helper.c
>>>> +++ b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
>>>> @@ -786,8 +786,10 @@ drm_atomic_helper_connector_hdmi_update(struct drm_connector *connector,
>>>>      const struct drm_edid *drm_edid;
>>>>
>>>>      if (status == connector_status_disconnected) {
>>>> +            drm_edid_connector_update(connector, NULL);
>>>
>>> Why is this needed? It's not mentionned in your commit log.
>>
>> The original implementation has it after reading the EDID, but I'm not
>> sure if we need the explicit reset in this case.
>>
>> I was going to submit a new revision switching the order, as Dmitry
>> suggested, or should we simply drop it?
> 
> If the EDID is not available, it needs to be reset.

Thanks for the confirmation - I will mention this in the commit description.

Regards,
Cristian