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 <ly...@redhat.com>
> Cc: Daniel Vetter <daniel.vet...@ffwll.ch>
> ---
> 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
