[PATCH 09/10] drm/vkms: totally reworked crc data tracking

[Daniel Vetter <daniel.vetter@ffwll.ch>]

The crc computation worker needs to be able to get at some data
structures and framebuffer mappings, while potentially more atomic
updates are going on. The solution thus far is to copy relevant bits
around, but that's very tedious.

Here's a new approach, which tries to be more clever, but relies on a
few not-so-obvious things:
- crtc_state is always updated when a plane_state changes. Therefore
  we can just stuff plane_state pointers into a crtc_state. That
  solves the problem of easily getting at the needed plane_states.
- with the flushing changes from previous patches the above also holds
  without races due to the next atomic update being a bit eager with
  cleaning up pending work - we always wait for all crc work items to
  complete before unmapping framebuffers.
- we also need to make sure that the hrtimer fires off the right
  worker. Keep a new distinct crc_state pointer, under the
  vkms_output->lock protection for this. Note that crtc->state is
  updated very early in the atomic commit, way before we arm the
  vblank event - the vblank event should always match the buffers we
  use to compute the crc. This also solves an issue in the hrtimer,
  where we've accessed drm_crtc->state without holding the right locks
  (we held none - oops).
- in the worker itself we can then just access the plane states we
  need, again solving a bunch of ordering and locking issues.
  Accessing plane->state requires locks, accessing the private
  vkms_crtc_state->active_planes pointer only requires that the memory
  doesn't get freed too early.

The idea behind vkms_crtc_state->active_planes is that this would
contain all visible planes, in z-order, as a first step towards a more
generic blending implementation.

Note that this patch also fixes races between prepare_fb/cleanup_fb
and the crc worker accessing ->vaddr.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
Cc: Haneen Mohammed <hamohammed.sa@gmail.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
---
 drivers/gpu/drm/vkms/vkms_crc.c  | 21 +++--------
 drivers/gpu/drm/vkms/vkms_crtc.c | 60 +++++++++++++++++++++++++++++---
 drivers/gpu/drm/vkms/vkms_drv.h  |  9 ++++-
 3 files changed, 67 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/vkms/vkms_crc.c b/drivers/gpu/drm/vkms/vkms_crc.c
index 9d15e5e85830..0d31cfc32042 100644
--- a/drivers/gpu/drm/vkms/vkms_crc.c
+++ b/drivers/gpu/drm/vkms/vkms_crc.c
@@ -159,11 +159,8 @@ void vkms_crc_work_handle(struct work_struct *work)
 						crc_work);
 	struct drm_crtc *crtc = crtc_state->base.crtc;
 	struct vkms_output *out = drm_crtc_to_vkms_output(crtc);
-	struct vkms_device *vdev = container_of(out, struct vkms_device,
-						output);
 	struct vkms_crc_data *primary_crc = NULL;
 	struct vkms_crc_data *cursor_crc = NULL;
-	struct drm_plane *plane;
 	u32 crc32 = 0;
 	u64 frame_start, frame_end;
 	bool crc_pending;
@@ -184,21 +181,11 @@ void vkms_crc_work_handle(struct work_struct *work)
 	if (!crc_pending)
 		return;
 
-	drm_for_each_plane(plane, &vdev->drm) {
-		struct vkms_plane_state *vplane_state;
-		struct vkms_crc_data *crc_data;
+	if (crtc_state->num_active_planes >= 1)
+		primary_crc = crtc_state->active_planes[0]->crc_data;
 
-		vplane_state = to_vkms_plane_state(plane->state);
-		crc_data = vplane_state->crc_data;
-
-		if (drm_framebuffer_read_refcount(&crc_data->fb) == 0)
-			continue;
-
-		if (plane->type == DRM_PLANE_TYPE_PRIMARY)
-			primary_crc = crc_data;
-		else
-			cursor_crc = crc_data;
-	}
+	if (crtc_state->num_active_planes == 2)
+		cursor_crc = crtc_state->active_planes[1]->crc_data;
 
 	if (primary_crc)
 		crc32 = _vkms_get_crc(primary_crc, cursor_crc);
diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
index 48a793ba4030..14156ed70415 100644
--- a/drivers/gpu/drm/vkms/vkms_crtc.c
+++ b/drivers/gpu/drm/vkms/vkms_crtc.c
@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0+
 
 #include "vkms_drv.h"
+#include <drm/drm_atomic.h>
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_probe_helper.h>
 
@@ -9,7 +10,7 @@ static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
 	struct vkms_output *output = container_of(timer, struct vkms_output,
 						  vblank_hrtimer);
 	struct drm_crtc *crtc = &output->crtc;
-	struct vkms_crtc_state *state = to_vkms_crtc_state(crtc->state);
+	struct vkms_crtc_state *state;
 	u64 ret_overrun;
 	bool ret;
 
@@ -23,6 +24,7 @@ static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
 	if (!ret)
 		DRM_ERROR("vkms failure on handling vblank");
 
+	state = output->crc_state;
 	if (state && output->crc_enabled) {
 		u64 frame = drm_crtc_accurate_vblank_count(crtc);
 
@@ -124,10 +126,9 @@ static void vkms_atomic_crtc_destroy_state(struct drm_crtc *crtc,
 
 	__drm_atomic_helper_crtc_destroy_state(state);
 
-	if (vkms_state) {
-		WARN_ON(work_pending(&vkms_state->crc_work));
-		kfree(vkms_state);
-	}
+	WARN_ON(work_pending(&vkms_state->crc_work));
+	kfree(vkms_state->active_planes);
+	kfree(vkms_state);
 }
 
 static void vkms_atomic_crtc_reset(struct drm_crtc *crtc)
@@ -156,6 +157,52 @@ static const struct drm_crtc_funcs vkms_crtc_funcs = {
 	.verify_crc_source	= vkms_verify_crc_source,
 };
 
+static int vkms_crtc_atomic_check(struct drm_crtc *crtc,
+				   struct drm_crtc_state *state)
+{
+	struct vkms_crtc_state *vkms_state = to_vkms_crtc_state(state);
+	struct drm_plane *plane;
+	struct drm_plane_state *plane_state;
+	int i = 0, ret;
+
+	if (vkms_state->active_planes)
+		return 0;
+
+	ret = drm_atomic_add_affected_planes(state->state, crtc);
+	if (ret < 0)
+		return ret;
+
+	drm_for_each_plane_mask(plane, crtc->dev, state->plane_mask) {
+		plane_state = drm_atomic_get_existing_plane_state(state->state,
+								  plane);
+		WARN_ON(!plane_state);
+
+		if (!plane_state->visible)
+			continue;
+
+		i++;
+	}
+
+	vkms_state->active_planes = kcalloc(i, sizeof(plane), GFP_KERNEL);
+	if (!vkms_state->active_planes)
+		return -ENOMEM;
+	vkms_state->num_active_planes = i;
+
+	i = 0;
+	drm_for_each_plane_mask(plane, crtc->dev, state->plane_mask) {
+		plane_state = drm_atomic_get_existing_plane_state(state->state,
+								  plane);
+
+		if (!plane_state->visible)
+			continue;
+
+		vkms_state->active_planes[i++] =
+			to_vkms_plane_state(plane_state);
+	}
+
+	return 0;
+}
+
 static void vkms_crtc_atomic_enable(struct drm_crtc *crtc,
 				    struct drm_crtc_state *old_state)
 {
@@ -197,10 +244,13 @@ static void vkms_crtc_atomic_flush(struct drm_crtc *crtc,
 		crtc->state->event = NULL;
 	}
 
+	vkms_output->crc_state = to_vkms_crtc_state(crtc->state);
+
 	spin_unlock_irq(&vkms_output->lock);
 }
 
 static const struct drm_crtc_helper_funcs vkms_crtc_helper_funcs = {
+	.atomic_check	= vkms_crtc_atomic_check,
 	.atomic_begin	= vkms_crtc_atomic_begin,
 	.atomic_flush	= vkms_crtc_atomic_flush,
 	.atomic_enable	= vkms_crtc_atomic_enable,
diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
index 2a35299bfb89..4e7450111d45 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.h
+++ b/drivers/gpu/drm/vkms/vkms_drv.h
@@ -49,6 +49,10 @@ struct vkms_crtc_state {
 	struct drm_crtc_state base;
 	struct work_struct crc_work;
 
+	int num_active_planes;
+	/* stack of active planes for crc computation, should be in z order */
+	struct vkms_plane_state **active_planes;
+
 	/* below three are protected by vkms_output.crc_lock */
 	bool crc_pending;
 	u64 frame_start;
@@ -62,12 +66,15 @@ struct vkms_output {
 	struct hrtimer vblank_hrtimer;
 	ktime_t period_ns;
 	struct drm_pending_vblank_event *event;
-	bool crc_enabled;
 	/* ordered wq for crc_work */
 	struct workqueue_struct *crc_workq;
 	/* protects concurrent access to crc_data */
 	spinlock_t lock;
 
+	/* protected by @lock */
+	bool crc_enabled;
+	struct vkms_crtc_state *crc_state;
+
 	spinlock_t crc_lock;
 };
 
-- 
2.20.1

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