[Intel-gfx] [PATCH v2 9/9] drm/dp: Track MST link bandwidth

[Daniel Vetter]

On Tue, Jan 24, 2017 at 03:49:37PM -0800, Dhinakaran Pandiyan wrote:
> Use the added helpers to track MST link bandwidth for atomic modesets.
> Link bw is acquired in the ->atomic_check() phase when CRTCs are being
> enabled with drm_atomic_find_vcpi_slots() instead of drm_find_vcpi_slots().
> Similarly, link bw is released during ->atomic_check() with the connector
> helper callback ->atomic_release() when CRTCs are disabled.
> 
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan at intel.com>
> ---
>  drivers/gpu/drm/drm_atomic_helper.c |  9 ++++++++-
>  drivers/gpu/drm/i915/intel_dp_mst.c | 13 ++++++++++++-
>  2 files changed, 20 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index dd34d21..0d42173 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -385,8 +385,15 @@ mode_fixup(struct drm_atomic_state *state)
>  
>  		WARN_ON(!!conn_state->best_encoder != !!conn_state->crtc);
>  
> -		if (!conn_state->crtc || !conn_state->best_encoder)
> +		if (!conn_state->crtc || !conn_state->best_encoder) {
> +			const struct drm_connector_helper_funcs *conn_funcs;
> +
> +			conn_funcs = connector->helper_private;
> +			if (conn_funcs->atomic_release)
> +				conn_funcs->atomic_release(connector,
> +							   conn_state);

I wonder whether this won't surprise drivers: The way you coded this
release only gets called when the connector is fully disabled. But it does
not get called when you atomically switch it from one crtc to the other
(in which case you also might want to release resources, before allocating
new ones). I think that case of directly switching stuff is even a bug in
your current atomic tracking code ...

We also need to extract the release calls into a separate loop which runs
_before_ we allocate any resources. Otherwise if you do an atomic commit
where you disable connector2 and enable connector1 and they can't run both
at the same time it'll fail, because we'll release the vcpi for connector2
only after we've tried to get it for connnector1.

>  			continue;
> +		}
>  
>  		crtc_state = drm_atomic_get_existing_crtc_state(state,
>  								conn_state->crtc);

Misplaced hunk, this needs to be in the patch that adds the
->atomic_release callback.

> diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
> index 2f57a56..ee5fedb 100644
> --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> @@ -44,6 +44,7 @@ static bool intel_dp_mst_compute_config(struct intel_encoder *encoder,
>  	int lane_count, slots;
>  	const struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode;
>  	int mst_pbn;
> +	struct drm_dp_mst_topology_state *topology_state;
>  
>  	pipe_config->has_pch_encoder = false;
>  	bpp = 24;
> @@ -65,7 +66,17 @@ static bool intel_dp_mst_compute_config(struct intel_encoder *encoder,
>  	mst_pbn = drm_dp_calc_pbn_mode(adjusted_mode->crtc_clock, bpp);
>  
>  	pipe_config->pbn = mst_pbn;
> -	slots = drm_dp_find_vcpi_slots(&intel_dp->mst_mgr, mst_pbn);
> +	topology_state = drm_atomic_get_mst_topology_state(state,
> +							   &intel_dp->mst_mgr);
> +	if (topology_state == NULL)
> +		return false;
> +
> +	slots = drm_dp_atomic_find_vcpi_slots(topology_state, connector->port,
> +					      mst_pbn);

Can we merge the get_mst_topology_state and find_vcpi_slots functions
together? Would be less code in drivers ...

> +	if (slots < 0) {
> +		DRM_DEBUG_KMS("not enough link bw for this mode\n");
> +		return false;
> +	}

And then also stuff the error checking in there, and just have a return
-EINVAL when there's not enough bw.

I think this should be together with the previous patch, to wire up the
entire mst state tracking for i915 at the same time. Atm the previous
patch just wires up dead code, which is a bit backwards.
-Daniel

>  
>  	intel_link_compute_m_n(bpp, lane_count,
>  			       adjusted_mode->crtc_clock,
> -- 
> 2.7.4
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch