On Wed, 2021-10-13 at 15:33 -0400, Bhawanpreet Lakha wrote:
>
> > I wonder if we could split this to separate drm dp helper and amd driver
> > patches?
Whoops! I thought it was strange that I would say this but it seems there was
a misunderstanding on my part: when the original patch series was submitted I
was only CC'd on the first patch and I guess I must not have noticed the 1/2
in the subject line, so I thought Jerry had submitted just a single patch for
the helper. Looking back in my email history though that definitely wasn't
correct, and the original patch structure was what we wanted to go with.
Sorry for the confusion on my part!
> >
> I believe that was the original structure but, lyude recommended to put
> them into the same patch to see how it is being used
> > > /**
> > > * Streams and planes are reset when there are changes that
> > > affect
> > > * bandwidth. Anything that affects bandwidth needs to go
> > > through
> > > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> > > b/drivers/gpu/drm/drm_dp_mst_topology.c
> > > index ad0795afc21c..fb5c47c4cb2e 100644
> > > --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> > > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> > > @@ -3368,7 +3368,7 @@ int drm_dp_update_payload_part1(struct
> > > drm_dp_mst_topology_mgr *mgr)
> > > struct drm_dp_payload req_payload;
> > > struct drm_dp_mst_port *port;
> > > int i, j;
> > > - int cur_slots = 1;
> > > + int cur_slots = mgr->start_slot;
> > > bool skip;
> > >
> > > mutex_lock(&mgr->payload_lock);
> > > @@ -4321,7 +4321,7 @@ int drm_dp_find_vcpi_slots(struct
> > > drm_dp_mst_topology_mgr *mgr,
> > > num_slots = DIV_ROUND_UP(pbn, mgr->pbn_div);
> > >
> > > /* max. time slots - one slot for MTP header */
> > > - if (num_slots > 63)
> > > + if (num_slots > mgr->total_avail_slots)
> > > return -ENOSPC;
> > > return num_slots;
> > > }
> > > @@ -4333,7 +4333,7 @@ static int drm_dp_init_vcpi(struct
> > > drm_dp_mst_topology_mgr *mgr,
> > > int ret;
> > >
> > > /* max. time slots - one slot for MTP header */
> > > - if (slots > 63)
> > > + if (slots > mgr->total_avail_slots)
> > > return -ENOSPC;
> > >
> > > vcpi->pbn = pbn;
> > > @@ -4507,6 +4507,18 @@ int drm_dp_atomic_release_vcpi_slots(struct
> > > drm_atomic_state *state,
> > > }
> > > EXPORT_SYMBOL(drm_dp_atomic_release_vcpi_slots);
> > >
> > > +void drm_dp_mst_update_coding_cap(struct drm_dp_mst_topology_state
> > > *mst_state, uint8_t link_coding_cap)
> > > +{
> > > + if (link_coding_cap == DP_CAP_ANSI_128B132B) {
> > > + mst_state->total_avail_slots = 64;
> > > + mst_state->start_slot = 0;
> > > + }
> > The values never change AFAICT, should we store the channel encoding
> > instead, and use that information to initialize the values?
> >
> > (Alternatively, why aren't the 8b/10b values initialized here if
> > 128b/132b are?)
> I agree, 8b/10 are the default, but in case where we switch from
> 128b/132 -> 8b/10b we should be updating them here aswell.
> > > +
> > > + DRM_DEBUG_KMS("%s coding format on mgr 0x%p\n",
> > > + (link_coding_cap == DP_CAP_ANSI_128B132B) ?
> > > "128b/132b":"8b/10b", mst_state->mgr);
> > > +}
> > > +EXPORT_SYMBOL(drm_dp_mst_update_coding_cap);
> > > +
> > > /**
> > > * drm_dp_mst_allocate_vcpi() - Allocate a virtual channel
> > > * @mgr: manager for this port
> > > @@ -4538,8 +4550,8 @@ bool drm_dp_mst_allocate_vcpi(struct
> > > drm_dp_mst_topology_mgr *mgr,
> > >
> > > ret = drm_dp_init_vcpi(mgr, &port->vcpi, pbn, slots);
> > > if (ret) {
> > > - drm_dbg_kms(mgr->dev, "failed to init vcpi slots=%d
> > > max=63 ret=%d\n",
> > > - DIV_ROUND_UP(pbn, mgr->pbn_div), ret);
> > > + drm_dbg_kms(mgr->dev, "failed to init vcpi slots=%d
> > > max=%d ret=%d\n",
> > > + DIV_ROUND_UP(pbn, mgr->pbn_div), mgr-
> > > >total_avail_slots, ret);
> > > drm_dp_mst_topology_put_port(port);
> > > goto out;
> > > }
> > > @@ -5226,7 +5238,7 @@ drm_dp_mst_atomic_check_vcpi_alloc_limit(struct
> > > drm_dp_mst_topology_mgr *mgr,
> > > struct
> > > drm_dp_mst_topology_state *mst_state)
> > > {
> > > struct drm_dp_vcpi_allocation *vcpi;
> > > - int avail_slots = 63, payload_count = 0;
> > > + int avail_slots = mgr->total_avail_slots, payload_count = 0;
> > >
> > > list_for_each_entry(vcpi, &mst_state->vcpis, next) {
> > > /* Releasing VCPI is always OK-even if the port is gone
> > > */
> > > @@ -5255,7 +5267,7 @@ drm_dp_mst_atomic_check_vcpi_alloc_limit(struct
> > > drm_dp_mst_topology_mgr *mgr,
> > > }
> > > }
> > > drm_dbg_atomic(mgr->dev, "[MST MGR:%p] mst state %p VCPI
> > > avail=%d used=%d\n",
> > > - mgr, mst_state, avail_slots, 63 - avail_slots);
> > > + mgr, mst_state, avail_slots, mgr-
> > > >total_avail_slots - avail_slots);
> > >
> > > return 0;
> > > }
> > > @@ -5421,6 +5433,10 @@ int drm_dp_mst_atomic_check(struct
> > > drm_atomic_state *state)
> > > break;
> > >
> > > mutex_lock(&mgr->lock);
> > > +
> > > + mgr->start_slot = mst_state->start_slot;
> > > + mgr->total_avail_slots = mst_state->total_avail_slots;
> > > +
> > It feels wrong to me to be copying stuff from mst_state to mgr in
> > general, and in atomic check hook in particular.
> previously we were setting it directly in the mgr, and this was
> suggested by lyude. I am not sure what the alternative is.
> > > 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;
> > > +
> > The magic values for 8b/10b are now duplicated to two places, with the
> > 128b/132b values in a separate place.
>
> 8b/10b is the default (to make sure we don't break any existing driver
> that doesn't use 128b/132b). Are you against setting it as the default
> here? or do you mean we should use #define for this? the magic numbers
> are currently being used directly right now (inside
> drm_dp_find_vcpi_slots, drm_dp_init_vcpi).
>
> Bhawan
>
> > BR,
> > Jani.
> >
> > > 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
