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