[PATCH] drm/vkms: use a bitmask instead of a bool for composer users

[PATCH] drm/vkms: use a bitmask instead of a bool for composer users

[Oarora Etimis]

The composer has a single on/off switch, shared by the CRC reader and the
writeback job, and no record of who needs it. Either of them turns it off
for both:

  - with CRC being captured, cleaning up a writeback job stops the CRC
    entries;
  - with a writeback job pending, closing the CRC source leaves the frame
    uncomposed and its out-fence never signalled.

Replace the switch with a bitmask holding one bit per user, so a user only
clears its own bit and the composer stays active while any bit is set. The
vblank reference follows the bitmask: taken when it becomes non-zero and
dropped when it becomes zero again. vkms_set_composer() becomes
vkms_composer_get() and vkms_composer_put().

Fixes: dbd9d80c1b2e ("drm/vkms: Add support for writeback")

Signed-off-by: Oarora Etimis <oaroraetimis@gmail.com>
---
- Why not use a refcount? Because there are only a few users, and a bit
  per user shows the working state of each one. A repeated put() from the
  same user does nothing, and it also helps future work that makes the
  vblank timeout synchronous.

- Tested with vng: read crc/data and submitted three writeback jobs with a
  small libdrm helper. The entries kept coming at the full rate of the
  1024x768@60 mode, 235 to 237 of them over the 3.97 s the three jobs
  took, where the current code stops at five.
---
 Documentation/gpu/vkms.rst            |  5 -----
 drivers/gpu/drm/vkms/vkms_composer.c  | 34 ++++++++++++++++++++++++++--------
 drivers/gpu/drm/vkms/vkms_crtc.c      |  2 +-
 drivers/gpu/drm/vkms/vkms_drv.h       | 27 +++++++++++++++++++++++----
 drivers/gpu/drm/vkms/vkms_writeback.c |  4 ++--
 5 files changed, 52 insertions(+), 20 deletions(-)

diff --git a/Documentation/gpu/vkms.rst b/Documentation/gpu/vkms.rst
index 1e79e62a6bc4..fe7dc8def44a 100644
--- a/Documentation/gpu/vkms.rst
+++ b/Documentation/gpu/vkms.rst
@@ -246,11 +246,6 @@ module through configfs. Use/Test-cases:
 Writeback support
 -----------------
 
-- The writeback and CRC capture operations share the use of composer_enabled
-  boolean to ensure vblanks. Probably, when these operations work together,
-  composer_enabled needs to refcounting the composer state to proper work.
-  [Good to get started]
-
 - Add support for cloned writeback outputs and related test cases using a
   cloned output in the IGT kms_writeback.
 
diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
index 0fc915a954ba..59f1dd50b5bd 100644
--- a/drivers/gpu/drm/vkms/vkms_composer.c
+++ b/drivers/gpu/drm/vkms/vkms_composer.c
@@ -711,19 +711,34 @@ int vkms_verify_crc_source(struct drm_crtc *crtc, const char *src_name,
 	return 0;
 }
 
-void vkms_set_composer(struct vkms_output *out, bool enabled)
+void vkms_composer_get(struct vkms_output *out, u8 user)
 {
-	bool old_enabled;
+	bool was_enabled;
 
-	if (enabled)
-		drm_crtc_vblank_get(&out->crtc);
+	if (drm_crtc_vblank_get(&out->crtc))
+		return;
+
+	spin_lock_irq(&out->lock);
+	was_enabled = out->composer_users;
+	out->composer_users |= user;
+	spin_unlock_irq(&out->lock);
+
+	/* Another user already holds the reference for the composer. */
+	if (was_enabled)
+		drm_crtc_vblank_put(&out->crtc);
+}
+
+void vkms_composer_put(struct vkms_output *out, u8 user)
+{
+	bool was_set, enabled;
 
 	spin_lock_irq(&out->lock);
-	old_enabled = out->composer_enabled;
-	out->composer_enabled = enabled;
+	was_set = out->composer_users & user;
+	out->composer_users &= ~user;
+	enabled = out->composer_users;
 	spin_unlock_irq(&out->lock);
 
-	if (old_enabled)
+	if (was_set && !enabled)
 		drm_crtc_vblank_put(&out->crtc);
 }
 
@@ -735,7 +750,10 @@ int vkms_set_crc_source(struct drm_crtc *crtc, const char *src_name)
 
 	ret = vkms_crc_parse_source(src_name, &enabled);
 
-	vkms_set_composer(out, enabled);
+	if (enabled)
+		vkms_composer_get(out, VKMS_COMPOSER_CRC);
+	else
+		vkms_composer_put(out, VKMS_COMPOSER_CRC);
 
 	return ret;
 }
diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
index fe17f49305a9..4e15abfd92a0 100644
--- a/drivers/gpu/drm/vkms/vkms_crtc.c
+++ b/drivers/gpu/drm/vkms/vkms_crtc.c
@@ -29,7 +29,7 @@ static bool vkms_crtc_handle_vblank_timeout(struct drm_crtc *crtc)
 	state = output->composer_state;
 	spin_unlock(&output->lock);
 
-	if (state && output->composer_enabled) {
+	if (state && READ_ONCE(output->composer_users)) {
 		u64 frame = drm_crtc_accurate_vblank_count(crtc);
 
 		/* update frame_start only if a queued vkms_composer_worker()
diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
index dea6cad3f1b9..c3bb3cd46863 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.h
+++ b/drivers/gpu/drm/vkms/vkms_drv.h
@@ -27,6 +27,9 @@
 
 #define VKMS_LUT_SIZE 256
 
+#define VKMS_COMPOSER_CRC  (1U << 0)
+#define VKMS_COMPOSER_WB   (1U << 1)
+
 /**
  * struct vkms_frame_info - Structure to store the state of a frame
  *
@@ -210,8 +213,9 @@ struct vkms_crtc_state {
  *	       vblank timestamps
  * @composer_workq: Ordered workqueue for @composer_state.composer_work.
  * @lock: Lock used to protect concurrent access to the composer
- * @composer_enabled: Protected by @lock, true when the VKMS composer is active (crc needed or
- *		      writeback)
+ * @composer_users: Bitmask of the users that need the VKMS composer; the composer is only
+ *		    active when it is non-zero. Updated under @lock, and read with
+ *		    READ_ONCE() from the vblank timeout.
  * @composer_state: Protected by @lock, current state of this VKMS output
  * @composer_lock: Lock used internally to protect @composer_state members
  */
@@ -222,7 +226,7 @@ struct vkms_output {
 	struct workqueue_struct *composer_workq;
 	spinlock_t lock;
 
-	bool composer_enabled;
+	u8 composer_users;
 	struct vkms_crtc_state *composer_state;
 
 	spinlock_t composer_lock;
@@ -317,7 +321,22 @@ int vkms_verify_crc_source(struct drm_crtc *crtc, const char *source_name,
 
 /* Composer Support */
 void vkms_composer_worker(struct work_struct *work);
-void vkms_set_composer(struct vkms_output *out, bool enabled);
+
+/**
+ * vkms_composer_get() - Enable the composer for a user
+ * @out: VKMS output
+ * @user: bit of the user that needs the composer (VKMS_COMPOSER_CRC or
+ *	  VKMS_COMPOSER_WB)
+ */
+void vkms_composer_get(struct vkms_output *out, u8 user);
+
+/**
+ * vkms_composer_put() - Disable the composer for a user
+ * @out: VKMS output
+ * @user: bit of the user that no longer needs the composer
+ */
+void vkms_composer_put(struct vkms_output *out, u8 user);
+
 void vkms_writeback_row(struct vkms_writeback_job *wb, const struct line_buffer *src_buffer, int y);
 
 /* Writeback */
diff --git a/drivers/gpu/drm/vkms/vkms_writeback.c b/drivers/gpu/drm/vkms/vkms_writeback.c
index 858df1f89f66..7e2f82e44b39 100644
--- a/drivers/gpu/drm/vkms/vkms_writeback.c
+++ b/drivers/gpu/drm/vkms/vkms_writeback.c
@@ -118,7 +118,7 @@ static void vkms_wb_cleanup_job(struct drm_writeback_connector *connector,
 
 	drm_framebuffer_put(vkmsjob->wb_frame_info.fb);
 
-	vkms_set_composer(vkms_output, false);
+	vkms_composer_put(vkms_output, VKMS_COMPOSER_WB);
 	kfree(vkmsjob);
 }
 
@@ -141,7 +141,7 @@ static void vkms_wb_atomic_commit(struct drm_connector *conn,
 	if (!conn_state)
 		return;
 
-	vkms_set_composer(output, true);
+	vkms_composer_get(output, VKMS_COMPOSER_WB);
 
 	active_wb = conn_state->writeback_job->priv;
 	wb_frame_info = &active_wb->wb_frame_info;

---
base-commit: 1116f4aee916ca13d3e40ce87e67835f51576fcb
change-id: 20261111-composer-cc9620515bcd

Best regards,
--  
Oarora Etimis <oaroraetimis@gmail.com>