[PATCH 4/5] drm/damage-helper: Do partial updates if framebuffer has not been changed

[Ville Syrjälä]

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