On Fri, Jul 28, 2023 at 1:11 AM Karol Herbst <kher...@redhat.com> wrote: > > On Thu, Jul 27, 2023 at 11:57 PM Lyude Paul <ly...@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 <ly...@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 <ly...@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 <kher...@redhat.com>
