On Wed, 2017-01-25 at 06:43 +0100, Daniel Vetter wrote:
> On Tue, Jan 24, 2017 at 03:49:30PM -0800, Dhinakaran Pandiyan wrote:
> > The avail_slots member in the MST topology manager is never updated to
> > reflect the available vcpi slots. The check is effectively against
> > total_slots. So, let's make that check obvious. Secondly, since the total
> > vcpi time slots is always 63 and does not depend on the link BW, remove
> > total_slots from MST topology manager struct. The third change is to
> > remove total_pbn which is hardcoded to 2560. The total PBN that the
> > topology manager allocates from depends on the link rate and is not a
> > constant. So, fix this by removing the total_pbn member itself. Finally,
> > make debug messages more descriptive.
>
> Ok, these are 3 different changes, and they need to be split up. Well, at
> least in 2 patches, to get the functional change out of there:
> - First hardcode avail_slots to 63, with the commit message explaining in
> detail why that's the right thing. You can already remove total_pbn and
> total_slots in that patch, since they will be unused. The commit message
> should have a reference to the DP spec to support that "it's always 63"
> claim.
> - Then garbage-collect ->avail_slots in a 2nd patch.
>
> If you smash these together it's a lot less obvious that this is not just
> a pure refactoring.
>
> Thanks, Daniel
>
Makes sense, will do this in the next revision.
-DK
> >
> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/drm_dp_mst_topology.c | 16 ++++++++--------
> > include/drm/drm_dp_mst_helper.h | 12 ------------
> > 2 files changed, 8 insertions(+), 20 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
> > index 122a1b0..d9edd84 100644
> > --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> > @@ -2042,10 +2042,6 @@ int drm_dp_mst_topology_mgr_set_mst(struct drm_dp_mst_topology_mgr *mgr, bool ms
> > goto out_unlock;
> > }
> >
> > - mgr->total_pbn = 2560;
> > - mgr->total_slots = DIV_ROUND_UP(mgr->total_pbn, mgr->pbn_div);
> > - mgr->avail_slots = mgr->total_slots;
> > -
> > /* add initial branch device at LCT 1 */
> > mstb = drm_dp_add_mst_branch_device(1, NULL);
> > if (mstb == NULL) {
> > @@ -2475,7 +2471,8 @@ int drm_dp_find_vcpi_slots(struct drm_dp_mst_topology_mgr *mgr,
> >
> > num_slots = DIV_ROUND_UP(pbn, mgr->pbn_div);
> >
> > - if (num_slots > mgr->avail_slots)
> > + /* max. time slots - one slot for MTP header */
> > + if (num_slots > 63)
> > return -ENOSPC;
> > return num_slots;
> > }
> > @@ -2489,7 +2486,8 @@ static int drm_dp_init_vcpi(struct drm_dp_mst_topology_mgr *mgr,
> >
> > num_slots = DIV_ROUND_UP(pbn, mgr->pbn_div);
> >
> > - if (num_slots > mgr->avail_slots)
> > + /* max. time slots - one slot for MTP header */
> > + if (num_slots > 63)
> > return -ENOSPC;
> >
> > vcpi->pbn = pbn;
> > @@ -2528,10 +2526,12 @@ bool drm_dp_mst_allocate_vcpi(struct drm_dp_mst_topology_mgr *mgr, struct drm_dp
> >
> > ret = drm_dp_init_vcpi(mgr, &port->vcpi, pbn);
> > if (ret) {
> > - DRM_DEBUG_KMS("failed to init vcpi %d %d %d\n", DIV_ROUND_UP(pbn, mgr->pbn_div), mgr->avail_slots, ret);
> > + DRM_DEBUG_KMS("failed to init vcpi slots=%d max=63 ret=%d\n",
> > + DIV_ROUND_UP(pbn, mgr->pbn_div), ret);
> > goto out;
> > }
> > - DRM_DEBUG_KMS("initing vcpi for %d %d\n", pbn, port->vcpi.num_slots);
> > + DRM_DEBUG_KMS("initing vcpi for pbn=%d slots=%d\n",
> > + pbn, port->vcpi.num_slots);
> > *slots = port->vcpi.num_slots;
> >
> > drm_dp_put_port(port);
> > diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h
> > index 27f3e99..b0f4a09 100644
> > --- a/include/drm/drm_dp_mst_helper.h
> > +++ b/include/drm/drm_dp_mst_helper.h
> > @@ -479,18 +479,6 @@ struct drm_dp_mst_topology_mgr {
> > * @pbn_div: PBN to slots divisor.
> > */
> > int pbn_div;
> > - /**
> > - * @total_slots: Total slots that can be allocated.
> > - */
> > - int total_slots;
> > - /**
> > - * @avail_slots: Still available slots that can be allocated.
> > - */
> > - int avail_slots;
> > - /**
> > - * @total_pbn: Total PBN count.
> > - */
> > - int total_pbn;
> >
> > /**
> > * @qlock: protects @tx_msg_downq, the tx_slots in struct
> > --
> > 2.7.4
> >
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
