[PATCH v2] drm/display: Don't assume dual mode adaptors support i2c sub-addressing

[PATCH v2] drm/display: Don't assume dual mode adaptors support i2c sub-addressing

[Simon Rettberg]

Current dual mode adaptor ("DP++") detection code assumes that all
adaptors support i2c sub-addressing for read operations from the
DP-HDMI adaptor ID buffer.  It has been observed that multiple
adaptors do not in fact support this, and always return data starting
at register 0.  On affected adaptors, the code fails to read the proper
registers that would identify the device as a type 2 adaptor, and
handles those as type 1, limiting the TMDS clock to 165MHz, even if
the according register would announce a higher TMDS clock.
Fix this by always reading the ID buffer starting from offset 0, and
discarding any bytes before the actual offset of interest.

We tried finding authoritative documentation on whether or not this is
allowed behaviour, but since all the official VESA docs are paywalled,
the best we could come up with was the spec sheet for Texas Instruments'
SNx5DP149 chip family.[1]  It explicitly mentions that sub-addressing is
supported for register writes, but *not* for reads (See NOTE in
section 8.5.3).  Unless TI openly decided to violate the VESA spec, one
could take that as a hint that sub-addressing is in fact not mandated
by VESA.
The other two adaptors affected used the PS8409(A) and the LT8611,
according to the data returned from their ID buffers.

[1] https://www.ti.com/lit/ds/symlink/sn75dp149.pdf

Signed-off-by: Simon Rettberg <simon.rettberg@rz.uni-freiburg.de>
Reviewed-by: Rafael Gieschke <rafael.gieschke@rz.uni-freiburg.de>
---

v2 changes form last submission's feedback (thanks for taking the time):
- Added a shortened version of our "background story" to the commit message
- Only use tmpbuf if the read offset is != 0

 .../gpu/drm/display/drm_dp_dual_mode_helper.c | 51 +++++++++++--------
 1 file changed, 29 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/display/drm_dp_dual_mode_helper.c b/drivers/gpu/drm/display/drm_dp_dual_mode_helper.c
