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? 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