mbox series

[RFC,00/20] drm/dp, i915, nouveau: Cleanup nouveau HPD and add DP features from i915

Message ID 20200811200457.134743-1-lyude@redhat.com (mailing list archive)
Headers show
Series drm/dp, i915, nouveau: Cleanup nouveau HPD and add DP features from i915 | expand

Message

Lyude Paul Aug. 11, 2020, 8:04 p.m. UTC
To start off: this patch series is less work to review then it looks -
most (but not all) of the nouveau related work has already been reviewed
elsewhere. Most of the reason I'm asking for an RFC here is because this
code pulls a lot of code out of i915 and into shared DP helpers.

Anyway-nouveau's HPD related code has been collecting dust for a while.
Other then the occasional runtime PM related and MST related fixes,
we're missing a lot of nice things that have been added to DRM since
this was originally written. Additionally, the code is just really
unoptimized in general:

* We handle connector probing in the same context that we handle short
  IRQs in for DP, which means connector probing could potentially block
  ESI handling for MST
* When we receive a hotplug event from a connector, we reprobe every
  single connector instead of just the connector that was hotplugged
* Additionally because of the above reason, combined with the fact I had
  the bad idea of reusing some of the MST locks when I last rewrote
  nouveau's DP MST detection, trying to handle any other events that
  require short HPD IRQs is a bit awkward to actually implement.
* We don't actually properly check whether EDIDs change or not when
  reprobing, which means we basically send out a hotplug event every
  single time we receive one even if nothing has changed

Additionally, the code for handling DP that we have in nouveau is also
quite unoptimized in general, doesn't use a lot of helpers that have
been added since it was written, and is also missing quite a number of
features:

* Downstream port capability probing
* Extended DPRX cap parsing
* SINK_COUNT handling for hpd on dongles

Luckily for us - all of these are implemented in i915 already. Since
there's no reason for us to reinvent the wheel, and having more shared
code is always nice, I decided to take the opportunity to extract the
code for all of these features from i915 into a set of core DP helpers,
which both i915 and nouveau (and hopefully other drivers in the future)
can use.

As well, this patch series also addesses the other general
connector probing related issues I mentioned above, along with rewriting
how we handle MST probing so we don't hit any surprise locking design
issues in the future.

As a note - most of this work is motivated by the fact that I'm
planning on adding max_bpc/output_bpc prop support, DSC support (for
both MST and SST, along with proper helpers for handling bandwidth
limitations and DSC), and fallback link retraining. I figured I might as
clean this code up and implement missing DP features like the ones
mentioned here before moving on to those tasks.

Also, I'm intending for this patch series to get merged through
drm-misc-next. Once that happens, upstream maintainers will probably
have an easier time with merging if they pull the pending patches for
nouveau from Ben's tree before merging drm-misc-next.

