* [PATCH 1/2] drm/nouveau: don't fini scheduler if not initialized
@ 2024-02-02 0:05 Danilo Krummrich
2024-02-02 0:05 ` [PATCH 2/2] drm/nouveau: omit to create schedulers using the legacy uAPI Danilo Krummrich
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Danilo Krummrich @ 2024-02-02 0:05 UTC (permalink / raw)
To: nouveau; +Cc: lyude, kherbst, airlied, ttabi, dri-devel, Danilo Krummrich
nouveau_abi16_ioctl_channel_alloc() and nouveau_cli_init() simply call
their corresponding *_fini() counterpart. This can lead to
nouveau_sched_fini() being called without struct nouveau_sched ever
being initialized in the first place.
Instead of embedding struct nouveau_sched into struct nouveau_cli and
struct nouveau_chan_abi16, allocate struct nouveau_sched separately,
such that we can check for the corresponding pointer to be NULL in the
particular *_fini() functions.
It makes sense to allocate struct nouveau_sched separately anyway, since
in a subsequent commit we can also avoid to allocate a struct
nouveau_sched in nouveau_abi16_ioctl_channel_alloc() at all, if the
VM_BIND uAPI has been disabled due to the legacy uAPI being used.
Fixes: 5f03a507b29e ("drm/nouveau: implement 1:1 scheduler - entity relationship")
Reported-by: Timur Tabi <ttabi@nvidia.com>
Closes: https://lore.kernel.org/nouveau/20240131213917.1545604-1-ttabi@nvidia.com/
Signed-off-by: Danilo Krummrich <dakr@redhat.com>
---
drivers/gpu/drm/nouveau/nouveau_abi16.c | 10 ++++---
drivers/gpu/drm/nouveau/nouveau_abi16.h | 2 +-
drivers/gpu/drm/nouveau/nouveau_drm.c | 7 +++--
drivers/gpu/drm/nouveau/nouveau_drv.h | 2 +-
drivers/gpu/drm/nouveau/nouveau_exec.c | 2 +-
drivers/gpu/drm/nouveau/nouveau_sched.c | 38 +++++++++++++++++++++++--
drivers/gpu/drm/nouveau/nouveau_sched.h | 6 ++--
drivers/gpu/drm/nouveau/nouveau_uvmm.c | 2 +-
8 files changed, 53 insertions(+), 16 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nouveau_abi16.c b/drivers/gpu/drm/nouveau/nouveau_abi16.c
index a04156ca8390..ca4b5ab3e59e 100644
--- a/drivers/gpu/drm/nouveau/nouveau_abi16.c
+++ b/drivers/gpu/drm/nouveau/nouveau_abi16.c
@@ -128,12 +128,14 @@ nouveau_abi16_chan_fini(struct nouveau_abi16 *abi16,
struct nouveau_abi16_ntfy *ntfy, *temp;
/* Cancel all jobs from the entity's queue. */
- drm_sched_entity_fini(&chan->sched.entity);
+ if (chan->sched)
+ drm_sched_entity_fini(&chan->sched->entity);
if (chan->chan)
nouveau_channel_idle(chan->chan);
- nouveau_sched_fini(&chan->sched);
+ if (chan->sched)
+ nouveau_sched_destroy(&chan->sched);
/* cleanup notifier state */
list_for_each_entry_safe(ntfy, temp, &chan->notifiers, head) {
@@ -337,8 +339,8 @@ nouveau_abi16_ioctl_channel_alloc(ABI16_IOCTL_ARGS)
if (ret)
goto done;
- ret = nouveau_sched_init(&chan->sched, drm, drm->sched_wq,
- chan->chan->dma.ib_max);
+ ret = nouveau_sched_create(&chan->sched, drm, drm->sched_wq,
+ chan->chan->dma.ib_max);
if (ret)
goto done;
diff --git a/drivers/gpu/drm/nouveau/nouveau_abi16.h b/drivers/gpu/drm/nouveau/nouveau_abi16.h
index 1f5e243c0c75..11c8c4a80079 100644
--- a/drivers/gpu/drm/nouveau/nouveau_abi16.h
+++ b/drivers/gpu/drm/nouveau/nouveau_abi16.h
@@ -26,7 +26,7 @@ struct nouveau_abi16_chan {
struct nouveau_bo *ntfy;
struct nouveau_vma *ntfy_vma;
struct nvkm_mm heap;
- struct nouveau_sched sched;
+ struct nouveau_sched *sched;
};
struct nouveau_abi16 {
diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c
index 6f6c31a9937b..a947e1d5f309 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
@@ -201,7 +201,8 @@ nouveau_cli_fini(struct nouveau_cli *cli)
WARN_ON(!list_empty(&cli->worker));
usif_client_fini(cli);
- nouveau_sched_fini(&cli->sched);
+ if (cli->sched)
+ nouveau_sched_destroy(&cli->sched);
if (uvmm)
nouveau_uvmm_fini(uvmm);
nouveau_vmm_fini(&cli->svm);
@@ -311,7 +312,7 @@ nouveau_cli_init(struct nouveau_drm *drm, const char *sname,
cli->mem = &mems[ret];
/* Don't pass in the (shared) sched_wq in order to let
- * nouveau_sched_init() create a dedicated one for VM_BIND jobs.
+ * nouveau_sched_create() create a dedicated one for VM_BIND jobs.
*
* This is required to ensure that for VM_BIND jobs free_job() work and
* run_job() work can always run concurrently and hence, free_job() work
@@ -320,7 +321,7 @@ nouveau_cli_init(struct nouveau_drm *drm, const char *sname,
* locks which indirectly or directly are held for allocations
* elsewhere.
*/
- ret = nouveau_sched_init(&cli->sched, drm, NULL, 1);
+ ret = nouveau_sched_create(&cli->sched, drm, NULL, 1);
if (ret)
goto done;
diff --git a/drivers/gpu/drm/nouveau/nouveau_drv.h b/drivers/gpu/drm/nouveau/nouveau_drv.h
index 8a6d94c8b163..e239c6bf4afa 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drv.h
+++ b/drivers/gpu/drm/nouveau/nouveau_drv.h
@@ -98,7 +98,7 @@ struct nouveau_cli {
bool disabled;
} uvmm;
- struct nouveau_sched sched;
+ struct nouveau_sched *sched;
const struct nvif_mclass *mem;
diff --git a/drivers/gpu/drm/nouveau/nouveau_exec.c b/drivers/gpu/drm/nouveau/nouveau_exec.c
index bc5d71b79ab2..e65c0ef23bc7 100644
--- a/drivers/gpu/drm/nouveau/nouveau_exec.c
+++ b/drivers/gpu/drm/nouveau/nouveau_exec.c
@@ -389,7 +389,7 @@ nouveau_exec_ioctl_exec(struct drm_device *dev,
if (ret)
goto out;
- args.sched = &chan16->sched;
+ args.sched = chan16->sched;
args.file_priv = file_priv;
args.chan = chan;
diff --git a/drivers/gpu/drm/nouveau/nouveau_sched.c b/drivers/gpu/drm/nouveau/nouveau_sched.c
index dd98f6910f9c..32fa2e273965 100644
--- a/drivers/gpu/drm/nouveau/nouveau_sched.c
+++ b/drivers/gpu/drm/nouveau/nouveau_sched.c
@@ -398,7 +398,7 @@ static const struct drm_sched_backend_ops nouveau_sched_ops = {
.free_job = nouveau_sched_free_job,
};
-int
+static int
nouveau_sched_init(struct nouveau_sched *sched, struct nouveau_drm *drm,
struct workqueue_struct *wq, u32 credit_limit)
{
@@ -453,7 +453,30 @@ nouveau_sched_init(struct nouveau_sched *sched, struct nouveau_drm *drm,
return ret;
}
-void
+int
+nouveau_sched_create(struct nouveau_sched **psched, struct nouveau_drm *drm,
+ struct workqueue_struct *wq, u32 credit_limit)
+{
+ struct nouveau_sched *sched;
+ int ret;
+
+ sched = kzalloc(sizeof(*sched), GFP_KERNEL);
+ if (!sched)
+ return -ENOMEM;
+
+ ret = nouveau_sched_init(sched, drm, wq, credit_limit);
+ if (ret) {
+ kfree(sched);
+ return ret;
+ }
+
+ *psched = sched;
+
+ return 0;
+}
+
+
+static void
nouveau_sched_fini(struct nouveau_sched *sched)
{
struct drm_gpu_scheduler *drm_sched = &sched->base;
@@ -471,3 +494,14 @@ nouveau_sched_fini(struct nouveau_sched *sched)
if (sched->wq)
destroy_workqueue(sched->wq);
}
+
+void
+nouveau_sched_destroy(struct nouveau_sched **psched)
+{
+ struct nouveau_sched *sched = *psched;
+
+ nouveau_sched_fini(sched);
+ kfree(sched);
+
+ *psched = NULL;
+}
diff --git a/drivers/gpu/drm/nouveau/nouveau_sched.h b/drivers/gpu/drm/nouveau/nouveau_sched.h
index a6528f5981e6..e1f01a23e6f6 100644
--- a/drivers/gpu/drm/nouveau/nouveau_sched.h
+++ b/drivers/gpu/drm/nouveau/nouveau_sched.h
@@ -111,8 +111,8 @@ struct nouveau_sched {
} job;
};
-int nouveau_sched_init(struct nouveau_sched *sched, struct nouveau_drm *drm,
- struct workqueue_struct *wq, u32 credit_limit);
-void nouveau_sched_fini(struct nouveau_sched *sched);
+int nouveau_sched_create(struct nouveau_sched **psched, struct nouveau_drm *drm,
+ struct workqueue_struct *wq, u32 credit_limit);
+void nouveau_sched_destroy(struct nouveau_sched **psched);
#endif
diff --git a/drivers/gpu/drm/nouveau/nouveau_uvmm.c b/drivers/gpu/drm/nouveau/nouveau_uvmm.c
index 4f223c972c6a..0a0a11dc9ec0 100644
--- a/drivers/gpu/drm/nouveau/nouveau_uvmm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_uvmm.c
@@ -1740,7 +1740,7 @@ nouveau_uvmm_ioctl_vm_bind(struct drm_device *dev,
if (ret)
return ret;
- args.sched = &cli->sched;
+ args.sched = cli->sched;
args.file_priv = file_priv;
ret = nouveau_uvmm_vm_bind(&args);
base-commit: 041261ac4c365e03b07427569d6735f8adfd21c8
--
2.43.0
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 2/2] drm/nouveau: omit to create schedulers using the legacy uAPI
2024-02-02 0:05 [PATCH 1/2] drm/nouveau: don't fini scheduler if not initialized Danilo Krummrich
@ 2024-02-02 0:05 ` Danilo Krummrich
2024-02-02 17:14 ` [PATCH 1/2] drm/nouveau: don't fini scheduler if not initialized Timur Tabi
2024-02-09 19:39 ` Dave Airlie
2 siblings, 0 replies; 9+ messages in thread
From: Danilo Krummrich @ 2024-02-02 0:05 UTC (permalink / raw)
To: nouveau; +Cc: lyude, kherbst, airlied, ttabi, dri-devel, Danilo Krummrich
Omit to create scheduler instances when using the legacy uAPI. When
using the legacy NOUVEAU_GEM_PUSHBUF ioctl no scheduler instance is
required, hence omit creating scheduler instances in
nouveau_abi16_ioctl_channel_alloc().
Signed-off-by: Danilo Krummrich <dakr@redhat.com>
---
drivers/gpu/drm/nouveau/nouveau_abi16.c | 14 ++++++++++----
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nouveau_abi16.c b/drivers/gpu/drm/nouveau/nouveau_abi16.c
index ca4b5ab3e59e..d1bb8151a1df 100644
--- a/drivers/gpu/drm/nouveau/nouveau_abi16.c
+++ b/drivers/gpu/drm/nouveau/nouveau_abi16.c
@@ -339,10 +339,16 @@ nouveau_abi16_ioctl_channel_alloc(ABI16_IOCTL_ARGS)
if (ret)
goto done;
- ret = nouveau_sched_create(&chan->sched, drm, drm->sched_wq,
- chan->chan->dma.ib_max);
- if (ret)
- goto done;
+ /* If we're not using the VM_BIND uAPI, we don't need a scheduler.
+ *
+ * The client lock is already acquired by nouveau_abi16_get().
+ */
+ if (nouveau_cli_uvmm(cli)) {
+ ret = nouveau_sched_create(&chan->sched, drm, drm->sched_wq,
+ chan->chan->dma.ib_max);
+ if (ret)
+ goto done;
+ }
init->channel = chan->chan->chid;
--
2.43.0
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] drm/nouveau: don't fini scheduler if not initialized
2024-02-02 0:05 [PATCH 1/2] drm/nouveau: don't fini scheduler if not initialized Danilo Krummrich
2024-02-02 0:05 ` [PATCH 2/2] drm/nouveau: omit to create schedulers using the legacy uAPI Danilo Krummrich
@ 2024-02-02 17:14 ` Timur Tabi
2024-02-02 17:24 ` Danilo Krummrich
2024-02-14 23:48 ` Timur Tabi
2024-02-09 19:39 ` Dave Airlie
2 siblings, 2 replies; 9+ messages in thread
From: Timur Tabi @ 2024-02-02 17:14 UTC (permalink / raw)
To: nouveau, dakr; +Cc: dri-devel, kherbst, lyude, airlied
On Fri, 2024-02-02 at 01:05 +0100, Danilo Krummrich wrote:
> nouveau_abi16_ioctl_channel_alloc() and nouveau_cli_init() simply call
> their corresponding *_fini() counterpart. This can lead to
> nouveau_sched_fini() being called without struct nouveau_sched ever
> being initialized in the first place.
Thanks, I've confirmed that these patches do fix the problem.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] drm/nouveau: don't fini scheduler if not initialized
2024-02-02 17:14 ` [PATCH 1/2] drm/nouveau: don't fini scheduler if not initialized Timur Tabi
@ 2024-02-02 17:24 ` Danilo Krummrich
2024-02-14 23:48 ` Timur Tabi
1 sibling, 0 replies; 9+ messages in thread
From: Danilo Krummrich @ 2024-02-02 17:24 UTC (permalink / raw)
To: Timur Tabi, nouveau; +Cc: dri-devel, kherbst, lyude, airlied
On 2/2/24 18:14, Timur Tabi wrote:
> On Fri, 2024-02-02 at 01:05 +0100, Danilo Krummrich wrote:
>> nouveau_abi16_ioctl_channel_alloc() and nouveau_cli_init() simply call
>> their corresponding *_fini() counterpart. This can lead to
>> nouveau_sched_fini() being called without struct nouveau_sched ever
>> being initialized in the first place.
>
> Thanks, I've confirmed that these patches do fix the problem
Cool, gonna add your 'Tested-by' then.
- Danilo
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] drm/nouveau: don't fini scheduler if not initialized
2024-02-02 0:05 [PATCH 1/2] drm/nouveau: don't fini scheduler if not initialized Danilo Krummrich
2024-02-02 0:05 ` [PATCH 2/2] drm/nouveau: omit to create schedulers using the legacy uAPI Danilo Krummrich
2024-02-02 17:14 ` [PATCH 1/2] drm/nouveau: don't fini scheduler if not initialized Timur Tabi
@ 2024-02-09 19:39 ` Dave Airlie
2 siblings, 0 replies; 9+ messages in thread
From: Dave Airlie @ 2024-02-09 19:39 UTC (permalink / raw)
To: Danilo Krummrich; +Cc: nouveau, lyude, kherbst, ttabi, dri-devel
On Fri, 2 Feb 2024 at 10:06, Danilo Krummrich <dakr@redhat.com> wrote:
>
> nouveau_abi16_ioctl_channel_alloc() and nouveau_cli_init() simply call
> their corresponding *_fini() counterpart. This can lead to
> nouveau_sched_fini() being called without struct nouveau_sched ever
> being initialized in the first place.
>
> Instead of embedding struct nouveau_sched into struct nouveau_cli and
> struct nouveau_chan_abi16, allocate struct nouveau_sched separately,
> such that we can check for the corresponding pointer to be NULL in the
> particular *_fini() functions.
>
> It makes sense to allocate struct nouveau_sched separately anyway, since
> in a subsequent commit we can also avoid to allocate a struct
> nouveau_sched in nouveau_abi16_ioctl_channel_alloc() at all, if the
> VM_BIND uAPI has been disabled due to the legacy uAPI being used.
Looks good,
for the series
Reviewed-by: Dave Airlie <airlied@redhat.com>
>
> Fixes: 5f03a507b29e ("drm/nouveau: implement 1:1 scheduler - entity relationship")
> Reported-by: Timur Tabi <ttabi@nvidia.com>
> Closes: https://lore.kernel.org/nouveau/20240131213917.1545604-1-ttabi@nvidia.com/
> Signed-off-by: Danilo Krummrich <dakr@redhat.com>
> ---
> drivers/gpu/drm/nouveau/nouveau_abi16.c | 10 ++++---
> drivers/gpu/drm/nouveau/nouveau_abi16.h | 2 +-
> drivers/gpu/drm/nouveau/nouveau_drm.c | 7 +++--
> drivers/gpu/drm/nouveau/nouveau_drv.h | 2 +-
> drivers/gpu/drm/nouveau/nouveau_exec.c | 2 +-
> drivers/gpu/drm/nouveau/nouveau_sched.c | 38 +++++++++++++++++++++++--
> drivers/gpu/drm/nouveau/nouveau_sched.h | 6 ++--
> drivers/gpu/drm/nouveau/nouveau_uvmm.c | 2 +-
> 8 files changed, 53 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/gpu/drm/nouveau/nouveau_abi16.c b/drivers/gpu/drm/nouveau/nouveau_abi16.c
> index a04156ca8390..ca4b5ab3e59e 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_abi16.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_abi16.c
> @@ -128,12 +128,14 @@ nouveau_abi16_chan_fini(struct nouveau_abi16 *abi16,
> struct nouveau_abi16_ntfy *ntfy, *temp;
>
> /* Cancel all jobs from the entity's queue. */
> - drm_sched_entity_fini(&chan->sched.entity);
> + if (chan->sched)
> + drm_sched_entity_fini(&chan->sched->entity);
>
> if (chan->chan)
> nouveau_channel_idle(chan->chan);
>
> - nouveau_sched_fini(&chan->sched);
> + if (chan->sched)
> + nouveau_sched_destroy(&chan->sched);
>
> /* cleanup notifier state */
> list_for_each_entry_safe(ntfy, temp, &chan->notifiers, head) {
> @@ -337,8 +339,8 @@ nouveau_abi16_ioctl_channel_alloc(ABI16_IOCTL_ARGS)
> if (ret)
> goto done;
>
> - ret = nouveau_sched_init(&chan->sched, drm, drm->sched_wq,
> - chan->chan->dma.ib_max);
> + ret = nouveau_sched_create(&chan->sched, drm, drm->sched_wq,
> + chan->chan->dma.ib_max);
> if (ret)
> goto done;
>
> diff --git a/drivers/gpu/drm/nouveau/nouveau_abi16.h b/drivers/gpu/drm/nouveau/nouveau_abi16.h
> index 1f5e243c0c75..11c8c4a80079 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_abi16.h
> +++ b/drivers/gpu/drm/nouveau/nouveau_abi16.h
> @@ -26,7 +26,7 @@ struct nouveau_abi16_chan {
> struct nouveau_bo *ntfy;
> struct nouveau_vma *ntfy_vma;
> struct nvkm_mm heap;
> - struct nouveau_sched sched;
> + struct nouveau_sched *sched;
> };
>
> struct nouveau_abi16 {
> diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c
> index 6f6c31a9937b..a947e1d5f309 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_drm.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
> @@ -201,7 +201,8 @@ nouveau_cli_fini(struct nouveau_cli *cli)
> WARN_ON(!list_empty(&cli->worker));
>
> usif_client_fini(cli);
> - nouveau_sched_fini(&cli->sched);
> + if (cli->sched)
> + nouveau_sched_destroy(&cli->sched);
> if (uvmm)
> nouveau_uvmm_fini(uvmm);
> nouveau_vmm_fini(&cli->svm);
> @@ -311,7 +312,7 @@ nouveau_cli_init(struct nouveau_drm *drm, const char *sname,
> cli->mem = &mems[ret];
>
> /* Don't pass in the (shared) sched_wq in order to let
> - * nouveau_sched_init() create a dedicated one for VM_BIND jobs.
> + * nouveau_sched_create() create a dedicated one for VM_BIND jobs.
> *
> * This is required to ensure that for VM_BIND jobs free_job() work and
> * run_job() work can always run concurrently and hence, free_job() work
> @@ -320,7 +321,7 @@ nouveau_cli_init(struct nouveau_drm *drm, const char *sname,
> * locks which indirectly or directly are held for allocations
> * elsewhere.
> */
> - ret = nouveau_sched_init(&cli->sched, drm, NULL, 1);
> + ret = nouveau_sched_create(&cli->sched, drm, NULL, 1);
> if (ret)
> goto done;
>
> diff --git a/drivers/gpu/drm/nouveau/nouveau_drv.h b/drivers/gpu/drm/nouveau/nouveau_drv.h
> index 8a6d94c8b163..e239c6bf4afa 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_drv.h
> +++ b/drivers/gpu/drm/nouveau/nouveau_drv.h
> @@ -98,7 +98,7 @@ struct nouveau_cli {
> bool disabled;
> } uvmm;
>
> - struct nouveau_sched sched;
> + struct nouveau_sched *sched;
>
> const struct nvif_mclass *mem;
>
> diff --git a/drivers/gpu/drm/nouveau/nouveau_exec.c b/drivers/gpu/drm/nouveau/nouveau_exec.c
> index bc5d71b79ab2..e65c0ef23bc7 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_exec.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_exec.c
> @@ -389,7 +389,7 @@ nouveau_exec_ioctl_exec(struct drm_device *dev,
> if (ret)
> goto out;
>
> - args.sched = &chan16->sched;
> + args.sched = chan16->sched;
> args.file_priv = file_priv;
> args.chan = chan;
>
> diff --git a/drivers/gpu/drm/nouveau/nouveau_sched.c b/drivers/gpu/drm/nouveau/nouveau_sched.c
> index dd98f6910f9c..32fa2e273965 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_sched.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_sched.c
> @@ -398,7 +398,7 @@ static const struct drm_sched_backend_ops nouveau_sched_ops = {
> .free_job = nouveau_sched_free_job,
> };
>
> -int
> +static int
> nouveau_sched_init(struct nouveau_sched *sched, struct nouveau_drm *drm,
> struct workqueue_struct *wq, u32 credit_limit)
> {
> @@ -453,7 +453,30 @@ nouveau_sched_init(struct nouveau_sched *sched, struct nouveau_drm *drm,
> return ret;
> }
>
> -void
> +int
> +nouveau_sched_create(struct nouveau_sched **psched, struct nouveau_drm *drm,
> + struct workqueue_struct *wq, u32 credit_limit)
> +{
> + struct nouveau_sched *sched;
> + int ret;
> +
> + sched = kzalloc(sizeof(*sched), GFP_KERNEL);
> + if (!sched)
> + return -ENOMEM;
> +
> + ret = nouveau_sched_init(sched, drm, wq, credit_limit);
> + if (ret) {
> + kfree(sched);
> + return ret;
> + }
> +
> + *psched = sched;
> +
> + return 0;
> +}
> +
> +
> +static void
> nouveau_sched_fini(struct nouveau_sched *sched)
> {
> struct drm_gpu_scheduler *drm_sched = &sched->base;
> @@ -471,3 +494,14 @@ nouveau_sched_fini(struct nouveau_sched *sched)
> if (sched->wq)
> destroy_workqueue(sched->wq);
> }
> +
> +void
> +nouveau_sched_destroy(struct nouveau_sched **psched)
> +{
> + struct nouveau_sched *sched = *psched;
> +
> + nouveau_sched_fini(sched);
> + kfree(sched);
> +
> + *psched = NULL;
> +}
> diff --git a/drivers/gpu/drm/nouveau/nouveau_sched.h b/drivers/gpu/drm/nouveau/nouveau_sched.h
> index a6528f5981e6..e1f01a23e6f6 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_sched.h
> +++ b/drivers/gpu/drm/nouveau/nouveau_sched.h
> @@ -111,8 +111,8 @@ struct nouveau_sched {
> } job;
> };
>
> -int nouveau_sched_init(struct nouveau_sched *sched, struct nouveau_drm *drm,
> - struct workqueue_struct *wq, u32 credit_limit);
> -void nouveau_sched_fini(struct nouveau_sched *sched);
> +int nouveau_sched_create(struct nouveau_sched **psched, struct nouveau_drm *drm,
> + struct workqueue_struct *wq, u32 credit_limit);
> +void nouveau_sched_destroy(struct nouveau_sched **psched);
>
> #endif
> diff --git a/drivers/gpu/drm/nouveau/nouveau_uvmm.c b/drivers/gpu/drm/nouveau/nouveau_uvmm.c
> index 4f223c972c6a..0a0a11dc9ec0 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_uvmm.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_uvmm.c
> @@ -1740,7 +1740,7 @@ nouveau_uvmm_ioctl_vm_bind(struct drm_device *dev,
> if (ret)
> return ret;
>
> - args.sched = &cli->sched;
> + args.sched = cli->sched;
> args.file_priv = file_priv;
>
> ret = nouveau_uvmm_vm_bind(&args);
>
> base-commit: 041261ac4c365e03b07427569d6735f8adfd21c8
> --
> 2.43.0
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] drm/nouveau: don't fini scheduler if not initialized
2024-02-02 17:14 ` [PATCH 1/2] drm/nouveau: don't fini scheduler if not initialized Timur Tabi
2024-02-02 17:24 ` Danilo Krummrich
@ 2024-02-14 23:48 ` Timur Tabi
2024-02-19 9:32 ` Danilo Krummrich
1 sibling, 1 reply; 9+ messages in thread
From: Timur Tabi @ 2024-02-14 23:48 UTC (permalink / raw)
To: nouveau, dakr; +Cc: dri-devel, kherbst, lyude, airlied
On Fri, 2024-02-02 at 17:14 +0000, Timur Tabi wrote:
> On Fri, 2024-02-02 at 01:05 +0100, Danilo Krummrich wrote:
> > nouveau_abi16_ioctl_channel_alloc() and nouveau_cli_init() simply call
> > their corresponding *_fini() counterpart. This can lead to
> > nouveau_sched_fini() being called without struct nouveau_sched ever
> > being initialized in the first place.
>
> Thanks, I've confirmed that these patches do fix the problem.
Looks like I spoke too soon, I just hit the problem with the drm-next tree.
I'm able to repro the problem by having r535_gsp_init() return an error.
r535_gsp_rpc_poll return -EINVAL (I'm testing my own GSP-RM build) and
nouveau_sched_fini() is called even though nouveau_sched_init() was never
called.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] drm/nouveau: don't fini scheduler if not initialized
2024-02-14 23:48 ` Timur Tabi
@ 2024-02-19 9:32 ` Danilo Krummrich
2024-02-20 19:45 ` Timur Tabi
0 siblings, 1 reply; 9+ messages in thread
From: Danilo Krummrich @ 2024-02-19 9:32 UTC (permalink / raw)
To: Timur Tabi, nouveau; +Cc: dri-devel, kherbst, lyude, airlied
On 2/15/24 00:48, Timur Tabi wrote:
> On Fri, 2024-02-02 at 17:14 +0000, Timur Tabi wrote:
>> On Fri, 2024-02-02 at 01:05 +0100, Danilo Krummrich wrote:
>>> nouveau_abi16_ioctl_channel_alloc() and nouveau_cli_init() simply call
>>> their corresponding *_fini() counterpart. This can lead to
>>> nouveau_sched_fini() being called without struct nouveau_sched ever
>>> being initialized in the first place.
>>
>> Thanks, I've confirmed that these patches do fix the problem.
>
> Looks like I spoke too soon, I just hit the problem with the drm-next tree.
Did you apply the patch to drm-next?
>
> I'm able to repro the problem by having r535_gsp_init() return an error.
> r535_gsp_rpc_poll return -EINVAL (I'm testing my own GSP-RM build) and
> nouveau_sched_fini() is called even though nouveau_sched_init() was never
> called.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] drm/nouveau: don't fini scheduler if not initialized
2024-02-19 9:32 ` Danilo Krummrich
@ 2024-02-20 19:45 ` Timur Tabi
2024-02-20 19:56 ` Danilo Krummrich
0 siblings, 1 reply; 9+ messages in thread
From: Timur Tabi @ 2024-02-20 19:45 UTC (permalink / raw)
To: nouveau, dakr; +Cc: dri-devel, kherbst, lyude, airlied
[-- Attachment #1: Type: text/plain, Size: 344 bytes --]
On Mon, 2024-02-19 at 10:32 +0100, Danilo Krummrich wrote:
Looks like I spoke too soon, I just hit the problem with the drm-next tree.
Did you apply the patch to drm-next?
Ugh, you're right. I don't how I got confused, but I could have sworn that I saw your two patches in drm-next, but they are not there.
Sorry for the noise.
[-- Attachment #2: Type: text/html, Size: 825 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] drm/nouveau: don't fini scheduler if not initialized
2024-02-20 19:45 ` Timur Tabi
@ 2024-02-20 19:56 ` Danilo Krummrich
0 siblings, 0 replies; 9+ messages in thread
From: Danilo Krummrich @ 2024-02-20 19:56 UTC (permalink / raw)
To: Timur Tabi, nouveau; +Cc: dri-devel, kherbst, lyude, airlied
On 2/20/24 20:45, Timur Tabi wrote:
> On Mon, 2024-02-19 at 10:32 +0100, Danilo Krummrich wrote:
>>> Looks like I spoke too soon, I just hit the problem with the drm-next tree.
>>
>> Did you apply the patch to drm-next?
>
> Ugh, you're right. I don't how I got confused, but I could have sworn that I saw your two patches in drm-next, but they are not there.
>
> Sorry for the noise.
No worries, thanks for testing! :-)
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2024-02-20 19:56 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-02-02 0:05 [PATCH 1/2] drm/nouveau: don't fini scheduler if not initialized Danilo Krummrich
2024-02-02 0:05 ` [PATCH 2/2] drm/nouveau: omit to create schedulers using the legacy uAPI Danilo Krummrich
2024-02-02 17:14 ` [PATCH 1/2] drm/nouveau: don't fini scheduler if not initialized Timur Tabi
2024-02-02 17:24 ` Danilo Krummrich
2024-02-14 23:48 ` Timur Tabi
2024-02-19 9:32 ` Danilo Krummrich
2024-02-20 19:45 ` Timur Tabi
2024-02-20 19:56 ` Danilo Krummrich
2024-02-09 19:39 ` Dave Airlie
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).