| Message ID | 20170105082442.xhiagp4765zyn4fg@phenom.ffwll.local (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@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
On Sat, Jan 07, 2017 at 12:35:36AM +0000, Pandiyan, Dhinakaran wrote: > 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@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? Yes :-) > > > + 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. Yes, swap state should go through all of them and not filter for a specific funcs. Probably best to have an internal/dangerous version with #define __drm_for_each_private_obj(__state, obj, obj_state, __i, __funcs) for ((__i) = 0; \ (__i) < (__state)->num_private_objs && \ ((obj) = (__state)->private_objs[__i].ptr, \ (__funcs) = (__state)->private_objs[__i].funcs, \ (obj_state) = (__state)->private_objs[__i].state, 1); \ (__i)++) \ for_each_if (__funcs) i.e. instead of filtering for funcs, assign the provided funcs pointer. Then swap_states and the cleanup functions could use that. > > > > > /** > > * 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. That's not an issue, since there's very few objects. And if it ever becomes an issue, we can easily add a hashtable or something for the void * -> entry lookup to speed it up. But a good thing to note, please add a comment to the commit message. -Daniel
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