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

[Matt Roper]

On Thu, Apr 09, 2015 at 04:18:56PM +0300, Ville Syrjälä 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 at 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;
> 
> We depend on the clipping to keep planes from getting enabled on a
> disabled pipe. So I think this is going to blow up.

That was why I made these changes...the idea here was that we should be
basing that clipping on what the CRTC state is going to be when the
plane state is actually committed, not what it happens to be now.  So if
the CRTC is going to be disabled, this should ensure that the planes are
properly clipped to off, even if the CRTC happens to be running at the
moment.  Conversely, if the CRTC is off at the moment, but will be on at
the end of this transaction, we want to make sure that the planes are
not improperly clipped to invisible, otherwise they won't show up.

Am I overlooking something?  It seems the current 'active' value is
unrelated to what we actually want to be basing our plane programming on
and just happens to work at the moment since we don't yet let  ->active
change over the course of an atomic transaction (meaning the final state
will always be the same as the current state).


Matt

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795