Re: [PATCH v2 2/9] drm/dp: Kill unused MST vcpi slot availability tracking

["Pandiyan, Dhinakaran" <dhinakaran.pandiyan@intel.com>]

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
> > 
> 

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx