[Intel-gfx] [PATCH 1/3] drm/i915/dp: Fail DP MST config when there are not enough vcpi slots

[Daniel Vetter]

On Sat, Nov 19, 2016 at 12:04:32AM +0000, Pandiyan, Dhinakaran wrote:
> On Fri, 2016-11-18 at 09:43 +0100, Daniel Vetter wrote:
> > On Thu, Nov 17, 2016 at 06:03:46PM -0800, Dhinakaran Pandiyan wrote:
> > > drm_dp_find_vcpi_slots() returns an error when there is not enough
> > > available bandwidth on a link to support a mode. This error should make
> > > compute_config() to fail. Not returning false could end up in a modeset
> > > which will not work.
> > > 
> > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan at intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_dp_mst.c | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
> > > index e21cf08..4280a83 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> > > @@ -63,6 +63,10 @@ static bool intel_dp_mst_compute_config(struct intel_encoder *encoder,
> > >  
> > >  	pipe_config->pbn = mst_pbn;
> > >  	slots = drm_dp_find_vcpi_slots(&intel_dp->mst_mgr, mst_pbn);
> > > +	if (slots < 0) {
> > > +		DRM_ERROR("not enough available time slots for pbn=%d", mst_pbn);
> > 
> > No DRM_ERROR for cases that are expected to fail, i.e. DRM_DEBUG_KMS is
> > the right level.
> > 
> It'd be nice to document the usage somewhere. There seems to be several
> not very obvious reasons to choose one over the other. I can volunteer
> to write it but I am not getting it right as it's evident here.

Aw yes! I definitely want this better documented, and I can help out with
it for sure. Probably best we discuss this over irc, but would be really
awesome if you volunteer here (if just for the learning experience).

> > And I don't think this works correctly either. Assume you do an atomic
> > modeset where you enable 2 mst sinks at the same time, and the following
> > happens:
> > - mst connector 1 can be allocated, and passes
> >   intel_dp_mst_compute_config.
> > - mst connector 2 can be allocated, but not together with connector 1.
> >   But drm_dp_find_vcpi_slots only checks what's available, it doesn't do a
> >   temporary reservation.
> > 
> > And we can just reserve the slot in drm_dp_find_vcpi_slots, because then
> > in the above case (when connector 2 doesn't have enough slots anymore)
> > we'd leak the vcpi allocation for connector 1.
> > 
> > Even worse, when we do a TEST_ONLY atomic commit (i.e. only run
> > atomic_check/compute_config code, but not commit anything) this can even
> > happen for a successful commit.
> > 
> > Long story short: To fix this properly we need to rewrite the dp helpers
> > to be compliant with atomic design. I think for that we roughly need:
> > 
> > - Exract vcpi allocations into a free-standing state structure. I'd call
> >   it drm_dp_mst_state or similar. Provide duplicate(get_state)/release
> >   functions like we already have for plane, connector and crtc states in
> >   the core, and e.g. dpll configuration in i915/intel_dpll_mgr.c. Ander
> >   Conselvan can help you with this. I'm also planning to write better
> >   documentation for how to do this exactly (since you also need a ww_mutex
> >   to protect this state), and I'll prioritize that work.
> > 
> > - Wire up that dp mst state at the right places globally (we need one slot
> >   per drm_dp_mst_topology_mgr, i.e. per port), and duplicate that state in
> >   intel_dp_mst_compute_config. We can't wire this up anywhere in the core
> >   since the dp mst library is a helper library, so all the integration
> >   points need to be done explicitly in drivers.
> > 
> > - Do the same for nouveau - nouveau is now also atomic, and it also is
> >   atomic with mst support.
> > 
> > - While doing all that make sure that the existing (non-atomic-compliant)
> >   functions in the dp mst helpers keep working, we need those for amggpu.
> > 
> > - Create a new drm_dp_state_allocate_vcpi_slots, which only touches the
> >   new drm_dp_mst_state structure and allocats the vcpi slots there. Also
> >   add some function to find the allocation for each sink again (we need
> >   that in the modeset commit functions).
> > 
> 
> Let me rephrase so that I am sure I understand this.
> With the way that I am doing this (assuming the mutex bug in Patch 3/3
> is fixed), we'll still end up passing compute_config() but fail modeset
> in the scenario you pointed out. This is because the slots are not
> reserved in drm_dp_find_vcpi_slots().
> 
> However, if we do reserve the slots for connector1 in
> drm_dp_find_vcpi_slots() and fail compute_config() for connector2, then
> the vcpi slots that were assigned to connector1 are not released.
> 
> But, by having drm_dp_mst_state, we can simulate vcpi slot allocation in
> the atomic_check()/compute_config() phase and fail without committing,
> while also releasing the slots. 

Yup, that's the idea. And like I promised I'll try to document the general
principles of atomic a bit better, so that's it's easier to extend this
state handling concept to new things like mst. Rob Clark as me last week
about how to handle shared state between his hw plans, and we already have
special state to handle our shared dplls in i915, so there's demand for
this ;-)

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