| Message ID | 1485301777-3465-3-git-send-email-dhinakaran.pandiyan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > --- > 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 >
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@intel.com> > > --- > > 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 > > >
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
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. Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> --- drivers/gpu/drm/drm_dp_mst_topology.c | 16 ++++++++-------- include/drm/drm_dp_mst_helper.h | 12 ------------ 2 files changed, 8 insertions(+), 20 deletions(-)