Noticed this while going through our DP code - we use an open-coded version of drm_dp_read_desc() instead of just using the helper, so change that. This will also let us use quirks in the future if we end up needing them.

Signed-off-by: Lyude Paul lyude@redhat.com Reviewed-by: Ben Skeggs bskeggs@redhat.com --- drivers/gpu/drm/nouveau/nouveau_connector.c | 3 ++- drivers/gpu/drm/nouveau/nouveau_dp.c | 30 +++++++-------------- drivers/gpu/drm/nouveau/nouveau_encoder.h | 4 ++- 3 files changed, 14 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nouveau_connector.c b/drivers/gpu/drm/nouveau/nouveau_connector.c index 7674025a4bfe8..e12957e6faa7c 100644 --- a/drivers/gpu/drm/nouveau/nouveau_connector.c +++ b/drivers/gpu/drm/nouveau/nouveau_connector.c @@ -435,7 +435,8 @@ nouveau_connector_ddc_detect(struct drm_connector *connector)

switch (nv_encoder->dcb->type) { case DCB_OUTPUT_DP: - ret = nouveau_dp_detect(nv_encoder); + ret = nouveau_dp_detect(nouveau_connector(connector), + nv_encoder); if (ret == NOUVEAU_DP_MST) return NULL; else if (ret == NOUVEAU_DP_SST) diff --git a/drivers/gpu/drm/nouveau/nouveau_dp.c b/drivers/gpu/drm/nouveau/nouveau_dp.c index ee778ddc95fae..c4e9c21d4dd2b 100644 --- a/drivers/gpu/drm/nouveau/nouveau_dp.c +++ b/drivers/gpu/drm/nouveau/nouveau_dp.c @@ -36,27 +36,9 @@ MODULE_PARM_DESC(mst, "Enable DisplayPort multi-stream (default: enabled)"); static int nouveau_mst = 1; module_param_named(mst, nouveau_mst, int, 0400);

-static void -nouveau_dp_probe_oui(struct drm_device *dev, struct nvkm_i2c_aux *aux, u8 *dpcd) -{ - struct nouveau_drm *drm = nouveau_drm(dev); - u8 buf[3]; - - if (!(dpcd[DP_DOWN_STREAM_PORT_COUNT] & DP_OUI_SUPPORT)) - return; - - if (!nvkm_rdaux(aux, DP_SINK_OUI, buf, 3)) - NV_DEBUG(drm, "Sink OUI: %02hx%02hx%02hx\n", - buf[0], buf[1], buf[2]); - - if (!nvkm_rdaux(aux, DP_BRANCH_OUI, buf, 3)) - NV_DEBUG(drm, "Branch OUI: %02hx%02hx%02hx\n", - buf[0], buf[1], buf[2]); - -} - int -nouveau_dp_detect(struct nouveau_encoder *nv_encoder) +nouveau_dp_detect(struct nouveau_connector *nv_connector, + struct nouveau_encoder *nv_encoder) { struct drm_device *dev = nv_encoder->base.base.dev; struct nouveau_drm *drm = nouveau_drm(dev); @@ -89,7 +71,13 @@ nouveau_dp_detect(struct nouveau_encoder *nv_encoder) NV_DEBUG(drm, "maximum: %dx%d\n", nv_encoder->dp.link_nr, nv_encoder->dp.link_bw);

- nouveau_dp_probe_oui(dev, aux, dpcd); + ret = drm_dp_read_desc(&nv_connector->aux, &nv_encoder->dp.desc, + drm_dp_is_branch(dpcd)); + if (ret) { + NV_ERROR(drm, "Failed to read DP descriptor on %s: %d\n", + nv_connector->base.name, ret); + return ret; + }

ret = nv50_mstm_detect(nv_encoder->dp.mstm, dpcd, nouveau_mst); if (ret == 1) diff --git a/drivers/gpu/drm/nouveau/nouveau_encoder.h b/drivers/gpu/drm/nouveau/nouveau_encoder.h index a72c412ac8b14..6424cdcb4913f 100644 --- a/drivers/gpu/drm/nouveau/nouveau_encoder.h +++ b/drivers/gpu/drm/nouveau/nouveau_encoder.h @@ -33,6 +33,7 @@ #include <drm/drm_dp_mst_helper.h> #include "dispnv04/disp.h" struct nv50_head_atom; +struct nouveau_connector;

#define NV_DPMS_CLEARED 0x80

@@ -64,6 +65,7 @@ struct nouveau_encoder { struct nv50_mstm *mstm; int link_nr; int link_bw; + struct drm_dp_desc desc; } dp; };

@@ -104,7 +106,7 @@ enum nouveau_dp_status { NOUVEAU_DP_MST, };

-int nouveau_dp_detect(struct nouveau_encoder *); +int nouveau_dp_detect(struct nouveau_connector *, struct nouveau_encoder *); enum drm_mode_status nv50_dp_mode_valid(struct drm_connector *, struct nouveau_encoder *, const struct drm_display_mode *,

No functional changes.

Signed-off-by: Lyude Paul lyude@redhat.com Reviewed-by: Ben Skeggs bskeggs@redhat.com --- drivers/gpu/drm/nouveau/nouveau_dp.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nouveau_dp.c b/drivers/gpu/drm/nouveau/nouveau_dp.c index 8db9216d52c69..4030806e3522b 100644 --- a/drivers/gpu/drm/nouveau/nouveau_dp.c +++ b/drivers/gpu/drm/nouveau/nouveau_dp.c @@ -50,11 +50,13 @@ nouveau_dp_detect(struct nouveau_connector *nv_connector, if (ret != sizeof(dpcd)) return ret;

- nv_encoder->dp.link_bw = 27000 * dpcd[1]; - nv_encoder->dp.link_nr = dpcd[2] & DP_MAX_LANE_COUNT_MASK; + nv_encoder->dp.link_bw = 27000 * dpcd[DP_MAX_LINK_RATE]; + nv_encoder->dp.link_nr = + dpcd[DP_MAX_LANE_COUNT] & DP_MAX_LANE_COUNT_MASK;

NV_DEBUG(drm, "display: %dx%d dpcd 0x%02x\n", - nv_encoder->dp.link_nr, nv_encoder->dp.link_bw, dpcd[0]); + nv_encoder->dp.link_nr, nv_encoder->dp.link_bw, + dpcd[DP_DPCD_REV]); NV_DEBUG(drm, "encoder: %dx%d\n", nv_encoder->dcb->dpconf.link_nr, nv_encoder->dcb->dpconf.link_bw);

While the way we find the associated connector for an encoder is just fine for legacy modesetting, it's not correct for nv50+ since that uses atomic modesetting. For reference, see the drm_encoder kdocs.

Fix this by removing nouveau_encoder_connector_get(), and replacing it with nv04_encoder_get_connector(), nv50_outp_get_old_connector(), and nv50_outp_get_new_connector().

v2: * Don't line-wrap for_each_(old|new)_connector_in_state in nv50_outp_get_(old|new)_connector() - sravn v3: * Fix potential uninitialized usage of nv_connector (needs to be initialized to NULL at the start). Thanks kernel test robot! v4: * Actually fix uninitialized nv_connector usage in nv50_audio_component_get_eld(). The previous fix wouldn't have worked since we would have started out with nv_connector == NULL, but wouldn't clear it after a single drm_for_each_encoder() iteration. Thanks again Kernel bot!

Signed-off-by: Lyude Paul lyude@redhat.com Reviewed-by: Ben Skeggs bskeggs@redhat.com --- drivers/gpu/drm/nouveau/dispnv04/dac.c | 2 +- drivers/gpu/drm/nouveau/dispnv04/dfp.c | 7 +- drivers/gpu/drm/nouveau/dispnv04/disp.c | 18 +++++ 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 | 88 +++++++++++++++++---- drivers/gpu/drm/nouveau/nouveau_connector.c | 14 ---- drivers/gpu/drm/nouveau/nouveau_encoder.h | 6 +- 9 files changed, 105 insertions(+), 38 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/dispnv04/dac.c b/drivers/gpu/drm/nouveau/dispnv04/dac.c index ffdd447d87068..22d10f3285597 100644 --- a/drivers/gpu/drm/nouveau/dispnv04/dac.c +++ b/drivers/gpu/drm/nouveau/dispnv04/dac.c @@ -419,7 +419,7 @@ static void nv04_dac_commit(struct drm_encoder *encoder) helper->dpms(encoder, DRM_MODE_DPMS_ON);

NV_DEBUG(drm, "Output %s is running on CRTC %d using output %c\n", - nouveau_encoder_connector_get(nv_encoder)->base.name, + nv04_encoder_get_connector(nv_encoder)->base.name, nv_crtc->index, '@' + ffs(nv_encoder->dcb->or)); }

diff --git a/drivers/gpu/drm/nouveau/dispnv04/dfp.c b/drivers/gpu/drm/nouveau/dispnv04/dfp.c index f9f4482c79b54..42687ea2a4ca3 100644 --- a/drivers/gpu/drm/nouveau/dispnv04/dfp.c +++ b/drivers/gpu/drm/nouveau/dispnv04/dfp.c @@ -184,7 +184,8 @@ static bool nv04_dfp_mode_fixup(struct drm_encoder *encoder, struct drm_display_mode *adjusted_mode) { struct nouveau_encoder *nv_encoder = nouveau_encoder(encoder); - struct nouveau_connector *nv_connector = nouveau_encoder_connector_get(nv_encoder); + struct nouveau_connector *nv_connector = + nv04_encoder_get_connector(nv_encoder);

if (!nv_connector->native_mode || nv_connector->scaling_mode == DRM_MODE_SCALE_NONE || @@ -478,7 +479,7 @@ static void nv04_dfp_commit(struct drm_encoder *encoder) helper->dpms(encoder, DRM_MODE_DPMS_ON);

NV_DEBUG(drm, "Output %s is running on CRTC %d using output %c\n", - nouveau_encoder_connector_get(nv_encoder)->base.name, + nv04_encoder_get_connector(nv_encoder)->base.name, nv_crtc->index, '@' + ffs(nv_encoder->dcb->or)); }

@@ -591,7 +592,7 @@ static void nv04_dfp_restore(struct drm_encoder *encoder)

if (nv_encoder->dcb->type == DCB_OUTPUT_LVDS) { struct nouveau_connector *connector = - nouveau_encoder_connector_get(nv_encoder); + nv04_encoder_get_connector(nv_encoder);

if (connector && connector->native_mode) call_lvds_script(dev, nv_encoder->dcb, head, diff --git a/drivers/gpu/drm/nouveau/dispnv04/disp.c b/drivers/gpu/drm/nouveau/dispnv04/disp.c index 900ab69df7e8f..3f046b917c85c 100644 --- a/drivers/gpu/drm/nouveau/dispnv04/disp.c +++ b/drivers/gpu/drm/nouveau/dispnv04/disp.c @@ -35,6 +35,24 @@

#include <nvif/if0004.h>

+struct nouveau_connector * +nv04_encoder_get_connector(struct nouveau_encoder *encoder) +{ + struct drm_device *dev = to_drm_encoder(encoder)->dev; + struct drm_connector *connector; + struct drm_connector_list_iter conn_iter; + struct nouveau_connector *nv_connector = NULL; + + drm_connector_list_iter_begin(dev, &conn_iter); + drm_for_each_connector_iter(connector, &conn_iter) { + if (connector->encoder == to_drm_encoder(encoder)) + nv_connector = nouveau_connector(connector); + } + drm_connector_list_iter_end(&conn_iter); + + return nv_connector; +} + static void nv04_display_fini(struct drm_device *dev, bool suspend) { diff --git a/drivers/gpu/drm/nouveau/dispnv04/disp.h b/drivers/gpu/drm/nouveau/dispnv04/disp.h index 495d3284e8766..5ace5e906949a 100644 --- a/drivers/gpu/drm/nouveau/dispnv04/disp.h +++ b/drivers/gpu/drm/nouveau/dispnv04/disp.h @@ -6,6 +6,8 @@

#include "nouveau_display.h"

+struct nouveau_encoder; + enum nv04_fp_display_regs { FP_DISPLAY_END, FP_TOTAL, @@ -93,6 +95,8 @@ nv04_display(struct drm_device *dev)

/* nv04_display.c */ int nv04_display_create(struct drm_device *); +struct nouveau_connector * +nv04_encoder_get_connector(struct nouveau_encoder *nv_encoder);

/* nv04_crtc.c */ int nv04_crtc_create(struct drm_device *, int index); diff --git a/drivers/gpu/drm/nouveau/dispnv04/tvnv04.c b/drivers/gpu/drm/nouveau/dispnv04/tvnv04.c index b701a4d8fe760..3ba7b59580d59 100644 --- a/drivers/gpu/drm/nouveau/dispnv04/tvnv04.c +++ b/drivers/gpu/drm/nouveau/dispnv04/tvnv04.c @@ -172,7 +172,7 @@ static void nv04_tv_commit(struct drm_encoder *encoder) helper->dpms(encoder, DRM_MODE_DPMS_ON);

NV_DEBUG(drm, "Output %s is running on CRTC %d using output %c\n", - nouveau_encoder_connector_get(nv_encoder)->base.name, + nv04_encoder_get_connector(nv_encoder)->base.name, nv_crtc->index, '@' + ffs(nv_encoder->dcb->or)); }

diff --git a/drivers/gpu/drm/nouveau/dispnv04/tvnv17.c b/drivers/gpu/drm/nouveau/dispnv04/tvnv17.c index 3a9489ed6544f..be28e7bd74903 100644 --- a/drivers/gpu/drm/nouveau/dispnv04/tvnv17.c +++ b/drivers/gpu/drm/nouveau/dispnv04/tvnv17.c @@ -599,7 +599,7 @@ static void nv17_tv_commit(struct drm_encoder *encoder) helper->dpms(encoder, DRM_MODE_DPMS_ON);

NV_INFO(drm, "Output %s is running on CRTC %d using output %c\n", - nouveau_encoder_connector_get(nv_encoder)->base.name, + nv04_encoder_get_connector(nv_encoder)->base.name, nv_crtc->index, '@' + ffs(nv_encoder->dcb->or)); }

diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c index c4d138f0ca054..255a281249bc8 100644 --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c @@ -411,6 +411,40 @@ nv50_outp_atomic_check(struct drm_encoder *encoder, return 0; }

+struct nouveau_connector * +nv50_outp_get_new_connector(struct nouveau_encoder *outp, + struct drm_atomic_state *state) +{ + struct drm_connector *connector; + struct drm_connector_state *connector_state; + struct drm_encoder *encoder = to_drm_encoder(outp); + int i; + + for_each_new_connector_in_state(state, connector, connector_state, i) { + if (connector_state->best_encoder == encoder) + return nouveau_connector(connector); + } + + return NULL; +} + +struct nouveau_connector * +nv50_outp_get_old_connector(struct nouveau_encoder *outp, + struct drm_atomic_state *state) +{ + struct drm_connector *connector; + struct drm_connector_state *connector_state; + struct drm_encoder *encoder = to_drm_encoder(outp); + int i; + + for_each_old_connector_in_state(state, connector, connector_state, i) { + if (connector_state->best_encoder == encoder) + return nouveau_connector(connector); + } + + return NULL; +} + /****************************************************************************** * DAC *****************************************************************************/ @@ -552,16 +586,31 @@ nv50_audio_component_get_eld(struct device *kdev, int port, int dev_id, struct nouveau_drm *drm = nouveau_drm(drm_dev); struct drm_encoder *encoder; struct nouveau_encoder *nv_encoder; - struct nouveau_connector *nv_connector; + struct drm_connector *connector; struct nouveau_crtc *nv_crtc; + struct drm_connector_list_iter conn_iter; int ret = 0;

*enabled = false; + drm_for_each_encoder(encoder, drm->dev) { + struct nouveau_connector *nv_connector = NULL; + nv_encoder = nouveau_encoder(encoder); - nv_connector = nouveau_encoder_connector_get(nv_encoder); + + drm_connector_list_iter_begin(drm_dev, &conn_iter); + drm_for_each_connector_iter(connector, &conn_iter) { + if (connector->state->best_encoder == encoder) { + nv_connector = nouveau_connector(connector); + break; + } + } + drm_connector_list_iter_end(&conn_iter); + if (!nv_connector) + continue; + nv_crtc = nouveau_crtc(encoder->crtc); - if (!nv_connector || !nv_crtc || nv_encoder->or != port || + if (!nv_crtc || nv_encoder->or != port || nv_crtc->index != dev_id) continue; *enabled = nv_encoder->audio; @@ -572,6 +621,7 @@ nv50_audio_component_get_eld(struct device *kdev, int port, int dev_id, } break; } + return ret; }

@@ -665,7 +715,8 @@ nv50_audio_disable(struct drm_encoder *encoder, struct nouveau_crtc *nv_crtc) }

static void -nv50_audio_enable(struct drm_encoder *encoder, struct drm_display_mode *mode) +nv50_audio_enable(struct drm_encoder *encoder, struct drm_atomic_state *state, + struct drm_display_mode *mode) { struct nouveau_drm *drm = nouveau_drm(encoder->dev); struct nouveau_encoder *nv_encoder = nouveau_encoder(encoder); @@ -686,7 +737,7 @@ nv50_audio_enable(struct drm_encoder *encoder, struct drm_display_mode *mode) (0x0100 << nv_crtc->index), };

- nv_connector = nouveau_encoder_connector_get(nv_encoder); + nv_connector = nv50_outp_get_new_connector(nv_encoder, state); if (!drm_detect_monitor_audio(nv_connector->edid)) return;

@@ -723,7 +774,8 @@ nv50_hdmi_disable(struct drm_encoder *encoder, struct nouveau_crtc *nv_crtc) }

static void -nv50_hdmi_enable(struct drm_encoder *encoder, struct drm_display_mode *mode) +nv50_hdmi_enable(struct drm_encoder *encoder, struct drm_atomic_state *state, + struct drm_display_mode *mode) { struct nouveau_drm *drm = nouveau_drm(encoder->dev); struct nouveau_encoder *nv_encoder = nouveau_encoder(encoder); @@ -752,7 +804,7 @@ nv50_hdmi_enable(struct drm_encoder *encoder, struct drm_display_mode *mode) int ret; int size;

- nv_connector = nouveau_encoder_connector_get(nv_encoder); + nv_connector = nv50_outp_get_new_connector(nv_encoder, state); if (!drm_detect_hdmi_monitor(nv_connector->edid)) return;

@@ -798,7 +850,7 @@ nv50_hdmi_enable(struct drm_encoder *encoder, struct drm_display_mode *mode) + args.pwr.vendor_infoframe_length; nvif_mthd(&disp->disp->object, 0, &args, size);

- nv50_audio_enable(encoder, mode); + nv50_audio_enable(encoder, state, mode);

/* If SCDC is supported by the downstream monitor, update * divider / scrambling settings to what we programmed above. @@ -1573,7 +1625,8 @@ nv50_sor_update(struct nouveau_encoder *nv_encoder, u8 head, }

static void -nv50_sor_disable(struct drm_encoder *encoder) +nv50_sor_disable(struct drm_encoder *encoder, + struct drm_atomic_state *state) { struct nouveau_encoder *nv_encoder = nouveau_encoder(encoder); struct nouveau_crtc *nv_crtc = nouveau_crtc(nv_encoder->crtc); @@ -1601,7 +1654,8 @@ nv50_sor_disable(struct drm_encoder *encoder) }

static void -nv50_sor_enable(struct drm_encoder *encoder) +nv50_sor_enable(struct drm_encoder *encoder, + struct drm_atomic_state *state) { struct nouveau_encoder *nv_encoder = nouveau_encoder(encoder); struct nouveau_crtc *nv_crtc = nouveau_crtc(encoder->crtc); @@ -1625,7 +1679,7 @@ nv50_sor_enable(struct drm_encoder *encoder) u8 proto = NV507D_SOR_SET_CONTROL_PROTOCOL_CUSTOM; u8 depth = NV837D_SOR_SET_CONTROL_PIXEL_DEPTH_DEFAULT;

- nv_connector = nouveau_encoder_connector_get(nv_encoder); + nv_connector = nv50_outp_get_new_connector(nv_encoder, state); nv_encoder->crtc = encoder->crtc;

if ((disp->disp->object.oclass == GT214_DISP || @@ -1652,7 +1706,7 @@ nv50_sor_enable(struct drm_encoder *encoder) proto = NV507D_SOR_SET_CONTROL_PROTOCOL_SINGLE_TMDS_B; }

- nv50_hdmi_enable(&nv_encoder->base.base, mode); + nv50_hdmi_enable(&nv_encoder->base.base, state, mode); break; case DCB_OUTPUT_LVDS: proto = NV507D_SOR_SET_CONTROL_PROTOCOL_LVDS_CUSTOM; @@ -1693,7 +1747,7 @@ nv50_sor_enable(struct drm_encoder *encoder) else proto = NV887D_SOR_SET_CONTROL_PROTOCOL_DP_B;

- nv50_audio_enable(encoder, mode); + nv50_audio_enable(encoder, state, mode); break; default: BUG(); @@ -1706,8 +1760,8 @@ nv50_sor_enable(struct drm_encoder *encoder) static const struct drm_encoder_helper_funcs nv50_sor_help = { .atomic_check = nv50_outp_atomic_check, - .enable = nv50_sor_enable, - .disable = nv50_sor_disable, + .atomic_enable = nv50_sor_enable, + .atomic_disable = nv50_sor_disable, };

static void @@ -2066,7 +2120,7 @@ nv50_disp_atomic_commit_tail(struct drm_atomic_state *state) outp->clr.mask, outp->set.mask);

if (outp->clr.mask) { - help->disable(encoder); + help->atomic_disable(encoder, state); interlock[NV50_DISP_INTERLOCK_CORE] |= 1; if (outp->flush_disable) { nv50_disp_atomic_commit_wndw(state, interlock); @@ -2105,7 +2159,7 @@ nv50_disp_atomic_commit_tail(struct drm_atomic_state *state) outp->set.mask, outp->clr.mask);

if (outp->set.mask) { - help->enable(encoder); + help->atomic_enable(encoder, state); interlock[NV50_DISP_INTERLOCK_CORE] = 1; }

diff --git a/drivers/gpu/drm/nouveau/nouveau_connector.c b/drivers/gpu/drm/nouveau/nouveau_connector.c index e12957e6faa7c..1d5696c39792a 100644 --- a/drivers/gpu/drm/nouveau/nouveau_connector.c +++ b/drivers/gpu/drm/nouveau/nouveau_connector.c @@ -391,20 +391,6 @@ find_encoder(struct drm_connector *connector, int type) return NULL; }

-struct nouveau_connector * -nouveau_encoder_connector_get(struct nouveau_encoder *encoder) -{ - struct drm_device *dev = to_drm_encoder(encoder)->dev; - struct drm_connector *drm_connector; - - list_for_each_entry(drm_connector, &dev->mode_config.connector_list, head) { - if (drm_connector->encoder == to_drm_encoder(encoder)) - return nouveau_connector(drm_connector); - } - - return NULL; -} - static void nouveau_connector_destroy(struct drm_connector *connector) { diff --git a/drivers/gpu/drm/nouveau/nouveau_encoder.h b/drivers/gpu/drm/nouveau/nouveau_encoder.h index 6424cdcb4913f..b0e1dad2367d6 100644 --- a/drivers/gpu/drm/nouveau/nouveau_encoder.h +++ b/drivers/gpu/drm/nouveau/nouveau_encoder.h @@ -113,7 +113,11 @@ enum drm_mode_status nv50_dp_mode_valid(struct drm_connector *, unsigned *clock);

struct nouveau_connector * -nouveau_encoder_connector_get(struct nouveau_encoder *encoder); +nv50_outp_get_new_connector(struct nouveau_encoder *outp, + struct drm_atomic_state *state); +struct nouveau_connector * +nv50_outp_get_old_connector(struct nouveau_encoder *outp, + struct drm_atomic_state *state);

int nv50_mstm_detect(struct nv50_mstm *, u8 dpcd[8], int allow); void nv50_mstm_remove(struct nv50_mstm *);

First some backstory here: Currently, we keep track of whether or not we've enabled MST or not by trying to piggy-back off the MST helpers. This means that in order to check whether MST is enabled or not, we actually need to grab drm_dp_mst_topology_mgr.lock.

Back when I originally wrote this, I did this piggy-backing with the intention that I'd eventually be teaching our MST helpers how to recover when an MST device has stopped responding, which in turn would require the MST helpers having a way of disabling MST independently of the driver. Note that this was before I reworked locking in the MST helpers, so at the time we were sticking random things under &mgr->lock - which grabbing this lock was meant to protect against.

This never came to fruition because doing such a reset safely turned out to be a lot more painful and impossible then it sounds, and also just risks us working around issues with our MST handlers that should be properly fixed instead. Even if it did though, simply calling drm_dp_mst_topology_mgr_set_mst() from the MST helpers (with the exception of when we're tearing down our MST managers, that's always OK) wouldn't have been a bad idea, since drivers like nouveau and i915 need to do their own book keeping immediately after disabling MST. So-implementing that would likely require adding a hook for helper-triggered MST disables anyway.

So, fast forward to now - we want to start adding support for all of the miscellaneous bits of the DP protocol (for both SST and MST) we're missing before moving on to supporting more complicated features like supporting different BPP values on MST, DSC, etc. Since many of these features only exist on SST and make use of DP HPD IRQs, we want to be able to atomically check whether we're servicing an MST IRQ or SST IRQ in nouveau_connector_hotplug(). Currently we literally don't do this at all, and just handle any kind of possible DP IRQ we could get including ESIs - even if MST isn't actually enabled.

This would be very complicated and difficult to fix if we need to hold &mgr->lock while handling SST IRQs to ensure that the MST topology state doesn't change under us. What we really want here is to do our own tracking of whether MST is enabled or not, similar to drivers like i915, and define our own locking order to decomplicate things and avoid hitting locking issues in the future.

So, let's do this by refactoring our MST probing/enabling code to use our own MST bookkeeping, along with adding a lock for protecting DP state that needs to be checked outside of our connector probing functions. While we're at it, we also remove a bunch of unneeded steps we perform when probing/enabling MST:

* Enabling bits in MSTM_CTRL before calling drm_dp_mst_topology_mgr_set_mst(). I don't think these ever actually did anything, since the nvif methods for enabling MST don't actually do anything DPCD related and merely indicate to nvkm that we've turned on MST. * Checking the MSTM_CTRL bit is intact when checking the state of an enabled MST topology in nv50_mstm_detect(). I just added this to be safe originally, but now that we try reading the DPCD when probing DP connectors it shouldn't be needed as that will abort our hotplug probing if the device was removed well before we start checking for MST.. * All of the duplicate DPCD version checks.

This leaves us with much nicer looking code, a much more sensible locking scheme, and an easy way of checking whether MST is enabled or not for handling DP HPD IRQs.

v2: * Get rid of accidental newlines v4: * Fix uninitialized usage of mstm in nv50_mstm_detect() - thanks kernel bot!

Signed-off-by: Lyude Paul lyude@redhat.com Reviewed-by: Ben Skeggs bskeggs@redhat.com --- drivers/gpu/drm/nouveau/dispnv04/disp.c | 6 +- drivers/gpu/drm/nouveau/dispnv50/disp.c | 192 +++++++++----------- drivers/gpu/drm/nouveau/nouveau_connector.c | 14 +- drivers/gpu/drm/nouveau/nouveau_display.c | 2 +- drivers/gpu/drm/nouveau/nouveau_display.h | 2 +- drivers/gpu/drm/nouveau/nouveau_dp.c | 132 ++++++++++++-- drivers/gpu/drm/nouveau/nouveau_encoder.h | 33 +++- 7 files changed, 244 insertions(+), 137 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/dispnv04/disp.c b/drivers/gpu/drm/nouveau/dispnv04/disp.c index 3f046b917c85c..3ee836dc5058f 100644 --- a/drivers/gpu/drm/nouveau/dispnv04/disp.c +++ b/drivers/gpu/drm/nouveau/dispnv04/disp.c @@ -54,8 +54,9 @@ nv04_encoder_get_connector(struct nouveau_encoder *encoder) }

static void -nv04_display_fini(struct drm_device *dev, bool suspend) +nv04_display_fini(struct drm_device *dev, bool runtime, bool suspend) { + struct nouveau_drm *drm = nouveau_drm(dev); struct nv04_display *disp = nv04_display(dev); struct drm_crtc *crtc;

@@ -67,6 +68,9 @@ nv04_display_fini(struct drm_device *dev, bool suspend) if (nv_two_heads(dev)) NVWriteCRTC(dev, 1, NV_PCRTC_INTR_EN_0, 0);

+ if (!runtime) + cancel_work_sync(&drm->hpd_work); + if (!suspend) return;

diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c index 612d98fa0a2dc..8e1effb10425d 100644 --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c @@ -879,16 +879,6 @@ nv50_hdmi_enable(struct drm_encoder *encoder, struct drm_atomic_state *state, #define nv50_mstc(p) container_of((p), struct nv50_mstc, connector) #define nv50_msto(p) container_of((p), struct nv50_msto, encoder)

-struct nv50_mstm { - struct nouveau_encoder *outp; - - struct drm_dp_mst_topology_mgr mgr; - - bool modified; - bool disabled; - int links; -}; - struct nv50_mstc { struct nv50_mstm *mstm; struct drm_dp_mst_port *port; @@ -1417,41 +1407,51 @@ nv50_mstm = { .add_connector = nv50_mstm_add_connector, };

-void -nv50_mstm_service(struct nv50_mstm *mstm) +bool +nv50_mstm_service(struct nouveau_drm *drm, + struct nouveau_connector *nv_connector, + struct nv50_mstm *mstm) { - struct drm_dp_aux *aux = mstm ? mstm->mgr.aux : NULL; - bool handled = true; - int ret; + struct drm_dp_aux *aux = &nv_connector->aux; + bool handled = true, ret = true; + int rc; u8 esi[8] = {};

- if (!aux) - return; - while (handled) { - ret = drm_dp_dpcd_read(aux, DP_SINK_COUNT_ESI, esi, 8); - if (ret != 8) { - drm_dp_mst_topology_mgr_set_mst(&mstm->mgr, false); - return; + rc = drm_dp_dpcd_read(aux, DP_SINK_COUNT_ESI, esi, 8); + if (rc != 8) { + ret = false; + break; }

drm_dp_mst_hpd_irq(&mstm->mgr, esi, &handled); if (!handled) break;

- drm_dp_dpcd_write(aux, DP_SINK_COUNT_ESI + 1, &esi[1], 3); + rc = drm_dp_dpcd_write(aux, DP_SINK_COUNT_ESI + 1, &esi[1], + 3); + if (rc != 3) { + ret = false; + break; + } } + + if (!ret) + NV_DEBUG(drm, "Failed to handle ESI on %s: %d\n", + nv_connector->base.name, rc); + + return ret; }

void nv50_mstm_remove(struct nv50_mstm *mstm) { - if (mstm) - drm_dp_mst_topology_mgr_set_mst(&mstm->mgr, false); + mstm->is_mst = false; + drm_dp_mst_topology_mgr_set_mst(&mstm->mgr, false); }

static int -nv50_mstm_enable(struct nv50_mstm *mstm, u8 dpcd, int state) +nv50_mstm_enable(struct nv50_mstm *mstm, int state) { struct nouveau_encoder *outp = mstm->outp; struct { @@ -1466,106 +1466,85 @@ nv50_mstm_enable(struct nv50_mstm *mstm, u8 dpcd, int state) }; struct nouveau_drm *drm = nouveau_drm(outp->base.base.dev); struct nvif_object *disp = &drm->display->disp.object; - int ret; - - if (dpcd >= 0x12) { - /* Even if we're enabling MST, start with disabling the - * branching unit to clear any sink-side MST topology state - * that wasn't set by us - */ - ret = drm_dp_dpcd_writeb(mstm->mgr.aux, DP_MSTM_CTRL, 0); - if (ret < 0) - return ret; - - if (state) { - /* Now, start initializing */ - ret = drm_dp_dpcd_writeb(mstm->mgr.aux, DP_MSTM_CTRL, - DP_MST_EN); - if (ret < 0) - return ret; - } - }

return nvif_mthd(disp, 0, &args, sizeof(args)); }

int -nv50_mstm_detect(struct nv50_mstm *mstm, u8 dpcd[8], int allow) +nv50_mstm_detect(struct nouveau_encoder *outp) { + struct nv50_mstm *mstm = outp->dp.mstm; struct drm_dp_aux *aux; int ret; - bool old_state, new_state; - u8 mstm_ctrl;

- if (!mstm) + if (!mstm || !mstm->can_mst) return 0;

- mutex_lock(&mstm->mgr.lock); - - old_state = mstm->mgr.mst_state; - new_state = old_state; aux = mstm->mgr.aux;

- if (old_state) { - /* Just check that the MST hub is still as we expect it */ - ret = drm_dp_dpcd_readb(aux, DP_MSTM_CTRL, &mstm_ctrl); - if (ret < 0 || !(mstm_ctrl & DP_MST_EN)) { - DRM_DEBUG_KMS("Hub gone, disabling MST topology\n"); - new_state = false; - } - } else if (dpcd[0] >= 0x12) { - ret = drm_dp_dpcd_readb(aux, DP_MSTM_CAP, &dpcd[1]); - if (ret < 0) - goto probe_error; - - if (!(dpcd[1] & DP_MST_CAP)) - dpcd[0] = 0x11; - else - new_state = allow; - } - - if (new_state == old_state) { - mutex_unlock(&mstm->mgr.lock); - return new_state; - } - - ret = nv50_mstm_enable(mstm, dpcd[0], new_state); - if (ret) - goto probe_error; - - mutex_unlock(&mstm->mgr.lock); + /* Clear any leftover MST state we didn't set ourselves by first + * disabling MST if it was already enabled + */ + ret = drm_dp_dpcd_writeb(aux, DP_MSTM_CTRL, 0); + if (ret < 0) + return ret;

- ret = drm_dp_mst_topology_mgr_set_mst(&mstm->mgr, new_state); + /* And start enabling */ + ret = nv50_mstm_enable(mstm, true); if (ret) - return nv50_mstm_enable(mstm, dpcd[0], 0); + return ret;

- return new_state; + ret = drm_dp_mst_topology_mgr_set_mst(&mstm->mgr, true); + if (ret) { + nv50_mstm_enable(mstm, false); + return ret; + }

-probe_error: - mutex_unlock(&mstm->mgr.lock); - return ret; + mstm->is_mst = true; + return 1; }

static void -nv50_mstm_fini(struct nv50_mstm *mstm) +nv50_mstm_fini(struct nouveau_encoder *outp) { - if (mstm && mstm->mgr.mst_state) + struct nv50_mstm *mstm = outp->dp.mstm; + + if (!mstm) + return; + + /* Don't change the MST state of this connector until we've finished + * resuming, since we can't safely grab hpd_irq_lock in our resume + * path to protect mstm->is_mst without potentially deadlocking + */ + mutex_lock(&outp->dp.hpd_irq_lock); + mstm->suspended = true; + mutex_unlock(&outp->dp.hpd_irq_lock); + + if (mstm->is_mst) drm_dp_mst_topology_mgr_suspend(&mstm->mgr); }

static void -nv50_mstm_init(struct nv50_mstm *mstm, bool runtime) +nv50_mstm_init(struct nouveau_encoder *outp, bool runtime) { - int ret; + struct nv50_mstm *mstm = outp->dp.mstm; + int ret = 0;

- if (!mstm || !mstm->mgr.mst_state) + if (!mstm) return;

- ret = drm_dp_mst_topology_mgr_resume(&mstm->mgr, !runtime); - if (ret == -1) { - drm_dp_mst_topology_mgr_set_mst(&mstm->mgr, false); - drm_kms_helper_hotplug_event(mstm->mgr.dev); + if (mstm->is_mst) { + ret = drm_dp_mst_topology_mgr_resume(&mstm->mgr, !runtime); + if (ret == -1) + nv50_mstm_remove(mstm); } + + mutex_lock(&outp->dp.hpd_irq_lock); + mstm->suspended = false; + mutex_unlock(&outp->dp.hpd_irq_lock); + + if (ret == -1) + drm_kms_helper_hotplug_event(mstm->mgr.dev); }

static void @@ -1773,6 +1752,10 @@ nv50_sor_destroy(struct drm_encoder *encoder) struct nouveau_encoder *nv_encoder = nouveau_encoder(encoder); nv50_mstm_del(&nv_encoder->dp.mstm); drm_encoder_cleanup(encoder); + + if (nv_encoder->dcb->type == DCB_OUTPUT_DP) + mutex_destroy(&nv_encoder->dp.hpd_irq_lock); + kfree(encoder); }

@@ -1832,6 +1815,8 @@ nv50_sor_create(struct drm_connector *connector, struct dcb_output *dcbe) struct nvkm_i2c_aux *aux = nvkm_i2c_aux_find(i2c, dcbe->i2c_index);

+ mutex_init(&nv_encoder->dp.hpd_irq_lock); + if (aux) { if (disp->disp->object.oclass < GF110_DISP) { /* HW has no support for address-only @@ -2530,9 +2515,9 @@ nv50_disp_func = { *****************************************************************************/

static void -nv50_display_fini(struct drm_device *dev, bool suspend) +nv50_display_fini(struct drm_device *dev, bool runtime, bool suspend) { - struct nouveau_encoder *nv_encoder; + struct nouveau_drm *drm = nouveau_drm(dev); struct drm_encoder *encoder; struct drm_plane *plane;

@@ -2544,11 +2529,12 @@ nv50_display_fini(struct drm_device *dev, bool suspend) }

list_for_each_entry(encoder, &dev->mode_config.encoder_list, head) { - if (encoder->encoder_type != DRM_MODE_ENCODER_DPMST) { - nv_encoder = nouveau_encoder(encoder); - nv50_mstm_fini(nv_encoder->dp.mstm); - } + if (encoder->encoder_type != DRM_MODE_ENCODER_DPMST) + nv50_mstm_fini(nouveau_encoder(encoder)); } + + if (!runtime) + cancel_work_sync(&drm->hpd_work); }

static int @@ -2565,7 +2551,7 @@ nv50_display_init(struct drm_device *dev, bool resume, bool runtime) if (encoder->encoder_type != DRM_MODE_ENCODER_DPMST) { struct nouveau_encoder *nv_encoder = nouveau_encoder(encoder); - nv50_mstm_init(nv_encoder->dp.mstm, runtime); + nv50_mstm_init(nv_encoder, runtime); } }

diff --git a/drivers/gpu/drm/nouveau/nouveau_connector.c b/drivers/gpu/drm/nouveau/nouveau_connector.c index 1d5696c39792a..b90591114faaf 100644 --- a/drivers/gpu/drm/nouveau/nouveau_connector.c +++ b/drivers/gpu/drm/nouveau/nouveau_connector.c @@ -1143,19 +1143,15 @@ nouveau_connector_hotplug(struct nvif_notify *notify) struct nouveau_connector *nv_connector = container_of(notify, typeof(*nv_connector), hpd); struct drm_connector *connector = &nv_connector->base; - struct nouveau_drm *drm = nouveau_drm(connector->dev); + struct drm_device *dev = connector->dev; + struct nouveau_drm *drm = nouveau_drm(dev); const struct nvif_notify_conn_rep_v0 *rep = notify->data; const char *name = connector->name; - struct nouveau_encoder *nv_encoder; int ret; bool plugged = (rep->mask != NVIF_NOTIFY_CONN_V0_UNPLUG);

if (rep->mask & NVIF_NOTIFY_CONN_V0_IRQ) { - NV_DEBUG(drm, "service %s\n", name); - drm_dp_cec_irq(&nv_connector->aux); - if ((nv_encoder = find_encoder(connector, DCB_OUTPUT_DP))) - nv50_mstm_service(nv_encoder->dp.mstm); - + nouveau_dp_irq(drm, nv_connector); return NVIF_NOTIFY_KEEP; }

@@ -1181,10 +1177,6 @@ nouveau_connector_hotplug(struct nvif_notify *notify) if (!plugged) drm_dp_cec_unset_edid(&nv_connector->aux); NV_DEBUG(drm, "%splugged %s\n", plugged ? "" : "un", name); - if ((nv_encoder = find_encoder(connector, DCB_OUTPUT_DP))) { - if (!plugged) - nv50_mstm_remove(nv_encoder->dp.mstm); - }

drm_helper_hpd_irq_event(connector->dev);

diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c b/drivers/gpu/drm/nouveau/nouveau_display.c index 5f31b11ac2e7e..13016769a194b 100644 --- a/drivers/gpu/drm/nouveau/nouveau_display.c +++ b/drivers/gpu/drm/nouveau/nouveau_display.c @@ -569,7 +569,7 @@ nouveau_display_fini(struct drm_device *dev, bool suspend, bool runtime) cancel_work_sync(&drm->hpd_work);

drm_kms_helper_poll_disable(dev); - disp->fini(dev, suspend); + disp->fini(dev, runtime, suspend); }

static void diff --git a/drivers/gpu/drm/nouveau/nouveau_display.h b/drivers/gpu/drm/nouveau/nouveau_display.h index 6e0d900441d69..76ba93e01aff4 100644 --- a/drivers/gpu/drm/nouveau/nouveau_display.h +++ b/drivers/gpu/drm/nouveau/nouveau_display.h @@ -18,7 +18,7 @@ struct nouveau_display { void *priv; void (*dtor)(struct drm_device *); int (*init)(struct drm_device *, bool resume, bool runtime); - void (*fini)(struct drm_device *, bool suspend); + void (*fini)(struct drm_device *, bool suspend, bool runtime);

struct nvif_disp disp;

diff --git a/drivers/gpu/drm/nouveau/nouveau_dp.c b/drivers/gpu/drm/nouveau/nouveau_dp.c index 4030806e3522b..032afc73e2a33 100644 --- a/drivers/gpu/drm/nouveau/nouveau_dp.c +++ b/drivers/gpu/drm/nouveau/nouveau_dp.c @@ -36,19 +36,89 @@ MODULE_PARM_DESC(mst, "Enable DisplayPort multi-stream (default: enabled)"); static int nouveau_mst = 1; module_param_named(mst, nouveau_mst, int, 0400);

+static enum drm_connector_status +nouveau_dp_probe_dpcd(struct nouveau_connector *nv_connector, + struct nouveau_encoder *outp) +{ + struct drm_dp_aux *aux = &nv_connector->aux; + struct nv50_mstm *mstm = NULL; + int ret; + u8 *dpcd = outp->dp.dpcd; + u8 tmp; + + ret = drm_dp_dpcd_read(aux, DP_DPCD_REV, dpcd, DP_RECEIVER_CAP_SIZE); + if (ret == DP_RECEIVER_CAP_SIZE && dpcd[DP_DPCD_REV]) { + ret = drm_dp_read_desc(aux, &outp->dp.desc, + drm_dp_is_branch(dpcd)); + if (ret < 0) + return connector_status_disconnected; + } else { + return connector_status_disconnected; + } + + if (nouveau_mst) + mstm = outp->dp.mstm; + + if (mstm) { + if (dpcd[DP_DPCD_REV] >= DP_DPCD_REV_12) { + ret = drm_dp_dpcd_readb(aux, DP_MSTM_CAP, &tmp); + if (ret < 0) + return connector_status_disconnected; + + mstm->can_mst = !!(tmp & DP_MST_CAP); + } else { + mstm->can_mst = false; + } + } + + return connector_status_connected; +} + int nouveau_dp_detect(struct nouveau_connector *nv_connector, struct nouveau_encoder *nv_encoder) { struct drm_device *dev = nv_encoder->base.base.dev; struct nouveau_drm *drm = nouveau_drm(dev); - struct drm_dp_aux *aux = &nv_connector->aux; - u8 dpcd[DP_RECEIVER_CAP_SIZE]; - int ret; + struct drm_connector *connector = &nv_connector->base; + struct nv50_mstm *mstm = nv_encoder->dp.mstm; + enum drm_connector_status status; + u8 *dpcd = nv_encoder->dp.dpcd; + int ret = NOUVEAU_DP_NONE; + + /* If we've already read the DPCD on an eDP device, we don't need to + * reread it as it won't change + */ + if (connector->connector_type == DRM_MODE_CONNECTOR_eDP && + dpcd[DP_DPCD_REV] != 0) + return NOUVEAU_DP_SST;

- ret = drm_dp_dpcd_read(aux, DP_DPCD_REV, dpcd, DP_RECEIVER_CAP_SIZE); - if (ret != sizeof(dpcd)) - return ret; + mutex_lock(&nv_encoder->dp.hpd_irq_lock); + if (mstm) { + /* If we're not ready to handle MST state changes yet, just + * report the last status of the connector. We'll reprobe it + * once we've resumed. + */ + if (mstm->suspended) { + if (mstm->is_mst) + ret = NOUVEAU_DP_MST; + else if (connector->status == + connector_status_connected) + ret = NOUVEAU_DP_SST; + + goto out; + } + } + + status = nouveau_dp_probe_dpcd(nv_connector, nv_encoder); + if (status == connector_status_disconnected) + goto out; + + /* If we're in MST mode, we're done here */ + if (mstm && mstm->can_mst && mstm->is_mst) { + ret = NOUVEAU_DP_MST; + goto out; + }

nv_encoder->dp.link_bw = 27000 * dpcd[DP_MAX_LINK_RATE]; nv_encoder->dp.link_nr = @@ -69,22 +139,50 @@ nouveau_dp_detect(struct nouveau_connector *nv_connector, NV_DEBUG(drm, "maximum: %dx%d\n", nv_encoder->dp.link_nr, nv_encoder->dp.link_bw);

- ret = drm_dp_read_desc(&nv_connector->aux, &nv_encoder->dp.desc, - drm_dp_is_branch(dpcd)); - if (ret) { - NV_ERROR(drm, "Failed to read DP descriptor on %s: %d\n", - nv_connector->base.name, ret); - return ret; + if (mstm && mstm->can_mst) { + ret = nv50_mstm_detect(nv_encoder); + if (ret == 1) { + ret = NOUVEAU_DP_MST; + goto out; + } else if (ret != 0) { + goto out; + } } + ret = NOUVEAU_DP_SST;

- ret = nv50_mstm_detect(nv_encoder->dp.mstm, dpcd, nouveau_mst); - if (ret == 1) - return NOUVEAU_DP_MST; - if (ret == 0) - return NOUVEAU_DP_SST; +out: + if (mstm && !mstm->suspended && ret != NOUVEAU_DP_MST) + nv50_mstm_remove(mstm); + + mutex_unlock(&nv_encoder->dp.hpd_irq_lock); return ret; }

+void nouveau_dp_irq(struct nouveau_drm *drm, + struct nouveau_connector *nv_connector) +{ + struct drm_connector *connector = &nv_connector->base; + struct nouveau_encoder *outp = find_encoder(connector, DCB_OUTPUT_DP); + struct nv50_mstm *mstm; + + if (!outp) + return; + + mstm = outp->dp.mstm; + NV_DEBUG(drm, "service %s\n", connector->name); + + mutex_lock(&outp->dp.hpd_irq_lock); + + if (mstm && mstm->is_mst) { + if (!nv50_mstm_service(drm, nv_connector, mstm)) + schedule_work(&drm->hpd_work); + } else { + drm_dp_cec_irq(&nv_connector->aux); + } + + mutex_unlock(&outp->dp.hpd_irq_lock); +} + /* TODO: * - Use the minimum possible BPC here, once we add support for the max bpc * property. diff --git a/drivers/gpu/drm/nouveau/nouveau_encoder.h b/drivers/gpu/drm/nouveau/nouveau_encoder.h index b0e1dad2367d6..eef4643f5f982 100644 --- a/drivers/gpu/drm/nouveau/nouveau_encoder.h +++ b/drivers/gpu/drm/nouveau/nouveau_encoder.h @@ -65,6 +65,13 @@ struct nouveau_encoder { struct nv50_mstm *mstm; int link_nr; int link_bw; + + /* Protects DP state that needs to be accessed outside + * connector reprobing contexts + */ + struct mutex hpd_irq_lock; + + u8 dpcd[DP_RECEIVER_CAP_SIZE]; struct drm_dp_desc desc; } dp; }; @@ -79,6 +86,21 @@ struct nouveau_encoder { struct nv50_head_atom *, u8 proto, u8 depth); };

+struct nv50_mstm { + struct nouveau_encoder *outp; + + struct drm_dp_mst_topology_mgr mgr; + + /* Protected under nouveau_encoder->dp.hpd_irq_lock */ + bool can_mst; + bool is_mst; + bool suspended; + + bool modified; + bool disabled; + int links; +}; + struct nouveau_encoder * find_encoder(struct drm_connector *connector, int type);

@@ -102,11 +124,14 @@ get_slave_funcs(struct drm_encoder *enc)

/* nouveau_dp.c */ enum nouveau_dp_status { + NOUVEAU_DP_NONE, NOUVEAU_DP_SST, NOUVEAU_DP_MST, };

int nouveau_dp_detect(struct nouveau_connector *, struct nouveau_encoder *); +void nouveau_dp_irq(struct nouveau_drm *drm, + struct nouveau_connector *nv_connector); enum drm_mode_status nv50_dp_mode_valid(struct drm_connector *, struct nouveau_encoder *, const struct drm_display_mode *, @@ -119,7 +144,9 @@ struct nouveau_connector * nv50_outp_get_old_connector(struct nouveau_encoder *outp, struct drm_atomic_state *state);

-int nv50_mstm_detect(struct nv50_mstm *, u8 dpcd[8], int allow); -void nv50_mstm_remove(struct nv50_mstm *); -void nv50_mstm_service(struct nv50_mstm *); +int nv50_mstm_detect(struct nouveau_encoder *encoder); +void nv50_mstm_remove(struct nv50_mstm *mstm); +bool nv50_mstm_service(struct nouveau_drm *drm, + struct nouveau_connector *nv_connector, + struct nv50_mstm *mstm); #endif /* __NOUVEAU_ENCODER_H__ */

For whatever reason we currently unset the EDID for DP CEC support when responding to the connector being unplugged, instead of just doing it in nouveau_connector_detect() where we set the CEC EDID. This isn't really needed and could even potentially cause us to forget to unset the EDID if the connector is removed without a corresponding hpd event, so let's fix that.

Signed-off-by: Lyude Paul lyude@redhat.com Reviewed-by: Ben Skeggs bskeggs@redhat.com --- drivers/gpu/drm/nouveau/nouveau_connector.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nouveau_connector.c b/drivers/gpu/drm/nouveau/nouveau_connector.c index b90591114faaf..4a29f691c08e4 100644 --- a/drivers/gpu/drm/nouveau/nouveau_connector.c +++ b/drivers/gpu/drm/nouveau/nouveau_connector.c @@ -633,10 +633,11 @@ nouveau_connector_detect(struct drm_connector *connector, bool force) conn_status = connector_status_connected; goto out; } - }

out: + if (!nv_connector->edid) + drm_dp_cec_unset_edid(&nv_connector->aux);

pm_runtime_mark_last_busy(dev->dev); pm_runtime_put_autosuspend(dev->dev); @@ -1174,8 +1175,6 @@ nouveau_connector_hotplug(struct nvif_notify *notify) return NVIF_NOTIFY_DROP; }

- if (!plugged) - drm_dp_cec_unset_edid(&nv_connector->aux); NV_DEBUG(drm, "%splugged %s\n", plugged ? "" : "un", name);

drm_helper_hpd_irq_event(connector->dev);

We're going to be doing the same probing process in nouveau for determining downstream DP port capabilities, so let's deduplicate the work by moving i915's code for handling this into a shared helper: drm_dp_downstream_read_info().

Note that when we do this, we also do make some functional changes while we're at it: * We always clear the downstream port info before trying to read it, just to make things easier for the caller * We skip reading downstream port info if the DPCD indicates that we don't support downstream port info * We only read as many bytes as needed for the reported number of downstream ports, no sense in reading the whole thing every time

v2: * Fixup logic for calculating the downstream port length to account for the fact that downstream port caps can be either 1 byte or 4 bytes long. We can actually skip fixing the max_clock/max_bpc helpers here since they all check for DP_DETAILED_CAP_INFO_AVAILABLE anyway. * Fix ret code check for drm_dp_dpcd_read

Reviewed-by: Sean Paul sean@poorly.run Signed-off-by: Lyude Paul lyude@redhat.com --- drivers/gpu/drm/drm_dp_helper.c | 46 +++++++++++++++++++++++++ drivers/gpu/drm/i915/display/intel_dp.c | 14 ++------ include/drm/drm_dp_helper.h | 3 ++ 3 files changed, 51 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c index 4c21cf69dad5a..4f845995f1f66 100644 --- a/drivers/gpu/drm/drm_dp_helper.c +++ b/drivers/gpu/drm/drm_dp_helper.c @@ -423,6 +423,52 @@ bool drm_dp_send_real_edid_checksum(struct drm_dp_aux *aux, } EXPORT_SYMBOL(drm_dp_send_real_edid_checksum);

+static u8 drm_dp_downstream_port_count(const u8 dpcd[DP_RECEIVER_CAP_SIZE]) +{ + u8 port_count = dpcd[DP_DOWN_STREAM_PORT_COUNT] & DP_PORT_COUNT_MASK; + + if (dpcd[DP_DOWNSTREAMPORT_PRESENT] & DP_DETAILED_CAP_INFO_AVAILABLE && port_count > 4) + port_count = 4; + + return port_count; +} + +/** + * drm_dp_downstream_read_info() - read DPCD downstream port info if available + * @aux: DisplayPort AUX channel + * @dpcd: A cached copy of the port's DPCD + * @downstream_ports: buffer to store the downstream port info in + * + * Returns: 0 if either the downstream port info was read successfully or + * there was no downstream info to read, or a negative error code otherwise. + */ +int drm_dp_downstream_read_info(struct drm_dp_aux *aux, + const u8 dpcd[DP_RECEIVER_CAP_SIZE], + u8 downstream_ports[DP_MAX_DOWNSTREAM_PORTS]) +{ + int ret; + u8 len; + + memset(downstream_ports, 0, DP_MAX_DOWNSTREAM_PORTS); + + /* No downstream info to read */ + if (!drm_dp_is_branch(dpcd) || + dpcd[DP_DPCD_REV] < DP_DPCD_REV_10 || + !(dpcd[DP_DOWNSTREAMPORT_PRESENT] & DP_DWN_STRM_PORT_PRESENT)) + return 0; + + len = drm_dp_downstream_port_count(dpcd); + if (dpcd[DP_DOWNSTREAMPORT_PRESENT] & DP_DETAILED_CAP_INFO_AVAILABLE) + len *= 4; + + ret = drm_dp_dpcd_read(aux, DP_DOWNSTREAM_PORT_0, downstream_ports, len); + if (ret < 0) + return ret; + + return ret == len ? 0 : -EIO; +} +EXPORT_SYMBOL(drm_dp_downstream_read_info); + /** * drm_dp_downstream_max_clock() - extract branch device max * pixel rate for legacy VGA diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c index 1e29d3a012856..984e49194ca31 100644 --- a/drivers/gpu/drm/i915/display/intel_dp.c +++ b/drivers/gpu/drm/i915/display/intel_dp.c @@ -4685,18 +4685,8 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp) return false; }

- if (!drm_dp_is_branch(intel_dp->dpcd)) - return true; /* native DP sink */ - - if (intel_dp->dpcd[DP_DPCD_REV] == 0x10) - return true; /* no per-port downstream info */ - - if (drm_dp_dpcd_read(&intel_dp->aux, DP_DOWNSTREAM_PORT_0, - intel_dp->downstream_ports, - DP_MAX_DOWNSTREAM_PORTS) < 0) - return false; /* downstream port status fetch failed */ - - return true; + return drm_dp_downstream_read_info(&intel_dp->aux, intel_dp->dpcd, + intel_dp->downstream_ports) == 0; }