index 3ea53bb67d3b..bd61e20770a5 100644
--- a/drivers/gpu/drm/display/drm_dp_dual_mode_helper.c
+++ b/drivers/gpu/drm/display/drm_dp_dual_mode_helper.c
@@ -63,23 +63,45 @@
 ssize_t drm_dp_dual_mode_read(struct i2c_adapter *adapter,
 			      u8 offset, void *buffer, size_t size)
 {
+	u8 zero = 0;
+	char *tmpbuf = NULL;
+	/*
+	 * As sub-addressing is not supported by all adaptors,
+	 * always explicitly read from the start and discard
+	 * any bytes that come before the requested offset.
+	 * This way, no matter whether the adaptor supports it
+	 * or not, we'll end up reading the proper data.
+	 */
 	struct i2c_msg msgs[] = {
 		{
 			.addr = DP_DUAL_MODE_SLAVE_ADDRESS,
 			.flags = 0,
 			.len = 1,
-			.buf = &offset,
+			.buf = &zero,
 		},
 		{
 			.addr = DP_DUAL_MODE_SLAVE_ADDRESS,
 			.flags = I2C_M_RD,
-			.len = size,
+			.len = size + offset,
 			.buf = buffer,
 		},
 	};
 	int ret;
 
+	if (offset) {
+		tmpbuf = kmalloc(size + offset, GFP_KERNEL);
+		if (!tmpbuf)
+			return -ENOMEM;
+
+		msgs[1].buf = tmpbuf;
+	}
+
 	ret = i2c_transfer(adapter, msgs, ARRAY_SIZE(msgs));
+	if (tmpbuf)
+		memcpy(buffer, tmpbuf + offset, size);
+
+	kfree(tmpbuf);
+
 	if (ret < 0)
 		return ret;
 	if (ret != ARRAY_SIZE(msgs))
@@ -208,18 +230,6 @@ enum drm_dp_dual_mode_type drm_dp_dual_mode_detect(const struct drm_device *dev,
 	if (ret)
 		return DRM_DP_DUAL_MODE_UNKNOWN;
 
-	/*
-	 * Sigh. Some (maybe all?) type 1 adaptors are broken and ack
-	 * the offset but ignore it, and instead they just always return
-	 * data from the start of the HDMI ID buffer. So for a broken
-	 * type 1 HDMI adaptor a single byte read will always give us
-	 * 0x44, and for a type 1 DVI adaptor it should give 0x00
-	 * (assuming it implements any registers). Fortunately neither
-	 * of those values will match the type 2 signature of the
-	 * DP_DUAL_MODE_ADAPTOR_ID register so we can proceed with
-	 * the type 2 adaptor detection safely even in the presence
-	 * of broken type 1 adaptors.
-	 */
 	ret = drm_dp_dual_mode_read(adapter, DP_DUAL_MODE_ADAPTOR_ID,
 				    &adaptor_id, sizeof(adaptor_id));
 	drm_dbg_kms(dev, "DP dual mode adaptor ID: %02x (err %zd)\n", adaptor_id, ret);
@@ -233,11 +243,10 @@ enum drm_dp_dual_mode_type drm_dp_dual_mode_detect(const struct drm_device *dev,
 				return DRM_DP_DUAL_MODE_TYPE2_DVI;
 		}
 		/*
-		 * If neither a proper type 1 ID nor a broken type 1 adaptor
-		 * as described above, assume type 1, but let the user know
-		 * that we may have misdetected the type.
+		 * If not a proper type 1 ID, still assume type 1, but let
+		 * the user know that we may have misdetected the type.
 		 */
-		if (!is_type1_adaptor(adaptor_id) && adaptor_id != hdmi_id[0])
+		if (!is_type1_adaptor(adaptor_id))
 			drm_err(dev, "Unexpected DP dual mode adaptor ID %02x\n", adaptor_id);
 
 	}
@@ -343,10 +352,8 @@ EXPORT_SYMBOL(drm_dp_dual_mode_get_tmds_output);
  * @enable: enable (as opposed to disable) the TMDS output buffers
  *
  * Set the state of the TMDS output buffers in the adaptor. For
- * type2 this is set via the DP_DUAL_MODE_TMDS_OEN register. As
- * some type 1 adaptors have problems with registers (see comments
- * in drm_dp_dual_mode_detect()) we avoid touching the register,
- * making this function a no-op on type 1 adaptors.
+ * type2 this is set via the DP_DUAL_MODE_TMDS_OEN register.
+ * Type1 adaptors do not support any register writes.
  *
  * Returns:
  * 0 on success, negative error code on failure
-- 
2.35.1

Re: [PATCH v2] drm/display: Don't assume dual mode adaptors support i2c sub-addressing

[Ville Syrjälä]

On Thu, Oct 06, 2022 at 11:33:14AM +0200, Simon Rettberg wrote:
> Current dual mode adaptor ("DP++") detection code assumes that all
> adaptors support i2c sub-addressing for read operations from the
> DP-HDMI adaptor ID buffer.  It has been observed that multiple
> adaptors do not in fact support this, and always return data starting
> at register 0.  On affected adaptors, the code fails to read the proper
> registers that would identify the device as a type 2 adaptor, and
> handles those as type 1, limiting the TMDS clock to 165MHz, even if
> the according register would announce a higher TMDS clock.
> Fix this by always reading the ID buffer starting from offset 0, and
> discarding any bytes before the actual offset of interest.
> 
> We tried finding authoritative documentation on whether or not this is
> allowed behaviour, but since all the official VESA docs are paywalled,
> the best we could come up with was the spec sheet for Texas Instruments'
> SNx5DP149 chip family.[1]  It explicitly mentions that sub-addressing is
> supported for register writes, but *not* for reads (See NOTE in
> section 8.5.3).  Unless TI openly decided to violate the VESA spec, one
> could take that as a hint that sub-addressing is in fact not mandated
> by VESA.
> The other two adaptors affected used the PS8409(A) and the LT8611,
> according to the data returned from their ID buffers.
> 
> [1] https://www.ti.com/lit/ds/symlink/sn75dp149.pdf
> 
> Signed-off-by: Simon Rettberg <simon.rettberg@rz.uni-freiburg.de>
> Reviewed-by: Rafael Gieschke <rafael.gieschke@rz.uni-freiburg.de>
> ---
> 
> v2 changes form last submission's feedback (thanks for taking the time):
> - Added a shortened version of our "background story" to the commit message
> - Only use tmpbuf if the read offset is != 0

Bounced to intel-gfx to get the i915 CI to check it...

> 
>  .../gpu/drm/display/drm_dp_dual_mode_helper.c | 51 +++++++++++--------
>  1 file changed, 29 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/gpu/drm/display/drm_dp_dual_mode_helper.c b/drivers/gpu/drm/display/drm_dp_dual_mode_helper.c
> index 3ea53bb67d3b..bd61e20770a5 100644
> --- a/drivers/gpu/drm/display/drm_dp_dual_mode_helper.c
> +++ b/drivers/gpu/drm/display/drm_dp_dual_mode_helper.c
> @@ -63,23 +63,45 @@
>  ssize_t drm_dp_dual_mode_read(struct i2c_adapter *adapter,
>  			      u8 offset, void *buffer, size_t size)
>  {
> +	u8 zero = 0;
> +	char *tmpbuf = NULL;
> +	/*
> +	 * As sub-addressing is not supported by all adaptors,
> +	 * always explicitly read from the start and discard
> +	 * any bytes that come before the requested offset.
> +	 * This way, no matter whether the adaptor supports it
> +	 * or not, we'll end up reading the proper data.
> +	 */
>  	struct i2c_msg msgs[] = {
>  		{
>  			.addr = DP_DUAL_MODE_SLAVE_ADDRESS,
>  			.flags = 0,
>  			.len = 1,
> -			.buf = &offset,
> +			.buf = &zero,
>  		},
>  		{
>  			.addr = DP_DUAL_MODE_SLAVE_ADDRESS,
>  			.flags = I2C_M_RD,
> -			.len = size,
> +			.len = size + offset,
>  			.buf = buffer,
>  		},
>  	};
>  	int ret;
>  
> +	if (offset) {
> +		tmpbuf = kmalloc(size + offset, GFP_KERNEL);
> +		if (!tmpbuf)
> +			return -ENOMEM;
> +
> +		msgs[1].buf = tmpbuf;
> +	}
> +
>  	ret = i2c_transfer(adapter, msgs, ARRAY_SIZE(msgs));
> +	if (tmpbuf)
> +		memcpy(buffer, tmpbuf + offset, size);
> +
> +	kfree(tmpbuf);
> +
>  	if (ret < 0)
>  		return ret;
>  	if (ret != ARRAY_SIZE(msgs))
> @@ -208,18 +230,6 @@ enum drm_dp_dual_mode_type drm_dp_dual_mode_detect(const struct drm_device *dev,
>  	if (ret)
>  		return DRM_DP_DUAL_MODE_UNKNOWN;
>  
> -	/*
> -	 * Sigh. Some (maybe all?) type 1 adaptors are broken and ack
> -	 * the offset but ignore it, and instead they just always return
> -	 * data from the start of the HDMI ID buffer. So for a broken
> -	 * type 1 HDMI adaptor a single byte read will always give us
> -	 * 0x44, and for a type 1 DVI adaptor it should give 0x00
> -	 * (assuming it implements any registers). Fortunately neither
> -	 * of those values will match the type 2 signature of the
> -	 * DP_DUAL_MODE_ADAPTOR_ID register so we can proceed with
> -	 * the type 2 adaptor detection safely even in the presence
> -	 * of broken type 1 adaptors.
> -	 */
>  	ret = drm_dp_dual_mode_read(adapter, DP_DUAL_MODE_ADAPTOR_ID,
>  				    &adaptor_id, sizeof(adaptor_id));
>  	drm_dbg_kms(dev, "DP dual mode adaptor ID: %02x (err %zd)\n", adaptor_id, ret);
> @@ -233,11 +243,10 @@ enum drm_dp_dual_mode_type drm_dp_dual_mode_detect(const struct drm_device *dev,
>  				return DRM_DP_DUAL_MODE_TYPE2_DVI;
>  		}
>  		/*
> -		 * If neither a proper type 1 ID nor a broken type 1 adaptor
> -		 * as described above, assume type 1, but let the user know
> -		 * that we may have misdetected the type.
> +		 * If not a proper type 1 ID, still assume type 1, but let
> +		 * the user know that we may have misdetected the type.
>  		 */
> -		if (!is_type1_adaptor(adaptor_id) && adaptor_id != hdmi_id[0])
> +		if (!is_type1_adaptor(adaptor_id))
>  			drm_err(dev, "Unexpected DP dual mode adaptor ID %02x\n", adaptor_id);
>  
>  	}
> @@ -343,10 +352,8 @@ EXPORT_SYMBOL(drm_dp_dual_mode_get_tmds_output);
>   * @enable: enable (as opposed to disable) the TMDS output buffers
>   *
>   * Set the state of the TMDS output buffers in the adaptor. For
> - * type2 this is set via the DP_DUAL_MODE_TMDS_OEN register. As
> - * some type 1 adaptors have problems with registers (see comments
> - * in drm_dp_dual_mode_detect()) we avoid touching the register,
> - * making this function a no-op on type 1 adaptors.
> + * type2 this is set via the DP_DUAL_MODE_TMDS_OEN register.
> + * Type1 adaptors do not support any register writes.
>   *
>   * Returns:
>   * 0 on success, negative error code on failure
> -- 
> 2.35.1

-- 
Ville Syrjälä
Intel

Re: [PATCH v2] drm/display: Don't assume dual mode adaptors support i2c sub-addressing

[Ville Syrjälä]

On Thu, Oct 06, 2022 at 06:11:37PM +0300, Ville Syrjälä wrote:
> On Thu, Oct 06, 2022 at 11:33:14AM +0200, Simon Rettberg wrote:
> > Current dual mode adaptor ("DP++") detection code assumes that all
> > adaptors support i2c sub-addressing for read operations from the
> > DP-HDMI adaptor ID buffer.  It has been observed that multiple
> > adaptors do not in fact support this, and always return data starting
> > at register 0.  On affected adaptors, the code fails to read the proper
> > registers that would identify the device as a type 2 adaptor, and
> > handles those as type 1, limiting the TMDS clock to 165MHz, even if
> > the according register would announce a higher TMDS clock.
> > Fix this by always reading the ID buffer starting from offset 0, and
> > discarding any bytes before the actual offset of interest.
> > 
> > We tried finding authoritative documentation on whether or not this is
> > allowed behaviour, but since all the official VESA docs are paywalled,
> > the best we could come up with was the spec sheet for Texas Instruments'
> > SNx5DP149 chip family.[1]  It explicitly mentions that sub-addressing is
> > supported for register writes, but *not* for reads (See NOTE in
> > section 8.5.3).  Unless TI openly decided to violate the VESA spec, one
> > could take that as a hint that sub-addressing is in fact not mandated
> > by VESA.
> > The other two adaptors affected used the PS8409(A) and the LT8611,
> > according to the data returned from their ID buffers.
> > 
> > [1] https://www.ti.com/lit/ds/symlink/sn75dp149.pdf
> > 
> > Signed-off-by: Simon Rettberg <simon.rettberg@rz.uni-freiburg.de>
> > Reviewed-by: Rafael Gieschke <rafael.gieschke@rz.uni-freiburg.de>
> > ---
> > 
> > v2 changes form last submission's feedback (thanks for taking the time):
> > - Added a shortened version of our "background story" to the commit message
> > - Only use tmpbuf if the read offset is != 0
> 
> Bounced to intel-gfx to get the i915 CI to check it...

CI didn't blow up, and I also gave this a quick smoking on my end
with both type 1 HDMI and type 2 HDMI adaptors. 

I'm thinking we want a cc:stable on this? I can slap that on
when pushing if there are no objections?

> 
> > 
> >  .../gpu/drm/display/drm_dp_dual_mode_helper.c | 51 +++++++++++--------
> >  1 file changed, 29 insertions(+), 22 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/display/drm_dp_dual_mode_helper.c b/drivers/gpu/drm/display/drm_dp_dual_mode_helper.c
> > index 3ea53bb67d3b..bd61e20770a5 100644
> > --- a/drivers/gpu/drm/display/drm_dp_dual_mode_helper.c
> > +++ b/drivers/gpu/drm/display/drm_dp_dual_mode_helper.c
> > @@ -63,23 +63,45 @@
> >  ssize_t drm_dp_dual_mode_read(struct i2c_adapter *adapter,
> >  			      u8 offset, void *buffer, size_t size)
> >  {
> > +	u8 zero = 0;
> > +	char *tmpbuf = NULL;
> > +	/*
> > +	 * As sub-addressing is not supported by all adaptors,
> > +	 * always explicitly read from the start and discard
> > +	 * any bytes that come before the requested offset.
> > +	 * This way, no matter whether the adaptor supports it
> > +	 * or not, we'll end up reading the proper data.
> > +	 */
> >  	struct i2c_msg msgs[] = {
> >  		{
> >  			.addr = DP_DUAL_MODE_SLAVE_ADDRESS,
> >  			.flags = 0,
> >  			.len = 1,
> > -			.buf = &offset,
> > +			.buf = &zero,
> >  		},
> >  		{
> >  			.addr = DP_DUAL_MODE_SLAVE_ADDRESS,
> >  			.flags = I2C_M_RD,
> > -			.len = size,
> > +			.len = size + offset,
> >  			.buf = buffer,
> >  		},
> >  	};
> >  	int ret;
> >  
> > +	if (offset) {
> > +		tmpbuf = kmalloc(size + offset, GFP_KERNEL);
> > +		if (!tmpbuf)
> > +			return -ENOMEM;
> > +
> > +		msgs[1].buf = tmpbuf;
> > +	}
> > +
> >  	ret = i2c_transfer(adapter, msgs, ARRAY_SIZE(msgs));
> > +	if (tmpbuf)
> > +		memcpy(buffer, tmpbuf + offset, size);
> > +
> > +	kfree(tmpbuf);
> > +
> >  	if (ret < 0)
> >  		return ret;
> >  	if (ret != ARRAY_SIZE(msgs))
> > @@ -208,18 +230,6 @@ enum drm_dp_dual_mode_type drm_dp_dual_mode_detect(const struct drm_device *dev,
> >  	if (ret)
> >  		return DRM_DP_DUAL_MODE_UNKNOWN;
> >  
> > -	/*
> > -	 * Sigh. Some (maybe all?) type 1 adaptors are broken and ack
> > -	 * the offset but ignore it, and instead they just always return
> > -	 * data from the start of the HDMI ID buffer. So for a broken
> > -	 * type 1 HDMI adaptor a single byte read will always give us
> > -	 * 0x44, and for a type 1 DVI adaptor it should give 0x00
> > -	 * (assuming it implements any registers). Fortunately neither
> > -	 * of those values will match the type 2 signature of the
> > -	 * DP_DUAL_MODE_ADAPTOR_ID register so we can proceed with
> > -	 * the type 2 adaptor detection safely even in the presence
> > -	 * of broken type 1 adaptors.
> > -	 */
> >  	ret = drm_dp_dual_mode_read(adapter, DP_DUAL_MODE_ADAPTOR_ID,
> >  				    &adaptor_id, sizeof(adaptor_id));
> >  	drm_dbg_kms(dev, "DP dual mode adaptor ID: %02x (err %zd)\n", adaptor_id, ret);
> > @@ -233,11 +243,10 @@ enum drm_dp_dual_mode_type drm_dp_dual_mode_detect(const struct drm_device *dev,
> >  				return DRM_DP_DUAL_MODE_TYPE2_DVI;
> >  		}
> >  		/*
> > -		 * If neither a proper type 1 ID nor a broken type 1 adaptor
> > -		 * as described above, assume type 1, but let the user know
> > -		 * that we may have misdetected the type.
> > +		 * If not a proper type 1 ID, still assume type 1, but let
> > +		 * the user know that we may have misdetected the type.
> >  		 */
> > -		if (!is_type1_adaptor(adaptor_id) && adaptor_id != hdmi_id[0])
> > +		if (!is_type1_adaptor(adaptor_id))
> >  			drm_err(dev, "Unexpected DP dual mode adaptor ID %02x\n", adaptor_id);
> >  
> >  	}
> > @@ -343,10 +352,8 @@ EXPORT_SYMBOL(drm_dp_dual_mode_get_tmds_output);
> >   * @enable: enable (as opposed to disable) the TMDS output buffers
> >   *
> >   * Set the state of the TMDS output buffers in the adaptor. For
> > - * type2 this is set via the DP_DUAL_MODE_TMDS_OEN register. As
> > - * some type 1 adaptors have problems with registers (see comments
> > - * in drm_dp_dual_mode_detect()) we avoid touching the register,
> > - * making this function a no-op on type 1 adaptors.
> > + * type2 this is set via the DP_DUAL_MODE_TMDS_OEN register.
> > + * Type1 adaptors do not support any register writes.
> >   *
> >   * Returns:
> >   * 0 on success, negative error code on failure
> > -- 
> > 2.35.1
> 
> -- 
> Ville Syrjälä
> Intel

-- 
Ville Syrjälä
Intel

Re: [PATCH v2] drm/display: Don't assume dual mode adaptors support i2c sub-addressing

[Jani Nikula]

On Fri, 07 Oct 2022, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Thu, Oct 06, 2022 at 06:11:37PM +0300, Ville Syrjälä wrote:
>> On Thu, Oct 06, 2022 at 11:33:14AM +0200, Simon Rettberg wrote:
>> > Current dual mode adaptor ("DP++") detection code assumes that all
>> > adaptors support i2c sub-addressing for read operations from the
>> > DP-HDMI adaptor ID buffer.  It has been observed that multiple
>> > adaptors do not in fact support this, and always return data starting
>> > at register 0.  On affected adaptors, the code fails to read the proper
>> > registers that would identify the device as a type 2 adaptor, and
>> > handles those as type 1, limiting the TMDS clock to 165MHz, even if
>> > the according register would announce a higher TMDS clock.
>> > Fix this by always reading the ID buffer starting from offset 0, and
>> > discarding any bytes before the actual offset of interest.
>> > 
>> > We tried finding authoritative documentation on whether or not this is
>> > allowed behaviour, but since all the official VESA docs are paywalled,
>> > the best we could come up with was the spec sheet for Texas Instruments'
>> > SNx5DP149 chip family.[1]  It explicitly mentions that sub-addressing is
>> > supported for register writes, but *not* for reads (See NOTE in
>> > section 8.5.3).  Unless TI openly decided to violate the VESA spec, one
>> > could take that as a hint that sub-addressing is in fact not mandated
>> > by VESA.
>> > The other two adaptors affected used the PS8409(A) and the LT8611,
>> > according to the data returned from their ID buffers.
>> > 
>> > [1] https://www.ti.com/lit/ds/symlink/sn75dp149.pdf
>> > 
>> > Signed-off-by: Simon Rettberg <simon.rettberg@rz.uni-freiburg.de>
>> > Reviewed-by: Rafael Gieschke <rafael.gieschke@rz.uni-freiburg.de>
>> > ---
>> > 
>> > v2 changes form last submission's feedback (thanks for taking the time):
>> > - Added a shortened version of our "background story" to the commit message
>> > - Only use tmpbuf if the read offset is != 0
>> 
>> Bounced to intel-gfx to get the i915 CI to check it...
>
> CI didn't blow up, and I also gave this a quick smoking on my end
> with both type 1 HDMI and type 2 HDMI adaptors. 
>
> I'm thinking we want a cc:stable on this? I can slap that on
> when pushing if there are no objections?

Acked-by: Jani Nikula <jani.nikula@intel.com>

>
>> 
>> > 
>> >  .../gpu/drm/display/drm_dp_dual_mode_helper.c | 51 +++++++++++--------
>> >  1 file changed, 29 insertions(+), 22 deletions(-)
>> > 
>> > diff --git a/drivers/gpu/drm/display/drm_dp_dual_mode_helper.c b/drivers/gpu/drm/display/drm_dp_dual_mode_helper.c
>> > index 3ea53bb67d3b..bd61e20770a5 100644
>> > --- a/drivers/gpu/drm/display/drm_dp_dual_mode_helper.c
>> > +++ b/drivers/gpu/drm/display/drm_dp_dual_mode_helper.c
>> > @@ -63,23 +63,45 @@
>> >  ssize_t drm_dp_dual_mode_read(struct i2c_adapter *adapter,
>> >  			      u8 offset, void *buffer, size_t size)
>> >  {
>> > +	u8 zero = 0;
>> > +	char *tmpbuf = NULL;
>> > +	/*
>> > +	 * As sub-addressing is not supported by all adaptors,
>> > +	 * always explicitly read from the start and discard
>> > +	 * any bytes that come before the requested offset.
>> > +	 * This way, no matter whether the adaptor supports it
>> > +	 * or not, we'll end up reading the proper data.
>> > +	 */
>> >  	struct i2c_msg msgs[] = {
>> >  		{
>> >  			.addr = DP_DUAL_MODE_SLAVE_ADDRESS,
>> >  			.flags = 0,
>> >  			.len = 1,
>> > -			.buf = &offset,
>> > +			.buf = &zero,
>> >  		},
>> >  		{
>> >  			.addr = DP_DUAL_MODE_SLAVE_ADDRESS,
>> >  			.flags = I2C_M_RD,
>> > -			.len = size,
>> > +			.len = size + offset,
>> >  			.buf = buffer,
>> >  		},
>> >  	};
>> >  	int ret;
>> >  
>> > +	if (offset) {
>> > +		tmpbuf = kmalloc(size + offset, GFP_KERNEL);
>> > +		if (!tmpbuf)
>> > +			return -ENOMEM;
>> > +
>> > +		msgs[1].buf = tmpbuf;
>> > +	}
>> > +
>> >  	ret = i2c_transfer(adapter, msgs, ARRAY_SIZE(msgs));
>> > +	if (tmpbuf)
>> > +		memcpy(buffer, tmpbuf + offset, size);
>> > +
>> > +	kfree(tmpbuf);
>> > +
>> >  	if (ret < 0)
>> >  		return ret;
>> >  	if (ret != ARRAY_SIZE(msgs))
>> > @@ -208,18 +230,6 @@ enum drm_dp_dual_mode_type drm_dp_dual_mode_detect(const struct drm_device *dev,
>> >  	if (ret)
>> >  		return DRM_DP_DUAL_MODE_UNKNOWN;
>> >  
>> > -	/*
>> > -	 * Sigh. Some (maybe all?) type 1 adaptors are broken and ack
>> > -	 * the offset but ignore it, and instead they just always return
>> > -	 * data from the start of the HDMI ID buffer. So for a broken
>> > -	 * type 1 HDMI adaptor a single byte read will always give us
>> > -	 * 0x44, and for a type 1 DVI adaptor it should give 0x00
>> > -	 * (assuming it implements any registers). Fortunately neither
>> > -	 * of those values will match the type 2 signature of the
>> > -	 * DP_DUAL_MODE_ADAPTOR_ID register so we can proceed with
>> > -	 * the type 2 adaptor detection safely even in the presence
>> > -	 * of broken type 1 adaptors.
>> > -	 */
>> >  	ret = drm_dp_dual_mode_read(adapter, DP_DUAL_MODE_ADAPTOR_ID,
>> >  				    &adaptor_id, sizeof(adaptor_id));
>> >  	drm_dbg_kms(dev, "DP dual mode adaptor ID: %02x (err %zd)\n", adaptor_id, ret);
>> > @@ -233,11 +243,10 @@ enum drm_dp_dual_mode_type drm_dp_dual_mode_detect(const struct drm_device *dev,
>> >  				return DRM_DP_DUAL_MODE_TYPE2_DVI;
>> >  		}
>> >  		/*
>> > -		 * If neither a proper type 1 ID nor a broken type 1 adaptor
>> > -		 * as described above, assume type 1, but let the user know
>> > -		 * that we may have misdetected the type.
>> > +		 * If not a proper type 1 ID, still assume type 1, but let
>> > +		 * the user know that we may have misdetected the type.
>> >  		 */
>> > -		if (!is_type1_adaptor(adaptor_id) && adaptor_id != hdmi_id[0])
>> > +		if (!is_type1_adaptor(adaptor_id))
>> >  			drm_err(dev, "Unexpected DP dual mode adaptor ID %02x\n", adaptor_id);
>> >  
>> >  	}
>> > @@ -343,10 +352,8 @@ EXPORT_SYMBOL(drm_dp_dual_mode_get_tmds_output);
>> >   * @enable: enable (as opposed to disable) the TMDS output buffers
>> >   *
>> >   * Set the state of the TMDS output buffers in the adaptor. For
>> > - * type2 this is set via the DP_DUAL_MODE_TMDS_OEN register. As
>> > - * some type 1 adaptors have problems with registers (see comments
>> > - * in drm_dp_dual_mode_detect()) we avoid touching the register,
>> > - * making this function a no-op on type 1 adaptors.
>> > + * type2 this is set via the DP_DUAL_MODE_TMDS_OEN register.
>> > + * Type1 adaptors do not support any register writes.
>> >   *
>> >   * Returns:
>> >   * 0 on success, negative error code on failure
>> > -- 
>> > 2.35.1
>> 
>> -- 
>> Ville Syrjälä
>> Intel

-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [PATCH v2] drm/display: Don't assume dual mode adaptors support i2c sub-addressing

[Jani Nikula]

On Fri, 07 Oct 2022, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Thu, Oct 06, 2022 at 06:11:37PM +0300, Ville Syrjälä wrote:
>> On Thu, Oct 06, 2022 at 11:33:14AM +0200, Simon Rettberg wrote:
>> > Current dual mode adaptor ("DP++") detection code assumes that all
>> > adaptors support i2c sub-addressing for read operations from the
>> > DP-HDMI adaptor ID buffer.  It has been observed that multiple
>> > adaptors do not in fact support this, and always return data starting
>> > at register 0.  On affected adaptors, the code fails to read the proper
>> > registers that would identify the device as a type 2 adaptor, and
>> > handles those as type 1, limiting the TMDS clock to 165MHz, even if
>> > the according register would announce a higher TMDS clock.
>> > Fix this by always reading the ID buffer starting from offset 0, and
>> > discarding any bytes before the actual offset of interest.
>> > 
>> > We tried finding authoritative documentation on whether or not this is
>> > allowed behaviour, but since all the official VESA docs are paywalled,
>> > the best we could come up with was the spec sheet for Texas Instruments'
>> > SNx5DP149 chip family.[1]  It explicitly mentions that sub-addressing is
>> > supported for register writes, but *not* for reads (See NOTE in
>> > section 8.5.3).  Unless TI openly decided to violate the VESA spec, one
>> > could take that as a hint that sub-addressing is in fact not mandated
>> > by VESA.
>> > The other two adaptors affected used the PS8409(A) and the LT8611,
>> > according to the data returned from their ID buffers.
>> > 
>> > [1] https://www.ti.com/lit/ds/symlink/sn75dp149.pdf
>> > 
>> > Signed-off-by: Simon Rettberg <simon.rettberg@rz.uni-freiburg.de>
>> > Reviewed-by: Rafael Gieschke <rafael.gieschke@rz.uni-freiburg.de>
>> > ---
>> > 
>> > v2 changes form last submission's feedback (thanks for taking the time):
>> > - Added a shortened version of our "background story" to the commit message
>> > - Only use tmpbuf if the read offset is != 0
>> 
>> Bounced to intel-gfx to get the i915 CI to check it...
>
> CI didn't blow up, and I also gave this a quick smoking on my end
> with both type 1 HDMI and type 2 HDMI adaptors. 
>
> I'm thinking we want a cc:stable on this? I can slap that on
> when pushing if there are no objections?

I guess this fell between the cracks? :(

Ville, r-b? Going to push?

BR,
Jani.






>
>> 
>> > 
>> >  .../gpu/drm/display/drm_dp_dual_mode_helper.c | 51 +++++++++++--------
>> >  1 file changed, 29 insertions(+), 22 deletions(-)
>> > 
>> > diff --git a/drivers/gpu/drm/display/drm_dp_dual_mode_helper.c b/drivers/gpu/drm/display/drm_dp_dual_mode_helper.c
>> > index 3ea53bb67d3b..bd61e20770a5 100644
>> > --- a/drivers/gpu/drm/display/drm_dp_dual_mode_helper.c
>> > +++ b/drivers/gpu/drm/display/drm_dp_dual_mode_helper.c
>> > @@ -63,23 +63,45 @@
>> >  ssize_t drm_dp_dual_mode_read(struct i2c_adapter *adapter,
>> >  			      u8 offset, void *buffer, size_t size)
>> >  {
>> > +	u8 zero = 0;
>> > +	char *tmpbuf = NULL;
>> > +	/*
>> > +	 * As sub-addressing is not supported by all adaptors,
>> > +	 * always explicitly read from the start and discard
>> > +	 * any bytes that come before the requested offset.
>> > +	 * This way, no matter whether the adaptor supports it
>> > +	 * or not, we'll end up reading the proper data.
>> > +	 */
>> >  	struct i2c_msg msgs[] = {
>> >  		{
>> >  			.addr = DP_DUAL_MODE_SLAVE_ADDRESS,
>> >  			.flags = 0,
>> >  			.len = 1,
>> > -			.buf = &offset,
>> > +			.buf = &zero,
>> >  		},
>> >  		{
>> >  			.addr = DP_DUAL_MODE_SLAVE_ADDRESS,
>> >  			.flags = I2C_M_RD,
>> > -			.len = size,
>> > +			.len = size + offset,
>> >  			.buf = buffer,
>> >  		},
>> >  	};
>> >  	int ret;
>> >  
>> > +	if (offset) {
>> > +		tmpbuf = kmalloc(size + offset, GFP_KERNEL);
>> > +		if (!tmpbuf)
>> > +			return -ENOMEM;
>> > +
>> > +		msgs[1].buf = tmpbuf;
>> > +	}
>> > +
>> >  	ret = i2c_transfer(adapter, msgs, ARRAY_SIZE(msgs));
>> > +	if (tmpbuf)
>> > +		memcpy(buffer, tmpbuf + offset, size);
>> > +
>> > +	kfree(tmpbuf);
>> > +
>> >  	if (ret < 0)
>> >  		return ret;
>> >  	if (ret != ARRAY_SIZE(msgs))
>> > @@ -208,18 +230,6 @@ enum drm_dp_dual_mode_type drm_dp_dual_mode_detect(const struct drm_device *dev,
>> >  	if (ret)
>> >  		return DRM_DP_DUAL_MODE_UNKNOWN;
>> >  
>> > -	/*
>> > -	 * Sigh. Some (maybe all?) type 1 adaptors are broken and ack
>> > -	 * the offset but ignore it, and instead they just always return
>> > -	 * data from the start of the HDMI ID buffer. So for a broken
>> > -	 * type 1 HDMI adaptor a single byte read will always give us
>> > -	 * 0x44, and for a type 1 DVI adaptor it should give 0x00
>> > -	 * (assuming it implements any registers). Fortunately neither
>> > -	 * of those values will match the type 2 signature of the
>> > -	 * DP_DUAL_MODE_ADAPTOR_ID register so we can proceed with
>> > -	 * the type 2 adaptor detection safely even in the presence
>> > -	 * of broken type 1 adaptors.
>> > -	 */
>> >  	ret = drm_dp_dual_mode_read(adapter, DP_DUAL_MODE_ADAPTOR_ID,
>> >  				    &adaptor_id, sizeof(adaptor_id));
>> >  	drm_dbg_kms(dev, "DP dual mode adaptor ID: %02x (err %zd)\n", adaptor_id, ret);
>> > @@ -233,11 +243,10 @@ enum drm_dp_dual_mode_type drm_dp_dual_mode_detect(const struct drm_device *dev,
>> >  				return DRM_DP_DUAL_MODE_TYPE2_DVI;
>> >  		}
>> >  		/*
>> > -		 * If neither a proper type 1 ID nor a broken type 1 adaptor
>> > -		 * as described above, assume type 1, but let the user know
>> > -		 * that we may have misdetected the type.
>> > +		 * If not a proper type 1 ID, still assume type 1, but let
>> > +		 * the user know that we may have misdetected the type.
>> >  		 */
>> > -		if (!is_type1_adaptor(adaptor_id) && adaptor_id != hdmi_id[0])
>> > +		if (!is_type1_adaptor(adaptor_id))
>> >  			drm_err(dev, "Unexpected DP dual mode adaptor ID %02x\n", adaptor_id);
>> >  
>> >  	}
>> > @@ -343,10 +352,8 @@ EXPORT_SYMBOL(drm_dp_dual_mode_get_tmds_output);
>> >   * @enable: enable (as opposed to disable) the TMDS output buffers
>> >   *
>> >   * Set the state of the TMDS output buffers in the adaptor. For
>> > - * type2 this is set via the DP_DUAL_MODE_TMDS_OEN register. As
>> > - * some type 1 adaptors have problems with registers (see comments
>> > - * in drm_dp_dual_mode_detect()) we avoid touching the register,
>> > - * making this function a no-op on type 1 adaptors.
>> > + * type2 this is set via the DP_DUAL_MODE_TMDS_OEN register.
>> > + * Type1 adaptors do not support any register writes.
>> >   *
>> >   * Returns:
>> >   * 0 on success, negative error code on failure
>> > -- 
>> > 2.35.1
>> 
>> -- 
>> Ville Syrjälä
>> Intel

-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [PATCH v2] drm/display: Don't assume dual mode adaptors support i2c sub-addressing

[Ville Syrjälä]

On Tue, Nov 15, 2022 at 06:31:31PM +0200, Jani Nikula wrote:
> On Fri, 07 Oct 2022, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > On Thu, Oct 06, 2022 at 06:11:37PM +0300, Ville Syrjälä wrote:
> >> On Thu, Oct 06, 2022 at 11:33:14AM +0200, Simon Rettberg wrote:
> >> > Current dual mode adaptor ("DP++") detection code assumes that all
> >> > adaptors support i2c sub-addressing for read operations from the
> >> > DP-HDMI adaptor ID buffer.  It has been observed that multiple
> >> > adaptors do not in fact support this, and always return data starting
> >> > at register 0.  On affected adaptors, the code fails to read the proper
> >> > registers that would identify the device as a type 2 adaptor, and
> >> > handles those as type 1, limiting the TMDS clock to 165MHz, even if
> >> > the according register would announce a higher TMDS clock.
> >> > Fix this by always reading the ID buffer starting from offset 0, and
> >> > discarding any bytes before the actual offset of interest.
> >> > 
> >> > We tried finding authoritative documentation on whether or not this is
> >> > allowed behaviour, but since all the official VESA docs are paywalled,
> >> > the best we could come up with was the spec sheet for Texas Instruments'
> >> > SNx5DP149 chip family.[1]  It explicitly mentions that sub-addressing is
> >> > supported for register writes, but *not* for reads (See NOTE in
> >> > section 8.5.3).  Unless TI openly decided to violate the VESA spec, one
> >> > could take that as a hint that sub-addressing is in fact not mandated
> >> > by VESA.
> >> > The other two adaptors affected used the PS8409(A) and the LT8611,
> >> > according to the data returned from their ID buffers.
> >> > 
> >> > [1] https://www.ti.com/lit/ds/symlink/sn75dp149.pdf
> >> > 
> >> > Signed-off-by: Simon Rettberg <simon.rettberg@rz.uni-freiburg.de>
> >> > Reviewed-by: Rafael Gieschke <rafael.gieschke@rz.uni-freiburg.de>
> >> > ---
> >> > 
> >> > v2 changes form last submission's feedback (thanks for taking the time):
> >> > - Added a shortened version of our "background story" to the commit message
> >> > - Only use tmpbuf if the read offset is != 0
> >> 
> >> Bounced to intel-gfx to get the i915 CI to check it...
> >
> > CI didn't blow up, and I also gave this a quick smoking on my end
> > with both type 1 HDMI and type 2 HDMI adaptors. 
> >
> > I'm thinking we want a cc:stable on this? I can slap that on
> > when pushing if there are no objections?
> 
> I guess this fell between the cracks? :(

I think I just got stuck waiting for people to say
something about the cc:stable idea. But presumbly
since no one objected everyone is happy with it.

> 
> Ville, r-b? Going to push?

Pushed to drm-misc-fixes now, with cc:stable.
Thanks for the patch.

-- 
Ville Syrjälä
Intel