Re: [PATCH 0/7] prepare for atomic.. the great propertyification

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, Jul 24, 2014 at 09:36:20AM -0400, Rob Clark wrote:
> On Thu, Jul 24, 2014 at 8:25 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Wed, Jul 23, 2014 at 03:38:13PM -0400, Rob Clark wrote:
> >> This is mostly just a rebase+resend.  Was going to send it a bit earlier
> >> but had a few things to fix up as a result of the rebase.
> >>
> >> At this point, I think next steps are roughly:
> >> 1) introduce plane->mutex
> >> 2) decide what we want to do about events
> >> 3) add actual ioctl
> >>
> >> I think we could shoot for merging this series next, and then adding
> >> plane->mutex in 3.18?
> >>
> >> Before we add the ioctl, I think we want to sort out events for updates
> >> to non-primary layers, and what the interface to drivers should look like.
> >> Ie. just add event to ->update_plane() or should we completely ditch
> >> ->page_flip() and ->update_plane()?
> >>
> >> Technically, I think we could get away without a new API and just let
> >> drivers grab all the events in their ->atomic_commit(), but I suspect
> >> core could provide something more useful to drivers.  I guess it would
> >> be useful to have a few more drivers converted over to see what makes
> >> sense.
> >
> > Ok so we've had a lot of discussions about this on irc and overall I'm not
> > terribly happy with what this looks like. I have a few concerns:
> >
> > - The entire atomic conversion for drivers is monolithic, at least at the
> >   interface level. So if you want to do this step-by-step you're forced to
> >   add hacks and e.g. bypass pageflips until that's implemented. E.g. on
> >   i915 I see no chance that we'll get atomic ready for all cases (so
> >   nonblocking flips and multi-crtc and everything on all platforms) in one
> >   go.
> 
> So, there interface is *intended* to be monolithic, in a way..  many
> years ago, I started by adding side by side atomic vs legacy paths,
> and it was pretty ugly in core.  I'm much happier with the current
> iteration.
> 
> A slightly later iteration preserved atomic helpers (so drivers could
> do completely their own thing).  I ended up dropping that because most
> of what atomic does is manage the interface to whatever is doing
> modeset/pageflip/whatever.  I completely expect some changes around
> the commit stuff..  that part is intended to either be
> wrapped/extended by the driver or replaced.  Possibly it could be made
> more clear what drivers are expected to use vs extend or replace.  I
> expect this to evolve a bit as more drivers are converted.
> 
> Note that the current atomic "layer" results in 1:1 conversion from
> old userspace API to legacy vfuncs.  This way what is on top of atomic
> API and what is beneath (ie. the driver) has same code paths either
> way (old or new userspace, converted or not driver).  The edge cases
> don't come in until atomic ioctl is exposed, and for that I expect a
> driver flag to enable the new ioctl.  You could even set the flag
> based on hw generation if you want.
> 
> > - Related to that is that we force legacy ioctls through atomic. E.g. on
> >   older i915 platforms I very much expect that we won't ever convert the
> >   pageflip code to atomic for them and just not support atomic flips of
> >   cursor + primary plane. At least not at the beginning. I know that's
> >   different from my stance a year ago, but I've become a bit more
> >   realistic.
> >
> > - The entire conversion is monolithic and we can't do it on a
> >   driver-by-driver basis. Everyone has to go through the new atomic
> >   interfaces on a flag day. drm is much bigger now and forcing everyone to
> >   convert at the same time is really risky. Again I know I've changed my
> >   mind again, but drm is a lot bigger and there's a lot more drivers that
> >   implement pageflip and cursors, i.e. stuff that's special.
> 
> the only flag day part is plugging in atomic functions and couple
> vfunc API changes around set_prop().. the current design allows for
> conversion on driver by driver basis.
> 
> > - The separation between interface marshalling code in the drm core and
> >   helper functions for drivers to implement stuff is fuzzy at best. Thus
> >   far we've had an excellent seperation in this are for kms, I don't think
> >   it's vise to throw that out.
> 
> Interface marshalling should not be helper.  Everyone needs the same
> thing, since what is on top is the same.

Erhm I've certainly not meant the interface marshalling to be a helper.
This started with "The separation ..." after all, so I want inteface
marshilling as fixed code in the drm core, and everything else outside in
a separate drm_atomic_helper.c module.

> > So here's my proposal for how to get this in without breaking the world
> >
> > 1. We add a complete new set of ->atomic_foo driver entry points to all
> > relevant objects. So instead of changing all the set_prop functions in a
> > flag-day like operation we just add a new interface. I haven't double
> > checked, but I guess that means per-obj ->atomic_set_prop functions plus a
> > global ->atomic_check and ->atomic_commit.
> 
> that sounds much worse

Why that? Afaics your patch only changes the interfaces but leaves the
semantics the same (i915 does set_config_internal all over the place in
set_prop). So essentially you have both interface, but merged into one.
And especially for the set_prop the semantics our different.

> > 2. We add a new drm_atomic_helper.c library which provides functions to
> > implement all the legacy interfaces (page_flip, update_plane, set_prop)
> > using atomic interfaces. We should be able to 1:1 reuse Rob's code for
> > this, but instead of changing the actual current interface code we put
> > that into helpers.
> 
> So, I've started ripping out page_flip.. now w/ primary plane helpers
> we can do everything in terms of update_plane().  I have an idea to
> handle events.  It isn't so bad, but forces me to do some re-arranging
> in drm/msm that I was planning to postpone otherwise.
> 
> (And I just got a fedex package w/ new toys, er, hardware.. but I'll
> try to finish this today)

Well in that case I think we should demote ->update_plane and friends to
helper status, since for a proper atomic implemenation for i915 we can't
use them.

> > We don't need anything for cursor ioctls since cursor plane helpers
> > already map the legacy cursor ioctls.
> >
> > 3. Rob's current code reuses the existing ->update_plane, ->pageflip and
> > other entry points to implement the atomic commit helper functions. Imo
> > that's a bad layering violation, and if we plug helpers in there we can't
> > use them.
> 
> Well, it is only a layering violation because you have a slightly
> different idea of where the layers should be.  ;-)
> 
> Atomic (or really the state stuff.. maybe I should have called it
> "transaction" or something like that to avoid the atomic
> connotation..) should be on top of, not below, helpers.

I'm talking only about the legacy interfaces here, and my thinking is that
we should not add a midlayer here (the atomic helper stuff) but directly
pass it to drivers. They can the choose how to implement this.

With the current scheme I essentially have to add a bunch of hacks for
i915 to fish out the old pageflip semantics to keep gen2 going. That's
fairly ugly, and the only reason is that you force a midlay between the
existing legacy ioctls and i915.

Of course for the actual atomic ioctl I don't want this inversion. But
that's not what I'm talking about here.

> Probably the commit part should be more clearly separated out and
> marked as "helper", because that is the part that is about taking the
> state that userspace (or fbdev) has asked for and applying it to the
> hw.  This is really the part where drivers for some hw might want to
> do something different.  Although I admit lumping it in w/
> drm_atomic.c makes this not very clear.
> 
> And then beneath atomic we introduce new interfaces (if needed..
> although update_plane isn't so bad once we toss out page_flip).  We
> could introduce new ->commit_state() vfuncs, and alternate set of
> commit helper which uses those.. at least for planes I suspect
> ->commit_state() ends up looking a lot like ->update_plane.

All ok with me, but really not my concern. This is all for drivers which
support atomic, I'll have a driver which will (at least partially and
likely forever) not support atomic everywhere.

> > Instead I think we should add a new per-plane ->atomic_commit functions
> > clearly marked as optional. Maybe even as part of an opaque
> > plane_helper_funcs pointer like we have with crtc/encoder/connector and
> > crtc helpers. msm would then implement it's atomic commit function in
> > there (since it's the only driver currently supporting atomic for real).
> >
> > 3b. We could adjust crtc helpers to use the plane commit hook instead of
> > the set_base function when avialable.
> 
> would be a nice cleanup either way.. but I think it is independent..

I think I've raised this a few times in the past, but imo having somewhat
clear helper semantics would help. Atm they try to do a bit too much
(legacy support, generic helpers, interface with crtc helpers without
changing too much) and don't look good in any of them.

> > 4. We do a pimped atomic helper based upon crtc helpers and the above
> > plane commit function added in 3. It would essentially implement what
> > Rob's current helper does, i.e.
> >
> > Step 1: Shut down all crtcs which change using the crtc helpers. This step
> > is obviously optional and ommitted when there's nothing to do.
> >
> > Step 2: Loop over all planes and call ->plane_commit or whatever we will
> > call the interface added in 3. above.
> >
> > Step 3: Enable all crtcs that need enabling.
> >
> > 5. We start converting drivers. Every driver can convert at it's own pace,
> > opting in for atomic behaviour step-by-step.
> >
> > 6. We optionally use the atomic interface in the fb helper. It's imo
> > important to keep the code here parallel so that drivers can convert at
> > their own leisure.
> 
> this is one thing I really wanted to avoid.  It was already getting
> ugly the first time I tried it, and I hadn't even converted
> everything.

Well I think we can't avoid ugliness in that area. There simply will be
parallel support for atomic in different drivers.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch