[PATCH RFC] drm/sysfb: Copy damage outside the DIRTYFB ioctl

[PATCH RFC] drm/sysfb: Copy damage outside the DIRTYFB ioctl

[Joseph Telaak]

The sysfb drivers implement DIRTYFB with drm_atomic_helper_dirtyfb(),
which runs a blocking commit. The commit waits for the framebuffer's
fences, so the ioctl doesn't return until the GPU has finished
rendering into the framebuffer.

Xorg's modesetting driver calls DIRTYFB from its main thread after
every frame. When a separate GPU renders for a sysfb display, X then
blocks for as long as the GPU takes to render the client's frame. With
a game running, X took a median of 1.85 s to answer a pointer query.

Queue the damage instead and copy it from a worker. The worker still
waits for the fences, so nothing unfinished reaches the screen, and
damage that arrives while a copy is pending is merged into it.

With this change, X answers in a median of 23 ms and the game's frame
rate is unchanged.

Signed-off-by: Joseph Telaak <joseph@telaak.dev>
---
This is an RFC because I'm not sure DIRTYFB may return before the copy
is done. The fbdev emulation already defers its damage to a worker,
but userspace might rely on DIRTYFB being synchronous. If the approach
is fine, would you rather have it as a nonblocking mode of
drm_atomic_helper_dirtyfb() for all drivers instead of in sysfb?

I hit this on an Intel Atom CE5300 board, where simpledrm drives the
display and the SoC's PowerVR SGX545 GPU renders with a driver I'm
writing (not upstream yet). I haven't reproduced it with an upstream
GPU driver, but nothing in the stall is specific to the GPU.
---
 drivers/gpu/drm/sysfb/corebootdrm.c       |   3 +
 drivers/gpu/drm/sysfb/drm_sysfb_helper.h  |  15 ++-
 drivers/gpu/drm/sysfb/drm_sysfb_modeset.c | 154 ++++++++++++++++++++++++++++++
 drivers/gpu/drm/sysfb/efidrm.c            |   3 +
 drivers/gpu/drm/sysfb/ofdrm.c             |   3 +
 drivers/gpu/drm/sysfb/simpledrm.c         |   3 +
 drivers/gpu/drm/sysfb/vesadrm.c           |   3 +
 7 files changed, 183 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/sysfb/corebootdrm.c b/drivers/gpu/drm/sysfb/corebootdrm.c
index 853da9a..4ca359b 100644
--- a/drivers/gpu/drm/sysfb/corebootdrm.c
+++ b/drivers/gpu/drm/sysfb/corebootdrm.c
@@ -206,6 +206,9 @@ static int corebootdrm_mode_config_init(struct corebootdrm_device *cdev,
 	int ret;
 
 	ret = drmm_mode_config_init(dev);
+	if (ret)
+		return ret;
+	ret = drm_sysfb_damage_init(sysfb);
 	if (ret)
 		return ret;
 
diff --git a/drivers/gpu/drm/sysfb/drm_sysfb_helper.h b/drivers/gpu/drm/sysfb/drm_sysfb_helper.h
index 6dab27b..59c4064 100644
--- a/drivers/gpu/drm/sysfb/drm_sysfb_helper.h
+++ b/drivers/gpu/drm/sysfb/drm_sysfb_helper.h
@@ -5,6 +5,8 @@
 
 #include <linux/container_of.h>
 #include <linux/iosys-map.h>
+#include <linux/spinlock.h>
+#include <linux/workqueue.h>
 
 #include <video/pixel_format.h>
 
@@ -81,6 +83,12 @@ struct drm_sysfb_device {
 
 	/* hardware-framebuffer kernel address */
 	struct iosys_map fb_addr;
+
+	/* damage from DIRTYFB, copied to the hardware framebuffer by damage_work */
+	spinlock_t damage_lock;
+	struct drm_framebuffer *damage_fb;
+	struct drm_rect damage;
+	struct work_struct damage_work;
 };
 
 static inline struct drm_sysfb_device *to_drm_sysfb_device(struct drm_device *dev)
@@ -203,8 +211,13 @@ int drm_sysfb_connector_helper_get_modes(struct drm_connector *connector);
  * Mode config
  */
 
+int drm_sysfb_damage_init(struct drm_sysfb_device *sysfb);
+struct drm_framebuffer *drm_sysfb_fb_create(struct drm_device *dev, struct drm_file *file,
+					    const struct drm_format_info *info,
+					    const struct drm_mode_fb_cmd2 *mode_cmd);
+
 #define DRM_SYSFB_MODE_CONFIG_FUNCS \
-	.fb_create = drm_gem_fb_create_with_dirty, \
+	.fb_create = drm_sysfb_fb_create, \
 	.atomic_check = drm_atomic_helper_check, \
 	.atomic_commit = drm_atomic_helper_commit
 
diff --git a/drivers/gpu/drm/sysfb/drm_sysfb_modeset.c b/drivers/gpu/drm/sysfb/drm_sysfb_modeset.c
index 13df092..e77f0c0 100644
--- a/drivers/gpu/drm/sysfb/drm_sysfb_modeset.c
+++ b/drivers/gpu/drm/sysfb/drm_sysfb_modeset.c
@@ -12,6 +12,7 @@
 #include <drm/drm_fourcc.h>
 #include <drm/drm_framebuffer.h>
 #include <drm/drm_gem_framebuffer_helper.h>
+#include <drm/drm_managed.h>
 #include <drm/drm_panic.h>
 #include <drm/drm_print.h>
 #include <drm/drm_probe_helper.h>
@@ -612,3 +613,156 @@ int drm_sysfb_connector_helper_get_modes(struct drm_connector *connector)
 	return drm_connector_helper_get_modes_fixed(connector, &sysfb->fb_mode);
 }
 EXPORT_SYMBOL(drm_sysfb_connector_helper_get_modes);
