[PATCH] drm/vkms: Keep composition enabled for concurrent users

[PATCH] drm/vkms: Keep composition enabled for concurrent users

[atipre]

CRC capture and writeback share composer_enabled. Cleaning up a writeback
clears this flag even when CRC capture is still active. The composer then
stops producing CRC samples while the CRC data file remains open. Cleanup
of a prepared but uncommitted writeback can also disable composition for
another user.

Replace the shared boolean with a user count protected by the output lock.
Hold one vblank reference for the combined lifetime of all composer users,
refunding redundant references when concurrent first users race. Track
whether each writeback job acquired a composer reference so its cleanup
only releases a reference it owns. Propagate CRC acquisition errors and
signal a failed writeback's own fence before cleaning up the unqueued job.
Read the user count under the output lock in the vblank callback and remove
the resolved documentation TODO.

A local IGT kms_writeback@writeback-preserves-crc regression times out
reading CRCs on the unmodified kernel and passes with this change on ARM64
QEMU with one and two CPUs. All four existing applicable writeback subtests
also pass; two unsupported XRGB2101010 subtests skip on both kernels.

Fixes: dbd9d80c1b2e ("drm/vkms: Add support for writeback")
Signed-off-by: atipre <tipreaditya@gmail.com>
---
IGT regression:
https://gist.github.com/atipre/36a712615c0300d5e09e11a3b5b83f29

Apply the linked patch to igt-gpu-tools, rebuild, then run:
  sudo IGT_FORCE_DRIVER=vkms ./build/tests/kms_writeback \
    --run-subtest writeback-preserves-crc

 Documentation/gpu/vkms.rst            |  5 ---
 drivers/gpu/drm/vkms/vkms_composer.c  | 47 ++++++++++++++++++++++-----
 drivers/gpu/drm/vkms/vkms_crtc.c      |  5 +--
 drivers/gpu/drm/vkms/vkms_drv.h       |  9 ++---
 drivers/gpu/drm/vkms/vkms_writeback.c | 21 ++++++++++--
 5 files changed, 64 insertions(+), 23 deletions(-)

diff --git a/Documentation/gpu/vkms.rst b/Documentation/gpu/vkms.rst
index 1e79e62a6..fe7dc8def 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 0fc915a95..5860e86e6 100644
--- a/drivers/gpu/drm/vkms/vkms_composer.c
+++ b/drivers/gpu/drm/vkms/vkms_composer.c
@@ -711,19 +711,44 @@ 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)
+int vkms_composer_get(struct vkms_output *out)
 {
-	bool old_enabled;
+	bool already_enabled;
+	int ret;
 
-	if (enabled)
-		drm_crtc_vblank_get(&out->crtc);
+	spin_lock_irq(&out->lock);
+	if (out->composer_users) {
+		out->composer_users++;
+		spin_unlock_irq(&out->lock);
+		return 0;
+	}
+	spin_unlock_irq(&out->lock);
+
+	ret = drm_crtc_vblank_get(&out->crtc);
+	if (ret)
+		return ret;
 
 	spin_lock_irq(&out->lock);
-	old_enabled = out->composer_enabled;
-	out->composer_enabled = enabled;
+	already_enabled = out->composer_users != 0;
+	out->composer_users++;
 	spin_unlock_irq(&out->lock);
 
-	if (old_enabled)
+	/* Another user may have enabled composition while we got vblank. */
+	if (already_enabled)
+		drm_crtc_vblank_put(&out->crtc);
+
+	return 0;
+}
+
+void vkms_composer_put(struct vkms_output *out)
+{
+	bool last_user;
+
+	spin_lock_irq(&out->lock);
+	last_user = --out->composer_users == 0;
+	spin_unlock_irq(&out->lock);
+
+	if (last_user)
 		drm_crtc_vblank_put(&out->crtc);
 }
 
@@ -734,8 +759,12 @@ int vkms_set_crc_source(struct drm_crtc *crtc, const char *src_name)
 	int ret = 0;
 
 	ret = vkms_crc_parse_source(src_name, &enabled);
+	if (ret)
+		return ret;
 
-	vkms_set_composer(out, enabled);
+	if (enabled)
+		return vkms_composer_get(out);
 
-	return ret;
+	vkms_composer_put(out);
+	return 0;
 }
diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
index fe17f4930..69ec6b14a 100644
--- a/drivers/gpu/drm/vkms/vkms_crtc.c
+++ b/drivers/gpu/drm/vkms/vkms_crtc.c
@@ -17,7 +17,7 @@ static bool vkms_crtc_handle_vblank_timeout(struct drm_crtc *crtc)
 {
 	struct vkms_output *output = drm_crtc_to_vkms_output(crtc);
 	struct vkms_crtc_state *state;
-	bool ret, fence_cookie;
+	bool ret, fence_cookie, composer_enabled;
 
 	fence_cookie = dma_fence_begin_signalling();
 
@@ -27,9 +27,10 @@ static bool vkms_crtc_handle_vblank_timeout(struct drm_crtc *crtc)
 		DRM_ERROR("vkms failure on handling vblank");
 
 	state = output->composer_state;
+	composer_enabled = output->composer_users != 0;
 	spin_unlock(&output->lock);
 
-	if (state && output->composer_enabled) {
+	if (state && composer_enabled) {
 		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 381483aa4..4919d8036 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.h
+++ b/drivers/gpu/drm/vkms/vkms_drv.h
@@ -89,6 +89,7 @@ struct vkms_writeback_job {
 	struct iosys_map data[DRM_FORMAT_MAX_PLANES];
 	struct vkms_frame_info wb_frame_info;
 	pixel_write_t pixel_write;
+	bool composer_ref;
 };
 
 /**
@@ -210,8 +211,7 @@ 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: Protected by @lock, number of CRC capture and writeback users
  * @composer_state: Protected by @lock, current state of this VKMS output
  * @composer_lock: Lock used internally to protect @composer_state members
  */
@@ -222,7 +222,7 @@ struct vkms_output {
 	struct workqueue_struct *composer_workq;
 	spinlock_t lock;
 
-	bool composer_enabled;
+	unsigned int composer_users;
 	struct vkms_crtc_state *composer_state;
 
 	spinlock_t composer_lock;
@@ -317,7 +317,8 @@ 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);
+int vkms_composer_get(struct vkms_output *out);
+void vkms_composer_put(struct vkms_output *out);
 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 ecf29a2c0..584a71862 100644
--- a/drivers/gpu/drm/vkms/vkms_writeback.c
+++ b/drivers/gpu/drm/vkms/vkms_writeback.c
@@ -1,5 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0+
 
+#include <linux/dma-fence.h>
 #include <linux/iosys-map.h>
 
 #include <drm/drm_atomic.h>
@@ -118,7 +119,9 @@ 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);
+	/* Prepared jobs can be cleaned up without ever being committed. */
+	if (vkmsjob->composer_ref)
+		vkms_composer_put(vkms_output);
 	kfree(vkmsjob);
 }
 
@@ -137,13 +140,25 @@ static void vkms_wb_atomic_commit(struct drm_connector *conn,
 	struct vkms_writeback_job *active_wb;
 	struct vkms_frame_info *wb_frame_info;
 	u32 wb_format = fb->format->format;
+	int ret;
 
 	if (!conn_state)
 		return;
 
-	vkms_set_composer(output, true);
-
 	active_wb = conn_state->writeback_job->priv;
+	ret = vkms_composer_get(output);
+	if (ret) {
+		struct drm_writeback_job *job = conn_state->writeback_job;
+
+		conn_state->writeback_job = NULL;
+		if (job->out_fence) {
+			dma_fence_set_error(job->out_fence, ret);
+			dma_fence_signal(job->out_fence);
+		}
+		drm_writeback_cleanup_job(job);
+		return;
+	}
+	active_wb->composer_ref = true;
 	wb_frame_info = &active_wb->wb_frame_info;
 
 	spin_lock_irq(&output->composer_lock);

base-commit: 21a7f44fd0765fcc5194130b28767786eba1d1e7
-- 
2.50.1 (Apple Git-155)