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

[Daniel Vetter <daniel@ffwll.ch>]

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@linux.intel.com wrote:
> > > > > > > From: Ville Syrjälä <ville.syrjala@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 ;-)

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx