Re: [PATCH 06/17] drm: Global atomic state handling

[Daniel Vetter <daniel@ffwll.ch>]

On Mon, Nov 03, 2014 at 03:41:32PM -0800, Matt Roper wrote:
> On Sun, Nov 02, 2014 at 02:19:19PM +0100, Daniel Vetter wrote:
> ...
> > +/**
> > + * drm_atomic_get_plane_state - get plane state
> > + * @state: global atomic state object
> > + * @plane: plane to get state object for
> > + *
> > + * This functions returns the plane state for the given plane, allocating it if
> > + * needed. It will also grab the relevant plane lock to make sure that the state
> > + * is consistent.
> > + *
> > + * Returns:
> > + *
> > + * Either the allocated state or the error code encoded into the pointer. When
> > + * the error is EDEADLK then the w/w mutex code has detected a deadlock and the
> > + * entire atomic sequence must be restarted. All other errors are fatal.
> > + */
> > +struct drm_plane_state *
> > +drm_atomic_get_plane_state(struct drm_atomic_state *state,
> > +			  struct drm_plane *plane)
> > +{
> > +	int ret, index;
> > +	struct drm_plane_state *plane_state;
> > +
> > +	index = drm_plane_index(plane);
> > +
> > +	if (state->plane_states[index])
> > +		return state->plane_states[index];
> > +
> > +	/*
> > +	 * TODO: We currently don't have per-plane mutexes. So instead of trying
> > +	 * crazy tricks with deferring plane->crtc and hoping for the best just
> > +	 * grab all crtc locks. Once we have per-plane locks we must update this
> > +	 * to only take the plane mutex.
> > +	 */
> > +	ret = drm_modeset_lock_all_crtcs(state->dev, state->acquire_ctx);
> > +	if (ret)
> > +		return ERR_PTR(ret);
> > +
> > +	plane_state = plane->funcs->atomic_duplicate_state(plane);
> > +	if (!plane_state)
> > +		return ERR_PTR(-ENOMEM);
> > +
> > +	state->plane_states[index] = plane_state;
> > +	state->planes[index] = plane;
> > +	plane_state->state = state;
> > +
> > +	DRM_DEBUG_KMS("Added [PLANE:%d] %p state to %p\n",
> > +		      plane->base.id, plane_state, state);
> > +
> > +	if (plane_state->crtc) {
> > +		struct drm_crtc_state *crtc_state;
> > +
> > +		crtc_state = drm_atomic_get_crtc_state(state,
> > +						       plane_state->crtc);
> > +		if (IS_ERR(crtc_state))
> > +			return ERR_CAST(crtc_state);
> > +	}
> 
> It's not immediately clear to me why we need to get (create) the crtc
> state here.  Is this just so that we know to do an
> atomic_begin()/atomic_flush() on this crtc later or do we actually use
> the state itself somewhere that I'm overlooking?

I do the same when grabbing the connector state and also add the crtc to
the lookup table. The reasoning is that essentially I couldn't come up
with any sane driver implementation where you would not want to have the
CRTC state (maybe just for a bit of book-keeping) if you either have a
plane or a connector using it. And then grabbing the state will also grab
the locks, which is fairly convenient.

Note that the core functions to change the links will grab the state+lock
of the new target for you, too.

Now what I didn't do is grab _all_ the connectors/planes for a given crtc
as soon as you grab the crtcs state. Especially for plane updates I think
most drivers don't need that, and even if you do a mode change you might
be able to implement that without touching the connectors at all. E.g. on
i915 we can change the pfit to change the "mode" on panels.

But there are helpers to grab all affected connectors since that's needed
by the new atomic modeset helpers. This stuff here is core, and I've put
it here since I really couldn't make up a sane driver that wouldn't need
this.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel