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

[Daniel Vetter <daniel@ffwll.ch>]

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

Cheers, 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