All of lore.kernel.org
 help / color / mirror / Atom feed
From: Daniel Vetter <daniel.vetter@ffwll.ch>
To: DRI Development <dri-devel@lists.freedesktop.org>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>,
	Haneen Mohammed <hamohammed.sa@gmail.com>,
	Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>,
	Daniel Vetter <daniel.vetter@intel.com>
Subject: [PATCH 09/10] drm/vkms: totally reworked crc data tracking
Date: Fri,  7 Jun 2019 00:27:50 +0200	[thread overview]
Message-ID: <20190606222751.32567-10-daniel.vetter@ffwll.ch> (raw)
In-Reply-To: <20190606222751.32567-1-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

  parent reply	other threads:[~2019-06-06 22:28 UTC|newest]

Thread overview: 44+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-06-06 22:27 [PATCH 00/10] drm/vkms: rework crc worker Daniel Vetter
2019-06-06 22:27 ` [PATCH 01/10] drm/vkms: Fix crc worker races Daniel Vetter
2019-06-12 13:33   ` Rodrigo Siqueira
2019-06-12 14:48     ` Daniel Vetter
2019-06-18  2:39       ` Rodrigo Siqueira
2019-06-18  8:49         ` Daniel Vetter
2019-06-06 22:27 ` [PATCH 02/10] drm/vkms: Use spin_lock_irq in process context Daniel Vetter
2019-06-12 13:34   ` Rodrigo Siqueira
2019-06-12 14:54     ` Daniel Vetter
2019-06-06 22:27 ` [PATCH 03/10] drm/vkms: Rename vkms_output.state_lock to crc_lock Daniel Vetter
2019-06-12 13:38   ` Rodrigo Siqueira
2019-06-13  7:48     ` Daniel Vetter
2019-06-06 22:27 ` [PATCH 04/10] drm/vkms: Move format arrays to vkms_plane.c Daniel Vetter
2019-06-12 13:39   ` Rodrigo Siqueira
2019-06-19  2:12   ` Rodrigo Siqueira
2019-06-06 22:27 ` [PATCH 05/10] drm/vkms: Add our own commit_tail Daniel Vetter
2019-06-06 22:27 ` [PATCH 06/10] drm/vkms: flush crc workers earlier in commit flow Daniel Vetter
2019-06-12 13:42   ` Rodrigo Siqueira
2019-06-13  7:53     ` Daniel Vetter
2019-06-13  7:55       ` Daniel Vetter
2019-06-18  2:31       ` Rodrigo Siqueira
2019-06-06 22:27 ` [PATCH 07/10] drm/vkms: Dont flush crc worker when we change crc status Daniel Vetter
2019-06-19  2:17   ` Rodrigo Siqueira
2019-06-19  7:47     ` Daniel Vetter
2019-06-06 22:27 ` [PATCH 08/10] drm/vkms: No _irqsave within spin_lock_irq needed Daniel Vetter
2019-06-12 13:43   ` Rodrigo Siqueira
2019-06-06 22:27 ` Daniel Vetter [this message]
2019-06-12 13:46   ` [PATCH 09/10] drm/vkms: totally reworked crc data tracking Rodrigo Siqueira
2019-06-13  7:59     ` Daniel Vetter
2019-06-06 22:27 ` [PATCH 10/10] drm/vkms: No need for ->pages_lock in crc work anymore Daniel Vetter
2019-06-12 13:47   ` Rodrigo Siqueira
2019-06-12 13:28 ` [PATCH 00/10] drm/vkms: rework crc worker Rodrigo Siqueira
2019-06-12 14:42   ` Daniel Vetter
2019-06-18  2:49     ` Rodrigo Siqueira
2019-06-18  8:56       ` Daniel Vetter
2019-06-18 21:54         ` Rodrigo Siqueira
2019-06-18 22:06           ` Daniel Vetter
2019-06-18 22:07             ` Daniel Vetter
2019-06-18 22:25               ` Rodrigo Siqueira
2019-06-18 22:39                 ` Daniel Vetter
2019-06-26  1:44             ` Rodrigo Siqueira
2019-06-26  7:54               ` Daniel Vetter
2019-06-26 13:46                 ` Rodrigo Siqueira
2019-07-01  3:30                 ` Rodrigo Siqueira

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190606222751.32567-10-daniel.vetter@ffwll.ch \
    --to=daniel.vetter@ffwll.ch \
    --cc=daniel.vetter@intel.com \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=hamohammed.sa@gmail.com \
    --cc=rodrigosiqueiramelo@gmail.com \
    --subject='Re: [PATCH 09/10] drm/vkms: totally reworked crc data tracking' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.