On Fri, Oct 26, 2018 at 04:35:49PM -0400, Lyude Paul wrote: > Currently, nouveau uses the yolo method of setting up MST displays: it > uses the old VCPI helpers (drm_dp_find_vcpi_slots()) for computing the > display configuration. These helpers don't take care to make sure they > take a reference to the mstb port that they're checking, and > additionally don't actually check whether or not the topology still has > enough bandwidth to provide the VCPI tokens required. > > So, drop usage of the old helpers and move entirely over to the atomic > helpers. > > Signed-off-by: Lyude Paul <lyude@xxxxxxxxxx> > Cc: Daniel Vetter <daniel.vetter@xxxxxxxx> > --- > drivers/gpu/drm/nouveau/dispnv50/disp.c | 54 +++++++++++++++++++++---- > 1 file changed, 47 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c > index 6cbbae3f438b..37503319e5b1 100644 > --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c > +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c > @@ -747,16 +747,22 @@ nv50_msto_atomic_check(struct drm_encoder *encoder, > struct drm_crtc_state *crtc_state, > struct drm_connector_state *conn_state) > { > - struct nv50_mstc *mstc = nv50_mstc(conn_state->connector); > + struct drm_atomic_state *state = crtc_state->state; > + struct drm_connector *connector = conn_state->connector; > + struct nv50_mstc *mstc = nv50_mstc(connector); > struct nv50_mstm *mstm = mstc->mstm; > - int bpp = conn_state->connector->display_info.bpc * 3; > + int bpp = connector->display_info.bpc * 3; > int slots; > > - mstc->pbn = drm_dp_calc_pbn_mode(crtc_state->adjusted_mode.clock, bpp); > - > - slots = drm_dp_find_vcpi_slots(&mstm->mgr, mstc->pbn); > - if (slots < 0) > - return slots; > + mstc->pbn = drm_dp_calc_pbn_mode(crtc_state->adjusted_mode.clock, > + bpp); > + /* Zombies don't need VCPI */ > + if (!drm_connector_is_unregistered(connector)) { > + slots = drm_dp_atomic_find_vcpi_slots(state, &mstm->mgr, > + mstc->port, mstc->pbn); > + if (slots < 0) > + return slots; > + } > > return nv50_outp_atomic_check_view(encoder, crtc_state, conn_state, > mstc->native); > @@ -920,12 +926,38 @@ nv50_mstc_get_modes(struct drm_connector *connector) > return ret; > } > > +static int > +nv50_mstc_atomic_check(struct drm_connector *connector, > + struct drm_connector_state *new_conn_state) > +{ > + struct drm_atomic_state *state = new_conn_state->state; > + struct nv50_mstc *mstc = nv50_mstc(connector); > + struct drm_dp_mst_topology_mgr *mgr = &mstc->mstm->mgr; > + struct drm_connector_state *old_conn_state; > + struct drm_crtc *old_crtc; > + > + old_conn_state = drm_atomic_get_old_connector_state(state, connector); > + old_crtc = old_conn_state->crtc; > + > + /* We only need to release VCPI here if we're going from having a CRTC > + * on this connector, to not having one > + */ > + if (!old_crtc || new_conn_state->crtc) > + return 0; > + > + /* Release the previous VCPI allocation since the encoder's > + * atomic_check() won't be called > + */ > + return drm_dp_atomic_release_vcpi_slots(state, mgr, mstc->port); > +} > + > static const struct drm_connector_helper_funcs > nv50_mstc_help = { > .get_modes = nv50_mstc_get_modes, > .mode_valid = nv50_mstc_mode_valid, > .best_encoder = nv50_mstc_best_encoder, > .atomic_best_encoder = nv50_mstc_atomic_best_encoder, > + .atomic_check = nv50_mstc_atomic_check, > }; > > static enum drm_connector_status > @@ -2084,6 +2116,8 @@ nv50_disp_atomic_check(struct drm_device *dev, struct drm_atomic_state *state) > struct drm_connector *connector; > struct drm_crtc_state *new_crtc_state; > struct drm_crtc *crtc; > + struct drm_dp_mst_topology_mgr *mgr; > + struct drm_dp_mst_topology_state *mst_state; > int ret, i; > > /* We need to handle colour management on a per-plane basis. */ > @@ -2109,6 +2143,12 @@ nv50_disp_atomic_check(struct drm_device *dev, struct drm_atomic_state *state) > return ret; > } > > + for_each_new_mst_mgr_in_state(state, mgr, mst_state, i) { > + ret = drm_dp_mst_atomic_check(mst_state); > + if (ret) > + return ret; > + } For even less code I think you could also push this loop into the helpers. I can't come up with any scenario where a driver does not want to check all of them at the same time in the atomic_check sequence. Otherwise lgtm. -Daniel > + > return 0; > } > > -- > 2.17.2 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel