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

[Ville Syrjälä]

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.

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

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.

> 
> - My idea for resolving our current nesting issues with
>   enable/disable_planes functions was two parts: a) open-code a hardcoded
>   disable-all-planes function by just calling plane disable code
>   unconditionally. Iirc there's been patches once somewhere to do that
>   split in i915 (maybe I'm dreaming), but this use-case is also why I
>   added the atomic_plane_disable hook. b) on restore we just do a normal
>   plane commit with the unchanged plane states, they should all still
>   work.
> 
> btw if we wire up the atomic_disable_plane hook then we can rip out
> intel_plane_atomic_update. The "don't disable twice" check is already done
> by the helpers in that case.
> 
> I'll grab some coffee and see what's all wrong with my ideas here now, but
> please bring in critique too ;-)
> 
> Cheers, Daniel
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

-- 
Ville Syrjälä
Intel OTC