[PATCH 0/3] drm: commit_work scheduling

[PATCH 0/3] drm: commit_work scheduling

[Rob Clark]

From: Rob Clark <robdclark@chromium.org>

The android userspace treats the display pipeline as a realtime problem.
And arguably, if your goal is to not miss frame deadlines (ie. vblank),
it is.  (See https://lwn.net/Articles/809545/ for the best explaination
that I found.)

But this presents a problem with using workqueues for non-blocking
atomic commit_work(), because the SCHED_FIFO userspace thread(s) can
preempt the worker.  Which is not really the outcome you want.. once
the required fences are scheduled, you want to push the atomic commit
down to hw ASAP.

But the decision of whether commit_work should be RT or not really
depends on what userspace is doing.  For a pure CFS userspace display
pipeline, commit_work() should remain SCHED_NORMAL.

To handle this, convert non-blocking commit_work() to use per-CRTC
kthread workers, instead of system_unbound_wq.  Per-CRTC workers are
used to avoid serializing commits when userspace is using a per-CRTC
update loop.

A client-cap is introduced so that userspace can opt-in to SCHED_FIFO
priority commit work.

A potential issue is that since 616d91b68cd ("sched: Remove
sched_setscheduler*() EXPORTs") we have limited RT priority levels,
meaning that commit_work() ends up running at the same priority level
as vblank-work.  This shouldn't be a big problem *yet*, due to limited
use of vblank-work at this point.  And if it could be arranged that
vblank-work is scheduled before signaling out-fences and/or sending
pageflip events, it could probably work ok to use a single priority
level for both commit-work and vblank-work.

Rob Clark (3):
  drm/crtc: Introduce per-crtc kworker
  drm/atomic: Use kthread worker for nonblocking commits
  drm: Add a client-cap to set scheduling mode

 drivers/gpu/drm/drm_atomic_helper.c | 13 ++++++----
 drivers/gpu/drm/drm_auth.c          |  4 ++++
 drivers/gpu/drm/drm_crtc.c          | 37 +++++++++++++++++++++++++++++
 drivers/gpu/drm/drm_ioctl.c         | 13 ++++++++++
 include/drm/drm_atomic.h            | 31 ++++++++++++++++++++++++
 include/drm/drm_crtc.h              | 10 ++++++++
 include/uapi/drm/drm.h              | 13 ++++++++++
 7 files changed, 117 insertions(+), 4 deletions(-)

-- 
2.26.2

[PATCH 1/3] drm/crtc: Introduce per-crtc kworker

[Rob Clark]

From: Rob Clark <robdclark@chromium.org>

This will be used for non-block atomic commits.

Signed-off-by: Rob Clark <robdclark@chromium.org>
---
 drivers/gpu/drm/drm_crtc.c | 11 +++++++++++
 include/drm/drm_crtc.h     |  8 ++++++++
 2 files changed, 19 insertions(+)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index aecdd7ea26dc..4f7c0bfce0a3 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -326,6 +326,14 @@ int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc,
 					   config->prop_out_fence_ptr, 0);
 		drm_object_attach_property(&crtc->base,
 					   config->prop_vrr_enabled, 0);
+
+		crtc->worker = kthread_create_worker(0, "%s-worker", crtc->name);
+		if (IS_ERR(crtc->worker)) {
+			drm_mode_object_unregister(dev, &crtc->base);
+			ret = PTR_ERR(crtc->worker);
+			crtc->worker = NULL;
+			return ret;
+		}
 	}
 
 	return 0;
@@ -366,6 +374,9 @@ void drm_crtc_cleanup(struct drm_crtc *crtc)
 
 	kfree(crtc->name);
 
+	if (crtc->worker)
+		kthread_destroy_worker(crtc->worker);
+
 	memset(crtc, 0, sizeof(*crtc));
 }
 EXPORT_SYMBOL(drm_crtc_cleanup);
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 59b51a09cae6..8964a3732bca 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -30,6 +30,7 @@
 #include <linux/types.h>
 #include <linux/fb.h>
 #include <linux/hdmi.h>
+#include <linux/kthread.h>
 #include <linux/media-bus-format.h>
 #include <uapi/drm/drm_mode.h>
 #include <uapi/drm/drm_fourcc.h>
@@ -1172,6 +1173,13 @@ struct drm_crtc {
 	 * Initialized via drm_self_refresh_helper_init().
 	 */
 	struct drm_self_refresh_data *self_refresh_data;
+
+	/**
+	 * worker:
+	 *
+	 * Per-CRTC worker for nonblock atomic commits.
+	 */
+	struct kthread_worker *worker;
 };
 
 /**
-- 
2.26.2

[PATCH 2/3] drm/atomic: Use kthread worker for nonblocking commits

[Rob Clark]

From: Rob Clark <robdclark@chromium.org>

This will allow us to more easily switch scheduling rules based on what
userspace wants.

Signed-off-by: Rob Clark <robdclark@chromium.org>
---
 drivers/gpu/drm/drm_atomic_helper.c | 13 ++++++++----
 include/drm/drm_atomic.h            | 31 +++++++++++++++++++++++++++++
 2 files changed, 40 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 9e1ad493e689..75eeec5e7b10 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1659,11 +1659,11 @@ static void commit_tail(struct drm_atomic_state *old_state)
 	drm_atomic_state_put(old_state);
 }
 
-static void commit_work(struct work_struct *work)
+static void commit_work(struct kthread_work *work)
 {
 	struct drm_atomic_state *state = container_of(work,
 						      struct drm_atomic_state,
-						      commit_work);
+						      commit_kwork);
 	commit_tail(state);
 }
 
@@ -1797,6 +1797,7 @@ int drm_atomic_helper_commit(struct drm_device *dev,
 			     struct drm_atomic_state *state,
 			     bool nonblock)
 {
+	struct kthread_worker *worker = NULL;
 	int ret;
 
 	if (state->async_update) {
@@ -1814,7 +1815,7 @@ int drm_atomic_helper_commit(struct drm_device *dev,
 	if (ret)
 		return ret;
 
-	INIT_WORK(&state->commit_work, commit_work);
+	kthread_init_work(&state->commit_kwork, commit_work);
 
 	ret = drm_atomic_helper_prepare_planes(dev, state);
 	if (ret)
@@ -1857,8 +1858,12 @@ int drm_atomic_helper_commit(struct drm_device *dev,
 	 */
 
 	drm_atomic_state_get(state);
+
 	if (nonblock)
-		queue_work(system_unbound_wq, &state->commit_work);
+		worker = drm_atomic_pick_worker(state);
+
+	if (worker)
+		kthread_queue_work(worker, &state->commit_kwork);
 	else
 		commit_tail(state);
 
diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
index d07c851d255b..8d0ee19953df 100644
--- a/include/drm/drm_atomic.h
+++ b/include/drm/drm_atomic.h
@@ -373,8 +373,18 @@ struct drm_atomic_state {
 	 *
 	 * Work item which can be used by the driver or helpers to execute the
 	 * commit without blocking.
+	 *
+	 * This is deprecated, use commit_kwork.
 	 */
 	struct work_struct commit_work;
+
+	/**
+	 * @commit_kwork:
+	 *
+	 * Work item which can be used by the driver or helpers to execute the
+	 * commit without blocking.
+	 */
+	struct kthread_work commit_kwork;
 };
 
 void __drm_crtc_commit_free(struct kref *kref);
@@ -954,6 +964,27 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p);
 		      (new_obj_state) = (__state)->private_objs[__i].new_state, 1); \
 	     (__i)++)
 
+/**
+ * drm_atomic_pick_worker - helper to get kworker to use for nonblocking commit
+ * @state: the &drm_atomic_state for the commit
+ *
+ * Pick an appropriate worker for a given atomic update.  The first CRTC
+ * invovled in the atomic update is used to pick the worker, to prevent
+ * serializing multiple pageflips / atomic-updates on indenpendent CRTCs.
+ */
+static inline struct kthread_worker *
+drm_atomic_pick_worker(const struct drm_atomic_state *state)
+{
+	struct drm_crtc_state *crtc_state;
+	struct drm_crtc *crtc;
+	unsigned i;
+
+	for_each_new_crtc_in_state(state, crtc, crtc_state, i)
+		return crtc->worker;
+
+	return NULL;
+}
+
 /**
  * drm_atomic_crtc_needs_modeset - compute combined modeset need
  * @state: &drm_crtc_state for the CRTC
-- 
2.26.2

[PATCH 3/3] drm: Add a client-cap to set scheduling mode

[Rob Clark]

From: Rob Clark <robdclark@chromium.org>

Add DRM_CLIENT_CAP_SCHED_MODE so that userspace can control the
scheduling mode for nonblocking atomic commits.  Userspace such as
android, which treats the display pipeline as realtime (SCHED_FIFO)
should set DRM_CLIENT_CAP_SCHED_FIFO to prevent userspace components
of the display pipeline (like surfaceflinger) from preempting atomic
commit_work.

This cap may only be set by the drm master, after setting the
DRM_CLIENT_CAP_ATOMIC cap.  The scheduling mode is returned to default
(SCHED_NORMAL) after master is dropped.

Signed-off-by: Rob Clark <robdclark@chromium.org>
---
 drivers/gpu/drm/drm_auth.c  |  4 ++++
 drivers/gpu/drm/drm_crtc.c  | 26 ++++++++++++++++++++++++++
 drivers/gpu/drm/drm_ioctl.c | 13 +++++++++++++
 include/drm/drm_crtc.h      |  2 ++
 include/uapi/drm/drm.h      | 13 +++++++++++++
 5 files changed, 58 insertions(+)

diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c
index f2d46b7ac6f9..217f422389f9 100644
--- a/drivers/gpu/drm/drm_auth.c
+++ b/drivers/gpu/drm/drm_auth.c
@@ -31,6 +31,7 @@
 #include <linux/slab.h>
 
 #include <drm/drm_auth.h>
+#include <drm/drm_crtc.h>
 #include <drm/drm_drv.h>
 #include <drm/drm_file.h>
 #include <drm/drm_lease.h>
@@ -335,6 +336,9 @@ void drm_master_release(struct drm_file *file_priv)
 		drm_lease_revoke(master);
 	}
 
+	if (drm_core_check_feature(dev, DRIVER_ATOMIC) && file_priv->is_master)
+		drm_crtc_set_sched_mode(dev, DRM_CLIENT_CAP_SCHED_NORMAL);
+
 	/* drop the master reference held by the file priv */
 	if (file_priv->master)
 		drm_master_put(&file_priv->master);
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 4f7c0bfce0a3..02f2be0b1700 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -93,6 +93,32 @@ struct drm_crtc *drm_crtc_from_index(struct drm_device *dev, int idx)
 }
 EXPORT_SYMBOL(drm_crtc_from_index);
 
+/**
+ * drm_crtc_set_sched_mode:
+ * @dev: DRM device
+ * @mode: one of DRM_CLIENT_CAP_SCHED_x
+ *
+ * Set the scheduling mode for per-CRTC kthread workers.  This controls
+ * whether nonblocking atomic commits will run with SCHED_NORMAL or
+ * SCHED_FIFO (rt) priority.
+ */
+void drm_crtc_set_sched_mode(struct drm_device *dev, int mode)
+{
+	struct drm_crtc *crtc;
+
+	drm_for_each_crtc(crtc, dev) {
+		switch (mode) {
+		case DRM_CLIENT_CAP_SCHED_NORMAL:
+			/* zero is default nice value for kthreads: */
+			sched_set_normal(crtc->worker->task, 0);
+			break;
+		case DRM_CLIENT_CAP_SCHED_FIFO:
+			sched_set_fifo(crtc->worker->task);
+			break;
+		}
+	}
+}
+
 int drm_crtc_force_disable(struct drm_crtc *crtc)
 {
 	struct drm_mode_set set = {
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index 789ee65ac1f5..44920621571c 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -362,6 +362,19 @@ drm_setclientcap(struct drm_device *dev, void *data, struct drm_file *file_priv)
 			return -EINVAL;
 		file_priv->writeback_connectors = req->value;
 		break;
+	case DRM_CLIENT_CAP_SCHED_MODE:
+		if (!file_priv->is_master)
+			return -EPERM;
+		if (!file_priv->atomic)
+			return -EOPNOTSUPP;
+		switch (req->value) {
+		case DRM_CLIENT_CAP_SCHED_NORMAL:
+		case DRM_CLIENT_CAP_SCHED_FIFO:
+			drm_crtc_set_sched_mode(dev, req->value);
+			return 0;
+		default:
+			return -EINVAL;
+		}
 	default:
 		return -EINVAL;
 	}
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 8964a3732bca..6dd4d01b7191 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -1245,6 +1245,8 @@ static inline uint32_t drm_crtc_mask(const struct drm_crtc *crtc)
 int drm_mode_set_config_internal(struct drm_mode_set *set);
 struct drm_crtc *drm_crtc_from_index(struct drm_device *dev, int idx);
 
+void drm_crtc_set_sched_mode(struct drm_device *dev, int mode);
+
 /**
  * drm_crtc_find - look up a CRTC object from its ID
  * @dev: DRM device
diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
index 808b48a93330..989e007ef608 100644
--- a/include/uapi/drm/drm.h
+++ b/include/uapi/drm/drm.h
@@ -698,6 +698,19 @@ struct drm_get_cap {
  */
 #define DRM_CLIENT_CAP_WRITEBACK_CONNECTORS	5
 
+/**
+ * DRM_CLIENT_CAP_SCHED_MODE
+ *
+ * Allow userspace to control the scheduling parameters for nonblocking
+ * commit.  The default is SCHED_NORMAL/CFS.  Userspace using SCHED_FIFO
+ * in the rendering/display pipeline should use DRM_CLIENT_CAP_SCHED_FIFO
+ * to prevent userspace portions of the display pipeline from preempting
+ * nonblocking commit_work.
+ */
+#define DRM_CLIENT_CAP_SCHED_MODE     6
+#  define DRM_CLIENT_CAP_SCHED_NORMAL 0
+#  define DRM_CLIENT_CAP_SCHED_FIFO   1
+
 /** DRM_IOCTL_SET_CLIENT_CAP ioctl argument type */
 struct drm_set_client_cap {
 	__u64 capability;
-- 
2.26.2

Re: [PATCH 1/3] drm/crtc: Introduce per-crtc kworker

[Jani Nikula]

On Sat, 19 Sep 2020, Rob Clark <robdclark@gmail.com> wrote:
> From: Rob Clark <robdclark@chromium.org>
>
> This will be used for non-block atomic commits.
>
> Signed-off-by: Rob Clark <robdclark@chromium.org>
> ---
>  drivers/gpu/drm/drm_crtc.c | 11 +++++++++++
>  include/drm/drm_crtc.h     |  8 ++++++++
>  2 files changed, 19 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index aecdd7ea26dc..4f7c0bfce0a3 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -326,6 +326,14 @@ int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc,
>  					   config->prop_out_fence_ptr, 0);
>  		drm_object_attach_property(&crtc->base,
>  					   config->prop_vrr_enabled, 0);
> +
> +		crtc->worker = kthread_create_worker(0, "%s-worker", crtc->name);
> +		if (IS_ERR(crtc->worker)) {
> +			drm_mode_object_unregister(dev, &crtc->base);
> +			ret = PTR_ERR(crtc->worker);
> +			crtc->worker = NULL;
> +			return ret;
> +		}
>  	}
>  
>  	return 0;
> @@ -366,6 +374,9 @@ void drm_crtc_cleanup(struct drm_crtc *crtc)
>  
>  	kfree(crtc->name);
>  
> +	if (crtc->worker)
> +		kthread_destroy_worker(crtc->worker);
> +
>  	memset(crtc, 0, sizeof(*crtc));
>  }
>  EXPORT_SYMBOL(drm_crtc_cleanup);
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 59b51a09cae6..8964a3732bca 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -30,6 +30,7 @@
>  #include <linux/types.h>
>  #include <linux/fb.h>
>  #include <linux/hdmi.h>
> +#include <linux/kthread.h>

A forward declaration would suffice.

