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

[Pandiyan, Dhinakaran]

On Wed, 2017-01-25 at 07:15 +0100, Daniel Vetter wrote:
> 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.
> 


Yeah, you are right. I thought of this problem and then forgot about it.
Is there any igt test to test the switching?


-DK
> >  			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
> > 
>