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().
> 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.
> 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.
>
> >
> > > 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
> >
--
Ville Syrjälä
Intel
