On Tue, Sep 20, 2022 at 03:47:39PM +0100, Daniel Stone wrote: > Hi, > > On Tue, 20 Sept 2022 at 15:31, Ville Syrjälä <ville.syrj...@linux.intel.com> > wrote: > > > On Tue, Sep 20, 2022 at 03:56:18PM +0200, Thomas Zimmermann wrote: > > > Set partial updates on a plane if the framebuffer has not been changed > > > on an atomic commit. If such a plane has damage clips, the driver will > > > use them; otherwise the update is effectively empty. Planes that change > > > their framebuffer still perform a full update. > > > > I have a feeling you're sort of papering over some inefficient > > userspace that's asking updates on planes that don't actually > > need them. I'm not sure adding more code for that is a particularly > > great idea. Wouldn't it be better to just fix the userspace code? > > > > I'm not sure why it would need to be 'fixed', when it's never been a > property of the atomic API that you must minimise updates. Weston does this > (dumps the full state in every time), and I know we're not the only ones.
Well, we've never tried to minimize in the kernel either, except for a few special cases that have one of these ad-hoc "foo_changed" flags. And I think those are more due to "I felt like adding one" rather than any real overall design goal. We even have one that is not used at all, except after this patch series (or at least I think I saw it in there). We could of course scan a lot more in the kernel to minimize stuff, but at least I always end up wondering how many joules are being wasted rescanning things when userspace might have already done the same thing. Also at some point it just might be cheaper to blast the hw with the stuff than to meticulously scan through it all. At least as long as we can't to the 100% short circuit. Side rant: I'm also not a huge fan of those current foo_changed flags because their granularity is a bit weird, and dictated by the core code rather than by the driver specific cost of updating each property. Eg. color_mgmt_changed cover all of crtc level color management, but at least for i915 there are both cheap and expensive things in there. So not sure the current flag really helps with anything. We do currently use it in i915, but maybe we should not and just try to look at each property separately instead. > > 'Fixing' it would require doing a bunch of diffing in userspace, because > maintaining a delta and trying to unwind that delta across multiple > TEST_ONLY runs, isn't much fun. It's certainly a far bigger diff than this > patch. OK. If userspace doesn't want to do it at all, then maybe the kernel should do at least a bit of it. I wonder how far we should take that. In the olden pre-atomic days i915 even automagically turned off the primary plane when fully covered by an opaque sprite plane. I presume modern userspace should at least try to use the available planes efficiently and that kind of optimizations would be a waste of time? > > > Any property change on the plane could need a full plane > > update as well (eg. some color mangement stuff etc.). So you'd > > have to keep adding exceptions to that list whenever new > > properties are added. > > > > Eh, it's just a blob ID comparison. Or some other integer, yes, but someone must remember to add it. This patch series certainly seems to have forgotten most of it, so perhaps a slightly shaky start :) OK, sorry bad pun, moving along... If we do this in some common code near the uapi level, then the driver might still have to repeat a bunch of it due to interactions with other stuff. But I already ranted about color_mgmt_changed earlier in the mail, I guess this is more of the same really. What would make this sort of discussion really interesting would be actual power and/or performance figures given some typical fixed workloads. But that sounds like a lot of work... > > > > And I think the semantics of the ioctl(s) has so far been that > > basically any page flip (whether or not it actually changes > > the fb) still ends up doing whatever flushing is needed to > > guarantee the fb contents are up to date on the screen (if > > someone sneaked in some front buffer rendering in between). > > Ie. you could even use basically a nop page flip in place > > of dirtyfb. > > > > Another thing the ioctls have always done is actually perform > > the commit whether anything changed or nor. That is, they > > still do all the same the same vblanky stuff and the commit > > takes the same amount of time. Not sure if your idea is > > to potentially short circuit the entire thing and make it > > take no time at all? > > > > I would expect it to always perform a commit, though. > > Cheers, > Daniel -- Ville Syrjälä Intel