Lyude Paul (20):
  drm/nouveau/kms: Fix some indenting in nouveau_dp_detect()
  drm/nouveau/kms/nv50-: Remove open-coded drm_dp_read_desc()
  drm/nouveau/kms/nv50-: Just use drm_dp_dpcd_read() in nouveau_dp.c
  drm/nouveau/kms/nv50-: Use macros for DP registers in nouveau_dp.c
  drm/nouveau/kms: Don't clear DP_MST_CTRL DPCD in nv50_mstm_new()
  drm/nouveau/kms: Search for encoders' connectors properly
  drm/nouveau/kms/nv50-: Use drm_dp_dpcd_(readb|writeb)() in
    nv50_sor_disable()
  drm/nouveau/kms/nv50-: Refactor and cleanup DP HPD handling
  drm/i915/dp: Extract drm_dp_has_mst()
  drm/nouveau/kms: Use new drm_dp_has_mst() helper for checking MST caps
  drm/nouveau/kms: Move drm_dp_cec_unset_edid() into
    nouveau_connector_detect()
  drm/nouveau/kms: Only use hpd_work for reprobing in HPD paths
  drm/i915/dp: Extract drm_dp_downstream_read_info()
  drm/nouveau/kms/nv50-: Use downstream DP clock limits for mode
    validation
  drm/i915/dp: Extract drm_dp_has_sink_count()
  drm/i915/dp: Extract drm_dp_get_sink_count()
  drm/nouveau/kms/nv50-: Add support for DP_SINK_COUNT
  drm/nouveau/kms: Don't change EDID when it hasn't actually changed
  drm/i915/dp: Extract drm_dp_read_dpcd_caps()
  drm/nouveau/kms: Start using drm_dp_read_dpcd_caps()

 drivers/gpu/drm/drm_dp_helper.c             | 150 ++++++++++
 drivers/gpu/drm/i915/display/intel_dp.c     | 130 ++-------
 drivers/gpu/drm/i915/display/intel_dp.h     |   1 -
 drivers/gpu/drm/i915/display/intel_lspcon.c |   2 +-
 drivers/gpu/drm/nouveau/dispnv04/dac.c      |   2 +-
 drivers/gpu/drm/nouveau/dispnv04/dfp.c      |   7 +-
 drivers/gpu/drm/nouveau/dispnv04/disp.c     |  24 +-
 drivers/gpu/drm/nouveau/dispnv04/disp.h     |   4 +
 drivers/gpu/drm/nouveau/dispnv04/tvnv04.c   |   2 +-
 drivers/gpu/drm/nouveau/dispnv04/tvnv17.c   |   2 +-
 drivers/gpu/drm/nouveau/dispnv50/disp.c     | 308 +++++++++++---------
 drivers/gpu/drm/nouveau/nouveau_connector.c | 132 ++++-----
 drivers/gpu/drm/nouveau/nouveau_connector.h |   1 +
 drivers/gpu/drm/nouveau/nouveau_display.c   |  72 ++++-
 drivers/gpu/drm/nouveau/nouveau_display.h   |   3 +-
 drivers/gpu/drm/nouveau/nouveau_dp.c        | 201 ++++++++++---
 drivers/gpu/drm/nouveau/nouveau_drm.c       |   4 +-
 drivers/gpu/drm/nouveau/nouveau_drv.h       |   2 +
 drivers/gpu/drm/nouveau/nouveau_encoder.h   |  48 ++-
 include/drm/drm_dp_helper.h                 |  15 +-
 include/drm/drm_dp_mst_helper.h             |  22 ++
 21 files changed, 741 insertions(+), 391 deletions(-)

Comments

