[Intel-gfx] [PATCH 4/9] drm/i915: Make derived plane state correct after crtc_enable

[Ville Syrjälä]

On Wed, Mar 11, 2015 at 05:23:27PM +0100, Daniel Vetter wrote:
> On Wed, Mar 11, 2015 at 02:19:38PM +0200, Ville Syrjälä wrote:
> > On Wed, Mar 11, 2015 at 11:24:34AM +0100, Daniel Vetter wrote:
> > > On Wed, Mar 11, 2015 at 12:05:39PM +0200, Ville Syrjälä wrote:
> > > > On Wed, Mar 11, 2015 at 10:52:29AM +0100, Daniel Vetter wrote:
> > > > > On Tue, Mar 10, 2015 at 07:57:13PM +0200, Ville Syrjälä wrote:
> > > > > > On Tue, Mar 10, 2015 at 10:01:51AM -0700, Matt Roper wrote:
> > > > > > > On Tue, Mar 10, 2015 at 01:15:24PM +0200, ville.syrjala at linux.intel.com wrote:
> > > > > > > > From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > > > > > > -static void disable_plane_internal(struct drm_plane *plane)
> > > > > > > > +static void _intel_crtc_enable_planes(struct intel_crtc *crtc)
> > > > > > > >  {
> > > > > > > > -	struct intel_plane *intel_plane = to_intel_plane(plane);
> > > > > > > > -	struct drm_plane_state *state =
> > > > > > > > -		plane->funcs->atomic_duplicate_state(plane);
> > > > > > > > -	struct intel_plane_state *intel_state = to_intel_plane_state(state);
> > > > > > > > +	struct drm_device *dev = crtc->base.dev;
> > > > > > > > +	enum pipe pipe = crtc->pipe;
> > > > > > > > +	struct intel_plane *plane;
> > > > > > > > +	const struct drm_crtc_helper_funcs *crtc_funcs =
> > > > > > > > +		crtc->base.helper_private;
> > > > > > > >  
> > > > > > > > -	intel_state->visible = false;
> > > > > > > > -	intel_plane->commit_plane(plane, intel_state);
> > > > > > > > +	for_each_intel_plane(dev, plane) {
> > > > > > > > +		const struct drm_plane_helper_funcs *funcs =
> > > > > > > > +			plane->base.helper_private;
> > > > > > > > +		struct intel_plane_state *state =
> > > > > > > > +			to_intel_plane_state(plane->base.state);
> > > > > > > >  
> > > > > > > > -	intel_plane_destroy_state(plane, state);
> > > > > > > > +		if (plane->pipe != pipe)
> > > > > > > > +			continue;
> > > > > > > > +
> > > > > > > > +		if (funcs->atomic_check(&plane->base, &state->base))
> > > > > > > 
> > > > > > > Maybe add a WARN_ON() here?  I'm assuming that this shouldn't really be
> > > > > > > possible since if this fails it means we've already previously done a
> > > > > > > commit of invalid state on a previous atomic transaction.  But if it
> > > > > > > does somehow happen, the WARN will give us a clue why the plane contents
> > > > > > > simply didn't show up.
> > > > > > 
> > > > > > I can think of one way to make it fail. That is, first set a smaller
> > > > > > mode with the primary plane (and fb) configured to cover that fully, and
> > > > > > then switch to a larger mode without reconfiguring the primary plane. If
> > > > > > the hardware requires the primary plane to be fullscreen it'll fail. But
> > > > > > that should actaully not be possible using the legacy modeset API as it
> > > > > > always reconfigures the primary, so we'd only have to worry about that
> > > > > > with full atomic modeset, and for that we anyway need to change the code
> > > > > > to do the check stuff up front.
> > > > > > 
> > > > > > So yeah, with the way things are this should not be able to fail. I'll
> > > > > > respin with the WARN.
> > > > > 
> > > > > I haven't fully dug into the details here, but a few randome comments:
> > > > > - While transitioning we're calling the transitional plane helpers, which
> > > > >   should call the atomic_check stuff for us on the primary plane. If we
> > > > >   need to call atomic_check on other planes too (why?)
> > > > 
> > > > Because we want the derived state to be updated to match the (potentially
> > > > changed) crtc config. We do call the .update_plane() hook from the
> > > > modeset path, but that happens at a time when the pipe is off, so our
> > > > clipping calculations end up saying the plane is invisible. I think fixing
> > > > that the right way pretty much involves the atomic conversion of the
> > > > modeset path.
> > > 
> > > Why do we conclude it's invisible?
> > 
> > Because crtc->active. So for this we'll be wanting crtc_state->active
> > or somesuch which tells us upfront whether the pipe is going to be
> > active or not.
> > 
> > But that's also beside the point a bit since we still want to make call
> > the .atomic_check() for all planes. Right now we'd call it for primary
> > (at the wrong point wrt. crtc->active) and we call it for sprites later
> > when crtc->active is showing the right state, but we don't call it at
> > all for cursors. That's why we still have that ad-hoc extra cursor
> > clipping code in intel_update_cursor(). If we could make the derived
> > plane state correct, we could throw that stuff out as well and trust the
> > regular plane clipping calculations to tell us when the cursor has gone
> > offscreen.
> > 
> > It'll also make the plane state behave in a consitent manner wrt.
> > crtc->active. As it stands you simply can't trust the plane state for
> > primary/cursor planes.
> > 
> > So to fix all of that, I just went ahead and called it for all planes at
> > the point where we currently call it for sprites. I could have just gone
> > ahead and called the higher level .update_plane() func for all of them
> > (as we did for sprites already) but going one level lower allowed me to
> > get the planes to pop in/out atomically.
> > 
> > I think it's the best way to move forward with getting the plane and wm
> > code into some kind of sane state.
> > 
> > > If we can fix the state to not depend
> > > upon the dpms state then things should work ...
> > 
> > That's a more drastic change and needs way more thought.
> 
> Yeah, but that's what we need to aim for eventually. So I want to make
> sure we're not running into the wrong direction and then have to backtrack
> even more.
> 
> > > > >   then I think that
> > > > >   should be done as close as possible to where we do that for the primary
> > > > >   one. Since eventually we need to unbury that call again.
> > > > 
> > > > With my patch _all_ planes get their .atomic_check() called in the same
> > > > place (during plane enable phase of .crtc_enable()).
> > > > 
> > > > > 
> > > > > - I don't like frobbing state objects which are committed (i.e. updating
> > > > >   visible like here), they're supposed to be invariant. With proper atomic
> > > > >   the way to deal with that is to grab all the required plane states and
> > > > >   put them into the drm_atomic_state update structure.
> > > > 
> > > > We really want to frob it so that the derived state will reflect
> > > > reality. Most importantly state->visible should be false whenever the
> > > > pipe is off, otherwise we can't trust state->visible and would also need
> > > > go look at the crtc state whenever we're trying to decide if the plane
> > > > is actually on or not.
> > > 
> > > Imo that's the correct thing to do. Calling plane hooks when the pipe is
> > > off just doesn't seem like a good idea to me. Together with runtime pm at
> > > least, crtc/atomic helpers have some "interesting" heritage.
> > > 
> > > But even there you can fix it by just reordering the commit_planes call to
> > > the bottom, where everything should be on. Iirc that's how Laurent fixed
> > > up rcar runtime pm issues to avoid touching planes when the hw is off.
> > > 
> > > The other reason why ->visible must take into account dpms state is that
> > > we'll screw up the watermark computation otherwise. Which could result
> > > into a failure on a subsequent dpms on, which is a big no-no.
> > 
> > Hmm, right. So for most calculations we'll just want to ignore the
> > DPMS state and calculate things as if the pipe was active (so only
> > looking at crtc_state->enabled I suppose). But when it's actually
> > time to write the values into the hardware we need too consider
> > the actual state of the pipe as well (so taking DPMS into account).
> > 
> > So yeah, if we decouple plane_state->visible from crtc->active and just
> > make it check crtc_state->enabled then plane_state->visible can be used
> > to do all the clipping, and upfront per-pipe wm calculations. But when
> > it comes time to apply the plane state or watermarks to the hardware we
> > need to check the actual pipe state.
> > 
> > So good plan, but needs quite a bit of work to get us there.
> > 
> > > > As for the direct state frobbing, we could make a copy I guess and frob
> > > > that instead, and then commit it. But that seems a lot of work for no gain.
> > > 
> > > That's how atomic is supposed to work really. But we can't make a copy in
> > > the crtc_enable really since that should never fail, and we can't push it
> > > out since we might not hold all the locks. That's all ofcourse for the
> > > atomic end-state, but I think even as an interim detour this approach here
> > > doesn't feel like a good approach to me.
> > 
> > We're going to need the "detour". That is, we do need to call the
> > .atomic_check() for all planes at modeset time. But trying to do it
> > earlier will mean changing all the plane visibility calculations to
> > consider the pipe active whenever crtc_state->enabled==true, and
> > that's going to require careful review of the code to make sure we
> > don't trust the plane visibility/clipping information too much. And
> > the same goes for the watermark code.
> > 
> > So I still think this is the right step to move forward. It'll allow us
> > to use the derived plane state correctly (and since it still depends on
> > crtc->active we won't risk enabling planes when the pipe is off), which
> > allows the plane and wm code to evolve in small steps, rather than some
> > mass conversion. I think there have been more than enough regressions
> > recently.
> 
> Ok, makes sense. It'll mean that there's more steps overall though, which
> might lead to more regressions overall, just spread out a bit. And we
> definitely need to make sure we unwrap this detour again. As long as
> Ander/Matt/you can roughly agree on a plan forward I'll go along with it.
> 
> For atomic in the end I think the important bit really from my pov is that
> we must not depend in any way upon neither intel_crtc->active nor
> crtc_state->active in the watermark computation code. Of course that means
> we need to do some untangling to make sure that we can correctly shut down
> everything if crtc_state->active isn't set.
> 
> Also do we have igt testcases for the regressions this patch series fixes?
> atomic is really complex, I think we should fill out gaps as we go along.
> So if we have them we need to add them to the list - Daniel Stone is
> singed up to work that one down ;-)

I'm not sure we've had real regressions here. But that's pretty much
just pure luck due to the fact that we haven't been using the derived
plane state except for sprites (for which we already did the full
.update_plane() thing).

So the cursor case was saved by the extra clipping step in
intel_update_cursor(), and for primary we've just been looking at the
user provided unclipped state, and we've been using the pipe source size
as the plane size. So as long as the user didn't put the primary
partially offscreen to require clipping, things have been working more
or less normally.

But to move forward with the primary plane windowing etc. we need to
start looking at the derived state, and so it must be first made
trustworthy.

-- 
Ville Syrjälä
Intel OTC