+
+/*
+ * Damage
+ *
+ * DIRTYFB returns at once: the damage is merged into one pending rectangle,
+ * and a worker hands it to drm_atomic_helper_dirtyfb(). That commit waits for
+ * the framebuffer's fences, the GPU work that last rendered into it, before
+ * the CPU copies it to the hardware framebuffer. Done in the ioctl, the wait
+ * blocks the compositor for as long as that rendering takes. Damage that
+ * arrives while the worker runs is merged and copied by the next run, so none
+ * is dropped.
+ */
+
+static void drm_sysfb_damage_work(struct work_struct *work)
+{
+	struct drm_sysfb_device *sysfb = container_of(work, struct drm_sysfb_device,
+						      damage_work);
+	struct drm_framebuffer *fb;
+	struct drm_clip_rect clip;
+	int idx, ret;
+
+	spin_lock(&sysfb->damage_lock);
+	fb = sysfb->damage_fb;
+	sysfb->damage_fb = NULL;
+	clip.x1 = sysfb->damage.x1;
+	clip.y1 = sysfb->damage.y1;
+	clip.x2 = sysfb->damage.x2;
+	clip.y2 = sysfb->damage.y2;
+	spin_unlock(&sysfb->damage_lock);
+	if (!fb)
+		return;
+
+	if (drm_dev_enter(&sysfb->dev, &idx)) {
+		ret = drm_atomic_helper_dirtyfb(fb, NULL, 0, 0, &clip, 1);
+		if (ret)
+			drm_err_ratelimited(&sysfb->dev, "damage update failed: %d\n", ret);
+		drm_dev_exit(idx);
+	}
+	drm_framebuffer_put(fb);
+}
+
+static void drm_sysfb_damage_fini(struct drm_device *dev, void *arg)
+{
+	struct drm_sysfb_device *sysfb = arg;
+
+	cancel_work_sync(&sysfb->damage_work);
+	if (sysfb->damage_fb)
+		drm_framebuffer_put(sysfb->damage_fb);
+	sysfb->damage_fb = NULL;
+}
+
+/**
+ * drm_sysfb_damage_init - Set up deferred damage for a sysfb device
+ * @sysfb: the device
+ *
+ * Call after drmm_mode_config_init(). Framebuffers from drm_sysfb_fb_create()
+ * then copy their damage outside the DIRTYFB ioctl.
+ *
+ * Returns: 0 on success, or a negative errno.
+ */
+int drm_sysfb_damage_init(struct drm_sysfb_device *sysfb)
+{
+	spin_lock_init(&sysfb->damage_lock);
+	INIT_WORK(&sysfb->damage_work, drm_sysfb_damage_work);
+
+	return drmm_add_action_or_reset(&sysfb->dev, drm_sysfb_damage_fini, sysfb);
+}
+EXPORT_SYMBOL(drm_sysfb_damage_init);
+
+static void drm_sysfb_damage_bounds(struct drm_framebuffer *fb, unsigned int flags,
+				    const struct drm_clip_rect *clips, unsigned int num_clips,
+				    struct drm_rect *r)
+{
+	unsigned int inc = 1;
+
+	if (!clips || !num_clips) {
+		drm_rect_init(r, 0, 0, fb->width, fb->height);
+		return;
+	}
+	if (flags & DRM_MODE_FB_DIRTY_ANNOTATE_COPY) {
+		inc = 2;
+		num_clips /= 2;
+	}
+	drm_rect_init(r, clips->x1, clips->y1, clips->x2 - clips->x1, clips->y2 - clips->y1);
+	for (unsigned int i = 1; i < num_clips; i++) {
+		clips += inc;
+		r->x1 = min_t(int, r->x1, clips->x1);
+		r->y1 = min_t(int, r->y1, clips->y1);
+		r->x2 = max_t(int, r->x2, clips->x2);
+		r->y2 = max_t(int, r->y2, clips->y2);
+	}
+}
+
+static int drm_sysfb_fb_dirty(struct drm_framebuffer *fb, struct drm_file *file,
+			      unsigned int flags, unsigned int color,
+			      struct drm_clip_rect *clips, unsigned int num_clips)
+{
+	struct drm_sysfb_device *sysfb = to_drm_sysfb_device(fb->dev);
+	struct drm_rect r;
+	bool queue = false;
+
+	drm_sysfb_damage_bounds(fb, flags, clips, num_clips, &r);
+
+	spin_lock(&sysfb->damage_lock);
+	/* Pending damage of another framebuffer is copied first. */
+	while (sysfb->damage_fb && sysfb->damage_fb != fb) {
+		spin_unlock(&sysfb->damage_lock);
+		flush_work(&sysfb->damage_work);
+		spin_lock(&sysfb->damage_lock);
+	}
+	if (!sysfb->damage_fb) {
+		drm_framebuffer_get(fb);
+		sysfb->damage_fb = fb;
+		sysfb->damage = r;
+		queue = true;
+	} else {
+		sysfb->damage.x1 = min(sysfb->damage.x1, r.x1);
+		sysfb->damage.y1 = min(sysfb->damage.y1, r.y1);
+		sysfb->damage.x2 = max(sysfb->damage.x2, r.x2);
+		sysfb->damage.y2 = max(sysfb->damage.y2, r.y2);
+	}
+	spin_unlock(&sysfb->damage_lock);
+
+	if (queue)
+		queue_work(system_dfl_wq, &sysfb->damage_work);
+	return 0;
+}
+
+static const struct drm_framebuffer_funcs drm_sysfb_fb_funcs = {
+	.destroy = drm_gem_fb_destroy,
+	.create_handle = drm_gem_fb_create_handle,
+	.dirty = drm_sysfb_fb_dirty,
+};
+
+/**
+ * drm_sysfb_fb_create - &drm_mode_config_funcs.fb_create for sysfb devices
+ * @dev: the device
+ * @file: the DRM file of the caller
+ * @info: the pixel format
+ * @mode_cmd: the framebuffer from userspace
+ *
+ * Like drm_gem_fb_create_with_dirty(), but the framebuffer's damage is copied
+ * outside the DIRTYFB ioctl (drm_sysfb_damage_init()).
+ *
+ * Returns: the framebuffer, or an ERR_PTR() on failure.
+ */
+struct drm_framebuffer *drm_sysfb_fb_create(struct drm_device *dev, struct drm_file *file,
+					    const struct drm_format_info *info,
+					    const struct drm_mode_fb_cmd2 *mode_cmd)
+{
+	return drm_gem_fb_create_with_funcs(dev, file, info, mode_cmd, &drm_sysfb_fb_funcs);
+}
+EXPORT_SYMBOL(drm_sysfb_fb_create);
diff --git a/drivers/gpu/drm/sysfb/efidrm.c b/drivers/gpu/drm/sysfb/efidrm.c
index 8b19f43..0ef093a 100644
--- a/drivers/gpu/drm/sysfb/efidrm.c
+++ b/drivers/gpu/drm/sysfb/efidrm.c
@@ -299,6 +299,9 @@ static struct efidrm_device *efidrm_device_create(struct drm_driver *drv,
 	 */
 
 	ret = drmm_mode_config_init(dev);
+	if (ret)
+		return ERR_PTR(ret);
+	ret = drm_sysfb_damage_init(sysfb);
 	if (ret)
 		return ERR_PTR(ret);
 
diff --git a/drivers/gpu/drm/sysfb/ofdrm.c b/drivers/gpu/drm/sysfb/ofdrm.c
index c412555..4d31bb6 100644
--- a/drivers/gpu/drm/sysfb/ofdrm.c
+++ b/drivers/gpu/drm/sysfb/ofdrm.c
@@ -1021,6 +1021,9 @@ static struct ofdrm_device *ofdrm_device_create(struct drm_driver *drv,
 	 */
 
 	ret = drmm_mode_config_init(dev);
+	if (ret)
+		return ERR_PTR(ret);
+	ret = drm_sysfb_damage_init(sysfb);
 	if (ret)
 		return ERR_PTR(ret);
 
diff --git a/drivers/gpu/drm/sysfb/simpledrm.c b/drivers/gpu/drm/sysfb/simpledrm.c
index deaeb7d..c40a6c5 100644
--- a/drivers/gpu/drm/sysfb/simpledrm.c
+++ b/drivers/gpu/drm/sysfb/simpledrm.c
@@ -813,6 +813,9 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
 	 */
 
 	ret = drmm_mode_config_init(dev);
+	if (ret)
+		return ERR_PTR(ret);
+	ret = drm_sysfb_damage_init(sysfb);
 	if (ret)
 		return ERR_PTR(ret);
 
diff --git a/drivers/gpu/drm/sysfb/vesadrm.c b/drivers/gpu/drm/sysfb/vesadrm.c
index 1668aab..812db4a 100644
--- a/drivers/gpu/drm/sysfb/vesadrm.c
+++ b/drivers/gpu/drm/sysfb/vesadrm.c
@@ -539,6 +539,9 @@ static struct vesadrm_device *vesadrm_device_create(struct drm_driver *drv,
 	 */
 
 	ret = drmm_mode_config_init(dev);
+	if (ret)
+		return ERR_PTR(ret);
+	ret = drm_sysfb_damage_init(sysfb);
 	if (ret)
 		return ERR_PTR(ret);
 

---
base-commit: 41b5458595c60143d0ebaa1e032c9cda5d764dff
change-id: 20261110-sysfb-dirtyfb-beda10fab602

Best regards,
--  
Joseph Telaak <joseph@telaak.dev>

Re: [PATCH RFC] drm/sysfb: Copy damage outside the DIRTYFB ioctl

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential
issues to consider:
- [High] drm/sysfb: busy-spin and livelock in drm_sysfb_fb_dirty()
  flush_work() loop
- [High] drm/sysfb: race between drm_sysfb_damage_work() and PM
  suspend/resume
- [Medium] drm/sysfb: lock inversion holding drm_dev_enter() in
  drm_sysfb_damage_work()
- [Medium] drm/sysfb: concurrent frontbuffer mutation and lock stall in
  drm_sysfb_damage_work()
--

commit 092a1b6cbfb2ea4259c79aad2c25c2ec9f2a259a
Author: Joseph Telaak <joseph@telaak.dev>

drm/sysfb: Copy damage outside the DIRTYFB ioctl

This commit defers sysfb framebuffer damage updates from the DIRTYFB ioctl to
a workqueue item that merges damage rectangles and runs
drm_atomic_helper_dirtyfb() asynchronously.

> diff --git a/drivers/gpu/drm/sysfb/drm_sysfb_modeset.c b/drivers/gpu/drm/sysfb/drm_sysfb_modeset.c
> index 13df0925163a5..e77f0c0ffc272 100644
> --- a/drivers/gpu/drm/sysfb/drm_sysfb_modeset.c
> +++ b/drivers/gpu/drm/sysfb/drm_sysfb_modeset.c
[ ... ]
> +static void drm_sysfb_damage_work(struct work_struct *work)
> +{
> +	struct drm_sysfb_device *sysfb = container_of(work, struct drm_sysfb_device,
> +						      damage_work);
> +	struct drm_framebuffer *fb;
> +	struct drm_clip_rect clip;
> +	int idx, ret;
> +
> +	spin_lock(&sysfb->damage_lock);
> +	fb = sysfb->damage_fb;
> +	sysfb->damage_fb = NULL;
> +	clip.x1 = sysfb->damage.x1;
> +	clip.y1 = sysfb->damage.y1;
> +	clip.x2 = sysfb->damage.x2;
> +	clip.y2 = sysfb->damage.y2;
> +	spin_unlock(&sysfb->damage_lock);
> +	if (!fb)
> +		return;
> +
> +	if (drm_dev_enter(&sysfb->dev, &idx)) {
> +		ret = drm_atomic_helper_dirtyfb(fb, NULL, 0, 0, &clip, 1);
> +		if (ret)
> +			drm_err_ratelimited(&sysfb->dev, "damage update failed: %d\n", ret);
> +		drm_dev_exit(idx);
> +	}
> +	drm_framebuffer_put(fb);
> +}

[Severity: High]
Can drm_sysfb_damage_work() race with system suspend and resume, causing a
use-after-free or NULL pointer dereference on plane->state?

All sysfb drivers use drm_mode_config_helper_suspend() and
drm_mode_config_helper_resume(), which do not flush or cancel
sysfb->damage_work, and system_dfl_wq is not freezable.  During resume,
drm_atomic_helper_resume() calls drm_mode_config_reset() before acquiring
modeset locks, which destroys and reallocates plane->state unlocked:

CPU0                              CPU1 (kworker)
drm_mode_config_helper_resume()
  drm_atomic_helper_resume()
    drm_mode_config_reset()
      ...
        kfree(plane->state)
        plane->state = NULL
                                  drm_sysfb_damage_work()
                                    drm_atomic_helper_dirtyfb()
                                      drm_modeset_lock(&plane->mutex, ...)
                                      plane->state->fb  <-- NULL deref / UAF

If drm_sysfb_damage_work() runs while the device is suspended or resuming, it
can also blit to the hardware framebuffer while the display is suspended.

[Severity: Medium]
Does holding drm_dev_enter() across drm_atomic_helper_dirtyfb() invert the
locking order between drm_unplug_srcu and modeset, dma_resv, and dma_fence
locks?

drm_dev_enter() acquires a read lock on the global drm_unplug_srcu, and
drm_atomic_helper_dirtyfb() then acquires modeset locks, takes dma_resv_lock()
in drm_gem_fb_vmap(), and waits on GPU fences in
drm_atomic_helper_wait_for_fences().  Normal atomic commits acquire modeset
locks, dma_resv_lock(), and dma_fence_map before calling drm_dev_enter() in
drm_sysfb_plane_helper_atomic_update():

CPU0 (kworker)                    CPU1
----                              ----
lock(drm_unplug_srcu)
                                  lock(&plane->mutex)
lock(&plane->mutex)
                                  lock(drm_unplug_srcu)

 *** DEADLOCK ***

CPU0: drm_sysfb_damage_work() -> drm_atomic_helper_dirtyfb()
CPU1: drm_mode_atomic_ioctl() -> ... -> drm_sysfb_plane_helper_atomic_update()

If the rendering GPU is unplugged or halted while drm_sysfb_damage_work() is
waiting on its fence, drm_dev_unplug() on the GPU will also block in
synchronize_srcu(&drm_unplug_srcu) before it can reach the fence teardown path
that aborts the in-flight fence.  Since drm_sysfb_plane_helper_atomic_update()
already guards the hardware framebuffer blit with drm_dev_enter(), the outer
drm_dev_enter() in drm_sysfb_damage_work() can be dropped.

[ ... ]

> +static int drm_sysfb_fb_dirty(struct drm_framebuffer *fb, struct drm_file *file,
> +			      unsigned int flags, unsigned int color,
> +			      struct drm_clip_rect *clips, unsigned int num_clips)
> +{
> +	struct drm_sysfb_device *sysfb = to_drm_sysfb_device(fb->dev);
> +	struct drm_rect r;
> +	bool queue = false;
> +
> +	drm_sysfb_damage_bounds(fb, flags, clips, num_clips, &r);
> +
> +	spin_lock(&sysfb->damage_lock);
> +	/* Pending damage of another framebuffer is copied first. */\
> +	while (sysfb->damage_fb && sysfb->damage_fb != fb) {
> +		spin_unlock(&sysfb->damage_lock);
> +		flush_work(&sysfb->damage_work);
> +		spin_lock(&sysfb->damage_lock);
> +	}
> +	if (!sysfb->damage_fb) {
> +		drm_framebuffer_get(fb);
> +		sysfb->damage_fb = fb;
> +		sysfb->damage = r;
> +		queue = true;
> +	} else {
> +		sysfb->damage.x1 = min(sysfb->damage.x1, r.x1);
> +		sysfb->damage.y1 = min(sysfb->damage.y1, r.y1);
> +		sysfb->damage.x2 = max(sysfb->damage.x2, r.x2);
> +		sysfb->damage.y2 = max(sysfb->damage.y2, r.y2);
> +	}
> +	spin_unlock(&sysfb->damage_lock);
> +
> +	if (queue)
> +		queue_work(system_dfl_wq, &sysfb->damage_work);
> +	return 0;
> +}

[Severity: High]
Can a second caller with a different framebuffer busy-spin or livelock in the
flush_work() loop when sysfb->damage_fb is set before queue_work() is called?

sysfb->damage_fb is published under sysfb->damage_lock, but queue_work() is
called only after dropping the lock.  If another thread calls
drm_sysfb_fb_dirty() for a different framebuffer in that window, flush_work()
sees that sysfb->damage_work is not queued and returns false immediately:

CPU0                              CPU1
drm_sysfb_fb_dirty(fb1)
  spin_lock(&sysfb->damage_lock)
  sysfb->damage_fb = fb1
  queue = true
  spin_unlock(&sysfb->damage_lock)
  <preempted>
                                  drm_sysfb_fb_dirty(fb2)
                                    spin_lock(&sysfb->damage_lock)
                                    while (sysfb->damage_fb != fb2)
                                      spin_unlock(&sysfb->damage_lock)
                                      flush_work(&sysfb->damage_work)
                                      spin_lock(&sysfb->damage_lock)
                                      <-- spins forever if RT thread on same CPU
  queue_work(..., &sysfb->damage_work)

In addition, because drm_sysfb_damage_work() clears sysfb->damage_fb to NULL
before calling drm_atomic_helper_dirtyfb(), further DIRTYFB calls on fb1 while
the worker is waiting on fences will re-populate sysfb->damage_fb with fb1
before flush_work() returns, forcing the waiter for fb2 to stall across
multiple fence-wait cycles.  Calling queue_work() while holding
sysfb->damage_lock fixes the unqueued flush_work() race.

[Severity: Medium]
Does returning immediately from drm_sysfb_fb_dirty() allow userspace or the
GPU to overwrite the frontbuffer while drm_sysfb_damage_work() is copying from
it, and stall non-blocking atomic commits on modeset locks?

When userspace renders into a mmap'd dumb buffer with the CPU (where no
dma_fence is attached) or submits the next frame's GPU job to the same
frontbuffer as soon as the previous fence signals, drm_sysfb_damage_work()
reads from the framebuffer concurrently with the new frame's writes, copying
unfinished frames to the screen and removing DIRTYFB upload rate-limiting.
Furthermore, drm_sysfb_damage_work() runs a blocking commit via
drm_atomic_helper_dirtyfb() while holding plane->mutex and crtc->mutex across
drm_atomic_helper_wait_for_fences(), which blocks concurrent
DRM_MODE_ATOMIC_NONBLOCK commits behind the GPU fence wait.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110-sysfb-dirtyfb-v1-1-8571d455ccbe@telaak.dev?part=1

Re: [PATCH RFC] drm/sysfb: Copy damage outside the DIRTYFB ioctl

[Thomas Zimmermann]

Hi

Am 11.11.26 um 02:37 schrieb Joseph Telaak:
> The sysfb drivers implement DIRTYFB with drm_atomic_helper_dirtyfb(),
> which runs a blocking commit. The commit waits for the framebuffer's
> fences, so the ioctl doesn't return until the GPU has finished
> rendering into the framebuffer.

Yes, that's a requirement.

>
> Xorg's modesetting driver calls DIRTYFB from its main thread after
> every frame. When a separate GPU renders for a sysfb display, X then
> blocks for as long as the GPU takes to render the client's frame. With
> a game running, X took a median of 1.85 s to answer a pointer query.
>
> Queue the damage instead and copy it from a worker. The worker still
> waits for the fences, so nothing unfinished reaches the screen, and
> damage that arrives while a copy is pending is merged into it.

Pushing out work from the display update into a worker breaks feedback 
from driver back to compositors. Your compositor/Xorg/whatever needs to 
adapt to and deal with slow framerates.

>
> With this change, X answers in a median of 23 ms and the game's frame
> rate is unchanged.

Fix X if you really care.

Best regards
Thomas

>
> Signed-off-by: Joseph Telaak <joseph@telaak.dev>
> ---
> This is an RFC because I'm not sure DIRTYFB may return before the copy
> is done. The fbdev emulation already defers its damage to a worker,
> but userspace might rely on DIRTYFB being synchronous. If the approach
> is fine, would you rather have it as a nonblocking mode of
> drm_atomic_helper_dirtyfb() for all drivers instead of in sysfb?
>
> I hit this on an Intel Atom CE5300 board, where simpledrm drives the
> display and the SoC's PowerVR SGX545 GPU renders with a driver I'm
> writing (not upstream yet). I haven't reproduced it with an upstream
> GPU driver, but nothing in the stall is specific to the GPU.
> ---
>   drivers/gpu/drm/sysfb/corebootdrm.c       |   3 +
>   drivers/gpu/drm/sysfb/drm_sysfb_helper.h  |  15 ++-
>   drivers/gpu/drm/sysfb/drm_sysfb_modeset.c | 154 ++++++++++++++++++++++++++++++
>   drivers/gpu/drm/sysfb/efidrm.c            |   3 +
>   drivers/gpu/drm/sysfb/ofdrm.c             |   3 +
>   drivers/gpu/drm/sysfb/simpledrm.c         |   3 +
>   drivers/gpu/drm/sysfb/vesadrm.c           |   3 +
>   7 files changed, 183 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/sysfb/corebootdrm.c b/drivers/gpu/drm/sysfb/corebootdrm.c
> index 853da9a..4ca359b 100644
> --- a/drivers/gpu/drm/sysfb/corebootdrm.c
> +++ b/drivers/gpu/drm/sysfb/corebootdrm.c
> @@ -206,6 +206,9 @@ static int corebootdrm_mode_config_init(struct corebootdrm_device *cdev,
>   	int ret;
>   
>   	ret = drmm_mode_config_init(dev);
> +	if (ret)
> +		return ret;
> +	ret = drm_sysfb_damage_init(sysfb);
>   	if (ret)
>   		return ret;
>   
> diff --git a/drivers/gpu/drm/sysfb/drm_sysfb_helper.h b/drivers/gpu/drm/sysfb/drm_sysfb_helper.h
> index 6dab27b..59c4064 100644
> --- a/drivers/gpu/drm/sysfb/drm_sysfb_helper.h
> +++ b/drivers/gpu/drm/sysfb/drm_sysfb_helper.h
> @@ -5,6 +5,8 @@
>   
>   #include <linux/container_of.h>
>   #include <linux/iosys-map.h>
> +#include <linux/spinlock.h>
> +#include <linux/workqueue.h>
>   
>   #include <video/pixel_format.h>
>   
> @@ -81,6 +83,12 @@ struct drm_sysfb_device {
>   
>   	/* hardware-framebuffer kernel address */
>   	struct iosys_map fb_addr;
> +
> +	/* damage from DIRTYFB, copied to the hardware framebuffer by damage_work */
> +	spinlock_t damage_lock;
> +	struct drm_framebuffer *damage_fb;
> +	struct drm_rect damage;
> +	struct work_struct damage_work;
>   };
>   
>   static inline struct drm_sysfb_device *to_drm_sysfb_device(struct drm_device *dev)
> @@ -203,8 +211,13 @@ int drm_sysfb_connector_helper_get_modes(struct drm_connector *connector);
>    * Mode config
>    */
>   
> +int drm_sysfb_damage_init(struct drm_sysfb_device *sysfb);
> +struct drm_framebuffer *drm_sysfb_fb_create(struct drm_device *dev, struct drm_file *file,
> +					    const struct drm_format_info *info,
> +					    const struct drm_mode_fb_cmd2 *mode_cmd);
> +
>   #define DRM_SYSFB_MODE_CONFIG_FUNCS \
> -	.fb_create = drm_gem_fb_create_with_dirty, \
> +	.fb_create = drm_sysfb_fb_create, \
>   	.atomic_check = drm_atomic_helper_check, \
>   	.atomic_commit = drm_atomic_helper_commit
>   
> diff --git a/drivers/gpu/drm/sysfb/drm_sysfb_modeset.c b/drivers/gpu/drm/sysfb/drm_sysfb_modeset.c
> index 13df092..e77f0c0 100644
> --- a/drivers/gpu/drm/sysfb/drm_sysfb_modeset.c
> +++ b/drivers/gpu/drm/sysfb/drm_sysfb_modeset.c
> @@ -12,6 +12,7 @@
>   #include <drm/drm_fourcc.h>
>   #include <drm/drm_framebuffer.h>
>   #include <drm/drm_gem_framebuffer_helper.h>
> +#include <drm/drm_managed.h>
>   #include <drm/drm_panic.h>
>   #include <drm/drm_print.h>
>   #include <drm/drm_probe_helper.h>
> @@ -612,3 +613,156 @@ int drm_sysfb_connector_helper_get_modes(struct drm_connector *connector)
>   	return drm_connector_helper_get_modes_fixed(connector, &sysfb->fb_mode);
>   }
>   EXPORT_SYMBOL(drm_sysfb_connector_helper_get_modes);
> +
> +/*
> + * Damage
> + *
> + * DIRTYFB returns at once: the damage is merged into one pending rectangle,
> + * and a worker hands it to drm_atomic_helper_dirtyfb(). That commit waits for
> + * the framebuffer's fences, the GPU work that last rendered into it, before
> + * the CPU copies it to the hardware framebuffer. Done in the ioctl, the wait
> + * blocks the compositor for as long as that rendering takes. Damage that
> + * arrives while the worker runs is merged and copied by the next run, so none
> + * is dropped.
> + */
> +
> +static void drm_sysfb_damage_work(struct work_struct *work)
> +{
> +	struct drm_sysfb_device *sysfb = container_of(work, struct drm_sysfb_device,
> +						      damage_work);
> +	struct drm_framebuffer *fb;
> +	struct drm_clip_rect clip;
> +	int idx, ret;
> +
> +	spin_lock(&sysfb->damage_lock);
> +	fb = sysfb->damage_fb;
> +	sysfb->damage_fb = NULL;
> +	clip.x1 = sysfb->damage.x1;
> +	clip.y1 = sysfb->damage.y1;
> +	clip.x2 = sysfb->damage.x2;
> +	clip.y2 = sysfb->damage.y2;
> +	spin_unlock(&sysfb->damage_lock);
> +	if (!fb)
> +		return;
> +
> +	if (drm_dev_enter(&sysfb->dev, &idx)) {
> +		ret = drm_atomic_helper_dirtyfb(fb, NULL, 0, 0, &clip, 1);
> +		if (ret)
> +			drm_err_ratelimited(&sysfb->dev, "damage update failed: %d\n", ret);
> +		drm_dev_exit(idx);
> +	}
> +	drm_framebuffer_put(fb);
> +}
> +
> +static void drm_sysfb_damage_fini(struct drm_device *dev, void *arg)
> +{
> +	struct drm_sysfb_device *sysfb = arg;
> +
> +	cancel_work_sync(&sysfb->damage_work);
> +	if (sysfb->damage_fb)
> +		drm_framebuffer_put(sysfb->damage_fb);
> +	sysfb->damage_fb = NULL;
> +}
> +
> +/**
> + * drm_sysfb_damage_init - Set up deferred damage for a sysfb device
> + * @sysfb: the device
> + *
> + * Call after drmm_mode_config_init(). Framebuffers from drm_sysfb_fb_create()
> + * then copy their damage outside the DIRTYFB ioctl.
> + *
> + * Returns: 0 on success, or a negative errno.
> + */
> +int drm_sysfb_damage_init(struct drm_sysfb_device *sysfb)
> +{
> +	spin_lock_init(&sysfb->damage_lock);
> +	INIT_WORK(&sysfb->damage_work, drm_sysfb_damage_work);
> +
> +	return drmm_add_action_or_reset(&sysfb->dev, drm_sysfb_damage_fini, sysfb);
> +}
> +EXPORT_SYMBOL(drm_sysfb_damage_init);
> +
> +static void drm_sysfb_damage_bounds(struct drm_framebuffer *fb, unsigned int flags,
> +				    const struct drm_clip_rect *clips, unsigned int num_clips,
> +				    struct drm_rect *r)
> +{
> +	unsigned int inc = 1;
> +
> +	if (!clips || !num_clips) {
> +		drm_rect_init(r, 0, 0, fb->width, fb->height);
> +		return;
> +	}
> +	if (flags & DRM_MODE_FB_DIRTY_ANNOTATE_COPY) {
> +		inc = 2;
> +		num_clips /= 2;
> +	}
> +	drm_rect_init(r, clips->x1, clips->y1, clips->x2 - clips->x1, clips->y2 - clips->y1);
> +	for (unsigned int i = 1; i < num_clips; i++) {
> +		clips += inc;
> +		r->x1 = min_t(int, r->x1, clips->x1);
> +		r->y1 = min_t(int, r->y1, clips->y1);
> +		r->x2 = max_t(int, r->x2, clips->x2);
> +		r->y2 = max_t(int, r->y2, clips->y2);
> +	}
> +}
> +
> +static int drm_sysfb_fb_dirty(struct drm_framebuffer *fb, struct drm_file *file,
> +			      unsigned int flags, unsigned int color,
> +			      struct drm_clip_rect *clips, unsigned int num_clips)
> +{
> +	struct drm_sysfb_device *sysfb = to_drm_sysfb_device(fb->dev);
> +	struct drm_rect r;
> +	bool queue = false;
> +
> +	drm_sysfb_damage_bounds(fb, flags, clips, num_clips, &r);
> +
> +	spin_lock(&sysfb->damage_lock);
> +	/* Pending damage of another framebuffer is copied first. */
> +	while (sysfb->damage_fb && sysfb->damage_fb != fb) {
> +		spin_unlock(&sysfb->damage_lock);
> +		flush_work(&sysfb->damage_work);
> +		spin_lock(&sysfb->damage_lock);
> +	}
> +	if (!sysfb->damage_fb) {
> +		drm_framebuffer_get(fb);
> +		sysfb->damage_fb = fb;
> +		sysfb->damage = r;
> +		queue = true;
> +	} else {
> +		sysfb->damage.x1 = min(sysfb->damage.x1, r.x1);
> +		sysfb->damage.y1 = min(sysfb->damage.y1, r.y1);
> +		sysfb->damage.x2 = max(sysfb->damage.x2, r.x2);
> +		sysfb->damage.y2 = max(sysfb->damage.y2, r.y2);
> +	}
> +	spin_unlock(&sysfb->damage_lock);
> +
> +	if (queue)
> +		queue_work(system_dfl_wq, &sysfb->damage_work);
> +	return 0;
> +}
> +
> +static const struct drm_framebuffer_funcs drm_sysfb_fb_funcs = {
> +	.destroy = drm_gem_fb_destroy,
> +	.create_handle = drm_gem_fb_create_handle,
> +	.dirty = drm_sysfb_fb_dirty,
> +};
> +
> +/**
> + * drm_sysfb_fb_create - &drm_mode_config_funcs.fb_create for sysfb devices
> + * @dev: the device
> + * @file: the DRM file of the caller
> + * @info: the pixel format
> + * @mode_cmd: the framebuffer from userspace
> + *
> + * Like drm_gem_fb_create_with_dirty(), but the framebuffer's damage is copied
> + * outside the DIRTYFB ioctl (drm_sysfb_damage_init()).
> + *
> + * Returns: the framebuffer, or an ERR_PTR() on failure.
> + */
> +struct drm_framebuffer *drm_sysfb_fb_create(struct drm_device *dev, struct drm_file *file,
> +					    const struct drm_format_info *info,
> +					    const struct drm_mode_fb_cmd2 *mode_cmd)
> +{
> +	return drm_gem_fb_create_with_funcs(dev, file, info, mode_cmd, &drm_sysfb_fb_funcs);
> +}
> +EXPORT_SYMBOL(drm_sysfb_fb_create);
> diff --git a/drivers/gpu/drm/sysfb/efidrm.c b/drivers/gpu/drm/sysfb/efidrm.c
> index 8b19f43..0ef093a 100644
> --- a/drivers/gpu/drm/sysfb/efidrm.c
> +++ b/drivers/gpu/drm/sysfb/efidrm.c
> @@ -299,6 +299,9 @@ static struct efidrm_device *efidrm_device_create(struct drm_driver *drv,
>   	 */
>   
>   	ret = drmm_mode_config_init(dev);
> +	if (ret)
> +		return ERR_PTR(ret);
> +	ret = drm_sysfb_damage_init(sysfb);
>   	if (ret)
>   		return ERR_PTR(ret);
>   
> diff --git a/drivers/gpu/drm/sysfb/ofdrm.c b/drivers/gpu/drm/sysfb/ofdrm.c
> index c412555..4d31bb6 100644
> --- a/drivers/gpu/drm/sysfb/ofdrm.c
> +++ b/drivers/gpu/drm/sysfb/ofdrm.c
> @@ -1021,6 +1021,9 @@ static struct ofdrm_device *ofdrm_device_create(struct drm_driver *drv,
>   	 */
>   
>   	ret = drmm_mode_config_init(dev);
> +	if (ret)
> +		return ERR_PTR(ret);
> +	ret = drm_sysfb_damage_init(sysfb);
>   	if (ret)
>   		return ERR_PTR(ret);
>   
> diff --git a/drivers/gpu/drm/sysfb/simpledrm.c b/drivers/gpu/drm/sysfb/simpledrm.c
> index deaeb7d..c40a6c5 100644
> --- a/drivers/gpu/drm/sysfb/simpledrm.c
> +++ b/drivers/gpu/drm/sysfb/simpledrm.c
> @@ -813,6 +813,9 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
>   	 */
>   
>   	ret = drmm_mode_config_init(dev);
> +	if (ret)
> +		return ERR_PTR(ret);
> +	ret = drm_sysfb_damage_init(sysfb);
>   	if (ret)
>   		return ERR_PTR(ret);
>   
> diff --git a/drivers/gpu/drm/sysfb/vesadrm.c b/drivers/gpu/drm/sysfb/vesadrm.c
> index 1668aab..812db4a 100644
> --- a/drivers/gpu/drm/sysfb/vesadrm.c
> +++ b/drivers/gpu/drm/sysfb/vesadrm.c
> @@ -539,6 +539,9 @@ static struct vesadrm_device *vesadrm_device_create(struct drm_driver *drv,
>   	 */
>   
>   	ret = drmm_mode_config_init(dev);
> +	if (ret)
> +		return ERR_PTR(ret);
> +	ret = drm_sysfb_damage_init(sysfb);
>   	if (ret)
>   		return ERR_PTR(ret);
>   
>
> ---
> base-commit: 41b5458595c60143d0ebaa1e032c9cda5d764dff
> change-id: 20261110-sysfb-dirtyfb-beda10fab602
>
> Best regards,
> --
> Joseph Telaak <joseph@telaak.dev>
>

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Stefan Gaiser, Jochen Jaser, Abhinav Puri, (HRB 36809, AG Nürnberg)

Re: [PATCH RFC] drm/sysfb: Copy damage outside the DIRTYFB ioctl

[Joseph Telaak]

Hi Thomas,

Thanks for the quick review, and for explaining why the blocking
commit is required. That makes sense, so I’ll drop this patch and look
at handling it on the X side if it becomes a real problem.

Best regards,
Joseph