Re: [PATCH 14/17] drm/msm: add atomic support

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, May 27, 2014 at 07:47:42PM -0400, Rob Clark wrote:
> On Tue, May 27, 2014 at 6:09 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Tue, May 27, 2014 at 04:06:28PM -0400, Rob Clark wrote:
> >> On Tue, May 27, 2014 at 3:26 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> >
> > [snip]
> >
> >> Well, there was the NONBLOCK atomic flag.. I'm not entirely sure if we
> >> should hang so much off of that one flag.
> >
> > Yeah, a separate VBLANK_SYNCED might be useful. Apparently people also
> > want non-blocking modesets.
> >
> > [snip]
> >
> >> > I think one excellent use-case we get for free (almost) without the ioctl
> >> > would be fbcon. It very much wants to do an atomic update, so converting
> >> > that over to the atomic interface would be good imo.
> >>
> >> Yes, iirc the remaining non-atomic paths are fbcon related.  In
> >> principle it should be a simple matter to increment the refcnt on
> >> fbcon state object and re-apply it.  Although at the moment we keep
> >> track of *how* to apply the state (ie. page_flip vs set_config, etc)
> >> as the state object is built up.. which isn't very conducive to
> >> re-committing an existing state object.  Which is part of the reason I
> >> wanted to deprecate the various existing
> >> ->page_flip/->update_plane/->set_config/etc and introduce per object
> >> ->commit()'s.  (Which could either be called by helpers, or called
> >> internally by driver or completely ignored by driver)
> >
> > Yeah, I think the approach in here with a few helpers to bend atomic
> > ->commit to the old hooks (somewhat-ish) is good. And with the crtc
> > helpers we should be able to move most drivers away from the old hooks
> > quickly. The exception will be pageflips/cursors, but that requires a lot
> > of driver-specific work, and probably first a full conversion to universal
> > planes (which atm don't support everything even for the primary plane due
> > to the arbitrary restriction to rgbx8888).
> 
> btw, I guess/hope most SoC drivers (ie. the ones that want to do crazy
> things with overlays) would be moving rapidly away from using primary
> plane helpers.  I think native primary planes fits better most of the
> non-trivial mobile display controller blocks..

Fully agreed. See my comment about the VBLANK_SYNCED flag, I think without
a driver implementation we should simply aggressively reject such updates.
Same with disabling the primary plane, at least until we've reworked the
interfaces a bit.

> >> I've been a bit reluctant so far to do too much additional refactoring
> >> on top of atomic, since I'm about at the limit of what I have time to
> >> repeatedly rebase each kernel version.  This is why I'm a bit anxious
> >> to start merging some of atomic, even if it doesn't do absolutely
> >> everything yet.
> >
> > I understand that, which is why I suggested a bunch of things to split out
> > already so we can get them in.
> >
> > On top of that I think with the split-up mode_config.mutex like I've just
> > proposed in an RFC we have a clear path for the locking issues, too. So
> > could go ahead an merge the w/w conversion, too.
> 
> I suppose now that I split modeset acquire ctx out into it's own thing
> (originally it was inline w/ drm_atomic_state), I could reshuffle the
> patches and move ww_mutex stuff below atomic.. will take a bit of
> patch surgery since some parts of that patch would have to move to
> later patches, but I can sort it out

below = earlier in the series or later? I'll assume earlier since that
what git log shows ;-)

As mentioned on irc I think we could also throw per-plane locks into the
mix to really validate the locking side of this. One thing we didn't
discuss that much in the last few emails is accessing the obj->state data
and other stuff which is autodetect (e.g. whether the sink is audio
capable or not). I still think we need one additional plain mutex to
protect that. It would sit between the mode_config.mutex and all the ww
mutexes in the locking hierarchy.

But we can look at that once the mode_config.mutex mess is untangled and
clear.

> > That leaves the set_prop refactoring which is still under discussion.
> > Those three pieces hopefully help a lot with reducing rebasing pain.
> >
> > On top of that I think we should look at cutting away functional pieces of
> > the conversion, e.g. ignore planes at first and only look at atomic
> > modeset. Or ignore atomic modesets and only look at plane updates,
> > rejecting anything that changes connectors or crtcs. By cutting out slices
> > we should be able to get patches into shape step-by-step.
> 
> hmm, well, not sure how much value it is splitting up like that.  I
> suppose the only-plane approach would sidestep/punt some issues.  But
> once locking and set_prop are in place / agreed, it isn't that much
> more from a churn perspective.

Hm, could be that it's not worth it, was just an idea.

> tbh, I think the worse problems are when we actually expose atomic
> ioctl to userspace.  Right now we are rather restricted in the updates
> triggered via atomic in that there are fixed well defined entry points
> to atomic.  Ie. only pageflip() sets _NONBLOCK flag, and it does not
> set mode property, so you won't ever see a _NONBLOCK modeset.

I guess we could lift limits step-by-step, e.g. reject any NONBLOCK
updates if they change crtc/connector properties. And reject any NONBLOCK
which isn't also vsynced.

Wrt earlier testing fbcon should give us basic in-kernel validation, since
it really wants to kill all the planes and everything and put a new config
into place in one go. Another good one would be suspend/resume. It needs
vt-switchless resume enabled (which is a per-driver knob and only i915 has
it for now, but it's easy to do). Then we could suspend/resume a full X
session with overlays, cursors and all and see whether the atomic engine
can cope and restore it all.

Once that's proven the ioctl shouldn't be that bad really any more. For
i915 I'll demand lots of tests for corner-cases, but the easiest to
implement workload might be the vt restore in the ddx. If we can do that
with atomic you can fire up a bunch of ridiculous (and conflicting) X
configs with cursors and overlays and the vt-switch between them like mad.
That should give us good testing I hope.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch