On Fri, Jul 25, 2014 at 4:15 AM, Daniel Vetter <daniel at ffwll.ch> wrote: > On Thu, Jul 24, 2014 at 10:56:07AM -0400, Rob Clark wrote: > [snip] > >> ok, it's entirely possibly that I'm missunderstanding a bit your proposal.. > > Yeah I get that feeling a bit too. I'll cut out the technical details for > now and will try to concentrate on one example. Maybe that clarifies. > > > [snip] > >> >> > 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. > > So currently our set_prop functions work like that: > 1. We parse the property, sanity check it and store it in the relevant > object structure. > 2. We check whether (after all the checking and parsing) there has been an > actual change in configuration. E.g. for the audio stuff if the use sets > back to "auto" we check whether the old config matches the new autoconfig > state or not. So it's not just "has the prop value changed", we actually > diff the resulting hw config. > 3. If there is a change, we update the hw state with a call to mode_set > iff the pipe is on. > > There's a few reasons why this works and why it looks like that. > - Our internal mode set code currently doesn't notice changes in property > values. We plan to fix this eventually by shuffling the compute_config > code around, but that's a lot of work. > - Our internal mode set function _always_ forces a full modeset on the > crtc you pass it (besides doing modeset on other crtcs where tracked > state has changed). We need to have this hack to make the above set_prop > sequence work. > > Now with atomic we want completely different semantics: > - Set_prop only updates state. We need to drop the state computation and > diffing and the forced mode_set.
So probably the first thing, is that we need to bring connectors into the atomic game.. probably that is something that i915 should take the lead on, since you actually have a bunch of connector properties which, from the sounds of it, should play by atomic rules. I don't have anything as complicated in msm, and wasn't entirely sure what would be needed for connectors, so I punted on that part. That said, crtc_state->set_config, etc, flags should be what you need to know if you need a full modeset or not. You just need your connector's set_prop to grab the appropriate crtc_state and set the appropriate flag(s). So I'm not sure that I see any fundamental problem here[*]. [*] well, combining connector changes w/ connector property changes perhaps.. you may actually need your ->atomic_check() step to propagate the "I need a modeset" flag from connector to crtc.. but otoh a connector change is a full modeset, so maybe not. > - The eventual commit will force a mode_set. Note that calling > ->set_config will not be enough since that doesn't have the "force full > mode-set" hack which the internal version has. And we can't do this > since userspace uses set_crtc/->set_config to update frontbuffers which > absolutely must not result in a full modeset. > > So looking just at the ->set_prop function we have 2 completely different > semantics. Now I agree that with your patch i915 keeps on working. But the > problem I have is converting i915 over to the new world. Since you've > removed the old entry point I am left with no other choice than to convert > everything at once. And there's a lot more than just the hdmi audio > property - page_flip has slightly different semantics with atomic, > update_plane dito, set-crtc the same. > > Which means I either have to convert i915 in one go (impossible given the > usual churn I face) or I just end up implementing the infrastructure I've > asked for (which means I get to reinstante all the old legacy ioctl > paths). Since with the doubled-up entry points I can e.g. just convert > hdmi set_prop to atomic, which means I have a minimal use-case to validate > the core infrastructure in i915 (i.e. the state diffing we need to improve > in the mode_set code) and can ignore all the nonsense in the tv connectors > and sdvo encoders and plane props and crtcs props and hacked-up other crap > we have all around. It's still going to be a major pain, but I expect the > transition to be much more smoothly. > > My experience with the universal plane stuff really is that even slight > semantic differences in the interfaces bite you hard and there's no way to > work around that. The only sane way really is to have parallel entry > points with helpers so that transitioned drivers can remap legacy > interfaces to the more powerful new ones. > > I hope this explains a bit better where I see the big risk with your > approach and what exactly I'm proposing. Well, I'm still not convinced that my approach won't work.. at the end of the day it is turning old interface -> new -> old. And as long as the new->old step is only having to deal with the subset of the new more powerful API that was the old interface, the round-trip should be doable/straightforward. But I agree that your approach is more conservative, in that it is more obviously not touching legacy stuff. Although I'm not completely sure yet how atomic helpers and crtc helpers fit together with your approach. I am really not ready to ditch crtc helpers for modeset yet in msm. I suppose that is mostly a matter of where the new vfuncs live (esp. if we have both atomic and non-atomic versions of ->set_prop()).. I've got some other things to finish first, but I'll probably cook up a patch to convert drm_display_mode to a refcnt'd object, which will let me use display mode ptrs in drm_crtc_state, and gets rid of the biggest awkwardness that I was seeing with your approach. After that, I suppose I can give your proposal a try. The change shuffles things around quite a bit, so I doubt I'll have time to finish it before the merge window, although as you point out it wouldn't really effect any existing driver so probably safe for later -rc merge. Not sure that I'd have the driver bits for msm ready in time, but I guess even if we were not ready to merge any driver bits, it might be more convenient for everyone to have the core bits in place the kernel before for merging driver bits in 3.18... BR, -R > Cheers, Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
