diff mbox

[v4,6/9] drm: Decouple EDID parsing from I2C adapter

Message ID 1416959247-23132-1-git-send-email-laurent.pinchart+renesas@ideasonboard.com (mailing list archive)
State New, archived
Headers show

Commit Message

Laurent Pinchart Nov. 25, 2014, 11:47 p.m. UTC
From: Lars-Peter Clausen <lars@metafoo.de>

The drm_get_edid() function performs direct I2C accesses to read EDID
blocks, assuming that the monitor DDC interface is directly connected to
the I2C bus. It can't thus be used with HDMI encoders that control the
DDC bus and expose EDID blocks through a different interface.

Refactor drm_do_get_edid() to take a block read callback function
instead of an I2C adapter, and export it for direct use by drivers.

As in the general case the DDC bus is accessible by the kernel at the
I2C level, drivers must make all reasonable efforts to expose it as an
I2C adapter and use drm_get_edid() instead of abusing this function.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/gpu/drm/drm_edid.c | 43 ++++++++++++++++++++++++++++++-------------
 include/drm/drm_edid.h     |  5 +++++
 2 files changed, 35 insertions(+), 13 deletions(-)

Daniel, could you please review and hopefully ack this ? If this new version is
acceptable I'd like to send an updated pull request for R-Car DU HDMI support
for v3.19, so time is running short.

Changes since v3:

- Add kerneldoc for the new exported drm_do_get_edid function

Comments

Rob Clark Nov. 26, 2014, 12:38 a.m. UTC | #1
On Tue, Nov 25, 2014 at 6:47 PM, Laurent Pinchart
<laurent.pinchart+renesas@ideasonboard.com> wrote:
> From: Lars-Peter Clausen <lars@metafoo.de>
>
> The drm_get_edid() function performs direct I2C accesses to read EDID
> blocks, assuming that the monitor DDC interface is directly connected to
> the I2C bus. It can't thus be used with HDMI encoders that control the
> DDC bus and expose EDID blocks through a different interface.
>
> Refactor drm_do_get_edid() to take a block read callback function
> instead of an I2C adapter, and export it for direct use by drivers.
>
> As in the general case the DDC bus is accessible by the kernel at the
> I2C level, drivers must make all reasonable efforts to expose it as an
> I2C adapter and use drm_get_edid() instead of abusing this function.
>
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>


I suppose if tda998x were converted over to use it, it would be a nice
negative diffstat ;-)

Reviewed-by: Rob Clark <robdclark@gmail.com>


> ---
>  drivers/gpu/drm/drm_edid.c | 43 ++++++++++++++++++++++++++++++-------------
>  include/drm/drm_edid.h     |  5 +++++
>  2 files changed, 35 insertions(+), 13 deletions(-)
>
> Daniel, could you please review and hopefully ack this ? If this new version is
> acceptable I'd like to send an updated pull request for R-Car DU HDMI support
> for v3.19, so time is running short.
>
> Changes since v3:
>
> - Add kerneldoc for the new exported drm_do_get_edid function
>
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 3bf999134bcc..1a77a49d2695 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -1125,9 +1125,9 @@ EXPORT_SYMBOL(drm_edid_is_valid);
>   * Return: 0 on success or -1 on failure.
>   */
>  static int
> -drm_do_probe_ddc_edid(struct i2c_adapter *adapter, unsigned char *buf,
> -                     int block, int len)
> +drm_do_probe_ddc_edid(void *data, u8 *buf, unsigned int block, size_t len)
>  {
> +       struct i2c_adapter *adapter = data;
>         unsigned char start = block * EDID_LENGTH;
>         unsigned char segment = block >> 1;
>         unsigned char xfers = segment ? 3 : 2;
> @@ -1184,8 +1184,26 @@ static bool drm_edid_is_zero(u8 *in_edid, int length)
>         return true;
>  }
>
> -static u8 *
> -drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
> +/**
> + * drm_do_get_edid - get EDID data using a custom EDID block read function
> + * @connector: connector we're probing
> + * @get_edid_block: EDID block read function
> + * @data: private data passed to the block read function
> + *
> + * When the I2C adapter connected to the DDC bus is hidden behind a device that
> + * exposes a different interface to read EDID blocks this function can be used
> + * to get EDID data using a custom block read function.
> + *
> + * As in the general case the DDC bus is accessible by the kernel at the I2C
> + * level, drivers must make all reasonable efforts to expose it as an I2C
> + * adapter and use drm_get_edid() instead of abusing this function.
> + *
> + * Return: Pointer to valid EDID or NULL if we couldn't find any.
> + */
> +struct edid *drm_do_get_edid(struct drm_connector *connector,
> +       int (*get_edid_block)(void *data, u8 *buf, unsigned int block,
> +                             size_t len),
> +       void *data)
>  {
>         int i, j = 0, valid_extensions = 0;
>         u8 *block, *new;
> @@ -1196,7 +1214,7 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
>
>         /* base block fetch */
>         for (i = 0; i < 4; i++) {
> -               if (drm_do_probe_ddc_edid(adapter, block, 0, EDID_LENGTH))
> +               if (get_edid_block(data, block, 0, EDID_LENGTH))
>                         goto out;
>                 if (drm_edid_block_valid(block, 0, print_bad_edid))
>                         break;
> @@ -1210,7 +1228,7 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
>
>         /* if there's no extensions, we're done */
>         if (block[0x7e] == 0)
> -               return block;
> +               return (struct edid *)block;
>
>         new = krealloc(block, (block[0x7e] + 1) * EDID_LENGTH, GFP_KERNEL);
>         if (!new)
> @@ -1219,7 +1237,7 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
>
>         for (j = 1; j <= block[0x7e]; j++) {
>                 for (i = 0; i < 4; i++) {
> -                       if (drm_do_probe_ddc_edid(adapter,
> +                       if (get_edid_block(data,
>                                   block + (valid_extensions + 1) * EDID_LENGTH,
>                                   j, EDID_LENGTH))
>                                 goto out;
> @@ -1247,7 +1265,7 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
>                 block = new;
>         }
>
> -       return block;
> +       return (struct edid *)block;
>
>  carp:
>         if (print_bad_edid) {
> @@ -1260,6 +1278,7 @@ out:
>         kfree(block);
>         return NULL;
>  }
> +EXPORT_SYMBOL_GPL(drm_do_get_edid);
>
>  /**
>   * drm_probe_ddc() - probe DDC presence
> @@ -1289,12 +1308,10 @@ EXPORT_SYMBOL(drm_probe_ddc);
>  struct edid *drm_get_edid(struct drm_connector *connector,
>                           struct i2c_adapter *adapter)
>  {
> -       struct edid *edid = NULL;
> -
> -       if (drm_probe_ddc(adapter))
> -               edid = (struct edid *)drm_do_get_edid(connector, adapter);
> +       if (!drm_probe_ddc(adapter))
> +               return NULL;
>
> -       return edid;
> +       return drm_do_get_edid(connector, drm_do_probe_ddc_edid, adapter);
>  }
>  EXPORT_SYMBOL(drm_get_edid);
>
> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> index c2f1bfa22010..d59240ffb1f7 100644
> --- a/include/drm/drm_edid.h
> +++ b/include/drm/drm_edid.h
> @@ -381,4 +381,9 @@ static inline int drm_eld_size(const uint8_t *eld)
>         return DRM_ELD_HEADER_BLOCK_SIZE + eld[DRM_ELD_BASELINE_ELD_LEN] * 4;
>  }
>
> +struct edid *drm_do_get_edid(struct drm_connector *connector,
> +       int (*get_edid_block)(void *data, u8 *buf, unsigned int block,
> +                             size_t len),
> +       void *data);
> +
>  #endif /* __DRM_EDID_H__ */
> --
> 2.0.4
>
Daniel Vetter Nov. 26, 2014, 7:28 a.m. UTC | #2
On Wed, Nov 26, 2014 at 01:47:27AM +0200, Laurent Pinchart wrote:
> From: Lars-Peter Clausen <lars@metafoo.de>
> 
> The drm_get_edid() function performs direct I2C accesses to read EDID
> blocks, assuming that the monitor DDC interface is directly connected to
> the I2C bus. It can't thus be used with HDMI encoders that control the
> DDC bus and expose EDID blocks through a different interface.
> 
> Refactor drm_do_get_edid() to take a block read callback function
> instead of an I2C adapter, and export it for direct use by drivers.
> 
> As in the general case the DDC bus is accessible by the kernel at the
> I2C level, drivers must make all reasonable efforts to expose it as an
> I2C adapter and use drm_get_edid() instead of abusing this function.
> 
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/gpu/drm/drm_edid.c | 43 ++++++++++++++++++++++++++++++-------------
>  include/drm/drm_edid.h     |  5 +++++
>  2 files changed, 35 insertions(+), 13 deletions(-)
> 
> Daniel, could you please review and hopefully ack this ? If this new version is
> acceptable I'd like to send an updated pull request for R-Car DU HDMI support
> for v3.19, so time is running short.
> 
> Changes since v3:
> 
> - Add kerneldoc for the new exported drm_do_get_edid function
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 3bf999134bcc..1a77a49d2695 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -1125,9 +1125,9 @@ EXPORT_SYMBOL(drm_edid_is_valid);
>   * Return: 0 on success or -1 on failure.
>   */
>  static int
> -drm_do_probe_ddc_edid(struct i2c_adapter *adapter, unsigned char *buf,
> -		      int block, int len)
> +drm_do_probe_ddc_edid(void *data, u8 *buf, unsigned int block, size_t len)
>  {
> +	struct i2c_adapter *adapter = data;
>  	unsigned char start = block * EDID_LENGTH;
>  	unsigned char segment = block >> 1;
>  	unsigned char xfers = segment ? 3 : 2;
> @@ -1184,8 +1184,26 @@ static bool drm_edid_is_zero(u8 *in_edid, int length)
>  	return true;
>  }
>  
> -static u8 *
> -drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
> +/**
> + * drm_do_get_edid - get EDID data using a custom EDID block read function
> + * @connector: connector we're probing
> + * @get_edid_block: EDID block read function
> + * @data: private data passed to the block read function
> + *
> + * When the I2C adapter connected to the DDC bus is hidden behind a device that
> + * exposes a different interface to read EDID blocks this function can be used
> + * to get EDID data using a custom block read function.
> + *
> + * As in the general case the DDC bus is accessible by the kernel at the I2C
> + * level, drivers must make all reasonable efforts to expose it as an I2C
> + * adapter and use drm_get_edid() instead of abusing this function.
> + *
> + * Return: Pointer to valid EDID or NULL if we couldn't find any.
> + */
> +struct edid *drm_do_get_edid(struct drm_connector *connector,
> +	int (*get_edid_block)(void *data, u8 *buf, unsigned int block,
> +			      size_t len),
> +	void *data)
>  {
>  	int i, j = 0, valid_extensions = 0;
>  	u8 *block, *new;
> @@ -1196,7 +1214,7 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
>  
>  	/* base block fetch */
>  	for (i = 0; i < 4; i++) {
> -		if (drm_do_probe_ddc_edid(adapter, block, 0, EDID_LENGTH))
> +		if (get_edid_block(data, block, 0, EDID_LENGTH))
>  			goto out;
>  		if (drm_edid_block_valid(block, 0, print_bad_edid))
>  			break;
> @@ -1210,7 +1228,7 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
>  
>  	/* if there's no extensions, we're done */
>  	if (block[0x7e] == 0)
> -		return block;
> +		return (struct edid *)block;
>  
>  	new = krealloc(block, (block[0x7e] + 1) * EDID_LENGTH, GFP_KERNEL);
>  	if (!new)
> @@ -1219,7 +1237,7 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
>  
>  	for (j = 1; j <= block[0x7e]; j++) {
>  		for (i = 0; i < 4; i++) {
> -			if (drm_do_probe_ddc_edid(adapter,
> +			if (get_edid_block(data,
>  				  block + (valid_extensions + 1) * EDID_LENGTH,
>  				  j, EDID_LENGTH))
>  				goto out;
> @@ -1247,7 +1265,7 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
>  		block = new;
>  	}
>  
> -	return block;
> +	return (struct edid *)block;
>  
>  carp:
>  	if (print_bad_edid) {
> @@ -1260,6 +1278,7 @@ out:
>  	kfree(block);
>  	return NULL;
>  }
> +EXPORT_SYMBOL_GPL(drm_do_get_edid);
>  
>  /**
>   * drm_probe_ddc() - probe DDC presence
> @@ -1289,12 +1308,10 @@ EXPORT_SYMBOL(drm_probe_ddc);
>  struct edid *drm_get_edid(struct drm_connector *connector,
>  			  struct i2c_adapter *adapter)
>  {
> -	struct edid *edid = NULL;
> -
> -	if (drm_probe_ddc(adapter))
> -		edid = (struct edid *)drm_do_get_edid(connector, adapter);
> +	if (!drm_probe_ddc(adapter))
> +		return NULL;
>  
> -	return edid;
> +	return drm_do_get_edid(connector, drm_do_probe_ddc_edid, adapter);
>  }
>  EXPORT_SYMBOL(drm_get_edid);
>  
> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> index c2f1bfa22010..d59240ffb1f7 100644
> --- a/include/drm/drm_edid.h
> +++ b/include/drm/drm_edid.h
> @@ -381,4 +381,9 @@ static inline int drm_eld_size(const uint8_t *eld)
>  	return DRM_ELD_HEADER_BLOCK_SIZE + eld[DRM_ELD_BASELINE_ELD_LEN] * 4;
>  }
>  
> +struct edid *drm_do_get_edid(struct drm_connector *connector,
> +	int (*get_edid_block)(void *data, u8 *buf, unsigned int block,
> +			      size_t len),
> +	void *data);
> +
>  #endif /* __DRM_EDID_H__ */
> -- 
> 2.0.4
>
Laurent Pinchart Nov. 26, 2014, 7:06 p.m. UTC | #3
Hi Rob,

On Tuesday 25 November 2014 19:38:47 Rob Clark wrote:
> On Tue, Nov 25, 2014 at 6:47 PM, Laurent Pinchart wrote:
> > From: Lars-Peter Clausen <lars@metafoo.de>
> > 
> > The drm_get_edid() function performs direct I2C accesses to read EDID
> > blocks, assuming that the monitor DDC interface is directly connected to
> > the I2C bus. It can't thus be used with HDMI encoders that control the
> > DDC bus and expose EDID blocks through a different interface.
> > 
> > Refactor drm_do_get_edid() to take a block read callback function
> > instead of an I2C adapter, and export it for direct use by drivers.
> > 
> > As in the general case the DDC bus is accessible by the kernel at the
> > I2C level, drivers must make all reasonable efforts to expose it as an
> > I2C adapter and use drm_get_edid() instead of abusing this function.
> > 
> > Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> > Signed-off-by: Laurent Pinchart
> > <laurent.pinchart+renesas@ideasonboard.com>
> 
> I suppose if tda998x were converted over to use it, it would be a nice
> negative diffstat ;-)

 tda998x_drv.c |   86 ++++++++++++-------------------------------------------
 1 file changed, 18 insertions(+), 68 deletions(-)

:-)

> Reviewed-by: Rob Clark <robdclark@gmail.com>
> 
> > ---
> > 
> >  drivers/gpu/drm/drm_edid.c | 43 ++++++++++++++++++++++++++++-------------
> >  include/drm/drm_edid.h     |  5 +++++
> >  2 files changed, 35 insertions(+), 13 deletions(-)
> > 
> > Daniel, could you please review and hopefully ack this ? If this new
> > version is acceptable I'd like to send an updated pull request for R-Car
> > DU HDMI support for v3.19, so time is running short.
> > 
> > Changes since v3:
> > 
> > - Add kerneldoc for the new exported drm_do_get_edid function
diff mbox

Patch

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 3bf999134bcc..1a77a49d2695 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -1125,9 +1125,9 @@  EXPORT_SYMBOL(drm_edid_is_valid);
  * Return: 0 on success or -1 on failure.
  */
 static int
-drm_do_probe_ddc_edid(struct i2c_adapter *adapter, unsigned char *buf,
-		      int block, int len)
+drm_do_probe_ddc_edid(void *data, u8 *buf, unsigned int block, size_t len)
 {
+	struct i2c_adapter *adapter = data;
 	unsigned char start = block * EDID_LENGTH;
 	unsigned char segment = block >> 1;
 	unsigned char xfers = segment ? 3 : 2;
@@ -1184,8 +1184,26 @@  static bool drm_edid_is_zero(u8 *in_edid, int length)
 	return true;
 }
 
-static u8 *
-drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
+/**
+ * drm_do_get_edid - get EDID data using a custom EDID block read function
+ * @connector: connector we're probing
+ * @get_edid_block: EDID block read function
+ * @data: private data passed to the block read function
+ *
+ * When the I2C adapter connected to the DDC bus is hidden behind a device that
+ * exposes a different interface to read EDID blocks this function can be used
+ * to get EDID data using a custom block read function.
+ *
+ * As in the general case the DDC bus is accessible by the kernel at the I2C
+ * level, drivers must make all reasonable efforts to expose it as an I2C
+ * adapter and use drm_get_edid() instead of abusing this function.
+ *
+ * Return: Pointer to valid EDID or NULL if we couldn't find any.
+ */
+struct edid *drm_do_get_edid(struct drm_connector *connector,
+	int (*get_edid_block)(void *data, u8 *buf, unsigned int block,
+			      size_t len),
+	void *data)
 {
 	int i, j = 0, valid_extensions = 0;
 	u8 *block, *new;
@@ -1196,7 +1214,7 @@  drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
 
 	/* base block fetch */
 	for (i = 0; i < 4; i++) {
-		if (drm_do_probe_ddc_edid(adapter, block, 0, EDID_LENGTH))
+		if (get_edid_block(data, block, 0, EDID_LENGTH))
 			goto out;
 		if (drm_edid_block_valid(block, 0, print_bad_edid))
 			break;
@@ -1210,7 +1228,7 @@  drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
 
 	/* if there's no extensions, we're done */
 	if (block[0x7e] == 0)
-		return block;
+		return (struct edid *)block;
 
 	new = krealloc(block, (block[0x7e] + 1) * EDID_LENGTH, GFP_KERNEL);
 	if (!new)
@@ -1219,7 +1237,7 @@  drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
 
 	for (j = 1; j <= block[0x7e]; j++) {
 		for (i = 0; i < 4; i++) {
-			if (drm_do_probe_ddc_edid(adapter,
+			if (get_edid_block(data,
 				  block + (valid_extensions + 1) * EDID_LENGTH,
 				  j, EDID_LENGTH))
 				goto out;
@@ -1247,7 +1265,7 @@  drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
 		block = new;
 	}
 
-	return block;
+	return (struct edid *)block;
 
 carp:
 	if (print_bad_edid) {
@@ -1260,6 +1278,7 @@  out:
 	kfree(block);
 	return NULL;
 }
+EXPORT_SYMBOL_GPL(drm_do_get_edid);
 
 /**
  * drm_probe_ddc() - probe DDC presence
@@ -1289,12 +1308,10 @@  EXPORT_SYMBOL(drm_probe_ddc);
 struct edid *drm_get_edid(struct drm_connector *connector,
 			  struct i2c_adapter *adapter)
 {
-	struct edid *edid = NULL;
-
-	if (drm_probe_ddc(adapter))
-		edid = (struct edid *)drm_do_get_edid(connector, adapter);
+	if (!drm_probe_ddc(adapter))
+		return NULL;
 
-	return edid;
+	return drm_do_get_edid(connector, drm_do_probe_ddc_edid, adapter);
 }
 EXPORT_SYMBOL(drm_get_edid);
 
diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
index c2f1bfa22010..d59240ffb1f7 100644
--- a/include/drm/drm_edid.h
+++ b/include/drm/drm_edid.h
@@ -381,4 +381,9 @@  static inline int drm_eld_size(const uint8_t *eld)
 	return DRM_ELD_HEADER_BLOCK_SIZE + eld[DRM_ELD_BASELINE_ELD_LEN] * 4;
 }
 
+struct edid *drm_do_get_edid(struct drm_connector *connector,
+	int (*get_edid_block)(void *data, u8 *buf, unsigned int block,
+			      size_t len),
+	void *data);
+
 #endif /* __DRM_EDID_H__ */