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