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 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?) 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.
> 
> - 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.
> 
> - 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 ;-)

One immediate problem is that we key off from intel_state->visible to
decide whether to enable or not, and the core helpers key off from
state->fb. So I think we'd need to roll our own, but with the same idea
of splitting out an explicit plane_disable hook.

Or maybe we should add a drm_plane_state->visible derived state which
helpers fill out to match drm_plane_state->fb by default. That might be
even neater, and matches somewhat how we allow drivers to overwrite
crtc_state->needs_modeset to control which hooks the helpers will call.

Clearly, more coffee is neede here ;-)
-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