Re: [Intel-gfx] [PATCH v3 2/5] drm/dp_mst: Start tracking per-port VCPI allocations

2018-11-07 Thread Lyude Paul 
On Wed, 2018-11-07 at 22:59 +0100, Daniel Vetter wrote:
> On Wed, Nov 07, 2018 at 04:39:57PM -0500, Lyude Paul wrote:
> > On Wed, 2018-11-07 at 16:23 -0500, Lyude Paul wrote:
> > > On Wed, 2018-11-07 at 21:59 +0100, Daniel Vetter wrote:
> > > > On Tue, Nov 06, 2018 at 08:21:14PM -0500, Lyude Paul wrote:
> > > > > There has been a TODO waiting for quite a long time in
> > > > > drm_dp_mst_topology.c:
> > > > > 
> > > > >   /* We cannot rely on port->vcpi.num_slots to update
> > > > >* topology_state->avail_slots as the port may not exist if
> > > > > the parent
> > > > >* branch device was unplugged. This should be fixed by
> > > > > tracking
> > > > >* per-port slot allocation in drm_dp_mst_topology_state
> > > > > instead of
> > > > >* depending on the caller to tell us how many slots to
> > > > > release.
> > > > >*/
> > > > > 
> > > > > That's not the only reason we should fix this: forcing the driver to
> > > > > track the VCPI allocations throughout a state's atomic check is
> > > > > error prone, because it means that extra care has to be taken with
> > > > > the
> > > > > order that drm_dp_atomic_find_vcpi_slots() and
> > > > > drm_dp_atomic_release_vcpi_slots() are called in in order to ensure
> > > > > idempotency. Currently the only driver actually using these helpers,
> > > > > i915, doesn't even do this correctly: multiple ->best_encoder()
> > > > > checks
> > > > > with i915's current implementation would not be idempotent and would
> > > > > over-allocate VCPI slots, something I learned trying to implement
> > > > > fallback retraining in MST.
> > > > > 
> > > > > So: simplify this whole mess, and teach
> > > > > drm_dp_atomic_find_vcpi_slots()
> > > > > and drm_dp_atomic_release_vcpi_slots() to track the VCPI allocations
> > > > > for
> > > > > each port. This allows us to ensure idempotency without having to
> > > > > rely
> > > > > on the driver as much. Additionally: the driver doesn't need to do
> > > > > any
> > > > > kind of VCPI slot tracking anymore if it doesn't need it for it's
> > > > > own
> > > > > internal state.
> > > > > 
> > > > > Additionally; this adds a new drm_dp_mst_atomic_check() helper which
> > > > > must be used by atomic drivers to perform validity checks for the
> > > > > new
> > > > > VCPI allocations incurred by a state.
> > > > > 
> > > > > Also: update the documentation and make it more obvious that these
> > > > > /must/ be called by /all/ atomic drivers supporting MST.
> > > > > 
> > > > > Changes since v2:
> > > > >  - Use kmemdup() for duplicating MST state - danvet
> > > > >  - Move port validation out of duplicate state callback - danvet
> > > > >  - Handle looping through MST topology states in
> > > > >drm_dp_mst_atomic_check() so the driver doesn't have to do it
> > > > >  - Fix documentation in drm_dp_atomic_find_vcpi_slots()
> > > > >  - Move the atomic check for each individual topology state into
> > > > > it's
> > > > >own function, reduces indenting
> > > > >  - Don't consider "stale" MST ports when calculating the bandwidth
> > > > >requirements. This is needed because originally we relied on the
> > > > >state duplication functions to prune any stale ports from the new
> > > > >state, which would prevent us from incorrectly considering their
> > > > >bandwidth requirements alongside legitimate new payloads.
> > > > >  - Add function references in drm_dp_atomic_release_vcpi_slots() -
> > > > > danvet
> > > > >  - Annotate atomic VCPI and atomic check functions with __must_check
> > > > >- danvet
> > > > > 
> > > > > Changes since v1:
> > > > >  - Don't use the now-removed ->atomic_check() for private objects
> > > > > hook,
> > > > >just give drivers a function to call themselves
> > > > > 
> > > > > Signed-off-by: Lyude Paul 
> > > > > Cc: Daniel Vetter 
> > > > > ---
> > > > >  drivers/gpu/drm/drm_dp_mst_topology.c | 222 ++-
> > > > > ---
> > > > >  drivers/gpu/drm/i915/intel_display.c  |   4 +
> > > > >  drivers/gpu/drm/i915/intel_dp_mst.c   |  31 ++--
> > > > >  include/drm/drm_dp_mst_helper.h   |  23 ++-
> > > > >  4 files changed, 225 insertions(+), 55 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> > > > > b/drivers/gpu/drm/drm_dp_mst_topology.c
> > > > > index 8c3cfac437f4..74823afb262e 100644
> > > > > --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> > > > > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> > > > > @@ -2614,21 +2614,34 @@ static int drm_dp_init_vcpi(struct
> > > > > drm_dp_mst_topology_mgr *mgr,
> > > > >  }
> > > > >  
> > > > >  /**
> > > > > - * drm_dp_atomic_find_vcpi_slots() - Find and add vcpi slots to the
> > > > > state
> > > > > + * drm_dp_atomic_find_vcpi_slots() - Find and add VCPI slots to the
> > > > > state
> > > > >   * @state: global atomic state
> > > > >   * @mgr: MST topology manager for the port
> > > > >   * @port: port to find vcpi slots for
> > > > >   * @pbn:

Re: [Intel-gfx] [PATCH v3 2/5] drm/dp_mst: Start tracking per-port VCPI allocations

2018-11-07 Thread Daniel Vetter 
On Wed, Nov 07, 2018 at 04:39:57PM -0500, Lyude Paul wrote:
> On Wed, 2018-11-07 at 16:23 -0500, Lyude Paul wrote:
> > On Wed, 2018-11-07 at 21:59 +0100, Daniel Vetter wrote:
> > > On Tue, Nov 06, 2018 at 08:21:14PM -0500, Lyude Paul wrote:
> > > > There has been a TODO waiting for quite a long time in
> > > > drm_dp_mst_topology.c:
> > > > 
> > > > /* We cannot rely on port->vcpi.num_slots to update
> > > >  * topology_state->avail_slots as the port may not exist if the 
> > > > parent
> > > >  * branch device was unplugged. This should be fixed by tracking
> > > >  * per-port slot allocation in drm_dp_mst_topology_state 
> > > > instead of
> > > >  * depending on the caller to tell us how many slots to release.
> > > >  */
> > > > 
> > > > That's not the only reason we should fix this: forcing the driver to
> > > > track the VCPI allocations throughout a state's atomic check is
> > > > error prone, because it means that extra care has to be taken with the
> > > > order that drm_dp_atomic_find_vcpi_slots() and
> > > > drm_dp_atomic_release_vcpi_slots() are called in in order to ensure
> > > > idempotency. Currently the only driver actually using these helpers,
> > > > i915, doesn't even do this correctly: multiple ->best_encoder() checks
> > > > with i915's current implementation would not be idempotent and would
> > > > over-allocate VCPI slots, something I learned trying to implement
> > > > fallback retraining in MST.
> > > > 
> > > > So: simplify this whole mess, and teach drm_dp_atomic_find_vcpi_slots()
> > > > and drm_dp_atomic_release_vcpi_slots() to track the VCPI allocations for
> > > > each port. This allows us to ensure idempotency without having to rely
> > > > on the driver as much. Additionally: the driver doesn't need to do any
> > > > kind of VCPI slot tracking anymore if it doesn't need it for it's own
> > > > internal state.
> > > > 
> > > > Additionally; this adds a new drm_dp_mst_atomic_check() helper which
> > > > must be used by atomic drivers to perform validity checks for the new
> > > > VCPI allocations incurred by a state.
> > > > 
> > > > Also: update the documentation and make it more obvious that these
> > > > /must/ be called by /all/ atomic drivers supporting MST.
> > > > 
> > > > Changes since v2:
> > > >  - Use kmemdup() for duplicating MST state - danvet
> > > >  - Move port validation out of duplicate state callback - danvet
> > > >  - Handle looping through MST topology states in
> > > >drm_dp_mst_atomic_check() so the driver doesn't have to do it
> > > >  - Fix documentation in drm_dp_atomic_find_vcpi_slots()
> > > >  - Move the atomic check for each individual topology state into it's
> > > >own function, reduces indenting
> > > >  - Don't consider "stale" MST ports when calculating the bandwidth
> > > >requirements. This is needed because originally we relied on the
> > > >state duplication functions to prune any stale ports from the new
> > > >state, which would prevent us from incorrectly considering their
> > > >bandwidth requirements alongside legitimate new payloads.
> > > >  - Add function references in drm_dp_atomic_release_vcpi_slots() -
> > > > danvet
> > > >  - Annotate atomic VCPI and atomic check functions with __must_check
> > > >- danvet
> > > > 
> > > > Changes since v1:
> > > >  - Don't use the now-removed ->atomic_check() for private objects hook,
> > > >just give drivers a function to call themselves
> > > > 
> > > > Signed-off-by: Lyude Paul 
> > > > Cc: Daniel Vetter 
> > > > ---
> > > >  drivers/gpu/drm/drm_dp_mst_topology.c | 222 ++
> > > >  drivers/gpu/drm/i915/intel_display.c  |   4 +
> > > >  drivers/gpu/drm/i915/intel_dp_mst.c   |  31 ++--
> > > >  include/drm/drm_dp_mst_helper.h   |  23 ++-
> > > >  4 files changed, 225 insertions(+), 55 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> > > > b/drivers/gpu/drm/drm_dp_mst_topology.c
> > > > index 8c3cfac437f4..74823afb262e 100644
> > > > --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> > > > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> > > > @@ -2614,21 +2614,34 @@ static int drm_dp_init_vcpi(struct
> > > > drm_dp_mst_topology_mgr *mgr,
> > > >  }
> > > >  
> > > >  /**
> > > > - * drm_dp_atomic_find_vcpi_slots() - Find and add vcpi slots to the
> > > > state
> > > > + * drm_dp_atomic_find_vcpi_slots() - Find and add VCPI slots to the
> > > > state
> > > >   * @state: global atomic state
> > > >   * @mgr: MST topology manager for the port
> > > >   * @port: port to find vcpi slots for
> > > >   * @pbn: bandwidth required for the mode in PBN
> > > >   *
> > > > - * RETURNS:
> > > > - * Total slots in the atomic state assigned for this port or error
> > > > + * Allocates VCPI slots to @port, replacing any previous VCPI
> > > > allocations
> > > > it
> > > > + * may have had. Any atomic drivers which support MST must call this
> > > > function
> > > > + *

Re: [Intel-gfx] [PATCH v3 2/5] drm/dp_mst: Start tracking per-port VCPI allocations

2018-11-07 Thread Lyude Paul 
On Wed, 2018-11-07 at 16:23 -0500, Lyude Paul wrote:
> On Wed, 2018-11-07 at 21:59 +0100, Daniel Vetter wrote:
> > On Tue, Nov 06, 2018 at 08:21:14PM -0500, Lyude Paul wrote:
> > > There has been a TODO waiting for quite a long time in
> > > drm_dp_mst_topology.c:
> > > 
> > >   /* We cannot rely on port->vcpi.num_slots to update
> > >* topology_state->avail_slots as the port may not exist if the parent
> > >* branch device was unplugged. This should be fixed by tracking
> > >* per-port slot allocation in drm_dp_mst_topology_state instead of
> > >* depending on the caller to tell us how many slots to release.
> > >*/
> > > 
> > > That's not the only reason we should fix this: forcing the driver to
> > > track the VCPI allocations throughout a state's atomic check is
> > > error prone, because it means that extra care has to be taken with the
> > > order that drm_dp_atomic_find_vcpi_slots() and
> > > drm_dp_atomic_release_vcpi_slots() are called in in order to ensure
> > > idempotency. Currently the only driver actually using these helpers,
> > > i915, doesn't even do this correctly: multiple ->best_encoder() checks
> > > with i915's current implementation would not be idempotent and would
> > > over-allocate VCPI slots, something I learned trying to implement
> > > fallback retraining in MST.
> > > 
> > > So: simplify this whole mess, and teach drm_dp_atomic_find_vcpi_slots()
> > > and drm_dp_atomic_release_vcpi_slots() to track the VCPI allocations for
> > > each port. This allows us to ensure idempotency without having to rely
> > > on the driver as much. Additionally: the driver doesn't need to do any
> > > kind of VCPI slot tracking anymore if it doesn't need it for it's own
> > > internal state.
> > > 
> > > Additionally; this adds a new drm_dp_mst_atomic_check() helper which
> > > must be used by atomic drivers to perform validity checks for the new
> > > VCPI allocations incurred by a state.
> > > 
> > > Also: update the documentation and make it more obvious that these
> > > /must/ be called by /all/ atomic drivers supporting MST.
> > > 
> > > Changes since v2:
> > >  - Use kmemdup() for duplicating MST state - danvet
> > >  - Move port validation out of duplicate state callback - danvet
> > >  - Handle looping through MST topology states in
> > >drm_dp_mst_atomic_check() so the driver doesn't have to do it
> > >  - Fix documentation in drm_dp_atomic_find_vcpi_slots()
> > >  - Move the atomic check for each individual topology state into it's
> > >own function, reduces indenting
> > >  - Don't consider "stale" MST ports when calculating the bandwidth
> > >requirements. This is needed because originally we relied on the
> > >state duplication functions to prune any stale ports from the new
> > >state, which would prevent us from incorrectly considering their
> > >bandwidth requirements alongside legitimate new payloads.
> > >  - Add function references in drm_dp_atomic_release_vcpi_slots() -
> > > danvet
> > >  - Annotate atomic VCPI and atomic check functions with __must_check
> > >- danvet
> > > 
> > > Changes since v1:
> > >  - Don't use the now-removed ->atomic_check() for private objects hook,
> > >just give drivers a function to call themselves
> > > 
> > > Signed-off-by: Lyude Paul 
> > > Cc: Daniel Vetter 
> > > ---
> > >  drivers/gpu/drm/drm_dp_mst_topology.c | 222 ++
> > >  drivers/gpu/drm/i915/intel_display.c  |   4 +
> > >  drivers/gpu/drm/i915/intel_dp_mst.c   |  31 ++--
> > >  include/drm/drm_dp_mst_helper.h   |  23 ++-
> > >  4 files changed, 225 insertions(+), 55 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> > > b/drivers/gpu/drm/drm_dp_mst_topology.c
> > > index 8c3cfac437f4..74823afb262e 100644
> > > --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> > > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> > > @@ -2614,21 +2614,34 @@ static int drm_dp_init_vcpi(struct
> > > drm_dp_mst_topology_mgr *mgr,
> > >  }
> > >  
> > >  /**
> > > - * drm_dp_atomic_find_vcpi_slots() - Find and add vcpi slots to the
> > > state
> > > + * drm_dp_atomic_find_vcpi_slots() - Find and add VCPI slots to the
> > > state
> > >   * @state: global atomic state
> > >   * @mgr: MST topology manager for the port
> > >   * @port: port to find vcpi slots for
> > >   * @pbn: bandwidth required for the mode in PBN
> > >   *
> > > - * RETURNS:
> > > - * Total slots in the atomic state assigned for this port or error
> > > + * Allocates VCPI slots to @port, replacing any previous VCPI
> > > allocations
> > > it
> > > + * may have had. Any atomic drivers which support MST must call this
> > > function
> > > + * in their _encoder_helper_funcs.atomic_check() callback to change
> > > the
> > > + * current VCPI allocation for the new state. The allocations are not
> > > checked
> > > + * against the bandwidth restraints of @mgr until the driver calls
> > > + * drm_dp_mst_atomic_check().
> > > + *
> > > + * See also:

Re: [Intel-gfx] [PATCH v3 2/5] drm/dp_mst: Start tracking per-port VCPI allocations

2018-11-07 Thread Lyude Paul 
On Wed, 2018-11-07 at 21:59 +0100, Daniel Vetter wrote:
> On Tue, Nov 06, 2018 at 08:21:14PM -0500, Lyude Paul wrote:
> > There has been a TODO waiting for quite a long time in
> > drm_dp_mst_topology.c:
> > 
> > /* We cannot rely on port->vcpi.num_slots to update
> >  * topology_state->avail_slots as the port may not exist if the parent
> >  * branch device was unplugged. This should be fixed by tracking
> >  * per-port slot allocation in drm_dp_mst_topology_state instead of
> >  * depending on the caller to tell us how many slots to release.
> >  */
> > 
> > That's not the only reason we should fix this: forcing the driver to
> > track the VCPI allocations throughout a state's atomic check is
> > error prone, because it means that extra care has to be taken with the
> > order that drm_dp_atomic_find_vcpi_slots() and
> > drm_dp_atomic_release_vcpi_slots() are called in in order to ensure
> > idempotency. Currently the only driver actually using these helpers,
> > i915, doesn't even do this correctly: multiple ->best_encoder() checks
> > with i915's current implementation would not be idempotent and would
> > over-allocate VCPI slots, something I learned trying to implement
> > fallback retraining in MST.
> > 
> > So: simplify this whole mess, and teach drm_dp_atomic_find_vcpi_slots()
> > and drm_dp_atomic_release_vcpi_slots() to track the VCPI allocations for
> > each port. This allows us to ensure idempotency without having to rely
> > on the driver as much. Additionally: the driver doesn't need to do any
> > kind of VCPI slot tracking anymore if it doesn't need it for it's own
> > internal state.
> > 
> > Additionally; this adds a new drm_dp_mst_atomic_check() helper which
> > must be used by atomic drivers to perform validity checks for the new
> > VCPI allocations incurred by a state.
> > 
> > Also: update the documentation and make it more obvious that these
> > /must/ be called by /all/ atomic drivers supporting MST.
> > 
> > Changes since v2:
> >  - Use kmemdup() for duplicating MST state - danvet
> >  - Move port validation out of duplicate state callback - danvet
> >  - Handle looping through MST topology states in
> >drm_dp_mst_atomic_check() so the driver doesn't have to do it
> >  - Fix documentation in drm_dp_atomic_find_vcpi_slots()
> >  - Move the atomic check for each individual topology state into it's
> >own function, reduces indenting
> >  - Don't consider "stale" MST ports when calculating the bandwidth
> >requirements. This is needed because originally we relied on the
> >state duplication functions to prune any stale ports from the new
> >state, which would prevent us from incorrectly considering their
> >bandwidth requirements alongside legitimate new payloads.
> >  - Add function references in drm_dp_atomic_release_vcpi_slots() - danvet
> >  - Annotate atomic VCPI and atomic check functions with __must_check
> >- danvet
> > 
> > Changes since v1:
> >  - Don't use the now-removed ->atomic_check() for private objects hook,
> >just give drivers a function to call themselves
> > 
> > Signed-off-by: Lyude Paul 
> > Cc: Daniel Vetter 
> > ---
> >  drivers/gpu/drm/drm_dp_mst_topology.c | 222 ++
> >  drivers/gpu/drm/i915/intel_display.c  |   4 +
> >  drivers/gpu/drm/i915/intel_dp_mst.c   |  31 ++--
> >  include/drm/drm_dp_mst_helper.h   |  23 ++-
> >  4 files changed, 225 insertions(+), 55 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> > b/drivers/gpu/drm/drm_dp_mst_topology.c
> > index 8c3cfac437f4..74823afb262e 100644
> > --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> > @@ -2614,21 +2614,34 @@ static int drm_dp_init_vcpi(struct
> > drm_dp_mst_topology_mgr *mgr,
> >  }
> >  
> >  /**
> > - * drm_dp_atomic_find_vcpi_slots() - Find and add vcpi slots to the state
> > + * drm_dp_atomic_find_vcpi_slots() - Find and add VCPI slots to the state
> >   * @state: global atomic state
> >   * @mgr: MST topology manager for the port
> >   * @port: port to find vcpi slots for
> >   * @pbn: bandwidth required for the mode in PBN
> >   *
> > - * RETURNS:
> > - * Total slots in the atomic state assigned for this port or error
> > + * Allocates VCPI slots to @port, replacing any previous VCPI allocations
> > it
> > + * may have had. Any atomic drivers which support MST must call this
> > function
> > + * in their _encoder_helper_funcs.atomic_check() callback to change
> > the
> > + * current VCPI allocation for the new state. The allocations are not
> > checked
> > + * against the bandwidth restraints of @mgr until the driver calls
> > + * drm_dp_mst_atomic_check().
> > + *
> > + * See also:
> > + * drm_dp_atomic_release_vcpi_slots()
> > + * drm_dp_mst_atomic_check()
> > + *
> > + * Returns:
> > + * Total slots in the atomic state assigned for this port, or a negative
> > error
> > + * code if the port no longer exists
> >   */
> >  int

Re: [Intel-gfx] [PATCH v3 2/5] drm/dp_mst: Start tracking per-port VCPI allocations

2018-11-07 Thread Daniel Vetter 
On Tue, Nov 06, 2018 at 08:21:14PM -0500, Lyude Paul wrote:
> There has been a TODO waiting for quite a long time in
> drm_dp_mst_topology.c:
> 
>   /* We cannot rely on port->vcpi.num_slots to update
>* topology_state->avail_slots as the port may not exist if the parent
>* branch device was unplugged. This should be fixed by tracking
>* per-port slot allocation in drm_dp_mst_topology_state instead of
>* depending on the caller to tell us how many slots to release.
>*/
> 
> That's not the only reason we should fix this: forcing the driver to
> track the VCPI allocations throughout a state's atomic check is
> error prone, because it means that extra care has to be taken with the
> order that drm_dp_atomic_find_vcpi_slots() and
> drm_dp_atomic_release_vcpi_slots() are called in in order to ensure
> idempotency. Currently the only driver actually using these helpers,
> i915, doesn't even do this correctly: multiple ->best_encoder() checks
> with i915's current implementation would not be idempotent and would
> over-allocate VCPI slots, something I learned trying to implement
> fallback retraining in MST.
> 
> So: simplify this whole mess, and teach drm_dp_atomic_find_vcpi_slots()
> and drm_dp_atomic_release_vcpi_slots() to track the VCPI allocations for
> each port. This allows us to ensure idempotency without having to rely
> on the driver as much. Additionally: the driver doesn't need to do any
> kind of VCPI slot tracking anymore if it doesn't need it for it's own
> internal state.
> 
> Additionally; this adds a new drm_dp_mst_atomic_check() helper which
> must be used by atomic drivers to perform validity checks for the new
> VCPI allocations incurred by a state.
> 
> Also: update the documentation and make it more obvious that these
> /must/ be called by /all/ atomic drivers supporting MST.
> 
> Changes since v2:
>  - Use kmemdup() for duplicating MST state - danvet
>  - Move port validation out of duplicate state callback - danvet
>  - Handle looping through MST topology states in
>drm_dp_mst_atomic_check() so the driver doesn't have to do it
>  - Fix documentation in drm_dp_atomic_find_vcpi_slots()
>  - Move the atomic check for each individual topology state into it's
>own function, reduces indenting
>  - Don't consider "stale" MST ports when calculating the bandwidth
>requirements. This is needed because originally we relied on the
>state duplication functions to prune any stale ports from the new
>state, which would prevent us from incorrectly considering their
>bandwidth requirements alongside legitimate new payloads.
>  - Add function references in drm_dp_atomic_release_vcpi_slots() - danvet
>  - Annotate atomic VCPI and atomic check functions with __must_check
>- danvet
> 
> Changes since v1:
>  - Don't use the now-removed ->atomic_check() for private objects hook,
>just give drivers a function to call themselves
> 
> Signed-off-by: Lyude Paul 
> Cc: Daniel Vetter 
> ---
>  drivers/gpu/drm/drm_dp_mst_topology.c | 222 ++
>  drivers/gpu/drm/i915/intel_display.c  |   4 +
>  drivers/gpu/drm/i915/intel_dp_mst.c   |  31 ++--
>  include/drm/drm_dp_mst_helper.h   |  23 ++-
>  4 files changed, 225 insertions(+), 55 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c 
> b/drivers/gpu/drm/drm_dp_mst_topology.c
> index 8c3cfac437f4..74823afb262e 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -2614,21 +2614,34 @@ static int drm_dp_init_vcpi(struct 
> drm_dp_mst_topology_mgr *mgr,
>  }
>  
>  /**
> - * drm_dp_atomic_find_vcpi_slots() - Find and add vcpi slots to the state
> + * drm_dp_atomic_find_vcpi_slots() - Find and add VCPI slots to the state
>   * @state: global atomic state
>   * @mgr: MST topology manager for the port
>   * @port: port to find vcpi slots for
>   * @pbn: bandwidth required for the mode in PBN
>   *
> - * RETURNS:
> - * Total slots in the atomic state assigned for this port or error
> + * Allocates VCPI slots to @port, replacing any previous VCPI allocations it
> + * may have had. Any atomic drivers which support MST must call this function
> + * in their _encoder_helper_funcs.atomic_check() callback to change the
> + * current VCPI allocation for the new state. The allocations are not checked
> + * against the bandwidth restraints of @mgr until the driver calls
> + * drm_dp_mst_atomic_check().
> + *
> + * See also:
> + * drm_dp_atomic_release_vcpi_slots()
> + * drm_dp_mst_atomic_check()
> + *
> + * Returns:
> + * Total slots in the atomic state assigned for this port, or a negative 
> error
> + * code if the port no longer exists
>   */
>  int drm_dp_atomic_find_vcpi_slots(struct drm_atomic_state *state,
> struct drm_dp_mst_topology_mgr *mgr,
> struct drm_dp_mst_port *port, int pbn)
>  {
>   struct drm_dp_mst_topology_state *topology_state;
> -

[Intel-gfx] [PATCH v3 2/5] drm/dp_mst: Start tracking per-port VCPI allocations

2018-11-06 Thread Lyude Paul 
There has been a TODO waiting for quite a long time in
drm_dp_mst_topology.c:

/* We cannot rely on port->vcpi.num_slots to update
 * topology_state->avail_slots as the port may not exist if the parent
 * branch device was unplugged. This should be fixed by tracking
 * per-port slot allocation in drm_dp_mst_topology_state instead of
 * depending on the caller to tell us how many slots to release.
 */

That's not the only reason we should fix this: forcing the driver to
track the VCPI allocations throughout a state's atomic check is
error prone, because it means that extra care has to be taken with the
order that drm_dp_atomic_find_vcpi_slots() and
drm_dp_atomic_release_vcpi_slots() are called in in order to ensure
idempotency. Currently the only driver actually using these helpers,
i915, doesn't even do this correctly: multiple ->best_encoder() checks
with i915's current implementation would not be idempotent and would
over-allocate VCPI slots, something I learned trying to implement
fallback retraining in MST.

So: simplify this whole mess, and teach drm_dp_atomic_find_vcpi_slots()
and drm_dp_atomic_release_vcpi_slots() to track the VCPI allocations for
each port. This allows us to ensure idempotency without having to rely
on the driver as much. Additionally: the driver doesn't need to do any
kind of VCPI slot tracking anymore if it doesn't need it for it's own
internal state.

Additionally; this adds a new drm_dp_mst_atomic_check() helper which
must be used by atomic drivers to perform validity checks for the new
VCPI allocations incurred by a state.

Also: update the documentation and make it more obvious that these
/must/ be called by /all/ atomic drivers supporting MST.

Changes since v2:
 - Use kmemdup() for duplicating MST state - danvet
 - Move port validation out of duplicate state callback - danvet
 - Handle looping through MST topology states in
   drm_dp_mst_atomic_check() so the driver doesn't have to do it
 - Fix documentation in drm_dp_atomic_find_vcpi_slots()
 - Move the atomic check for each individual topology state into it's
   own function, reduces indenting
 - Don't consider "stale" MST ports when calculating the bandwidth
   requirements. This is needed because originally we relied on the
   state duplication functions to prune any stale ports from the new
   state, which would prevent us from incorrectly considering their
   bandwidth requirements alongside legitimate new payloads.
 - Add function references in drm_dp_atomic_release_vcpi_slots() - danvet
 - Annotate atomic VCPI and atomic check functions with __must_check
   - danvet

Changes since v1:
 - Don't use the now-removed ->atomic_check() for private objects hook,
   just give drivers a function to call themselves

Signed-off-by: Lyude Paul 
Cc: Daniel Vetter 
---
 drivers/gpu/drm/drm_dp_mst_topology.c | 222 ++
 drivers/gpu/drm/i915/intel_display.c  |   4 +
 drivers/gpu/drm/i915/intel_dp_mst.c   |  31 ++--
 include/drm/drm_dp_mst_helper.h   |  23 ++-
 4 files changed, 225 insertions(+), 55 deletions(-)

diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c 
b/drivers/gpu/drm/drm_dp_mst_topology.c
index 8c3cfac437f4..74823afb262e 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -2614,21 +2614,34 @@ static int drm_dp_init_vcpi(struct 
drm_dp_mst_topology_mgr *mgr,
 }
 
 /**
- * drm_dp_atomic_find_vcpi_slots() - Find and add vcpi slots to the state
+ * drm_dp_atomic_find_vcpi_slots() - Find and add VCPI slots to the state
  * @state: global atomic state
  * @mgr: MST topology manager for the port
  * @port: port to find vcpi slots for
  * @pbn: bandwidth required for the mode in PBN
  *
- * RETURNS:
- * Total slots in the atomic state assigned for this port or error
+ * Allocates VCPI slots to @port, replacing any previous VCPI allocations it
+ * may have had. Any atomic drivers which support MST must call this function
+ * in their _encoder_helper_funcs.atomic_check() callback to change the
+ * current VCPI allocation for the new state. The allocations are not checked
+ * against the bandwidth restraints of @mgr until the driver calls
+ * drm_dp_mst_atomic_check().
+ *
+ * See also:
+ * drm_dp_atomic_release_vcpi_slots()
+ * drm_dp_mst_atomic_check()
+ *
+ * Returns:
+ * Total slots in the atomic state assigned for this port, or a negative error
+ * code if the port no longer exists
  */
 int drm_dp_atomic_find_vcpi_slots(struct drm_atomic_state *state,
  struct drm_dp_mst_topology_mgr *mgr,
  struct drm_dp_mst_port *port, int pbn)
 {
struct drm_dp_mst_topology_state *topology_state;
-   int req_slots;
+   struct drm_dp_vcpi_allocation *pos, *vcpi = NULL;
+   int prev_slots, req_slots, ret;
 
topology_state = drm_atomic_get_mst_topology_state(state, mgr);
if (IS_ERR(topology_state))
@@ -2637,20 +2650,41 @@