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

From: Simon Rettberg
Date: Mon Sep 26 2022 - 12:44:45 EST

Next message: Liang, Kan: "Re: [RFC PATCH v2 2/3] perf/x86/intel/pt: Introduce and export pt_get_curr_event()"
Previous message: Krishna Chaitanya Chundru: "Re: [PATCH v7 1/5] PCI: qcom: Add system suspend and resume support"
In reply to: Ville Syrjälä: "Re: [PATCH RESEND] drm/display: Don't assume dual mode adaptors support i2c sub-addressing"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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

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

Next message: Liang, Kan: "Re: [RFC PATCH v2 2/3] perf/x86/intel/pt: Introduce and export pt_get_curr_event()"
Previous message: Krishna Chaitanya Chundru: "Re: [PATCH v7 1/5] PCI: qcom: Add system suspend and resume support"
In reply to: Ville Syrjälä: "Re: [PATCH RESEND] drm/display: Don't assume dual mode adaptors support i2c sub-addressing"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]