[Intel-gfx] [PATCH 4/6] drm/dp: Introduce DP MST topology manager state to track DP link bw
On Thu, Jan 05, 2017 at 03:54:54AM +, Pandiyan, Dhinakaran wrote:
> On Wed, 2017-01-04 at 19:20 +, Pandiyan, Dhinakaran wrote:
> > On Wed, 2017-01-04 at 10:33 +0100, Daniel Vetter wrote:
> > > On Tue, Jan 03, 2017 at 01:01:49PM -0800, Dhinakaran Pandiyan wrote:
> > > > Link bandwidth is shared between multiple display streams in DP MST
> > > > configurations. The DP MST topology manager structure maintains the
> > > > shared
> > > > link bandwidth for a primary link directly connected to the GPU. For
> > > > atomic
> > > > modesetting drivers, checking if there is sufficient link bandwidth for
> > > > a
> > > > mode needs to be done during the atomic_check phase to avoid failed
> > > > modesets. Let's encsapsulate the available link bw information in a
> > > > state
> > > > structure so that bw can be allocated and released atomically for each
> > > > of
> > > > the ports sharing the primary link.
> > > >
> > > > Signed-off-by: Dhinakaran Pandiyan
> > >
> > > Overall issue with the patch is that dp helpers now have 2 places where
> > > available_slots is stored: One for atomic drivers in ->state, and the
> > > legacy one. I think it'd be good to rework the legacy codepaths (i.e.
> > > drm_dp_find_vcpi_slots) to use mgr->state->avail_slots, and remove
> > > mgr->avail_slots entirely.
> >
> > PATCH 2/6 does that. mgr->avail_slots is not updated in the legacy code
> > path, so the check turns out to be against mgr->total_slots. So, I did
> > just that, albeit explicitly.
Ah right, I missed that.
> > > > diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> > > > index fd2d971..7ac5ed6 100644
> > > > --- a/include/drm/drm_atomic.h
> > > > +++ b/include/drm/drm_atomic.h
> > > > @@ -153,6 +153,11 @@ struct __drm_connnectors_state {
> > > > struct drm_connector_state *state;
> > > > };
> > > >
> > > > +struct __drm_dp_mst_topology_state {
> > > > + struct drm_dp_mst_topology_mgr *ptr;
> > > > + struct drm_dp_mst_topology_state *state;
> > >
> > > One way to fix that control inversion I mentioned above is to use void*
> > > pionters here, and then have callbacks for atomic_destroy and swap_state
> > > on top. A bit more shuffling, but we could then use that for other driver
> > > private objects.
> > >
> > > Other option is to stuff it into intel_atomic_state.
>
> Hmm... I think I understand what you are saying. The core atomic
> functions like swap_state should not be able alter the topology
> manager's current state?
>
> Did you mean something like this - https://paste.ubuntu.com/23743485/ ?
Not quite yet, here's what I had in mind as a sketch:
diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
index 2e28fdca9c3d..6ce704b1e900 100644
--- a/include/drm/drm_atomic.h
+++ b/include/drm/drm_atomic.h
@@ -154,6 +154,17 @@ struct __drm_connnectors_state {
struct drm_connector_state *state;
};
+struct drm_private_state_funcs {
+ void (*swap)(void *obj, void *state);
+ void (*destroy_state)(void *state);
+};
+
+struct __drm_private_obj_state {
+ struct obj *ptr;
+ struct obj_state *state;
+ struct drm_private_state_funcs *funcs;
+}
+
/**
* struct drm_atomic_state - the global state object for atomic updates
* @ref: count of all references to this state (will not be freed until zero)
@@ -178,6 +189,8 @@ struct drm_atomic_state {
struct __drm_crtcs_state *crtcs;
int num_connector;
struct __drm_connnectors_state *connectors;
+ int num_private_objs;
+ struct __drm_private_obj_state *private_objs;
struct drm_modeset_acquire_ctx *acquire_ctx;
@@ -414,6 +427,19 @@ void drm_state_dump(struct drm_device *dev, struct
drm_printer *p);
(__i)++) \
for_each_if (plane_state)
+/* The magic here is that if obj and obj_state have the right type, then this
+ * will automatically cast to the right type. Since we allow any kind of
private
+ * object mixed into the same array, runtime type casting is done using the
+ * funcs pointer.
+ */
+#define for_each_private_obj(__state, obj, obj_state, __i, funcs)
+ for ((__i) = 0; \
+(__i) < (__state)->num_private_objs &&
\
+((obj) = (__state)->private_objs[__i].ptr, \
+(obj_state) = (__state)->private_objs[__i].state, 1); \
+(__i)++) \
+ for_each_if ((__state)->private_objs[__i].funcs == (funcs))
+
/**
* drm_atomic_crtc_needs_modeset - compute combined modeset need
* @state: _crtc_state for the CRTC
I didn't sketch helper functions for looking up/inserting objects, and ofc
we'd need the boilerplat for cleanup/swap, but I hope that part is clear.
If we add a duplicate_state callback to drm_private_state_funcs then we
might even be able to
[Intel-gfx] [PATCH 4/6] drm/dp: Introduce DP MST topology manager state to track DP link bw
On Wed, 2017-01-04 at 19:20 +, Pandiyan, Dhinakaran wrote:
> On Wed, 2017-01-04 at 10:33 +0100, Daniel Vetter wrote:
> > On Tue, Jan 03, 2017 at 01:01:49PM -0800, Dhinakaran Pandiyan wrote:
> > > Link bandwidth is shared between multiple display streams in DP MST
> > > configurations. The DP MST topology manager structure maintains the shared
> > > link bandwidth for a primary link directly connected to the GPU. For
> > > atomic
> > > modesetting drivers, checking if there is sufficient link bandwidth for a
> > > mode needs to be done during the atomic_check phase to avoid failed
> > > modesets. Let's encsapsulate the available link bw information in a state
> > > structure so that bw can be allocated and released atomically for each of
> > > the ports sharing the primary link.
> > >
> > > Signed-off-by: Dhinakaran Pandiyan
> >
> > Overall issue with the patch is that dp helpers now have 2 places where
> > available_slots is stored: One for atomic drivers in ->state, and the
> > legacy one. I think it'd be good to rework the legacy codepaths (i.e.
> > drm_dp_find_vcpi_slots) to use mgr->state->avail_slots, and remove
> > mgr->avail_slots entirely.
>
> PATCH 2/6 does that. mgr->avail_slots is not updated in the legacy code
> path, so the check turns out to be against mgr->total_slots. So, I did
> just that, albeit explicitly.
>
> -DK
>
> >
> > Another design concern below, but in principle this looks like what we
> > need.
> > -Daniel
> >
> >
> > > ---
> > > drivers/gpu/drm/drm_atomic.c | 66
> > > +++
> > > drivers/gpu/drm/drm_atomic_helper.c | 10 ++
> > > drivers/gpu/drm/drm_dp_mst_topology.c | 10 ++
> > > include/drm/drm_atomic.h | 13 +++
> > > include/drm/drm_dp_mst_helper.h | 13 +++
> > > 5 files changed, 112 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > > index 681d5f9..b8e2cea 100644
> > > --- a/drivers/gpu/drm/drm_atomic.c
> > > +++ b/drivers/gpu/drm/drm_atomic.c
> > > @@ -31,6 +31,7 @@
> > > #include
> > > #include
> > > #include
> > > +#include
> > > #include
> > >
> > > #include "drm_crtc_internal.h"
> > > @@ -62,6 +63,7 @@ void drm_atomic_state_default_release(struct
> > > drm_atomic_state *state)
> > > kfree(state->connectors);
> > > kfree(state->crtcs);
> > > kfree(state->planes);
> > > + kfree(state->dp_mst_topologies);
> > > }
> > > EXPORT_SYMBOL(drm_atomic_state_default_release);
> > >
> > > @@ -189,6 +191,18 @@ void drm_atomic_state_default_clear(struct
> > > drm_atomic_state *state)
> > > state->planes[i].ptr = NULL;
> > > state->planes[i].state = NULL;
> > > }
> > > +
> > > + for (i = 0; i < state->num_mst_topologies; i++) {
> > > + struct drm_dp_mst_topology_mgr *mgr =
> > > state->dp_mst_topologies[i].ptr;
> > > +
> > > + if (!mgr)
> > > + continue;
> > > +
> > > + kfree(state->dp_mst_topologies[i].state);
> > > + state->dp_mst_topologies[i].ptr = NULL;
> > > + state->dp_mst_topologies[i].state = NULL;
> > > + }
> > > +
> > > }
> > > EXPORT_SYMBOL(drm_atomic_state_default_clear);
> > >
> > > @@ -981,6 +995,58 @@ static void drm_atomic_plane_print_state(struct
> > > drm_printer *p,
> > > plane->funcs->atomic_print_state(p, state);
> > > }
> > >
> > > +struct drm_dp_mst_topology_state
> > > *drm_atomic_get_mst_topology_state(struct drm_atomic_state *state,
> > > + struct drm_dp_mst_topology_mgr *mgr)
> > > +{
> > > +
> > > + int ret, i;
> > > + size_t new_size;
> > > + struct __drm_dp_mst_topology_state *new_arr;
> > > + struct drm_dp_mst_topology_state *new_mst_state;
> > > + int num_topologies;
> > > + struct drm_mode_config *config = >dev->mode_config;
> > > +
> > > + WARN_ON(!state->acquire_ctx);
> > > +
> > > + ret = drm_modeset_lock(>connection_mutex, state->acquire_ctx);
> > > + if (ret)
> > > + return ERR_PTR(ret);
> > > +
> > > + for (i = 0; i < state->num_mst_topologies; i++) {
> > > + if (mgr == state->dp_mst_topologies[i].ptr &&
> > > + state->dp_mst_topologies[i].state) {
> > > + return state->dp_mst_topologies[i].state;
> > > + }
> > > + }
> > > +
> > > + num_topologies = state->num_mst_topologies + 1;
> > > + new_size = sizeof(*state->dp_mst_topologies) * num_topologies;
> > > + new_arr = krealloc(state->dp_mst_topologies, new_size, GFP_KERNEL);
> > > + if (!new_arr)
> > > + return ERR_PTR(-ENOMEM);
> > > +
> > > + state->dp_mst_topologies = new_arr;
> > > + memset(>dp_mst_topologies[state->num_mst_topologies], 0,
> > > + sizeof(*state->dp_mst_topologies));
> > > +
> > > + new_mst_state = kmalloc(sizeof(*mgr->state), GFP_KERNEL);
> > > + if (!new_mst_state)
> > > + return ERR_PTR(-ENOMEM);
> > > +
> > > + new_mst_state->avail_slots = mgr->state->avail_slots;
> > > +
[Intel-gfx] [PATCH 4/6] drm/dp: Introduce DP MST topology manager state to track DP link bw
On Wed, 2017-01-04 at 10:33 +0100, Daniel Vetter wrote:
> On Tue, Jan 03, 2017 at 01:01:49PM -0800, Dhinakaran Pandiyan wrote:
> > Link bandwidth is shared between multiple display streams in DP MST
> > configurations. The DP MST topology manager structure maintains the shared
> > link bandwidth for a primary link directly connected to the GPU. For atomic
> > modesetting drivers, checking if there is sufficient link bandwidth for a
> > mode needs to be done during the atomic_check phase to avoid failed
> > modesets. Let's encsapsulate the available link bw information in a state
> > structure so that bw can be allocated and released atomically for each of
> > the ports sharing the primary link.
> >
> > Signed-off-by: Dhinakaran Pandiyan
>
> Overall issue with the patch is that dp helpers now have 2 places where
> available_slots is stored: One for atomic drivers in ->state, and the
> legacy one. I think it'd be good to rework the legacy codepaths (i.e.
> drm_dp_find_vcpi_slots) to use mgr->state->avail_slots, and remove
> mgr->avail_slots entirely.
PATCH 2/6 does that. mgr->avail_slots is not updated in the legacy code
path, so the check turns out to be against mgr->total_slots. So, I did
just that, albeit explicitly.
-DK
>
> Another design concern below, but in principle this looks like what we
> need.
> -Daniel
>
>
> > ---
> > drivers/gpu/drm/drm_atomic.c | 66
> > +++
> > drivers/gpu/drm/drm_atomic_helper.c | 10 ++
> > drivers/gpu/drm/drm_dp_mst_topology.c | 10 ++
> > include/drm/drm_atomic.h | 13 +++
> > include/drm/drm_dp_mst_helper.h | 13 +++
> > 5 files changed, 112 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > index 681d5f9..b8e2cea 100644
> > --- a/drivers/gpu/drm/drm_atomic.c
> > +++ b/drivers/gpu/drm/drm_atomic.c
> > @@ -31,6 +31,7 @@
> > #include
> > #include
> > #include
> > +#include
> > #include
> >
> > #include "drm_crtc_internal.h"
> > @@ -62,6 +63,7 @@ void drm_atomic_state_default_release(struct
> > drm_atomic_state *state)
> > kfree(state->connectors);
> > kfree(state->crtcs);
> > kfree(state->planes);
> > + kfree(state->dp_mst_topologies);
> > }
> > EXPORT_SYMBOL(drm_atomic_state_default_release);
> >
> > @@ -189,6 +191,18 @@ void drm_atomic_state_default_clear(struct
> > drm_atomic_state *state)
> > state->planes[i].ptr = NULL;
> > state->planes[i].state = NULL;
> > }
> > +
> > + for (i = 0; i < state->num_mst_topologies; i++) {
> > + struct drm_dp_mst_topology_mgr *mgr =
> > state->dp_mst_topologies[i].ptr;
> > +
> > + if (!mgr)
> > + continue;
> > +
> > + kfree(state->dp_mst_topologies[i].state);
> > + state->dp_mst_topologies[i].ptr = NULL;
> > + state->dp_mst_topologies[i].state = NULL;
> > + }
> > +
> > }
> > EXPORT_SYMBOL(drm_atomic_state_default_clear);
> >
> > @@ -981,6 +995,58 @@ static void drm_atomic_plane_print_state(struct
> > drm_printer *p,
> > plane->funcs->atomic_print_state(p, state);
> > }
> >
> > +struct drm_dp_mst_topology_state *drm_atomic_get_mst_topology_state(struct
> > drm_atomic_state *state,
> > + struct drm_dp_mst_topology_mgr *mgr)
> > +{
> > +
> > + int ret, i;
> > + size_t new_size;
> > + struct __drm_dp_mst_topology_state *new_arr;
> > + struct drm_dp_mst_topology_state *new_mst_state;
> > + int num_topologies;
> > + struct drm_mode_config *config = >dev->mode_config;
> > +
> > + WARN_ON(!state->acquire_ctx);
> > +
> > + ret = drm_modeset_lock(>connection_mutex, state->acquire_ctx);
> > + if (ret)
> > + return ERR_PTR(ret);
> > +
> > + for (i = 0; i < state->num_mst_topologies; i++) {
> > + if (mgr == state->dp_mst_topologies[i].ptr &&
> > + state->dp_mst_topologies[i].state) {
> > + return state->dp_mst_topologies[i].state;
> > + }
> > + }
> > +
> > + num_topologies = state->num_mst_topologies + 1;
> > + new_size = sizeof(*state->dp_mst_topologies) * num_topologies;
> > + new_arr = krealloc(state->dp_mst_topologies, new_size, GFP_KERNEL);
> > + if (!new_arr)
> > + return ERR_PTR(-ENOMEM);
> > +
> > + state->dp_mst_topologies = new_arr;
> > + memset(>dp_mst_topologies[state->num_mst_topologies], 0,
> > + sizeof(*state->dp_mst_topologies));
> > +
> > + new_mst_state = kmalloc(sizeof(*mgr->state), GFP_KERNEL);
> > + if (!new_mst_state)
> > + return ERR_PTR(-ENOMEM);
> > +
> > + new_mst_state->avail_slots = mgr->state->avail_slots;
> > + state->dp_mst_topologies[state->num_mst_topologies].state =
> > new_mst_state;
> > + state->dp_mst_topologies[state->num_mst_topologies].ptr = mgr;
> > + state->num_mst_topologies = num_topologies;
> > + new_mst_state->mgr = mgr;
> > + mgr->state->state = state;
> > +
> > +
