Message-ID: <20220926173214.5af2cd8f@computer>
Date:   Mon, 26 Sep 2022 17:32:14 +0200
From:   Simon Rettberg <simon.rettberg@...uni-freiburg.de>
To:     Ville Syrjälä <ville.syrjala@...ux.intel.com>
Cc:     linux-kernel@...r.kernel.org, dri-devel@...ts.freedesktop.org,
        David Airlie <airlied@...il.com>,
        Daniel Vetter <daniel@...ll.ch>, Lyude Paul <lyude@...hat.com>,
        Thomas Zimmermann <tzimmermann@...e.de>,
        Alex Deucher <alexander.deucher@....com>
Subject: Re: [PATCH RESEND] drm/display: Don't assume dual mode adaptors
 support i2c sub-addressing

Am Mon, 26 Sep 2022 17:57:47 +0300
schrieb Ville Syrjälä <ville.syrjala@...ux.intel.com>:

> On Mon, Sep 26, 2022 at 04:34:08PM +0200, Simon Rettberg wrote:
> > On Mon, 26 Sep 2022 15:38:43 +0300
> >  Ville Syrjälä <ville.syrjala@...ux.intel.com> wrote:
> >   
> > > On Mon, Sep 26, 2022 at 12:40:17PM +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 failed to read the proper registers that would identify
> > > > the device as a type 2 adaptor, and handled those as type 1,
> > > > limiting the TMDS clock to 165MHz. Fix this by always reading
> > > > the ID buffer starting from offset 0, and discarding any bytes
> > > > before the actual offset of interest.
> > > > 
> > > > Signed-off-by: Simon Rettberg
> > > > <simon.rettberg@...uni-freiburg.de> Reviewed-by: Rafael
> > > > Gieschke <rafael.gieschke@...uni-freiburg.de> ---
> > > > (Resend because of no response, probably my fault since I ran
> > > > get_maintainers on a shallow clone and missed a bunch of people)
> > > > 
> > > > We had problems with multiple different "4k ready" DP++ adaptors
> > > > only resulting in 1080p resolution on Linux. While one of them
> > > > turned out to actually just be a type1 adaptor, the others,
> > > > according to the data retreived via i2cdump, were in fact proper
> > > > type2 adaptors, advertising a TMDS clock of 300MHz. As it turned
> > > > out, none of them supported sub-addressing when reading from the
> > > > DP-HDMI adaptor ID buffer via i2c. The existing code suggested
> > > > that this is known to happen with "broken" type1 adaptors, but
> > > > evidently, type2 adaptors are also affected. We tried finding
> > > > authoritative documentation on whether or not this is allowed
> > > > behavior, 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-adressing is supported for register writes, but *not*
> > > > for reads (See NOTE in section 8.5.3). Unless TI blatantly and
> > > > openly decided to violate the VESA spec, one could take that as
> > > > a strong hint that sub-addressing is in fact not mandated by
> > > > VESA.    
> > > 
> > > I don't think that would pass the dual mode CTS for type2 adaptors
> > > since it explicitly calls for reading individual bytes from
> > > various offsets.
> > > 
> > > The actual dual mode spec specifies things rather poorly.
> > > Technically it doesn't even specify the write protocol, and the
> > > read protocol is only specified in the form of an example read of
> > > the HDMI ID buffer. There it says the offset write is optional
> > > for the master, but mandatory for the slave to ack. It neither
> > > explicitly allows nor disallows the ack+ignore behaviour, but
> > > IIRC there is some text in there that suggests that type1
> > > adaptors might ignore it.  
> > 
> > Interesting, but poor spec would explain why it's not implemented by
> > at least three such chips. That's the TI one (we don't actually have
> > it, but the data sheet above seems quite clear), and the two we
> > confirmed it with: the PS8409(A), and the LT8611.
> > So either way it might make sense to handle this. Since the first
> > submission of this patch I also took the time to check it on
> > Windows 10, and both adaptors make Windows list 4k resolutions with
> > both the intel iGPU and an nvidia card.
> > 
> > Here are the two dumps for completeness:
> > 
> >      0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f
> > 0123456789abcdef 00: 44 50 2d 48 44 4d 49 20 41 44 41 50 54 4f 52
> > 04    DP-HDMI ADAPTOR? 10: a0 00 1c f8 50 53 38 34 30 39 a2 00 00
> > 78 08 ff    ?.??PS8409?..x?.
> > 
> >      0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f
> > 0123456789abcdef 00: 44 50 2d 48 44 4d 49 20 41 44 41 50 54 4f 52
> > 04    DP-HDMI ADAPTOR? 10: a0 ff ff ff 4c 54 38 36 31 31 a2 00 00
> > 78 0f 00    ?...LT8611?..x?. 
> > >   
> > > > 
> > > > [1] https://www.ti.com/lit/ds/symlink/sn75dp149.pdf
> > > > 
> > > >  .../gpu/drm/display/drm_dp_dual_mode_helper.c | 52
> > > > ++++++++++--------- 1 file changed, 28 insertions(+), 24
> > > > 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
> > > > 3ea53bb67..6147da983 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,42 @@ ssize_t drm_dp_dual_mode_read(struct i2c_adapter
> > > > *adapter, u8 offset, void *buffer, size_t size)
> > > >  {
> > > > +	int ret;
> > > > +	u8 zero = 0;
> > > > +	char *tmpbuf;
> > > > +	/*
> > > > +	 * 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,
> > > > -			.buf = buffer,
> > > > +			.len = size + offset,
> > > > +			.buf = NULL,
> > > >  		},
> > > >  	};
> > > > -	int ret;
> > > >  
> > > > +	tmpbuf = kmalloc(size + offset, GFP_KERNEL);
> > > > +	if (!tmpbuf)
> > > > +		return -ENOMEM;
> > > > +
> > > > +	msgs[1].buf = tmpbuf;
> > > >  	ret = i2c_transfer(adapter, msgs, ARRAY_SIZE(msgs));
> > > > +	if (ret == ARRAY_SIZE(msgs))
> > > > +		memcpy(buffer, tmpbuf + offset, size);
> > > > +
> > > > +	kfree(tmpbuf);    
> > > 
> > > Could optimize a bit here and avoid the temp buffer when
> > > the original offset is 0.  
> > 
> > Was thinking about that too while writing the patch, but decided
> > to keep it as straight forward as possible for the initial
> > submission; it's also not really performance critical, should be
> > called a few times when the adaptor is plugged in, and probably
> > just once with offset 0.  
> 
> Would avoid the extra failure point.
> 
> Looks like all you really need to do is make the tmpbuf 
> allocation (+ msg mangling) conditional, and check that
> tmpbuf was allocated before calling memcpy().

Right, that sounds good.

> 
> > It also didn't feel nice to have the "if (ret == ARRAY_SIZE(msgs))"
> > check duplicated for the memcpy, to avoid copying potentially
> > uninitialised memory into the output buffer. I didn't see how this
> > would lead to an information leak to user space with the current
> > code base, but better safe than sorry? :)
> > The alternative is to move the memcpy down and merge it with the
> > other if-block, but then we'd need a cleanup label at the bottom
> > to do the kfree in the error case that comes before that...  
> 
> kfree(NULL) is perfectly legal.

This was regarding the conditional for the memcpy(). In the current
patch, it's not copying anything from tmpbuf into the output buffer
if the i2c_transfer() call fails, because maybe there is some
sensitive information in tmpbuf from a previous allocation. But
it's not strictly necessary for correctness, as drm_dp_dual_mode_read()
would return nonzero in this case and the output buffer is ignored
at all current call sites. This was along the lines of "what if later
on, someone adds a sysfs interface that calls drm_dp_dual_mode_read()
and returns the data without checking the return value". Or maybe just
make it a kzalloc instead? I'm probably overthinking this.

So, a rough prototype:

	char *tmpbuf = NULL;
	struct i2c_msg msgs[] = {
		{
			.addr = DP_DUAL_MODE_SLAVE_ADDRESS,
			.flags = 0,
			.len = 1,
			.buf = &zero,
		},
		{
			.addr = DP_DUAL_MODE_SLAVE_ADDRESS,
			.flags = I2C_M_RD,
			.len = size + offset,
			.buf = buffer,
		},
	};

	if (offset) {
		tmpbuf = kzalloc(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))
		return -EPROTO;
	return 0;


> 
> > But I'll happily refine that further and submit a v2 if desired.
> >   
> > >   
> > > > +
> > > >  	if (ret < 0)
> > > >  		return ret;
> > > >  	if (ret != ARRAY_SIZE(msgs))
> > > > @@ -208,18 +227,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));    
> > > 
> > > Another optimization opportunity here to maybe combine the HDMI ID
> > > buffer read with this one. Could perhaps just read the full 32
> > > bytes static capabilities section. But this one should probably
> > > be left for a separate patch. Ideally I guess we'd also combine
> > > the max TMDS clock read with this one. But for that we'd need to
> > > return more than the single enum drm_dp_dual_mode_type from this
> > > function.  
> > 
> > Pretty much same as above, keep v1 simple, but I noticed that too.
> > If that's going to be another patch anyways, it might make sense
> > if that's done by someone more familiar with that code in general
> > (basically had to research all this DP++/i2c stuff from scratch).
> > But I could give it a spin.  
> 
> I think this one will need a bit more restructuing so better
> done as a separate patch. Might not really be worth the hassle
> unless we go all in and try to do just a single 32byte read
> for everything including the max TMDS clock.

ack!

> 
> >   
> > >   
> > > >  	drm_dbg_kms(dev, "DP dual mode adaptor ID: %02x (err
> > > > %zd)\n", adaptor_id, ret); @@ -233,11 +240,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 +349,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    
> > >   
> 

Powered by blists - more mailing lists