Lyude Paul Aug. 11, 2020, 10:17 p.m. UTC | #1
Can someone take a look at this? All of the CI related links here just give a
permission denied when you try to view them, and I'm positive this test
failure has nothing to do with this series
On Tue, 2020-08-11 at 22:13 +0000, Patchwork wrote:
> 
> Patch Details
> 
> Series:drm/dp, i915, nouveau: Cleanup nouveau HPD and add DP features from
> i915 (rev2)
> URL:https://patchwork.freedesktop.org/series/80542/
> State:failure
> 
>     Details:
> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18347/index.html
> 
> 
> 
> 
>     CI Bug Log - changes from CI_DRM_8872 -> Patchwork_18347
> Summary
> FAILURE
> Serious unknown changes coming with Patchwork_18347 absolutely need to be
> 
>   verified manually.
> If you think the reported changes have nothing to do with the changes
> 
>   introduced in Patchwork_18347, please notify your bug team to allow them
> 
>   to document this new failure mode, which will reduce false positives in
> CI.
> External URL: 
> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18347/index.html
> Possible new issues
> Here are the unknown changes that may have been introduced in
> Patchwork_18347:
> IGT changes
> Possible regressions
> 
> igt@runner@aborted:
> fi-skl-6700k2:      NOTRUN -> FAIL
> 
> 
> 
> Known issues
> Here are the changes found in Patchwork_18347 that come from known issues:
> IGT changes
> Issues hit
> 
> 
> igt@i915_module_load@reload:
> 
> fi-byt-j1900:       PASS -> DMESG-WARN (i915#1982)
> 
> 
> 
> igt@i915_selftest@live@execlists:
> 
> fi-icl-y:           PASS -> INCOMPLETE (i915#2276)
> 
> 
> 
> igt@kms_flip@basic-flip-vs-wf_vblank@b-edp1:
> 
> fi-icl-u2:          PASS -> DMESG-WARN (i915#1982) +1 similar issue
> 
> 
> 
> Possible fixes
> 
> 
> igt@i915_module_load@reload:
> 
> 
> fi-apl-guc:         DMESG-WARN (i915#1635 / i915#1982) -> PASS
> 
> 
> fi-cml-s:           DMESG-WARN (i915#1982) -> PASS
> 
> 
> fi-kbl-x1275:       DMESG-WARN (i915#62 / i915#92) -> PASS
> 
> 
> 
> 
> igt@kms_busy@basic@flip:
> 
> 
> {fi-tgl-dsi}:       DMESG-WARN (i915#1982) -> PASS
> 
> 
> fi-kbl-x1275:       DMESG-WARN (i915#62 / i915#92 / i915#95) -> PASS
> 
> 
> 
> 
> Warnings
> 
> 
> igt@i915_pm_rpm@module-reload:
> 
> fi-kbl-x1275:       SKIP (fdo#109271) -> DMESG-FAIL (i915#62)
> 
> 
> 
> igt@kms_force_connector_basic@force-edid:
> 
> fi-kbl-x1275:       DMESG-WARN (i915#62 / i915#92) -> DMESG-WARN (i915#62 /
> i915#92 / i915#95) +1 similar issue
> 
> 
> 
> {name}: This element is suppressed. This means it is ignored when computing
> 
>           the status of the difference (SUCCESS, WARNING, or FAILURE).
> Participating hosts (42 -> 37)
> Additional (1): fi-kbl-soraka 
> 
>   Missing    (6): fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-
> byt-clapper fi-bdw-samus 
> Build changes
> 
> Linux: CI_DRM_8872 -> Patchwork_18347
> 
> CI-20190529: 20190529
> 
>   CI_DRM_8872: 494f4611d8ee77b49fec39886b8b97c14f291f18 @
> git://anongit.freedesktop.org/gfx-ci/linux
> 
>   IGT_5767: 39e9aa1032a4e60f776f34b3ccf4fb728abbfe5c @
> git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
> 
>   Patchwork_18347: ed9c2cbd0615944411c1dcfa8383a8174b1f215f @
> git://anongit.freedesktop.org/gfx-ci/linux
> == Linux commits ==
> ed9c2cbd0615 drm/nouveau/kms: Start using drm_dp_read_dpcd_caps()
> 
> dbc6ce6444f9 drm/i915/dp: Extract drm_dp_read_dpcd_caps()
> 
> b2b88eaa8158 drm/nouveau/kms: Don't change EDID when it hasn't actually
> changed
> 
> c726b1b06424 drm/nouveau/kms/nv50-: Add support for DP_SINK_COUNT
> 
> dd59fa4b47de drm/i915/dp: Extract drm_dp_get_sink_count()
> 
> e308120f0a75 drm/i915/dp: Extract drm_dp_has_sink_count()
> 
> 7ebbddd8fe5f drm/nouveau/kms/nv50-: Use downstream DP clock limits for mode
> validation
> 
> 43964c882505 drm/i915/dp: Extract drm_dp_downstream_read_info()
> 
> 00e33908c535 drm/nouveau/kms: Only use hpd_work for reprobing in HPD paths
> 
> 67b2ac39f22b drm/nouveau/kms: Move drm_dp_cec_unset_edid() into
> nouveau_connector_detect()
> 
> 1616b919e4b2 drm/nouveau/kms: Use new drm_dp_has_mst() helper for checking
> MST caps
> 
> 8b6671700abf drm/i915/dp: Extract drm_dp_has_mst()
> 
> 2aa54c3a5de3 drm/nouveau/kms/nv50-: Refactor and cleanup DP HPD handling
> 
> b197d002e32f drm/nouveau/kms/nv50-: Use drm_dp_dpcd_(readb|writeb)() in
> nv50_sor_disable()
> 
> 7cb0374b4558 drm/nouveau/kms: Search for encoders' connectors properly
> 
> eb430d7e3dc4 drm/nouveau/kms: Don't clear DP_MST_CTRL DPCD in
> nv50_mstm_new()
> 
> bd4724757ea9 drm/nouveau/kms/nv50-: Use macros for DP registers in
> nouveau_dp.c
> 
> 478aa516d10c drm/nouveau/kms/nv50-: Just use drm_dp_dpcd_read() in
> nouveau_dp.c
> 
> d6d94517b732 drm/nouveau/kms/nv50-: Remove open-coded drm_dp_read_desc()
> 
> 7429fbed7536 drm/nouveau/kms: Fix some indenting in nouveau_dp_detect()
> 
> 
>
Lyude Paul Aug. 11, 2020, 10:44 p.m. UTC | #2
Ah-nevermind, Ickle pointed out to me that it just takes a little bit for the
results to migrate over to AWS, things seem to be viewable now.
On Tue, 2020-08-11 at 18:17 -0400, Lyude Paul wrote:
> Can someone take a look at this? All of the CI related links here just give
> a permission denied when you try to view them, and I'm positive this test
> failure has nothing to do with this series
> On Tue, 2020-08-11 at 22:13 +0000, Patchwork wrote:
> > 
> > Patch Details
> > 
> > Series:drm/dp, i915, nouveau: Cleanup nouveau HPD and add DP features from
> > i915 (rev2)
> > URL:https://patchwork.freedesktop.org/series/80542/
> > State:failure
> > 
> >     Details:
> > https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18347/index.html
> > 
> > 
> > 
> > 
> >     CI Bug Log - changes from CI_DRM_8872 -> Patchwork_18347
> > Summary
> > FAILURE
> > Serious unknown changes coming with Patchwork_18347 absolutely need to be
> > 
> >   verified manually.
> > If you think the reported changes have nothing to do with the changes
> > 
> >   introduced in Patchwork_18347, please notify your bug team to allow them
> > 
> >   to document this new failure mode, which will reduce false positives in
> > CI.
> > External URL: 
> > https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18347/index.html
> > Possible new issues
> > Here are the unknown changes that may have been introduced in
> > Patchwork_18347:
> > IGT changes
> > Possible regressions
> > 
> > igt@runner@aborted:
> > fi-skl-6700k2:      NOTRUN -> FAIL
> > 
> > 
> > 
> > Known issues
> > Here are the changes found in Patchwork_18347 that come from known issues:
> > IGT changes
> > Issues hit
> > 
> > 
> > igt@i915_module_load@reload:
> > 
> > fi-byt-j1900:       PASS -> DMESG-WARN (i915#1982)
> > 
> > 
> > 
> > igt@i915_selftest@live@execlists:
> > 
> > fi-icl-y:           PASS -> INCOMPLETE (i915#2276)
> > 
> > 
> > 
> > igt@kms_flip@basic-flip-vs-wf_vblank@b-edp1:
> > 
> > fi-icl-u2:          PASS -> DMESG-WARN (i915#1982) +1 similar issue
> > 
> > 
> > 
> > Possible fixes
> > 
> > 
> > igt@i915_module_load@reload:
> > 
> > 
> > fi-apl-guc:         DMESG-WARN (i915#1635 / i915#1982) -> PASS
> > 
> > 
> > fi-cml-s:           DMESG-WARN (i915#1982) -> PASS
> > 
> > 
> > fi-kbl-x1275:       DMESG-WARN (i915#62 / i915#92) -> PASS
> > 
> > 
> > 
> > 
> > igt@kms_busy@basic@flip:
> > 
> > 
> > {fi-tgl-dsi}:       DMESG-WARN (i915#1982) -> PASS
> > 
> > 
> > fi-kbl-x1275:       DMESG-WARN (i915#62 / i915#92 / i915#95) -> PASS
> > 
> > 
> > 
> > 
> > Warnings
> > 
> > 
> > igt@i915_pm_rpm@module-reload:
> > 
> > fi-kbl-x1275:       SKIP (fdo#109271) -> DMESG-FAIL (i915#62)
> > 
> > 
> > 
> > igt@kms_force_connector_basic@force-edid:
> > 
> > fi-kbl-x1275:       DMESG-WARN (i915#62 / i915#92) -> DMESG-WARN (i915#62
> > / i915#92 / i915#95) +1 similar issue
> > 
> > 
> > 
> > {name}: This element is suppressed. This means it is ignored when
> > computing
> > 
> >           the status of the difference (SUCCESS, WARNING, or FAILURE).
> > Participating hosts (42 -> 37)
> > Additional (1): fi-kbl-soraka 
> > 
> >   Missing    (6): fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-
> > byt-clapper fi-bdw-samus 
> > Build changes
> > 
> > Linux: CI_DRM_8872 -> Patchwork_18347
> > 
> > CI-20190529: 20190529
> > 
> >   CI_DRM_8872: 494f4611d8ee77b49fec39886b8b97c14f291f18 @
> > git://anongit.freedesktop.org/gfx-ci/linux
> > 
> >   IGT_5767: 39e9aa1032a4e60f776f34b3ccf4fb728abbfe5c @
> > git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
> > 
> >   Patchwork_18347: ed9c2cbd0615944411c1dcfa8383a8174b1f215f @
> > git://anongit.freedesktop.org/gfx-ci/linux
> > == Linux commits ==
> > ed9c2cbd0615 drm/nouveau/kms: Start using drm_dp_read_dpcd_caps()
> > 
> > dbc6ce6444f9 drm/i915/dp: Extract drm_dp_read_dpcd_caps()
> > 
> > b2b88eaa8158 drm/nouveau/kms: Don't change EDID when it hasn't actually
> > changed
> > 
> > c726b1b06424 drm/nouveau/kms/nv50-: Add support for DP_SINK_COUNT
> > 
> > dd59fa4b47de drm/i915/dp: Extract drm_dp_get_sink_count()
> > 
> > e308120f0a75 drm/i915/dp: Extract drm_dp_has_sink_count()
> > 
> > 7ebbddd8fe5f drm/nouveau/kms/nv50-: Use downstream DP clock limits for
> > mode validation
> > 
> > 43964c882505 drm/i915/dp: Extract drm_dp_downstream_read_info()
> > 
> > 00e33908c535 drm/nouveau/kms: Only use hpd_work for reprobing in HPD paths
> > 
> > 67b2ac39f22b drm/nouveau/kms: Move drm_dp_cec_unset_edid() into
> > nouveau_connector_detect()
> > 
> > 1616b919e4b2 drm/nouveau/kms: Use new drm_dp_has_mst() helper for checking
> > MST caps
> > 
> > 8b6671700abf drm/i915/dp: Extract drm_dp_has_mst()
> > 
> > 2aa54c3a5de3 drm/nouveau/kms/nv50-: Refactor and cleanup DP HPD handling
> > 
> > b197d002e32f drm/nouveau/kms/nv50-: Use drm_dp_dpcd_(readb|writeb)() in
> > nv50_sor_disable()
> > 
> > 7cb0374b4558 drm/nouveau/kms: Search for encoders' connectors properly
> > 
> > eb430d7e3dc4 drm/nouveau/kms: Don't clear DP_MST_CTRL DPCD in
> > nv50_mstm_new()
> > 
> > bd4724757ea9 drm/nouveau/kms/nv50-: Use macros for DP registers in
> > nouveau_dp.c
> > 
> > 478aa516d10c drm/nouveau/kms/nv50-: Just use drm_dp_dpcd_read() in
> > nouveau_dp.c
> > 
> > d6d94517b732 drm/nouveau/kms/nv50-: Remove open-coded drm_dp_read_desc()
> > 
> > 7429fbed7536 drm/nouveau/kms: Fix some indenting in nouveau_dp_detect()
> > 
> > 
> > 
> -- 
> Cheers,
> 	Lyude Paul (she/her)
> 	Software Engineer at Red Hat