Re: [RFC v5 02/10] drm/vblank: Add vblank works
On Mon, Jun 22, 2020 at 04:07:22PM -0400, Lyude Paul wrote:
> Add some kind of vblank workers. The interface is similar to regular
> delayed works, and is mostly based off kthread_work. It allows for
> scheduling delayed works that execute once a particular vblank sequence
> has passed. It also allows for accurate flushing of scheduled vblank
> works - in that flushing waits for both the vblank sequence and job
> execution to complete, or for the work to get cancelled - whichever
> comes first.
>
> Whatever hardware programming we do in the work must be fast (must at
> least complete during the vblank or scanout period, sometimes during the
> first few scanlines of the vblank). As such we use a high-priority
> per-CRTC thread to accomplish this.
>
> [based off patches from Ville Syrjälä ,
> change below to signoff later]
>
> Changes since v4:
> * Get rid of kthread interfaces we tried adding and move all of the
> locking into drm_vblank.c. For implementing drm_vblank_work_flush(),
> we now use a wait_queue and sequence counters in order to
> differentiate between multiple work item executions.
> * Get rid of drm_vblank_work_cancel() - this would have been pretty
> difficult to actually reimplement and it occurred to me that neither
> nouveau or i915 are even planning to use this function. Since there's
> also no async cancel function for most of the work interfaces in the
> kernel, it seems a bit unnecessary anyway.
> * Get rid of to_drm_vblank_work() since we now are also able to just
> pass the struct drm_vblank_work to work item callbacks anyway
> Changes since v3:
> * Use our own spinlocks, don't integrate so tightly with kthread_works
> Changes since v2:
> * Use kthread_workers instead of reinventing the wheel.
>
> Cc: Daniel Vetter
> Cc: Tejun Heo
> Cc: Ville Syrjälä
> Cc: dri-devel@lists.freedesktop.org
> Cc: nouv...@lists.freedesktop.org
> Signed-off-by: Lyude Paul
Ok, I think this melted my brain a bit, but I tried to review this
carefully. Bunch of suggestions to simplify the flow, but might also
simply be that I missed something and my suggestions dont work at all.
Thanks for doing all this!
Cheers, Daniel
> ---
> drivers/gpu/drm/drm_vblank.c | 266 +++
> include/drm/drm_vblank.h | 49 +++
> 2 files changed, 315 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> index ce5c1e1d29963..4d76eb2fed5e9 100644
> --- a/drivers/gpu/drm/drm_vblank.c
> +++ b/drivers/gpu/drm/drm_vblank.c
> @@ -25,7 +25,9 @@
> */
>
> #include
> +#include
> #include
> +#include
>
> #include
> #include
> @@ -155,6 +157,8 @@
> static bool
> drm_get_last_vbltimestamp(struct drm_device *dev, unsigned int pipe,
> ktime_t *tvblank, bool in_vblank_irq);
> +static int drm_vblank_get(struct drm_device *dev, unsigned int pipe);
> +static void drm_vblank_put(struct drm_device *dev, unsigned int pipe);
>
> static unsigned int drm_timestamp_precision = 20; /* Default to 20 usecs. */
>
> @@ -490,6 +494,35 @@ static void vblank_disable_fn(struct timer_list *t)
> spin_unlock_irqrestore(>vbl_lock, irqflags);
> }
>
> +static void drm_vblank_work_release(struct drm_vblank_crtc *vblank)
> +{
> + struct kthread_worker *worker = vblank->worker;
> + struct drm_vblank_work *work, *tmp;
> + bool wake = false;
> +
> + if (!worker)
> + return;
> +
> + spin_lock_irq(>work_lock);
> + vblank->worker = NULL;
> +
> + list_for_each_entry_safe(work, tmp, >pending_work, node) {
> + drm_vblank_put(vblank->dev, vblank->pipe);
> + list_del(>node);
> +
> + if (!--work->pending) {
> + write_seqcount_invalidate(>seqcount);
> + wake = true;
> + }
> + }
> +
> + spin_unlock_irq(>work_lock);
> +
> + if (wake)
> + wake_up_all(>work_wait_queue);
So drmm_ is for cleaning up software stuff, but the above is for cleaning
up vblank callbacks and things. Those should all be cleaned up and stop
after the hardware is gone.
If you look at drm_crtc_vblank_off, that already has code to send out any
pending vblank events. I think that's also where we should clean up any
leaked vblank worker things, to make sure they don't get re-queued. Once
the vblank is off it shouldn't be possible to have new stuff enqueued.
In vblank_off we probably want an additional kthrea_flush_worker() to make
sure nothing spills out.
> + kthread_destroy_worker(worker);
This one looks good here I think, but maybe just inline it since it's
just one line after you moved the cleanup.
> +}
> +
> static void drm_vblank_init_release(struct drm_device *dev, void *ptr)
> {
> struct drm_vblank_crtc *vblank = ptr;
> @@ -497,9 +530,66 @@ static void drm_vblank_init_release(struct drm_device
> *dev, void *ptr)
> drm_WARN_ON(dev, READ_ONCE(vblank->enabled) &&
>
[RFC v5 02/10] drm/vblank: Add vblank works
Add some kind of vblank workers. The interface is similar to regular
delayed works, and is mostly based off kthread_work. It allows for
scheduling delayed works that execute once a particular vblank sequence
has passed. It also allows for accurate flushing of scheduled vblank
works - in that flushing waits for both the vblank sequence and job
execution to complete, or for the work to get cancelled - whichever
comes first.
Whatever hardware programming we do in the work must be fast (must at
least complete during the vblank or scanout period, sometimes during the
first few scanlines of the vblank). As such we use a high-priority
per-CRTC thread to accomplish this.
[based off patches from Ville Syrjälä ,
change below to signoff later]
Changes since v4:
* Get rid of kthread interfaces we tried adding and move all of the
locking into drm_vblank.c. For implementing drm_vblank_work_flush(),
we now use a wait_queue and sequence counters in order to
differentiate between multiple work item executions.
* Get rid of drm_vblank_work_cancel() - this would have been pretty
difficult to actually reimplement and it occurred to me that neither
nouveau or i915 are even planning to use this function. Since there's
also no async cancel function for most of the work interfaces in the
kernel, it seems a bit unnecessary anyway.
* Get rid of to_drm_vblank_work() since we now are also able to just
pass the struct drm_vblank_work to work item callbacks anyway
Changes since v3:
* Use our own spinlocks, don't integrate so tightly with kthread_works
Changes since v2:
* Use kthread_workers instead of reinventing the wheel.
Cc: Daniel Vetter
Cc: Tejun Heo
Cc: Ville Syrjälä
Cc: dri-devel@lists.freedesktop.org
Cc: nouv...@lists.freedesktop.org
Signed-off-by: Lyude Paul
---
drivers/gpu/drm/drm_vblank.c | 266 +++
include/drm/drm_vblank.h | 49 +++
2 files changed, 315 insertions(+)
diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
index ce5c1e1d29963..4d76eb2fed5e9 100644
--- a/drivers/gpu/drm/drm_vblank.c
+++ b/drivers/gpu/drm/drm_vblank.c
@@ -25,7 +25,9 @@
*/
#include
+#include
#include
+#include
#include
#include
@@ -155,6 +157,8 @@
static bool
drm_get_last_vbltimestamp(struct drm_device *dev, unsigned int pipe,
ktime_t *tvblank, bool in_vblank_irq);
+static int drm_vblank_get(struct drm_device *dev, unsigned int pipe);
+static void drm_vblank_put(struct drm_device *dev, unsigned int pipe);
static unsigned int drm_timestamp_precision = 20; /* Default to 20 usecs. */
@@ -490,6 +494,35 @@ static void vblank_disable_fn(struct timer_list *t)
spin_unlock_irqrestore(>vbl_lock, irqflags);
}
+static void drm_vblank_work_release(struct drm_vblank_crtc *vblank)
+{
+ struct kthread_worker *worker = vblank->worker;
+ struct drm_vblank_work *work, *tmp;
+ bool wake = false;
+
+ if (!worker)
+ return;
+
+ spin_lock_irq(>work_lock);
+ vblank->worker = NULL;
+
+ list_for_each_entry_safe(work, tmp, >pending_work, node) {
+ drm_vblank_put(vblank->dev, vblank->pipe);
+ list_del(>node);
+
+ if (!--work->pending) {
+ write_seqcount_invalidate(>seqcount);
+ wake = true;
+ }
+ }
+
+ spin_unlock_irq(>work_lock);
+
+ if (wake)
+ wake_up_all(>work_wait_queue);
+ kthread_destroy_worker(worker);
+}
+
static void drm_vblank_init_release(struct drm_device *dev, void *ptr)
{
struct drm_vblank_crtc *vblank = ptr;
@@ -497,9 +530,66 @@ static void drm_vblank_init_release(struct drm_device
*dev, void *ptr)
drm_WARN_ON(dev, READ_ONCE(vblank->enabled) &&
drm_core_check_feature(dev, DRIVER_MODESET));
+ drm_vblank_work_release(vblank);
del_timer_sync(>disable_timer);
}
+static void vblank_work_fn(struct kthread_work *base)
+{
+ struct drm_vblank_work *work =
+ container_of(base, struct drm_vblank_work, base);
+ struct drm_vblank_crtc *vblank = work->vblank;
+
+ work->func(work);
+
+ spin_lock_irq(>work_lock);
+ work->pending--;
+
+ write_seqcount_invalidate(>seqcount);
+ wake_up_all(>work_wait_queue);
+ spin_unlock_irq(>work_lock);
+}
+
+/**
+ * drm_vblank_work_init - initialize a vblank work item
+ * @work: vblank work item
+ * @crtc: CRTC whose vblank will trigger the work execution
+ * @func: work function to be executed
+ *
+ * Initialize a vblank work item for a specific crtc.
+ */
+void drm_vblank_work_init(struct drm_vblank_work *work, struct drm_crtc *crtc,
+ void (*func)(struct drm_vblank_work *work))
+{
+ kthread_init_work(>base, vblank_work_fn);
+ work->func = func;
+ INIT_LIST_HEAD(>node);
+ seqcount_init(>seqcount);
+ work->vblank =
