From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id C37C4C07E9D for ; Mon, 26 Sep 2022 12:38:51 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 5A76B10E3E6; Mon, 26 Sep 2022 12:38:50 +0000 (UTC) Received: from mga17.intel.com (mga17.intel.com [192.55.52.151]) by gabe.freedesktop.org (Postfix) with ESMTPS id 11DB810E258 for ; Mon, 26 Sep 2022 12:38:48 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1664195928; x=1695731928; h=date:from:to:cc:subject:message-id:references: mime-version:content-transfer-encoding:in-reply-to; bh=FbTGbCNs121ZGo7Dpzt5/DPqhFbOJ0ZmE5LfYMNH8EA=; b=UhPpdeqnHk8GTGEFWisn8JteCg21pvNKNThFii0C2Tf+Kqok9YAud+K7 lFpm7qLP0BQx9OUrpvXI5Tt5NhaRPGrsEdx5b++rSNbAAbqHYYb6KmXtA cqiMSkIshq1L/LXNVna++DjHgn/swotbsa29kCSPHKHSPAxwPk5RCPo02 43zEP7EpMLBEg9jqtwjyA8GKY9VSmhqalK901p9RhtXI2a4iG8IJ1bDxs RGIaViBGyTdMVZvReqyDPtefGG+R8lHpc1uYHjBSEmTBITq9vHI1dmBim yH6dofan7FrWoJTcmhYsS03jkQYOoFLzUnj7dAIU9LFfrZJZJcSaM8Eco w==; X-IronPort-AV: E=McAfee;i="6500,9779,10481"; a="281388624" X-IronPort-AV: E=Sophos;i="5.93,346,1654585200"; d="scan'208";a="281388624" Received: from orsmga008.jf.intel.com ([10.7.209.65]) by fmsmga107.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 26 Sep 2022 05:38:47 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6500,9779,10481"; a="651808560" X-IronPort-AV: E=Sophos;i="5.93,346,1654585200"; d="scan'208";a="651808560" Received: from stinkpipe.fi.intel.com (HELO stinkbox) ([10.237.72.191]) by orsmga008.jf.intel.com with SMTP; 26 Sep 2022 05:38:44 -0700 Received: by stinkbox (sSMTP sendmail emulation); Mon, 26 Sep 2022 15:38:43 +0300 Date: Mon, 26 Sep 2022 15:38:43 +0300 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= To: Simon Rettberg Subject: Re: [PATCH RESEND] drm/display: Don't assume dual mode adaptors support i2c sub-addressing Message-ID: References: <20220926124017.529806df@computer> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20220926124017.529806df@computer> X-Patchwork-Hint: comment X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Thomas Zimmermann , linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, Alex Deucher Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" 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 > Reviewed-by: Rafael Gieschke > --- > (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. > > [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. > + > 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. > 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 From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id F1342C07E9D for ; Mon, 26 Sep 2022 14:26:49 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S234716AbiIZO0r (ORCPT ); Mon, 26 Sep 2022 10:26:47 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:47646 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S234150AbiIZO0R (ORCPT ); Mon, 26 Sep 2022 10:26:17 -0400 Received: from mga12.intel.com (mga12.intel.com [192.55.52.136]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id F074328726 for ; Mon, 26 Sep 2022 05:38:47 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1664195927; x=1695731927; h=date:from:to:cc:subject:message-id:references: mime-version:content-transfer-encoding:in-reply-to; bh=FbTGbCNs121ZGo7Dpzt5/DPqhFbOJ0ZmE5LfYMNH8EA=; b=GEQczkSG5C26jP8LSmIfgXjlj2K+0lflpEQUSmTgnIhaAbrg0UtXNo9G +fwcuy7ZyNFXW47L0nE0K7RVkXydEXWJ0PBqJ4Ew6LNaIkuhAzPk6KYON Pmr/9HraXmSRhkhJkGclsXyqipgRtXnPxVPL+MZSzP1o6KKdOE41GuTxu wzbFWo8YQdZjOU9+Y+xVYevrlx2K63N0OcF24ahNO7t4kUi+XFCqHlEML 8vJCwdPKhjcIBKLBEgHaWNgCzur57xl5IuwjLEt6Y2BQMRcwLvOXyv59b Htykdf9QN2sHeGmQ7yizEJBlb38HxYXepn5+hXd1sWNFyBDcpSfsWBnMl w==; X-IronPort-AV: E=McAfee;i="6500,9779,10481"; a="280740667" X-IronPort-AV: E=Sophos;i="5.93,346,1654585200"; d="scan'208";a="280740667" Received: from orsmga008.jf.intel.com ([10.7.209.65]) by fmsmga106.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 26 Sep 2022 05:38:47 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6500,9779,10481"; a="651808560" X-IronPort-AV: E=Sophos;i="5.93,346,1654585200"; d="scan'208";a="651808560" Received: from stinkpipe.fi.intel.com (HELO stinkbox) ([10.237.72.191]) by orsmga008.jf.intel.com with SMTP; 26 Sep 2022 05:38:44 -0700 Received: by stinkbox (sSMTP sendmail emulation); Mon, 26 Sep 2022 15:38:43 +0300 Date: Mon, 26 Sep 2022 15:38:43 +0300 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= To: Simon Rettberg Cc: linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, David Airlie , Daniel Vetter , Lyude Paul , Thomas Zimmermann , Alex Deucher Subject: Re: [PATCH RESEND] drm/display: Don't assume dual mode adaptors support i2c sub-addressing Message-ID: References: <20220926124017.529806df@computer> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20220926124017.529806df@computer> X-Patchwork-Hint: comment Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > Reviewed-by: Rafael Gieschke > --- > (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. > > [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. > + > 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. > 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