>  #include <linux/media-bus-format.h>
>  #include <uapi/drm/drm_mode.h>
>  #include <uapi/drm/drm_fourcc.h>
> @@ -1172,6 +1173,13 @@ struct drm_crtc {
>  	 * Initialized via drm_self_refresh_helper_init().
>  	 */
>  	struct drm_self_refresh_data *self_refresh_data;
> +
> +	/**
> +	 * worker:

Missing @, should be "@worker:".

> +	 *
> +	 * Per-CRTC worker for nonblock atomic commits.
> +	 */
> +	struct kthread_worker *worker;
>  };
>  
>  /**

-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [PATCH 0/3] drm: commit_work scheduling

[Daniel Vetter]

On Sat, Sep 19, 2020 at 12:37:23PM -0700, Rob Clark wrote:
> From: Rob Clark <robdclark@chromium.org>
> 
> The android userspace treats the display pipeline as a realtime problem.
> And arguably, if your goal is to not miss frame deadlines (ie. vblank),
> it is.  (See https://lwn.net/Articles/809545/ for the best explaination
> that I found.)
> 
> But this presents a problem with using workqueues for non-blocking
> atomic commit_work(), because the SCHED_FIFO userspace thread(s) can
> preempt the worker.  Which is not really the outcome you want.. once
> the required fences are scheduled, you want to push the atomic commit
> down to hw ASAP.
> 
> But the decision of whether commit_work should be RT or not really
> depends on what userspace is doing.  For a pure CFS userspace display
> pipeline, commit_work() should remain SCHED_NORMAL.
> 
> To handle this, convert non-blocking commit_work() to use per-CRTC
> kthread workers, instead of system_unbound_wq.  Per-CRTC workers are
> used to avoid serializing commits when userspace is using a per-CRTC
> update loop.
> 
> A client-cap is introduced so that userspace can opt-in to SCHED_FIFO
> priority commit work.
> 
> A potential issue is that since 616d91b68cd ("sched: Remove
> sched_setscheduler*() EXPORTs") we have limited RT priority levels,
> meaning that commit_work() ends up running at the same priority level
> as vblank-work.  This shouldn't be a big problem *yet*, due to limited
> use of vblank-work at this point.  And if it could be arranged that
> vblank-work is scheduled before signaling out-fences and/or sending
> pageflip events, it could probably work ok to use a single priority
> level for both commit-work and vblank-work.

The part I don't like about this is that it all feels rather hacked
together, and if we add more stuff (or there's some different thing in the
system that also needs rt scheduling) then it doesn't compose.

So question to rt/worker folks: What's the best way to let userspace set
the scheduling mode and priorities of things the kernel does on its
behalf? Surely we're not the first ones where if userspace runs with some
rt priority it'll starve out the kernel workers that it needs. Hardcoding
something behind a subsystem ioctl (which just means every time userspace
changes what it does, we need a new such flag or mode) can't be the right
thing.

Peter, Tejun?

Thanks, Daniel

> 
> Rob Clark (3):
>   drm/crtc: Introduce per-crtc kworker
>   drm/atomic: Use kthread worker for nonblocking commits
>   drm: Add a client-cap to set scheduling mode
> 
>  drivers/gpu/drm/drm_atomic_helper.c | 13 ++++++----
>  drivers/gpu/drm/drm_auth.c          |  4 ++++
>  drivers/gpu/drm/drm_crtc.c          | 37 +++++++++++++++++++++++++++++
>  drivers/gpu/drm/drm_ioctl.c         | 13 ++++++++++
>  include/drm/drm_atomic.h            | 31 ++++++++++++++++++++++++
>  include/drm/drm_crtc.h              | 10 ++++++++
>  include/uapi/drm/drm.h              | 13 ++++++++++
>  7 files changed, 117 insertions(+), 4 deletions(-)
> 
> -- 
> 2.26.2
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 2/3] drm/atomic: Use kthread worker for nonblocking commits

[Daniel Vetter]

On Sat, Sep 19, 2020 at 12:37:25PM -0700, Rob Clark wrote:
> From: Rob Clark <robdclark@chromium.org>
> 
> This will allow us to more easily switch scheduling rules based on what
> userspace wants.
> 
> Signed-off-by: Rob Clark <robdclark@chromium.org>

I still think switching to the highpriority systemwq as a start (like i915
already does) would be a good first step no matter what we end up doing
for the android thing.
-Daniel

> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 13 ++++++++----
>  include/drm/drm_atomic.h            | 31 +++++++++++++++++++++++++++++
>  2 files changed, 40 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 9e1ad493e689..75eeec5e7b10 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1659,11 +1659,11 @@ static void commit_tail(struct drm_atomic_state *old_state)
>  	drm_atomic_state_put(old_state);
>  }
>  
> -static void commit_work(struct work_struct *work)
> +static void commit_work(struct kthread_work *work)
>  {
>  	struct drm_atomic_state *state = container_of(work,
>  						      struct drm_atomic_state,
> -						      commit_work);
> +						      commit_kwork);
>  	commit_tail(state);
>  }
>  
> @@ -1797,6 +1797,7 @@ int drm_atomic_helper_commit(struct drm_device *dev,
>  			     struct drm_atomic_state *state,
>  			     bool nonblock)
>  {
> +	struct kthread_worker *worker = NULL;
>  	int ret;
>  
>  	if (state->async_update) {
> @@ -1814,7 +1815,7 @@ int drm_atomic_helper_commit(struct drm_device *dev,
>  	if (ret)
>  		return ret;
>  
> -	INIT_WORK(&state->commit_work, commit_work);
> +	kthread_init_work(&state->commit_kwork, commit_work);
>  
>  	ret = drm_atomic_helper_prepare_planes(dev, state);
>  	if (ret)
> @@ -1857,8 +1858,12 @@ int drm_atomic_helper_commit(struct drm_device *dev,
>  	 */
>  
>  	drm_atomic_state_get(state);
> +
>  	if (nonblock)
> -		queue_work(system_unbound_wq, &state->commit_work);
> +		worker = drm_atomic_pick_worker(state);
> +
> +	if (worker)
> +		kthread_queue_work(worker, &state->commit_kwork);
>  	else
>  		commit_tail(state);
>  
> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> index d07c851d255b..8d0ee19953df 100644
> --- a/include/drm/drm_atomic.h
> +++ b/include/drm/drm_atomic.h
> @@ -373,8 +373,18 @@ struct drm_atomic_state {
>  	 *
>  	 * Work item which can be used by the driver or helpers to execute the
>  	 * commit without blocking.
> +	 *
> +	 * This is deprecated, use commit_kwork.
>  	 */
>  	struct work_struct commit_work;
> +
> +	/**
> +	 * @commit_kwork:
> +	 *
> +	 * Work item which can be used by the driver or helpers to execute the
> +	 * commit without blocking.
> +	 */
> +	struct kthread_work commit_kwork;
>  };
>  
>  void __drm_crtc_commit_free(struct kref *kref);
> @@ -954,6 +964,27 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p);
>  		      (new_obj_state) = (__state)->private_objs[__i].new_state, 1); \
>  	     (__i)++)
>  
> +/**
> + * drm_atomic_pick_worker - helper to get kworker to use for nonblocking commit
> + * @state: the &drm_atomic_state for the commit
> + *
> + * Pick an appropriate worker for a given atomic update.  The first CRTC
> + * invovled in the atomic update is used to pick the worker, to prevent
> + * serializing multiple pageflips / atomic-updates on indenpendent CRTCs.
> + */
> +static inline struct kthread_worker *
> +drm_atomic_pick_worker(const struct drm_atomic_state *state)
> +{
> +	struct drm_crtc_state *crtc_state;
> +	struct drm_crtc *crtc;
> +	unsigned i;
> +
> +	for_each_new_crtc_in_state(state, crtc, crtc_state, i)
> +		return crtc->worker;
> +
> +	return NULL;
> +}
> +
>  /**
>   * drm_atomic_crtc_needs_modeset - compute combined modeset need
>   * @state: &drm_crtc_state for the CRTC
> -- 
> 2.26.2
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 0/3] drm: commit_work scheduling

[peterz]

On Mon, Sep 21, 2020 at 11:21:54AM +0200, Daniel Vetter wrote:

> So question to rt/worker folks: What's the best way to let userspace set
> the scheduling mode and priorities of things the kernel does on its
> behalf? Surely we're not the first ones where if userspace runs with some
> rt priority it'll starve out the kernel workers that it needs. Hardcoding
> something behind a subsystem ioctl (which just means every time userspace
> changes what it does, we need a new such flag or mode) can't be the right
> thing.
> 
> Peter, Tejun?

So regular workqueues do not support RT priorities, but you can set
their nice value somewhere in /sys.

The kthread_work stuff used in these patches result in a regular kthread
and as such the user interface for changing its scheduling class or
priority is that of any other 'random' task.

Re: [PATCH 0/3] drm: commit_work scheduling

[Tejun Heo]

Hello,

On Mon, Sep 21, 2020 at 11:21:54AM +0200, Daniel Vetter wrote:
> The part I don't like about this is that it all feels rather hacked
> together, and if we add more stuff (or there's some different thing in the
> system that also needs rt scheduling) then it doesn't compose.
> 
> So question to rt/worker folks: What's the best way to let userspace set
> the scheduling mode and priorities of things the kernel does on its
> behalf? Surely we're not the first ones where if userspace runs with some
> rt priority it'll starve out the kernel workers that it needs. Hardcoding
> something behind a subsystem ioctl (which just means every time userspace
> changes what it does, we need a new such flag or mode) can't be the right
> thing.

Maybe not first but there haven't been many. The main benefit of workqueue
is that the users get to pool the worker threads automatically. I don't
think the existing workqueue design is something suitable for actual RT use
cases. Furthermore, there are inherent conflicts between sharing resources
and RT as this this patchset is already showing w/ needing per-crtc worker
thread. Maybe we can further abstract it if there are more use cases but for
now kthread_worker based implementation sounds about right to me.

Thanks.

-- 
tejun

Re: [PATCH 2/3] drm/atomic: Use kthread worker for nonblocking commits

[Rob Clark]

On Mon, Sep 21, 2020 at 2:23 AM Daniel Vetter <daniel@ffwll.ch> wrote:
>
> On Sat, Sep 19, 2020 at 12:37:25PM -0700, Rob Clark wrote:
> > From: Rob Clark <robdclark@chromium.org>
> >
> > This will allow us to more easily switch scheduling rules based on what
> > userspace wants.
> >
> > Signed-off-by: Rob Clark <robdclark@chromium.org>
>
> I still think switching to the highpriority systemwq as a start (like i915
> already does) would be a good first step no matter what we end up doing
> for the android thing.

highpri wq is probably better than the current state, but it doesn't
really address the problem.  You'll still end up with surfaceflinger
preempting commit_work..

And with non-RT priority, you'll still occasionally get lower priority
threads which haven't had a chance to run for a while preempting you.

BR,
-R


> -Daniel
>
> > ---
> >  drivers/gpu/drm/drm_atomic_helper.c | 13 ++++++++----
> >  include/drm/drm_atomic.h            | 31 +++++++++++++++++++++++++++++
> >  2 files changed, 40 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > index 9e1ad493e689..75eeec5e7b10 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -1659,11 +1659,11 @@ static void commit_tail(struct drm_atomic_state *old_state)
> >       drm_atomic_state_put(old_state);
> >  }
> >
> > -static void commit_work(struct work_struct *work)
> > +static void commit_work(struct kthread_work *work)
> >  {
> >       struct drm_atomic_state *state = container_of(work,
> >                                                     struct drm_atomic_state,
> > -                                                   commit_work);
> > +                                                   commit_kwork);
> >       commit_tail(state);
> >  }
> >
> > @@ -1797,6 +1797,7 @@ int drm_atomic_helper_commit(struct drm_device *dev,
> >                            struct drm_atomic_state *state,
> >                            bool nonblock)
> >  {
> > +     struct kthread_worker *worker = NULL;
> >       int ret;
> >
> >       if (state->async_update) {
> > @@ -1814,7 +1815,7 @@ int drm_atomic_helper_commit(struct drm_device *dev,
> >       if (ret)
> >               return ret;
> >
> > -     INIT_WORK(&state->commit_work, commit_work);
> > +     kthread_init_work(&state->commit_kwork, commit_work);
> >
> >       ret = drm_atomic_helper_prepare_planes(dev, state);
> >       if (ret)
> > @@ -1857,8 +1858,12 @@ int drm_atomic_helper_commit(struct drm_device *dev,
> >        */
> >
> >       drm_atomic_state_get(state);
> > +
> >       if (nonblock)
> > -             queue_work(system_unbound_wq, &state->commit_work);
> > +             worker = drm_atomic_pick_worker(state);
> > +
> > +     if (worker)
> > +             kthread_queue_work(worker, &state->commit_kwork);
> >       else
> >               commit_tail(state);
> >
> > diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> > index d07c851d255b..8d0ee19953df 100644
> > --- a/include/drm/drm_atomic.h
> > +++ b/include/drm/drm_atomic.h
> > @@ -373,8 +373,18 @@ struct drm_atomic_state {
> >        *
> >        * Work item which can be used by the driver or helpers to execute the
> >        * commit without blocking.
> > +      *
> > +      * This is deprecated, use commit_kwork.
> >        */
> >       struct work_struct commit_work;
> > +
> > +     /**
> > +      * @commit_kwork:
> > +      *
> > +      * Work item which can be used by the driver or helpers to execute the
> > +      * commit without blocking.
> > +      */
> > +     struct kthread_work commit_kwork;
> >  };
> >
> >  void __drm_crtc_commit_free(struct kref *kref);
> > @@ -954,6 +964,27 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p);
> >                     (new_obj_state) = (__state)->private_objs[__i].new_state, 1); \
> >            (__i)++)
> >
> > +/**
> > + * drm_atomic_pick_worker - helper to get kworker to use for nonblocking commit
> > + * @state: the &drm_atomic_state for the commit
> > + *
> > + * Pick an appropriate worker for a given atomic update.  The first CRTC
> > + * invovled in the atomic update is used to pick the worker, to prevent
> > + * serializing multiple pageflips / atomic-updates on indenpendent CRTCs.
> > + */
> > +static inline struct kthread_worker *
> > +drm_atomic_pick_worker(const struct drm_atomic_state *state)
> > +{
> > +     struct drm_crtc_state *crtc_state;
> > +     struct drm_crtc *crtc;
> > +     unsigned i;
> > +
> > +     for_each_new_crtc_in_state(state, crtc, crtc_state, i)
> > +             return crtc->worker;
> > +
> > +     return NULL;
> > +}
> > +
> >  /**
> >   * drm_atomic_crtc_needs_modeset - compute combined modeset need
> >   * @state: &drm_crtc_state for the CRTC
> > --
> > 2.26.2
> >
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

Re: [PATCH 0/3] drm: commit_work scheduling

[Rob Clark]

On Mon, Sep 21, 2020 at 2:21 AM Daniel Vetter <daniel@ffwll.ch> wrote:
>
> On Sat, Sep 19, 2020 at 12:37:23PM -0700, Rob Clark wrote:
> > From: Rob Clark <robdclark@chromium.org>
> >
> > The android userspace treats the display pipeline as a realtime problem.
> > And arguably, if your goal is to not miss frame deadlines (ie. vblank),
> > it is.  (See https://lwn.net/Articles/809545/ for the best explaination
> > that I found.)
> >
> > But this presents a problem with using workqueues for non-blocking
> > atomic commit_work(), because the SCHED_FIFO userspace thread(s) can
> > preempt the worker.  Which is not really the outcome you want.. once
> > the required fences are scheduled, you want to push the atomic commit
> > down to hw ASAP.
> >
> > But the decision of whether commit_work should be RT or not really
> > depends on what userspace is doing.  For a pure CFS userspace display
> > pipeline, commit_work() should remain SCHED_NORMAL.
> >
> > To handle this, convert non-blocking commit_work() to use per-CRTC
> > kthread workers, instead of system_unbound_wq.  Per-CRTC workers are
> > used to avoid serializing commits when userspace is using a per-CRTC
> > update loop.
> >
> > A client-cap is introduced so that userspace can opt-in to SCHED_FIFO
> > priority commit work.
> >
> > A potential issue is that since 616d91b68cd ("sched: Remove
> > sched_setscheduler*() EXPORTs") we have limited RT priority levels,
> > meaning that commit_work() ends up running at the same priority level
> > as vblank-work.  This shouldn't be a big problem *yet*, due to limited
> > use of vblank-work at this point.  And if it could be arranged that
> > vblank-work is scheduled before signaling out-fences and/or sending
> > pageflip events, it could probably work ok to use a single priority
> > level for both commit-work and vblank-work.
>
> The part I don't like about this is that it all feels rather hacked
> together, and if we add more stuff (or there's some different thing in the
> system that also needs rt scheduling) then it doesn't compose.

The ideal thing would be that userspace is in control of the
priorities.. the setclientcap approach seemed like a reasonable way to
give the drm-master a way to opt in.

I suppose instead userspace could use sched_setscheduler().. but that
would require userspace to be root, and would require some way to find
the tid.

Is there some way we could arrange for the per-crtc kthread's to be
owned by the drm master?  That would solve the "must be root" issue.
And since the target audience is an atomic userspace, I suppose we
could expose the tid as a read-only property on the crtc?

BR,
-R

> So question to rt/worker folks: What's the best way to let userspace set
> the scheduling mode and priorities of things the kernel does on its
> behalf? Surely we're not the first ones where if userspace runs with some
> rt priority it'll starve out the kernel workers that it needs. Hardcoding
> something behind a subsystem ioctl (which just means every time userspace
> changes what it does, we need a new such flag or mode) can't be the right
> thing.
>
> Peter, Tejun?
>
> Thanks, Daniel
>
> >
> > Rob Clark (3):
> >   drm/crtc: Introduce per-crtc kworker
> >   drm/atomic: Use kthread worker for nonblocking commits
> >   drm: Add a client-cap to set scheduling mode
> >
> >  drivers/gpu/drm/drm_atomic_helper.c | 13 ++++++----
> >  drivers/gpu/drm/drm_auth.c          |  4 ++++
> >  drivers/gpu/drm/drm_crtc.c          | 37 +++++++++++++++++++++++++++++
> >  drivers/gpu/drm/drm_ioctl.c         | 13 ++++++++++
> >  include/drm/drm_atomic.h            | 31 ++++++++++++++++++++++++
> >  include/drm/drm_crtc.h              | 10 ++++++++
> >  include/uapi/drm/drm.h              | 13 ++++++++++
> >  7 files changed, 117 insertions(+), 4 deletions(-)
> >
> > --
> > 2.26.2
> >
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

Re: [PATCH 0/3] drm: commit_work scheduling

[Rob Clark]

On Mon, Sep 21, 2020 at 8:16 AM Rob Clark <robdclark@gmail.com> wrote:
>
> On Mon, Sep 21, 2020 at 2:21 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> >
> > On Sat, Sep 19, 2020 at 12:37:23PM -0700, Rob Clark wrote:
> > > From: Rob Clark <robdclark@chromium.org>
> > >
> > > The android userspace treats the display pipeline as a realtime problem.
> > > And arguably, if your goal is to not miss frame deadlines (ie. vblank),
> > > it is.  (See https://lwn.net/Articles/809545/ for the best explaination
> > > that I found.)
> > >
> > > But this presents a problem with using workqueues for non-blocking
> > > atomic commit_work(), because the SCHED_FIFO userspace thread(s) can
> > > preempt the worker.  Which is not really the outcome you want.. once
> > > the required fences are scheduled, you want to push the atomic commit
> > > down to hw ASAP.
> > >
> > > But the decision of whether commit_work should be RT or not really
> > > depends on what userspace is doing.  For a pure CFS userspace display
> > > pipeline, commit_work() should remain SCHED_NORMAL.
> > >
> > > To handle this, convert non-blocking commit_work() to use per-CRTC
> > > kthread workers, instead of system_unbound_wq.  Per-CRTC workers are
> > > used to avoid serializing commits when userspace is using a per-CRTC
> > > update loop.
> > >
> > > A client-cap is introduced so that userspace can opt-in to SCHED_FIFO
> > > priority commit work.
> > >
> > > A potential issue is that since 616d91b68cd ("sched: Remove
> > > sched_setscheduler*() EXPORTs") we have limited RT priority levels,
> > > meaning that commit_work() ends up running at the same priority level
> > > as vblank-work.  This shouldn't be a big problem *yet*, due to limited
> > > use of vblank-work at this point.  And if it could be arranged that
> > > vblank-work is scheduled before signaling out-fences and/or sending
> > > pageflip events, it could probably work ok to use a single priority
> > > level for both commit-work and vblank-work.
> >
> > The part I don't like about this is that it all feels rather hacked
> > together, and if we add more stuff (or there's some different thing in the
> > system that also needs rt scheduling) then it doesn't compose.
>
> The ideal thing would be that userspace is in control of the
> priorities.. the setclientcap approach seemed like a reasonable way to
> give the drm-master a way to opt in.
>
> I suppose instead userspace could use sched_setscheduler().. but that
> would require userspace to be root, and would require some way to find
> the tid.
>
> Is there some way we could arrange for the per-crtc kthread's to be
> owned by the drm master?  That would solve the "must be root" issue.
> And since the target audience is an atomic userspace, I suppose we
> could expose the tid as a read-only property on the crtc?

Side-note, we have the same issue with work scheduled when GPU
completes a batch/submit.. I'm less sure what to do with that, so
figured I'd start with the commit_work because that was the "easy"
part ;-)

The retire_work tends to complete quickly, so maybe sched_set_fifo()
is sufficient.  (That plus these days things that run android tend to
have 8 cores so you can kinda get away with multiple things at the
same RT priority level to some degree..)

> BR,
> -R
>
> > So question to rt/worker folks: What's the best way to let userspace set
> > the scheduling mode and priorities of things the kernel does on its
> > behalf? Surely we're not the first ones where if userspace runs with some
> > rt priority it'll starve out the kernel workers that it needs. Hardcoding
> > something behind a subsystem ioctl (which just means every time userspace
> > changes what it does, we need a new such flag or mode) can't be the right
> > thing.
> >
> > Peter, Tejun?
> >
> > Thanks, Daniel
> >
> > >
> > > Rob Clark (3):
> > >   drm/crtc: Introduce per-crtc kworker
> > >   drm/atomic: Use kthread worker for nonblocking commits
> > >   drm: Add a client-cap to set scheduling mode
> > >
> > >  drivers/gpu/drm/drm_atomic_helper.c | 13 ++++++----
> > >  drivers/gpu/drm/drm_auth.c          |  4 ++++
> > >  drivers/gpu/drm/drm_crtc.c          | 37 +++++++++++++++++++++++++++++
> > >  drivers/gpu/drm/drm_ioctl.c         | 13 ++++++++++
> > >  include/drm/drm_atomic.h            | 31 ++++++++++++++++++++++++
> > >  include/drm/drm_crtc.h              | 10 ++++++++
> > >  include/uapi/drm/drm.h              | 13 ++++++++++
> > >  7 files changed, 117 insertions(+), 4 deletions(-)
> > >
> > > --
> > > 2.26.2
> > >
> > > _______________________________________________
> > > dri-devel mailing list
> > > dri-devel@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch

Re: [PATCH 0/3] drm: commit_work scheduling

[Qais Yousef]

On 09/19/20 12:37, Rob Clark wrote:
> From: Rob Clark <robdclark@chromium.org>
> 
> The android userspace treats the display pipeline as a realtime problem.
> And arguably, if your goal is to not miss frame deadlines (ie. vblank),
> it is.  (See https://lwn.net/Articles/809545/ for the best explaination
> that I found.)
> 
> But this presents a problem with using workqueues for non-blocking
> atomic commit_work(), because the SCHED_FIFO userspace thread(s) can
> preempt the worker.  Which is not really the outcome you want.. once
> the required fences are scheduled, you want to push the atomic commit
> down to hw ASAP.
> 
> But the decision of whether commit_work should be RT or not really
> depends on what userspace is doing.  For a pure CFS userspace display
> pipeline, commit_work() should remain SCHED_NORMAL.

Just a side note; this RT vs CFS inter-operatability is an issue that
creeps up every now and again.

https://lore.kernel.org/lkml/1567048502-6064-1-git-send-email-jing-ting.wu@mediatek.com/

Does the UI thread in Android ever run as RT by the way? I always suspected it
is one susceptible to such potential delays since it is part of the application
thread and thought it can't be trusted to become RT.

Those 120MHz displays will stress the pipeline :-)

> 
> To handle this, convert non-blocking commit_work() to use per-CRTC
> kthread workers, instead of system_unbound_wq.  Per-CRTC workers are
> used to avoid serializing commits when userspace is using a per-CRTC
> update loop.
> 
> A client-cap is introduced so that userspace can opt-in to SCHED_FIFO
> priority commit work.
> 
> A potential issue is that since 616d91b68cd ("sched: Remove
> sched_setscheduler*() EXPORTs") we have limited RT priority levels,
> meaning that commit_work() ends up running at the same priority level
> as vblank-work.  This shouldn't be a big problem *yet*, due to limited
> use of vblank-work at this point.  And if it could be arranged that
> vblank-work is scheduled before signaling out-fences and/or sending
> pageflip events, it could probably work ok to use a single priority
> level for both commit-work and vblank-work.

This is a function of num_cpus too. As long as nr_cpus > nr_running_rt_tasks
you should be fine.

Cheers

--
Qais Yousef

Re: [PATCH 0/3] drm: commit_work scheduling

[Rob Clark]

On Mon, Sep 21, 2020 at 8:16 AM Rob Clark <robdclark@gmail.com> wrote:
>
> On Mon, Sep 21, 2020 at 2:21 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> >
> > On Sat, Sep 19, 2020 at 12:37:23PM -0700, Rob Clark wrote:
> > > From: Rob Clark <robdclark@chromium.org>
> > >
> > > The android userspace treats the display pipeline as a realtime problem.
> > > And arguably, if your goal is to not miss frame deadlines (ie. vblank),
> > > it is.  (See https://lwn.net/Articles/809545/ for the best explaination
> > > that I found.)
> > >
> > > But this presents a problem with using workqueues for non-blocking
> > > atomic commit_work(), because the SCHED_FIFO userspace thread(s) can
> > > preempt the worker.  Which is not really the outcome you want.. once
> > > the required fences are scheduled, you want to push the atomic commit
> > > down to hw ASAP.
> > >
> > > But the decision of whether commit_work should be RT or not really
> > > depends on what userspace is doing.  For a pure CFS userspace display
> > > pipeline, commit_work() should remain SCHED_NORMAL.
> > >
> > > To handle this, convert non-blocking commit_work() to use per-CRTC
> > > kthread workers, instead of system_unbound_wq.  Per-CRTC workers are
> > > used to avoid serializing commits when userspace is using a per-CRTC
> > > update loop.
> > >
> > > A client-cap is introduced so that userspace can opt-in to SCHED_FIFO
> > > priority commit work.
> > >
> > > A potential issue is that since 616d91b68cd ("sched: Remove
> > > sched_setscheduler*() EXPORTs") we have limited RT priority levels,
> > > meaning that commit_work() ends up running at the same priority level
> > > as vblank-work.  This shouldn't be a big problem *yet*, due to limited
> > > use of vblank-work at this point.  And if it could be arranged that
> > > vblank-work is scheduled before signaling out-fences and/or sending
> > > pageflip events, it could probably work ok to use a single priority
> > > level for both commit-work and vblank-work.
> >
> > The part I don't like about this is that it all feels rather hacked
> > together, and if we add more stuff (or there's some different thing in the
> > system that also needs rt scheduling) then it doesn't compose.
>
> The ideal thing would be that userspace is in control of the
> priorities.. the setclientcap approach seemed like a reasonable way to
> give the drm-master a way to opt in.
>
> I suppose instead userspace could use sched_setscheduler().. but that
> would require userspace to be root, and would require some way to find
> the tid.
>
> Is there some way we could arrange for the per-crtc kthread's to be
> owned by the drm master?  That would solve the "must be root" issue.
> And since the target audience is an atomic userspace, I suppose we
> could expose the tid as a read-only property on the crtc?

Looks like kthread goes out of it's way to *not* be owned by users (to
avoid fork, and such complications?)

But maybe we could modify the kthread_worker's task->real_cred?  I
didn't see any examples of anything else doing something similar, so
I'm not sure what sorts of dragons there lie..

> BR,
> -R
>
> > So question to rt/worker folks: What's the best way to let userspace set
> > the scheduling mode and priorities of things the kernel does on its
> > behalf? Surely we're not the first ones where if userspace runs with some
> > rt priority it'll starve out the kernel workers that it needs. Hardcoding
> > something behind a subsystem ioctl (which just means every time userspace
> > changes what it does, we need a new such flag or mode) can't be the right
> > thing.
> >
> > Peter, Tejun?
> >
> > Thanks, Daniel
> >
> > >
> > > Rob Clark (3):
> > >   drm/crtc: Introduce per-crtc kworker
> > >   drm/atomic: Use kthread worker for nonblocking commits
> > >   drm: Add a client-cap to set scheduling mode
> > >
> > >  drivers/gpu/drm/drm_atomic_helper.c | 13 ++++++----
> > >  drivers/gpu/drm/drm_auth.c          |  4 ++++
> > >  drivers/gpu/drm/drm_crtc.c          | 37 +++++++++++++++++++++++++++++
> > >  drivers/gpu/drm/drm_ioctl.c         | 13 ++++++++++
> > >  include/drm/drm_atomic.h            | 31 ++++++++++++++++++++++++
> > >  include/drm/drm_crtc.h              | 10 ++++++++
> > >  include/uapi/drm/drm.h              | 13 ++++++++++
> > >  7 files changed, 117 insertions(+), 4 deletions(-)
> > >
> > > --
> > > 2.26.2
> > >
> > > _______________________________________________
> > > dri-devel mailing list
> > > dri-devel@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch

Re: [PATCH 0/3] drm: commit_work scheduling

[Rob Clark]

On Mon, Sep 21, 2020 at 9:10 AM Qais Yousef <qais.yousef@arm.com> wrote:
>
> On 09/19/20 12:37, Rob Clark wrote:
> > From: Rob Clark <robdclark@chromium.org>
> >
> > The android userspace treats the display pipeline as a realtime problem.
> > And arguably, if your goal is to not miss frame deadlines (ie. vblank),
> > it is.  (See https://lwn.net/Articles/809545/ for the best explaination
> > that I found.)
> >
> > But this presents a problem with using workqueues for non-blocking
> > atomic commit_work(), because the SCHED_FIFO userspace thread(s) can
> > preempt the worker.  Which is not really the outcome you want.. once
> > the required fences are scheduled, you want to push the atomic commit
> > down to hw ASAP.
> >
> > But the decision of whether commit_work should be RT or not really
> > depends on what userspace is doing.  For a pure CFS userspace display
> > pipeline, commit_work() should remain SCHED_NORMAL.
>
> Just a side note; this RT vs CFS inter-operatability is an issue that
> creeps up every now and again.
>
> https://lore.kernel.org/lkml/1567048502-6064-1-git-send-email-jing-ting.wu@mediatek.com/
>
> Does the UI thread in Android ever run as RT by the way? I always suspected it
> is one susceptible to such potential delays since it is part of the application
> thread and thought it can't be trusted to become RT.

The application itself is not RT, since there is no good way to know
what other things the app may be doing.  Although that is mentioned in
the lwn article (ie. the hypothetical SCHED_DEADLINE + token passing
part)

But at least once the app / ui thread is done, then it is all (or at
least in theory "all" if atomic-helper doesn't gum up the works ;-))
SCHED_FIFO from there to the display.

> Those 120MHz displays will stress the pipeline :-)

indeed

BR,
-R

> >
> > To handle this, convert non-blocking commit_work() to use per-CRTC
> > kthread workers, instead of system_unbound_wq.  Per-CRTC workers are
> > used to avoid serializing commits when userspace is using a per-CRTC
> > update loop.
> >
> > A client-cap is introduced so that userspace can opt-in to SCHED_FIFO
> > priority commit work.
> >
> > A potential issue is that since 616d91b68cd ("sched: Remove
> > sched_setscheduler*() EXPORTs") we have limited RT priority levels,
> > meaning that commit_work() ends up running at the same priority level
> > as vblank-work.  This shouldn't be a big problem *yet*, due to limited
> > use of vblank-work at this point.  And if it could be arranged that
> > vblank-work is scheduled before signaling out-fences and/or sending
> > pageflip events, it could probably work ok to use a single priority
> > level for both commit-work and vblank-work.
>
> This is a function of num_cpus too. As long as nr_cpus > nr_running_rt_tasks
> you should be fine.
>
> Cheers
>
> --
> Qais Yousef

Re: [PATCH 0/3] drm: commit_work scheduling

[Daniel Vetter]

On Mon, Sep 21, 2020 at 5:16 PM Rob Clark <robdclark@gmail.com> wrote:
>
> On Mon, Sep 21, 2020 at 2:21 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> >
> > On Sat, Sep 19, 2020 at 12:37:23PM -0700, Rob Clark wrote:
> > > From: Rob Clark <robdclark@chromium.org>
> > >
> > > The android userspace treats the display pipeline as a realtime problem.
> > > And arguably, if your goal is to not miss frame deadlines (ie. vblank),
> > > it is.  (See https://lwn.net/Articles/809545/ for the best explaination
> > > that I found.)
> > >
> > > But this presents a problem with using workqueues for non-blocking
> > > atomic commit_work(), because the SCHED_FIFO userspace thread(s) can
> > > preempt the worker.  Which is not really the outcome you want.. once
> > > the required fences are scheduled, you want to push the atomic commit
> > > down to hw ASAP.
> > >
> > > But the decision of whether commit_work should be RT or not really
> > > depends on what userspace is doing.  For a pure CFS userspace display
> > > pipeline, commit_work() should remain SCHED_NORMAL.
> > >
> > > To handle this, convert non-blocking commit_work() to use per-CRTC
> > > kthread workers, instead of system_unbound_wq.  Per-CRTC workers are
> > > used to avoid serializing commits when userspace is using a per-CRTC
> > > update loop.
> > >
> > > A client-cap is introduced so that userspace can opt-in to SCHED_FIFO
> > > priority commit work.
> > >
> > > A potential issue is that since 616d91b68cd ("sched: Remove
> > > sched_setscheduler*() EXPORTs") we have limited RT priority levels,
> > > meaning that commit_work() ends up running at the same priority level
> > > as vblank-work.  This shouldn't be a big problem *yet*, due to limited
> > > use of vblank-work at this point.  And if it could be arranged that
> > > vblank-work is scheduled before signaling out-fences and/or sending
> > > pageflip events, it could probably work ok to use a single priority
> > > level for both commit-work and vblank-work.
> >
> > The part I don't like about this is that it all feels rather hacked
> > together, and if we add more stuff (or there's some different thing in the
> > system that also needs rt scheduling) then it doesn't compose.
>
> The ideal thing would be that userspace is in control of the
> priorities.. the setclientcap approach seemed like a reasonable way to
> give the drm-master a way to opt in.
>
> I suppose instead userspace could use sched_setscheduler().. but that
> would require userspace to be root, and would require some way to find
> the tid.

Userspace already needs that for the SCHED_FIFO for surface-flinger.
Or is the problem that CAP_SYS_NICE is only good for your own
processes?

Other question I have for this is whether there's any recommendations
for naming the kthreads (since I guess that name is what becomes the
uapi for userspace to control this)?

Otherwise I think "userspace calls sched_setscheduler on the right
kthreads" sounds like a good interface, since it lets userspace decide
how it all needs to fit together and compose. Anything we hard-code in
an ioctl is kinda lost cause. And we can choose the default values to
work reasonably well when the compositor runs at normal priority
(lowest niceness or something like that for the commit work).
-Daniel

> Is there some way we could arrange for the per-crtc kthread's to be
> owned by the drm master?  That would solve the "must be root" issue.
> And since the target audience is an atomic userspace, I suppose we
> could expose the tid as a read-only property on the crtc?
>
> BR,
> -R
>
> > So question to rt/worker folks: What's the best way to let userspace set
> > the scheduling mode and priorities of things the kernel does on its
> > behalf? Surely we're not the first ones where if userspace runs with some
> > rt priority it'll starve out the kernel workers that it needs. Hardcoding
> > something behind a subsystem ioctl (which just means every time userspace
> > changes what it does, we need a new such flag or mode) can't be the right
> > thing.
> >
> > Peter, Tejun?
> >
> > Thanks, Daniel
> >
> > >
> > > Rob Clark (3):
> > >   drm/crtc: Introduce per-crtc kworker
> > >   drm/atomic: Use kthread worker for nonblocking commits
> > >   drm: Add a client-cap to set scheduling mode
> > >
> > >  drivers/gpu/drm/drm_atomic_helper.c | 13 ++++++----
> > >  drivers/gpu/drm/drm_auth.c          |  4 ++++
> > >  drivers/gpu/drm/drm_crtc.c          | 37 +++++++++++++++++++++++++++++
> > >  drivers/gpu/drm/drm_ioctl.c         | 13 ++++++++++
> > >  include/drm/drm_atomic.h            | 31 ++++++++++++++++++++++++
> > >  include/drm/drm_crtc.h              | 10 ++++++++
> > >  include/uapi/drm/drm.h              | 13 ++++++++++
> > >  7 files changed, 117 insertions(+), 4 deletions(-)
> > >
> > > --
> > > 2.26.2
> > >
> > > _______________________________________________
> > > dri-devel mailing list
> > > dri-devel@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel



-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 2/3] drm/atomic: Use kthread worker for nonblocking commits

[Daniel Vetter]

On Mon, Sep 21, 2020 at 07:55:42AM -0700, Rob Clark wrote:
> On Mon, Sep 21, 2020 at 2:23 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> >
> > On Sat, Sep 19, 2020 at 12:37:25PM -0700, Rob Clark wrote:
> > > From: Rob Clark <robdclark@chromium.org>
> > >
> > > This will allow us to more easily switch scheduling rules based on what
> > > userspace wants.
> > >
> > > Signed-off-by: Rob Clark <robdclark@chromium.org>
> >
> > I still think switching to the highpriority systemwq as a start (like i915
> > already does) would be a good first step no matter what we end up doing
> > for the android thing.
> 
> highpri wq is probably better than the current state, but it doesn't
> really address the problem.  You'll still end up with surfaceflinger
> preempting commit_work..
> 
> And with non-RT priority, you'll still occasionally get lower priority
> threads which haven't had a chance to run for a while preempting you.

Sure the priority inversion is still there and needs a different fix. But
maybe it'll make everyone else at least a bit happier.

Plus it's really hard to make kms drivers rt, it's not really been part of
the design (nor are gpus really rt friendly, if they even can preempt it
generally takes forever compared to the deadline you might want for some
present work).
-Daniel
> 
> BR,
> -R
> 
> 
> > -Daniel
> >
> > > ---
> > >  drivers/gpu/drm/drm_atomic_helper.c | 13 ++++++++----
> > >  include/drm/drm_atomic.h            | 31 +++++++++++++++++++++++++++++
> > >  2 files changed, 40 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > > index 9e1ad493e689..75eeec5e7b10 100644
> > > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > > @@ -1659,11 +1659,11 @@ static void commit_tail(struct drm_atomic_state *old_state)
> > >       drm_atomic_state_put(old_state);
> > >  }
> > >
> > > -static void commit_work(struct work_struct *work)
> > > +static void commit_work(struct kthread_work *work)
> > >  {
> > >       struct drm_atomic_state *state = container_of(work,
> > >                                                     struct drm_atomic_state,
> > > -                                                   commit_work);
> > > +                                                   commit_kwork);
> > >       commit_tail(state);
> > >  }
> > >
> > > @@ -1797,6 +1797,7 @@ int drm_atomic_helper_commit(struct drm_device *dev,
> > >                            struct drm_atomic_state *state,
> > >                            bool nonblock)
> > >  {
> > > +     struct kthread_worker *worker = NULL;
> > >       int ret;
> > >
> > >       if (state->async_update) {
> > > @@ -1814,7 +1815,7 @@ int drm_atomic_helper_commit(struct drm_device *dev,
> > >       if (ret)
> > >               return ret;
> > >
> > > -     INIT_WORK(&state->commit_work, commit_work);
> > > +     kthread_init_work(&state->commit_kwork, commit_work);
> > >
> > >       ret = drm_atomic_helper_prepare_planes(dev, state);
> > >       if (ret)
> > > @@ -1857,8 +1858,12 @@ int drm_atomic_helper_commit(struct drm_device *dev,
> > >        */
> > >
> > >       drm_atomic_state_get(state);
> > > +
> > >       if (nonblock)
> > > -             queue_work(system_unbound_wq, &state->commit_work);
> > > +             worker = drm_atomic_pick_worker(state);
> > > +
> > > +     if (worker)
> > > +             kthread_queue_work(worker, &state->commit_kwork);
> > >       else
> > >               commit_tail(state);
> > >
> > > diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> > > index d07c851d255b..8d0ee19953df 100644
> > > --- a/include/drm/drm_atomic.h
> > > +++ b/include/drm/drm_atomic.h
> > > @@ -373,8 +373,18 @@ struct drm_atomic_state {
> > >        *
> > >        * Work item which can be used by the driver or helpers to execute the
> > >        * commit without blocking.
> > > +      *
> > > +      * This is deprecated, use commit_kwork.
> > >        */
> > >       struct work_struct commit_work;
> > > +
> > > +     /**
> > > +      * @commit_kwork:
> > > +      *
> > > +      * Work item which can be used by the driver or helpers to execute the
> > > +      * commit without blocking.
> > > +      */
> > > +     struct kthread_work commit_kwork;
> > >  };
> > >
> > >  void __drm_crtc_commit_free(struct kref *kref);
> > > @@ -954,6 +964,27 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p);
> > >                     (new_obj_state) = (__state)->private_objs[__i].new_state, 1); \
> > >            (__i)++)
> > >
> > > +/**
> > > + * drm_atomic_pick_worker - helper to get kworker to use for nonblocking commit
> > > + * @state: the &drm_atomic_state for the commit
> > > + *
> > > + * Pick an appropriate worker for a given atomic update.  The first CRTC
> > > + * invovled in the atomic update is used to pick the worker, to prevent
> > > + * serializing multiple pageflips / atomic-updates on indenpendent CRTCs.
> > > + */
> > > +static inline struct kthread_worker *
> > > +drm_atomic_pick_worker(const struct drm_atomic_state *state)
> > > +{
> > > +     struct drm_crtc_state *crtc_state;
> > > +     struct drm_crtc *crtc;
> > > +     unsigned i;
> > > +
> > > +     for_each_new_crtc_in_state(state, crtc, crtc_state, i)
> > > +             return crtc->worker;
> > > +
> > > +     return NULL;
> > > +}
> > > +
> > >  /**
> > >   * drm_atomic_crtc_needs_modeset - compute combined modeset need
> > >   * @state: &drm_crtc_state for the CRTC
> > > --
> > > 2.26.2
> > >
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 0/3] drm: commit_work scheduling

[Rob Clark]

On Mon, Sep 21, 2020 at 11:59 PM Daniel Vetter <daniel@ffwll.ch> wrote:
>
> On Mon, Sep 21, 2020 at 5:16 PM Rob Clark <robdclark@gmail.com> wrote:
> >
> > On Mon, Sep 21, 2020 at 2:21 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> > >
> > > On Sat, Sep 19, 2020 at 12:37:23PM -0700, Rob Clark wrote:
> > > > From: Rob Clark <robdclark@chromium.org>
> > > >
> > > > The android userspace treats the display pipeline as a realtime problem.
> > > > And arguably, if your goal is to not miss frame deadlines (ie. vblank),
> > > > it is.  (See https://lwn.net/Articles/809545/ for the best explaination
> > > > that I found.)
> > > >
> > > > But this presents a problem with using workqueues for non-blocking
> > > > atomic commit_work(), because the SCHED_FIFO userspace thread(s) can
> > > > preempt the worker.  Which is not really the outcome you want.. once
> > > > the required fences are scheduled, you want to push the atomic commit
> > > > down to hw ASAP.
> > > >
> > > > But the decision of whether commit_work should be RT or not really
> > > > depends on what userspace is doing.  For a pure CFS userspace display
> > > > pipeline, commit_work() should remain SCHED_NORMAL.
> > > >
> > > > To handle this, convert non-blocking commit_work() to use per-CRTC
> > > > kthread workers, instead of system_unbound_wq.  Per-CRTC workers are
> > > > used to avoid serializing commits when userspace is using a per-CRTC
> > > > update loop.
> > > >
> > > > A client-cap is introduced so that userspace can opt-in to SCHED_FIFO
> > > > priority commit work.
> > > >
> > > > A potential issue is that since 616d91b68cd ("sched: Remove
> > > > sched_setscheduler*() EXPORTs") we have limited RT priority levels,
> > > > meaning that commit_work() ends up running at the same priority level
> > > > as vblank-work.  This shouldn't be a big problem *yet*, due to limited
> > > > use of vblank-work at this point.  And if it could be arranged that
> > > > vblank-work is scheduled before signaling out-fences and/or sending
> > > > pageflip events, it could probably work ok to use a single priority
> > > > level for both commit-work and vblank-work.
> > >
> > > The part I don't like about this is that it all feels rather hacked
> > > together, and if we add more stuff (or there's some different thing in the
> > > system that also needs rt scheduling) then it doesn't compose.
> >
> > The ideal thing would be that userspace is in control of the
> > priorities.. the setclientcap approach seemed like a reasonable way to
> > give the drm-master a way to opt in.
> >
> > I suppose instead userspace could use sched_setscheduler().. but that
> > would require userspace to be root, and would require some way to find
> > the tid.
>
> Userspace already needs that for the SCHED_FIFO for surface-flinger.
> Or is the problem that CAP_SYS_NICE is only good for your own
> processes?

tbh, I'm not completely sure offhand what gives surfaceflinger
permission to set itself SCHED_FIFO

(But on CrOS there are a few more pieces to the puzzle)

> Other question I have for this is whether there's any recommendations
> for naming the kthreads (since I guess that name is what becomes the
> uapi for userspace to control this)?
>
> Otherwise I think "userspace calls sched_setscheduler on the right
> kthreads" sounds like a good interface, since it lets userspace decide
> how it all needs to fit together and compose. Anything we hard-code in
> an ioctl is kinda lost cause. And we can choose the default values to
> work reasonably well when the compositor runs at normal priority
> (lowest niceness or something like that for the commit work).

I don't really like the naming convention approach.. what is to stop
some unrelated process to name it's thread the same thing to get a
SCHED_FIFO boost..

But we can stick with my idea to expose the thread id as a read-only
CRTC property, for userspace to find the things to call
sched_setscheduler() on.  If for whatever reason the drm master is not
privileged (or is running in a sandbox, etc), a small helper that has
the necessary permissions could open the drm device to find the CRTC
thread-ids and call sched_setscheduler()..

BR,
-R

> -Daniel
>
> > Is there some way we could arrange for the per-crtc kthread's to be
> > owned by the drm master?  That would solve the "must be root" issue.
> > And since the target audience is an atomic userspace, I suppose we
> > could expose the tid as a read-only property on the crtc?
> >
> > BR,
> > -R
> >
> > > So question to rt/worker folks: What's the best way to let userspace set
> > > the scheduling mode and priorities of things the kernel does on its
> > > behalf? Surely we're not the first ones where if userspace runs with some
> > > rt priority it'll starve out the kernel workers that it needs. Hardcoding
> > > something behind a subsystem ioctl (which just means every time userspace
> > > changes what it does, we need a new such flag or mode) can't be the right
> > > thing.
> > >
> > > Peter, Tejun?
> > >
> > > Thanks, Daniel
> > >
> > > >
> > > > Rob Clark (3):
> > > >   drm/crtc: Introduce per-crtc kworker
> > > >   drm/atomic: Use kthread worker for nonblocking commits
> > > >   drm: Add a client-cap to set scheduling mode
> > > >
> > > >  drivers/gpu/drm/drm_atomic_helper.c | 13 ++++++----
> > > >  drivers/gpu/drm/drm_auth.c          |  4 ++++
> > > >  drivers/gpu/drm/drm_crtc.c          | 37 +++++++++++++++++++++++++++++
> > > >  drivers/gpu/drm/drm_ioctl.c         | 13 ++++++++++
> > > >  include/drm/drm_atomic.h            | 31 ++++++++++++++++++++++++
> > > >  include/drm/drm_crtc.h              | 10 ++++++++
> > > >  include/uapi/drm/drm.h              | 13 ++++++++++
> > > >  7 files changed, 117 insertions(+), 4 deletions(-)
> > > >
> > > > --
> > > > 2.26.2
> > > >
> > > > _______________________________________________
> > > > dri-devel mailing list
> > > > dri-devel@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > >
> > > --
> > > Daniel Vetter
> > > Software Engineer, Intel Corporation
> > > http://blog.ffwll.ch
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
>
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

Re: [PATCH 0/3] drm: commit_work scheduling

[Daniel Vetter]

On Tue, Sep 22, 2020 at 07:48:10AM -0700, Rob Clark wrote:
> On Mon, Sep 21, 2020 at 11:59 PM Daniel Vetter <daniel@ffwll.ch> wrote:
> >
> > On Mon, Sep 21, 2020 at 5:16 PM Rob Clark <robdclark@gmail.com> wrote:
> > >
> > > On Mon, Sep 21, 2020 at 2:21 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> > > >
> > > > On Sat, Sep 19, 2020 at 12:37:23PM -0700, Rob Clark wrote:
> > > > > From: Rob Clark <robdclark@chromium.org>
> > > > >
> > > > > The android userspace treats the display pipeline as a realtime problem.
> > > > > And arguably, if your goal is to not miss frame deadlines (ie. vblank),
> > > > > it is.  (See https://lwn.net/Articles/809545/ for the best explaination
> > > > > that I found.)
> > > > >
> > > > > But this presents a problem with using workqueues for non-blocking
> > > > > atomic commit_work(), because the SCHED_FIFO userspace thread(s) can
> > > > > preempt the worker.  Which is not really the outcome you want.. once
> > > > > the required fences are scheduled, you want to push the atomic commit
> > > > > down to hw ASAP.
> > > > >
> > > > > But the decision of whether commit_work should be RT or not really
> > > > > depends on what userspace is doing.  For a pure CFS userspace display
> > > > > pipeline, commit_work() should remain SCHED_NORMAL.
> > > > >
> > > > > To handle this, convert non-blocking commit_work() to use per-CRTC
> > > > > kthread workers, instead of system_unbound_wq.  Per-CRTC workers are
> > > > > used to avoid serializing commits when userspace is using a per-CRTC
> > > > > update loop.
> > > > >
> > > > > A client-cap is introduced so that userspace can opt-in to SCHED_FIFO
> > > > > priority commit work.
> > > > >
> > > > > A potential issue is that since 616d91b68cd ("sched: Remove
> > > > > sched_setscheduler*() EXPORTs") we have limited RT priority levels,
> > > > > meaning that commit_work() ends up running at the same priority level
> > > > > as vblank-work.  This shouldn't be a big problem *yet*, due to limited
> > > > > use of vblank-work at this point.  And if it could be arranged that
> > > > > vblank-work is scheduled before signaling out-fences and/or sending
> > > > > pageflip events, it could probably work ok to use a single priority
> > > > > level for both commit-work and vblank-work.
> > > >
> > > > The part I don't like about this is that it all feels rather hacked
> > > > together, and if we add more stuff (or there's some different thing in the
> > > > system that also needs rt scheduling) then it doesn't compose.
> > >
> > > The ideal thing would be that userspace is in control of the
> > > priorities.. the setclientcap approach seemed like a reasonable way to
> > > give the drm-master a way to opt in.
> > >
> > > I suppose instead userspace could use sched_setscheduler().. but that
> > > would require userspace to be root, and would require some way to find
> > > the tid.
> >
> > Userspace already needs that for the SCHED_FIFO for surface-flinger.
> > Or is the problem that CAP_SYS_NICE is only good for your own
> > processes?
> 
> tbh, I'm not completely sure offhand what gives surfaceflinger
> permission to set itself SCHED_FIFO
> 
> (But on CrOS there are a few more pieces to the puzzle)
> 
> > Other question I have for this is whether there's any recommendations
> > for naming the kthreads (since I guess that name is what becomes the
> > uapi for userspace to control this)?
> >
> > Otherwise I think "userspace calls sched_setscheduler on the right
> > kthreads" sounds like a good interface, since it lets userspace decide
> > how it all needs to fit together and compose. Anything we hard-code in
> > an ioctl is kinda lost cause. And we can choose the default values to
> > work reasonably well when the compositor runs at normal priority
> > (lowest niceness or something like that for the commit work).
> 
> I don't really like the naming convention approach.. what is to stop
> some unrelated process to name it's thread the same thing to get a
> SCHED_FIFO boost..
> 
> But we can stick with my idea to expose the thread id as a read-only
> CRTC property, for userspace to find the things to call
> sched_setscheduler() on.  If for whatever reason the drm master is not
> privileged (or is running in a sandbox, etc), a small helper that has
> the necessary permissions could open the drm device to find the CRTC
> thread-ids and call sched_setscheduler()..

Hm thread ids don't translate too well across PID namespaces I think ...
So that's another can of worms. And pidfd doesn't really work as a
property.

I also thought kernel threads can be distinguished from others, so
userspace shouldn't be able to sneak in and get elevated by accident.
-Daniel

> 
> BR,
> -R
> 
> > -Daniel
> >
> > > Is there some way we could arrange for the per-crtc kthread's to be
> > > owned by the drm master?  That would solve the "must be root" issue.
> > > And since the target audience is an atomic userspace, I suppose we
> > > could expose the tid as a read-only property on the crtc?
> > >
> > > BR,
> > > -R
> > >
> > > > So question to rt/worker folks: What's the best way to let userspace set
> > > > the scheduling mode and priorities of things the kernel does on its
> > > > behalf? Surely we're not the first ones where if userspace runs with some
> > > > rt priority it'll starve out the kernel workers that it needs. Hardcoding
> > > > something behind a subsystem ioctl (which just means every time userspace
> > > > changes what it does, we need a new such flag or mode) can't be the right
> > > > thing.
> > > >
> > > > Peter, Tejun?
> > > >
> > > > Thanks, Daniel
> > > >
> > > > >
> > > > > Rob Clark (3):
> > > > >   drm/crtc: Introduce per-crtc kworker
> > > > >   drm/atomic: Use kthread worker for nonblocking commits
> > > > >   drm: Add a client-cap to set scheduling mode
> > > > >
> > > > >  drivers/gpu/drm/drm_atomic_helper.c | 13 ++++++----
> > > > >  drivers/gpu/drm/drm_auth.c          |  4 ++++
> > > > >  drivers/gpu/drm/drm_crtc.c          | 37 +++++++++++++++++++++++++++++
> > > > >  drivers/gpu/drm/drm_ioctl.c         | 13 ++++++++++
> > > > >  include/drm/drm_atomic.h            | 31 ++++++++++++++++++++++++
> > > > >  include/drm/drm_crtc.h              | 10 ++++++++
> > > > >  include/uapi/drm/drm.h              | 13 ++++++++++
> > > > >  7 files changed, 117 insertions(+), 4 deletions(-)
> > > > >
> > > > > --
> > > > > 2.26.2
> > > > >
> > > > > _______________________________________________
> > > > > dri-devel mailing list
> > > > > dri-devel@lists.freedesktop.org
> > > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > > >
> > > > --
> > > > Daniel Vetter
> > > > Software Engineer, Intel Corporation
> > > > http://blog.ffwll.ch
> > > _______________________________________________
> > > dri-devel mailing list
> > > dri-devel@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >
> >
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 0/3] drm: commit_work scheduling

[Rob Clark]

On Wed, Sep 23, 2020 at 8:25 AM Daniel Vetter <daniel@ffwll.ch> wrote:
>
> On Tue, Sep 22, 2020 at 07:48:10AM -0700, Rob Clark wrote:
> > On Mon, Sep 21, 2020 at 11:59 PM Daniel Vetter <daniel@ffwll.ch> wrote:
> > >
> > > On Mon, Sep 21, 2020 at 5:16 PM Rob Clark <robdclark@gmail.com> wrote:
> > > >
> > > > On Mon, Sep 21, 2020 at 2:21 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> > > > >
> > > > > On Sat, Sep 19, 2020 at 12:37:23PM -0700, Rob Clark wrote:
> > > > > > From: Rob Clark <robdclark@chromium.org>
> > > > > >
> > > > > > The android userspace treats the display pipeline as a realtime problem.
> > > > > > And arguably, if your goal is to not miss frame deadlines (ie. vblank),
> > > > > > it is.  (See https://lwn.net/Articles/809545/ for the best explaination
> > > > > > that I found.)
> > > > > >
> > > > > > But this presents a problem with using workqueues for non-blocking
> > > > > > atomic commit_work(), because the SCHED_FIFO userspace thread(s) can
> > > > > > preempt the worker.  Which is not really the outcome you want.. once
> > > > > > the required fences are scheduled, you want to push the atomic commit
> > > > > > down to hw ASAP.
> > > > > >
> > > > > > But the decision of whether commit_work should be RT or not really
> > > > > > depends on what userspace is doing.  For a pure CFS userspace display
> > > > > > pipeline, commit_work() should remain SCHED_NORMAL.
> > > > > >
> > > > > > To handle this, convert non-blocking commit_work() to use per-CRTC
> > > > > > kthread workers, instead of system_unbound_wq.  Per-CRTC workers are
> > > > > > used to avoid serializing commits when userspace is using a per-CRTC
> > > > > > update loop.
> > > > > >
> > > > > > A client-cap is introduced so that userspace can opt-in to SCHED_FIFO
> > > > > > priority commit work.
> > > > > >
> > > > > > A potential issue is that since 616d91b68cd ("sched: Remove
> > > > > > sched_setscheduler*() EXPORTs") we have limited RT priority levels,
> > > > > > meaning that commit_work() ends up running at the same priority level
> > > > > > as vblank-work.  This shouldn't be a big problem *yet*, due to limited
> > > > > > use of vblank-work at this point.  And if it could be arranged that
> > > > > > vblank-work is scheduled before signaling out-fences and/or sending
> > > > > > pageflip events, it could probably work ok to use a single priority
> > > > > > level for both commit-work and vblank-work.
> > > > >
> > > > > The part I don't like about this is that it all feels rather hacked
> > > > > together, and if we add more stuff (or there's some different thing in the
> > > > > system that also needs rt scheduling) then it doesn't compose.
> > > >
> > > > The ideal thing would be that userspace is in control of the
> > > > priorities.. the setclientcap approach seemed like a reasonable way to
> > > > give the drm-master a way to opt in.
> > > >
> > > > I suppose instead userspace could use sched_setscheduler().. but that
> > > > would require userspace to be root, and would require some way to find
> > > > the tid.
> > >
> > > Userspace already needs that for the SCHED_FIFO for surface-flinger.
> > > Or is the problem that CAP_SYS_NICE is only good for your own
> > > processes?
> >
> > tbh, I'm not completely sure offhand what gives surfaceflinger
> > permission to set itself SCHED_FIFO
> >
> > (But on CrOS there are a few more pieces to the puzzle)
> >
> > > Other question I have for this is whether there's any recommendations
> > > for naming the kthreads (since I guess that name is what becomes the
> > > uapi for userspace to control this)?
> > >
> > > Otherwise I think "userspace calls sched_setscheduler on the right
> > > kthreads" sounds like a good interface, since it lets userspace decide
> > > how it all needs to fit together and compose. Anything we hard-code in
> > > an ioctl is kinda lost cause. And we can choose the default values to
> > > work reasonably well when the compositor runs at normal priority
> > > (lowest niceness or something like that for the commit work).
> >
> > I don't really like the naming convention approach.. what is to stop
> > some unrelated process to name it's thread the same thing to get a
> > SCHED_FIFO boost..
> >
> > But we can stick with my idea to expose the thread id as a read-only
> > CRTC property, for userspace to find the things to call
> > sched_setscheduler() on.  If for whatever reason the drm master is not
> > privileged (or is running in a sandbox, etc), a small helper that has
> > the necessary permissions could open the drm device to find the CRTC
> > thread-ids and call sched_setscheduler()..
>
> Hm thread ids don't translate too well across PID namespaces I think ...
> So that's another can of worms. And pidfd doesn't really work as a
> property.

hmm, I was kinda hoping there was already a solution for translating
thread-id's, but hadn't had a chance to dig through it yet

> I also thought kernel threads can be distinguished from others, so
> userspace shouldn't be able to sneak in and get elevated by accident.

I guess maybe you could look at the parent?  I still would like to
think that we could come up with something a bit less shaking than
matching thread names by regexp..

BR,
-R

> -Daniel
>
> >
> > BR,
> > -R
> >
> > > -Daniel
> > >
> > > > Is there some way we could arrange for the per-crtc kthread's to be
> > > > owned by the drm master?  That would solve the "must be root" issue.
> > > > And since the target audience is an atomic userspace, I suppose we
> > > > could expose the tid as a read-only property on the crtc?
> > > >
> > > > BR,
> > > > -R
> > > >
> > > > > So question to rt/worker folks: What's the best way to let userspace set
> > > > > the scheduling mode and priorities of things the kernel does on its
> > > > > behalf? Surely we're not the first ones where if userspace runs with some
> > > > > rt priority it'll starve out the kernel workers that it needs. Hardcoding
> > > > > something behind a subsystem ioctl (which just means every time userspace
> > > > > changes what it does, we need a new such flag or mode) can't be the right
> > > > > thing.
> > > > >
> > > > > Peter, Tejun?
> > > > >
> > > > > Thanks, Daniel
> > > > >
> > > > > >
> > > > > > Rob Clark (3):
> > > > > >   drm/crtc: Introduce per-crtc kworker
> > > > > >   drm/atomic: Use kthread worker for nonblocking commits
> > > > > >   drm: Add a client-cap to set scheduling mode
> > > > > >
> > > > > >  drivers/gpu/drm/drm_atomic_helper.c | 13 ++++++----
> > > > > >  drivers/gpu/drm/drm_auth.c          |  4 ++++
> > > > > >  drivers/gpu/drm/drm_crtc.c          | 37 +++++++++++++++++++++++++++++
> > > > > >  drivers/gpu/drm/drm_ioctl.c         | 13 ++++++++++
> > > > > >  include/drm/drm_atomic.h            | 31 ++++++++++++++++++++++++
> > > > > >  include/drm/drm_crtc.h              | 10 ++++++++
> > > > > >  include/uapi/drm/drm.h              | 13 ++++++++++
> > > > > >  7 files changed, 117 insertions(+), 4 deletions(-)
> > > > > >
> > > > > > --
> > > > > > 2.26.2
> > > > > >
> > > > > > _______________________________________________
> > > > > > dri-devel mailing list
> > > > > > dri-devel@lists.freedesktop.org
> > > > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > > > >
> > > > > --
> > > > > Daniel Vetter
> > > > > Software Engineer, Intel Corporation
> > > > > http://blog.ffwll.ch
> > > > _______________________________________________
> > > > dri-devel mailing list
> > > > dri-devel@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > >
> > >
> > >
> > > --
> > > Daniel Vetter
> > > Software Engineer, Intel Corporation
> > > http://blog.ffwll.ch
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

Re: [PATCH 0/3] drm: commit_work scheduling

[Daniel Vetter]

On Wed, Sep 23, 2020 at 07:33:17PM -0700, Rob Clark wrote:
> On Wed, Sep 23, 2020 at 8:25 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> >
> > On Tue, Sep 22, 2020 at 07:48:10AM -0700, Rob Clark wrote:
> > > On Mon, Sep 21, 2020 at 11:59 PM Daniel Vetter <daniel@ffwll.ch> wrote:
> > > >
> > > > On Mon, Sep 21, 2020 at 5:16 PM Rob Clark <robdclark@gmail.com> wrote:
> > > > >
> > > > > On Mon, Sep 21, 2020 at 2:21 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> > > > > >
> > > > > > On Sat, Sep 19, 2020 at 12:37:23PM -0700, Rob Clark wrote:
> > > > > > > From: Rob Clark <robdclark@chromium.org>
> > > > > > >
> > > > > > > The android userspace treats the display pipeline as a realtime problem.
> > > > > > > And arguably, if your goal is to not miss frame deadlines (ie. vblank),
> > > > > > > it is.  (See https://lwn.net/Articles/809545/ for the best explaination
> > > > > > > that I found.)
> > > > > > >
> > > > > > > But this presents a problem with using workqueues for non-blocking
> > > > > > > atomic commit_work(), because the SCHED_FIFO userspace thread(s) can
> > > > > > > preempt the worker.  Which is not really the outcome you want.. once
> > > > > > > the required fences are scheduled, you want to push the atomic commit
> > > > > > > down to hw ASAP.
> > > > > > >
> > > > > > > But the decision of whether commit_work should be RT or not really
> > > > > > > depends on what userspace is doing.  For a pure CFS userspace display
> > > > > > > pipeline, commit_work() should remain SCHED_NORMAL.
> > > > > > >
> > > > > > > To handle this, convert non-blocking commit_work() to use per-CRTC
> > > > > > > kthread workers, instead of system_unbound_wq.  Per-CRTC workers are
> > > > > > > used to avoid serializing commits when userspace is using a per-CRTC
> > > > > > > update loop.
> > > > > > >
> > > > > > > A client-cap is introduced so that userspace can opt-in to SCHED_FIFO
> > > > > > > priority commit work.
> > > > > > >
> > > > > > > A potential issue is that since 616d91b68cd ("sched: Remove
> > > > > > > sched_setscheduler*() EXPORTs") we have limited RT priority levels,
> > > > > > > meaning that commit_work() ends up running at the same priority level
> > > > > > > as vblank-work.  This shouldn't be a big problem *yet*, due to limited
> > > > > > > use of vblank-work at this point.  And if it could be arranged that
> > > > > > > vblank-work is scheduled before signaling out-fences and/or sending
> > > > > > > pageflip events, it could probably work ok to use a single priority
> > > > > > > level for both commit-work and vblank-work.
> > > > > >
> > > > > > The part I don't like about this is that it all feels rather hacked
> > > > > > together, and if we add more stuff (or there's some different thing in the
> > > > > > system that also needs rt scheduling) then it doesn't compose.
> > > > >
> > > > > The ideal thing would be that userspace is in control of the
> > > > > priorities.. the setclientcap approach seemed like a reasonable way to
> > > > > give the drm-master a way to opt in.
> > > > >
> > > > > I suppose instead userspace could use sched_setscheduler().. but that
> > > > > would require userspace to be root, and would require some way to find
> > > > > the tid.
> > > >
> > > > Userspace already needs that for the SCHED_FIFO for surface-flinger.
> > > > Or is the problem that CAP_SYS_NICE is only good for your own
> > > > processes?
> > >
> > > tbh, I'm not completely sure offhand what gives surfaceflinger
> > > permission to set itself SCHED_FIFO
> > >
> > > (But on CrOS there are a few more pieces to the puzzle)
> > >
> > > > Other question I have for this is whether there's any recommendations
> > > > for naming the kthreads (since I guess that name is what becomes the
> > > > uapi for userspace to control this)?
> > > >
> > > > Otherwise I think "userspace calls sched_setscheduler on the right
> > > > kthreads" sounds like a good interface, since it lets userspace decide
> > > > how it all needs to fit together and compose. Anything we hard-code in
> > > > an ioctl is kinda lost cause. And we can choose the default values to
> > > > work reasonably well when the compositor runs at normal priority
> > > > (lowest niceness or something like that for the commit work).
> > >
> > > I don't really like the naming convention approach.. what is to stop
> > > some unrelated process to name it's thread the same thing to get a
> > > SCHED_FIFO boost..
> > >
> > > But we can stick with my idea to expose the thread id as a read-only
> > > CRTC property, for userspace to find the things to call
> > > sched_setscheduler() on.  If for whatever reason the drm master is not
> > > privileged (or is running in a sandbox, etc), a small helper that has
> > > the necessary permissions could open the drm device to find the CRTC
> > > thread-ids and call sched_setscheduler()..
> >
> > Hm thread ids don't translate too well across PID namespaces I think ...
> > So that's another can of worms. And pidfd doesn't really work as a
> > property.
> 
> hmm, I was kinda hoping there was already a solution for translating
> thread-id's, but hadn't had a chance to dig through it yet

You can translate them, and it happens automatically in process context
(iirc at least). But when we set the read-only prop we don't know which
process namespace the compositor is sitting in, so that translation isn't
doing us any good.

I think there's a root namespace that the kernel uses, but tbh I'm not
sure how this all works.

> > I also thought kernel threads can be distinguished from others, so
> > userspace shouldn't be able to sneak in and get elevated by accident.
> 
> I guess maybe you could look at the parent?  I still would like to
> think that we could come up with something a bit less shaking than
> matching thread names by regexp..

ps marks up kernel threads with [], so there is a way. But I haven't
looked at what it is exactly that tells kernel threads apart from others.

But aside from that sounds like "match right kernel thread with regex and
set its scheduler class" is how this is currently done, if I'm
understanding what Tejun and Peter said correctly.

Not pretty, but also *shrug* ...
-Daniel
 
> BR,
> -R
> 
> > -Daniel
> >
> > >
> > > BR,
> > > -R
> > >
> > > > -Daniel
> > > >
> > > > > Is there some way we could arrange for the per-crtc kthread's to be
> > > > > owned by the drm master?  That would solve the "must be root" issue.
> > > > > And since the target audience is an atomic userspace, I suppose we
> > > > > could expose the tid as a read-only property on the crtc?
> > > > >
> > > > > BR,
> > > > > -R
> > > > >
> > > > > > So question to rt/worker folks: What's the best way to let userspace set
> > > > > > the scheduling mode and priorities of things the kernel does on its
> > > > > > behalf? Surely we're not the first ones where if userspace runs with some
> > > > > > rt priority it'll starve out the kernel workers that it needs. Hardcoding
> > > > > > something behind a subsystem ioctl (which just means every time userspace
> > > > > > changes what it does, we need a new such flag or mode) can't be the right
> > > > > > thing.
> > > > > >
> > > > > > Peter, Tejun?
> > > > > >
> > > > > > Thanks, Daniel
> > > > > >
> > > > > > >
> > > > > > > Rob Clark (3):
> > > > > > >   drm/crtc: Introduce per-crtc kworker
> > > > > > >   drm/atomic: Use kthread worker for nonblocking commits
> > > > > > >   drm: Add a client-cap to set scheduling mode
> > > > > > >
> > > > > > >  drivers/gpu/drm/drm_atomic_helper.c | 13 ++++++----
> > > > > > >  drivers/gpu/drm/drm_auth.c          |  4 ++++
> > > > > > >  drivers/gpu/drm/drm_crtc.c          | 37 +++++++++++++++++++++++++++++
> > > > > > >  drivers/gpu/drm/drm_ioctl.c         | 13 ++++++++++
> > > > > > >  include/drm/drm_atomic.h            | 31 ++++++++++++++++++++++++
> > > > > > >  include/drm/drm_crtc.h              | 10 ++++++++
> > > > > > >  include/uapi/drm/drm.h              | 13 ++++++++++
> > > > > > >  7 files changed, 117 insertions(+), 4 deletions(-)
> > > > > > >
> > > > > > > --
> > > > > > > 2.26.2
> > > > > > >
> > > > > > > _______________________________________________
> > > > > > > dri-devel mailing list
> > > > > > > dri-devel@lists.freedesktop.org
> > > > > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > > > > >
> > > > > > --
> > > > > > Daniel Vetter
> > > > > > Software Engineer, Intel Corporation
> > > > > > http://blog.ffwll.ch
> > > > > _______________________________________________
> > > > > dri-devel mailing list
> > > > > dri-devel@lists.freedesktop.org
> > > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > > >
> > > >
> > > >
> > > > --
> > > > Daniel Vetter
> > > > Software Engineer, Intel Corporation
> > > > http://blog.ffwll.ch
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 0/3] drm: commit_work scheduling

[Rob Clark]

On Thu, Sep 24, 2020 at 1:49 AM Daniel Vetter <daniel@ffwll.ch> wrote:
>
> On Wed, Sep 23, 2020 at 07:33:17PM -0700, Rob Clark wrote:
> > On Wed, Sep 23, 2020 at 8:25 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> > >
> > > On Tue, Sep 22, 2020 at 07:48:10AM -0700, Rob Clark wrote:
> > > > On Mon, Sep 21, 2020 at 11:59 PM Daniel Vetter <daniel@ffwll.ch> wrote:
> > > > >
> > > > > On Mon, Sep 21, 2020 at 5:16 PM Rob Clark <robdclark@gmail.com> wrote:
> > > > > >
> > > > > > On Mon, Sep 21, 2020 at 2:21 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> > > > > > >
> > > > > > > On Sat, Sep 19, 2020 at 12:37:23PM -0700, Rob Clark wrote:
> > > > > > > > From: Rob Clark <robdclark@chromium.org>
> > > > > > > >
> > > > > > > > The android userspace treats the display pipeline as a realtime problem.
> > > > > > > > And arguably, if your goal is to not miss frame deadlines (ie. vblank),
> > > > > > > > it is.  (See https://lwn.net/Articles/809545/ for the best explaination
> > > > > > > > that I found.)
> > > > > > > >
> > > > > > > > But this presents a problem with using workqueues for non-blocking
> > > > > > > > atomic commit_work(), because the SCHED_FIFO userspace thread(s) can
> > > > > > > > preempt the worker.  Which is not really the outcome you want.. once
> > > > > > > > the required fences are scheduled, you want to push the atomic commit
> > > > > > > > down to hw ASAP.
> > > > > > > >
> > > > > > > > But the decision of whether commit_work should be RT or not really
> > > > > > > > depends on what userspace is doing.  For a pure CFS userspace display
> > > > > > > > pipeline, commit_work() should remain SCHED_NORMAL.
> > > > > > > >
> > > > > > > > To handle this, convert non-blocking commit_work() to use per-CRTC
> > > > > > > > kthread workers, instead of system_unbound_wq.  Per-CRTC workers are
> > > > > > > > used to avoid serializing commits when userspace is using a per-CRTC
> > > > > > > > update loop.
> > > > > > > >
> > > > > > > > A client-cap is introduced so that userspace can opt-in to SCHED_FIFO
> > > > > > > > priority commit work.
> > > > > > > >
> > > > > > > > A potential issue is that since 616d91b68cd ("sched: Remove
> > > > > > > > sched_setscheduler*() EXPORTs") we have limited RT priority levels,
> > > > > > > > meaning that commit_work() ends up running at the same priority level
> > > > > > > > as vblank-work.  This shouldn't be a big problem *yet*, due to limited
> > > > > > > > use of vblank-work at this point.  And if it could be arranged that
> > > > > > > > vblank-work is scheduled before signaling out-fences and/or sending
> > > > > > > > pageflip events, it could probably work ok to use a single priority
> > > > > > > > level for both commit-work and vblank-work.
> > > > > > >
> > > > > > > The part I don't like about this is that it all feels rather hacked
> > > > > > > together, and if we add more stuff (or there's some different thing in the
> > > > > > > system that also needs rt scheduling) then it doesn't compose.
> > > > > >
> > > > > > The ideal thing would be that userspace is in control of the
> > > > > > priorities.. the setclientcap approach seemed like a reasonable way to
> > > > > > give the drm-master a way to opt in.
> > > > > >
> > > > > > I suppose instead userspace could use sched_setscheduler().. but that
> > > > > > would require userspace to be root, and would require some way to find
> > > > > > the tid.
> > > > >
> > > > > Userspace already needs that for the SCHED_FIFO for surface-flinger.
> > > > > Or is the problem that CAP_SYS_NICE is only good for your own
> > > > > processes?
> > > >
> > > > tbh, I'm not completely sure offhand what gives surfaceflinger
> > > > permission to set itself SCHED_FIFO
> > > >
> > > > (But on CrOS there are a few more pieces to the puzzle)
> > > >
> > > > > Other question I have for this is whether there's any recommendations
> > > > > for naming the kthreads (since I guess that name is what becomes the
> > > > > uapi for userspace to control this)?
> > > > >
> > > > > Otherwise I think "userspace calls sched_setscheduler on the right
> > > > > kthreads" sounds like a good interface, since it lets userspace decide
> > > > > how it all needs to fit together and compose. Anything we hard-code in
> > > > > an ioctl is kinda lost cause. And we can choose the default values to
> > > > > work reasonably well when the compositor runs at normal priority
> > > > > (lowest niceness or something like that for the commit work).
> > > >
> > > > I don't really like the naming convention approach.. what is to stop
> > > > some unrelated process to name it's thread the same thing to get a
> > > > SCHED_FIFO boost..
> > > >
> > > > But we can stick with my idea to expose the thread id as a read-only
> > > > CRTC property, for userspace to find the things to call
> > > > sched_setscheduler() on.  If for whatever reason the drm master is not
> > > > privileged (or is running in a sandbox, etc), a small helper that has
> > > > the necessary permissions could open the drm device to find the CRTC
> > > > thread-ids and call sched_setscheduler()..
> > >
> > > Hm thread ids don't translate too well across PID namespaces I think ...
> > > So that's another can of worms. And pidfd doesn't really work as a
> > > property.
> >
> > hmm, I was kinda hoping there was already a solution for translating
> > thread-id's, but hadn't had a chance to dig through it yet
>
> You can translate them, and it happens automatically in process context
> (iirc at least). But when we set the read-only prop we don't know which
> process namespace the compositor is sitting in, so that translation isn't
> doing us any good.

Well, that only requires writing some code.. when I mentioned
read-only, I just meant that it is read-only from the userspace
standpoint.  But we would need some hook when the property is read to
do the translation so userspace sees the appropriate value

BR,
-R

> I think there's a root namespace that the kernel uses, but tbh I'm not
> sure how this all works.
>
> > > I also thought kernel threads can be distinguished from others, so
> > > userspace shouldn't be able to sneak in and get elevated by accident.
> >
> > I guess maybe you could look at the parent?  I still would like to
> > think that we could come up with something a bit less shaking than
> > matching thread names by regexp..
>
> ps marks up kernel threads with [], so there is a way. But I haven't
> looked at what it is exactly that tells kernel threads apart from others.
>
> But aside from that sounds like "match right kernel thread with regex and
> set its scheduler class" is how this is currently done, if I'm
> understanding what Tejun and Peter said correctly.
>
> Not pretty, but also *shrug* ...
> -Daniel
>
> > BR,
> > -R
> >
> > > -Daniel
> > >
> > > >
> > > > BR,
> > > > -R
> > > >
> > > > > -Daniel
> > > > >
> > > > > > Is there some way we could arrange for the per-crtc kthread's to be
> > > > > > owned by the drm master?  That would solve the "must be root" issue.
> > > > > > And since the target audience is an atomic userspace, I suppose we
> > > > > > could expose the tid as a read-only property on the crtc?
> > > > > >
> > > > > > BR,
> > > > > > -R
> > > > > >
> > > > > > > So question to rt/worker folks: What's the best way to let userspace set
> > > > > > > the scheduling mode and priorities of things the kernel does on its
> > > > > > > behalf? Surely we're not the first ones where if userspace runs with some
> > > > > > > rt priority it'll starve out the kernel workers that it needs. Hardcoding
> > > > > > > something behind a subsystem ioctl (which just means every time userspace
> > > > > > > changes what it does, we need a new such flag or mode) can't be the right
> > > > > > > thing.
> > > > > > >
> > > > > > > Peter, Tejun?
> > > > > > >
> > > > > > > Thanks, Daniel
> > > > > > >
> > > > > > > >
> > > > > > > > Rob Clark (3):
> > > > > > > >   drm/crtc: Introduce per-crtc kworker
> > > > > > > >   drm/atomic: Use kthread worker for nonblocking commits
> > > > > > > >   drm: Add a client-cap to set scheduling mode
> > > > > > > >
> > > > > > > >  drivers/gpu/drm/drm_atomic_helper.c | 13 ++++++----
> > > > > > > >  drivers/gpu/drm/drm_auth.c          |  4 ++++
> > > > > > > >  drivers/gpu/drm/drm_crtc.c          | 37 +++++++++++++++++++++++++++++
> > > > > > > >  drivers/gpu/drm/drm_ioctl.c         | 13 ++++++++++
> > > > > > > >  include/drm/drm_atomic.h            | 31 ++++++++++++++++++++++++
> > > > > > > >  include/drm/drm_crtc.h              | 10 ++++++++
> > > > > > > >  include/uapi/drm/drm.h              | 13 ++++++++++
> > > > > > > >  7 files changed, 117 insertions(+), 4 deletions(-)
> > > > > > > >
> > > > > > > > --
> > > > > > > > 2.26.2
> > > > > > > >
> > > > > > > > _______________________________________________
> > > > > > > > dri-devel mailing list
> > > > > > > > dri-devel@lists.freedesktop.org
> > > > > > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > > > > > >
> > > > > > > --
> > > > > > > Daniel Vetter
> > > > > > > Software Engineer, Intel Corporation
> > > > > > > http://blog.ffwll.ch
> > > > > > _______________________________________________
> > > > > > dri-devel mailing list
> > > > > > dri-devel@lists.freedesktop.org
> > > > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > > > >
> > > > >
> > > > >
> > > > > --
> > > > > Daniel Vetter
> > > > > Software Engineer, Intel Corporation
> > > > > http://blog.ffwll.ch
> > >
> > > --
> > > Daniel Vetter
> > > Software Engineer, Intel Corporation
> > > http://blog.ffwll.ch
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

Re: [PATCH 0/3] drm: commit_work scheduling

[Qais Yousef]

On 09/24/20 10:49, Daniel Vetter wrote:

[...]

> > > I also thought kernel threads can be distinguished from others, so
> > > userspace shouldn't be able to sneak in and get elevated by accident.
> > 
> > I guess maybe you could look at the parent?  I still would like to
> > think that we could come up with something a bit less shaking than
> > matching thread names by regexp..
> 
> ps marks up kernel threads with [], so there is a way. But I haven't
> looked at what it is exactly that tells kernel threads apart from others.
> 
> But aside from that sounds like "match right kernel thread with regex and
> set its scheduler class" is how this is currently done, if I'm
> understanding what Tejun and Peter said correctly.
> 
> Not pretty, but also *shrug* ...

Isn't there a real danger that a sneaky application names its threads to match
this regex and get a free promotion to RT without having the capability to do
so?

Cheers

--
Qais Yousef

Re: [PATCH 0/3] drm: commit_work scheduling

[Daniel Vetter]

On Thu, Sep 24, 2020 at 05:15:00PM +0100, Qais Yousef wrote:
> On 09/24/20 10:49, Daniel Vetter wrote:
> 
> [...]
> 
> > > > I also thought kernel threads can be distinguished from others, so
> > > > userspace shouldn't be able to sneak in and get elevated by accident.
> > > 
> > > I guess maybe you could look at the parent?  I still would like to
> > > think that we could come up with something a bit less shaking than
> > > matching thread names by regexp..
> > 
> > ps marks up kernel threads with [], so there is a way. But I haven't
> > looked at what it is exactly that tells kernel threads apart from others.
> > 
> > But aside from that sounds like "match right kernel thread with regex and
> > set its scheduler class" is how this is currently done, if I'm
> > understanding what Tejun and Peter said correctly.
> > 
> > Not pretty, but also *shrug* ...
> 
> Isn't there a real danger that a sneaky application names its threads to match
> this regex and get a free promotion to RT without having the capability to do
> so?

A sneaky application can't fake being a kernel thread, at least that's
what I thought. You need to check for that _and_ that the name matches.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch