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@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxx>
>> > Reviewed-by: Rafael Gieschke <rafael.gieschke@xxxxxxxxxxxxxxxxxx>
>> > ---
>> > 
>> > 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@xxxxxxxxx>

>
>> 
>> > 
>> >  .../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