Message ID | 20230707215851.590754-1-lyude@redhat.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:9f45:0:b0:3ea:f831:8777 with SMTP id v5csp3568005vqx; Fri, 7 Jul 2023 15:12:32 -0700 (PDT) X-Google-Smtp-Source: APBJJlEPa0VoCbh/qUwSYiq0WT31WWPBDT2zjY4DZGYc6NfHngWhYENrmCAxTHkq5vNa4Nenhh5Y X-Received: by 2002:a05:6808:159d:b0:3a3:76c6:a46f with SMTP id t29-20020a056808159d00b003a376c6a46fmr7228134oiw.38.1688767951716; Fri, 07 Jul 2023 15:12:31 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1688767951; cv=none; d=google.com; s=arc-20160816; b=UQ91UxcjpaWYJmEMLyNQG1WZmhuPwYz29cmexEiIRyLrB30c6uhwICVnhqvvs0d5aw H+F0TG0pSbtmfH0sgoqnnlklDwXtR9rCr0mI9aMiPqFz7BwQtoijwJt4daj5gfg+1za0 MYmM5jrl9Ci5ftPUIIZGGA7UtEWroOkxY5SmaXap8PMJ/N7H7D/xsLe+/OFQpYMWGbXM Vgyk9zJVrVgix2boUiZB9wK/6GUuw7iKdiRCrDKxaf4tG0tNWv9MPLjjRg0+Fne5vJvJ vOm8SPLA3DmQfT+x3eUv8H8eCwA+L1yZncEXlS9yvHfWbhwm4eZZo4kE2nIzSVlhAtqR iyhQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :message-id:date:subject:cc:to:from:dkim-signature; bh=+rsK4w5s/iuBLQer6P3Y3zMk8adlTjvdF1ODpcoFTPU=; fh=xdXaP+7bKTRzJU3rz/edbNHRkizq+ygjdCPDor55Hzk=; b=y7Kq0hRKl2T7SdzY30kJ/UL6sBtmlCmHEkBN/Q7pLBa/Yon0r4rSCkuT6nj28Eb/uM U0P2433G8jryES9ww02RhhtRLpQ/oJHo+uspaj3R+nw4G5ACaZCSgT8fookC3Nr6zoif LZns/spscydVafAJS1YyI1ylGr28J7icWO6E1ytsP/g1ZITwkM7VSK7VNIoSHNXP6fUC EQM5kUu0wP2+wRIxDZ+Ib3ei5+6HO7QidaHiET3X3AZIt5OsrkDC/4ZOF3YUT73fjIyN td3qeZF3wSEi2H8/M2qztiZiojlENSe31H1lTpwyAjWhKLPTd6tV/CCVnqBdebKAjFmJ //Pg== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=RIikZgml; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=redhat.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id g20-20020a056a0023d400b0066e96a581c3si4610569pfc.5.2023.07.07.15.12.17; Fri, 07 Jul 2023 15:12:31 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=RIikZgml; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=redhat.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231150AbjGGV7s (ORCPT <rfc822;tebrre53rla2o@gmail.com> + 99 others); Fri, 7 Jul 2023 17:59:48 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:46922 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229902AbjGGV7p (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Fri, 7 Jul 2023 17:59:45 -0400 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 13C7A2694 for <linux-kernel@vger.kernel.org>; Fri, 7 Jul 2023 14:58:57 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1688767137; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version: content-transfer-encoding:content-transfer-encoding; bh=+rsK4w5s/iuBLQer6P3Y3zMk8adlTjvdF1ODpcoFTPU=; b=RIikZgml/bd2Fp6jjRlKy57+sVSfQJsnel/A1EBvZNgebfC0OaMOq75e16F1SNokD47Mpu qAmfOGaaoMWHlCZiS1v03rwaL2p9+wORlklgWV605/K5JBYCCdqzz/l3igzvKweYF+n8mR Gez2hZr42Q46WwmrGuNbIM9RMeTm5xI= Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-675-Vrnd9375MzivrBaS-TSFGQ-1; Fri, 07 Jul 2023 17:58:56 -0400 X-MC-Unique: Vrnd9375MzivrBaS-TSFGQ-1 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.rdu2.redhat.com [10.11.54.8]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id A294B858EED; Fri, 7 Jul 2023 21:58:55 +0000 (UTC) Received: from emerald.redhat.com (unknown [10.22.33.28]) by smtp.corp.redhat.com (Postfix) with ESMTP id 1D10BC09A09; Fri, 7 Jul 2023 21:58:55 +0000 (UTC) From: Lyude Paul <lyude@redhat.com> To: nouveau@lists.freedesktop.org, dri-devel@lists.freedesktop.org Cc: Ben Skeggs <bskeggs@redhat.com>, Karol Herbst <kherbst@redhat.com>, David Airlie <airlied@gmail.com>, Daniel Vetter <daniel@ffwll.ch>, Dave Airlie <airlied@redhat.com>, linux-kernel@vger.kernel.org (open list) Subject: [PATCH] drm/nouveau/nvkm/dp: Add hack to fix DP 1.3+ DPCD issues Date: Fri, 7 Jul 2023 17:58:51 -0400 Message-Id: <20230707215851.590754-1-lyude@redhat.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Scanned-By: MIMEDefang 3.1 on 10.11.54.8 X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H4,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE, T_SCC_BODY_TEXT_LINE,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1770801543630831088?= X-GMAIL-MSGID: =?utf-8?q?1770801543630831088?= |
Series |
drm/nouveau/nvkm/dp: Add hack to fix DP 1.3+ DPCD issues
|
|
Commit Message
Lyude Paul
July 7, 2023, 9:58 p.m. UTC
Currently we use the drm_dp_dpcd_read_caps() helper in the DRM side of
nouveau in order to read the DPCD of a DP connector, which makes sure we do
the right thing and also check for extended DPCD caps. However, it turns
out we're not currently doing this on the nvkm side since we don't have
access to the drm_dp_aux structure there - which means that the DRM side of
the driver and the NVKM side can end up with different DPCD capabilities
for the same connector.
Ideally to fix this, we want to start setting up the drm_dp_aux device in
NVKM before we've made contact with the DRM side - which should be pretty
easy to accomplish (I'm already working on it!). Until then however, let's
workaround this problem by porting a copy of drm_dp_read_dpcd_caps() into
NVKM - which should fix this issue.
Issue: https://gitlab.freedesktop.org/drm/nouveau/-/issues/211
Signed-off-by: Lyude Paul <lyude@redhat.com>
---
drivers/gpu/drm/nouveau/nvkm/engine/disp/dp.c | 48 ++++++++++++++++++-
1 file changed, 47 insertions(+), 1 deletion(-)
Comments
On Fri, Jul 7, 2023 at 11:58 PM Lyude Paul <lyude@redhat.com> wrote: > > Currently we use the drm_dp_dpcd_read_caps() helper in the DRM side of > nouveau in order to read the DPCD of a DP connector, which makes sure we do > the right thing and also check for extended DPCD caps. However, it turns > out we're not currently doing this on the nvkm side since we don't have > access to the drm_dp_aux structure there - which means that the DRM side of > the driver and the NVKM side can end up with different DPCD capabilities > for the same connector. > > Ideally to fix this, we want to start setting up the drm_dp_aux device in > NVKM before we've made contact with the DRM side - which should be pretty > easy to accomplish (I'm already working on it!). Until then however, let's > workaround this problem by porting a copy of drm_dp_read_dpcd_caps() into > NVKM - which should fix this issue. > > Issue: https://gitlab.freedesktop.org/drm/nouveau/-/issues/211 Should a Fixes: or Cc: stable tag be added so it gets backported? > Signed-off-by: Lyude Paul <lyude@redhat.com> > --- > drivers/gpu/drm/nouveau/nvkm/engine/disp/dp.c | 48 ++++++++++++++++++- > 1 file changed, 47 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/nouveau/nvkm/engine/disp/dp.c b/drivers/gpu/drm/nouveau/nvkm/engine/disp/dp.c > index 40c8ea43c42f..b8ac66b4a2c4 100644 > --- a/drivers/gpu/drm/nouveau/nvkm/engine/disp/dp.c > +++ b/drivers/gpu/drm/nouveau/nvkm/engine/disp/dp.c > @@ -26,6 +26,8 @@ > #include "head.h" > #include "ior.h" > > +#include <drm/display/drm_dp.h> > + > #include <subdev/bios.h> > #include <subdev/bios/init.h> > #include <subdev/gpio.h> > @@ -634,6 +636,50 @@ nvkm_dp_enable_supported_link_rates(struct nvkm_outp *outp) > return outp->dp.rates != 0; > } > > +/* XXX: This is a big fat hack, and this is just drm_dp_read_dpcd_caps() Well.. maybe we should rephrase that _if_ we want to see it backported. But is this code really that bad? It kinda looks reasonable enough. > + * converted to work inside nvkm. This is a temporary holdover until we start > + * passing the drm_dp_aux device through NVKM > + */ > +static int > +nvkm_dp_read_dpcd_caps(struct nvkm_outp *outp) > +{ > + struct nvkm_i2c_aux *aux = outp->dp.aux; > + u8 dpcd_ext[DP_RECEIVER_CAP_SIZE]; > + int ret; > + > + ret = nvkm_rdaux(aux, DPCD_RC00_DPCD_REV, outp->dp.dpcd, DP_RECEIVER_CAP_SIZE); > + if (ret < 0) > + return ret; > + > + /* > + * Prior to DP1.3 the bit represented by > + * DP_EXTENDED_RECEIVER_CAP_FIELD_PRESENT was reserved. > + * If it is set DP_DPCD_REV at 0000h could be at a value less than > + * the true capability of the panel. The only way to check is to > + * then compare 0000h and 2200h. > + */ > + if (!(outp->dp.dpcd[DP_TRAINING_AUX_RD_INTERVAL] & > + DP_EXTENDED_RECEIVER_CAP_FIELD_PRESENT)) > + return 0; > + > + ret = nvkm_rdaux(aux, DP_DP13_DPCD_REV, dpcd_ext, sizeof(dpcd_ext)); > + if (ret < 0) > + return ret; > + > + if (outp->dp.dpcd[DP_DPCD_REV] > dpcd_ext[DP_DPCD_REV]) { > + OUTP_DBG(outp, "Extended DPCD rev less than base DPCD rev (%d > %d)\n", > + outp->dp.dpcd[DP_DPCD_REV], dpcd_ext[DP_DPCD_REV]); > + return 0; > + } > + > + if (!memcmp(outp->dp.dpcd, dpcd_ext, sizeof(dpcd_ext))) > + return 0; > + > + memcpy(outp->dp.dpcd, dpcd_ext, sizeof(dpcd_ext)); > + > + return 0; > +} > + > void > nvkm_dp_enable(struct nvkm_outp *outp, bool auxpwr) > { > @@ -689,7 +735,7 @@ nvkm_dp_enable(struct nvkm_outp *outp, bool auxpwr) > memset(outp->dp.lttpr, 0x00, sizeof(outp->dp.lttpr)); > } > > - if (!nvkm_rdaux(aux, DPCD_RC00_DPCD_REV, outp->dp.dpcd, sizeof(outp->dp.dpcd))) { > + if (!nvkm_dp_read_dpcd_caps(outp)) { > const u8 rates[] = { 0x1e, 0x14, 0x0a, 0x06, 0 }; > const u8 *rate; > int rate_max; > -- > 2.40.1 >
On Sat, 8 Jul 2023 at 07:59, Lyude Paul <lyude@redhat.com> wrote: > > Currently we use the drm_dp_dpcd_read_caps() helper in the DRM side of > nouveau in order to read the DPCD of a DP connector, which makes sure we do > the right thing and also check for extended DPCD caps. However, it turns > out we're not currently doing this on the nvkm side since we don't have > access to the drm_dp_aux structure there - which means that the DRM side of > the driver and the NVKM side can end up with different DPCD capabilities > for the same connector. > > Ideally to fix this, we want to start setting up the drm_dp_aux device in > NVKM before we've made contact with the DRM side - which should be pretty > easy to accomplish (I'm already working on it!). Until then however, let's > workaround this problem by porting a copy of drm_dp_read_dpcd_caps() into > NVKM - which should fix this issue. I wouldn't worry about this. I'm moving basically everything to the DRM side of the driver for the GSP work anyway. Ben. > > Issue: https://gitlab.freedesktop.org/drm/nouveau/-/issues/211 > Signed-off-by: Lyude Paul <lyude@redhat.com> > --- > drivers/gpu/drm/nouveau/nvkm/engine/disp/dp.c | 48 ++++++++++++++++++- > 1 file changed, 47 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/nouveau/nvkm/engine/disp/dp.c b/drivers/gpu/drm/nouveau/nvkm/engine/disp/dp.c > index 40c8ea43c42f..b8ac66b4a2c4 100644 > --- a/drivers/gpu/drm/nouveau/nvkm/engine/disp/dp.c > +++ b/drivers/gpu/drm/nouveau/nvkm/engine/disp/dp.c > @@ -26,6 +26,8 @@ > #include "head.h" > #include "ior.h" > > +#include <drm/display/drm_dp.h> > + > #include <subdev/bios.h> > #include <subdev/bios/init.h> > #include <subdev/gpio.h> > @@ -634,6 +636,50 @@ nvkm_dp_enable_supported_link_rates(struct nvkm_outp *outp) > return outp->dp.rates != 0; > } > > +/* XXX: This is a big fat hack, and this is just drm_dp_read_dpcd_caps() > + * converted to work inside nvkm. This is a temporary holdover until we start > + * passing the drm_dp_aux device through NVKM > + */ > +static int > +nvkm_dp_read_dpcd_caps(struct nvkm_outp *outp) > +{ > + struct nvkm_i2c_aux *aux = outp->dp.aux; > + u8 dpcd_ext[DP_RECEIVER_CAP_SIZE]; > + int ret; > + > + ret = nvkm_rdaux(aux, DPCD_RC00_DPCD_REV, outp->dp.dpcd, DP_RECEIVER_CAP_SIZE); > + if (ret < 0) > + return ret; > + > + /* > + * Prior to DP1.3 the bit represented by > + * DP_EXTENDED_RECEIVER_CAP_FIELD_PRESENT was reserved. > + * If it is set DP_DPCD_REV at 0000h could be at a value less than > + * the true capability of the panel. The only way to check is to > + * then compare 0000h and 2200h. > + */ > + if (!(outp->dp.dpcd[DP_TRAINING_AUX_RD_INTERVAL] & > + DP_EXTENDED_RECEIVER_CAP_FIELD_PRESENT)) > + return 0; > + > + ret = nvkm_rdaux(aux, DP_DP13_DPCD_REV, dpcd_ext, sizeof(dpcd_ext)); > + if (ret < 0) > + return ret; > + > + if (outp->dp.dpcd[DP_DPCD_REV] > dpcd_ext[DP_DPCD_REV]) { > + OUTP_DBG(outp, "Extended DPCD rev less than base DPCD rev (%d > %d)\n", > + outp->dp.dpcd[DP_DPCD_REV], dpcd_ext[DP_DPCD_REV]); > + return 0; > + } > + > + if (!memcmp(outp->dp.dpcd, dpcd_ext, sizeof(dpcd_ext))) > + return 0; > + > + memcpy(outp->dp.dpcd, dpcd_ext, sizeof(dpcd_ext)); > + > + return 0; > +} > + > void > nvkm_dp_enable(struct nvkm_outp *outp, bool auxpwr) > { > @@ -689,7 +735,7 @@ nvkm_dp_enable(struct nvkm_outp *outp, bool auxpwr) > memset(outp->dp.lttpr, 0x00, sizeof(outp->dp.lttpr)); > } > > - if (!nvkm_rdaux(aux, DPCD_RC00_DPCD_REV, outp->dp.dpcd, sizeof(outp->dp.dpcd))) { > + if (!nvkm_dp_read_dpcd_caps(outp)) { > const u8 rates[] = { 0x1e, 0x14, 0x0a, 0x06, 0 }; > const u8 *rate; > int rate_max; > -- > 2.40.1 >
On Sun, 2023-07-09 at 01:42 +0200, Karol Herbst wrote: > On Fri, Jul 7, 2023 at 11:58 PM Lyude Paul <lyude@redhat.com> wrote: > > > > Currently we use the drm_dp_dpcd_read_caps() helper in the DRM side of > > nouveau in order to read the DPCD of a DP connector, which makes sure we do > > the right thing and also check for extended DPCD caps. However, it turns > > out we're not currently doing this on the nvkm side since we don't have > > access to the drm_dp_aux structure there - which means that the DRM side of > > the driver and the NVKM side can end up with different DPCD capabilities > > for the same connector. > > > > Ideally to fix this, we want to start setting up the drm_dp_aux device in > > NVKM before we've made contact with the DRM side - which should be pretty > > easy to accomplish (I'm already working on it!). Until then however, let's > > workaround this problem by porting a copy of drm_dp_read_dpcd_caps() into > > NVKM - which should fix this issue. > > > > Issue: https://gitlab.freedesktop.org/drm/nouveau/-/issues/211 > > Should a Fixes: or Cc: stable tag be added so it gets backported? JFYI I think not adding one is fine nowadays? The stable bot seems to be pretty good at catching anything with the words fix/fixes in it > > > Signed-off-by: Lyude Paul <lyude@redhat.com> > > --- > > drivers/gpu/drm/nouveau/nvkm/engine/disp/dp.c | 48 ++++++++++++++++++- > > 1 file changed, 47 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/nouveau/nvkm/engine/disp/dp.c b/drivers/gpu/drm/nouveau/nvkm/engine/disp/dp.c > > index 40c8ea43c42f..b8ac66b4a2c4 100644 > > --- a/drivers/gpu/drm/nouveau/nvkm/engine/disp/dp.c > > +++ b/drivers/gpu/drm/nouveau/nvkm/engine/disp/dp.c > > @@ -26,6 +26,8 @@ > > #include "head.h" > > #include "ior.h" > > > > +#include <drm/display/drm_dp.h> > > + > > #include <subdev/bios.h> > > #include <subdev/bios/init.h> > > #include <subdev/gpio.h> > > @@ -634,6 +636,50 @@ nvkm_dp_enable_supported_link_rates(struct nvkm_outp *outp) > > return outp->dp.rates != 0; > > } > > > > +/* XXX: This is a big fat hack, and this is just drm_dp_read_dpcd_caps() > > Well.. maybe we should rephrase that _if_ we want to see it > backported. But is this code really that bad? It kinda looks > reasonable enough. > > > + * converted to work inside nvkm. This is a temporary holdover until we start > > + * passing the drm_dp_aux device through NVKM > > + */ > > +static int > > +nvkm_dp_read_dpcd_caps(struct nvkm_outp *outp) > > +{ > > + struct nvkm_i2c_aux *aux = outp->dp.aux; > > + u8 dpcd_ext[DP_RECEIVER_CAP_SIZE]; > > + int ret; > > + > > + ret = nvkm_rdaux(aux, DPCD_RC00_DPCD_REV, outp->dp.dpcd, DP_RECEIVER_CAP_SIZE); > > + if (ret < 0) > > + return ret; > > + > > + /* > > + * Prior to DP1.3 the bit represented by > > + * DP_EXTENDED_RECEIVER_CAP_FIELD_PRESENT was reserved. > > + * If it is set DP_DPCD_REV at 0000h could be at a value less than > > + * the true capability of the panel. The only way to check is to > > + * then compare 0000h and 2200h. > > + */ > > + if (!(outp->dp.dpcd[DP_TRAINING_AUX_RD_INTERVAL] & > > + DP_EXTENDED_RECEIVER_CAP_FIELD_PRESENT)) > > + return 0; > > + > > + ret = nvkm_rdaux(aux, DP_DP13_DPCD_REV, dpcd_ext, sizeof(dpcd_ext)); > > + if (ret < 0) > > + return ret; > > + > > + if (outp->dp.dpcd[DP_DPCD_REV] > dpcd_ext[DP_DPCD_REV]) { > > + OUTP_DBG(outp, "Extended DPCD rev less than base DPCD rev (%d > %d)\n", > > + outp->dp.dpcd[DP_DPCD_REV], dpcd_ext[DP_DPCD_REV]); > > + return 0; > > + } > > + > > + if (!memcmp(outp->dp.dpcd, dpcd_ext, sizeof(dpcd_ext))) > > + return 0; > > + > > + memcpy(outp->dp.dpcd, dpcd_ext, sizeof(dpcd_ext)); > > + > > + return 0; > > +} > > + > > void > > nvkm_dp_enable(struct nvkm_outp *outp, bool auxpwr) > > { > > @@ -689,7 +735,7 @@ nvkm_dp_enable(struct nvkm_outp *outp, bool auxpwr) > > memset(outp->dp.lttpr, 0x00, sizeof(outp->dp.lttpr)); > > } > > > > - if (!nvkm_rdaux(aux, DPCD_RC00_DPCD_REV, outp->dp.dpcd, sizeof(outp->dp.dpcd))) { > > + if (!nvkm_dp_read_dpcd_caps(outp)) { > > const u8 rates[] = { 0x1e, 0x14, 0x0a, 0x06, 0 }; > > const u8 *rate; > > int rate_max; > > -- > > 2.40.1 > > >
On Thu, Jul 27, 2023 at 11:57 PM Lyude Paul <lyude@redhat.com> wrote: > > On Sun, 2023-07-09 at 01:42 +0200, Karol Herbst wrote: > > On Fri, Jul 7, 2023 at 11:58 PM Lyude Paul <lyude@redhat.com> wrote: > > > > > > Currently we use the drm_dp_dpcd_read_caps() helper in the DRM side of > > > nouveau in order to read the DPCD of a DP connector, which makes sure we do > > > the right thing and also check for extended DPCD caps. However, it turns > > > out we're not currently doing this on the nvkm side since we don't have > > > access to the drm_dp_aux structure there - which means that the DRM side of > > > the driver and the NVKM side can end up with different DPCD capabilities > > > for the same connector. > > > > > > Ideally to fix this, we want to start setting up the drm_dp_aux device in > > > NVKM before we've made contact with the DRM side - which should be pretty > > > easy to accomplish (I'm already working on it!). Until then however, let's > > > workaround this problem by porting a copy of drm_dp_read_dpcd_caps() into > > > NVKM - which should fix this issue. > > > > > > Issue: https://gitlab.freedesktop.org/drm/nouveau/-/issues/211 > > > > Should a Fixes: or Cc: stable tag be added so it gets backported? > > JFYI I think not adding one is fine nowadays? The stable bot seems to be > pretty good at catching anything with the words fix/fixes in it > Yeah not sure.. I'd rather be specific and add it just to be sure. Anyway, it could also be done while pushing. I think the bigger question here was if this fix is good enough for stable or if you plan to rework it. > > > > > Signed-off-by: Lyude Paul <lyude@redhat.com> > > > --- > > > drivers/gpu/drm/nouveau/nvkm/engine/disp/dp.c | 48 ++++++++++++++++++- > > > 1 file changed, 47 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/nouveau/nvkm/engine/disp/dp.c b/drivers/gpu/drm/nouveau/nvkm/engine/disp/dp.c > > > index 40c8ea43c42f..b8ac66b4a2c4 100644 > > > --- a/drivers/gpu/drm/nouveau/nvkm/engine/disp/dp.c > > > +++ b/drivers/gpu/drm/nouveau/nvkm/engine/disp/dp.c > > > @@ -26,6 +26,8 @@ > > > #include "head.h" > > > #include "ior.h" > > > > > > +#include <drm/display/drm_dp.h> > > > + > > > #include <subdev/bios.h> > > > #include <subdev/bios/init.h> > > > #include <subdev/gpio.h> > > > @@ -634,6 +636,50 @@ nvkm_dp_enable_supported_link_rates(struct nvkm_outp *outp) > > > return outp->dp.rates != 0; > > > } > > > > > > +/* XXX: This is a big fat hack, and this is just drm_dp_read_dpcd_caps() > > > > Well.. maybe we should rephrase that _if_ we want to see it > > backported. But is this code really that bad? It kinda looks > > reasonable enough. > > > > > + * converted to work inside nvkm. This is a temporary holdover until we start > > > + * passing the drm_dp_aux device through NVKM > > > + */ > > > +static int > > > +nvkm_dp_read_dpcd_caps(struct nvkm_outp *outp) > > > +{ > > > + struct nvkm_i2c_aux *aux = outp->dp.aux; > > > + u8 dpcd_ext[DP_RECEIVER_CAP_SIZE]; > > > + int ret; > > > + > > > + ret = nvkm_rdaux(aux, DPCD_RC00_DPCD_REV, outp->dp.dpcd, DP_RECEIVER_CAP_SIZE); > > > + if (ret < 0) > > > + return ret; > > > + > > > + /* > > > + * Prior to DP1.3 the bit represented by > > > + * DP_EXTENDED_RECEIVER_CAP_FIELD_PRESENT was reserved. > > > + * If it is set DP_DPCD_REV at 0000h could be at a value less than > > > + * the true capability of the panel. The only way to check is to > > > + * then compare 0000h and 2200h. > > > + */ > > > + if (!(outp->dp.dpcd[DP_TRAINING_AUX_RD_INTERVAL] & > > > + DP_EXTENDED_RECEIVER_CAP_FIELD_PRESENT)) > > > + return 0; > > > + > > > + ret = nvkm_rdaux(aux, DP_DP13_DPCD_REV, dpcd_ext, sizeof(dpcd_ext)); > > > + if (ret < 0) > > > + return ret; > > > + > > > + if (outp->dp.dpcd[DP_DPCD_REV] > dpcd_ext[DP_DPCD_REV]) { > > > + OUTP_DBG(outp, "Extended DPCD rev less than base DPCD rev (%d > %d)\n", > > > + outp->dp.dpcd[DP_DPCD_REV], dpcd_ext[DP_DPCD_REV]); > > > + return 0; > > > + } > > > + > > > + if (!memcmp(outp->dp.dpcd, dpcd_ext, sizeof(dpcd_ext))) > > > + return 0; > > > + > > > + memcpy(outp->dp.dpcd, dpcd_ext, sizeof(dpcd_ext)); > > > + > > > + return 0; > > > +} > > > + > > > void > > > nvkm_dp_enable(struct nvkm_outp *outp, bool auxpwr) > > > { > > > @@ -689,7 +735,7 @@ nvkm_dp_enable(struct nvkm_outp *outp, bool auxpwr) > > > memset(outp->dp.lttpr, 0x00, sizeof(outp->dp.lttpr)); > > > } > > > > > > - if (!nvkm_rdaux(aux, DPCD_RC00_DPCD_REV, outp->dp.dpcd, sizeof(outp->dp.dpcd))) { > > > + if (!nvkm_dp_read_dpcd_caps(outp)) { > > > const u8 rates[] = { 0x1e, 0x14, 0x0a, 0x06, 0 }; > > > const u8 *rate; > > > int rate_max; > > > -- > > > 2.40.1 > > > > > > > -- > Cheers, > Lyude Paul (she/her) > Software Engineer at Red Hat >
On Fri, Jul 28, 2023 at 1:11 AM Karol Herbst <kherbst@redhat.com> wrote: > > On Thu, Jul 27, 2023 at 11:57 PM Lyude Paul <lyude@redhat.com> wrote: > > > > On Sun, 2023-07-09 at 01:42 +0200, Karol Herbst wrote: > > > On Fri, Jul 7, 2023 at 11:58 PM Lyude Paul <lyude@redhat.com> wrote: > > > > > > > > Currently we use the drm_dp_dpcd_read_caps() helper in the DRM side of > > > > nouveau in order to read the DPCD of a DP connector, which makes sure we do > > > > the right thing and also check for extended DPCD caps. However, it turns > > > > out we're not currently doing this on the nvkm side since we don't have > > > > access to the drm_dp_aux structure there - which means that the DRM side of > > > > the driver and the NVKM side can end up with different DPCD capabilities > > > > for the same connector. > > > > > > > > Ideally to fix this, we want to start setting up the drm_dp_aux device in > > > > NVKM before we've made contact with the DRM side - which should be pretty > > > > easy to accomplish (I'm already working on it!). Until then however, let's > > > > workaround this problem by porting a copy of drm_dp_read_dpcd_caps() into > > > > NVKM - which should fix this issue. > > > > > > > > Issue: https://gitlab.freedesktop.org/drm/nouveau/-/issues/211 > > > > > > Should a Fixes: or Cc: stable tag be added so it gets backported? > > > > JFYI I think not adding one is fine nowadays? The stable bot seems to be > > pretty good at catching anything with the words fix/fixes in it > > > > Yeah not sure.. I'd rather be specific and add it just to be sure. > Anyway, it could also be done while pushing. I think the bigger > question here was if this fix is good enough for stable or if you plan > to rework it. > > > > > > > > Signed-off-by: Lyude Paul <lyude@redhat.com> > > > > --- > > > > drivers/gpu/drm/nouveau/nvkm/engine/disp/dp.c | 48 ++++++++++++++++++- > > > > 1 file changed, 47 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/gpu/drm/nouveau/nvkm/engine/disp/dp.c b/drivers/gpu/drm/nouveau/nvkm/engine/disp/dp.c > > > > index 40c8ea43c42f..b8ac66b4a2c4 100644 > > > > --- a/drivers/gpu/drm/nouveau/nvkm/engine/disp/dp.c > > > > +++ b/drivers/gpu/drm/nouveau/nvkm/engine/disp/dp.c > > > > @@ -26,6 +26,8 @@ > > > > #include "head.h" > > > > #include "ior.h" > > > > > > > > +#include <drm/display/drm_dp.h> > > > > + > > > > #include <subdev/bios.h> > > > > #include <subdev/bios/init.h> > > > > #include <subdev/gpio.h> > > > > @@ -634,6 +636,50 @@ nvkm_dp_enable_supported_link_rates(struct nvkm_outp *outp) > > > > return outp->dp.rates != 0; > > > > } > > > > > > > > +/* XXX: This is a big fat hack, and this is just drm_dp_read_dpcd_caps() > > > > > > Well.. maybe we should rephrase that _if_ we want to see it > > > backported. But is this code really that bad? It kinda looks > > > reasonable enough. > > > > > > > + * converted to work inside nvkm. This is a temporary holdover until we start > > > > + * passing the drm_dp_aux device through NVKM > > > > + */ > > > > +static int > > > > +nvkm_dp_read_dpcd_caps(struct nvkm_outp *outp) > > > > +{ > > > > + struct nvkm_i2c_aux *aux = outp->dp.aux; > > > > + u8 dpcd_ext[DP_RECEIVER_CAP_SIZE]; > > > > + int ret; > > > > + > > > > + ret = nvkm_rdaux(aux, DPCD_RC00_DPCD_REV, outp->dp.dpcd, DP_RECEIVER_CAP_SIZE); > > > > + if (ret < 0) > > > > + return ret; > > > > + > > > > + /* > > > > + * Prior to DP1.3 the bit represented by > > > > + * DP_EXTENDED_RECEIVER_CAP_FIELD_PRESENT was reserved. > > > > + * If it is set DP_DPCD_REV at 0000h could be at a value less than > > > > + * the true capability of the panel. The only way to check is to > > > > + * then compare 0000h and 2200h. > > > > + */ > > > > + if (!(outp->dp.dpcd[DP_TRAINING_AUX_RD_INTERVAL] & > > > > + DP_EXTENDED_RECEIVER_CAP_FIELD_PRESENT)) > > > > + return 0; > > > > + > > > > + ret = nvkm_rdaux(aux, DP_DP13_DPCD_REV, dpcd_ext, sizeof(dpcd_ext)); > > > > + if (ret < 0) > > > > + return ret; > > > > + > > > > + if (outp->dp.dpcd[DP_DPCD_REV] > dpcd_ext[DP_DPCD_REV]) { > > > > + OUTP_DBG(outp, "Extended DPCD rev less than base DPCD rev (%d > %d)\n", > > > > + outp->dp.dpcd[DP_DPCD_REV], dpcd_ext[DP_DPCD_REV]); > > > > + return 0; > > > > + } > > > > + > > > > + if (!memcmp(outp->dp.dpcd, dpcd_ext, sizeof(dpcd_ext))) > > > > + return 0; > > > > + > > > > + memcpy(outp->dp.dpcd, dpcd_ext, sizeof(dpcd_ext)); > > > > + > > > > + return 0; > > > > +} > > > > + > > > > void > > > > nvkm_dp_enable(struct nvkm_outp *outp, bool auxpwr) > > > > { > > > > @@ -689,7 +735,7 @@ nvkm_dp_enable(struct nvkm_outp *outp, bool auxpwr) > > > > memset(outp->dp.lttpr, 0x00, sizeof(outp->dp.lttpr)); > > > > } > > > > > > > > - if (!nvkm_rdaux(aux, DPCD_RC00_DPCD_REV, outp->dp.dpcd, sizeof(outp->dp.dpcd))) { > > > > + if (!nvkm_dp_read_dpcd_caps(outp)) { > > > > const u8 rates[] = { 0x1e, 0x14, 0x0a, 0x06, 0 }; > > > > const u8 *rate; > > > > int rate_max; > > > > -- > > > > 2.40.1 > > > > > > > > > > > -- > > Cheers, > > Lyude Paul (she/her) > > Software Engineer at Red Hat > > before I forget: Reviewed-by: Karol Herbst <kherbst@redhat.com>
diff --git a/drivers/gpu/drm/nouveau/nvkm/engine/disp/dp.c b/drivers/gpu/drm/nouveau/nvkm/engine/disp/dp.c index 40c8ea43c42f..b8ac66b4a2c4 100644 --- a/drivers/gpu/drm/nouveau/nvkm/engine/disp/dp.c +++ b/drivers/gpu/drm/nouveau/nvkm/engine/disp/dp.c @@ -26,6 +26,8 @@ #include "head.h" #include "ior.h" +#include <drm/display/drm_dp.h> + #include <subdev/bios.h> #include <subdev/bios/init.h> #include <subdev/gpio.h> @@ -634,6 +636,50 @@ nvkm_dp_enable_supported_link_rates(struct nvkm_outp *outp) return outp->dp.rates != 0; } +/* XXX: This is a big fat hack, and this is just drm_dp_read_dpcd_caps() + * converted to work inside nvkm. This is a temporary holdover until we start + * passing the drm_dp_aux device through NVKM + */ +static int +nvkm_dp_read_dpcd_caps(struct nvkm_outp *outp) +{ + struct nvkm_i2c_aux *aux = outp->dp.aux; + u8 dpcd_ext[DP_RECEIVER_CAP_SIZE]; + int ret; + + ret = nvkm_rdaux(aux, DPCD_RC00_DPCD_REV, outp->dp.dpcd, DP_RECEIVER_CAP_SIZE); + if (ret < 0) + return ret; + + /* + * Prior to DP1.3 the bit represented by + * DP_EXTENDED_RECEIVER_CAP_FIELD_PRESENT was reserved. + * If it is set DP_DPCD_REV at 0000h could be at a value less than + * the true capability of the panel. The only way to check is to + * then compare 0000h and 2200h. + */ + if (!(outp->dp.dpcd[DP_TRAINING_AUX_RD_INTERVAL] & + DP_EXTENDED_RECEIVER_CAP_FIELD_PRESENT)) + return 0; + + ret = nvkm_rdaux(aux, DP_DP13_DPCD_REV, dpcd_ext, sizeof(dpcd_ext)); + if (ret < 0) + return ret; + + if (outp->dp.dpcd[DP_DPCD_REV] > dpcd_ext[DP_DPCD_REV]) { + OUTP_DBG(outp, "Extended DPCD rev less than base DPCD rev (%d > %d)\n", + outp->dp.dpcd[DP_DPCD_REV], dpcd_ext[DP_DPCD_REV]); + return 0; + } + + if (!memcmp(outp->dp.dpcd, dpcd_ext, sizeof(dpcd_ext))) + return 0; + + memcpy(outp->dp.dpcd, dpcd_ext, sizeof(dpcd_ext)); + + return 0; +} + void nvkm_dp_enable(struct nvkm_outp *outp, bool auxpwr) { @@ -689,7 +735,7 @@ nvkm_dp_enable(struct nvkm_outp *outp, bool auxpwr) memset(outp->dp.lttpr, 0x00, sizeof(outp->dp.lttpr)); } - if (!nvkm_rdaux(aux, DPCD_RC00_DPCD_REV, outp->dp.dpcd, sizeof(outp->dp.dpcd))) { + if (!nvkm_dp_read_dpcd_caps(outp)) { const u8 rates[] = { 0x1e, 0x14, 0x0a, 0x06, 0 }; const u8 *rate; int rate_max;