On Wed, 2017-01-04 at 19:20 +0000, 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 <dhinakaran.pandiyan@xxxxxxxxx>
> >
> > 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 <drm/drm_mode.h>
> > > #include <drm/drm_plane_helper.h>
> > > #include <drm/drm_print.h>
> > > +#include <drm/drm_dp_mst_helper.h>
> > > #include <linux/sync_file.h>
> > >
> > > #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 = &mgr->dev->mode_config;
> > > +
> > > + WARN_ON(!state->acquire_ctx);
> > > +
> > > + ret = drm_modeset_lock(&config->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(&state->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;
> > > +
> > > + DRM_DEBUG_ATOMIC("Added [MST Topology w/ base connector:%d] %p state to %p\n",
> > > + mgr->conn_base_id, new_mst_state, state);
> > > +
> > > + return new_mst_state;
> > > +}
> > > +EXPORT_SYMBOL(drm_atomic_get_mst_topology_state);
> > > +
> > > /**
> > > * drm_atomic_get_connector_state - get connector state
> > > * @state: global atomic state object
> > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > > index 5e52244..1cd38b7 100644
> > > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > > @@ -30,6 +30,7 @@
> > > #include <drm/drm_plane_helper.h>
> > > #include <drm/drm_crtc_helper.h>
> > > #include <drm/drm_atomic_helper.h>
> > > +#include <drm/drm_dp_mst_helper.h>
> > > #include <linux/dma-fence.h>
> > >
> > > #include "drm_crtc_internal.h"
> > > @@ -1992,6 +1993,15 @@ void drm_atomic_helper_swap_state(struct drm_atomic_state *state,
> > > connector->state->state = NULL;
> > > }
> > >
> > > + for (i = 0; i < state->num_mst_topologies; i++) {
> > > + struct drm_dp_mst_topology_mgr *mgr;
> > > +
> > > + mgr = state->dp_mst_topologies[i].ptr;
> > > + mgr->state->state = state;
> > > + swap(state->dp_mst_topologies[i].state, mgr->state);
> > > + mgr->state->state = NULL;
> > > + }
> >
> > I'm not sure we want to tie the dp mst helpers that closely with the
> > atomic machinery, otoh it makes things a lot easier. My concern is that
> > drm_atomic_state is a core structure, whereas the dp_mst stuff is a
> > helper, which means we have a control inversion here compared to the
> > normal pattern.
> >
> > > +
> > > for_each_crtc_in_state(state, crtc, crtc_state, i) {
> > > crtc->state->state = state;
> > > swap(state->crtcs[i].state, crtc->state);
> > > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
> > > index d42a6c0..1be19e1 100644
> > > --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> > > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> > > @@ -2042,6 +2042,9 @@ int drm_dp_mst_topology_mgr_set_mst(struct drm_dp_mst_topology_mgr *mgr, bool ms
> > > goto out_unlock;
> > > }
> > >
> > > + /* max. time slots - one slot for MTP header */
> > > + mgr->state->avail_slots = 63;
> > > +
> > > /* add initial branch device at LCT 1 */
> > > mstb = drm_dp_add_mst_branch_device(1, NULL);
> > > if (mstb == NULL) {
> > > @@ -2973,6 +2976,11 @@ int drm_dp_mst_topology_mgr_init(struct drm_dp_mst_topology_mgr *mgr,
> > > mgr->proposed_vcpis = kcalloc(max_payloads, sizeof(struct drm_dp_vcpi *), GFP_KERNEL);
> > > if (!mgr->proposed_vcpis)
> > > return -ENOMEM;
> > > + mgr->state = kzalloc(sizeof(*mgr->state), GFP_KERNEL);
> > > + if (!mgr->state)
> > > + return -ENOMEM;
> > > + mgr->state->mgr = mgr;
> > > +
> > > set_bit(0, &mgr->payload_mask);
> > > if (test_calc_pbn_mode() < 0)
> > > DRM_ERROR("MST PBN self-test failed\n");
> > > @@ -2995,6 +3003,8 @@ void drm_dp_mst_topology_mgr_destroy(struct drm_dp_mst_topology_mgr *mgr)
> > > kfree(mgr->proposed_vcpis);
> > > mgr->proposed_vcpis = NULL;
> > > mutex_unlock(&mgr->payload_lock);
> > > + kfree(mgr->state);
> > > + mgr->state = NULL;
> > > mgr->dev = NULL;
> > > mgr->aux = NULL;
> > > }
> > > 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/ ?
Do we need the destroy callback too? drm_atomic_state_default_clear()
does not have to dereference drm_dp_mst_topology_mgr.
Moving this to intel_atomic_state would be mean that nouveau will
require a re-implementation. Is that right?
-DK
> > > +};
> > > +
> > > /**
> > > * 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)
> > > @@ -164,6 +169,8 @@ struct __drm_connnectors_state {
> > > * @crtcs: pointer to array of CRTC pointers
> > > * @num_connector: size of the @connectors and @connector_states arrays
> > > * @connectors: pointer to array of structures with per-connector data
> > > + * @num_mst_topologies: size of the @dp_mst_topologies array
> > > + * @dp_mst_topologies: pointer to array of structures with per-MST topology manager data
> > > * @acquire_ctx: acquire context for this atomic modeset state update
> > > */
> > > struct drm_atomic_state {
> > > @@ -177,6 +184,8 @@ struct drm_atomic_state {
> > > struct __drm_crtcs_state *crtcs;
> > > int num_connector;
> > > struct __drm_connnectors_state *connectors;
> > > + int num_mst_topologies;
> > > + struct __drm_dp_mst_topology_state *dp_mst_topologies;
> > >
> > > struct drm_modeset_acquire_ctx *acquire_ctx;
> > >
> > > @@ -250,6 +259,10 @@ int drm_atomic_connector_set_property(struct drm_connector *connector,
> > > struct drm_connector_state *state, struct drm_property *property,
> > > uint64_t val);
> > >
> > > +struct drm_dp_mst_topology_state * __must_check
> > > +drm_atomic_get_mst_topology_state(struct drm_atomic_state *state,
> > > + struct drm_dp_mst_topology_mgr *mgr);
> > > +
> > > /**
> > > * drm_atomic_get_existing_crtc_state - get crtc state, if it exists
> > > * @state: global atomic state object
> > > diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h
> > > index 98d3c73..0a9bf20 100644
> > > --- a/include/drm/drm_dp_mst_helper.h
> > > +++ b/include/drm/drm_dp_mst_helper.h
> > > @@ -403,6 +403,12 @@ struct drm_dp_payload {
> > > int vcpi;
> > > };
> > >
> > > +struct drm_dp_mst_topology_state {
> > > + int avail_slots;
> > > + struct drm_atomic_state *state;
> > > + struct drm_dp_mst_topology_mgr *mgr;
> > > +};
> > > +
> > > /**
> > > * struct drm_dp_mst_topology_mgr - DisplayPort MST manager
> > > *
> > > @@ -481,6 +487,11 @@ struct drm_dp_mst_topology_mgr {
> > > int pbn_div;
> > >
> > > /**
> > > + * @state: MST topology manager state for atomic modesetting drivers
> > > + */
> > > + struct drm_dp_mst_topology_state *state;
> > > +
> > > + /**
> > > * @qlock: protects @tx_msg_downq, the tx_slots in struct
> > > * &drm_dp_mst_branch and txmsg->state once they are queued
> > > */
> > > @@ -596,4 +607,6 @@ void drm_dp_mst_dump_topology(struct seq_file *m,
> > >
> > > void drm_dp_mst_topology_mgr_suspend(struct drm_dp_mst_topology_mgr *mgr);
> > > int drm_dp_mst_topology_mgr_resume(struct drm_dp_mst_topology_mgr *mgr);
> > > +struct drm_dp_mst_topology_state *drm_atomic_get_mst_topology_state(struct drm_atomic_state *state,
> > > + struct drm_dp_mst_topology_mgr *mgr);
> > > #endif
> > > --
> > > 2.7.4
> > >
> >
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
