First off, sidenote: I wonder if we even need total_avail_slots and
could just use start_slot. Anyway, more productive comments below:
On Mon, 2021-10-18 at 15:47 -0400, Bhawanpreet Lakha wrote:
> I understand the mst_state argument its just that most of the mst
> functions are using mst_mgr/port structs and there is no easy way to
> extract the mst_state using mgr/port in these places
> (drm_dp_update_payload_part1, drm_dp_mst_allocate_vcpi, drm_dp_init_vcpi
> etc) where we need the slot info.
>
Ahh, I see why this might be confusing. I think surprisingly though,
this actually should probably be OK. Mostly, two of these functions
don't actually need the slot count and one I think I have a workaround
for:
* drm_dp_init_vcpi() - This function does use the total slot count here:
if (slots > 63)
return -ENOSPC;
However, drm_dp_init_vcpi() assigns the current payload which means
it's called by the driver at commit time, not atomic check time. Since
atomic commits are only allowed to happen after we've successfully
tested the state in question, we aren't allowed to fail them in the
middle of a commit - which is definitely what we're doing in
drm_dp_init_vcpi(). So, I'd actually say we should either totally
ignore this bit of drm_dp_init_vcpi() or preferably, just entirely
drop it in a prerequisite patch.
(If you aren't familiar with atomic modesetting, the reason we "can't"
just fail in the middle of committing a new atomic state is because we
may very well have already committed that state to hardware partially.
So, there's no nice way of aborting at that point without seriously
complicating things - hence the need for having an atomic check before
commits)
* drm_dp_mst_allocate_vcpi() - this seems to only be an issue because of
drm_dp_init_vcpi(), and we additionally print the maximum number of
slots in a drm_dbg_kms() message:
drm_dbg_kms(mgr->dev, "failed to init vcpi slots=%d max=63 ret=%d\n",
…
Since we should have already decided
all of the new payloads by the time we're in drm_dp_mst_allocate_vcpi
(again, since we're in atomic commit), that's probably not the best
place for us to print that anyway. So, I think we'd be fine just
dropping the "max=63" from that string.
* drm_dp_update_payload_part1() - This one does need the current slot
count, you're right. I would normally say we should just fix this
function and move the payload info over to the atomic state, which is
the eventual plan, but doing that would require dealing with the
radeon.ko MST mess which is probably too much to ask for with
something as simple as this patch. I think I know a workaround though:
Let's keep this new slot info (e.g. num_slots, and possibly
total_avail_slots if we keep that) in the MST atomic state, _but_ as a
temporary solution we can instead add a start_slot argument to
drm_dp_update_payload_part1(). That way we have an easy solution for
making sure radeon still compiles, and atomic drivers can just extract
the start_slot info themselves from the topology state and pass it
into drm_dp_update_payload_part1(). Then I can get rid of that
start_slots argument at a later date when I've started moving all of
the payload info for MST into the atomic state.
If we do this solution though, we should definitely document in the
patch and in the kdocs for drm_dp_update_payload_part1() that passing
the start slot is a temporary workaround for non-atomic drivers, and
will be removed when non-atomic portions of the MST helpers have been
moved out of helpers and into atomic state.
Does this sound good? There's a lot of moving pieces here so hopefully I
didn't miss anything
> So either we need to keep a copy of the slots in the mgr because that's
> what most of the code is using right now or pass around the atomic state
> to get the mgr->state mapping. (I don't have much experience with the
> mst code so maybe I am missing some key detail here?)
Worst case scenario, if there was something I missed that implies we DO
need access to a mgr->state mapping, I might be OK with us using that in
the interim for these patches. I don't think we need that quite yet as
far as I can tell though.
>
>
> Thanks,
>
> Bhawan
>
>
> On 2021-10-15 4:41 p.m., Lyude Paul wrote:
> > [more snip]
> >
> > On Fri, 2021-10-15 at 15:43 -0400, Bhawanpreet Lakha wrote:
> > > Thanks for the response,
> > >
> > > That function is per port so not sure how that will work. Also we only
> > > need to check this inside drm_dp_mst_atomic_check_vcpi_alloc_limit(),
> > > which doesn't have a state.
> > >
> > > 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() and then update the mgr->slot in
> > > commit.
> > TBH - I think we can actually just get away with having all of this info
> > in
> > drm_dp_mst_topology_state
> >
> > >
> > > 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
