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 05:46:30PM +0300, Ville Syrjälä wrote:
> On Thu, Apr 09, 2015 at 07:10:57AM -0700, Matt Roper wrote:
> > 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@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.
> 
> Well yeah, we want to change the clipping computation to use the future
> state but I don't think were ready for it yet. At least I wouldn't want
> to make this change until the watermark code gets fixed and we clean up
> the primary/cursor plane state handling.
> 
> For instance what happens if you dpms off and then enable a plane? If
> the clipping is based on the enabled state of the crtc then it'll try
> to enable the plane, no?

Yeah, I think that was Daniel's concern too... ->enable is the wrong
field to be pulling out of the state object here and we should be using
->active instead since that takes DPMS and such into account.

I'm not sure if ->active is properly updated in our CRTC state today by
the legacy modesetting codepaths, but if we can get that squared away
and switch to using crtc_state->active rather than crtc_state->enable do
you forsee any other issues with pulling the value out of the in-flight
state like this patch tries to do?


Matt

> 
> -- 
> Ville Syrjälä
> Intel OTC

-- 
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