[snip]
On Thu, 2021-10-14 at 16:21 -0400, Bhawanpreet Lakha wrote:
>
> Thanks for the response,
>
> That function is per port (drm_dp_atomic_find_vcpi_slots) so not sure
> how that will work, maybe I don't understand what you mean?
Yeah - drm_dp_atomic_find_vcpi_slots() is just one part of the atomic helpers
though, we can always add more. JFYI, the main atomic check function for MST
is drm_dp_mst_atomic_check(). So we'd probably just want to add something into
drm_dp_mst_topology_state that handles making sure we go through
drm_dp_vcpi_allocation struct and recalculates everything in it. We might also
want to add an atomic helper to set the new starting slot and slot count in
the atomic state, then go through the current atomic topology state and ensure
that we add any CRTCs that would need full modesets as a result of that info
changing.
>
> Also we only need to check this inside
> drm_dp_mst_atomic_check_vcpi_alloc_limit(), which doesn't have a state,
> so we still need to have this on the mgr somehow.
It does, we pass drm_dp_mst_topology_state to it. So we could just add these
fields there.
>
> I was thinking we could add the slots(or some DP version indicator)
> inside the drm_connector, and add a parameter to
> drm_dp_mst_atomic_check_vcpi_alloc_limit(int slots) and call it with
> this info via drm_dp_mst_atomic_check().
So before I continue: I should note that some of the code in MST goes against
what I'm about to say, in particular a lot of the payload updating functions
in MST that keep their payload state outside of drm_dp_mst_topology_state and
friends, but that's because some of this code is old and needs to be updated
anyway (and some of it was actually just being kept around because we were
worried about breaking radeon.ko, the only driver relying on behavior from the
non-atomic paths in our topology helper). Also - I'm not sure what your prior
experience is with modesetting in the linux kernel so my apologies if I'm
explaining anything you already understand here.
Anyway-drm_connector wouldn't be the right place to put it because it's not
part of the atomic state. The reason we have atomic modesetting in the first
place is so that we can come up with a new display state, and then have the
kernel verify the configurations in that new state and potentially reject it
if we tried to program something that wouldn't have worked in hardware. As
well, having it in drm_connector would mean that it wouldn't be safe to access
unless we've somehow locked the drm_connector. drm_connector_state would be
'safe' to have this data in, but considering that we already have a private
atomic state object for MST (drm_dp_mst_topology_state) it doesn't make much
sense to keep MST info elsewhere.
>
>
> Bhawan
>
> >
> > > ret = drm_dp_mst_atomic_check_mstb_bw_limit(mgr-
> > > > mst_primary,
> > > mst_state);
> > > mutex_unlock(&mgr->lock);
> > > @@ -5527,11 +5543,16 @@ int drm_dp_mst_topology_mgr_init(struct
> > > drm_dp_mst_topology_mgr *mgr,
> > > if (!mgr->proposed_vcpis)
> > > return -ENOMEM;
> > > set_bit(0, &mgr->payload_mask);
> > > + mgr->total_avail_slots = 63;
> > > + mgr->start_slot = 1;
> > >
> > > mst_state = kzalloc(sizeof(*mst_state), GFP_KERNEL);
> > > if (mst_state == NULL)
> > > return -ENOMEM;
> > >
> > > + mst_state->total_avail_slots = 63;
> > > + mst_state->start_slot = 1;
> > > +
> > > mst_state->mgr = mgr;
> > > INIT_LIST_HEAD(&mst_state->vcpis);
> > >
> > > diff --git a/include/drm/drm_dp_mst_helper.h
> > > b/include/drm/drm_dp_mst_helper.h
> > > index ddb9231d0309..f8152dfb34ed 100644
> > > --- a/include/drm/drm_dp_mst_helper.h
> > > +++ b/include/drm/drm_dp_mst_helper.h
> > > @@ -554,6 +554,8 @@ struct drm_dp_mst_topology_state {
> > > struct drm_private_state base;
> > > struct list_head vcpis;
> > > struct drm_dp_mst_topology_mgr *mgr;
> > > + u8 total_avail_slots;
> > > + u8 start_slot;
> > > };
> > >
> > > #define to_dp_mst_topology_mgr(x) container_of(x, struct
> > > drm_dp_mst_topology_mgr, base)
> > > @@ -661,6 +663,16 @@ struct drm_dp_mst_topology_mgr {
> > > */
> > > int pbn_div;
> > >
> > > + /**
> > > + * @total_avail_slots: 63 for 8b/10b, 64 for 128/132b
> > > + */
> > > + u8 total_avail_slots;
> > > +
> > > + /**
> > > + * @start_slot: 1 for 8b/10b, 0 for 128/132b
> > > + */
> > > + u8 start_slot;
> > > +
> > > /**
> > > * @funcs: Atomic helper callbacks
> > > */
> > > @@ -806,6 +818,7 @@ int drm_dp_mst_get_vcpi_slots(struct
> > > drm_dp_mst_topology_mgr *mgr, struct drm_dp
> > >
> > > void drm_dp_mst_reset_vcpi_slots(struct drm_dp_mst_topology_mgr *mgr,
> > > struct drm_dp_mst_port *port);
> > >
> > > +void drm_dp_mst_update_coding_cap(struct drm_dp_mst_topology_state
> > > *mst_state, uint8_t link_coding_cap);
> > >
> > > void drm_dp_mst_deallocate_vcpi(struct drm_dp_mst_topology_mgr *mgr,
> > > struct drm_dp_mst_port *port);
>
--
Cheers,
Lyude Paul (she/her)
Software Engineer at Red Hat
