[Intel-gfx] [PATCH 4/6] drm/dp: Introduce DP MST topology manager state to track DP link bw

[Pandiyan, Dhinakaran]

On Thu, 2017-01-05 at 09:24 +0100, Daniel Vetter wrote:
> On Thu, Jan 05, 2017 at 03:54:54AM +0000, Pandiyan, Dhinakaran wrote:
> > On Wed, 2017-01-04 at 19:20 +0000, Pandiyan, Dhinakaran wrote:
> > > On Wed, 2017-01-04 at 10:33 +0100, Daniel Vetter wrote:
> > > > On Tue, Jan 03, 2017 at 01:01:49PM -0800, Dhinakaran Pandiyan wrote:
> > > > > Link bandwidth is shared between multiple display streams in DP MST
> > > > > configurations. The DP MST topology manager structure maintains the shared
> > > > > link bandwidth for a primary link directly connected to the GPU. For atomic
> > > > > modesetting drivers, checking if there is sufficient link bandwidth for a
> > > > > mode needs to be done during the atomic_check phase to avoid failed
> > > > > modesets. Let's encsapsulate the available link bw information in a state
> > > > > structure so that bw can be allocated and released atomically for each of
> > > > > the ports sharing the primary link.
> > > > > 
> > > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan at intel.com>
> > > > 
> > > > Overall issue with the patch is that dp helpers now have 2 places where
> > > > available_slots is stored: One for atomic drivers in ->state, and the
> > > > legacy one. I think it'd be good to rework the legacy codepaths (i.e.
> > > > drm_dp_find_vcpi_slots) to use mgr->state->avail_slots, and remove
> > > > mgr->avail_slots entirely.
> > > 
> > > PATCH 2/6 does that. mgr->avail_slots is not updated in the legacy code
> > > path, so the check turns out to be against mgr->total_slots. So, I did
> > > just that, albeit explicitly. 
> 
> Ah right, I missed that.
> 
> > > > > diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> > > > > index fd2d971..7ac5ed6 100644
> > > > > --- a/include/drm/drm_atomic.h
> > > > > +++ b/include/drm/drm_atomic.h
> > > > > @@ -153,6 +153,11 @@ struct __drm_connnectors_state {
> > > > >  	struct drm_connector_state *state;
> > > > >  };
> > > > >  
> > > > > +struct __drm_dp_mst_topology_state {
> > > > > +	struct drm_dp_mst_topology_mgr *ptr;
> > > > > +	struct drm_dp_mst_topology_state *state;
> > > > 
> > > > One way to fix that control inversion I mentioned above is to use void*
> > > > pionters here, and then have callbacks for atomic_destroy and swap_state
> > > > on top. A bit more shuffling, but we could then use that for other driver
> > > > private objects.
> > > > 
> > > > Other option is to stuff it into intel_atomic_state.
> > 
> > Hmm... I think I understand what you are saying. The core atomic
> > functions like swap_state should not be able alter the topology
> > manager's current state?
> > 
> > Did you mean something like this - https://paste.ubuntu.com/23743485/ ?
> 
> Not quite yet, here's what I had in mind as a sketch:
> 
> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> index 2e28fdca9c3d..6ce704b1e900 100644
> --- a/include/drm/drm_atomic.h
> +++ b/include/drm/drm_atomic.h
> @@ -154,6 +154,17 @@ struct __drm_connnectors_state {
>  	struct drm_connector_state *state;
>  };
>  
> +struct drm_private_state_funcs {
> +	void (*swap)(void *obj, void *state);
> +	void (*destroy_state)(void *state);
> +};
> +
> +struct __drm_private_obj_state {
> +	struct obj *ptr;
> +	struct obj_state *state;

Thanks for the sketch Daniel, I have a couple of questions.
Should this be void *obj and void *obj_state? 

> +	struct drm_private_state_funcs *funcs;
> +}
> +
>  /**
>   * struct drm_atomic_state - the global state object for atomic updates
>   * @ref: count of all references to this state (will not be freed until zero)
> @@ -178,6 +189,8 @@ struct drm_atomic_state {
>  	struct __drm_crtcs_state *crtcs;
>  	int num_connector;
>  	struct __drm_connnectors_state *connectors;
> +	int num_private_objs;
> +	struct __drm_private_obj_state *private_objs;
>  
>  	struct drm_modeset_acquire_ctx *acquire_ctx;
>  
> @@ -414,6 +427,19 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p);
>  	     (__i)++)							\
>  		for_each_if (plane_state)
>  
> +/* The magic here is that if obj and obj_state have the right type, then this
> + * will automatically cast to the right type. Since we allow any kind of private
> + * object mixed into the same array, runtime type casting is done using the
> + * funcs pointer.
> + */
> +#define for_each_private_obj(__state, obj, obj_state, __i, funcs)
> +	for ((__i) = 0;							\
> +	     (__i) < (__state)->num_private_objs &&				\
> +	     ((obj) = (__state)->private_objs[__i].ptr,			\
> +	     (obj_state) = (__state)->private_objs[__i].state, 1); 	\
> +	     (__i)++)							\
> +		for_each_if ((__state)->private_objs[__i].funcs == (funcs))
> +

So, this macro iterates through a specific type of private obj in the
array. You are not implying that drm_atomic_helper_swap_state is
expected to use this, right? If we don't do
"((__state)->private_objs[__i].funcs == (funcs))", we could swap the
state for all private objs.



>  /**
>   * drm_atomic_crtc_needs_modeset - compute combined modeset need
>   * @state: &drm_crtc_state for the CRTC
> 
> I didn't sketch helper functions for looking up/inserting objects, and ofc
> we'd need the boilerplat for cleanup/swap, but I hope that part is clear.
> 
> If we add a duplicate_state callback to drm_private_state_funcs then we
> might even be able to abstract away the entire lookup-or-dupliocate logic
> into a helper, and all that's left for a specific implementation would be
> 
> struct drm_dp_mst_topology_state*
> drm_dp_mst_get_atomic_state(struct drm_atomic_state *state,
> 			    struct dp_mst_topology_mgr *mgr)
> {
> 	return drm_atomic_get_private_state(state, mgr, mgr->state,
> 					    &dp_mst_state_funcs);
> }
> 

I like this idea, except for the part we have to do a linear search for
lookups.

-DK

> The upside of going to all this trouble is that we could reuse this for
> all the other driver private state, like dpll, or wm or whatever. And we
> wouldn't need to type the same boring boilerplate for all of them, since
> that would all be hidden. And since I expect that there will be more&more
> use-cases and needs for driver private atomic state for all the fancy
> features coming down the pipeline, this might be worth it. But not yet
> sure.
> 
> > Do we need the destroy callback too? drm_atomic_state_default_clear()
> > does not have to dereference drm_dp_mst_topology_mgr.
> > 
> > Moving this to intel_atomic_state would be mean that nouveau will
> > require a re-implementation. Is that right?
> 
> Yeah, that's the downside, and I think we could also make other
> driver-private objects a bit easier to handle.
> -Daniel