Re: [PATCH v2 2/4] drm/dp_mst: Start tracking per-port VCPI allocations
On Mon, Oct 29, 2018 at 03:24:29PM +0100, Daniel Vetter wrote:
> On Fri, Oct 26, 2018 at 04:35:47PM -0400, 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 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 | 190 +-
> > drivers/gpu/drm/i915/intel_display.c | 8 ++
> > drivers/gpu/drm/i915/intel_dp_mst.c | 31 +++--
> > include/drm/drm_dp_mst_helper.h | 11 +-
> > 4 files changed, 192 insertions(+), 48 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> > b/drivers/gpu/drm/drm_dp_mst_topology.c
> > index 8c3cfac437f4..dcfab7536914 100644
> > --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> > @@ -2614,21 +2614,33 @@ 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
> > *
> > + * 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 atomic_check() handlers to change the current VCPI allocation
> > for
>
> Maybe do a nice kerneldoc reference to the right atomic_check here.
>
> > + * the new state. After the ->atomic_check() hooks of the driver and all
> > other
>
> This will upset the kerneldoc parser I think.
>
> > + * mode objects in the state have been called, DRM will check the final
> > VCPI
> > + * allocations to ensure that they will fit into the available bandwidth on
> > + * the topology.
> > + *
> > + * See also: drm_dp_atomic_release_vcpi_slots()
>
> Also need to reference drm_dp_mst_atomic_check() here and that drivers
> must call it or nothing happens.
> > + *
> > * RETURNS:
> > - * Total slots in the atomic state assigned for this port or error
> > + * 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 +2649,41 @@ int drm_dp_atomic_find_vcpi_slots(struct
> > drm_atomic_state *state,
> > port = drm_dp_get_validated_port_ref(mgr, port);
> > if (port == NULL)
> >
Re: [PATCH v2 2/4] drm/dp_mst: Start tracking per-port VCPI allocations
On Mon, 2018-10-29 at 15:24 +0100, Daniel Vetter wrote:
> On Fri, Oct 26, 2018 at 04:35:47PM -0400, 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 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 | 190 +-
> > drivers/gpu/drm/i915/intel_display.c | 8 ++
> > drivers/gpu/drm/i915/intel_dp_mst.c | 31 +++--
> > include/drm/drm_dp_mst_helper.h | 11 +-
> > 4 files changed, 192 insertions(+), 48 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> > b/drivers/gpu/drm/drm_dp_mst_topology.c
> > index 8c3cfac437f4..dcfab7536914 100644
> > --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> > @@ -2614,21 +2614,33 @@ 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
> > *
> > + * 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 atomic_check() handlers to change the current VCPI allocation
> > for
>
> Maybe do a nice kerneldoc reference to the right atomic_check here.
>
> > + * the new state. After the ->atomic_check() hooks of the driver and all
> > other
>
> This will upset the kerneldoc parser I think.
>
> > + * mode objects in the state have been called, DRM will check the final
> > VCPI
> > + * allocations to ensure that they will fit into the available bandwidth
> > on
> > + * the topology.
> > + *
> > + * See also: drm_dp_atomic_release_vcpi_slots()
>
> Also need to reference drm_dp_mst_atomic_check() here and that drivers
> must call it or nothing happens.
> > + *
> > * RETURNS:
> > - * Total slots in the atomic state assigned for this port or error
> > + * 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 +2649,41 @@ int drm_dp_atomic_find_vcpi_slots(struct
> > drm_atomic_state *state,
> > port = drm_dp_get_validated_port_ref(mgr, port);
> > if (port == NULL)
> >
Re: [PATCH v2 2/4] drm/dp_mst: Start tracking per-port VCPI allocations
On Fri, Oct 26, 2018 at 04:35:47PM -0400, 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 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 | 190 +-
> drivers/gpu/drm/i915/intel_display.c | 8 ++
> drivers/gpu/drm/i915/intel_dp_mst.c | 31 +++--
> include/drm/drm_dp_mst_helper.h | 11 +-
> 4 files changed, 192 insertions(+), 48 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> b/drivers/gpu/drm/drm_dp_mst_topology.c
> index 8c3cfac437f4..dcfab7536914 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -2614,21 +2614,33 @@ 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
> *
> + * 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 atomic_check() handlers to change the current VCPI allocation for
Maybe do a nice kerneldoc reference to the right atomic_check here.
> + * the new state. After the ->atomic_check() hooks of the driver and all
> other
This will upset the kerneldoc parser I think.
> + * mode objects in the state have been called, DRM will check the final VCPI
> + * allocations to ensure that they will fit into the available bandwidth on
> + * the topology.
> + *
> + * See also: drm_dp_atomic_release_vcpi_slots()
Also need to reference drm_dp_mst_atomic_check() here and that drivers
must call it or nothing happens.
> + *
> * RETURNS:
> - * Total slots in the atomic state assigned for this port or error
> + * 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 +2649,41 @@ int drm_dp_atomic_find_vcpi_slots(struct
> drm_atomic_state *state,
> port = drm_dp_get_validated_port_ref(mgr, port);
> if (port == NULL)
> return -EINVAL;
> - req_slots = DIV_ROUND_UP(pbn, mgr->pbn_div);
> - DRM_DEBUG_KMS("vcpi slots req=%d, avail=%d\n",
> - req_slots, topology_state->avail_slots);
>
> - if (req_slots > topology_state->avail_slots) {
> -
[PATCH v2 2/4] drm/dp_mst: Start tracking per-port VCPI allocations
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 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 | 190 +-
drivers/gpu/drm/i915/intel_display.c | 8 ++
drivers/gpu/drm/i915/intel_dp_mst.c | 31 +++--
include/drm/drm_dp_mst_helper.h | 11 +-
4 files changed, 192 insertions(+), 48 deletions(-)
diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
b/drivers/gpu/drm/drm_dp_mst_topology.c
index 8c3cfac437f4..dcfab7536914 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -2614,21 +2614,33 @@ 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
*
+ * 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 atomic_check() handlers to change the current VCPI allocation for
+ * the new state. After the ->atomic_check() hooks of the driver and all other
+ * mode objects in the state have been called, DRM will check the final VCPI
+ * allocations to ensure that they will fit into the available bandwidth on
+ * the topology.
+ *
+ * See also: drm_dp_atomic_release_vcpi_slots()
+ *
* RETURNS:
- * Total slots in the atomic state assigned for this port or error
+ * 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 +2649,41 @@ int drm_dp_atomic_find_vcpi_slots(struct
drm_atomic_state *state,
port = drm_dp_get_validated_port_ref(mgr, port);
if (port == NULL)
return -EINVAL;
- req_slots = DIV_ROUND_UP(pbn, mgr->pbn_div);
- DRM_DEBUG_KMS("vcpi slots req=%d, avail=%d\n",
- req_slots, topology_state->avail_slots);
- if (req_slots > topology_state->avail_slots) {
- drm_dp_put_port(port);
- return -ENOSPC;
+ /* Find the current allocation for this port, if any */
+ list_for_each_entry(pos, &topology_state->vcpis, next) {
+ if (pos->port == port) {
+ vcpi = pos;
+ prev_slots = vcpi->vcpi;
+ break;
+ }
}
+ if (!vcpi)
+ prev_slots = 0;
+
+ req_slot
