On Thu, Apr 09, 2015 at 04:45:04PM +0200, Daniel Vetter wrote:
> On Thu, Apr 09, 2015 at 07:00:31AM -0700, Matt Roper wrote:
> > 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@xxxxxxxxx>
> > > > ---
> > > > 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.
>
> We add both the old and the new crtc state, so when you disable a plane
> you should have the crtc state at hand. Except when a plane is already
> disable, but I guess in that case we should just not call any of the
> callbacks?
> -Daniel
Right, if both old and new state both have NULL CRTC's, then no state
gets added. That typically happens when you update a property of a
disabled plane that stays disabled (which is easily seen with fbdev
restoration resetting rotation for all planes, even the disabled ones).
The driver's check function still gets called in this case, but i915
just bails out because we don't have any driver-specific checking that
needs to be done.
Matt
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
--
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
