[PATCH] drm/probe-helper: convert drm_connector_helper_get_modes_from_ddc() to struct drm_edid

[PATCH] drm/probe-helper: convert drm_connector_helper_get_modes_from_ddc() to struct drm_edid

[Jani Nikula]

Going forward, the struct drm_edid based functions drm_edid_read(),
drm_edid_connector_update() and drm_edid_connector_add_modes() are the
preferred ways of retrieving the EDID and updating the connector.

Cc: Thomas Zimmermann <tzimmermann@suse.de>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/drm_probe_helper.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
index 3f479483d7d8..309d88f13648 100644
--- a/drivers/gpu/drm/drm_probe_helper.c
+++ b/drivers/gpu/drm/drm_probe_helper.c
@@ -1116,21 +1116,20 @@ EXPORT_SYMBOL(drm_crtc_helper_mode_valid_fixed);
  */
 int drm_connector_helper_get_modes_from_ddc(struct drm_connector *connector)
 {
-	struct edid *edid;
-	int count = 0;
+	const struct drm_edid *drm_edid;
+	int count;
 
 	if (!connector->ddc)
 		return 0;
 
-	edid = drm_get_edid(connector, connector->ddc);
+	drm_edid = drm_edid_read(connector);
+
+	/* clears property if EDID is NULL */
+	drm_edid_connector_update(connector, drm_edid);
 
-	// clears property if EDID is NULL
-	drm_connector_update_edid_property(connector, edid);
+	count = drm_edid_connector_add_modes(connector);
 
-	if (edid) {
-		count = drm_add_edid_modes(connector, edid);
-		kfree(edid);
-	}
+	drm_edid_free(drm_edid);
 
 	return count;
 }
-- 
2.39.2

Re: [PATCH] drm/probe-helper: convert drm_connector_helper_get_modes_from_ddc() to struct drm_edid

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 2132 bytes --]

Hi

Am 14.11.23 um 11:58 schrieb Jani Nikula:
> Going forward, the struct drm_edid based functions drm_edid_read(),
> drm_edid_connector_update() and drm_edid_connector_add_modes() are the
> preferred ways of retrieving the EDID and updating the connector.
> 
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>

Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>

If I'm not mistaken, the correct pattern is to read the EDID block in 
the detect callback and only parse it for modes in get_modes. If so, you 
might also inline this helper into its only caller in mgag200. I'll 
later split it up into detect and get_modes.

Best regards
Thomas

> ---
>   drivers/gpu/drm/drm_probe_helper.c | 17 ++++++++---------
>   1 file changed, 8 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
> index 3f479483d7d8..309d88f13648 100644
> --- a/drivers/gpu/drm/drm_probe_helper.c
> +++ b/drivers/gpu/drm/drm_probe_helper.c
> @@ -1116,21 +1116,20 @@ EXPORT_SYMBOL(drm_crtc_helper_mode_valid_fixed);
>    */
>   int drm_connector_helper_get_modes_from_ddc(struct drm_connector *connector)
>   {
> -	struct edid *edid;
> -	int count = 0;
> +	const struct drm_edid *drm_edid;
> +	int count;
>   
>   	if (!connector->ddc)
>   		return 0;
>   
> -	edid = drm_get_edid(connector, connector->ddc);
> +	drm_edid = drm_edid_read(connector);
> +
> +	/* clears property if EDID is NULL */
> +	drm_edid_connector_update(connector, drm_edid);
>   
> -	// clears property if EDID is NULL
> -	drm_connector_update_edid_property(connector, edid);
> +	count = drm_edid_connector_add_modes(connector);
>   
> -	if (edid) {
> -		count = drm_add_edid_modes(connector, edid);
> -		kfree(edid);
> -	}
> +	drm_edid_free(drm_edid);
>   
>   	return count;
>   }

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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH] drm/probe-helper: convert drm_connector_helper_get_modes_from_ddc() to struct drm_edid

[Jani Nikula]

On Tue, 14 Nov 2023, Thomas Zimmermann <tzimmermann@suse.de> wrote:
> Hi
>
> Am 14.11.23 um 11:58 schrieb Jani Nikula:
>> Going forward, the struct drm_edid based functions drm_edid_read(),
>> drm_edid_connector_update() and drm_edid_connector_add_modes() are the
>> preferred ways of retrieving the EDID and updating the connector.
>> 
>> Cc: Thomas Zimmermann <tzimmermann@suse.de>
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>
> Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
>
> If I'm not mistaken, the correct pattern is to read the EDID block in 
> the detect callback and only parse it for modes in get_modes. If so, you 
> might also inline this helper into its only caller in mgag200. I'll 
> later split it up into detect and get_modes.

That's what we do in i915 anyway.

drm_edid_read() and drm_edid_connector_update() in ->detect and ->force,
and drm_edid_connector_add_modes() in ->get_modes.

BR,
Jani.


>
> Best regards
> Thomas
>
>> ---
>>   drivers/gpu/drm/drm_probe_helper.c | 17 ++++++++---------
>>   1 file changed, 8 insertions(+), 9 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
>> index 3f479483d7d8..309d88f13648 100644
>> --- a/drivers/gpu/drm/drm_probe_helper.c
>> +++ b/drivers/gpu/drm/drm_probe_helper.c
>> @@ -1116,21 +1116,20 @@ EXPORT_SYMBOL(drm_crtc_helper_mode_valid_fixed);
>>    */
>>   int drm_connector_helper_get_modes_from_ddc(struct drm_connector *connector)
>>   {
>> -	struct edid *edid;
>> -	int count = 0;
>> +	const struct drm_edid *drm_edid;
>> +	int count;
>>   
>>   	if (!connector->ddc)
>>   		return 0;
>>   
>> -	edid = drm_get_edid(connector, connector->ddc);
>> +	drm_edid = drm_edid_read(connector);
>> +
>> +	/* clears property if EDID is NULL */
>> +	drm_edid_connector_update(connector, drm_edid);
>>   
>> -	// clears property if EDID is NULL
>> -	drm_connector_update_edid_property(connector, edid);
>> +	count = drm_edid_connector_add_modes(connector);
>>   
>> -	if (edid) {
>> -		count = drm_add_edid_modes(connector, edid);
>> -		kfree(edid);
>> -	}
>> +	drm_edid_free(drm_edid);
>>   
>>   	return count;
>>   }

-- 
Jani Nikula, Intel

Re: [PATCH] drm/probe-helper: convert drm_connector_helper_get_modes_from_ddc() to struct drm_edid

[Jani Nikula]

On Tue, 14 Nov 2023, Thomas Zimmermann <tzimmermann@suse.de> wrote:
> If I'm not mistaken, the correct pattern is to read the EDID block in 
> the detect callback and only parse it for modes in get_modes. If so, you 
> might also inline this helper into its only caller in mgag200. I'll 
> later split it up into detect and get_modes.

Never merged this, so I sent your suggested alternative instead [1].

BR,
Jani.

[1] https://patchwork.freedesktop.org/series/128269/


-- 
Jani Nikula, Intel