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

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, Apr 09, 2015 at 07:54:14AM -0700, Matt Roper wrote:
> 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?

On reconsideration ->enable is imo the right field we should use for wm
computation and everything. Otherwise resource constraint checking will be
fail and a dpms on might not work, and that's not good.

Ofc we can't just try to enable planes when the crtc is off, but imo that
needs to be fixed differently. Rough plan (which is what Maarten's working
on).
- Unconditionally shut down planes in crtc_disable, bypassing any state
  changes. Maarten is working on some force plane disable patches to
  overwrite the ->visible checks.
- When a crtc is in the disable_pipes or modeset_pipes sets add all the
  plane states to the atomic update (in the crtc compute_config functions
  with a hack for disabled pipes until we get that one right). Also call
  plane atomic_check functions for all planes in the state update.
- Remove the plane enable code from crtc_enable and instead just do an
  atomic plane update (as we already have from Matt's atomic work) at the
  very end of the modeset sequence, but _only_ when a crtc is active.

This way we shouldn't have any issue with checking state and ->active is
only gating what hw we enable.

What we might want to change is pimp the shared atomic helpers to not call
any atomic_check functions for planes/crtcs if the crtc is not enabled. I
think that should functionally substitute this patch here.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx