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 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? If we can fix the state to not depend
upon the dpms state then things should work ...

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

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