On Monday, November 13th, 2023 at 11:15, Pekka Paalanen <ppaala...@gmail.com>
wrote:
>
>
> On Mon, 13 Nov 2023 09:44:15 +0000
> Simon Ser cont...@emersion.fr wrote:
>
> > On Monday, November 13th, 2023 at 10:38, Pekka Paalanen ppaala...@gmail.com
> > wrote:
> >
> > > On Mon, 13 Nov 2023 09:18:39 +0000
> > > Simon Ser cont...@emersion.fr wrote:
> > >
> > > > On Monday, October 23rd, 2023 at 10:25, Simon Ser cont...@emersion.fr
> > > > wrote:
> > > >
> > > > > > > > > > > > > > +An atomic commit with the flag
> > > > > > > > > > > > > > DRM_MODE_PAGE_FLIP_ASYNC is allowed to
> > > > > > > > > > > > > > +effectively change only the FB_ID property on any
> > > > > > > > > > > > > > planes. No-operation changes
> > > > > > > > > > > > > > +are ignored as always. [...]
> > > > > > > > > > > > > > During the hackfest in Brno, it was mentioned that
> > > > > > > > > > > > > > a commit which re-sets the same FB_ID could
> > > > > > > > > > > > > > actually have an effect with VRR: It could trigger
> > > > > > > > > > > > > > scanout of the next frame before vertical blank has
> > > > > > > > > > > > > > reached its maximum duration. Some kind of
> > > > > > > > > > > > > > mechanism is required for this in order to allow
> > > > > > > > > > > > > > user space to perform low frame rate compensation.
> > > > > > > > > > > >
> > > > > > > > > > > > Xaver tested this hypothesis in a flipping the same fb
> > > > > > > > > > > > in a VRR monitor
> > > > > > > > > > > > and it worked as expected, so this shouldn't be a
> > > > > > > > > > > > concern.
> > > > > > > > > > > > Right, so it must have some effect. It cannot be simply
> > > > > > > > > > > > ignored like in
> > > > > > > > > > > > the proposed doc wording. Do we special-case re-setting
> > > > > > > > > > > > the same FB_ID
> > > > > > > > > > > > as "not a no-op" or "not ignored" or some other way?
> > > > > > > > > > > > There's an effect in the refresh rate, the image won't
> > > > > > > > > > > > change but it
> > > > > > > > > > > > will report that a flip had happened asynchronously so
> > > > > > > > > > > > the reported
> > > > > > > > > > > > framerate will be increased. Maybe an additional
> > > > > > > > > > > > wording could be like:
> > > > > > > > > >
> > > > > > > > > > Flipping to the same FB_ID will result in a immediate flip
> > > > > > > > > > as if it was
> > > > > > > > > > changing to a different one, with no effect on the image
> > > > > > > > > > but effecting
> > > > > > > > > > the reported frame rate.
> > > > > > > > >
> > > > > > > > > Re-setting FB_ID to its current value is a special case
> > > > > > > > > regardless of
> > > > > > > > > PAGE_FLIP_ASYNC, is it not?
> > > > > > > >
> > > > > > > > No. The rule has so far been that all side effects are observed
> > > > > > > > even if you flip to the same fb. And that is one of my
> > > > > > > > annoyances
> > > > > > > > with this proposal. The rules will now be different for async
> > > > > > > > flips
> > > > > > > > vs. everything else.
> > > > > > >
> > > > > > > Well with the patches the async page-flip case is exactly the
> > > > > > > same as
> > > > > > > the non-async page-flip case. In both cases, if a FB_ID is
> > > > > > > included in
> > > > > > > an atomic commit then the side effects are triggered even if the
> > > > > > > property
> > > > > > > value didn't change. The rules are the same for everything.
> > > > > >
> > > > > > I see it only checking if FB_ID changes or not. If it doesn't
> > > > > > change then the implication is that the side effects will in
> > > > > > fact be skipped as not all planes may even support async flips.
> > > > >
> > > > > Hm right. So the problem is that setting any prop = same value as
> > > > > previous one will result in a new page-flip for asynchronous
> > > > > page-flips,
> > > > > but will not result in any side-effect for asynchronous page-flips.
> > > > >
> > > > > Does it actually matter though? For async page-flips, I don't think
> > > > > this
> > > > > would result in any actual difference in behavior?
> > >
> > > Hi Simon,
> > >
> > > a fly-by question...
> > >
> > > > To sum this up, here is a matrix of behavior as seen by user-space:
> > > >
> > > > - Sync atomic page-flip
> > > > - Set FB_ID to different value: programs hw for page-flip, sends uevent
> > > > - Set FB_ID to same value: same (important for VRR)
> > > > - Set another plane prop to same value: same
> > > > - Set another plane prop to different value: maybe rejected if modeset
> > > > required
> > > > - Async atomic page-flip
> > > > - Set FB_ID to different value: updates hw with new FB address, sends
> > > > immediate uevent
> > > > - Set FB_ID to same value: same (no-op for the hw)
> > >
> > > It should not be a no-op for the hw, because the hw might be in the
> > > middle of a VRR front-porch waiting period, and the commit needs to end
> > > the waiting immediately rather than time out?
> >
> > I'm not sure
>
> Would people not want to use async commits to trigger new VRR scanout
> cycles without content updates?
>
> I seem to recall previous comments that switching between sync and
> async commit modes may take a moment (intel's one last sync flip), so
> using sync once in a while then using async otherwise is probably not a
> good idea.
Sorry, my line got cut off. I meant: "I'm not sure what you mean".
> > > > - Set another plane prop to same value: ignored, sends immediate uevent
> > > > (special codepath)
> > >
> > > If the sync case says "same", then shouldn't this say "same" as well to
> > > be consistent?
> >
> > Okay, I think I chose my words badly. By "same" I meant "same as
> > previous item in the list".
> >
> > Here I tried to be more explicit and explain why it's the same behavior.
> > We have a special path in the kernel code that ignores the change, but
> > the effective result is that it doesn't differ from the sync case.
> >
> > Here's a fixed matrix where I don't use confusing words:
> >
> > - Sync atomic page-flip
> > - Set FB_ID to different value: programs hw for page-flip, sends uevent
> > - Set FB_ID to same value: programs hw for page-flip, sends uevent
> > (important
> > for VRR)
> > - Set another plane prop to same value: programs hw for page-flip, sends
> > uevent
>
> Programming hw for page-flip probably triggers a new VRR scanout cycle,
> even if the FB address didn't change.
>
> > - Set another plane prop to different value: maybe rejected if modeset
> > required
> > - Async atomic page-flip
> > - Set FB_ID to different value: updates hw with new FB address, sends
> > immediate uevent
> > - Set FB_ID to same value: updates hw with new FB address (no-op for the
> > hw),
> > sends immediate uevent
> > - Set another plane prop to same value: ignored, sends immediate uevent
> > (special codepath)
>
> Just like Michel points out: if this case has a special case ignoring
> the set, then this case will not trigger a new VRR scanout cycle like
> the corresponding sync case does.
Ignore the prop change, but still include the CRTC in the commit, so
everything is fine.
