Re: [PATCH v2 4/4] drm/nouveau: Use atomic VCPI helpers for MST

[Daniel Vetter <daniel@ffwll.ch>]

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@redhat.com>
> Cc: Daniel Vetter <daniel.vetter@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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel