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 -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx