[Intel-gfx] [PATCH 1/3] drm/i915/dp: Fail DP MST config when there are not enough vcpi slots
On Sat, Nov 19, 2016 at 12:04:32AM +, 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
> > > ---
> > > 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(_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
[Intel-gfx] [PATCH 1/3] drm/i915/dp: Fail DP MST config when there are not enough vcpi slots
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
> > ---
> > 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(_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.
> 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.
-DK
> - Rework our compute_config and modeset code to use this new function, and
> not the old legacy find/allocate functions.
>
> To make this happen we need buy-in from Ben Skeggs (nouveau maintainer)
> and preferrably also from the AMD display team (since they support mst
> already, and also plan to eventually support atomic).
>
> Fixing this correctly is unfortunately a _lot_ more work than your simple
> patch here :(
> -Daniel
