Re: [PATCH 1/4] drm/i915: Make atomic use in-flight state for CRTC active value

[Matt Roper <matthew.d.roper@intel.com>]

On Thu, Apr 09, 2015 at 02:42:36PM +0200, Daniel Vetter wrote:
> On Wed, Apr 08, 2015 at 06:56:51PM -0700, Matt Roper wrote:
> > Our atomic plane code currently uses intel_crtc->active to determine
> > how/when to update some derived state values.  This works fine for pure
> > plane updates at the moment since the CRTC state itself isn't changed as
> > part of the operation.  However as we convert more of our driver
> > internals over to atomic modesetting, we need to look at whether the
> > CRTC will be active at the *end* of the atomic transaction (which may
> > not match the currently committed state).
> > 
> > The in-flight value we want to use is generally in a crtc_state object
> > associated with our top-level atomic transaction.  However there are a
> > few cases where this isn't the case:
> > 
> >  * While using transitional atomic helpers (as we are at the moment),
> >    SetPlane() calls will operate on orphaned plane states that aren't
> >    part of a top-level atomic transaction.  In this case, we're not
> >    touching the CRTC state, so it's fine to use the already-committed
> >    value from crtc->state.
> > 
> >  * While updating properties of a disabled plane, we'll have a top-level
> >    atomic state, but it may not contain the CRTC state we're looking
> >    for.  Once again, this means we're not actually touching any CRTC
> >    state so it's safe to use the value from crtc->state directly.
> > 
> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_atomic_plane.c | 11 +++--
> >  drivers/gpu/drm/i915/intel_display.c      | 73 +++++++++++++++++++++++++++++--
> >  drivers/gpu/drm/i915/intel_drv.h          |  3 ++
> >  drivers/gpu/drm/i915/intel_sprite.c       | 16 +++++--
> >  4 files changed, 93 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
> > index 976b891..90c4a82 100644
> > --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
> > +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
> > @@ -111,12 +111,17 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
> >  {
> >  	struct drm_crtc *crtc = state->crtc;
> >  	struct intel_crtc *intel_crtc;
> > +	struct intel_crtc_state *intel_crtc_state;
> >  	struct intel_plane *intel_plane = to_intel_plane(plane);
> >  	struct intel_plane_state *intel_state = to_intel_plane_state(state);
> > +	bool active;
> >  
> >  	crtc = crtc ? crtc : plane->crtc;
> >  	intel_crtc = to_intel_crtc(crtc);
> >  
> > +	intel_crtc_state = intel_crtc_state_for_plane(intel_state);
> > +	active = intel_crtc_state->base.enable;
> > +
> >  	/*
> >  	 * Both crtc and plane->crtc could be NULL if we're updating a
> >  	 * property while the plane is disabled.  We don't actually have
> > @@ -143,10 +148,8 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
> >  	/* Clip all planes to CRTC size, or 0x0 if CRTC is disabled */
> >  	intel_state->clip.x1 = 0;
> >  	intel_state->clip.y1 = 0;
> > -	intel_state->clip.x2 =
> > -		intel_crtc->active ? intel_crtc->config->pipe_src_w : 0;
> > -	intel_state->clip.y2 =
> > -		intel_crtc->active ? intel_crtc->config->pipe_src_h : 0;
> > +	intel_state->clip.x2 = active ? intel_crtc_state->pipe_src_w : 0;
> > +	intel_state->clip.y2 = active ? intel_crtc_state->pipe_src_h : 0;
> >  
> >  	/*
> >  	 * Disabling a plane is always okay; we just need to update
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 7bfe2af..88b0f69 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -12562,6 +12562,53 @@ intel_cleanup_plane_fb(struct drm_plane *plane,
> >  	}
> >  }
> >  
> > +/**
> > + * intel_crtc_state_for_plane - Obtain CRTC state for a plane
> > + * @pstate: plane state to lookup corresponding crtc state for
> > + *
> > + * When working with a top-level atomic transaction (drm_atomic_state),
> > + * a CRTC state should be present that corresponds to the provided
> > + * plane state; this function provides a quick way to fetch that
> > + * CRTC state.  In cases where we have a plane state unassociated with any
> > + * top-level atomic transaction (e.g., while using the transitional atomic
> > + * helpers), the current CRTC state from crtc->state will be returned
> > + * instead.
> > + */
> > +struct intel_crtc_state *
> > +intel_crtc_state_for_plane(struct intel_plane_state *pstate)
> > +{
> > +	struct drm_atomic_state *state = pstate->base.state;
> > +	struct intel_plane *plane = to_intel_plane(pstate->base.plane);
> > +	struct drm_crtc *crtc = intel_get_crtc_for_pipe(pstate->base.plane->dev,
> > +							plane->pipe);
> > +	int i;
> > +
> > +	/*
> > +	 * While using transitional plane helpers, we may not have a top-level
> > +	 * atomic transaction.  Of course that also means that we're not
> > +	 * actually touching CRTC state, so just return the currently
> > +	 * committed state.
> > +	 */
> 
> Imo this needs a big FIXME at the top of the comment ;-)
> 
> > +	if (!state)
> > +		return to_intel_crtc_state(crtc->state);
> > +
> > +	for (i = 0; i < state->dev->mode_config.num_crtc; i++) {
> > +		if (!state->crtcs[i])
> > +			continue;
> > +
> > +		if (to_intel_crtc(state->crtcs[i])->pipe == plane->pipe)
> > +			return to_intel_crtc_state(state->crtc_states[i]);
> > +	}
> > +
> > +	/*
> > +	 * We may have a plane state without a corresponding CRTC state if
> > +	 * we're updating a property of a disabled plane.  Again, just using
> > +	 * the already-committed state for this plane's CRTC should be fine
> > +	 * since we're not actually touching the CRTC here.
> > +	 */
> 
> Is this really still true with Ander's patches? If the udpate is part of a
> drm_atomic_state structure, then we should always have the corresponding
> crtc state handy I think. Which cases still fail this assumption?

Any time a transaction updates a plane, the corresponding CRTC state (as
defined by plane_state->crtc) should get added to the transaction by the
atomic core code (specifically in drm_atomic_get_plane_state).  But when
a plane is disabled, state->crtc is NULL so there simply is no
associated CRTC to add (at least as far as the core is concerned).  So
you wind up with just a plane state being added to the top-level atomic
state in that case.


Matt

> 
> > +	return to_intel_crtc_state(crtc->state);
> > +}
> > +
> >  static int
> >  intel_check_primary_plane(struct drm_plane *plane,
> >  			  struct intel_plane_state *state)
> > @@ -12570,15 +12617,20 @@ intel_check_primary_plane(struct drm_plane *plane,
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	struct drm_crtc *crtc = state->base.crtc;
> >  	struct intel_crtc *intel_crtc;
> > +	struct intel_crtc_state *intel_crtc_state;
> >  	struct drm_framebuffer *fb = state->base.fb;
> >  	struct drm_rect *dest = &state->dst;
> >  	struct drm_rect *src = &state->src;
> >  	const struct drm_rect *clip = &state->clip;
> > +	bool active;
> >  	int ret;
> >  
> >  	crtc = crtc ? crtc : plane->crtc;
> >  	intel_crtc = to_intel_crtc(crtc);
> >  
> > +	intel_crtc_state = intel_crtc_state_for_plane(state);
> > +	active = intel_crtc_state->base.enable;
> 
> This is the wrong one I think. drm_crtc_state->enable is the logical
> enabling state, i.e. whether there's connectors connected to the crtc and
> a mode set. state->active is the desired hw state, i.e. taking dpms and
> all that into account, and hence reflecting our intel_crtc->active hw
> tracking boolean (they match names intentionally).
> 
> Do we still miss cases where we update crtc_state->active or what's the
> reason for picking ->enable here?

I'll have to double check; I thought this was a little fishy, but it
looked like our i915 code wasn't actually setting ->active anywhere yet
in our legacy modeset codepaths (although maybe I just missed it).  We
do set ->enable directly to intel_crtc->active in a few places though,
which is why I went with that one for now.


Matt

> -Daniel
> 

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx