[PATCH] drm/rockchip: remove atomic helper dirtyfb

[PATCH] drm/rockchip: remove atomic helper dirtyfb

[Toni Spets]

[-- Attachment #1.1: Type: text/plain, Size: 578 bytes --]

The blocking implementation of the dirtyfb ioctl is extremely slow when
used for damage tracking on RK3399. If this implementation is in place Xorg
will default to using it and will slow down considerably when doing a lot
of small draws. This is most apparent with the fvwm window manager on
startup where it will almost lock up for many seconds seconds on RK3399.

Removing this implementation did not cause any visible issues on RK3399 but
it did fix the performance issues on Xorg as it will disable damage
tracking when the ioctl returns it's not supported.

-- 
Toni Spets

[-- Attachment #1.2: Type: text/html, Size: 752 bytes --]

[-- Attachment #2: 0001-drm-rockchip-remove-atomic-helper-dirtyfb.patch --]
[-- Type: text/x-patch, Size: 798 bytes --]

From 79984ee67c801f552e9eaf4d0cfb62101d1f0f2e Mon Sep 17 00:00:00 2001
From: Toni Spets <toni.spets@iki.fi>
Date: Wed, 3 Feb 2021 21:14:50 +0200
Subject: [PATCH] drm/rockchip: remove atomic helper dirtyfb

---
 drivers/gpu/drm/rockchip/rockchip_drm_fb.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
index 3aa37e177667..2554fd1c8aeb 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
@@ -21,7 +21,6 @@
 static const struct drm_framebuffer_funcs rockchip_drm_fb_funcs = {
 	.destroy       = drm_gem_fb_destroy,
 	.create_handle = drm_gem_fb_create_handle,
-	.dirty	       = drm_atomic_helper_dirtyfb,
 };
 
 static struct drm_framebuffer *
-- 
2.27.0


[-- Attachment #3: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/rockchip: remove atomic helper dirtyfb

[Daniel Vetter]

On Wed, Feb 03, 2021 at 09:53:40PM +0200, Toni Spets wrote:
> The blocking implementation of the dirtyfb ioctl is extremely slow when
> used for damage tracking on RK3399. If this implementation is in place Xorg
> will default to using it and will slow down considerably when doing a lot
> of small draws. This is most apparent with the fvwm window manager on
> startup where it will almost lock up for many seconds seconds on RK3399.
> 
> Removing this implementation did not cause any visible issues on RK3399 but
> it did fix the performance issues on Xorg as it will disable damage
> tracking when the ioctl returns it's not supported.

Then you don't have a manual update panel.

Iirc there were patches to make this faster in recent kernels, on what
kernels did you try this?

Also X should only call this in the blocker handler, not all the time.

So yeah we need to make this faster, not break manual update panels.
-Daniel

> 
> -- 
> Toni Spets

> From 79984ee67c801f552e9eaf4d0cfb62101d1f0f2e Mon Sep 17 00:00:00 2001
> From: Toni Spets <toni.spets@iki.fi>
> Date: Wed, 3 Feb 2021 21:14:50 +0200
> Subject: [PATCH] drm/rockchip: remove atomic helper dirtyfb
> 
> ---
>  drivers/gpu/drm/rockchip/rockchip_drm_fb.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> index 3aa37e177667..2554fd1c8aeb 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> @@ -21,7 +21,6 @@
>  static const struct drm_framebuffer_funcs rockchip_drm_fb_funcs = {
>  	.destroy       = drm_gem_fb_destroy,
>  	.create_handle = drm_gem_fb_create_handle,
> -	.dirty	       = drm_atomic_helper_dirtyfb,
>  };
>  
>  static struct drm_framebuffer *
> -- 
> 2.27.0
> 

> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel


-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/rockchip: remove atomic helper dirtyfb

[Toni Spets]

[-- Attachment #1.1: Type: text/plain, Size: 2917 bytes --]

On Thu, Feb 4, 2021, 17:20 Daniel Vetter <daniel@ffwll.ch> wrote:

> On Wed, Feb 03, 2021 at 09:53:40PM +0200, Toni Spets wrote:
> > The blocking implementation of the dirtyfb ioctl is extremely slow when
> > used for damage tracking on RK3399. If this implementation is in place
> Xorg
> > will default to using it and will slow down considerably when doing a lot
> > of small draws. This is most apparent with the fvwm window manager on
> > startup where it will almost lock up for many seconds seconds on RK3399.
> >
> > Removing this implementation did not cause any visible issues on RK3399
> but
> > it did fix the performance issues on Xorg as it will disable damage
> > tracking when the ioctl returns it's not supported.
>
> Then you don't have a manual update panel.
>
> Iirc there were patches to make this faster in recent kernels, on what
> kernels did you try this?
>

Latest was 5.10.12. If there are fixes for this in later kernels I will
definitely try it out.


> Also X should only call this in the blocker handler, not all the time.
>

It does but fvwm is an example that forces it to be called a lot and it's
slow enough to cause significant issues.


> So yeah we need to make this faster, not break manual update panels.
>

Pardon my ignorance but while making this operation faster will indeed make
it better wouldn't the correct behavior be to know if a panel requires this
or not?

Making a low performance device wait any extra time for no reason doesn't
sound like the correct fix either.

I'm not defending the patch itself as I don't have enough understanding of
the drm or kernel so if it's indeed definitely breaking something then of
course it can't be used as is.

Thanks.


-Daniel
>
> >
> > --
> > Toni Spets
>
> > From 79984ee67c801f552e9eaf4d0cfb62101d1f0f2e Mon Sep 17 00:00:00 2001
> > From: Toni Spets <toni.spets@iki.fi>
> > Date: Wed, 3 Feb 2021 21:14:50 +0200
> > Subject: [PATCH] drm/rockchip: remove atomic helper dirtyfb
> >
> > ---
> >  drivers/gpu/drm/rockchip/rockchip_drm_fb.c | 1 -
> >  1 file changed, 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> > index 3aa37e177667..2554fd1c8aeb 100644
> > --- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> > @@ -21,7 +21,6 @@
> >  static const struct drm_framebuffer_funcs rockchip_drm_fb_funcs = {
> >       .destroy       = drm_gem_fb_destroy,
> >       .create_handle = drm_gem_fb_create_handle,
> > -     .dirty         = drm_atomic_helper_dirtyfb,
> >  };
> >
> >  static struct drm_framebuffer *
> > --
> > 2.27.0
> >
>
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
>

[-- Attachment #1.2: Type: text/html, Size: 4810 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/rockchip: remove atomic helper dirtyfb

[Daniel Vetter]

On Thu, Feb 4, 2021 at 5:03 PM Toni Spets <toni.spets@iki.fi> wrote:
>
>
>
> On Thu, Feb 4, 2021, 17:20 Daniel Vetter <daniel@ffwll.ch> wrote:
>>
>> On Wed, Feb 03, 2021 at 09:53:40PM +0200, Toni Spets wrote:
>> > The blocking implementation of the dirtyfb ioctl is extremely slow when
>> > used for damage tracking on RK3399. If this implementation is in place Xorg
>> > will default to using it and will slow down considerably when doing a lot
>> > of small draws. This is most apparent with the fvwm window manager on
>> > startup where it will almost lock up for many seconds seconds on RK3399.
>> >
>> > Removing this implementation did not cause any visible issues on RK3399 but
>> > it did fix the performance issues on Xorg as it will disable damage
>> > tracking when the ioctl returns it's not supported.
>>
>> Then you don't have a manual update panel.
>>
>> Iirc there were patches to make this faster in recent kernels, on what
>> kernels did you try this?
>
>
> Latest was 5.10.12. If there are fixes for this in later kernels I will definitely try it out.

Hm I thought it landed already. But checking it the optimization was
for fbdev to batch up updates more (because that one doesn't even
try), not direct X usage. So your X should work faster if you use
fbdev as backend (just as an experiment).

>> Also X should only call this in the blocker handler, not all the time.
>
>
> It does but fvwm is an example that forces it to be called a lot and it's slow enough to cause significant issues.
>
>>
>> So yeah we need to make this faster, not break manual update panels.
>
>
> Pardon my ignorance but while making this operation faster will indeed make it better wouldn't the correct behavior be to know if a panel requires this or not?

Not impossible, but there's a pile of layers in the way. And generally
frontbuffer rendering doesn't see a lot of love, since aside from
bootloaders and old sckool X window systems without compositors,
they're not really seeing any use. Everything Wayland or composited
desktops is double-buffered and fast.

For fbdev we're also doing the dirty tracking now at a driver level
(using the helpers), unconditionally whether the given hw actually
needs it or not.

More pragmatic approach would be to throw a kernel thread at this
problem. Will be tricky since we need to make sure that from
userspace's pov nothing breaks, which is always a bit an issue when
making things more asynchronous. Specifically we need to make sure
that userspace doesn't get ahead of the kernel, so might need to
require that we only batch up updates for the same framebuffer object,
but stall when we switch.

The locking for this will get interesting.

Cheers, Daniel

> Making a low performance device wait any extra time for no reason doesn't sound like the correct fix either.
>
> I'm not defending the patch itself as I don't have enough understanding of the drm or kernel so if it's indeed definitely breaking something then of course it can't be used as is.
>
> Thanks.
>
>
>> -Daniel
>>
>> >
>> > --
>> > Toni Spets
>>
>> > From 79984ee67c801f552e9eaf4d0cfb62101d1f0f2e Mon Sep 17 00:00:00 2001
>> > From: Toni Spets <toni.spets@iki.fi>
>> > Date: Wed, 3 Feb 2021 21:14:50 +0200
>> > Subject: [PATCH] drm/rockchip: remove atomic helper dirtyfb
>> >
>> > ---
>> >  drivers/gpu/drm/rockchip/rockchip_drm_fb.c | 1 -
>> >  1 file changed, 1 deletion(-)
>> >
>> > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
>> > index 3aa37e177667..2554fd1c8aeb 100644
>> > --- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
>> > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
>> > @@ -21,7 +21,6 @@
>> >  static const struct drm_framebuffer_funcs rockchip_drm_fb_funcs = {
>> >       .destroy       = drm_gem_fb_destroy,
>> >       .create_handle = drm_gem_fb_create_handle,
>> > -     .dirty         = drm_atomic_helper_dirtyfb,
>> >  };
>> >
>> >  static struct drm_framebuffer *
>> > --
>> > 2.27.0
>> >
>>
>> > _______________________________________________
>> > dri-devel mailing list
>> > dri-devel@lists.freedesktop.org
>> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>
>>
>> --
>> Daniel Vetter
>> Software Engineer, Intel Corporation
>> http://blog.ffwll.ch



-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/rockchip: remove atomic helper dirtyfb

[Toni Spets]

[-- Attachment #1.1: Type: text/plain, Size: 5851 bytes --]

On Thu, Feb 4, 2021, 18:18 Daniel Vetter <daniel@ffwll.ch> wrote:

> On Thu, Feb 4, 2021 at 5:03 PM Toni Spets <toni.spets@iki.fi> wrote:
> >
> >
> >
> > On Thu, Feb 4, 2021, 17:20 Daniel Vetter <daniel@ffwll.ch> wrote:
> >>
> >> On Wed, Feb 03, 2021 at 09:53:40PM +0200, Toni Spets wrote:
> >> > The blocking implementation of the dirtyfb ioctl is extremely slow
> when
> >> > used for damage tracking on RK3399. If this implementation is in
> place Xorg
> >> > will default to using it and will slow down considerably when doing a
> lot
> >> > of small draws. This is most apparent with the fvwm window manager on
> >> > startup where it will almost lock up for many seconds seconds on
> RK3399.
> >> >
> >> > Removing this implementation did not cause any visible issues on
> RK3399 but
> >> > it did fix the performance issues on Xorg as it will disable damage
> >> > tracking when the ioctl returns it's not supported.
> >>
> >> Then you don't have a manual update panel.
> >>
> >> Iirc there were patches to make this faster in recent kernels, on what
> >> kernels did you try this?
> >
> >
> > Latest was 5.10.12. If there are fixes for this in later kernels I will
> definitely try it out.
>
> Hm I thought it landed already. But checking it the optimization was
> for fbdev to batch up updates more (because that one doesn't even
> try), not direct X usage. So your X should work faster if you use
> fbdev as backend (just as an experiment).
>

Yeah, I used fbdev to test before which got me down this path to look up
what's wrong with the modesetting driver.

Fbdev runs flawlessly but then we lose panfrost for other acceleration so
it's unfortunately not a silver bullet.


> >> Also X should only call this in the blocker handler, not all the time.
> >
> >
> > It does but fvwm is an example that forces it to be called a lot and
> it's slow enough to cause significant issues.
> >
> >>
> >> So yeah we need to make this faster, not break manual update panels.
> >
> >
> > Pardon my ignorance but while making this operation faster will indeed
> make it better wouldn't the correct behavior be to know if a panel requires
> this or not?
>
> Not impossible, but there's a pile of layers in the way. And generally
> frontbuffer rendering doesn't see a lot of love, since aside from
> bootloaders and old sckool X window systems without compositors,
> they're not really seeing any use. Everything Wayland or composited
> desktops is double-buffered and fast.
>

It's not this black and white, unfortunately. Compositing even on Xorg is
slower than not using it at all at least on Xfce where currently this
hardware gets the best results with this patch applied and compositor off.
Maybe the implementation is just bad on Xfce?


> For fbdev we're also doing the dirty tracking now at a driver level
> (using the helpers), unconditionally whether the given hw actually
> needs it or not.
>
> More pragmatic approach would be to throw a kernel thread at this
> problem. Will be tricky since we need to make sure that from
> userspace's pov nothing breaks, which is always a bit an issue when
> making things more asynchronous. Specifically we need to make sure
> that userspace doesn't get ahead of the kernel, so might need to
> require that we only batch up updates for the same framebuffer object,
> but stall when we switch.
>
> The locking for this will get interesting.
>

Sounds like an awful lot of work for a niche use. Although if Wayland ends
up calling this even every now and then it may prove to be useful.

I haven't done any testing or benchmarking on Wayland so I have no
knowledge if this makes any difference there.

Maybe this could solved in Xorg by having an option to disable damage
tracking manually, though ideally no manual setup would be needed and I
doubt anyone wants new options for the modesetting driver for an edge case
like this.

Don't really know where to go from here. Wouldn't want to carry this patch
manually just to get a well working X either.

Thanks for the insight and I'm happy to test any solution if something
comes up!


> Cheers, Daniel
>
> > Making a low performance device wait any extra time for no reason
> doesn't sound like the correct fix either.
> >
> > I'm not defending the patch itself as I don't have enough understanding
> of the drm or kernel so if it's indeed definitely breaking something then
> of course it can't be used as is.
> >
> > Thanks.
> >
> >
> >> -Daniel
> >>
> >> >
> >> > --
> >> > Toni Spets
> >>
> >> > From 79984ee67c801f552e9eaf4d0cfb62101d1f0f2e Mon Sep 17 00:00:00 2001
> >> > From: Toni Spets <toni.spets@iki.fi>
> >> > Date: Wed, 3 Feb 2021 21:14:50 +0200
> >> > Subject: [PATCH] drm/rockchip: remove atomic helper dirtyfb
> >> >
> >> > ---
> >> >  drivers/gpu/drm/rockchip/rockchip_drm_fb.c | 1 -
> >> >  1 file changed, 1 deletion(-)
> >> >
> >> > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> >> > index 3aa37e177667..2554fd1c8aeb 100644
> >> > --- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> >> > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> >> > @@ -21,7 +21,6 @@
> >> >  static const struct drm_framebuffer_funcs rockchip_drm_fb_funcs = {
> >> >       .destroy       = drm_gem_fb_destroy,
> >> >       .create_handle = drm_gem_fb_create_handle,
> >> > -     .dirty         = drm_atomic_helper_dirtyfb,
> >> >  };
> >> >
> >> >  static struct drm_framebuffer *
> >> > --
> >> > 2.27.0
> >> >
> >>
> >> > _______________________________________________
> >> > dri-devel mailing list
> >> > dri-devel@lists.freedesktop.org
> >> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >>
> >>
> >> --
> >> Daniel Vetter
> >> Software Engineer, Intel Corporation
> >> http://blog.ffwll.ch
>
>
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
>

[-- Attachment #1.2: Type: text/html, Size: 8780 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel