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

[Daniel Vetter <daniel@ffwll.ch>]

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@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;
+	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))
+
 /**
  * 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);
}

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx