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