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

[Rob Clark]

On Thu, Jul 24, 2014 at 10:00 AM, Daniel Vetter <daniel at ffwll.ch> wrote:
> 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 at 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.

ok, it's entirely possibly that I'm missunderstanding a bit your proposal..

>> > 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.

well, it was the doubling up of code paths in core for handling legacy
and atomic side-by-side that I was trying to avoid

I do remember seeing i915 set_config_internal (looks like it has been
refactored into intel_set_mode())..  iirc, it was all on
connector->set_prop(), so it would essentially be it's own
atomic/transaction.

>> > 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.

->update_plane() probably should have been helper all along (but I'd
kinda like to get something merged before we do too much other
cleanup.. I spend enough of my time rebasing patches..)

>> > 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.

so you actually have exactly what you want already.. just rename
'struct drm_atomic_funcs' to 'struct drm_atomic_helper_funcs' and your
own i915_atomic_commit() fxn.

> 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.

not quite sure I follow.. isn't that what primary-plane helpers are
supposed to do for you?

update: oh, I guess they don't.. that sort of screws my plan to move
over to just ->update_plane().

> 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.

admittedly it wasn't a case I thought of much (one driver, supporting
atomic for only certain generations).. although I don't particularly
see the problem here.  You might just need to plug in different helper
fxn ptrs depending on generation.

>> > 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.

I think it is avoidable.  I mean, I've used my patchset on an
unconverted i915 (as well as both converted and unconverted msm).. the
commit helper part of things successfully turns legacy APIs back into
legacy vfunc calls.

BR,
-R


> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch