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

[Ville Syrjälä]

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.

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

-- 
Ville Syrjälä
Intel OTC