Re: [PATCH 1/2] drm: Fix dirtyfb stalls

[Pekka Paalanen <ppaalanen@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 6161 bytes --]

On Wed, 12 May 2021 07:57:26 -0700
Rob Clark <robdclark@gmail.com> wrote:

> On Wed, May 12, 2021 at 1:23 AM Pekka Paalanen <ppaalanen@gmail.com> wrote:
> >
> > On Tue, 11 May 2021 18:44:17 +0200
> > Daniel Vetter <daniel@ffwll.ch> wrote:
> >  
> > > On Mon, May 10, 2021 at 12:06:05PM -0700, Rob Clark wrote:  
> > > > On Mon, May 10, 2021 at 10:44 AM Daniel Vetter <daniel@ffwll.ch> wrote:  
> > > > >
> > > > > On Mon, May 10, 2021 at 6:51 PM Rob Clark <robdclark@gmail.com> wrote:  
> > > > > >
> > > > > > On Mon, May 10, 2021 at 9:14 AM Daniel Vetter <daniel@ffwll.ch> wrote:  
> > > > > > >
> > > > > > > On Sat, May 08, 2021 at 12:56:38PM -0700, Rob Clark wrote:  
> > > > > > > > From: Rob Clark <robdclark@chromium.org>
> > > > > > > >
> > > > > > > > drm_atomic_helper_dirtyfb() will end up stalling for vblank on "video
> > > > > > > > mode" type displays, which is pointless and unnecessary.  Add an
> > > > > > > > optional helper vfunc to determine if a plane is attached to a CRTC
> > > > > > > > that actually needs dirtyfb, and skip over them.
> > > > > > > >
> > > > > > > > Signed-off-by: Rob Clark <robdclark@chromium.org>  

...

> > > > > > But we could re-work drm_framebuffer_funcs::dirty to operate on a
> > > > > > per-crtc basis and hoist the loop and check if dirtyfb is needed out
> > > > > > of drm_atomic_helper_dirtyfb()  
> > > > >
> > > > > That's still using information that userspace doesn't have, which is a
> > > > > bit irky. We might as well go with your thing here then.  
> > > >
> > > > arguably, this is something we should expose to userspace.. for DSI
> > > > command-mode panels, you probably want to make a different decision
> > > > with regard to how many buffers in your flip-chain..
> > > >
> > > > Possibly we should add/remove the fb_damage_clips property depending
> > > > on the display type (ie. video/pull vs cmd/push mode)?  
> > >
> > > I'm not sure whether atomic actually needs this exposed:
> > > - clients will do full flips for every frame anyway, I've not heard of
> > >   anyone seriously doing frontbuffer rendering.  
> >
> > That may or may not be changing, depending on whether the DRM drivers
> > will actually support tearing flips. There has been a huge amount of
> > debate for needing tearing for Wayland [1], and while I haven't really
> > joined that discussion, using front-buffer rendering (blits) to work
> > around the driver inability to flip-tear might be something some people
> > will want.  
> 
> jfwiw, there is a lot of hw that just can't do tearing pageflips.. I
> think this probably includes most arm hw.  What is done instead is to
> skip the pageflip and render directly to the front-buffer.
> 
> EGL_KHR_mutable_render_buffer is a thing you might be interested in..
> it is wired up for android on i965 and there is a WIP MR[1] for
> mesa/st (gallium):
> 
> Possibly it could be useful to add support for platform_wayland?
> 
> [1] https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/10685

Thanks Rob, that's interesting.

I would like to say that EGL Wayland platform cannot and has no reason
to support frontbuffer rendering in Wayland clients, because the
compositor may be reading the current client frontbuffer at any time,
including *not reading it again* until another update is posted via
Wayland. So if a Wayland client is doing frontbuffer rendering, then I
would expect it to be very likely that the window might almost never
show a "good" picture, meaning that it is literally just the
half-rendered frame after the current one with continuously updating
clients.

That is because a Wayland client doing frontbuffer rendering is
completely unrelated to the Wayland compositor putting the client
buffer on scanout.

Frontbuffer rendering used by a Wayland compositor would be used for
fallback tearing updates, where the rendering is roughly just a blit
from a client buffer. By definition, it means blit instead of scanout
from client buffers, so the performance/power hit is going to be...
let's say observable.

If a Wayland client did frontbuffer rendering, and then it used a
shadow buffer of its own to minimise the level of garbage on screen by
doing only blits into the frontbuffer, that would again be a blit. And
then the compositor might be doing another blit because it doesn't know
the client is doing frontbuffer rendering which would theoretically
allow the compositor to scan out the client buffer.

There emerges the need for a Wayland extension for clients to be
telling the compositor explicitly that they are going to be doing
frontbuffer rendering now, it is ok to put the client buffer on scanout
even if you want to do tearing updates on hardware that cannot
tear-flip.

However, knowing that a client buffer is good for scanout is not
sufficient for scanout in a compositor, so it might still not be
scanned out. If the compositor is instead render-compositing, you have
the first problem of "likely never a good picture".

I'm sure there can be specialised use cases (e.g. a game console
Wayland compositor) where scanout can be guaranteed, but generally
for desktops it's not so.

I believe there will be people wanting EGL Wayland platform frontbuffer
rendering for very special cases, and I also believe it will just break
horribly everywhere else. Would it be worth it to implement? No idea.

Maybe there would need to be a Wayland extension so that compositors
can control the availability of frontbuffer rendering in EGL Wayland
platform?

There is the dmabuf-hints Wayland addition that is aimed at dynamically
providing information to help optimise for scanout and
render-compositing. If a compositor could control frontbuffer rendering
in a client, it could tell the client to use frontbuffer rendering when
it does hit scanout, and tell the client to stop frontbuffer rendering
when scanout is no longer possible. The problem with the latter is a
glitch, but since frontbuffer rendering is by definition glitchy (when
done in clients), maybe that is acceptable to some?


Thanks,
pq

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]