[PATCH 0/1] drm/lima: implement the file flush callback

[PATCH 0/1] drm/lima: implement the file flush callback

[Erico Nunes]

This prevents a user bug in lima reported in [1].
I'm not sure this is the entire bug fix, but Christian had also
suggested we implement this callback in a previous review [2].
I took the amdgpu implementation as a base for this.

I noticed that most other drivers don't implement this though, in
particular the other ones for embedded GPUs. Should they, or does
this indicate that we have a different bug to be fixed in lima?

[1] https://gitlab.freedesktop.org/mesa/mesa/-/issues/12007
[2] https://lore.kernel.org/all/20230606143247.433018-1-nunes.erico@gmail.com/T/#mf5c7a2492201c8ec82bee47eb5615714d5c5aac2

Erico Nunes (1):
  drm/lima: implement the file flush callback

 drivers/gpu/drm/lima/lima_ctx.c | 18 ++++++++++++++++++
 drivers/gpu/drm/lima/lima_ctx.h |  1 +
 drivers/gpu/drm/lima/lima_drv.c | 17 ++++++++++++++++-
 3 files changed, 35 insertions(+), 1 deletion(-)

-- 
2.49.0

[PATCH 1/1] drm/lima: implement the file flush callback

[Erico Nunes]

With this callback implemented, a terminating application will wait for
the sched entity to be flushed out to the hardware and cancel all other
pending jobs before destroying its context.
This prevents applications with multiple contexts from running into a
race condition between running tasks and context destroy when
terminating.

Signed-off-by: Erico Nunes <nunes.erico@gmail.com>
---
 drivers/gpu/drm/lima/lima_ctx.c | 18 ++++++++++++++++++
 drivers/gpu/drm/lima/lima_ctx.h |  1 +
 drivers/gpu/drm/lima/lima_drv.c | 17 ++++++++++++++++-
 3 files changed, 35 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/lima/lima_ctx.c b/drivers/gpu/drm/lima/lima_ctx.c
index 0e668fc1e0f9..e8fb5788ca69 100644
--- a/drivers/gpu/drm/lima/lima_ctx.c
+++ b/drivers/gpu/drm/lima/lima_ctx.c
@@ -100,3 +100,21 @@ void lima_ctx_mgr_fini(struct lima_ctx_mgr *mgr)
 	xa_destroy(&mgr->handles);
 	mutex_destroy(&mgr->lock);
 }
+
+long lima_ctx_mgr_flush(struct lima_ctx_mgr *mgr, long timeout)
+{
+	struct lima_ctx *ctx;
+	unsigned long id;
+
+	mutex_lock(&mgr->lock);
+	xa_for_each(&mgr->handles, id, ctx) {
+		for (int i = 0; i < lima_pipe_num; i++) {
+			struct lima_sched_context *context = &ctx->context[i];
+			struct drm_sched_entity *entity = &context->base;
+
+			timeout = drm_sched_entity_flush(entity, timeout);
+		}
+	}
+	mutex_unlock(&mgr->lock);
+	return timeout;
+}
diff --git a/drivers/gpu/drm/lima/lima_ctx.h b/drivers/gpu/drm/lima/lima_ctx.h
index 5b1063ce968b..ff133db6ae4c 100644
--- a/drivers/gpu/drm/lima/lima_ctx.h
+++ b/drivers/gpu/drm/lima/lima_ctx.h
@@ -30,5 +30,6 @@ struct lima_ctx *lima_ctx_get(struct lima_ctx_mgr *mgr, u32 id);
 void lima_ctx_put(struct lima_ctx *ctx);
 void lima_ctx_mgr_init(struct lima_ctx_mgr *mgr);
 void lima_ctx_mgr_fini(struct lima_ctx_mgr *mgr);
+long lima_ctx_mgr_flush(struct lima_ctx_mgr *mgr, long timeout);
 
 #endif
diff --git a/drivers/gpu/drm/lima/lima_drv.c b/drivers/gpu/drm/lima/lima_drv.c
index 11ace5cebf4c..08169b0d9c28 100644
--- a/drivers/gpu/drm/lima/lima_drv.c
+++ b/drivers/gpu/drm/lima/lima_drv.c
@@ -254,7 +254,22 @@ static const struct drm_ioctl_desc lima_drm_driver_ioctls[] = {
 	DRM_IOCTL_DEF_DRV(LIMA_CTX_FREE, lima_ioctl_ctx_free, DRM_RENDER_ALLOW),
 };
 
-DEFINE_DRM_GEM_FOPS(lima_drm_driver_fops);
+static int lima_drm_driver_flush(struct file *filp, fl_owner_t id)
+{
+	struct drm_file *file = filp->private_data;
+	struct lima_drm_priv *priv = file->driver_priv;
+	long timeout = MAX_WAIT_SCHED_ENTITY_Q_EMPTY;
+
+	timeout = lima_ctx_mgr_flush(&priv->ctx_mgr, timeout);
+
+	return timeout >= 0 ? 0 : timeout;
+}
+
+static const struct file_operations lima_drm_driver_fops = {
+	.owner = THIS_MODULE,
+	.flush = lima_drm_driver_flush,
+	DRM_GEM_FOPS,
+};
 
 /*
  * Changelog:
-- 
2.49.0

Re: [PATCH 1/1] drm/lima: implement the file flush callback

[Christian König]

Am 08.04.25 um 17:46 schrieb Erico Nunes:
> With this callback implemented, a terminating application will wait for
> the sched entity to be flushed out to the hardware and cancel all other
> pending jobs before destroying its context.
> This prevents applications with multiple contexts from running into a
> race condition between running tasks and context destroy when
> terminating.
>
> Signed-off-by: Erico Nunes <nunes.erico@gmail.com>

Looks perfectly valid to me and is most likely the right thing to do for other drivers as well.

Reviewed-by: Christian König <christian.koenig@amd.com>

> ---
>  drivers/gpu/drm/lima/lima_ctx.c | 18 ++++++++++++++++++
>  drivers/gpu/drm/lima/lima_ctx.h |  1 +
>  drivers/gpu/drm/lima/lima_drv.c | 17 ++++++++++++++++-
>  3 files changed, 35 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/lima/lima_ctx.c b/drivers/gpu/drm/lima/lima_ctx.c
> index 0e668fc1e0f9..e8fb5788ca69 100644
> --- a/drivers/gpu/drm/lima/lima_ctx.c
> +++ b/drivers/gpu/drm/lima/lima_ctx.c
> @@ -100,3 +100,21 @@ void lima_ctx_mgr_fini(struct lima_ctx_mgr *mgr)
>  	xa_destroy(&mgr->handles);
>  	mutex_destroy(&mgr->lock);
>  }
> +
> +long lima_ctx_mgr_flush(struct lima_ctx_mgr *mgr, long timeout)
> +{
> +	struct lima_ctx *ctx;
> +	unsigned long id;
> +
> +	mutex_lock(&mgr->lock);
> +	xa_for_each(&mgr->handles, id, ctx) {
> +		for (int i = 0; i < lima_pipe_num; i++) {
> +			struct lima_sched_context *context = &ctx->context[i];
> +			struct drm_sched_entity *entity = &context->base;
> +
> +			timeout = drm_sched_entity_flush(entity, timeout);
> +		}
> +	}
> +	mutex_unlock(&mgr->lock);
> +	return timeout;
> +}
> diff --git a/drivers/gpu/drm/lima/lima_ctx.h b/drivers/gpu/drm/lima/lima_ctx.h
> index 5b1063ce968b..ff133db6ae4c 100644
> --- a/drivers/gpu/drm/lima/lima_ctx.h
> +++ b/drivers/gpu/drm/lima/lima_ctx.h
> @@ -30,5 +30,6 @@ struct lima_ctx *lima_ctx_get(struct lima_ctx_mgr *mgr, u32 id);
>  void lima_ctx_put(struct lima_ctx *ctx);
>  void lima_ctx_mgr_init(struct lima_ctx_mgr *mgr);
>  void lima_ctx_mgr_fini(struct lima_ctx_mgr *mgr);
> +long lima_ctx_mgr_flush(struct lima_ctx_mgr *mgr, long timeout);
>  
>  #endif
> diff --git a/drivers/gpu/drm/lima/lima_drv.c b/drivers/gpu/drm/lima/lima_drv.c
> index 11ace5cebf4c..08169b0d9c28 100644
> --- a/drivers/gpu/drm/lima/lima_drv.c
> +++ b/drivers/gpu/drm/lima/lima_drv.c
> @@ -254,7 +254,22 @@ static const struct drm_ioctl_desc lima_drm_driver_ioctls[] = {
>  	DRM_IOCTL_DEF_DRV(LIMA_CTX_FREE, lima_ioctl_ctx_free, DRM_RENDER_ALLOW),
>  };
>  
> -DEFINE_DRM_GEM_FOPS(lima_drm_driver_fops);
> +static int lima_drm_driver_flush(struct file *filp, fl_owner_t id)
> +{
> +	struct drm_file *file = filp->private_data;
> +	struct lima_drm_priv *priv = file->driver_priv;
> +	long timeout = MAX_WAIT_SCHED_ENTITY_Q_EMPTY;
> +
> +	timeout = lima_ctx_mgr_flush(&priv->ctx_mgr, timeout);
> +
> +	return timeout >= 0 ? 0 : timeout;
> +}
> +
> +static const struct file_operations lima_drm_driver_fops = {
> +	.owner = THIS_MODULE,
> +	.flush = lima_drm_driver_flush,
> +	DRM_GEM_FOPS,
> +};
>  
>  /*
>   * Changelog:

Re: [PATCH 1/1] drm/lima: implement the file flush callback

[Qiang Yu]

On Tue, Apr 8, 2025 at 11:48 PM Erico Nunes <nunes.erico@gmail.com> wrote:
>
> With this callback implemented, a terminating application will wait for
> the sched entity to be flushed out to the hardware and cancel all other
> pending jobs before destroying its context.

We do flush when file release in lima_ctx_mgr_fini. Why do we wait here
in flush? What's the difference?

> This prevents applications with multiple contexts from running into a
> race condition between running tasks and context destroy when
> terminating.
>
> Signed-off-by: Erico Nunes <nunes.erico@gmail.com>
> ---
>  drivers/gpu/drm/lima/lima_ctx.c | 18 ++++++++++++++++++
>  drivers/gpu/drm/lima/lima_ctx.h |  1 +
>  drivers/gpu/drm/lima/lima_drv.c | 17 ++++++++++++++++-
>  3 files changed, 35 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/lima/lima_ctx.c b/drivers/gpu/drm/lima/lima_ctx.c
> index 0e668fc1e0f9..e8fb5788ca69 100644
> --- a/drivers/gpu/drm/lima/lima_ctx.c
> +++ b/drivers/gpu/drm/lima/lima_ctx.c
> @@ -100,3 +100,21 @@ void lima_ctx_mgr_fini(struct lima_ctx_mgr *mgr)
>         xa_destroy(&mgr->handles);
>         mutex_destroy(&mgr->lock);
>  }
> +
> +long lima_ctx_mgr_flush(struct lima_ctx_mgr *mgr, long timeout)
> +{
> +       struct lima_ctx *ctx;
> +       unsigned long id;
> +
> +       mutex_lock(&mgr->lock);
> +       xa_for_each(&mgr->handles, id, ctx) {
> +               for (int i = 0; i < lima_pipe_num; i++) {
> +                       struct lima_sched_context *context = &ctx->context[i];
> +                       struct drm_sched_entity *entity = &context->base;
> +
> +                       timeout = drm_sched_entity_flush(entity, timeout);
> +               }
> +       }
> +       mutex_unlock(&mgr->lock);
> +       return timeout;
> +}
> diff --git a/drivers/gpu/drm/lima/lima_ctx.h b/drivers/gpu/drm/lima/lima_ctx.h
> index 5b1063ce968b..ff133db6ae4c 100644
> --- a/drivers/gpu/drm/lima/lima_ctx.h
> +++ b/drivers/gpu/drm/lima/lima_ctx.h
> @@ -30,5 +30,6 @@ struct lima_ctx *lima_ctx_get(struct lima_ctx_mgr *mgr, u32 id);
>  void lima_ctx_put(struct lima_ctx *ctx);
>  void lima_ctx_mgr_init(struct lima_ctx_mgr *mgr);
>  void lima_ctx_mgr_fini(struct lima_ctx_mgr *mgr);
> +long lima_ctx_mgr_flush(struct lima_ctx_mgr *mgr, long timeout);
>
>  #endif
> diff --git a/drivers/gpu/drm/lima/lima_drv.c b/drivers/gpu/drm/lima/lima_drv.c
> index 11ace5cebf4c..08169b0d9c28 100644
> --- a/drivers/gpu/drm/lima/lima_drv.c
> +++ b/drivers/gpu/drm/lima/lima_drv.c
> @@ -254,7 +254,22 @@ static const struct drm_ioctl_desc lima_drm_driver_ioctls[] = {
>         DRM_IOCTL_DEF_DRV(LIMA_CTX_FREE, lima_ioctl_ctx_free, DRM_RENDER_ALLOW),
>  };
>
> -DEFINE_DRM_GEM_FOPS(lima_drm_driver_fops);
> +static int lima_drm_driver_flush(struct file *filp, fl_owner_t id)
> +{
> +       struct drm_file *file = filp->private_data;
> +       struct lima_drm_priv *priv = file->driver_priv;
> +       long timeout = MAX_WAIT_SCHED_ENTITY_Q_EMPTY;
> +
> +       timeout = lima_ctx_mgr_flush(&priv->ctx_mgr, timeout);
> +
> +       return timeout >= 0 ? 0 : timeout;
> +}
> +
> +static const struct file_operations lima_drm_driver_fops = {
> +       .owner = THIS_MODULE,
> +       .flush = lima_drm_driver_flush,
> +       DRM_GEM_FOPS,
> +};
>
>  /*
>   * Changelog:
> --
> 2.49.0
>

Re: [PATCH 1/1] drm/lima: implement the file flush callback

[Christian König]

Am 10.04.25 um 11:33 schrieb Qiang Yu:
> On Tue, Apr 8, 2025 at 11:48 PM Erico Nunes <nunes.erico@gmail.com> wrote:
>> With this callback implemented, a terminating application will wait for
>> the sched entity to be flushed out to the hardware and cancel all other
>> pending jobs before destroying its context.
> We do flush when file release in lima_ctx_mgr_fini. Why do we wait here
> in flush? What's the difference?

Waiting for submissions when you release the file descriptor is actually a bad idea since that can prevent SIGKILL from acting immediately. For example the OOM killer absolutely doesn't like that and eventually calls panic().

Flush is called either manually, on process termination or when you send a SIGTERM. This should then wait for any I/O to complete.

The idea is now that you can then still send a SIGKILL to abort waiting for the I/O as well and so get pending GPU operations not submitted to the HW.

The DRM scheduler helps doing that by providing the drm_sched_entity_flush() and drm_sched_entity_fini() functions.

When there is still pending work when drm_sched_entity_fini() is called a callback to kill it is installed and the job just freed instead of executed.

Regards,
Christian.

>
>> This prevents applications with multiple contexts from running into a
>> race condition between running tasks and context destroy when
>> terminating.
>>
>> Signed-off-by: Erico Nunes <nunes.erico@gmail.com>
>> ---
>>  drivers/gpu/drm/lima/lima_ctx.c | 18 ++++++++++++++++++
>>  drivers/gpu/drm/lima/lima_ctx.h |  1 +
>>  drivers/gpu/drm/lima/lima_drv.c | 17 ++++++++++++++++-
>>  3 files changed, 35 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/lima/lima_ctx.c b/drivers/gpu/drm/lima/lima_ctx.c
>> index 0e668fc1e0f9..e8fb5788ca69 100644
>> --- a/drivers/gpu/drm/lima/lima_ctx.c
>> +++ b/drivers/gpu/drm/lima/lima_ctx.c
>> @@ -100,3 +100,21 @@ void lima_ctx_mgr_fini(struct lima_ctx_mgr *mgr)
>>         xa_destroy(&mgr->handles);
>>         mutex_destroy(&mgr->lock);
>>  }
>> +
>> +long lima_ctx_mgr_flush(struct lima_ctx_mgr *mgr, long timeout)
>> +{
>> +       struct lima_ctx *ctx;
>> +       unsigned long id;
>> +
>> +       mutex_lock(&mgr->lock);
>> +       xa_for_each(&mgr->handles, id, ctx) {
>> +               for (int i = 0; i < lima_pipe_num; i++) {
>> +                       struct lima_sched_context *context = &ctx->context[i];
>> +                       struct drm_sched_entity *entity = &context->base;
>> +
>> +                       timeout = drm_sched_entity_flush(entity, timeout);
>> +               }
>> +       }
>> +       mutex_unlock(&mgr->lock);
>> +       return timeout;
>> +}
>> diff --git a/drivers/gpu/drm/lima/lima_ctx.h b/drivers/gpu/drm/lima/lima_ctx.h
>> index 5b1063ce968b..ff133db6ae4c 100644
>> --- a/drivers/gpu/drm/lima/lima_ctx.h
>> +++ b/drivers/gpu/drm/lima/lima_ctx.h
>> @@ -30,5 +30,6 @@ struct lima_ctx *lima_ctx_get(struct lima_ctx_mgr *mgr, u32 id);
>>  void lima_ctx_put(struct lima_ctx *ctx);
>>  void lima_ctx_mgr_init(struct lima_ctx_mgr *mgr);
>>  void lima_ctx_mgr_fini(struct lima_ctx_mgr *mgr);
>> +long lima_ctx_mgr_flush(struct lima_ctx_mgr *mgr, long timeout);
>>
>>  #endif
>> diff --git a/drivers/gpu/drm/lima/lima_drv.c b/drivers/gpu/drm/lima/lima_drv.c
>> index 11ace5cebf4c..08169b0d9c28 100644
>> --- a/drivers/gpu/drm/lima/lima_drv.c
>> +++ b/drivers/gpu/drm/lima/lima_drv.c
>> @@ -254,7 +254,22 @@ static const struct drm_ioctl_desc lima_drm_driver_ioctls[] = {
>>         DRM_IOCTL_DEF_DRV(LIMA_CTX_FREE, lima_ioctl_ctx_free, DRM_RENDER_ALLOW),
>>  };
>>
>> -DEFINE_DRM_GEM_FOPS(lima_drm_driver_fops);
>> +static int lima_drm_driver_flush(struct file *filp, fl_owner_t id)
>> +{
>> +       struct drm_file *file = filp->private_data;
>> +       struct lima_drm_priv *priv = file->driver_priv;
>> +       long timeout = MAX_WAIT_SCHED_ENTITY_Q_EMPTY;
>> +
>> +       timeout = lima_ctx_mgr_flush(&priv->ctx_mgr, timeout);
>> +
>> +       return timeout >= 0 ? 0 : timeout;
>> +}
>> +
>> +static const struct file_operations lima_drm_driver_fops = {
>> +       .owner = THIS_MODULE,
>> +       .flush = lima_drm_driver_flush,
>> +       DRM_GEM_FOPS,
>> +};
>>
>>  /*
>>   * Changelog:
>> --
>> 2.49.0
>>

Re: [PATCH 1/1] drm/lima: implement the file flush callback

[Qiang Yu]

On Thu, Apr 10, 2025 at 8:35 PM Christian König
<christian.koenig@amd.com> wrote:
>
> Am 10.04.25 um 11:33 schrieb Qiang Yu:
> > On Tue, Apr 8, 2025 at 11:48 PM Erico Nunes <nunes.erico@gmail.com> wrote:
> >> With this callback implemented, a terminating application will wait for
> >> the sched entity to be flushed out to the hardware and cancel all other
> >> pending jobs before destroying its context.
> > We do flush when file release in lima_ctx_mgr_fini. Why do we wait here
> > in flush? What's the difference?
>
> Waiting for submissions when you release the file descriptor is actually a bad idea since that can prevent SIGKILL from acting immediately. For example the OOM killer absolutely doesn't like that and eventually calls panic().
>
> Flush is called either manually, on process termination or when you send a SIGTERM. This should then wait for any I/O to complete.
>
> The idea is now that you can then still send a SIGKILL to abort waiting for the I/O as well and so get pending GPU operations not submitted to the HW.
>
> The DRM scheduler helps doing that by providing the drm_sched_entity_flush() and drm_sched_entity_fini() functions.
>
> When there is still pending work when drm_sched_entity_fini() is called a callback to kill it is installed and the job just freed instead of executed.
>
So the difference is we can always wait in flush, but better not in
release when SIGKILL?

> >
> >> This prevents applications with multiple contexts from running into a
> >> race condition between running tasks and context destroy when
> >> terminating.
> >>
If implementing flush callback fix the problem, it must not be when SIGKILL.
Could you describe the problem and how this fix solves it? As I don't understand
how the above difference could fix a race bug.

> >> Signed-off-by: Erico Nunes <nunes.erico@gmail.com>
> >> ---
> >>  drivers/gpu/drm/lima/lima_ctx.c | 18 ++++++++++++++++++
> >>  drivers/gpu/drm/lima/lima_ctx.h |  1 +
> >>  drivers/gpu/drm/lima/lima_drv.c | 17 ++++++++++++++++-
> >>  3 files changed, 35 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/gpu/drm/lima/lima_ctx.c b/drivers/gpu/drm/lima/lima_ctx.c
> >> index 0e668fc1e0f9..e8fb5788ca69 100644
> >> --- a/drivers/gpu/drm/lima/lima_ctx.c
> >> +++ b/drivers/gpu/drm/lima/lima_ctx.c
> >> @@ -100,3 +100,21 @@ void lima_ctx_mgr_fini(struct lima_ctx_mgr *mgr)
> >>         xa_destroy(&mgr->handles);
> >>         mutex_destroy(&mgr->lock);
> >>  }
> >> +
> >> +long lima_ctx_mgr_flush(struct lima_ctx_mgr *mgr, long timeout)
> >> +{
> >> +       struct lima_ctx *ctx;
> >> +       unsigned long id;
> >> +
> >> +       mutex_lock(&mgr->lock);
> >> +       xa_for_each(&mgr->handles, id, ctx) {
> >> +               for (int i = 0; i < lima_pipe_num; i++) {
> >> +                       struct lima_sched_context *context = &ctx->context[i];
> >> +                       struct drm_sched_entity *entity = &context->base;
> >> +
> >> +                       timeout = drm_sched_entity_flush(entity, timeout);
> >> +               }
> >> +       }
> >> +       mutex_unlock(&mgr->lock);
> >> +       return timeout;
> >> +}
> >> diff --git a/drivers/gpu/drm/lima/lima_ctx.h b/drivers/gpu/drm/lima/lima_ctx.h
> >> index 5b1063ce968b..ff133db6ae4c 100644
> >> --- a/drivers/gpu/drm/lima/lima_ctx.h
> >> +++ b/drivers/gpu/drm/lima/lima_ctx.h
> >> @@ -30,5 +30,6 @@ struct lima_ctx *lima_ctx_get(struct lima_ctx_mgr *mgr, u32 id);
> >>  void lima_ctx_put(struct lima_ctx *ctx);
> >>  void lima_ctx_mgr_init(struct lima_ctx_mgr *mgr);
> >>  void lima_ctx_mgr_fini(struct lima_ctx_mgr *mgr);
> >> +long lima_ctx_mgr_flush(struct lima_ctx_mgr *mgr, long timeout);
> >>
> >>  #endif
> >> diff --git a/drivers/gpu/drm/lima/lima_drv.c b/drivers/gpu/drm/lima/lima_drv.c
> >> index 11ace5cebf4c..08169b0d9c28 100644
> >> --- a/drivers/gpu/drm/lima/lima_drv.c
> >> +++ b/drivers/gpu/drm/lima/lima_drv.c
> >> @@ -254,7 +254,22 @@ static const struct drm_ioctl_desc lima_drm_driver_ioctls[] = {
> >>         DRM_IOCTL_DEF_DRV(LIMA_CTX_FREE, lima_ioctl_ctx_free, DRM_RENDER_ALLOW),
> >>  };
> >>
> >> -DEFINE_DRM_GEM_FOPS(lima_drm_driver_fops);
> >> +static int lima_drm_driver_flush(struct file *filp, fl_owner_t id)
> >> +{
> >> +       struct drm_file *file = filp->private_data;
> >> +       struct lima_drm_priv *priv = file->driver_priv;
> >> +       long timeout = MAX_WAIT_SCHED_ENTITY_Q_EMPTY;
> >> +
> >> +       timeout = lima_ctx_mgr_flush(&priv->ctx_mgr, timeout);
> >> +
> >> +       return timeout >= 0 ? 0 : timeout;
> >> +}
> >> +
> >> +static const struct file_operations lima_drm_driver_fops = {
> >> +       .owner = THIS_MODULE,
> >> +       .flush = lima_drm_driver_flush,
> >> +       DRM_GEM_FOPS,
> >> +};
> >>
> >>  /*
> >>   * Changelog:
> >> --
> >> 2.49.0
> >>
>

Re: [PATCH 1/1] drm/lima: implement the file flush callback

[Christian König]

Am 10.04.25 um 15:56 schrieb Qiang Yu:
> On Thu, Apr 10, 2025 at 8:35 PM Christian König
> <christian.koenig@amd.com> wrote:
>> Am 10.04.25 um 11:33 schrieb Qiang Yu:
>>> On Tue, Apr 8, 2025 at 11:48 PM Erico Nunes <nunes.erico@gmail.com> wrote:
>>>> With this callback implemented, a terminating application will wait for
>>>> the sched entity to be flushed out to the hardware and cancel all other
>>>> pending jobs before destroying its context.
>>> We do flush when file release in lima_ctx_mgr_fini. Why do we wait here
>>> in flush? What's the difference?
>> Waiting for submissions when you release the file descriptor is actually a bad idea since that can prevent SIGKILL from acting immediately. For example the OOM killer absolutely doesn't like that and eventually calls panic().
>>
>> Flush is called either manually, on process termination or when you send a SIGTERM. This should then wait for any I/O to complete.
>>
>> The idea is now that you can then still send a SIGKILL to abort waiting for the I/O as well and so get pending GPU operations not submitted to the HW.
>>
>> The DRM scheduler helps doing that by providing the drm_sched_entity_flush() and drm_sched_entity_fini() functions.
>>
>> When there is still pending work when drm_sched_entity_fini() is called a callback to kill it is installed and the job just freed instead of executed.
>>
> So the difference is we can always wait in flush, but better not in
> release when SIGKILL?

Exactly that, yes.

>
>>>> This prevents applications with multiple contexts from running into a
>>>> race condition between running tasks and context destroy when
>>>> terminating.
>>>>
> If implementing flush callback fix the problem, it must not be when SIGKILL.

SIGKILL also calls flush (again eventually), but we can detect that in the scheduler code.

See the code and comment in drm_sched_entity_flush(). We could potentially improve the comment cause it's not 100% correct, but it should work under all cases.

> Could you describe the problem and how this fix solves it? As I don't understand
> how the above difference could fix a race bug.

That was the point I wasn't sure about either. It should *not* fix any race, it's just gives a nicer SIGKILL experience.

Regards,
Christian.

>
>>>> Signed-off-by: Erico Nunes <nunes.erico@gmail.com>
>>>> ---
>>>>  drivers/gpu/drm/lima/lima_ctx.c | 18 ++++++++++++++++++
>>>>  drivers/gpu/drm/lima/lima_ctx.h |  1 +
>>>>  drivers/gpu/drm/lima/lima_drv.c | 17 ++++++++++++++++-
>>>>  3 files changed, 35 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/lima/lima_ctx.c b/drivers/gpu/drm/lima/lima_ctx.c
>>>> index 0e668fc1e0f9..e8fb5788ca69 100644
>>>> --- a/drivers/gpu/drm/lima/lima_ctx.c
>>>> +++ b/drivers/gpu/drm/lima/lima_ctx.c
>>>> @@ -100,3 +100,21 @@ void lima_ctx_mgr_fini(struct lima_ctx_mgr *mgr)
>>>>         xa_destroy(&mgr->handles);
>>>>         mutex_destroy(&mgr->lock);
>>>>  }
>>>> +
>>>> +long lima_ctx_mgr_flush(struct lima_ctx_mgr *mgr, long timeout)
>>>> +{
>>>> +       struct lima_ctx *ctx;
>>>> +       unsigned long id;
>>>> +
>>>> +       mutex_lock(&mgr->lock);
>>>> +       xa_for_each(&mgr->handles, id, ctx) {
>>>> +               for (int i = 0; i < lima_pipe_num; i++) {
>>>> +                       struct lima_sched_context *context = &ctx->context[i];
>>>> +                       struct drm_sched_entity *entity = &context->base;
>>>> +
>>>> +                       timeout = drm_sched_entity_flush(entity, timeout);
>>>> +               }
>>>> +       }
>>>> +       mutex_unlock(&mgr->lock);
>>>> +       return timeout;
>>>> +}
>>>> diff --git a/drivers/gpu/drm/lima/lima_ctx.h b/drivers/gpu/drm/lima/lima_ctx.h
>>>> index 5b1063ce968b..ff133db6ae4c 100644
>>>> --- a/drivers/gpu/drm/lima/lima_ctx.h
>>>> +++ b/drivers/gpu/drm/lima/lima_ctx.h
>>>> @@ -30,5 +30,6 @@ struct lima_ctx *lima_ctx_get(struct lima_ctx_mgr *mgr, u32 id);
>>>>  void lima_ctx_put(struct lima_ctx *ctx);
>>>>  void lima_ctx_mgr_init(struct lima_ctx_mgr *mgr);
>>>>  void lima_ctx_mgr_fini(struct lima_ctx_mgr *mgr);
>>>> +long lima_ctx_mgr_flush(struct lima_ctx_mgr *mgr, long timeout);
>>>>
>>>>  #endif
>>>> diff --git a/drivers/gpu/drm/lima/lima_drv.c b/drivers/gpu/drm/lima/lima_drv.c
>>>> index 11ace5cebf4c..08169b0d9c28 100644
>>>> --- a/drivers/gpu/drm/lima/lima_drv.c
>>>> +++ b/drivers/gpu/drm/lima/lima_drv.c
>>>> @@ -254,7 +254,22 @@ static const struct drm_ioctl_desc lima_drm_driver_ioctls[] = {
>>>>         DRM_IOCTL_DEF_DRV(LIMA_CTX_FREE, lima_ioctl_ctx_free, DRM_RENDER_ALLOW),
>>>>  };
>>>>
>>>> -DEFINE_DRM_GEM_FOPS(lima_drm_driver_fops);
>>>> +static int lima_drm_driver_flush(struct file *filp, fl_owner_t id)
>>>> +{
>>>> +       struct drm_file *file = filp->private_data;
>>>> +       struct lima_drm_priv *priv = file->driver_priv;
>>>> +       long timeout = MAX_WAIT_SCHED_ENTITY_Q_EMPTY;
>>>> +
>>>> +       timeout = lima_ctx_mgr_flush(&priv->ctx_mgr, timeout);
>>>> +
>>>> +       return timeout >= 0 ? 0 : timeout;
>>>> +}
>>>> +
>>>> +static const struct file_operations lima_drm_driver_fops = {
>>>> +       .owner = THIS_MODULE,
>>>> +       .flush = lima_drm_driver_flush,
>>>> +       DRM_GEM_FOPS,
>>>> +};
>>>>
>>>>  /*
>>>>   * Changelog:
>>>> --
>>>> 2.49.0
>>>>

Re: [PATCH 1/1] drm/lima: implement the file flush callback

[Erico Nunes]

On Thu, Apr 10, 2025 at 4:04 PM Christian König
<christian.koenig@amd.com> wrote:
>
> Am 10.04.25 um 15:56 schrieb Qiang Yu:
> >>>> This prevents applications with multiple contexts from running into a
> >>>> race condition between running tasks and context destroy when
> >>>> terminating.
> >>>>
> > If implementing flush callback fix the problem, it must not be when SIGKILL.
>
> SIGKILL also calls flush (again eventually), but we can detect that in the scheduler code.
>
> See the code and comment in drm_sched_entity_flush(). We could potentially improve the comment cause it's not 100% correct, but it should work under all cases.
>
> > Could you describe the problem and how this fix solves it? As I don't understand
> > how the above difference could fix a race bug.
>
> That was the point I wasn't sure about either. It should *not* fix any race, it's just gives a nicer SIGKILL experience.

Thanks for this feedback. So as mentioned in the initial letter, I'm
also trying to understand if this is the correct fix.

This problem is unfortunately not trivial to reproduce, there is only
one application which can reproduce this so far and it is a
complicated one with multiple contexts and relatively lenghty jobs.
What I observed so far is that as it is right now, a context might be
destroyed while a job is running (e.g. by killing the application at
the right time), and a context destroy appears to release buffers that
are still being used by the running job which is what causes the read
faults.
This added flush made it so that the jobs always finish before the
context destroy gets called, which prevents the issue for this
application in my attempts. But I suppose it might also just be that
it had some more time to finish now. If this is not the correct fix
then there might be some more missing synchronization between running
job and context destroy in lima?

Thanks

Erico

Re: [PATCH 1/1] drm/lima: implement the file flush callback

[Qiang Yu]

On Mon, Apr 14, 2025 at 9:19 PM Erico Nunes <nunes.erico@gmail.com> wrote:
>
> On Thu, Apr 10, 2025 at 4:04 PM Christian König
> <christian.koenig@amd.com> wrote:
> >
> > Am 10.04.25 um 15:56 schrieb Qiang Yu:
> > >>>> This prevents applications with multiple contexts from running into a
> > >>>> race condition between running tasks and context destroy when
> > >>>> terminating.
> > >>>>
> > > If implementing flush callback fix the problem, it must not be when SIGKILL.
> >
> > SIGKILL also calls flush (again eventually), but we can detect that in the scheduler code.
> >
> > See the code and comment in drm_sched_entity_flush(). We could potentially improve the comment cause it's not 100% correct, but it should work under all cases.
> >
> > > Could you describe the problem and how this fix solves it? As I don't understand
> > > how the above difference could fix a race bug.
> >
> > That was the point I wasn't sure about either. It should *not* fix any race, it's just gives a nicer SIGKILL experience.
>
> Thanks for this feedback. So as mentioned in the initial letter, I'm
> also trying to understand if this is the correct fix.
>
> This problem is unfortunately not trivial to reproduce, there is only
> one application which can reproduce this so far and it is a
> complicated one with multiple contexts and relatively lenghty jobs.
> What I observed so far is that as it is right now, a context might be
> destroyed while a job is running (e.g. by killing the application at
> the right time), and a context destroy appears to release buffers that
> are still being used by the running job which is what causes the read
> faults.

Free buffer in context destroy when job running is the bug, we should
dig into it how it happens. And if multiple context play a key role.

> This added flush made it so that the jobs always finish before the
> context destroy gets called, which prevents the issue for this
> application in my attempts. But I suppose it might also just be that
> it had some more time to finish now. If this is not the correct fix
> then there might be some more missing synchronization between running
> job and context destroy in lima?
>
I'm afraid this patch does not fix the root cause, we should find out
the root cause first. This patch could be added as SIGKILL improvement
later.

> Thanks
>
> Erico