static bool diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h index 5c28199248626..1349f16564ace 100644 --- a/include/drm/drm_dp_helper.h +++ b/include/drm/drm_dp_helper.h @@ -1613,6 +1613,9 @@ int drm_dp_dpcd_read_link_status(struct drm_dp_aux *aux, bool drm_dp_send_real_edid_checksum(struct drm_dp_aux *aux, u8 real_edid_checksum);

+int drm_dp_downstream_read_info(struct drm_dp_aux *aux, + const u8 dpcd[DP_RECEIVER_CAP_SIZE], + u8 downstream_ports[DP_MAX_DOWNSTREAM_PORTS]); int drm_dp_downstream_max_clock(const u8 dpcd[DP_RECEIVER_CAP_SIZE], const u8 port_cap[4]); int drm_dp_downstream_max_bpc(const u8 dpcd[DP_RECEIVER_CAP_SIZE],

Since other drivers are also going to need to be aware of the sink count in order to do proper dongle detection, we might as well steal i915's DP_SINK_COUNT helpers and move them into DRM helpers so that other dirvers can use them as well.

Note that this also starts using intel_dp_has_sink_count() in intel_dp_detect_dpcd(), which is a functional change.

Signed-off-by: Lyude Paul lyude@redhat.com Reviewed-by: Sean Paul sean@poorly.run --- drivers/gpu/drm/drm_dp_helper.c | 22 ++++++++++++++++++++++ drivers/gpu/drm/i915/display/intel_dp.c | 21 ++++++++++++--------- include/drm/drm_dp_helper.h | 8 +++++++- 3 files changed, 41 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c index 4f845995f1f66..863e0babc1903 100644 --- a/drivers/gpu/drm/drm_dp_helper.c +++ b/drivers/gpu/drm/drm_dp_helper.c @@ -714,6 +714,28 @@ void drm_dp_set_subconnector_property(struct drm_connector *connector, } EXPORT_SYMBOL(drm_dp_set_subconnector_property);

+/** + * drm_dp_has_sink_count() - Check whether a given connector has a valid sink + * count + * @connector: The DRM connector to check + * @dpcd: A cached copy of the connector's DPCD RX capabilities + * @desc: A cached copy of the connector's DP descriptor + * + * Returns: %True if the (e)DP connector has a valid sink count that should + * be probed, %false otherwise. + */ +bool drm_dp_has_sink_count(struct drm_connector *connector, + const u8 dpcd[DP_RECEIVER_CAP_SIZE], + const struct drm_dp_desc *desc) +{ + /* Some eDP panels don't set a valid value for the sink count */ + return connector->connector_type != DRM_MODE_CONNECTOR_eDP && + dpcd[DP_DPCD_REV] >= DP_DPCD_REV_11 && + dpcd[DP_DOWNSTREAMPORT_PRESENT] & DP_DWN_STRM_PORT_PRESENT && + !drm_dp_has_quirk(desc, 0, DP_DPCD_QUIRK_NO_SINK_COUNT); +} +EXPORT_SYMBOL(drm_dp_has_sink_count); + /* * I2C-over-AUX implementation */ diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c index 984e49194ca31..35a4779a442e2 100644 --- a/drivers/gpu/drm/i915/display/intel_dp.c +++ b/drivers/gpu/drm/i915/display/intel_dp.c @@ -4634,6 +4634,16 @@ intel_edp_init_dpcd(struct intel_dp *intel_dp) return true; }

+static bool +intel_dp_has_sink_count(struct intel_dp *intel_dp) +{ + if (!intel_dp->attached_connector) + return false; + + return drm_dp_has_sink_count(&intel_dp->attached_connector->base, + intel_dp->dpcd, + &intel_dp->desc); +}

static bool intel_dp_get_dpcd(struct intel_dp *intel_dp) @@ -4653,13 +4663,7 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp) intel_dp_set_common_rates(intel_dp); }

- /* - * Some eDP panels do not set a valid value for sink count, that is why - * it don't care about read it here and in intel_edp_init_dpcd(). - */ - if (!intel_dp_is_edp(intel_dp) && - !drm_dp_has_quirk(&intel_dp->desc, 0, - DP_DPCD_QUIRK_NO_SINK_COUNT)) { + if (intel_dp_has_sink_count(intel_dp)) { u8 count; ssize_t r;

@@ -5939,9 +5943,8 @@ intel_dp_detect_dpcd(struct intel_dp *intel_dp) return connector_status_connected;

/* If we're HPD-aware, SINK_COUNT changes dynamically */ - if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 && + if (intel_dp_has_sink_count(intel_dp) && intel_dp->downstream_ports[0] & DP_DS_PORT_HPD) { - return intel_dp->sink_count ? connector_status_connected : connector_status_disconnected; } diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h index 1349f16564ace..a1413a531eaf4 100644 --- a/include/drm/drm_dp_helper.h +++ b/include/drm/drm_dp_helper.h @@ -1631,6 +1631,11 @@ void drm_dp_set_subconnector_property(struct drm_connector *connector, const u8 *dpcd, const u8 port_cap[4]);

+struct drm_dp_desc; +bool drm_dp_has_sink_count(struct drm_connector *connector, + const u8 dpcd[DP_RECEIVER_CAP_SIZE], + const struct drm_dp_desc *desc); + void drm_dp_remote_aux_init(struct drm_dp_aux *aux); void drm_dp_aux_init(struct drm_dp_aux *aux); int drm_dp_aux_register(struct drm_dp_aux *aux); @@ -1689,7 +1694,8 @@ enum drm_dp_quirk { * @DP_DPCD_QUIRK_NO_SINK_COUNT: * * The device does not set SINK_COUNT to a non-zero value. - * The driver should ignore SINK_COUNT during detection. + * The driver should ignore SINK_COUNT during detection. Note that + * drm_dp_has_sink_count() automatically checks for this quirk. */ DP_DPCD_QUIRK_NO_SINK_COUNT, /**

Currently in nouveau_connector_ddc_detect() and nouveau_connector_detect_lvds(), we start the connector probing process by releasing the previous EDID and informing DRM of the change. However, since commit 5186421cbfe2 ("drm: Introduce epoch counter to drm_connector") drm_connector_update_edid_property() actually checks whether the new EDID we've specified is different from the previous one, and updates the connector's epoch accordingly if it is. But, because we always set the EDID to NULL first in nouveau_connector_ddc_detect() and nouveau_connector_detect_lvds() we end up making DRM think that the EDID changes every single time we do a connector probe - which isn't needed.

So, let's fix this by not clearing the EDID at the start of the connector probing process, and instead simply changing or removing it once near the end of the probing process. This will help prevent us from sending unneeded hotplug events to userspace when nothing has actually changed.

Signed-off-by: Lyude Paul lyude@redhat.com Reviewed-by: Ben Skeggs bskeggs@redhat.com --- drivers/gpu/drm/nouveau/nouveau_connector.c | 54 ++++++++++----------- 1 file changed, 27 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nouveau_connector.c b/drivers/gpu/drm/nouveau/nouveau_connector.c index 637e91594fbe8..49dd0cbc332ff 100644 --- a/drivers/gpu/drm/nouveau/nouveau_connector.c +++ b/drivers/gpu/drm/nouveau/nouveau_connector.c @@ -528,6 +528,17 @@ nouveau_connector_set_encoder(struct drm_connector *connector, } }

+static void +nouveau_connector_set_edid(struct nouveau_connector *nv_connector, + struct edid *edid) +{ + struct edid *old_edid = nv_connector->edid; + + drm_connector_update_edid_property(&nv_connector->base, edid); + kfree(old_edid); + nv_connector->edid = edid; +} + static enum drm_connector_status nouveau_connector_detect(struct drm_connector *connector, bool force) { @@ -541,13 +552,6 @@ nouveau_connector_detect(struct drm_connector *connector, bool force) int ret; enum drm_connector_status conn_status = connector_status_disconnected;

- /* Cleanup the previous EDID block. */ - if (nv_connector->edid) { - drm_connector_update_edid_property(connector, NULL); - kfree(nv_connector->edid); - nv_connector->edid = NULL; - } - /* Outputs are only polled while runtime active, so resuming the * device here is unnecessary (and would deadlock upon runtime suspend * because it waits for polling to finish). We do however, want to @@ -560,22 +564,23 @@ nouveau_connector_detect(struct drm_connector *connector, bool force) ret = pm_runtime_get_sync(dev->dev); if (ret < 0 && ret != -EACCES) { pm_runtime_put_autosuspend(dev->dev); + nouveau_connector_set_edid(nv_connector, NULL); return conn_status; } }

nv_encoder = nouveau_connector_ddc_detect(connector); if (nv_encoder && (i2c = nv_encoder->i2c) != NULL) { + struct edid *new_edid; + if ((vga_switcheroo_handler_flags() & VGA_SWITCHEROO_CAN_SWITCH_DDC) && nv_connector->type == DCB_CONNECTOR_LVDS) - nv_connector->edid = drm_get_edid_switcheroo(connector, - i2c); + new_edid = drm_get_edid_switcheroo(connector, i2c); else - nv_connector->edid = drm_get_edid(connector, i2c); + new_edid = drm_get_edid(connector, i2c);

- drm_connector_update_edid_property(connector, - nv_connector->edid); + nouveau_connector_set_edid(nv_connector, new_edid); if (!nv_connector->edid) { NV_ERROR(drm, "DDC responded, but no EDID for %s\n", connector->name); @@ -609,6 +614,8 @@ nouveau_connector_detect(struct drm_connector *connector, bool force) conn_status = connector_status_connected; drm_dp_cec_set_edid(&nv_connector->aux, nv_connector->edid); goto out; + } else { + nouveau_connector_set_edid(nv_connector, NULL); }

nv_encoder = nouveau_connector_of_detect(connector); @@ -652,18 +659,12 @@ nouveau_connector_detect_lvds(struct drm_connector *connector, bool force) struct nouveau_drm *drm = nouveau_drm(dev); struct nouveau_connector *nv_connector = nouveau_connector(connector); struct nouveau_encoder *nv_encoder = NULL; + struct edid *edid = NULL; enum drm_connector_status status = connector_status_disconnected;

- /* Cleanup the previous EDID block. */ - if (nv_connector->edid) { - drm_connector_update_edid_property(connector, NULL); - kfree(nv_connector->edid); - nv_connector->edid = NULL; - } - nv_encoder = find_encoder(connector, DCB_OUTPUT_LVDS); if (!nv_encoder) - return connector_status_disconnected; + goto out;

/* Try retrieving EDID via DDC */ if (!drm->vbios.fp_no_ddc) { @@ -682,7 +683,8 @@ nouveau_connector_detect_lvds(struct drm_connector *connector, bool force) * valid - it's not (rh#613284) */ if (nv_encoder->dcb->lvdsconf.use_acpi_for_edid) { - if ((nv_connector->edid = nouveau_acpi_edid(dev, connector))) { + edid = nouveau_acpi_edid(dev, connector); + if (edid) { status = connector_status_connected; goto out; } @@ -702,12 +704,10 @@ nouveau_connector_detect_lvds(struct drm_connector *connector, bool force) * stored for the panel stored in them. */ if (!drm->vbios.fp_no_ddc) { - struct edid *edid = - (struct edid *)nouveau_bios_embedded_edid(dev); + edid = (struct edid *)nouveau_bios_embedded_edid(dev); if (edid) { - nv_connector->edid = - kmemdup(edid, EDID_LENGTH, GFP_KERNEL); - if (nv_connector->edid) + edid = kmemdup(edid, EDID_LENGTH, GFP_KERNEL); + if (edid) status = connector_status_connected; } } @@ -720,7 +720,7 @@ nouveau_connector_detect_lvds(struct drm_connector *connector, bool force) status = connector_status_unknown; #endif

- drm_connector_update_edid_property(connector, nv_connector->edid); + nouveau_connector_set_edid(nv_connector, edid); nouveau_connector_set_encoder(connector, nv_encoder); return status; }

Now that we've extracted i915's code for reading both the normal DPCD caps and extended DPCD caps into a shared helper, let's start using this in nouveau to enable us to start checking extended DPCD caps for free.

Signed-off-by: Lyude Paul lyude@redhat.com Reviewed-by: Ben Skeggs bskeggs@redhat.com --- drivers/gpu/drm/nouveau/nouveau_dp.c | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nouveau_dp.c b/drivers/gpu/drm/nouveau/nouveau_dp.c index 89afc97ee2591..271a0a863a0e1 100644 --- a/drivers/gpu/drm/nouveau/nouveau_dp.c +++ b/drivers/gpu/drm/nouveau/nouveau_dp.c @@ -55,15 +55,13 @@ nouveau_dp_probe_dpcd(struct nouveau_connector *nv_connector, int ret; u8 *dpcd = outp->dp.dpcd;

- ret = drm_dp_dpcd_read(aux, DP_DPCD_REV, dpcd, DP_RECEIVER_CAP_SIZE); - if (ret == DP_RECEIVER_CAP_SIZE && dpcd[DP_DPCD_REV]) { - ret = drm_dp_read_desc(aux, &outp->dp.desc, - drm_dp_is_branch(dpcd)); - if (ret < 0) - goto out; - } else { + ret = drm_dp_read_dpcd_caps(aux, dpcd); + if (ret < 0) + goto out; + + ret = drm_dp_read_desc(aux, &outp->dp.desc, drm_dp_is_branch(dpcd)); + if (ret < 0) goto out; - }

if (nouveau_mst) { mstm = outp->dp.mstm;