[PATCH 0/7] drm/udl: Convert to struct drm_edid

[PATCH 0/7] drm/udl: Convert to struct drm_edid

[Thomas Zimmermann]

Convert udl to use struct drm_edid and its helpers. Also clean up
a few things in the process.

Patches 1 and 2 add a custom EDID probe function similar to the
existing drm_probe_ddc(). The seconds patch is necessary to make
it useful for udl.

Patch 3 fixes a bug.

Patches 4 to 6 convert the current EDID handling to struct drm_edid
and its helpers. Patch 6 also separates the helpers for .get_modes()
and .detect_ctx() from each other.

Patch 7 removes the obsolete struct udl_connector.

Thomas Zimmermann (7):
  drm/edid: Implement drm_probe_ddc() with drm_edid_probe_custom()
  drm/edid: Test for EDID header in drm_edit_probe_custom()
  drm/udl: Remove DRM_CONNECTOR_POLL_HPD
  drm/udl: Move drm_dev_{enter,exit}() into udl_get_edid_block()
  drm/udl: Clean up Makefile
  drm/udl: Untangle .get_modes() and .detect_ctx()
  drm/udl: Remove struct udl_connector

 drivers/gpu/drm/drm_edid.c        |  62 +++++++++++---
 drivers/gpu/drm/udl/Makefile      |   8 +-
 drivers/gpu/drm/udl/udl_drv.h     |  12 +--
 drivers/gpu/drm/udl/udl_edid.c    |  67 +++++++++++++++
 drivers/gpu/drm/udl/udl_edid.h    |  15 ++++
 drivers/gpu/drm/udl/udl_modeset.c | 136 ++++++------------------------
 include/drm/drm_edid.h            |   3 +
 7 files changed, 173 insertions(+), 130 deletions(-)
 create mode 100644 drivers/gpu/drm/udl/udl_edid.c
 create mode 100644 drivers/gpu/drm/udl/udl_edid.h

-- 
2.44.0

[PATCH 1/7] drm/edid: Implement drm_probe_ddc() with drm_edid_probe_custom()

[Thomas Zimmermann]

Move the logic from drm_probe_ddc() into drm_edid_probe_custom(),
which receives the EDID read function as argument. Allows drivers
without DDC to implement similar functionality without duplicating
the code.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/drm_edid.c | 22 +++++++++++++++++++---
 include/drm/drm_edid.h     |  3 +++
 2 files changed, 22 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index ea77577a37864..a3e9333f9177a 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -2608,6 +2608,24 @@ void drm_edid_free(const struct drm_edid *drm_edid)
 }
 EXPORT_SYMBOL(drm_edid_free);
 
+/**
+ * drm_edid_probe_custom() - probe DDC presence
+ * @read_block: EDID block read function
+ * @context: Private data passed to the block read function
+ *
+ * Probes for EDID data. Only reads enough data to detect the presence
+ * of the EDID channel.
+ *
+ * Return: True on success, false on failure.
+ */
+bool drm_edid_probe_custom(read_block_fn read_block, void *context)
+{
+	unsigned char out;
+
+	return (read_block(context, &out, 0, 1) == 0);
+}
+EXPORT_SYMBOL(drm_edid_probe_custom);
+
 /**
  * drm_probe_ddc() - probe DDC presence
  * @adapter: I2C adapter to probe
@@ -2617,9 +2635,7 @@ EXPORT_SYMBOL(drm_edid_free);
 bool
 drm_probe_ddc(struct i2c_adapter *adapter)
 {
-	unsigned char out;
-
-	return (drm_do_probe_ddc_edid(adapter, &out, 0, 1) == 0);
+	return drm_edid_probe_custom(drm_do_probe_ddc_edid, adapter);
 }
 EXPORT_SYMBOL(drm_probe_ddc);
 
diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
index 6f65bbf655a1f..4d1797ade5f1d 100644
--- a/include/drm/drm_edid.h
+++ b/include/drm/drm_edid.h
@@ -410,6 +410,9 @@ static inline void drm_edid_decode_panel_id(u32 panel_id, char vend[4], u16 *pro
 	drm_edid_decode_mfg_id(panel_id >> 16, vend);
 }
 
+bool
+drm_edid_probe_custom(int (*read_block)(void *context, u8 *buf, unsigned int block, size_t len),
+		      void *context);
 bool drm_probe_ddc(struct i2c_adapter *adapter);
 struct edid *drm_do_get_edid(struct drm_connector *connector,
 	int (*get_edid_block)(void *data, u8 *buf, unsigned int block,
-- 
2.44.0

[PATCH 2/7] drm/edid: Test for EDID header in drm_edit_probe_custom()

[Thomas Zimmermann]

EDID read functions do not validate their return data. So they might
return the required number of bytes of probing, but with invalid
data. Therefore test for the presence of an EDID header similar to
the code in edid_block_check().

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/drm_edid.c | 50 +++++++++++++++++++++++++++++---------
 include/drm/drm_edid.h     |  2 +-
 2 files changed, 39 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index a3e9333f9177a..4e12d4b83a720 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -1758,6 +1758,18 @@ static void edid_header_fix(void *edid)
 	memcpy(edid, edid_header, sizeof(edid_header));
 }
 
+static int edid_header_score(const u8 *header)
+{
+	int i, score = 0;
+
+	for (i = 0; i < sizeof(edid_header); i++) {
+		if (header[i] == edid_header[i])
+			score++;
+	}
+
+	return score;
+}
+
 /**
  * drm_edid_header_is_valid - sanity check the header of the base EDID block
  * @_edid: pointer to raw base EDID block
@@ -1769,14 +1781,8 @@ static void edid_header_fix(void *edid)
 int drm_edid_header_is_valid(const void *_edid)
 {
 	const struct edid *edid = _edid;
-	int i, score = 0;
 
-	for (i = 0; i < sizeof(edid_header); i++) {
-		if (edid->header[i] == edid_header[i])
-			score++;
-	}
-
-	return score;
+	return edid_header_score(edid->header);
 }
 EXPORT_SYMBOL(drm_edid_header_is_valid);
 
@@ -2612,17 +2618,37 @@ EXPORT_SYMBOL(drm_edid_free);
  * drm_edid_probe_custom() - probe DDC presence
  * @read_block: EDID block read function
  * @context: Private data passed to the block read function
+ * @validate: True to validate the EDID header
  *
  * Probes for EDID data. Only reads enough data to detect the presence
- * of the EDID channel.
+ * of the EDID channel. Some EDID block read functions do not fail,
+ * but return invalid data if no EDID data is available. If @validate
+ * has been specified, drm_edid_probe_custom() validates the retrieved
+ * EDID header before signalling success.
  *
  * Return: True on success, false on failure.
  */
-bool drm_edid_probe_custom(read_block_fn read_block, void *context)
+bool drm_edid_probe_custom(read_block_fn read_block, void *context, bool validate)
 {
-	unsigned char out;
+	u8 header[8] = {0, 0, 0, 0, 0, 0, 0, 0};
+	int ret;
+	size_t len = 1;
+
+	if (validate)
+		len = sizeof(header); // read full header
+
+	ret = read_block(context, header, 0, len);
+	if (ret)
+		return false;
+
+	if (validate) {
+		int score = edid_header_score(header);
+
+		if (score < clamp(edid_fixup, 0, 8))
+			return false;
+	}
 
-	return (read_block(context, &out, 0, 1) == 0);
+	return true;
 }
 EXPORT_SYMBOL(drm_edid_probe_custom);
 
@@ -2635,7 +2661,7 @@ EXPORT_SYMBOL(drm_edid_probe_custom);
 bool
 drm_probe_ddc(struct i2c_adapter *adapter)
 {
-	return drm_edid_probe_custom(drm_do_probe_ddc_edid, adapter);
+	return drm_edid_probe_custom(drm_do_probe_ddc_edid, adapter, false);
 }
 EXPORT_SYMBOL(drm_probe_ddc);
 
diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
index 4d1797ade5f1d..299278c7ee1c2 100644
--- a/include/drm/drm_edid.h
+++ b/include/drm/drm_edid.h
@@ -412,7 +412,7 @@ static inline void drm_edid_decode_panel_id(u32 panel_id, char vend[4], u16 *pro
 
 bool
 drm_edid_probe_custom(int (*read_block)(void *context, u8 *buf, unsigned int block, size_t len),
-		      void *context);
+		      void *context, bool validate);
 bool drm_probe_ddc(struct i2c_adapter *adapter);
 struct edid *drm_do_get_edid(struct drm_connector *connector,
 	int (*get_edid_block)(void *data, u8 *buf, unsigned int block,
-- 
2.44.0

Re: [PATCH 2/7] drm/edid: Test for EDID header in drm_edit_probe_custom()

[Jani Nikula]

On Thu, 04 Apr 2024, Thomas Zimmermann <tzimmermann@suse.de> wrote:
> EDID read functions do not validate their return data. So they might
> return the required number of bytes of probing, but with invalid
> data. Therefore test for the presence of an EDID header similar to
> the code in edid_block_check().

I don't think the point of drm_probe_ddc() ever was to validate
anything. It reads one byte to see if there's any response. That's all
there is to it.

All EDID validation happens in the _drm_do_get_edid() when actually
reading the EDID.

I don't think I like duplicating this behaviour in both the probe and
the EDID read. And I'm not sure why we're giving drivers the option to
pass a parameter whether to validate or not. Just why?

BR,
Jani.


>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/gpu/drm/drm_edid.c | 50 +++++++++++++++++++++++++++++---------
>  include/drm/drm_edid.h     |  2 +-
>  2 files changed, 39 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index a3e9333f9177a..4e12d4b83a720 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -1758,6 +1758,18 @@ static void edid_header_fix(void *edid)
>  	memcpy(edid, edid_header, sizeof(edid_header));
>  }
>  
> +static int edid_header_score(const u8 *header)
> +{
> +	int i, score = 0;
> +
> +	for (i = 0; i < sizeof(edid_header); i++) {
> +		if (header[i] == edid_header[i])
> +			score++;
> +	}
> +
> +	return score;
> +}
> +
>  /**
>   * drm_edid_header_is_valid - sanity check the header of the base EDID block
>   * @_edid: pointer to raw base EDID block
> @@ -1769,14 +1781,8 @@ static void edid_header_fix(void *edid)
>  int drm_edid_header_is_valid(const void *_edid)
>  {
>  	const struct edid *edid = _edid;
> -	int i, score = 0;
>  
> -	for (i = 0; i < sizeof(edid_header); i++) {
> -		if (edid->header[i] == edid_header[i])
> -			score++;
> -	}
> -
> -	return score;
> +	return edid_header_score(edid->header);
>  }
>  EXPORT_SYMBOL(drm_edid_header_is_valid);
>  
> @@ -2612,17 +2618,37 @@ EXPORT_SYMBOL(drm_edid_free);
>   * drm_edid_probe_custom() - probe DDC presence
>   * @read_block: EDID block read function
>   * @context: Private data passed to the block read function
> + * @validate: True to validate the EDID header
>   *
>   * Probes for EDID data. Only reads enough data to detect the presence
> - * of the EDID channel.
> + * of the EDID channel. Some EDID block read functions do not fail,
> + * but return invalid data if no EDID data is available. If @validate
> + * has been specified, drm_edid_probe_custom() validates the retrieved
> + * EDID header before signalling success.
>   *
>   * Return: True on success, false on failure.
>   */
> -bool drm_edid_probe_custom(read_block_fn read_block, void *context)
> +bool drm_edid_probe_custom(read_block_fn read_block, void *context, bool validate)
>  {
> -	unsigned char out;
> +	u8 header[8] = {0, 0, 0, 0, 0, 0, 0, 0};
> +	int ret;
> +	size_t len = 1;
> +
> +	if (validate)
> +		len = sizeof(header); // read full header
> +
> +	ret = read_block(context, header, 0, len);
> +	if (ret)
> +		return false;
> +
> +	if (validate) {
> +		int score = edid_header_score(header);
> +
> +		if (score < clamp(edid_fixup, 0, 8))
> +			return false;
> +	}
>  
> -	return (read_block(context, &out, 0, 1) == 0);
> +	return true;
>  }
>  EXPORT_SYMBOL(drm_edid_probe_custom);
>  
> @@ -2635,7 +2661,7 @@ EXPORT_SYMBOL(drm_edid_probe_custom);
>  bool
>  drm_probe_ddc(struct i2c_adapter *adapter)
>  {
> -	return drm_edid_probe_custom(drm_do_probe_ddc_edid, adapter);
> +	return drm_edid_probe_custom(drm_do_probe_ddc_edid, adapter, false);
>  }
>  EXPORT_SYMBOL(drm_probe_ddc);
>  
> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> index 4d1797ade5f1d..299278c7ee1c2 100644
> --- a/include/drm/drm_edid.h
> +++ b/include/drm/drm_edid.h
> @@ -412,7 +412,7 @@ static inline void drm_edid_decode_panel_id(u32 panel_id, char vend[4], u16 *pro
>  
>  bool
>  drm_edid_probe_custom(int (*read_block)(void *context, u8 *buf, unsigned int block, size_t len),
> -		      void *context);
> +		      void *context, bool validate);
>  bool drm_probe_ddc(struct i2c_adapter *adapter);
>  struct edid *drm_do_get_edid(struct drm_connector *connector,
>  	int (*get_edid_block)(void *data, u8 *buf, unsigned int block,

-- 
Jani Nikula, Intel

Re: [PATCH 2/7] drm/edid: Test for EDID header in drm_edit_probe_custom()

[Thomas Zimmermann]

Hi

Am 05.04.24 um 08:38 schrieb Jani Nikula:
> On Thu, 04 Apr 2024, Thomas Zimmermann <tzimmermann@suse.de> wrote:
>> EDID read functions do not validate their return data. So they might
>> return the required number of bytes of probing, but with invalid
>> data. Therefore test for the presence of an EDID header similar to
>> the code in edid_block_check().
> I don't think the point of drm_probe_ddc() ever was to validate
> anything. It reads one byte to see if there's any response. That's all
> there is to it.
>
> All EDID validation happens in the _drm_do_get_edid() when actually
> reading the EDID.
>
> I don't think I like duplicating this behaviour in both the probe and
> the EDID read. And I'm not sure why we're giving drivers the option to
> pass a parameter whether to validate or not. Just why?

Udl doesn't use DDC, but a custom USB protocol. When queried for the 
EDID, the udl adapter sends data even if there's no monitor connected. 
All bytes are zero in this case. So the driver needs to do some sort of 
EDID validation on the received bytes to ensure that there's a monitor 
present.

drm_edid.c has all code necessary to validate the header. And there's 
the edid_fixup parameter, which I think should be respected by any 
validation helper. I'd preferably not duplicate this in udl.

Can we instead implement drm_probe_ddc() separately from 
drm_edid_probe_custom()? The former would remain as it is. The latter 
would validate unconditionally.

Best regards
Thomas

>
> BR,
> Jani.
>
>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>>   drivers/gpu/drm/drm_edid.c | 50 +++++++++++++++++++++++++++++---------
>>   include/drm/drm_edid.h     |  2 +-
>>   2 files changed, 39 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>> index a3e9333f9177a..4e12d4b83a720 100644
>> --- a/drivers/gpu/drm/drm_edid.c
>> +++ b/drivers/gpu/drm/drm_edid.c
>> @@ -1758,6 +1758,18 @@ static void edid_header_fix(void *edid)
>>   	memcpy(edid, edid_header, sizeof(edid_header));
>>   }
>>   
>> +static int edid_header_score(const u8 *header)
>> +{
>> +	int i, score = 0;
>> +
>> +	for (i = 0; i < sizeof(edid_header); i++) {
>> +		if (header[i] == edid_header[i])
>> +			score++;
>> +	}
>> +
>> +	return score;
>> +}
>> +
>>   /**
>>    * drm_edid_header_is_valid - sanity check the header of the base EDID block
>>    * @_edid: pointer to raw base EDID block
>> @@ -1769,14 +1781,8 @@ static void edid_header_fix(void *edid)
>>   int drm_edid_header_is_valid(const void *_edid)
>>   {
>>   	const struct edid *edid = _edid;
>> -	int i, score = 0;
>>   
>> -	for (i = 0; i < sizeof(edid_header); i++) {
>> -		if (edid->header[i] == edid_header[i])
>> -			score++;
>> -	}
>> -
>> -	return score;
>> +	return edid_header_score(edid->header);
>>   }
>>   EXPORT_SYMBOL(drm_edid_header_is_valid);
>>   
>> @@ -2612,17 +2618,37 @@ EXPORT_SYMBOL(drm_edid_free);
>>    * drm_edid_probe_custom() - probe DDC presence
>>    * @read_block: EDID block read function
>>    * @context: Private data passed to the block read function
>> + * @validate: True to validate the EDID header
>>    *
>>    * Probes for EDID data. Only reads enough data to detect the presence
>> - * of the EDID channel.
>> + * of the EDID channel. Some EDID block read functions do not fail,
>> + * but return invalid data if no EDID data is available. If @validate
>> + * has been specified, drm_edid_probe_custom() validates the retrieved
>> + * EDID header before signalling success.
>>    *
>>    * Return: True on success, false on failure.
>>    */
>> -bool drm_edid_probe_custom(read_block_fn read_block, void *context)
>> +bool drm_edid_probe_custom(read_block_fn read_block, void *context, bool validate)
>>   {
>> -	unsigned char out;
>> +	u8 header[8] = {0, 0, 0, 0, 0, 0, 0, 0};
>> +	int ret;
>> +	size_t len = 1;
>> +
>> +	if (validate)
>> +		len = sizeof(header); // read full header
>> +
>> +	ret = read_block(context, header, 0, len);
>> +	if (ret)
>> +		return false;
>> +
>> +	if (validate) {
>> +		int score = edid_header_score(header);
>> +
>> +		if (score < clamp(edid_fixup, 0, 8))
>> +			return false;
>> +	}
>>   
>> -	return (read_block(context, &out, 0, 1) == 0);
>> +	return true;
>>   }
>>   EXPORT_SYMBOL(drm_edid_probe_custom);
>>   
>> @@ -2635,7 +2661,7 @@ EXPORT_SYMBOL(drm_edid_probe_custom);
>>   bool
>>   drm_probe_ddc(struct i2c_adapter *adapter)
>>   {
>> -	return drm_edid_probe_custom(drm_do_probe_ddc_edid, adapter);
>> +	return drm_edid_probe_custom(drm_do_probe_ddc_edid, adapter, false);
>>   }
>>   EXPORT_SYMBOL(drm_probe_ddc);
>>   
>> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
>> index 4d1797ade5f1d..299278c7ee1c2 100644
>> --- a/include/drm/drm_edid.h
>> +++ b/include/drm/drm_edid.h
>> @@ -412,7 +412,7 @@ static inline void drm_edid_decode_panel_id(u32 panel_id, char vend[4], u16 *pro
>>   
>>   bool
>>   drm_edid_probe_custom(int (*read_block)(void *context, u8 *buf, unsigned int block, size_t len),
>> -		      void *context);
>> +		      void *context, bool validate);
>>   bool drm_probe_ddc(struct i2c_adapter *adapter);
>>   struct edid *drm_do_get_edid(struct drm_connector *connector,
>>   	int (*get_edid_block)(void *data, u8 *buf, unsigned int block,

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

[PATCH 3/7] drm/udl: Remove DRM_CONNECTOR_POLL_HPD

[Thomas Zimmermann]

DisplayLink devices do not generate hotplug events. Remove the poll
flag DRM_CONNECTOR_POLL_HPD, as it may not be specified together with
DRM_CONNECTOR_POLL_CONNECT or DRM_CONNECTOR_POLL_DISCONNECT.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Fixes: afdfc4c6f55f ("drm/udl: Fixed problem with UDL adpater reconnection")
Cc: Robert Tarasov <tutankhamen@chromium.org>
Cc: Alex Deucher <alexander.deucher@amd.com>
Cc: Dave Airlie <airlied@redhat.com>
Cc: Sean Paul <sean@poorly.run>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: dri-devel@lists.freedesktop.org
Cc: <stable@vger.kernel.org> # v4.15+
---
 drivers/gpu/drm/udl/udl_modeset.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c
index 7702359c90c22..751da3a294c44 100644
--- a/drivers/gpu/drm/udl/udl_modeset.c
+++ b/drivers/gpu/drm/udl/udl_modeset.c
@@ -527,8 +527,7 @@ struct drm_connector *udl_connector_init(struct drm_device *dev)
 
 	drm_connector_helper_add(connector, &udl_connector_helper_funcs);
 
-	connector->polled = DRM_CONNECTOR_POLL_HPD |
-			    DRM_CONNECTOR_POLL_CONNECT |
+	connector->polled = DRM_CONNECTOR_POLL_CONNECT |
 			    DRM_CONNECTOR_POLL_DISCONNECT;
 
 	return connector;
-- 
2.44.0

[PATCH 4/7] drm/udl: Move drm_dev_{enter, exit}() into udl_get_edid_block()

[Thomas Zimmermann]

Protect the code in udl_get_edid_block() with drm_dev_enter() and
drm_dev_exit(), so that all callers automatically invoke it. The
function uses hardware resources, which can be hot-unplugged at
any time. The other code in udl_connector_detect() does not use the
resources of the hardware device and therefore does not require
protection.

This change will allow to use udl_get_edid_block() in various
contexts easily.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/udl/udl_modeset.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c
index 751da3a294c44..3df9fc38388b4 100644
--- a/drivers/gpu/drm/udl/udl_modeset.c
+++ b/drivers/gpu/drm/udl/udl_modeset.c
@@ -434,13 +434,18 @@ static int udl_get_edid_block(void *data, u8 *buf, unsigned int block, size_t le
 	struct drm_device *dev = &udl->drm;
 	struct usb_device *udev = udl_to_usb_device(udl);
 	u8 *read_buff;
-	int ret;
+	int idx, ret;
 	size_t i;
 
 	read_buff = kmalloc(2, GFP_KERNEL);
 	if (!read_buff)
 		return -ENOMEM;
 
+	if (!drm_dev_enter(dev, &idx)) {
+		ret = -ENODEV;
+		goto err_kfree;
+	}
+
 	for (i = 0; i < len; i++) {
 		int bval = (i + block * EDID_LENGTH) << 8;
 
@@ -449,20 +454,23 @@ static int udl_get_edid_block(void *data, u8 *buf, unsigned int block, size_t le
 				      0xA1, read_buff, 2, USB_CTRL_GET_TIMEOUT);
 		if (ret < 0) {
 			drm_err(dev, "Read EDID byte %zu failed err %x\n", i, ret);
-			goto err_kfree;
+			goto err_drm_dev_exit;
 		} else if (ret < 1) {
 			ret = -EIO;
 			drm_err(dev, "Read EDID byte %zu failed\n", i);
-			goto err_kfree;
+			goto err_drm_dev_exit;
 		}
 
 		buf[i] = read_buff[1];
 	}
 
+	drm_dev_exit(idx);
 	kfree(read_buff);
 
 	return 0;
 
+err_drm_dev_exit:
+	drm_dev_exit(idx);
 err_kfree:
 	kfree(read_buff);
 	return ret;
@@ -474,21 +482,15 @@ static enum drm_connector_status udl_connector_detect(struct drm_connector *conn
 	struct udl_device *udl = to_udl(dev);
 	struct udl_connector *udl_connector = to_udl_connector(connector);
 	enum drm_connector_status status = connector_status_disconnected;
-	int idx;
 
 	/* cleanup previous EDID */
 	kfree(udl_connector->edid);
 	udl_connector->edid = NULL;
 
-	if (!drm_dev_enter(dev, &idx))
-		return connector_status_disconnected;
-
 	udl_connector->edid = drm_do_get_edid(connector, udl_get_edid_block, udl);
 	if (udl_connector->edid)
 		status = connector_status_connected;
 
-	drm_dev_exit(idx);
-
 	return status;
 }
 
-- 
2.44.0

[PATCH 5/7] drm/udl: Clean up Makefile

[Thomas Zimmermann]

Clean up Makefile before listing new object files. No functional
changes.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/udl/Makefile | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/udl/Makefile b/drivers/gpu/drm/udl/Makefile
index 3f6db179455d1..00690741db376 100644
--- a/drivers/gpu/drm/udl/Makefile
+++ b/drivers/gpu/drm/udl/Makefile
@@ -1,4 +1,9 @@
 # SPDX-License-Identifier: GPL-2.0-only
-udl-y := udl_drv.o udl_modeset.o udl_main.o udl_transfer.o
+
+udl-y := \
+	udl_drv.o \
+	udl_main.o \
+	udl_modeset.o \
+	udl_transfer.o
 
 obj-$(CONFIG_DRM_UDL) := udl.o
-- 
2.44.0

[PATCH 6/7] drm/udl: Untangle .get_modes() and .detect_ctx()

[Thomas Zimmermann]

Provide separate implementations of .get_modes() and .detect_ctx()
from struct drm_connector. Switch to struct drm_edid.

Udl's .detect() helper used to fetch the EDID from the adapter and the
.get_modes() helper provided display modes from the data. Switching to
the new helpers around struct drm_edid separates both from each other. The
.get_modes() helper now fetches the EDID by itself and the .detect_ctx()
helper only tests for its presence.

The patch does a number of things to implement this.

- Move udl_get_edid_block() to udl_edid.c and rename it to
udl_read_edid_block(). Then use the helper to implement probing in
udl_probe_edid() and reading in udl_edid_read(). Both udl helpers
are build on top of DRM helpers.

- Replace the existing code in .get_modes() and .detect() with udl's
new EDID helpers. The new code behaves like DRM's similar DDC-based
helpers. Instead of .detect(), udl now implements .detect_ctx().

- Remove the edid data from struct udl_connector. The field cached
the EDID data between calls to .detect() and .get_modes(), but is now
unused.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/udl/Makefile      |  1 +
 drivers/gpu/drm/udl/udl_drv.h     |  2 -
 drivers/gpu/drm/udl/udl_edid.c    | 67 +++++++++++++++++++++++
 drivers/gpu/drm/udl/udl_edid.h    | 15 ++++++
 drivers/gpu/drm/udl/udl_modeset.c | 90 +++++++------------------------
 5 files changed, 102 insertions(+), 73 deletions(-)
 create mode 100644 drivers/gpu/drm/udl/udl_edid.c
 create mode 100644 drivers/gpu/drm/udl/udl_edid.h

diff --git a/drivers/gpu/drm/udl/Makefile b/drivers/gpu/drm/udl/Makefile
index 00690741db376..43d69a16af183 100644
--- a/drivers/gpu/drm/udl/Makefile
+++ b/drivers/gpu/drm/udl/Makefile
@@ -2,6 +2,7 @@
 
 udl-y := \
 	udl_drv.o \
+	udl_edid.o \
 	udl_main.o \
 	udl_modeset.o \
 	udl_transfer.o
diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
index 282ebd6c02fda..f112cfb270f31 100644
--- a/drivers/gpu/drm/udl/udl_drv.h
+++ b/drivers/gpu/drm/udl/udl_drv.h
@@ -51,8 +51,6 @@ struct urb_list {
 
 struct udl_connector {
 	struct drm_connector connector;
-	/* last udl_detect edid */
-	struct edid *edid;
 };
 
 static inline struct udl_connector *to_udl_connector(struct drm_connector *connector)
diff --git a/drivers/gpu/drm/udl/udl_edid.c b/drivers/gpu/drm/udl/udl_edid.c
new file mode 100644
index 0000000000000..caa9641996e92
--- /dev/null
+++ b/drivers/gpu/drm/udl/udl_edid.c
@@ -0,0 +1,67 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include <drm/drm_drv.h>
+#include <drm/drm_edid.h>
+
+#include "udl_drv.h"
+#include "udl_edid.h"
+
+static int udl_read_edid_block(void *data, u8 *buf, unsigned int block, size_t len)
+{
+	struct udl_device *udl = data;
+	struct drm_device *dev = &udl->drm;
+	struct usb_device *udev = udl_to_usb_device(udl);
+	u8 *read_buff;
+	int idx, ret;
+	size_t i;
+
+	read_buff = kmalloc(2, GFP_KERNEL);
+	if (!read_buff)
+		return -ENOMEM;
+
+	if (!drm_dev_enter(dev, &idx)) {
+		ret = -ENODEV;
+		goto err_kfree;
+	}
+
+	for (i = 0; i < len; i++) {
+		int bval = (i + block * EDID_LENGTH) << 8;
+
+		ret = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0),
+				      0x02, (0x80 | (0x02 << 5)), bval,
+				      0xA1, read_buff, 2, USB_CTRL_GET_TIMEOUT);
+		if (ret < 0) {
+			drm_err(dev, "Read EDID byte %zu failed err %x\n", i, ret);
+			goto err_drm_dev_exit;
+		} else if (ret < 1) {
+			ret = -EIO;
+			drm_err(dev, "Read EDID byte %zu failed\n", i);
+			goto err_drm_dev_exit;
+		}
+
+		buf[i] = read_buff[1];
+	}
+
+	drm_dev_exit(idx);
+	kfree(read_buff);
+
+	return 0;
+
+err_drm_dev_exit:
+	drm_dev_exit(idx);
+err_kfree:
+	kfree(read_buff);
+	return ret;
+}
+
+bool udl_probe_edid(struct udl_device *udl)
+{
+	return drm_edid_probe_custom(udl_read_edid_block, udl, true);
+}
+
+const struct drm_edid *udl_edid_read(struct drm_connector *connector)
+{
+	struct udl_device *udl = to_udl(connector->dev);
+
+	return drm_edid_read_custom(connector, udl_read_edid_block, udl);
+}
diff --git a/drivers/gpu/drm/udl/udl_edid.h b/drivers/gpu/drm/udl/udl_edid.h
new file mode 100644
index 0000000000000..fe15ff3752b7d
--- /dev/null
+++ b/drivers/gpu/drm/udl/udl_edid.h
@@ -0,0 +1,15 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+#ifndef UDL_EDID_H
+#define UDL_EDID_H
+
+#include <linux/types.h>
+
+struct drm_connector;
+struct drm_edid;
+struct udl_device;
+
+bool udl_probe_edid(struct udl_device *udl);
+const struct drm_edid *udl_edid_read(struct drm_connector *connector);
+
+#endif
diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c
index 3df9fc38388b4..4236ce57f5945 100644
--- a/drivers/gpu/drm/udl/udl_modeset.c
+++ b/drivers/gpu/drm/udl/udl_modeset.c
@@ -25,6 +25,7 @@
 #include <drm/drm_vblank.h>
 
 #include "udl_drv.h"
+#include "udl_edid.h"
 #include "udl_proto.h"
 
 /*
@@ -415,97 +416,44 @@ static const struct drm_encoder_funcs udl_encoder_funcs = {
 
 static int udl_connector_helper_get_modes(struct drm_connector *connector)
 {
-	struct udl_connector *udl_connector = to_udl_connector(connector);
+	const struct drm_edid *drm_edid;
+	int count;
 
-	drm_connector_update_edid_property(connector, udl_connector->edid);
-	if (udl_connector->edid)
-		return drm_add_edid_modes(connector, udl_connector->edid);
+	drm_edid = udl_edid_read(connector);
+	drm_edid_connector_update(connector, drm_edid);
+	count = drm_edid_connector_add_modes(connector);
+	drm_edid_free(drm_edid);
 
-	return 0;
+	return count;
 }
 
-static const struct drm_connector_helper_funcs udl_connector_helper_funcs = {
-	.get_modes = udl_connector_helper_get_modes,
-};
-
-static int udl_get_edid_block(void *data, u8 *buf, unsigned int block, size_t len)
+static int udl_connector_helper_detect_ctx(struct drm_connector *connector,
+					   struct drm_modeset_acquire_ctx *ctx,
+					   bool force)
 {
-	struct udl_device *udl = data;
-	struct drm_device *dev = &udl->drm;
-	struct usb_device *udev = udl_to_usb_device(udl);
-	u8 *read_buff;
-	int idx, ret;
-	size_t i;
-
-	read_buff = kmalloc(2, GFP_KERNEL);
-	if (!read_buff)
-		return -ENOMEM;
+	struct udl_device *udl = to_udl(connector->dev);
 
-	if (!drm_dev_enter(dev, &idx)) {
-		ret = -ENODEV;
-		goto err_kfree;
-	}
-
-	for (i = 0; i < len; i++) {
-		int bval = (i + block * EDID_LENGTH) << 8;
-
-		ret = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0),
-				      0x02, (0x80 | (0x02 << 5)), bval,
-				      0xA1, read_buff, 2, USB_CTRL_GET_TIMEOUT);
-		if (ret < 0) {
-			drm_err(dev, "Read EDID byte %zu failed err %x\n", i, ret);
-			goto err_drm_dev_exit;
-		} else if (ret < 1) {
-			ret = -EIO;
-			drm_err(dev, "Read EDID byte %zu failed\n", i);
-			goto err_drm_dev_exit;
-		}
-
-		buf[i] = read_buff[1];
-	}
+	if (udl_probe_edid(udl))
+		return connector_status_connected;
 
-	drm_dev_exit(idx);
-	kfree(read_buff);
-
-	return 0;
-
-err_drm_dev_exit:
-	drm_dev_exit(idx);
-err_kfree:
-	kfree(read_buff);
-	return ret;
+	return connector_status_disconnected;
 }
 
-static enum drm_connector_status udl_connector_detect(struct drm_connector *connector, bool force)
-{
-	struct drm_device *dev = connector->dev;
-	struct udl_device *udl = to_udl(dev);
-	struct udl_connector *udl_connector = to_udl_connector(connector);
-	enum drm_connector_status status = connector_status_disconnected;
-
-	/* cleanup previous EDID */
-	kfree(udl_connector->edid);
-	udl_connector->edid = NULL;
-
-	udl_connector->edid = drm_do_get_edid(connector, udl_get_edid_block, udl);
-	if (udl_connector->edid)
-		status = connector_status_connected;
-
-	return status;
-}
+static const struct drm_connector_helper_funcs udl_connector_helper_funcs = {
+	.get_modes = udl_connector_helper_get_modes,
+	.detect_ctx = udl_connector_helper_detect_ctx,
+};
 
 static void udl_connector_destroy(struct drm_connector *connector)
 {
 	struct udl_connector *udl_connector = to_udl_connector(connector);
 
 	drm_connector_cleanup(connector);
-	kfree(udl_connector->edid);
 	kfree(udl_connector);
 }
 
 static const struct drm_connector_funcs udl_connector_funcs = {
 	.reset = drm_atomic_helper_connector_reset,
-	.detect = udl_connector_detect,
 	.fill_modes = drm_helper_probe_single_connector_modes,
 	.destroy = udl_connector_destroy,
 	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
-- 
2.44.0

Re: [PATCH 6/7] drm/udl: Untangle .get_modes() and .detect_ctx()

[Jani Nikula]

On Thu, 04 Apr 2024, Thomas Zimmermann <tzimmermann@suse.de> wrote:
> Provide separate implementations of .get_modes() and .detect_ctx()
> from struct drm_connector. Switch to struct drm_edid.
>
> Udl's .detect() helper used to fetch the EDID from the adapter and the
> .get_modes() helper provided display modes from the data. Switching to
> the new helpers around struct drm_edid separates both from each other. The
> .get_modes() helper now fetches the EDID by itself and the .detect_ctx()
> helper only tests for its presence.

FWIW, this is what I had for UDL in my branch of various drm_edid
conversions:

https://gitlab.freedesktop.org/jani/linux/-/commit/c6357a778182eff7acfb1eb832809377f799edaf

You seem to claim that there's inherent value in separating detect and
get_modes hooks from each other, with the former not reading the full
EDID.

If you do read the full EDID using drm_edid_read_custom() in detect,
you'll get more validation of the EDID for free, with no need to add
extra drm edid interfaces for probing with optional header validation.

Some drivers need to use EDID contents for the purpose of detect
too. Perhaps not UDL.

Also let's look at the override/firmware EDID mechanism. If you have a
completely broken EDID, the newly added drm_edid_probe_custom() will
never detect connected. You'll need to use connector forcing, even if
the DDC probe would have been enough.

BR,
Jani.

>
> The patch does a number of things to implement this.
>
> - Move udl_get_edid_block() to udl_edid.c and rename it to
> udl_read_edid_block(). Then use the helper to implement probing in
> udl_probe_edid() and reading in udl_edid_read(). Both udl helpers
> are build on top of DRM helpers.
>
> - Replace the existing code in .get_modes() and .detect() with udl's
> new EDID helpers. The new code behaves like DRM's similar DDC-based
> helpers. Instead of .detect(), udl now implements .detect_ctx().
>
> - Remove the edid data from struct udl_connector. The field cached
> the EDID data between calls to .detect() and .get_modes(), but is now
> unused.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/gpu/drm/udl/Makefile      |  1 +
>  drivers/gpu/drm/udl/udl_drv.h     |  2 -
>  drivers/gpu/drm/udl/udl_edid.c    | 67 +++++++++++++++++++++++
>  drivers/gpu/drm/udl/udl_edid.h    | 15 ++++++
>  drivers/gpu/drm/udl/udl_modeset.c | 90 +++++++------------------------
>  5 files changed, 102 insertions(+), 73 deletions(-)
>  create mode 100644 drivers/gpu/drm/udl/udl_edid.c
>  create mode 100644 drivers/gpu/drm/udl/udl_edid.h
>
> diff --git a/drivers/gpu/drm/udl/Makefile b/drivers/gpu/drm/udl/Makefile
> index 00690741db376..43d69a16af183 100644
> --- a/drivers/gpu/drm/udl/Makefile
> +++ b/drivers/gpu/drm/udl/Makefile
> @@ -2,6 +2,7 @@
>  
>  udl-y := \
>  	udl_drv.o \
> +	udl_edid.o \
>  	udl_main.o \
>  	udl_modeset.o \
>  	udl_transfer.o
> diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
> index 282ebd6c02fda..f112cfb270f31 100644
> --- a/drivers/gpu/drm/udl/udl_drv.h
> +++ b/drivers/gpu/drm/udl/udl_drv.h
> @@ -51,8 +51,6 @@ struct urb_list {
>  
>  struct udl_connector {
>  	struct drm_connector connector;
> -	/* last udl_detect edid */
> -	struct edid *edid;
>  };
>  
>  static inline struct udl_connector *to_udl_connector(struct drm_connector *connector)
> diff --git a/drivers/gpu/drm/udl/udl_edid.c b/drivers/gpu/drm/udl/udl_edid.c
> new file mode 100644
> index 0000000000000..caa9641996e92
> --- /dev/null
> +++ b/drivers/gpu/drm/udl/udl_edid.c
> @@ -0,0 +1,67 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +#include <drm/drm_drv.h>
> +#include <drm/drm_edid.h>
> +
> +#include "udl_drv.h"
> +#include "udl_edid.h"
> +
> +static int udl_read_edid_block(void *data, u8 *buf, unsigned int block, size_t len)
> +{
> +	struct udl_device *udl = data;
> +	struct drm_device *dev = &udl->drm;
> +	struct usb_device *udev = udl_to_usb_device(udl);
> +	u8 *read_buff;
> +	int idx, ret;
> +	size_t i;
> +
> +	read_buff = kmalloc(2, GFP_KERNEL);
> +	if (!read_buff)
> +		return -ENOMEM;
> +
> +	if (!drm_dev_enter(dev, &idx)) {
> +		ret = -ENODEV;
> +		goto err_kfree;
> +	}
> +
> +	for (i = 0; i < len; i++) {
> +		int bval = (i + block * EDID_LENGTH) << 8;
> +
> +		ret = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0),
> +				      0x02, (0x80 | (0x02 << 5)), bval,
> +				      0xA1, read_buff, 2, USB_CTRL_GET_TIMEOUT);
> +		if (ret < 0) {
> +			drm_err(dev, "Read EDID byte %zu failed err %x\n", i, ret);
> +			goto err_drm_dev_exit;
> +		} else if (ret < 1) {
> +			ret = -EIO;
> +			drm_err(dev, "Read EDID byte %zu failed\n", i);
> +			goto err_drm_dev_exit;
> +		}
> +
> +		buf[i] = read_buff[1];
> +	}
> +
> +	drm_dev_exit(idx);
> +	kfree(read_buff);
> +
> +	return 0;
> +
> +err_drm_dev_exit:
> +	drm_dev_exit(idx);
> +err_kfree:
> +	kfree(read_buff);
> +	return ret;
> +}
> +
> +bool udl_probe_edid(struct udl_device *udl)
> +{
> +	return drm_edid_probe_custom(udl_read_edid_block, udl, true);
> +}
> +
> +const struct drm_edid *udl_edid_read(struct drm_connector *connector)
> +{
> +	struct udl_device *udl = to_udl(connector->dev);
> +
> +	return drm_edid_read_custom(connector, udl_read_edid_block, udl);
> +}
> diff --git a/drivers/gpu/drm/udl/udl_edid.h b/drivers/gpu/drm/udl/udl_edid.h
> new file mode 100644
> index 0000000000000..fe15ff3752b7d
> --- /dev/null
> +++ b/drivers/gpu/drm/udl/udl_edid.h
> @@ -0,0 +1,15 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +
> +#ifndef UDL_EDID_H
> +#define UDL_EDID_H
> +
> +#include <linux/types.h>
> +
> +struct drm_connector;
> +struct drm_edid;
> +struct udl_device;
> +
> +bool udl_probe_edid(struct udl_device *udl);
> +const struct drm_edid *udl_edid_read(struct drm_connector *connector);
> +
> +#endif
> diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c
> index 3df9fc38388b4..4236ce57f5945 100644
> --- a/drivers/gpu/drm/udl/udl_modeset.c
> +++ b/drivers/gpu/drm/udl/udl_modeset.c
> @@ -25,6 +25,7 @@
>  #include <drm/drm_vblank.h>
>  
>  #include "udl_drv.h"
> +#include "udl_edid.h"
>  #include "udl_proto.h"
>  
>  /*
> @@ -415,97 +416,44 @@ static const struct drm_encoder_funcs udl_encoder_funcs = {
>  
>  static int udl_connector_helper_get_modes(struct drm_connector *connector)
>  {
> -	struct udl_connector *udl_connector = to_udl_connector(connector);
> +	const struct drm_edid *drm_edid;
> +	int count;
>  
> -	drm_connector_update_edid_property(connector, udl_connector->edid);
> -	if (udl_connector->edid)
> -		return drm_add_edid_modes(connector, udl_connector->edid);
> +	drm_edid = udl_edid_read(connector);
> +	drm_edid_connector_update(connector, drm_edid);
> +	count = drm_edid_connector_add_modes(connector);
> +	drm_edid_free(drm_edid);
>  
> -	return 0;
> +	return count;
>  }
>  
> -static const struct drm_connector_helper_funcs udl_connector_helper_funcs = {
> -	.get_modes = udl_connector_helper_get_modes,
> -};
> -
> -static int udl_get_edid_block(void *data, u8 *buf, unsigned int block, size_t len)
> +static int udl_connector_helper_detect_ctx(struct drm_connector *connector,
> +					   struct drm_modeset_acquire_ctx *ctx,
> +					   bool force)
>  {
> -	struct udl_device *udl = data;
> -	struct drm_device *dev = &udl->drm;
> -	struct usb_device *udev = udl_to_usb_device(udl);
> -	u8 *read_buff;
> -	int idx, ret;
> -	size_t i;
> -
> -	read_buff = kmalloc(2, GFP_KERNEL);
> -	if (!read_buff)
> -		return -ENOMEM;
> +	struct udl_device *udl = to_udl(connector->dev);
>  
> -	if (!drm_dev_enter(dev, &idx)) {
> -		ret = -ENODEV;
> -		goto err_kfree;
> -	}
> -
> -	for (i = 0; i < len; i++) {
> -		int bval = (i + block * EDID_LENGTH) << 8;
> -
> -		ret = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0),
> -				      0x02, (0x80 | (0x02 << 5)), bval,
> -				      0xA1, read_buff, 2, USB_CTRL_GET_TIMEOUT);
> -		if (ret < 0) {
> -			drm_err(dev, "Read EDID byte %zu failed err %x\n", i, ret);
> -			goto err_drm_dev_exit;
> -		} else if (ret < 1) {
> -			ret = -EIO;
> -			drm_err(dev, "Read EDID byte %zu failed\n", i);
> -			goto err_drm_dev_exit;
> -		}
> -
> -		buf[i] = read_buff[1];
> -	}
> +	if (udl_probe_edid(udl))
> +		return connector_status_connected;
>  
> -	drm_dev_exit(idx);
> -	kfree(read_buff);
> -
> -	return 0;
> -
> -err_drm_dev_exit:
> -	drm_dev_exit(idx);
> -err_kfree:
> -	kfree(read_buff);
> -	return ret;
> +	return connector_status_disconnected;
>  }
>  
> -static enum drm_connector_status udl_connector_detect(struct drm_connector *connector, bool force)
> -{
> -	struct drm_device *dev = connector->dev;
> -	struct udl_device *udl = to_udl(dev);
> -	struct udl_connector *udl_connector = to_udl_connector(connector);
> -	enum drm_connector_status status = connector_status_disconnected;
> -
> -	/* cleanup previous EDID */
> -	kfree(udl_connector->edid);
> -	udl_connector->edid = NULL;
> -
> -	udl_connector->edid = drm_do_get_edid(connector, udl_get_edid_block, udl);
> -	if (udl_connector->edid)
> -		status = connector_status_connected;
> -
> -	return status;
> -}
> +static const struct drm_connector_helper_funcs udl_connector_helper_funcs = {
> +	.get_modes = udl_connector_helper_get_modes,
> +	.detect_ctx = udl_connector_helper_detect_ctx,
> +};
>  
>  static void udl_connector_destroy(struct drm_connector *connector)
>  {
>  	struct udl_connector *udl_connector = to_udl_connector(connector);
>  
>  	drm_connector_cleanup(connector);
> -	kfree(udl_connector->edid);
>  	kfree(udl_connector);
>  }
>  
>  static const struct drm_connector_funcs udl_connector_funcs = {
>  	.reset = drm_atomic_helper_connector_reset,
> -	.detect = udl_connector_detect,
>  	.fill_modes = drm_helper_probe_single_connector_modes,
>  	.destroy = udl_connector_destroy,
>  	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,

-- 
Jani Nikula, Intel

Re: [PATCH 6/7] drm/udl: Untangle .get_modes() and .detect_ctx()

[Thomas Zimmermann]

Hi

Am 05.04.24 um 09:09 schrieb Jani Nikula:
> On Thu, 04 Apr 2024, Thomas Zimmermann <tzimmermann@suse.de> wrote:
>> Provide separate implementations of .get_modes() and .detect_ctx()
>> from struct drm_connector. Switch to struct drm_edid.
>>
>> Udl's .detect() helper used to fetch the EDID from the adapter and the
>> .get_modes() helper provided display modes from the data. Switching to
>> the new helpers around struct drm_edid separates both from each other. The
>> .get_modes() helper now fetches the EDID by itself and the .detect_ctx()
>> helper only tests for its presence.
> FWIW, this is what I had for UDL in my branch of various drm_edid
> conversions:
>
> https://gitlab.freedesktop.org/jani/linux/-/commit/c6357a778182eff7acfb1eb832809377f799edaf
>
> You seem to claim that there's inherent value in separating detect and
> get_modes hooks from each other, with the former not reading the full
> EDID.

Yes. If udl stores the EDID in struct udl_connector and later uses it in 
get_modes, the detect always has to run before get_modes. That logic is 
deeply hidden in the DRM probe helpers. So in any case, I want get_modes 
to read the EDID by itself.

In detect or detect_ctx, using drm_edid_read_custom() might do 
unnecessary USB transfers. Maybe not a problem per-se, but udl is 
already pushing the limits of its USB 2 bus. And the read function also 
leaves a warning about the all-zero EDID data in the kernel log every 
few seconds. Probably from [1].

[1] 
https://elixir.bootlin.com/linux/v6.8/source/drivers/gpu/drm/drm_edid.c#L2389

>
> If you do read the full EDID using drm_edid_read_custom() in detect,
> you'll get more validation of the EDID for free, with no need to add
> extra drm edid interfaces for probing with optional header validation.
>
> Some drivers need to use EDID contents for the purpose of detect
> too. Perhaps not UDL.
>
> Also let's look at the override/firmware EDID mechanism. If you have a
> completely broken EDID, the newly added drm_edid_probe_custom() will
> never detect connected. You'll need to use connector forcing, even if
> the DDC probe would have been enough.

The EDID probe helper can take this into account. If it's just the code 
at [2], it looks simple enough to re-use.

[2] 
https://elixir.bootlin.com/linux/v6.8/source/drivers/gpu/drm/drm_edid.c#L2373

Best regards
Thomas

>
> BR,
> Jani.
>
>> The patch does a number of things to implement this.
>>
>> - Move udl_get_edid_block() to udl_edid.c and rename it to
>> udl_read_edid_block(). Then use the helper to implement probing in
>> udl_probe_edid() and reading in udl_edid_read(). Both udl helpers
>> are build on top of DRM helpers.
>>
>> - Replace the existing code in .get_modes() and .detect() with udl's
>> new EDID helpers. The new code behaves like DRM's similar DDC-based
>> helpers. Instead of .detect(), udl now implements .detect_ctx().
>>
>> - Remove the edid data from struct udl_connector. The field cached
>> the EDID data between calls to .detect() and .get_modes(), but is now
>> unused.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>>   drivers/gpu/drm/udl/Makefile      |  1 +
>>   drivers/gpu/drm/udl/udl_drv.h     |  2 -
>>   drivers/gpu/drm/udl/udl_edid.c    | 67 +++++++++++++++++++++++
>>   drivers/gpu/drm/udl/udl_edid.h    | 15 ++++++
>>   drivers/gpu/drm/udl/udl_modeset.c | 90 +++++++------------------------
>>   5 files changed, 102 insertions(+), 73 deletions(-)
>>   create mode 100644 drivers/gpu/drm/udl/udl_edid.c
>>   create mode 100644 drivers/gpu/drm/udl/udl_edid.h
>>
>> diff --git a/drivers/gpu/drm/udl/Makefile b/drivers/gpu/drm/udl/Makefile
>> index 00690741db376..43d69a16af183 100644
>> --- a/drivers/gpu/drm/udl/Makefile
>> +++ b/drivers/gpu/drm/udl/Makefile
>> @@ -2,6 +2,7 @@
>>   
>>   udl-y := \
>>   	udl_drv.o \
>> +	udl_edid.o \
>>   	udl_main.o \
>>   	udl_modeset.o \
>>   	udl_transfer.o
>> diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
>> index 282ebd6c02fda..f112cfb270f31 100644
>> --- a/drivers/gpu/drm/udl/udl_drv.h
>> +++ b/drivers/gpu/drm/udl/udl_drv.h
>> @@ -51,8 +51,6 @@ struct urb_list {
>>   
>>   struct udl_connector {
>>   	struct drm_connector connector;
>> -	/* last udl_detect edid */
>> -	struct edid *edid;
>>   };
>>   
>>   static inline struct udl_connector *to_udl_connector(struct drm_connector *connector)
>> diff --git a/drivers/gpu/drm/udl/udl_edid.c b/drivers/gpu/drm/udl/udl_edid.c
>> new file mode 100644
>> index 0000000000000..caa9641996e92
>> --- /dev/null
>> +++ b/drivers/gpu/drm/udl/udl_edid.c
>> @@ -0,0 +1,67 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +
>> +#include <drm/drm_drv.h>
>> +#include <drm/drm_edid.h>
>> +
>> +#include "udl_drv.h"
>> +#include "udl_edid.h"
>> +
>> +static int udl_read_edid_block(void *data, u8 *buf, unsigned int block, size_t len)
>> +{
>> +	struct udl_device *udl = data;
>> +	struct drm_device *dev = &udl->drm;
>> +	struct usb_device *udev = udl_to_usb_device(udl);
>> +	u8 *read_buff;
>> +	int idx, ret;
>> +	size_t i;
>> +
>> +	read_buff = kmalloc(2, GFP_KERNEL);
>> +	if (!read_buff)
>> +		return -ENOMEM;
>> +
>> +	if (!drm_dev_enter(dev, &idx)) {
>> +		ret = -ENODEV;
>> +		goto err_kfree;
>> +	}
>> +
>> +	for (i = 0; i < len; i++) {
>> +		int bval = (i + block * EDID_LENGTH) << 8;
>> +
>> +		ret = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0),
>> +				      0x02, (0x80 | (0x02 << 5)), bval,
>> +				      0xA1, read_buff, 2, USB_CTRL_GET_TIMEOUT);
>> +		if (ret < 0) {
>> +			drm_err(dev, "Read EDID byte %zu failed err %x\n", i, ret);
>> +			goto err_drm_dev_exit;
>> +		} else if (ret < 1) {
>> +			ret = -EIO;
>> +			drm_err(dev, "Read EDID byte %zu failed\n", i);
>> +			goto err_drm_dev_exit;
>> +		}
>> +
>> +		buf[i] = read_buff[1];
>> +	}
>> +
>> +	drm_dev_exit(idx);
>> +	kfree(read_buff);
>> +
>> +	return 0;
>> +
>> +err_drm_dev_exit:
>> +	drm_dev_exit(idx);
>> +err_kfree:
>> +	kfree(read_buff);
>> +	return ret;
>> +}
>> +
>> +bool udl_probe_edid(struct udl_device *udl)
>> +{
>> +	return drm_edid_probe_custom(udl_read_edid_block, udl, true);
>> +}
>> +
>> +const struct drm_edid *udl_edid_read(struct drm_connector *connector)
>> +{
>> +	struct udl_device *udl = to_udl(connector->dev);
>> +
>> +	return drm_edid_read_custom(connector, udl_read_edid_block, udl);
>> +}
>> diff --git a/drivers/gpu/drm/udl/udl_edid.h b/drivers/gpu/drm/udl/udl_edid.h
>> new file mode 100644
>> index 0000000000000..fe15ff3752b7d
>> --- /dev/null
>> +++ b/drivers/gpu/drm/udl/udl_edid.h
>> @@ -0,0 +1,15 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +
>> +#ifndef UDL_EDID_H
>> +#define UDL_EDID_H
>> +
>> +#include <linux/types.h>
>> +
>> +struct drm_connector;
>> +struct drm_edid;
>> +struct udl_device;
>> +
>> +bool udl_probe_edid(struct udl_device *udl);
>> +const struct drm_edid *udl_edid_read(struct drm_connector *connector);
>> +
>> +#endif
>> diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c
>> index 3df9fc38388b4..4236ce57f5945 100644
>> --- a/drivers/gpu/drm/udl/udl_modeset.c
>> +++ b/drivers/gpu/drm/udl/udl_modeset.c
>> @@ -25,6 +25,7 @@
>>   #include <drm/drm_vblank.h>
>>   
>>   #include "udl_drv.h"
>> +#include "udl_edid.h"
>>   #include "udl_proto.h"
>>   
>>   /*
>> @@ -415,97 +416,44 @@ static const struct drm_encoder_funcs udl_encoder_funcs = {
>>   
>>   static int udl_connector_helper_get_modes(struct drm_connector *connector)
>>   {
>> -	struct udl_connector *udl_connector = to_udl_connector(connector);
>> +	const struct drm_edid *drm_edid;
>> +	int count;
>>   
>> -	drm_connector_update_edid_property(connector, udl_connector->edid);
>> -	if (udl_connector->edid)
>> -		return drm_add_edid_modes(connector, udl_connector->edid);
>> +	drm_edid = udl_edid_read(connector);
>> +	drm_edid_connector_update(connector, drm_edid);
>> +	count = drm_edid_connector_add_modes(connector);
>> +	drm_edid_free(drm_edid);
>>   
>> -	return 0;
>> +	return count;
>>   }
>>   
>> -static const struct drm_connector_helper_funcs udl_connector_helper_funcs = {
>> -	.get_modes = udl_connector_helper_get_modes,
>> -};
>> -
>> -static int udl_get_edid_block(void *data, u8 *buf, unsigned int block, size_t len)
>> +static int udl_connector_helper_detect_ctx(struct drm_connector *connector,
>> +					   struct drm_modeset_acquire_ctx *ctx,
>> +					   bool force)
>>   {
>> -	struct udl_device *udl = data;
>> -	struct drm_device *dev = &udl->drm;
>> -	struct usb_device *udev = udl_to_usb_device(udl);
>> -	u8 *read_buff;
>> -	int idx, ret;
>> -	size_t i;
>> -
>> -	read_buff = kmalloc(2, GFP_KERNEL);
>> -	if (!read_buff)
>> -		return -ENOMEM;
>> +	struct udl_device *udl = to_udl(connector->dev);
>>   
>> -	if (!drm_dev_enter(dev, &idx)) {
>> -		ret = -ENODEV;
>> -		goto err_kfree;
>> -	}
>> -
>> -	for (i = 0; i < len; i++) {
>> -		int bval = (i + block * EDID_LENGTH) << 8;
>> -
>> -		ret = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0),
>> -				      0x02, (0x80 | (0x02 << 5)), bval,
>> -				      0xA1, read_buff, 2, USB_CTRL_GET_TIMEOUT);
>> -		if (ret < 0) {
>> -			drm_err(dev, "Read EDID byte %zu failed err %x\n", i, ret);
>> -			goto err_drm_dev_exit;
>> -		} else if (ret < 1) {
>> -			ret = -EIO;
>> -			drm_err(dev, "Read EDID byte %zu failed\n", i);
>> -			goto err_drm_dev_exit;
>> -		}
>> -
>> -		buf[i] = read_buff[1];
>> -	}
>> +	if (udl_probe_edid(udl))
>> +		return connector_status_connected;
>>   
>> -	drm_dev_exit(idx);
>> -	kfree(read_buff);
>> -
>> -	return 0;
>> -
>> -err_drm_dev_exit:
>> -	drm_dev_exit(idx);
>> -err_kfree:
>> -	kfree(read_buff);
>> -	return ret;
>> +	return connector_status_disconnected;
>>   }
>>   
>> -static enum drm_connector_status udl_connector_detect(struct drm_connector *connector, bool force)
>> -{
>> -	struct drm_device *dev = connector->dev;
>> -	struct udl_device *udl = to_udl(dev);
>> -	struct udl_connector *udl_connector = to_udl_connector(connector);
>> -	enum drm_connector_status status = connector_status_disconnected;
>> -
>> -	/* cleanup previous EDID */
>> -	kfree(udl_connector->edid);
>> -	udl_connector->edid = NULL;
>> -
>> -	udl_connector->edid = drm_do_get_edid(connector, udl_get_edid_block, udl);
>> -	if (udl_connector->edid)
>> -		status = connector_status_connected;
>> -
>> -	return status;
>> -}
>> +static const struct drm_connector_helper_funcs udl_connector_helper_funcs = {
>> +	.get_modes = udl_connector_helper_get_modes,
>> +	.detect_ctx = udl_connector_helper_detect_ctx,
>> +};
>>   
>>   static void udl_connector_destroy(struct drm_connector *connector)
>>   {
>>   	struct udl_connector *udl_connector = to_udl_connector(connector);
>>   
>>   	drm_connector_cleanup(connector);
>> -	kfree(udl_connector->edid);
>>   	kfree(udl_connector);
>>   }
>>   
>>   static const struct drm_connector_funcs udl_connector_funcs = {
>>   	.reset = drm_atomic_helper_connector_reset,
>> -	.detect = udl_connector_detect,
>>   	.fill_modes = drm_helper_probe_single_connector_modes,
>>   	.destroy = udl_connector_destroy,
>>   	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,

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

[PATCH 7/7] drm/udl: Remove struct udl_connector

[Thomas Zimmermann]

Udl's struct udl_connector is an empty wrapper around struct
drm_connector. Remove it. Allocate the connector as part of struct
udl_device and inline the init function into its only caller.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/udl/udl_drv.h     | 10 +------
 drivers/gpu/drm/udl/udl_modeset.c | 47 ++++++-------------------------
 2 files changed, 10 insertions(+), 47 deletions(-)

diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
index f112cfb270f31..1eb716d9dad57 100644
--- a/drivers/gpu/drm/udl/udl_drv.h
+++ b/drivers/gpu/drm/udl/udl_drv.h
@@ -49,15 +49,6 @@ struct urb_list {
 	size_t size;
 };
 
-struct udl_connector {
-	struct drm_connector connector;
-};
-
-static inline struct udl_connector *to_udl_connector(struct drm_connector *connector)
-{
-	return container_of(connector, struct udl_connector, connector);
-}
-
 struct udl_device {
 	struct drm_device drm;
 	struct device *dev;
@@ -66,6 +57,7 @@ struct udl_device {
 	struct drm_plane primary_plane;
 	struct drm_crtc crtc;
 	struct drm_encoder encoder;
+	struct drm_connector connector;
 
 	struct mutex gem_lock;
 
diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c
index 4236ce57f5945..ce82382b7a423 100644
--- a/drivers/gpu/drm/udl/udl_modeset.c
+++ b/drivers/gpu/drm/udl/udl_modeset.c
@@ -444,49 +444,14 @@ static const struct drm_connector_helper_funcs udl_connector_helper_funcs = {
 	.detect_ctx = udl_connector_helper_detect_ctx,
 };
 
-static void udl_connector_destroy(struct drm_connector *connector)
-{
-	struct udl_connector *udl_connector = to_udl_connector(connector);
-
-	drm_connector_cleanup(connector);
-	kfree(udl_connector);
-}
-
 static const struct drm_connector_funcs udl_connector_funcs = {
 	.reset = drm_atomic_helper_connector_reset,
 	.fill_modes = drm_helper_probe_single_connector_modes,
-	.destroy = udl_connector_destroy,
+	.destroy = drm_connector_cleanup,
 	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
 	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
 };
 
-struct drm_connector *udl_connector_init(struct drm_device *dev)
-{
-	struct udl_connector *udl_connector;
-	struct drm_connector *connector;
-	int ret;
-
-	udl_connector = kzalloc(sizeof(*udl_connector), GFP_KERNEL);
-	if (!udl_connector)
-		return ERR_PTR(-ENOMEM);
-
-	connector = &udl_connector->connector;
-	ret = drm_connector_init(dev, connector, &udl_connector_funcs, DRM_MODE_CONNECTOR_VGA);
-	if (ret)
-		goto err_kfree;
-
-	drm_connector_helper_add(connector, &udl_connector_helper_funcs);
-
-	connector->polled = DRM_CONNECTOR_POLL_CONNECT |
-			    DRM_CONNECTOR_POLL_DISCONNECT;
-
-	return connector;
-
-err_kfree:
-	kfree(udl_connector);
-	return ERR_PTR(ret);
-}
-
 /*
  * Modesetting
  */
@@ -556,9 +521,15 @@ int udl_modeset_init(struct drm_device *dev)
 		return ret;
 	encoder->possible_crtcs = drm_crtc_mask(crtc);
 
-	connector = udl_connector_init(dev);
-	if (IS_ERR(connector))
+	connector = &udl->connector;
+	ret = drm_connector_init(dev, connector, &udl_connector_funcs, DRM_MODE_CONNECTOR_VGA);
+	if (ret)
 		return PTR_ERR(connector);
+	drm_connector_helper_add(connector, &udl_connector_helper_funcs);
+
+	connector->polled = DRM_CONNECTOR_POLL_CONNECT |
+			    DRM_CONNECTOR_POLL_DISCONNECT;
+
 	ret = drm_connector_attach_encoder(connector, encoder);
 	if (ret)
 		return ret;
-- 
2.44.0

Re: [PATCH 7/7] drm/udl: Remove struct udl_connector

[Dan Carpenter]

Hi Thomas,

kernel test robot noticed the following build warnings:

https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Thomas-Zimmermann/drm-edid-Implement-drm_probe_ddc-with-drm_edid_probe_custom/20240404-231057
base:   git://anongit.freedesktop.org/drm/drm-misc drm-misc-next
patch link:    https://lore.kernel.org/r/20240404150857.5520-8-tzimmermann%40suse.de
patch subject: [PATCH 7/7] drm/udl: Remove struct udl_connector
config: parisc-randconfig-r071-20240405 (https://download.01.org/0day-ci/archive/20240405/202404051359.Y6AgUwFi-lkp@intel.com/config)
compiler: hppa-linux-gcc (GCC) 13.2.0

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
| Closes: https://lore.kernel.org/r/202404051359.Y6AgUwFi-lkp@intel.com/

smatch warnings:
drivers/gpu/drm/udl/udl_modeset.c:527 udl_modeset_init() warn: passing a valid pointer to 'PTR_ERR'

vim +/PTR_ERR +527 drivers/gpu/drm/udl/udl_modeset.c

72d73dd3a95c7e8 Thomas Zimmermann 2022-10-06  518  	encoder = &udl->encoder;
72d73dd3a95c7e8 Thomas Zimmermann 2022-10-06  519  	ret = drm_encoder_init(dev, encoder, &udl_encoder_funcs, DRM_MODE_ENCODER_DAC, NULL);
72d73dd3a95c7e8 Thomas Zimmermann 2022-10-06  520  	if (ret)
72d73dd3a95c7e8 Thomas Zimmermann 2022-10-06  521  		return ret;
72d73dd3a95c7e8 Thomas Zimmermann 2022-10-06  522  	encoder->possible_crtcs = drm_crtc_mask(crtc);
5320918b9a87865 Dave Airlie       2010-12-15  523  
a80d9e00c8195dc Thomas Zimmermann 2024-04-04  524  	connector = &udl->connector;
a80d9e00c8195dc Thomas Zimmermann 2024-04-04  525  	ret = drm_connector_init(dev, connector, &udl_connector_funcs, DRM_MODE_CONNECTOR_VGA);
a80d9e00c8195dc Thomas Zimmermann 2024-04-04  526  	if (ret)
                                                        ^^^^^^^^

fe5b7c86d6068ac Daniel Vetter     2020-03-23 @527  		return PTR_ERR(connector);
                                                                       ^^^^^^^^^^^^^^^^^^^

This needs to be updated to "return ret;"

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki