Replace drm_detect_hdmi_monitor() with drm_display_info.is_hdmi since
monitor HDMI information is available after EDID is parsed. Additionally
rewrite the code the code to have fewer indentation levels.

Signed-off-by: Tejas Vipin <tejasvipin76@gmail.com>
---
Changes in v2:
    - Use drm_edid instead of edid

Link to v1: https://lore.kernel.org/all/20240910051856.700210-1-tejasvipin76@gmail.com/
---
 drivers/gpu/drm/gma500/cdv_intel_hdmi.c | 24 +++++++++++++-----------
 1 file changed, 13 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/gma500/cdv_intel_hdmi.c b/drivers/gpu/drm/gma500/cdv_intel_hdmi.c
index 2d95e0471291..701f8bbd5f2b 100644
--- a/drivers/gpu/drm/gma500/cdv_intel_hdmi.c
+++ b/drivers/gpu/drm/gma500/cdv_intel_hdmi.c
@@ -128,23 +128,25 @@ static enum drm_connector_status cdv_hdmi_detect(
 {
 	struct gma_encoder *gma_encoder = gma_attached_encoder(connector);
 	struct mid_intel_hdmi_priv *hdmi_priv = gma_encoder->dev_priv;
-	struct edid *edid = NULL;
+	const struct drm_edid *drm_edid;
+	int ret;
 	enum drm_connector_status status = connector_status_disconnected;
 
-	edid = drm_get_edid(connector, connector->ddc);
+	drm_edid = drm_edid_read_ddc(connector, connector->ddc);
+	ret = drm_edid_connector_update(connector, drm_edid);
 
 	hdmi_priv->has_hdmi_sink = false;
 	hdmi_priv->has_hdmi_audio = false;
-	if (edid) {
-		if (edid->input & DRM_EDID_INPUT_DIGITAL) {
-			status = connector_status_connected;
-			hdmi_priv->has_hdmi_sink =
-						drm_detect_hdmi_monitor(edid);
-			hdmi_priv->has_hdmi_audio =
-						drm_detect_monitor_audio(edid);
-		}
-		kfree(edid);
+	if (ret)
+		return status;
+
+	if (drm_edid_is_digital(drm_edid)) {
+		status = connector_status_connected;
+		hdmi_priv->has_hdmi_sink = connector->display_info.is_hdmi;
+		hdmi_priv->has_hdmi_audio = connector->display_info.has_audio;
 	}
+	drm_edid_free(drm_edid);
+
 	return status;
 }
 
-- 
2.46.0

Hi

Am 11.09.24 um 20:06 schrieb Tejas Vipin:
> Replace drm_detect_hdmi_monitor() with drm_display_info.is_hdmi since
> monitor HDMI information is available after EDID is parsed. Additionally
> rewrite the code the code to have fewer indentation levels.

The problem is that the entire logic is outdated. The content 
of cdv_hdmi_detect() should go into cdv_hdmi_get_modes(), the detect_ctx 
callback should be set to drm_connector_helper_detect_from_ddc() and 
cdv_hdmi_detect() should be deleted. The result is that ->detect_ctx 
will detect the presence of a display and ->get_modes will update EDID 
and other properties.

Do you have  a device for testing such a change?

Best regards
Thomas

>
> Signed-off-by: Tejas Vipin <tejasvipin76@gmail.com>
> ---
> Changes in v2:
>      - Use drm_edid instead of edid
>
> Link to v1: https://lore.kernel.org/all/20240910051856.700210-1-tejasvipin76@gmail.com/
> ---
>   drivers/gpu/drm/gma500/cdv_intel_hdmi.c | 24 +++++++++++++-----------
>   1 file changed, 13 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/gma500/cdv_intel_hdmi.c b/drivers/gpu/drm/gma500/cdv_intel_hdmi.c
> index 2d95e0471291..701f8bbd5f2b 100644
> --- a/drivers/gpu/drm/gma500/cdv_intel_hdmi.c
> +++ b/drivers/gpu/drm/gma500/cdv_intel_hdmi.c
> @@ -128,23 +128,25 @@ static enum drm_connector_status cdv_hdmi_detect(
>   {
>   	struct gma_encoder *gma_encoder = gma_attached_encoder(connector);
>   	struct mid_intel_hdmi_priv *hdmi_priv = gma_encoder->dev_priv;
> -	struct edid *edid = NULL;
> +	const struct drm_edid *drm_edid;
> +	int ret;
>   	enum drm_connector_status status = connector_status_disconnected;
>   
> -	edid = drm_get_edid(connector, connector->ddc);
> +	drm_edid = drm_edid_read_ddc(connector, connector->ddc);
> +	ret = drm_edid_connector_update(connector, drm_edid);
>   
>   	hdmi_priv->has_hdmi_sink = false;
>   	hdmi_priv->has_hdmi_audio = false;
> -	if (edid) {
> -		if (edid->input & DRM_EDID_INPUT_DIGITAL) {
> -			status = connector_status_connected;
> -			hdmi_priv->has_hdmi_sink =
> -						drm_detect_hdmi_monitor(edid);
> -			hdmi_priv->has_hdmi_audio =
> -						drm_detect_monitor_audio(edid);
> -		}
> -		kfree(edid);
> +	if (ret)
> +		return status;
> +
> +	if (drm_edid_is_digital(drm_edid)) {
> +		status = connector_status_connected;
> +		hdmi_priv->has_hdmi_sink = connector->display_info.is_hdmi;
> +		hdmi_priv->has_hdmi_audio = connector->display_info.has_audio;
>   	}
> +	drm_edid_free(drm_edid);
> +
>   	return status;
>   }
>   

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

On 9/12/24 12:49 PM, Thomas Zimmermann wrote:
> Hi
> 
> Am 11.09.24 um 20:06 schrieb Tejas Vipin:
>> Replace drm_detect_hdmi_monitor() with drm_display_info.is_hdmi since
>> monitor HDMI information is available after EDID is parsed. Additionally
>> rewrite the code the code to have fewer indentation levels.
> 
> The problem is that the entire logic is outdated. The content of cdv_hdmi_detect() should go into cdv_hdmi_get_modes(), the detect_ctx callback should be set to drm_connector_helper_detect_from_ddc() and cdv_hdmi_detect() should be deleted. The result is that ->detect_ctx will detect the presence of a display and ->get_modes will update EDID and other properties.
> 
> Do you have  a device for testing such a change?
> 
> Best regards
> Thomas

I do not have a device to test this. Reading the rest of the series and
given my circumstances, I do not think I will be continuing with this
patch.

-- 
Tejas Vipin

On Thu, 12 Sep 2024, Tejas Vipin <tejasvipin76@gmail.com> wrote:
> On 9/12/24 12:49 PM, Thomas Zimmermann wrote:
>> Hi
>> 
>> Am 11.09.24 um 20:06 schrieb Tejas Vipin:
>>> Replace drm_detect_hdmi_monitor() with drm_display_info.is_hdmi since
>>> monitor HDMI information is available after EDID is parsed. Additionally
>>> rewrite the code the code to have fewer indentation levels.
>> 
>> The problem is that the entire logic is outdated. The content of cdv_hdmi_detect() should go into cdv_hdmi_get_modes(), the detect_ctx callback should be set to drm_connector_helper_detect_from_ddc() and cdv_hdmi_detect() should be deleted. The result is that ->detect_ctx will detect the presence of a display and ->get_modes will update EDID and other properties.
>> 
>> Do you have  a device for testing such a change?
>> 
>> Best regards
>> Thomas
>
> I do not have a device to test this. Reading the rest of the series and
> given my circumstances, I do not think I will be continuing with this
> patch.

*sad trombone*

I think we could've made concrete incremental positive changes here,
without changing everything about detect and get_modes.

BR,
Jani.



-- 
Jani Nikula, Intel

On Thu, 12 Sep 2024, Thomas Zimmermann <tzimmermann@suse.de> wrote:
> Hi
>
> Am 11.09.24 um 20:06 schrieb Tejas Vipin:
>> Replace drm_detect_hdmi_monitor() with drm_display_info.is_hdmi since
>> monitor HDMI information is available after EDID is parsed. Additionally
>> rewrite the code the code to have fewer indentation levels.
>
> The problem is that the entire logic is outdated. The content 
> of cdv_hdmi_detect() should go into cdv_hdmi_get_modes(), the detect_ctx 
> callback should be set to drm_connector_helper_detect_from_ddc() and 
> cdv_hdmi_detect() should be deleted. The result is that ->detect_ctx 
> will detect the presence of a display and ->get_modes will update EDID 
> and other properties.

I guess I didn't get the memo on this one.

What's the problem with reading the EDID at detect? The subsequent
drm_edid_connector_add_modes() called from .get_modes() does not need to
read the EDID again.

I think it should be fine to do incremental refactors like the patch at
hand (modulo some issues I mention below).

BR,
Jani.


>
> Do you have  a device for testing such a change?
>
> Best regards
> Thomas
>
>>
>> Signed-off-by: Tejas Vipin <tejasvipin76@gmail.com>
>> ---
>> Changes in v2:
>>      - Use drm_edid instead of edid
>>
>> Link to v1: https://lore.kernel.org/all/20240910051856.700210-1-tejasvipin76@gmail.com/
>> ---
>>   drivers/gpu/drm/gma500/cdv_intel_hdmi.c | 24 +++++++++++++-----------
>>   1 file changed, 13 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/gma500/cdv_intel_hdmi.c b/drivers/gpu/drm/gma500/cdv_intel_hdmi.c
>> index 2d95e0471291..701f8bbd5f2b 100644
>> --- a/drivers/gpu/drm/gma500/cdv_intel_hdmi.c
>> +++ b/drivers/gpu/drm/gma500/cdv_intel_hdmi.c
>> @@ -128,23 +128,25 @@ static enum drm_connector_status cdv_hdmi_detect(
>>   {
>>   	struct gma_encoder *gma_encoder = gma_attached_encoder(connector);
>>   	struct mid_intel_hdmi_priv *hdmi_priv = gma_encoder->dev_priv;
>> -	struct edid *edid = NULL;
>> +	const struct drm_edid *drm_edid;
>> +	int ret;
>>   	enum drm_connector_status status = connector_status_disconnected;
>>   
>> -	edid = drm_get_edid(connector, connector->ddc);
>> +	drm_edid = drm_edid_read_ddc(connector, connector->ddc);

Just drm_edid_read() is enough when you're using connector->ddc.

>> +	ret = drm_edid_connector_update(connector, drm_edid);
>>   
>>   	hdmi_priv->has_hdmi_sink = false;
>>   	hdmi_priv->has_hdmi_audio = false;
>> -	if (edid) {
>> -		if (edid->input & DRM_EDID_INPUT_DIGITAL) {
>> -			status = connector_status_connected;
>> -			hdmi_priv->has_hdmi_sink =
>> -						drm_detect_hdmi_monitor(edid);
>> -			hdmi_priv->has_hdmi_audio =
>> -						drm_detect_monitor_audio(edid);
>> -		}
>> -		kfree(edid);
>> +	if (ret)

This error path leaks the EDID.

>> +		return status;
>> +
>> +	if (drm_edid_is_digital(drm_edid)) {
>> +		status = connector_status_connected;
>> +		hdmi_priv->has_hdmi_sink = connector->display_info.is_hdmi;
>> +		hdmi_priv->has_hdmi_audio = connector->display_info.has_audio;
>>   	}
>> +	drm_edid_free(drm_edid);
>> +
>>   	return status;
>>   }
>>   

-- 
Jani Nikula, Intel

Hi

Am 12.09.24 um 10:48 schrieb Jani Nikula:
> On Thu, 12 Sep 2024, Thomas Zimmermann <tzimmermann@suse.de> wrote:
>> Hi
>>
>> Am 11.09.24 um 20:06 schrieb Tejas Vipin:
>>> Replace drm_detect_hdmi_monitor() with drm_display_info.is_hdmi since
>>> monitor HDMI information is available after EDID is parsed. Additionally
>>> rewrite the code the code to have fewer indentation levels.
>> The problem is that the entire logic is outdated. The content
>> of cdv_hdmi_detect() should go into cdv_hdmi_get_modes(), the detect_ctx
>> callback should be set to drm_connector_helper_detect_from_ddc() and
>> cdv_hdmi_detect() should be deleted. The result is that ->detect_ctx
>> will detect the presence of a display and ->get_modes will update EDID
>> and other properties.
> I guess I didn't get the memo on this one.
>
> What's the problem with reading the EDID at detect? The subsequent
> drm_edid_connector_add_modes() called from .get_modes() does not need to
> read the EDID again.

With drm_connector_helper_detect_from_ddc() there is already a helper 
for detection. It makes sense to use it. And if we continue to update 
the properties in detect (instead of get_modes), what is the correct 
connector_status on errors? Right now and with the patch applied, detect 
returns status_disconnected on errors. But this isn't correct if there 
actually is a display. By separating detect and get_modes cleanly, we 
can detect the display reliably, but also handle errors better than we 
currently do in gma500. Get_modes is already expected to update the EDID 
property, [1] for detect it's not so clear AFAICT. I think that from a 
design perspective, it makes sense to have a read-only function that 
only detects the physical state of the connector and a read-write 
function that updates the connector's properties. Best regards Thomas 
[1] 
https://elixir.bootlin.com/linux/v6.10.9/source/include/drm/drm_modeset_helper_vtables.h#L865 

>
> I think it should be fine to do incremental refactors like the patch at
> hand (modulo some issues I mention below).
>
> BR,
> Jani.
>
>
>> Do you have  a device for testing such a change?
>>
>> Best regards
>> Thomas
>>
>>> Signed-off-by: Tejas Vipin <tejasvipin76@gmail.com>
>>> ---
>>> Changes in v2:
>>>       - Use drm_edid instead of edid
>>>
>>> Link to v1: https://lore.kernel.org/all/20240910051856.700210-1-tejasvipin76@gmail.com/
>>> ---
>>>    drivers/gpu/drm/gma500/cdv_intel_hdmi.c | 24 +++++++++++++-----------
>>>    1 file changed, 13 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/gma500/cdv_intel_hdmi.c b/drivers/gpu/drm/gma500/cdv_intel_hdmi.c
>>> index 2d95e0471291..701f8bbd5f2b 100644
>>> --- a/drivers/gpu/drm/gma500/cdv_intel_hdmi.c
>>> +++ b/drivers/gpu/drm/gma500/cdv_intel_hdmi.c
>>> @@ -128,23 +128,25 @@ static enum drm_connector_status cdv_hdmi_detect(
>>>    {
>>>    	struct gma_encoder *gma_encoder = gma_attached_encoder(connector);
>>>    	struct mid_intel_hdmi_priv *hdmi_priv = gma_encoder->dev_priv;
>>> -	struct edid *edid = NULL;
>>> +	const struct drm_edid *drm_edid;
>>> +	int ret;
>>>    	enum drm_connector_status status = connector_status_disconnected;
>>>    
>>> -	edid = drm_get_edid(connector, connector->ddc);
>>> +	drm_edid = drm_edid_read_ddc(connector, connector->ddc);
> Just drm_edid_read() is enough when you're using connector->ddc.
>
>>> +	ret = drm_edid_connector_update(connector, drm_edid);
>>>    
>>>    	hdmi_priv->has_hdmi_sink = false;
>>>    	hdmi_priv->has_hdmi_audio = false;
>>> -	if (edid) {
>>> -		if (edid->input & DRM_EDID_INPUT_DIGITAL) {
>>> -			status = connector_status_connected;
>>> -			hdmi_priv->has_hdmi_sink =
>>> -						drm_detect_hdmi_monitor(edid);
>>> -			hdmi_priv->has_hdmi_audio =
>>> -						drm_detect_monitor_audio(edid);
>>> -		}
>>> -		kfree(edid);
>>> +	if (ret)
> This error path leaks the EDID.
>
>>> +		return status;
>>> +
>>> +	if (drm_edid_is_digital(drm_edid)) {
>>> +		status = connector_status_connected;
>>> +		hdmi_priv->has_hdmi_sink = connector->display_info.is_hdmi;
>>> +		hdmi_priv->has_hdmi_audio = connector->display_info.has_audio;
>>>    	}
>>> +	drm_edid_free(drm_edid);
>>> +
>>>    	return status;
>>>    }
>>>    

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

On Thu, 12 Sep 2024, Thomas Zimmermann <tzimmermann@suse.de> wrote:
> Hi
>
> Am 12.09.24 um 10:48 schrieb Jani Nikula:
>> On Thu, 12 Sep 2024, Thomas Zimmermann <tzimmermann@suse.de> wrote:
>>> Hi
>>>
>>> Am 11.09.24 um 20:06 schrieb Tejas Vipin:
>>>> Replace drm_detect_hdmi_monitor() with drm_display_info.is_hdmi since
>>>> monitor HDMI information is available after EDID is parsed. Additionally
>>>> rewrite the code the code to have fewer indentation levels.
>>> The problem is that the entire logic is outdated. The content
>>> of cdv_hdmi_detect() should go into cdv_hdmi_get_modes(), the detect_ctx
>>> callback should be set to drm_connector_helper_detect_from_ddc() and
>>> cdv_hdmi_detect() should be deleted. The result is that ->detect_ctx
>>> will detect the presence of a display and ->get_modes will update EDID
>>> and other properties.
>> I guess I didn't get the memo on this one.
>>
>> What's the problem with reading the EDID at detect? The subsequent
>> drm_edid_connector_add_modes() called from .get_modes() does not need to
>> read the EDID again.
>
> With drm_connector_helper_detect_from_ddc() there is already a helper 
> for detection. It makes sense to use it. And if we continue to update 
> the properties in detect (instead of get_modes), what is the correct 
> connector_status on errors? Right now and with the patch applied, detect 
> returns status_disconnected on errors. But this isn't correct if there 
> actually is a display. By separating detect and get_modes cleanly, we 
> can detect the display reliably, but also handle errors better than we 
> currently do in gma500. Get_modes is already expected to update the EDID 
> property, [1] for detect it's not so clear AFAICT. I think that from a 
> design perspective, it makes sense to have a read-only function that 
> only detects the physical state of the connector and a read-write 
> function that updates the connector's properties. Best regards Thomas 
> [1] 
> https://elixir.bootlin.com/linux/v6.10.9/source/include/drm/drm_modeset_helper_vtables.h#L865 

So what if you can probe DDC but can't actually read an EDID of any
kind? IMO that's a detect failure.

Or how about things like CEC attach? Seems natural to do it at
.detect(). Doing it at .get_modes() just seems wrong. However, it needs
the EDID for physical address.

I just don't think one size fits all here.

BR,
Jani.


>
>>
>> I think it should be fine to do incremental refactors like the patch at
>> hand (modulo some issues I mention below).
>>
>> BR,
>> Jani.
>>
>>
>>> Do you have  a device for testing such a change?
>>>
>>> Best regards
>>> Thomas
>>>
>>>> Signed-off-by: Tejas Vipin <tejasvipin76@gmail.com>
>>>> ---
>>>> Changes in v2:
>>>>       - Use drm_edid instead of edid
>>>>
>>>> Link to v1: https://lore.kernel.org/all/20240910051856.700210-1-tejasvipin76@gmail.com/
>>>> ---
>>>>    drivers/gpu/drm/gma500/cdv_intel_hdmi.c | 24 +++++++++++++-----------
>>>>    1 file changed, 13 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/gma500/cdv_intel_hdmi.c b/drivers/gpu/drm/gma500/cdv_intel_hdmi.c
>>>> index 2d95e0471291..701f8bbd5f2b 100644
>>>> --- a/drivers/gpu/drm/gma500/cdv_intel_hdmi.c
>>>> +++ b/drivers/gpu/drm/gma500/cdv_intel_hdmi.c
>>>> @@ -128,23 +128,25 @@ static enum drm_connector_status cdv_hdmi_detect(
>>>>    {
>>>>    	struct gma_encoder *gma_encoder = gma_attached_encoder(connector);
>>>>    	struct mid_intel_hdmi_priv *hdmi_priv = gma_encoder->dev_priv;
>>>> -	struct edid *edid = NULL;
>>>> +	const struct drm_edid *drm_edid;
>>>> +	int ret;
>>>>    	enum drm_connector_status status = connector_status_disconnected;
>>>>    
>>>> -	edid = drm_get_edid(connector, connector->ddc);
>>>> +	drm_edid = drm_edid_read_ddc(connector, connector->ddc);
>> Just drm_edid_read() is enough when you're using connector->ddc.
>>
>>>> +	ret = drm_edid_connector_update(connector, drm_edid);
>>>>    
>>>>    	hdmi_priv->has_hdmi_sink = false;
>>>>    	hdmi_priv->has_hdmi_audio = false;
>>>> -	if (edid) {
>>>> -		if (edid->input & DRM_EDID_INPUT_DIGITAL) {
>>>> -			status = connector_status_connected;
>>>> -			hdmi_priv->has_hdmi_sink =
>>>> -						drm_detect_hdmi_monitor(edid);
>>>> -			hdmi_priv->has_hdmi_audio =
>>>> -						drm_detect_monitor_audio(edid);
>>>> -		}
>>>> -		kfree(edid);
>>>> +	if (ret)
>> This error path leaks the EDID.
>>
>>>> +		return status;
>>>> +
>>>> +	if (drm_edid_is_digital(drm_edid)) {
>>>> +		status = connector_status_connected;
>>>> +		hdmi_priv->has_hdmi_sink = connector->display_info.is_hdmi;
>>>> +		hdmi_priv->has_hdmi_audio = connector->display_info.has_audio;
>>>>    	}
>>>> +	drm_edid_free(drm_edid);
>>>> +
>>>>    	return status;
>>>>    }
>>>>    

-- 
Jani Nikula, Intel

Hi

Am 12.09.24 um 11:30 schrieb Jani Nikula:
> On Thu, 12 Sep 2024, Thomas Zimmermann <tzimmermann@suse.de> wrote:
>> Hi
>>
>> Am 12.09.24 um 10:48 schrieb Jani Nikula:
>>> On Thu, 12 Sep 2024, Thomas Zimmermann <tzimmermann@suse.de> wrote:
>>>> Hi
>>>>
>>>> Am 11.09.24 um 20:06 schrieb Tejas Vipin:
>>>>> Replace drm_detect_hdmi_monitor() with drm_display_info.is_hdmi since
>>>>> monitor HDMI information is available after EDID is parsed. Additionally
>>>>> rewrite the code the code to have fewer indentation levels.
>>>> The problem is that the entire logic is outdated. The content
>>>> of cdv_hdmi_detect() should go into cdv_hdmi_get_modes(), the detect_ctx
>>>> callback should be set to drm_connector_helper_detect_from_ddc() and
>>>> cdv_hdmi_detect() should be deleted. The result is that ->detect_ctx
>>>> will detect the presence of a display and ->get_modes will update EDID
>>>> and other properties.
>>> I guess I didn't get the memo on this one.
>>>
>>> What's the problem with reading the EDID at detect? The subsequent
>>> drm_edid_connector_add_modes() called from .get_modes() does not need to
>>> read the EDID again.
>> With drm_connector_helper_detect_from_ddc() there is already a helper
>> for detection. It makes sense to use it. And if we continue to update
>> the properties in detect (instead of get_modes), what is the correct
>> connector_status on errors? Right now and with the patch applied, detect
>> returns status_disconnected on errors. But this isn't correct if there
>> actually is a display. By separating detect and get_modes cleanly, we
>> can detect the display reliably, but also handle errors better than we
>> currently do in gma500. Get_modes is already expected to update the EDID
>> property, [1] for detect it's not so clear AFAICT. I think that from a
>> design perspective, it makes sense to have a read-only function that
>> only detects the physical state of the connector and a read-write
>> function that updates the connector's properties. Best regards Thomas
>> [1]
>> https://elixir.bootlin.com/linux/v6.10.9/source/include/drm/drm_modeset_helper_vtables.h#L865
> So what if you can probe DDC but can't actually read an EDID of any
> kind? IMO that's a detect failure.

Not being able to read the EDID is not a failure IMHO. It's better to 
report a detected display and only provide minimal support, than to 
outright reject it. The display is essential for most users being able 
to use the computer at all, so it's often better to display something at 
lower quality than display nothing at all.

>
> Or how about things like CEC attach? Seems natural to do it at
> .detect(). Doing it at .get_modes() just seems wrong. However, it needs
> the EDID for physical address.
>
> I just don't think one size fits all here.

The good thing about the EDID probe helpers is that it only reads a 
minimal amount of data, like a single byte or the EDID header, so 
something like that. Many drivers poll the DDC every 10 seconds via 
->detect. Which means that code running in ->detect possibly runs 
concurrently to other DRM operations, such as page flips. Hence, 
->detect can interfere with the driver's hot path. The call to 
->get_modes usually only runs after the connector status changes to 
connected, which rarely happens. It's also not time critical as no 
modeset has happened yet.

And as everyone copies code from everyone else, we should establish best 
practices that take such things into account. Even with drivers that 
don't use connector polling, such as gma500, we should encourage devs to 
do the right thing and move code out of ->detect.

Best regards
Thomas


>
> BR,
> Jani.
>
>
>>> I think it should be fine to do incremental refactors like the patch at
>>> hand (modulo some issues I mention below).
>>>
>>> BR,
>>> Jani.
>>>
>>>
>>>> Do you have  a device for testing such a change?
>>>>
>>>> Best regards
>>>> Thomas
>>>>
>>>>> Signed-off-by: Tejas Vipin <tejasvipin76@gmail.com>
>>>>> ---
>>>>> Changes in v2:
>>>>>        - Use drm_edid instead of edid
>>>>>
>>>>> Link to v1: https://lore.kernel.org/all/20240910051856.700210-1-tejasvipin76@gmail.com/
>>>>> ---
>>>>>     drivers/gpu/drm/gma500/cdv_intel_hdmi.c | 24 +++++++++++++-----------
>>>>>     1 file changed, 13 insertions(+), 11 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/gma500/cdv_intel_hdmi.c b/drivers/gpu/drm/gma500/cdv_intel_hdmi.c
>>>>> index 2d95e0471291..701f8bbd5f2b 100644
>>>>> --- a/drivers/gpu/drm/gma500/cdv_intel_hdmi.c
>>>>> +++ b/drivers/gpu/drm/gma500/cdv_intel_hdmi.c
>>>>> @@ -128,23 +128,25 @@ static enum drm_connector_status cdv_hdmi_detect(
>>>>>     {
>>>>>     	struct gma_encoder *gma_encoder = gma_attached_encoder(connector);
>>>>>     	struct mid_intel_hdmi_priv *hdmi_priv = gma_encoder->dev_priv;
>>>>> -	struct edid *edid = NULL;
>>>>> +	const struct drm_edid *drm_edid;
>>>>> +	int ret;
>>>>>     	enum drm_connector_status status = connector_status_disconnected;
>>>>>     
>>>>> -	edid = drm_get_edid(connector, connector->ddc);
>>>>> +	drm_edid = drm_edid_read_ddc(connector, connector->ddc);
>>> Just drm_edid_read() is enough when you're using connector->ddc.
>>>
>>>>> +	ret = drm_edid_connector_update(connector, drm_edid);
>>>>>     
>>>>>     	hdmi_priv->has_hdmi_sink = false;
>>>>>     	hdmi_priv->has_hdmi_audio = false;
>>>>> -	if (edid) {
>>>>> -		if (edid->input & DRM_EDID_INPUT_DIGITAL) {
>>>>> -			status = connector_status_connected;
>>>>> -			hdmi_priv->has_hdmi_sink =
>>>>> -						drm_detect_hdmi_monitor(edid);
>>>>> -			hdmi_priv->has_hdmi_audio =
>>>>> -						drm_detect_monitor_audio(edid);
>>>>> -		}
>>>>> -		kfree(edid);
>>>>> +	if (ret)
>>> This error path leaks the EDID.
>>>
>>>>> +		return status;
>>>>> +
>>>>> +	if (drm_edid_is_digital(drm_edid)) {
>>>>> +		status = connector_status_connected;
>>>>> +		hdmi_priv->has_hdmi_sink = connector->display_info.is_hdmi;
>>>>> +		hdmi_priv->has_hdmi_audio = connector->display_info.has_audio;
>>>>>     	}
>>>>> +	drm_edid_free(drm_edid);
>>>>> +
>>>>>     	return status;
>>>>>     }
>>>>>     

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

On Thu, Sep 12, 2024 at 01:08:06PM +0200, Thomas Zimmermann wrote:
> Hi
> 
> Am 12.09.24 um 11:30 schrieb Jani Nikula:
> > On Thu, 12 Sep 2024, Thomas Zimmermann <tzimmermann@suse.de> wrote:
> >> Hi
> >>
> >> Am 12.09.24 um 10:48 schrieb Jani Nikula:
> >>> On Thu, 12 Sep 2024, Thomas Zimmermann <tzimmermann@suse.de> wrote:
> >>>> Hi
> >>>>
> >>>> Am 11.09.24 um 20:06 schrieb Tejas Vipin:
> >>>>> Replace drm_detect_hdmi_monitor() with drm_display_info.is_hdmi since
> >>>>> monitor HDMI information is available after EDID is parsed. Additionally
> >>>>> rewrite the code the code to have fewer indentation levels.
> >>>> The problem is that the entire logic is outdated. The content
> >>>> of cdv_hdmi_detect() should go into cdv_hdmi_get_modes(), the detect_ctx
> >>>> callback should be set to drm_connector_helper_detect_from_ddc() and
> >>>> cdv_hdmi_detect() should be deleted. The result is that ->detect_ctx
> >>>> will detect the presence of a display and ->get_modes will update EDID
> >>>> and other properties.
> >>> I guess I didn't get the memo on this one.
> >>>
> >>> What's the problem with reading the EDID at detect? The subsequent
> >>> drm_edid_connector_add_modes() called from .get_modes() does not need to
> >>> read the EDID again.
> >> With drm_connector_helper_detect_from_ddc() there is already a helper
> >> for detection. It makes sense to use it. And if we continue to update
> >> the properties in detect (instead of get_modes), what is the correct
> >> connector_status on errors? Right now and with the patch applied, detect
> >> returns status_disconnected on errors. But this isn't correct if there
> >> actually is a display. By separating detect and get_modes cleanly, we
> >> can detect the display reliably, but also handle errors better than we
> >> currently do in gma500. Get_modes is already expected to update the EDID
> >> property, [1] for detect it's not so clear AFAICT. I think that from a
> >> design perspective, it makes sense to have a read-only function that
> >> only detects the physical state of the connector and a read-write
> >> function that updates the connector's properties. Best regards Thomas
> >> [1]
> >> https://elixir.bootlin.com/linux/v6.10.9/source/include/drm/drm_modeset_helper_vtables.h#L865
> > So what if you can probe DDC but can't actually read an EDID of any
> > kind? IMO that's a detect failure.
> 
> Not being able to read the EDID is not a failure IMHO. It's better to 
> report a detected display and only provide minimal support, than to 
> outright reject it. The display is essential for most users being able 
> to use the computer at all, so it's often better to display something at 
> lower quality than display nothing at all.
> 
> >
> > Or how about things like CEC attach? Seems natural to do it at
> > .detect(). Doing it at .get_modes() just seems wrong. However, it needs
> > the EDID for physical address.
> >
> > I just don't think one size fits all here.
> 
> The good thing about the EDID probe helpers is that it only reads a 
> minimal amount of data, like a single byte or the EDID header, so 
> something like that. Many drivers poll the DDC every 10 seconds via 
> ->detect. Which means that code running in ->detect possibly runs 
> concurrently to other DRM operations, such as page flips. Hence, 
> ->detect can interfere with the driver's hot path. The call to 
> ->get_modes usually only runs after the connector status changes to 
> connected, which rarely happens. It's also not time critical as no 
> modeset has happened yet.

I don't see how you can really optimize polling with this because
the only way to figure out if the EDID changed is to read it.

Anyways, my gut feeling is that we need things in .detect()
because that's also where DPCD is read, and we defintiely need that
to do anything sensible. And dongles can also snoop the EDID reads
and I think even potentially change their DPCD based on that, so
having that go out of sync by skipping the EDID read might end up
with weird behaviour.

-- 
Ville Syrjälä
Intel

On Thu, 12 Sep 2024, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> On Thu, 12 Sep 2024, Thomas Zimmermann <tzimmermann@suse.de> wrote:
>> Hi
>>
>> Am 11.09.24 um 20:06 schrieb Tejas Vipin:
>>> Replace drm_detect_hdmi_monitor() with drm_display_info.is_hdmi since
>>> monitor HDMI information is available after EDID is parsed. Additionally
>>> rewrite the code the code to have fewer indentation levels.
>>
>> The problem is that the entire logic is outdated. The content 
>> of cdv_hdmi_detect() should go into cdv_hdmi_get_modes(), the detect_ctx 
>> callback should be set to drm_connector_helper_detect_from_ddc() and 
>> cdv_hdmi_detect() should be deleted. The result is that ->detect_ctx 
>> will detect the presence of a display and ->get_modes will update EDID 
>> and other properties.
>
> I guess I didn't get the memo on this one.
>
> What's the problem with reading the EDID at detect? The subsequent
> drm_edid_connector_add_modes() called from .get_modes() does not need to
> read the EDID again.

Moreover, in this case .detect() only detects digital displays as
reported by EDID. If you postpone that to .get_modes(), the probe helper
will still report connected, and invent non-EDID fallback modes. The
behaviour changes.

> I think it should be fine to do incremental refactors like the patch at
> hand (modulo some issues I mention below).

Another issue in the patch is that it should also change .get_modes() to
not read the EDID again, but just call drm_edid_connector_add_modes().

BR,
Jani.

>
> BR,
> Jani.
>
>
>>
>> Do you have  a device for testing such a change?
>>
>> Best regards
>> Thomas
>>
>>>
>>> Signed-off-by: Tejas Vipin <tejasvipin76@gmail.com>
>>> ---
>>> Changes in v2:
>>>      - Use drm_edid instead of edid
>>>
>>> Link to v1: https://lore.kernel.org/all/20240910051856.700210-1-tejasvipin76@gmail.com/
>>> ---
>>>   drivers/gpu/drm/gma500/cdv_intel_hdmi.c | 24 +++++++++++++-----------
>>>   1 file changed, 13 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/gma500/cdv_intel_hdmi.c b/drivers/gpu/drm/gma500/cdv_intel_hdmi.c
>>> index 2d95e0471291..701f8bbd5f2b 100644
>>> --- a/drivers/gpu/drm/gma500/cdv_intel_hdmi.c
>>> +++ b/drivers/gpu/drm/gma500/cdv_intel_hdmi.c
>>> @@ -128,23 +128,25 @@ static enum drm_connector_status cdv_hdmi_detect(
>>>   {
>>>   	struct gma_encoder *gma_encoder = gma_attached_encoder(connector);
>>>   	struct mid_intel_hdmi_priv *hdmi_priv = gma_encoder->dev_priv;
>>> -	struct edid *edid = NULL;
>>> +	const struct drm_edid *drm_edid;
>>> +	int ret;
>>>   	enum drm_connector_status status = connector_status_disconnected;
>>>   
>>> -	edid = drm_get_edid(connector, connector->ddc);
>>> +	drm_edid = drm_edid_read_ddc(connector, connector->ddc);
>
> Just drm_edid_read() is enough when you're using connector->ddc.
>
>>> +	ret = drm_edid_connector_update(connector, drm_edid);
>>>   
>>>   	hdmi_priv->has_hdmi_sink = false;
>>>   	hdmi_priv->has_hdmi_audio = false;
>>> -	if (edid) {
>>> -		if (edid->input & DRM_EDID_INPUT_DIGITAL) {
>>> -			status = connector_status_connected;
>>> -			hdmi_priv->has_hdmi_sink =
>>> -						drm_detect_hdmi_monitor(edid);
>>> -			hdmi_priv->has_hdmi_audio =
>>> -						drm_detect_monitor_audio(edid);
>>> -		}
>>> -		kfree(edid);
>>> +	if (ret)
>
> This error path leaks the EDID.
>
>>> +		return status;
>>> +
>>> +	if (drm_edid_is_digital(drm_edid)) {
>>> +		status = connector_status_connected;
>>> +		hdmi_priv->has_hdmi_sink = connector->display_info.is_hdmi;
>>> +		hdmi_priv->has_hdmi_audio = connector->display_info.has_audio;
>>>   	}
>>> +	drm_edid_free(drm_edid);
>>> +
>>>   	return status;
>>>   }
>>>   

-- 
Jani Nikula, Intel

Hi

Am 12.09.24 um 10:56 schrieb Jani Nikula:
> On Thu, 12 Sep 2024, Jani Nikula <jani.nikula@linux.intel.com> wrote:
>> On Thu, 12 Sep 2024, Thomas Zimmermann <tzimmermann@suse.de> wrote:
>>> Hi
>>>
>>> Am 11.09.24 um 20:06 schrieb Tejas Vipin:
>>>> Replace drm_detect_hdmi_monitor() with drm_display_info.is_hdmi since
>>>> monitor HDMI information is available after EDID is parsed. Additionally
>>>> rewrite the code the code to have fewer indentation levels.
>>> The problem is that the entire logic is outdated. The content
>>> of cdv_hdmi_detect() should go into cdv_hdmi_get_modes(), the detect_ctx
>>> callback should be set to drm_connector_helper_detect_from_ddc() and
>>> cdv_hdmi_detect() should be deleted. The result is that ->detect_ctx
>>> will detect the presence of a display and ->get_modes will update EDID
>>> and other properties.
>> I guess I didn't get the memo on this one.
>>
>> What's the problem with reading the EDID at detect? The subsequent
>> drm_edid_connector_add_modes() called from .get_modes() does not need to
>> read the EDID again.
> Moreover, in this case .detect() only detects digital displays as
> reported by EDID. If you postpone that to .get_modes(), the probe helper
> will still report connected, and invent non-EDID fallback modes. The
> behaviour changes.

The change in behavior is intentional, because the current test seems 
arbitrary. Does the driver not work with analog outputs?

Best regards
Thomas

>
>> I think it should be fine to do incremental refactors like the patch at
>> hand (modulo some issues I mention below).
> Another issue in the patch is that it should also change .get_modes() to
> not read the EDID again, but just call drm_edid_connector_add_modes().
>
> BR,
> Jani.
>
>> BR,
>> Jani.
>>
>>
>>> Do you have  a device for testing such a change?
>>>
>>> Best regards
>>> Thomas
>>>
>>>> Signed-off-by: Tejas Vipin <tejasvipin76@gmail.com>
>>>> ---
>>>> Changes in v2:
>>>>       - Use drm_edid instead of edid
>>>>
>>>> Link to v1: https://lore.kernel.org/all/20240910051856.700210-1-tejasvipin76@gmail.com/
>>>> ---
>>>>    drivers/gpu/drm/gma500/cdv_intel_hdmi.c | 24 +++++++++++++-----------
>>>>    1 file changed, 13 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/gma500/cdv_intel_hdmi.c b/drivers/gpu/drm/gma500/cdv_intel_hdmi.c
>>>> index 2d95e0471291..701f8bbd5f2b 100644
>>>> --- a/drivers/gpu/drm/gma500/cdv_intel_hdmi.c
>>>> +++ b/drivers/gpu/drm/gma500/cdv_intel_hdmi.c
>>>> @@ -128,23 +128,25 @@ static enum drm_connector_status cdv_hdmi_detect(
>>>>    {
>>>>    	struct gma_encoder *gma_encoder = gma_attached_encoder(connector);
>>>>    	struct mid_intel_hdmi_priv *hdmi_priv = gma_encoder->dev_priv;
>>>> -	struct edid *edid = NULL;
>>>> +	const struct drm_edid *drm_edid;
>>>> +	int ret;
>>>>    	enum drm_connector_status status = connector_status_disconnected;
>>>>    
>>>> -	edid = drm_get_edid(connector, connector->ddc);
>>>> +	drm_edid = drm_edid_read_ddc(connector, connector->ddc);
>> Just drm_edid_read() is enough when you're using connector->ddc.
>>
>>>> +	ret = drm_edid_connector_update(connector, drm_edid);
>>>>    
>>>>    	hdmi_priv->has_hdmi_sink = false;
>>>>    	hdmi_priv->has_hdmi_audio = false;
>>>> -	if (edid) {
>>>> -		if (edid->input & DRM_EDID_INPUT_DIGITAL) {
>>>> -			status = connector_status_connected;
>>>> -			hdmi_priv->has_hdmi_sink =
>>>> -						drm_detect_hdmi_monitor(edid);
>>>> -			hdmi_priv->has_hdmi_audio =
>>>> -						drm_detect_monitor_audio(edid);
>>>> -		}
>>>> -		kfree(edid);
>>>> +	if (ret)
>> This error path leaks the EDID.
>>
>>>> +		return status;
>>>> +
>>>> +	if (drm_edid_is_digital(drm_edid)) {
>>>> +		status = connector_status_connected;
>>>> +		hdmi_priv->has_hdmi_sink = connector->display_info.is_hdmi;
>>>> +		hdmi_priv->has_hdmi_audio = connector->display_info.has_audio;
>>>>    	}
>>>> +	drm_edid_free(drm_edid);
>>>> +
>>>>    	return status;
>>>>    }
>>>>    

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

On Thu, 12 Sep 2024, Thomas Zimmermann <tzimmermann@suse.de> wrote:
> Am 12.09.24 um 10:56 schrieb Jani Nikula:
>> Moreover, in this case .detect() only detects digital displays as
>> reported by EDID. If you postpone that to .get_modes(), the probe helper
>> will still report connected, and invent non-EDID fallback modes. The
>> behaviour changes.
>
> The change in behavior is intentional, because the current test seems 
> arbitrary. Does the driver not work with analog outputs?

Not on a DVI/HDMI port. Same with i915.

That's possibly the only way to distinguish a DVI-A display connected to
DVI-D source.


BR,
Jani.


-- 
Jani Nikula, Intel

Hi

Am 12.09.24 um 11:38 schrieb Jani Nikula:
> On Thu, 12 Sep 2024, Thomas Zimmermann <tzimmermann@suse.de> wrote:
>> Am 12.09.24 um 10:56 schrieb Jani Nikula:
>>> Moreover, in this case .detect() only detects digital displays as
>>> reported by EDID. If you postpone that to .get_modes(), the probe helper
>>> will still report connected, and invent non-EDID fallback modes. The
>>> behaviour changes.
>> The change in behavior is intentional, because the current test seems
>> arbitrary. Does the driver not work with analog outputs?
> Not on a DVI/HDMI port. Same with i915.
>
> That's possibly the only way to distinguish a DVI-A display connected to
> DVI-D source.

That's a detect failure, but IMHO our probe helpers should really handle 
this case.

Best regards
Thomas

>
>
> BR,
> Jani.
>
>

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

On Thu, 12 Sep 2024, Thomas Zimmermann <tzimmermann@suse.de> wrote:
> Hi
>
> Am 12.09.24 um 11:38 schrieb Jani Nikula:
>> On Thu, 12 Sep 2024, Thomas Zimmermann <tzimmermann@suse.de> wrote:
>>> Am 12.09.24 um 10:56 schrieb Jani Nikula:
>>>> Moreover, in this case .detect() only detects digital displays as
>>>> reported by EDID. If you postpone that to .get_modes(), the probe helper
>>>> will still report connected, and invent non-EDID fallback modes. The
>>>> behaviour changes.
>>> The change in behavior is intentional, because the current test seems
>>> arbitrary. Does the driver not work with analog outputs?
>> Not on a DVI/HDMI port. Same with i915.
>>
>> That's possibly the only way to distinguish a DVI-A display connected to
>> DVI-D source.
>
> That's a detect failure, but IMHO our probe helpers should really handle 
> this case.

How? Allow returning detect failures from .get_modes()?

BR,
Jani.


-- 
Jani Nikula, Intel

Hi

Am 12.09.24 um 13:25 schrieb Jani Nikula:
> On Thu, 12 Sep 2024, Thomas Zimmermann <tzimmermann@suse.de> wrote:
>> Hi
>>
>> Am 12.09.24 um 11:38 schrieb Jani Nikula:
>>> On Thu, 12 Sep 2024, Thomas Zimmermann <tzimmermann@suse.de> wrote:
>>>> Am 12.09.24 um 10:56 schrieb Jani Nikula:
>>>>> Moreover, in this case .detect() only detects digital displays as
>>>>> reported by EDID. If you postpone that to .get_modes(), the probe helper
>>>>> will still report connected, and invent non-EDID fallback modes. The
>>>>> behaviour changes.
>>>> The change in behavior is intentional, because the current test seems
>>>> arbitrary. Does the driver not work with analog outputs?
>>> Not on a DVI/HDMI port. Same with i915.
>>>
>>> That's possibly the only way to distinguish a DVI-A display connected to
>>> DVI-D source.
>> That's a detect failure, but IMHO our probe helpers should really handle
>> this case.
> How? Allow returning detect failures from .get_modes()?

Something like that, I guess.

For the specific problem it would be enough to read the first 20 bytes 
of EDID data on DVI connectors and test the digital-input flag bit 
against the exact connector requirements. drm_probe_ddc() could do this. 
Non-DVI connectors would continue to read a single bytes to detect the DDC.

For more sophisticated problems, it would be good to introduce an 
intermediate callback that updates the connector state. So the probe 
logic would look like:

  1) call ->detect to read physical connector status
  2) return if physical status did not change
  3) increment epoch counter
  4) call ->update to update connector state and properties (EDID, etc) 
get new connector status
  5) call ->get_modes if connected

The initial ->detect would be minimal. The ->update, if implemented, 
could do more processing and error checking. It's result would be the 
connector's new status.

On a side note, I've recently spend quite a few patches on getting the 
BMC output for ast and mgag200 usable. Something like the above logic 
would have helped, I think. Because with the current probe logic, I had 
to implement steps 1 to 4 in ->detect itself. The result has to maintain 
physical status and epoch counter by itself. [1]

Best regards
Thomas

[1] 
https://gitlab.freedesktop.org/drm/kernel/-/commit/2a2391f857cdc5cf16f8df030944cef8d3d2bc30

>
> BR,
> Jani.
>
>

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

On Thu, 12 Sep 2024, Thomas Zimmermann <tzimmermann@suse.de> wrote:
> Hi
>
> Am 12.09.24 um 13:25 schrieb Jani Nikula:
>> On Thu, 12 Sep 2024, Thomas Zimmermann <tzimmermann@suse.de> wrote:
>>> Hi
>>>
>>> Am 12.09.24 um 11:38 schrieb Jani Nikula:
>>>> On Thu, 12 Sep 2024, Thomas Zimmermann <tzimmermann@suse.de> wrote:
>>>>> Am 12.09.24 um 10:56 schrieb Jani Nikula:
>>>>>> Moreover, in this case .detect() only detects digital displays as
>>>>>> reported by EDID. If you postpone that to .get_modes(), the probe helper
>>>>>> will still report connected, and invent non-EDID fallback modes. The
>>>>>> behaviour changes.
>>>>> The change in behavior is intentional, because the current test seems
>>>>> arbitrary. Does the driver not work with analog outputs?
>>>> Not on a DVI/HDMI port. Same with i915.
>>>>
>>>> That's possibly the only way to distinguish a DVI-A display connected to
>>>> DVI-D source.
>>> That's a detect failure, but IMHO our probe helpers should really handle
>>> this case.
>> How? Allow returning detect failures from .get_modes()?
>
> Something like that, I guess.
>
> For the specific problem it would be enough to read the first 20 bytes 
> of EDID data on DVI connectors and test the digital-input flag bit 
> against the exact connector requirements. drm_probe_ddc() could do this. 

Just a quick reply on this particular point:

I'm very strongly against doing partial EDID reads. It's all geared
towards EDID block sized handling. And you can't do checksum checking on
the 20 bytes. It would be a maze of special casing, something the EDID
code could have less, not more.

BR,
Jani.

> Non-DVI connectors would continue to read a single bytes to detect the DDC.
>
> For more sophisticated problems, it would be good to introduce an 
> intermediate callback that updates the connector state. So the probe 
> logic would look like:
>
>   1) call ->detect to read physical connector status
>   2) return if physical status did not change
>   3) increment epoch counter
>   4) call ->update to update connector state and properties (EDID, etc) 
> get new connector status
>   5) call ->get_modes if connected
>
> The initial ->detect would be minimal. The ->update, if implemented, 
> could do more processing and error checking. It's result would be the 
> connector's new status.
>
> On a side note, I've recently spend quite a few patches on getting the 
> BMC output for ast and mgag200 usable. Something like the above logic 
> would have helped, I think. Because with the current probe logic, I had 
> to implement steps 1 to 4 in ->detect itself. The result has to maintain 
> physical status and epoch counter by itself. [1]
>
> Best regards
> Thomas
>
> [1] 
> https://gitlab.freedesktop.org/drm/kernel/-/commit/2a2391f857cdc5cf16f8df030944cef8d3d2bc30
>
>>
>> BR,
>> Jani.
>>
>>

-- 
Jani Nikula, Intel