drm/msm/gem: drm_exec conversion
Subsystems (7)
cover drm/msm/gem: drm_exec conversion
From: Rob Clark <[email protected]>
Simplify the exec path (removing a legacy optimization) and convert to drm_exec. One drm_exec patch to allow passing in the expected # of GEM objects to avoid re-allocation.
I'd be a bit happier if I could avoid the extra objects table allocation in drm_exec in the first place, but wasn't really happy with any of the things I tried to get rid of that.
Rob Clark (7):
drm/msm/gem: Remove "valid" tracking drm/msm/gem: Remove submit_unlock_unpin_bo() drm/msm/gem: Don't queue job to sched in error cases drm/msm/gem: Split out submit_unpin_objects() helper drm/msm/gem: Cleanup submit_cleanup_bo() drm/exec: Pass in initial # of objects drm/msm/gem: Convert to drm_exec
drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 2 +- drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c | 4 +- drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 4 +- drivers/gpu/drm/amd/amdgpu/amdgpu_mes.c | 4 +- drivers/gpu/drm/drm_exec.c | 15 +- drivers/gpu/drm/msm/Kconfig | 1 + drivers/gpu/drm/msm/msm_gem.h | 13 +- drivers/gpu/drm/msm/msm_gem_submit.c | 197 ++++++------------------ drivers/gpu/drm/msm/msm_ringbuffer.c | 3 +- drivers/gpu/drm/nouveau/nouveau_exec.c | 2 +- drivers/gpu/drm/nouveau/nouveau_uvmm.c | 2 +- include/drm/drm_exec.h | 2 +- 12 files changed, 79 insertions(+), 170 deletions(-)
Signature · 1 line
2.41.0
PATCH 1/7 drm/msm/gem: Remove "valid" tracking
From: Rob Clark <[email protected]>
This was a small optimization for pre-soft-pin userspace. But mesa switched to soft-pin nearly 5yrs ago. So lets drop the optimization and simplify the code.
Signed-off-by Rob Clark <[email protected]>
--- drivers/gpu/drm/msm/msm_gem.h | 2 -- drivers/gpu/drm/msm/msm_gem_submit.c | 44 +++++----------------------- 2 files changed, 8 insertions(+), 38 deletions(-)
drivers/gpu/drm/msm/msm_gem.h +0 -2
@@ -271,7 +271,6 @@ struct msm_gem_submit {struct msm_gpu_submitqueue *queue;struct pid *pid;/* submitting process */ bool fault_dumped;/* Limit devcoredump dumping to one per submit */ -bool valid;/* true if no cmdstream patching needed */ bool in_rb;/* "sudo" mode, copy cmds into RB */ struct msm_ringbuffer *ring;unsigned int nr_cmds;
@@ -288,7 +287,6 @@ struct msm_gem_submit { } *cmd;/* array of size nr_cmds */ struct {/* make sure these don't conflict w/ MSM_SUBMIT_BO_x */ -#define BO_VALID 0x8000/* is current addr in cmdstream correct/valid? */ #define BO_LOCKED 0x4000/* obj lock is held */ #define BO_PINNED 0x2000/* obj (pages) is pinned and on active list */ uint32_t flags;
drivers/gpu/drm/msm/msm_gem_submit.c +8 -36
116 lines not on this page · raw message
Signature · 1 line
2.41.0
PATCH 2/7 drm/msm/gem: Remove submit_unlock_unpin_bo()
In reply to Rob ClarkFrom: Rob Clark <[email protected]>
The only point it is called is before pinning objects, so the "unpin" part of the name is fiction. Just remove call submit_cleanup_bo() directly.
Signed-off-by Rob Clark <[email protected]>
--- drivers/gpu/drm/msm/msm_gem_submit.c | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-)
drivers/gpu/drm/msm/msm_gem_submit.c +2 -8
@@ -272,12 +272,6 @@ static void submit_cleanup_bo(struct msm_gem_submit *submit, int i,dma_resv_unlock (obj->resv); } -static void submit_unlock_unpin_bo (struct msm_gem_submit *submit,int i) -{ -unsigned cleanup_flags =BO_PINNED |BO_LOCKED ; -submit_cleanup_bo (submit, i, cleanup_flags); -} -/* This is where we make sure all the bo's are reserved and pin'd: */ static int submit_lock_objects (struct msm_gem_submit *submit) {
@@ -313,10 +307,10 @@ static int submit_lock_objects(struct msm_gem_submit *submit) }for (; i >=0 ; i--) -submit_unlock_unpin_bo (submit, i); +submit_cleanup_bo (submit, i,BO_LOCKED );if (slow_locked >0 ) -submit_unlock_unpin_bo (submit, slow_locked); +submit_cleanup_bo (submit, slow_locked,BO_LOCKED );if (ret == -EDEADLK ) {struct drm_gem_object *obj = submit->bos[contended].obj;
Signature · 1 line
2.41.0
Re: [PATCH 2/7] drm/msm/gem: Remove submit_unlock_unpin_bo()
Quoted 5 lines · Rob Clark
From: Rob Clark <[email protected]>
The only point it is called is before pinning objects, so the "unpin" part of the name is fiction. Just remove call
submit_cleanup_bo()
Nit: 'remove it and call'
Other than that:
Reviewed-by Dmitry Baryshkov <[email protected]>
Quoted 6 lines
directly.
Signed-off-by Rob Clark <[email protected]>--- drivers/gpu/drm/msm/msm_gem_submit.c | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-)
Signature · 2 lines
With best wishes Dmitry
PATCH 3/7 drm/msm/gem: Don't queue job to sched in error cases
In reply to Rob ClarkFrom: Rob Clark <[email protected]>
We shouldn't be running the job in error cases. This also avoids having to think too hard about where the objs get unpinned (and if necessary, the resv takes over tracking that the obj is busy).. ie. error cases it always happens synchronously, and normal cases it happens from scheduler job_run() callback.
Signed-off-by Rob Clark <[email protected]>
--- drivers/gpu/drm/msm/msm_gem_submit.c | 3 +++ 1 file changed, 3 insertions(+)
drivers/gpu/drm/msm/msm_gem_submit.c +3 -0
@@ -946,6 +946,9 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data, } } +if (ret) + goto out; +submit_attach_object_fences (submit);/* The scheduler owns a ref now: */
Signature · 1 line
2.41.0
Re: [PATCH 3/7] drm/msm/gem: Don't queue job to sched in error cases
Quoted 10 lines · Rob Clark
From: Rob Clark <[email protected]>
We shouldn't be running the job in error cases. This also avoids having to think too hard about where the objs get unpinned (and if necessary, the resv takes over tracking that the obj is busy).. ie. error cases it always happens synchronously, and normal cases it happens from scheduler
job_run()callback.Signed-off-by Rob Clark <[email protected]>
Reviewed-by Dmitry Baryshkov <[email protected]>
Quoted 3 lines
--- drivers/gpu/drm/msm/msm_gem_submit.c | 3 +++ 1 file changed, 3 insertions(+)
Signature · 2 lines
With best wishes Dmitry
PATCH 4/7 drm/msm/gem: Split out submit_unpin_objects() helper
In reply to Rob ClarkFrom: Rob Clark <[email protected]>
Untangle unpinning from unlock/unref loop. The unpin only happens in error paths so it is easier to decouple from the normal unlock path.
Since we never have an intermediate state where a subset of buffers are pinned (ie. we never bail out of the pin or unpin loops) we can replace the bo state flag bit with a global flag in the submit.
Signed-off-by Rob Clark <[email protected]>
--- drivers/gpu/drm/msm/msm_gem.h | 6 +++--- drivers/gpu/drm/msm/msm_gem_submit.c | 22 +++++++++++++++++----- drivers/gpu/drm/msm/msm_ringbuffer.c | 3 ++- 3 files changed, 22 insertions(+), 9 deletions(-)
drivers/gpu/drm/msm/msm_gem.h +3 -3
@@ -270,8 +270,9 @@ struct msm_gem_submit {int fence_id;/* key into queue->fence_idr */ struct msm_gpu_submitqueue *queue;struct pid *pid;/* submitting process */ -bool fault_dumped;/* Limit devcoredump dumping to one per submit */ -bool in_rb;/* "sudo" mode, copy cmds into RB */ + bool bos_pinned : 1; + bool fault_dumped:1;/* Limit devcoredump dumping to one per submit */ + bool in_rb : 1; /* "sudo" mode, copy cmds into RB */struct msm_ringbuffer *ring;unsigned int nr_cmds;unsigned int nr_bos;
@@ -288,7 +289,6 @@ struct msm_gem_submit {struct {/* make sure these don't conflict w/ MSM_SUBMIT_BO_x */ #define BO_LOCKED 0x4000/* obj lock is held */ -#define BO_PINNED 0x2000/* obj (pages) is pinned and on active list */ uint32_t flags;union {struct drm_gem_object *obj;
drivers/gpu/drm/msm/msm_gem_submit.c +17 -5
@@ -265,9 +265,6 @@ static void submit_cleanup_bo(struct msm_gem_submit *submit, int i,*/ submit->bos[i].flags &= ~cleanup_flags; -if (flags &BO_PINNED ) -msm_gem_unpin_locked (obj); -if (flags &BO_LOCKED )dma_resv_unlock (obj->resv); }
@@ -407,13 +404,28 @@ static int submit_pin_objects(struct msm_gem_submit *submit)mutex_lock (&priv->lru.lock);for (i =0 ; i < submit->nr_bos; i++) {msm_gem_pin_obj_locked (submit->bos[i].obj); - submit->bos[i].flags |=BO_PINNED ; }mutex_unlock (&priv->lru.lock); + submit->bos_pinned = true; +return ret; } +static void submit_unpin_objects (struct msm_gem_submit *submit) +{ +if (!submit->bos_pinned) +return ; + +for (int i =0 ; i < submit->nr_bos; i++) { +struct drm_gem_object *obj = submit->bos[i].obj; + +msm_gem_unpin_locked (obj); + } + + submit->bos_pinned = false; +} +static void submit_attach_object_fences (struct msm_gem_submit *submit) {int i;
@@ -525,7 +537,7 @@ static void submit_cleanup(struct msm_gem_submit *submit, bool error)unsigned i;if (error) - cleanup_flags |=BO_PINNED ; +submit_unpin_objects (submit);for (i =0 ; i < submit->nr_bos; i++) {struct drm_gem_object *obj = submit->bos[i].obj;
drivers/gpu/drm/msm/msm_ringbuffer.c +2 -1
@@ -29,9 +29,10 @@ static struct dma_fence *msm_job_run(struct drm_sched_job *job)struct drm_gem_object *obj = submit->bos[i].obj;msm_gem_unpin_active (obj); - submit->bos[i].flags &= ~BO_PINNED ; } + submit->bos_pinned = false; +mutex_unlock (&priv->lru.lock);msm_gpu_submit (gpu, submit);
Signature · 1 line
2.41.0
PATCH 5/7 drm/msm/gem: Cleanup submit_cleanup_bo()
In reply to Rob ClarkFrom: Rob Clark <[email protected]>
Now that it only handles unlock duty, drop the superfluous arg and rename it.
Signed-off-by Rob Clark <[email protected]>
--- drivers/gpu/drm/msm/msm_gem_submit.c | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-)
drivers/gpu/drm/msm/msm_gem_submit.c +5 -10
@@ -248,14 +248,10 @@ static int submit_lookup_cmds(struct msm_gem_submit *submit,return ret; } -/* Unwind bo state, according to cleanup_flags. In the success case, only -* the lock is dropped at the end of the submit (and active/pin ref is dropped -* later when the submit is retired). -*/ -static void submit_cleanup_bo (struct msm_gem_submit *submit,int i, -unsigned cleanup_flags) +static void submit_unlock_bo (struct msm_gem_submit *submit,int i) {struct drm_gem_object *obj = submit->bos[i].obj; +unsigned cleanup_flags =BO_LOCKED ;unsigned flags = submit->bos[i].flags & cleanup_flags;/*
@@ -304,10 +300,10 @@ static int submit_lock_objects(struct msm_gem_submit *submit) }for (; i >=0 ; i--) -submit_cleanup_bo (submit, i,BO_LOCKED ); +submit_unlock_bo (submit, i);if (slow_locked >0 ) -submit_cleanup_bo (submit, slow_locked,BO_LOCKED ); +submit_unlock_bo (submit, slow_locked);if (ret == -EDEADLK ) {struct drm_gem_object *obj = submit->bos[contended].obj;
@@ -533,7 +529,6 @@ static int submit_reloc(struct msm_gem_submit *submit, struct drm_gem_object *ob*/ static void submit_cleanup (struct msm_gem_submit *submit,bool error) { -unsigned cleanup_flags =BO_LOCKED ;unsigned i;if (error)
@@ -541,7 +536,7 @@ static void submit_cleanup(struct msm_gem_submit *submit, bool error)for (i =0 ; i < submit->nr_bos; i++) {struct drm_gem_object *obj = submit->bos[i].obj; -submit_cleanup_bo (submit, i, cleanup_flags); +submit_unlock_bo (submit, i);if (error)drm_gem_object_put (obj); }
Signature · 1 line
2.41.0
PATCH 6/7 drm/exec: Pass in initial # of objects
In reply to Rob ClarkFrom: Rob Clark <[email protected]>
In cases where the # is known ahead of time, it is silly to do the table resize dance.
Signed-off-by Rob Clark <[email protected]>
--- drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 2 +- drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c | 4 ++-- drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 4 ++-- drivers/gpu/drm/amd/amdgpu/amdgpu_mes.c | 4 ++-- drivers/gpu/drm/drm_exec.c | 15 ++++++++++++--- drivers/gpu/drm/nouveau/nouveau_exec.c | 2 +- drivers/gpu/drm/nouveau/nouveau_uvmm.c | 2 +- include/drm/drm_exec.h | 2 +- 8 files changed, 22 insertions(+), 13 deletions(-)
drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c +1 -1
@@ -65,7 +65,7 @@ static int amdgpu_cs_parser_init(struct amdgpu_cs_parser *p, }amdgpu_sync_create (&p->sync); -drm_exec_init (&p->exec,DRM_EXEC_INTERRUPTIBLE_WAIT ); +drm_exec_init (&p->exec,DRM_EXEC_INTERRUPTIBLE_WAIT ,0 );return 0 ; }
drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c +2 -2
@@ -70,7 +70,7 @@ int amdgpu_map_static_csa(struct amdgpu_device *adev, struct amdgpu_vm *vm,struct drm_exec exec;int r; -drm_exec_init (&exec,DRM_EXEC_INTERRUPTIBLE_WAIT ); +drm_exec_init (&exec,DRM_EXEC_INTERRUPTIBLE_WAIT ,0 ); drm_exec_until_all_locked(&exec) { r =amdgpu_vm_lock_pd (vm, &exec,0 );if (likely (!r))
@@ -110,7 +110,7 @@ int amdgpu_unmap_static_csa(struct amdgpu_device *adev, struct amdgpu_vm *vm,struct drm_exec exec;int r; -drm_exec_init (&exec,DRM_EXEC_INTERRUPTIBLE_WAIT ); +drm_exec_init (&exec,DRM_EXEC_INTERRUPTIBLE_WAIT ,0 ); drm_exec_until_all_locked(&exec) { r =amdgpu_vm_lock_pd (vm, &exec,0 );if (likely (!r))
drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c +2 -2
@@ -203,7 +203,7 @@ static void amdgpu_gem_object_close(struct drm_gem_object *obj,struct drm_exec exec;long r; -drm_exec_init (&exec,DRM_EXEC_IGNORE_DUPLICATES ); +drm_exec_init (&exec,DRM_EXEC_IGNORE_DUPLICATES ,0 ); drm_exec_until_all_locked(&exec) { r =drm_exec_prepare_obj (&exec, &bo->tbo.base,1 );drm_exec_retry_on_contention (&exec);
@@ -739,7 +739,7 @@ int amdgpu_gem_va_ioctl(struct drm_device *dev, void *data, }drm_exec_init (&exec,DRM_EXEC_INTERRUPTIBLE_WAIT | -DRM_EXEC_IGNORE_DUPLICATES ); +DRM_EXEC_IGNORE_DUPLICATES ,0 ); drm_exec_until_all_locked(&exec) {if (gobj) { r =drm_exec_lock_obj (&exec, gobj);
drivers/gpu/drm/amd/amdgpu/amdgpu_mes.c +2 -2
@@ -1105,7 +1105,7 @@ int amdgpu_mes_ctx_map_meta_data(struct amdgpu_device *adev,amdgpu_sync_create (&sync); -drm_exec_init (&exec,0 ); +drm_exec_init (&exec,0 ,0 ); drm_exec_until_all_locked(&exec) { r =drm_exec_lock_obj (&exec, &ctx_data->meta_data_obj->tbo.base);
@@ -1176,7 +1176,7 @@ int amdgpu_mes_ctx_unmap_meta_data(struct amdgpu_device *adev,struct drm_exec exec;long r; -drm_exec_init (&exec,0 ); +drm_exec_init (&exec,0 ,0 ); drm_exec_until_all_locked(&exec) { r =drm_exec_lock_obj (&exec, &ctx_data->meta_data_obj->tbo.base);
drivers/gpu/drm/drm_exec.c +12 -3
@@ -69,16 +69,25 @@ static void drm_exec_unlock_all(struct drm_exec *exec)* drm_exec_init - initialize a drm_exec object * @exec: the drm_exec object to initialize * @flags: controls locking behavior, see DRM_EXEC_* defines +* @nr: the initial # of objects * * Initialize the object and make sure that we can track locked objects. +* +* If nr is non-zero then it is used as the initial objects table size. +* In either case, the table will grow (be re-allocated) on demand. */ -void drm_exec_init (struct drm_exec *exec,uint32_t flags) +void drm_exec_init (struct drm_exec *exec,uint32_t flags,unsigned nr) { +size_t sz =PAGE_SIZE ; + +if (nr) + sz = (size_t )nr *sizeof (void *); + exec->flags = flags; - exec->objects =kmalloc (PAGE_SIZE ,GFP_KERNEL ); + exec->objects =kmalloc (sz,GFP_KERNEL );/* If allocation here fails, just delay that till the first use */ - exec->max_objects = exec->objects ?PAGE_SIZE /sizeof (void *) :0 ; + exec->max_objects = exec->objects ? sz /sizeof (void *) :0 ; exec->num_objects =0 ; exec->contended =DRM_EXEC_DUMMY ; exec->prelocked =NULL ;
drivers/gpu/drm/nouveau/nouveau_exec.c +1 -1
@@ -103,7 +103,7 @@ nouveau_exec_job_submit(struct nouveau_job *job)nouveau_uvmm_lock (uvmm);drm_exec_init (exec,DRM_EXEC_INTERRUPTIBLE_WAIT | -DRM_EXEC_IGNORE_DUPLICATES ); +DRM_EXEC_IGNORE_DUPLICATES ,0 );drm_exec_until_all_locked (exec) {struct drm_gpuva *va;
drivers/gpu/drm/nouveau/nouveau_uvmm.c +1 -1
@@ -1288,7 +1288,7 @@ nouveau_uvmm_bind_job_submit(struct nouveau_job *job) }drm_exec_init (exec,DRM_EXEC_INTERRUPTIBLE_WAIT | -DRM_EXEC_IGNORE_DUPLICATES ); +DRM_EXEC_IGNORE_DUPLICATES ,0 );drm_exec_until_all_locked (exec) {list_for_each_op (op, &bind_job->ops) {struct drm_gpuva_op *va_op;
include/drm/drm_exec.h +1 -1
@@ -135,7 +135,7 @@ static inline bool drm_exec_is_contended(struct drm_exec *exec)return !!exec->contended; } -void drm_exec_init (struct drm_exec *exec,uint32_t flags); +void drm_exec_init (struct drm_exec *exec,uint32_t flags,unsigned nr);void drm_exec_fini (struct drm_exec *exec);bool drm_exec_cleanup (struct drm_exec *exec);int drm_exec_lock_obj (struct drm_exec *exec,struct drm_gem_object *obj);
Signature · 1 line
2.41.0
Christian König
maintainerRe: [PATCH 6/7] drm/exec: Pass in initial # of objects
Am 27.10.23 um 18:58 schrieb Rob Clark:
Quoted 4 lines
From: Rob Clark <[email protected]>
In cases where the # is known ahead of time, it is silly to do the table resize dance.
Ah, yes that was my initial implementation as well, but I ditched that because nobody actually used it.
One comment below.
Quoted 108 lines
Signed-off-by Rob Clark <[email protected]>--- drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 2 +- drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c | 4 ++-- drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 4 ++-- drivers/gpu/drm/amd/amdgpu/amdgpu_mes.c | 4 ++-- drivers/gpu/drm/drm_exec.c | 15 ++++++++++++--- drivers/gpu/drm/nouveau/nouveau_exec.c | 2 +- drivers/gpu/drm/nouveau/nouveau_uvmm.c | 2 +- include/drm/drm_exec.h | 2 +- 8 files changed, 22 insertions(+), 13 deletions(-)drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c +1
-1@@ -65,7 +65,7 @@ static int amdgpu_cs_parser_init(struct amdgpu_cs_parser *p, }amdgpu_sync_create (&p->sync); -drm_exec_init (&p->exec,DRM_EXEC_INTERRUPTIBLE_WAIT ); +drm_exec_init (&p->exec,DRM_EXEC_INTERRUPTIBLE_WAIT ,0 );return 0 ; }drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c +2
-2@@ -70,7 +70,7 @@ int amdgpu_map_static_csa(struct amdgpu_device *adev, struct amdgpu_vm *vm,struct drm_exec exec;int r; -drm_exec_init (&exec,DRM_EXEC_INTERRUPTIBLE_WAIT ); +drm_exec_init (&exec,DRM_EXEC_INTERRUPTIBLE_WAIT ,0 ); drm_exec_until_all_locked(&exec) { r =amdgpu_vm_lock_pd (vm, &exec,0 );if (likely (!r))@@ -110,7 +110,7 @@ int amdgpu_unmap_static_csa(struct amdgpu_device *adev, struct amdgpu_vm *vm,struct drm_exec exec;int r; -drm_exec_init (&exec,DRM_EXEC_INTERRUPTIBLE_WAIT ); +drm_exec_init (&exec,DRM_EXEC_INTERRUPTIBLE_WAIT ,0 ); drm_exec_until_all_locked(&exec) { r =amdgpu_vm_lock_pd (vm, &exec,0 );if (likely (!r))drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c +2
-2@@ -203,7 +203,7 @@ static void amdgpu_gem_object_close(struct drm_gem_object *obj,struct drm_exec exec;long r; -drm_exec_init (&exec,DRM_EXEC_IGNORE_DUPLICATES ); +drm_exec_init (&exec,DRM_EXEC_IGNORE_DUPLICATES ,0 ); drm_exec_until_all_locked(&exec) { r =drm_exec_prepare_obj (&exec, &bo->tbo.base,1 );drm_exec_retry_on_contention (&exec);@@ -739,7 +739,7 @@ int amdgpu_gem_va_ioctl(struct drm_device *dev, void *data, }drm_exec_init (&exec,DRM_EXEC_INTERRUPTIBLE_WAIT | -DRM_EXEC_IGNORE_DUPLICATES ); +DRM_EXEC_IGNORE_DUPLICATES ,0 ); drm_exec_until_all_locked(&exec) {if (gobj) { r =drm_exec_lock_obj (&exec, gobj);drivers/gpu/drm/amd/amdgpu/amdgpu_mes.c +2
-2@@ -1105,7 +1105,7 @@ int amdgpu_mes_ctx_map_meta_data(struct amdgpu_device *adev,amdgpu_sync_create (&sync); -drm_exec_init (&exec,0 ); +drm_exec_init (&exec,0 ,0 ); drm_exec_until_all_locked(&exec) { r =drm_exec_lock_obj (&exec, &ctx_data->meta_data_obj->tbo.base);@@ -1176,7 +1176,7 @@ int amdgpu_mes_ctx_unmap_meta_data(struct amdgpu_device *adev,struct drm_exec exec;long r; -drm_exec_init (&exec,0 ); +drm_exec_init (&exec,0 ,0 ); drm_exec_until_all_locked(&exec) { r =drm_exec_lock_obj (&exec, &ctx_data->meta_data_obj->tbo.base);drivers/gpu/drm/drm_exec.c +4
-1@@ -69,16 +69,25 @@ static void drm_exec_unlock_all(struct drm_exec *exec)* drm_exec_init - initialize a drm_exec object * @exec: the drm_exec object to initialize * @flags: controls locking behavior, see DRM_EXEC_* defines +* @nr: the initial # of objects * * Initialize the object and make sure that we can track locked objects. +* +* If nr is non-zero then it is used as the initial objects table size. +* In either case, the table will grow (be re-allocated) on demand. */ -void drm_exec_init (struct drm_exec *exec,uint32_t flags)
drivers/gpu/drm/drm_exec.c static void drm_exec_unlock_all(struct drm_exec *exec) · line 87
Quoted 10 lines
drivers/gpu/drm/drm_exec.c +7
-1+void drm_exec_init (struct drm_exec *exec,uint32_t flags,unsigned nr) { +size_t sz =PAGE_SIZE ; + +if (nr) + sz = (size_t )nr *sizeof (void *); + exec->flags = flags; - exec->objects =kmalloc (PAGE_SIZE ,GFP_KERNEL ); + exec->objects =kmalloc (sz,GFP_KERNEL );
Please use k*v*malloc() here since we can't predict how large that will be.
With that fixed the patch is Reviewed-by: Christian König <[email protected]>.
Regards,
Christian.
Quoted 46 lines
drivers/gpu/drm/drm_exec.c +1
-1/* If allocation here fails, just delay that till the first use */ - exec->max_objects = exec->objects ?PAGE_SIZE /sizeof (void *) :0 ; + exec->max_objects = exec->objects ? sz /sizeof (void *) :0 ; exec->num_objects =0 ; exec->contended =DRM_EXEC_DUMMY ; exec->prelocked =NULL ;drivers/gpu/drm/nouveau/nouveau_exec.c +1
-1@@ -103,7 +103,7 @@ nouveau_exec_job_submit(struct nouveau_job *job)nouveau_uvmm_lock (uvmm);drm_exec_init (exec,DRM_EXEC_INTERRUPTIBLE_WAIT | -DRM_EXEC_IGNORE_DUPLICATES ); +DRM_EXEC_IGNORE_DUPLICATES ,0 );drm_exec_until_all_locked (exec) {struct drm_gpuva *va;drivers/gpu/drm/nouveau/nouveau_uvmm.c +1
-1@@ -1288,7 +1288,7 @@ nouveau_uvmm_bind_job_submit(struct nouveau_job *job) }drm_exec_init (exec,DRM_EXEC_INTERRUPTIBLE_WAIT | -DRM_EXEC_IGNORE_DUPLICATES ); +DRM_EXEC_IGNORE_DUPLICATES ,0 );drm_exec_until_all_locked (exec) {list_for_each_op (op, &bind_job->ops) {struct drm_gpuva_op *va_op;include/drm/drm_exec.h +1
-1@@ -135,7 +135,7 @@ static inline bool drm_exec_is_contended(struct drm_exec *exec)return !!exec->contended; } -void drm_exec_init (struct drm_exec *exec,uint32_t flags); +void drm_exec_init (struct drm_exec *exec,uint32_t flags,unsigned nr);void drm_exec_fini (struct drm_exec *exec);bool drm_exec_cleanup (struct drm_exec *exec);int drm_exec_lock_obj (struct drm_exec *exec,struct drm_gem_object *obj);
Re: [PATCH 6/7] drm/exec: Pass in initial # of objects
Quoted 124 lines · Christian König
Am 27.10.23 um 18:58 schrieb Rob Clark:
Quoted 4 lines
From: Rob Clark <[email protected]>
In cases where the # is known ahead of time, it is silly to do the table resize dance.
Ah, yes that was my initial implementation as well, but I ditched that because nobody actually used it.
One comment below.
Quoted 112 lines
Signed-off-by Rob Clark <[email protected]>--- drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 2 +- drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c | 4 ++-- drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 4 ++-- drivers/gpu/drm/amd/amdgpu/amdgpu_mes.c | 4 ++-- drivers/gpu/drm/drm_exec.c | 15 ++++++++++++--- drivers/gpu/drm/nouveau/nouveau_exec.c | 2 +- drivers/gpu/drm/nouveau/nouveau_uvmm.c | 2 +- include/drm/drm_exec.h | 2 +- 8 files changed, 22 insertions(+), 13 deletions(-)drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c +1
-1@@ -65,7 +65,7 @@ static int amdgpu_cs_parser_init(struct amdgpu_cs_parser *p, }amdgpu_sync_create (&p->sync); -drm_exec_init (&p->exec,DRM_EXEC_INTERRUPTIBLE_WAIT ); +drm_exec_init (&p->exec,DRM_EXEC_INTERRUPTIBLE_WAIT ,0 );return 0 ; }drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c +2
-2@@ -70,7 +70,7 @@ int amdgpu_map_static_csa(struct amdgpu_device *adev, struct amdgpu_vm *vm,struct drm_exec exec;int r; -drm_exec_init (&exec,DRM_EXEC_INTERRUPTIBLE_WAIT ); +drm_exec_init (&exec,DRM_EXEC_INTERRUPTIBLE_WAIT ,0 ); drm_exec_until_all_locked(&exec) { r =amdgpu_vm_lock_pd (vm, &exec,0 );if (likely (!r))@@ -110,7 +110,7 @@ int amdgpu_unmap_static_csa(struct amdgpu_device *adev, struct amdgpu_vm *vm,struct drm_exec exec;int r; -drm_exec_init (&exec,DRM_EXEC_INTERRUPTIBLE_WAIT ); +drm_exec_init (&exec,DRM_EXEC_INTERRUPTIBLE_WAIT ,0 ); drm_exec_until_all_locked(&exec) { r =amdgpu_vm_lock_pd (vm, &exec,0 );if (likely (!r))drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c +2
-2@@ -203,7 +203,7 @@ static void amdgpu_gem_object_close(struct drm_gem_object *obj,struct drm_exec exec;long r; -drm_exec_init (&exec,DRM_EXEC_IGNORE_DUPLICATES ); +drm_exec_init (&exec,DRM_EXEC_IGNORE_DUPLICATES ,0 ); drm_exec_until_all_locked(&exec) { r =drm_exec_prepare_obj (&exec, &bo->tbo.base,1 );drm_exec_retry_on_contention (&exec);@@ -739,7 +739,7 @@ int amdgpu_gem_va_ioctl(struct drm_device *dev, void *data, }drm_exec_init (&exec,DRM_EXEC_INTERRUPTIBLE_WAIT | -DRM_EXEC_IGNORE_DUPLICATES ); +DRM_EXEC_IGNORE_DUPLICATES ,0 ); drm_exec_until_all_locked(&exec) {if (gobj) { r =drm_exec_lock_obj (&exec, gobj);drivers/gpu/drm/amd/amdgpu/amdgpu_mes.c +2
-2@@ -1105,7 +1105,7 @@ int amdgpu_mes_ctx_map_meta_data(struct amdgpu_device *adev,amdgpu_sync_create (&sync); -drm_exec_init (&exec,0 ); +drm_exec_init (&exec,0 ,0 ); drm_exec_until_all_locked(&exec) { r =drm_exec_lock_obj (&exec, &ctx_data->meta_data_obj->tbo.base);@@ -1176,7 +1176,7 @@ int amdgpu_mes_ctx_unmap_meta_data(struct amdgpu_device *adev,struct drm_exec exec;long r; -drm_exec_init (&exec,0 ); +drm_exec_init (&exec,0 ,0 ); drm_exec_until_all_locked(&exec) { r =drm_exec_lock_obj (&exec, &ctx_data->meta_data_obj->tbo.base);drivers/gpu/drm/drm_exec.c +7
-1@@ -69,16 +69,25 @@ static void drm_exec_unlock_all(struct drm_exec *exec)* drm_exec_init - initialize a drm_exec object * @exec: the drm_exec object to initialize * @flags: controls locking behavior, see DRM_EXEC_* defines +* @nr: the initial # of objects * * Initialize the object and make sure that we can track locked objects. +* +* If nr is non-zero then it is used as the initial objects table size. +* In either case, the table will grow (be re-allocated) on demand. */ -void drm_exec_init (struct drm_exec *exec,uint32_t flags) +void drm_exec_init (struct drm_exec *exec,uint32_t flags,unsigned nr) { +size_t sz =PAGE_SIZE ; +
Quoted 8 lines
drivers/gpu/drm/drm_exec.c static void drm_exec_unlock_all(struct drm_exec *exec) · line 87
Quoted 6 lines
drivers/gpu/drm/drm_exec.c +4
-1+if (nr) + sz = (size_t )nr *sizeof (void *); + exec->flags = flags; - exec->objects =kmalloc (PAGE_SIZE ,GFP_KERNEL ); + exec->objects =kmalloc (sz,GFP_KERNEL );Please use k*v*malloc() here since we can't predict how large that will be.
or __GFP_NOWARN? If userspace (or kasan) is cheeky and asks for ~0 objects, we should probably just fail?
BR,
-R
Quoted 53 lines
With that fixed the patch is Reviewed-by: Christian König <[email protected]>.
Regards,
Christian.Quoted 46 lines
drivers/gpu/drm/drm_exec.c +1
-1/* If allocation here fails, just delay that till the first use */ - exec->max_objects = exec->objects ?PAGE_SIZE /sizeof (void *) :0 ; + exec->max_objects = exec->objects ? sz /sizeof (void *) :0 ; exec->num_objects =0 ; exec->contended =DRM_EXEC_DUMMY ; exec->prelocked =NULL ;drivers/gpu/drm/nouveau/nouveau_exec.c +1
-1@@ -103,7 +103,7 @@ nouveau_exec_job_submit(struct nouveau_job *job)nouveau_uvmm_lock (uvmm);drm_exec_init (exec,DRM_EXEC_INTERRUPTIBLE_WAIT | -DRM_EXEC_IGNORE_DUPLICATES ); +DRM_EXEC_IGNORE_DUPLICATES ,0 );drm_exec_until_all_locked (exec) {struct drm_gpuva *va;drivers/gpu/drm/nouveau/nouveau_uvmm.c +1
-1@@ -1288,7 +1288,7 @@ nouveau_uvmm_bind_job_submit(struct nouveau_job *job) }drm_exec_init (exec,DRM_EXEC_INTERRUPTIBLE_WAIT | -DRM_EXEC_IGNORE_DUPLICATES ); +DRM_EXEC_IGNORE_DUPLICATES ,0 );drm_exec_until_all_locked (exec) {list_for_each_op (op, &bind_job->ops) {struct drm_gpuva_op *va_op;include/drm/drm_exec.h +1
-1@@ -135,7 +135,7 @@ static inline bool drm_exec_is_contended(struct drm_exec *exec)return !!exec->contended; } -void drm_exec_init (struct drm_exec *exec,uint32_t flags); +void drm_exec_init (struct drm_exec *exec,uint32_t flags,unsigned nr);void drm_exec_fini (struct drm_exec *exec);bool drm_exec_cleanup (struct drm_exec *exec);int drm_exec_lock_obj (struct drm_exec *exec,struct drm_gem_object *obj);
Re: [PATCH 6/7] drm/exec: Pass in initial # of objects
Am 30.10.23 um 14:38 schrieb Rob Clark:
Quoted 130 lines
Quoted 120 lines · Christian König
Am 27.10.23 um 18:58 schrieb Rob Clark:
Quoted 4 lines
From: Rob Clark <[email protected]>
In cases where the # is known ahead of time, it is silly to do the table resize dance.
Ah, yes that was my initial implementation as well, but I ditched that because nobody actually used it.
One comment below.
Quoted 110 lines
Signed-off-by Rob Clark <[email protected]>--- drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 2 +- drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c | 4 ++-- drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 4 ++-- drivers/gpu/drm/amd/amdgpu/amdgpu_mes.c | 4 ++-- drivers/gpu/drm/drm_exec.c | 15 ++++++++++++--- drivers/gpu/drm/nouveau/nouveau_exec.c | 2 +- drivers/gpu/drm/nouveau/nouveau_uvmm.c | 2 +- include/drm/drm_exec.h | 2 +- 8 files changed, 22 insertions(+), 13 deletions(-)drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c +1
-1@@ -65,7 +65,7 @@ static int amdgpu_cs_parser_init(struct amdgpu_cs_parser *p, }amdgpu_sync_create (&p->sync); -drm_exec_init (&p->exec,DRM_EXEC_INTERRUPTIBLE_WAIT ); +drm_exec_init (&p->exec,DRM_EXEC_INTERRUPTIBLE_WAIT ,0 );return 0 ; }drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c +2
-2@@ -70,7 +70,7 @@ int amdgpu_map_static_csa(struct amdgpu_device *adev, struct amdgpu_vm *vm,struct drm_exec exec;int r; -drm_exec_init (&exec,DRM_EXEC_INTERRUPTIBLE_WAIT ); +drm_exec_init (&exec,DRM_EXEC_INTERRUPTIBLE_WAIT ,0 ); drm_exec_until_all_locked(&exec) { r =amdgpu_vm_lock_pd (vm, &exec,0 );if (likely (!r))@@ -110,7 +110,7 @@ int amdgpu_unmap_static_csa(struct amdgpu_device *adev, struct amdgpu_vm *vm,struct drm_exec exec;int r; -drm_exec_init (&exec,DRM_EXEC_INTERRUPTIBLE_WAIT ); +drm_exec_init (&exec,DRM_EXEC_INTERRUPTIBLE_WAIT ,0 ); drm_exec_until_all_locked(&exec) { r =amdgpu_vm_lock_pd (vm, &exec,0 );if (likely (!r))drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c +2
-2@@ -203,7 +203,7 @@ static void amdgpu_gem_object_close(struct drm_gem_object *obj,struct drm_exec exec;long r; -drm_exec_init (&exec,DRM_EXEC_IGNORE_DUPLICATES ); +drm_exec_init (&exec,DRM_EXEC_IGNORE_DUPLICATES ,0 ); drm_exec_until_all_locked(&exec) { r =drm_exec_prepare_obj (&exec, &bo->tbo.base,1 );drm_exec_retry_on_contention (&exec);@@ -739,7 +739,7 @@ int amdgpu_gem_va_ioctl(struct drm_device *dev, void *data, }drm_exec_init (&exec,DRM_EXEC_INTERRUPTIBLE_WAIT | -DRM_EXEC_IGNORE_DUPLICATES ); +DRM_EXEC_IGNORE_DUPLICATES ,0 ); drm_exec_until_all_locked(&exec) {if (gobj) { r =drm_exec_lock_obj (&exec, gobj);drivers/gpu/drm/amd/amdgpu/amdgpu_mes.c +2
-2@@ -1105,7 +1105,7 @@ int amdgpu_mes_ctx_map_meta_data(struct amdgpu_device *adev,amdgpu_sync_create (&sync); -drm_exec_init (&exec,0 ); +drm_exec_init (&exec,0 ,0 ); drm_exec_until_all_locked(&exec) { r =drm_exec_lock_obj (&exec, &ctx_data->meta_data_obj->tbo.base);@@ -1176,7 +1176,7 @@ int amdgpu_mes_ctx_unmap_meta_data(struct amdgpu_device *adev,struct drm_exec exec;long r; -drm_exec_init (&exec,0 ); +drm_exec_init (&exec,0 ,0 ); drm_exec_until_all_locked(&exec) { r =drm_exec_lock_obj (&exec, &ctx_data->meta_data_obj->tbo.base);drivers/gpu/drm/drm_exec.c +6
-1@@ -69,16 +69,25 @@ static void drm_exec_unlock_all(struct drm_exec *exec)* drm_exec_init - initialize a drm_exec object * @exec: the drm_exec object to initialize * @flags: controls locking behavior, see DRM_EXEC_* defines +* @nr: the initial # of objects * * Initialize the object and make sure that we can track locked objects. +* +* If nr is non-zero then it is used as the initial objects table size. +* In either case, the table will grow (be re-allocated) on demand. */ -void drm_exec_init (struct drm_exec *exec,uint32_t flags) +void drm_exec_init (struct drm_exec *exec,uint32_t flags,unsigned nr) { +size_t sz =PAGE_SIZE ;Quoted 8 lines
drivers/gpu/drm/drm_exec.c static void drm_exec_unlock_all(struct drm_exec *exec) · line 87
Quoted 7 lines
drivers/gpu/drm/drm_exec.c +5
-1+ +if (nr) + sz = (size_t )nr *sizeof (void *); + exec->flags = flags; - exec->objects =kmalloc (PAGE_SIZE ,GFP_KERNEL ); + exec->objects =kmalloc (sz,GFP_KERNEL );Please use k*v*malloc() here since we can't predict how large that will be.
Quoted 2 lines
or
__GFP_NOWARN? If userspace (or kasan) is cheeky and asks for ~0 objects, we should probably just fail?
Oh, good point! If this value is controlled by userspace we must be much more careful.
Instead of __GFP_NOWARN or any other workaround we should use kvmalloc_array() here.
Maybe turn the code upside down, in other words something like this here:
if (!nr)
nr = PAGE_SIZE / sizeof(void *);
exec->objects = kvmalloc_array(nr, sizeof(void *), GFP_KERNEL); exec->max_objects = exec->objects ? nr : 0;
Regards,
Christian.
Quoted 55 lines
BR,
-RQuoted 51 lines
With that fixed the patch is Reviewed-by: Christian König <[email protected]>.
Regards,
Christian.Quoted 45 lines
drivers/gpu/drm/drm_exec.c +1
-1/* If allocation here fails, just delay that till the first use */ - exec->max_objects = exec->objects ?PAGE_SIZE /sizeof (void *) :0 ; + exec->max_objects = exec->objects ? sz /sizeof (void *) :0 ; exec->num_objects =0 ; exec->contended =DRM_EXEC_DUMMY ; exec->prelocked =NULL ;drivers/gpu/drm/nouveau/nouveau_exec.c +1
-1@@ -103,7 +103,7 @@ nouveau_exec_job_submit(struct nouveau_job *job)nouveau_uvmm_lock (uvmm);drm_exec_init (exec,DRM_EXEC_INTERRUPTIBLE_WAIT | -DRM_EXEC_IGNORE_DUPLICATES ); +DRM_EXEC_IGNORE_DUPLICATES ,0 );drm_exec_until_all_locked (exec) {struct drm_gpuva *va;drivers/gpu/drm/nouveau/nouveau_uvmm.c +1
-1@@ -1288,7 +1288,7 @@ nouveau_uvmm_bind_job_submit(struct nouveau_job *job) }drm_exec_init (exec,DRM_EXEC_INTERRUPTIBLE_WAIT | -DRM_EXEC_IGNORE_DUPLICATES ); +DRM_EXEC_IGNORE_DUPLICATES ,0 );drm_exec_until_all_locked (exec) {list_for_each_op (op, &bind_job->ops) {struct drm_gpuva_op *va_op;include/drm/drm_exec.h +1
-1@@ -135,7 +135,7 @@ static inline bool drm_exec_is_contended(struct drm_exec *exec)return !!exec->contended; } -void drm_exec_init (struct drm_exec *exec,uint32_t flags); +void drm_exec_init (struct drm_exec *exec,uint32_t flags,unsigned nr);void drm_exec_fini (struct drm_exec *exec);bool drm_exec_cleanup (struct drm_exec *exec);int drm_exec_lock_obj (struct drm_exec *exec,struct drm_gem_object *obj);
Re: [PATCH 6/7] drm/exec: Pass in initial # of objects
Quoted 141 lines · Christian König
Am 30.10.23 um 14:38 schrieb Rob Clark:
Quoted 130 lines
Quoted 120 lines · Christian König
Am 27.10.23 um 18:58 schrieb Rob Clark:
Quoted 4 lines
From: Rob Clark <[email protected]>
In cases where the # is known ahead of time, it is silly to do the table resize dance.
Ah, yes that was my initial implementation as well, but I ditched that because nobody actually used it.
One comment below.
Quoted 110 lines
Signed-off-by Rob Clark <[email protected]>--- drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 2 +- drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c | 4 ++-- drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 4 ++-- drivers/gpu/drm/amd/amdgpu/amdgpu_mes.c | 4 ++-- drivers/gpu/drm/drm_exec.c | 15 ++++++++++++--- drivers/gpu/drm/nouveau/nouveau_exec.c | 2 +- drivers/gpu/drm/nouveau/nouveau_uvmm.c | 2 +- include/drm/drm_exec.h | 2 +- 8 files changed, 22 insertions(+), 13 deletions(-)drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c +1
-1@@ -65,7 +65,7 @@ static int amdgpu_cs_parser_init(struct amdgpu_cs_parser *p, }amdgpu_sync_create (&p->sync); -drm_exec_init (&p->exec,DRM_EXEC_INTERRUPTIBLE_WAIT ); +drm_exec_init (&p->exec,DRM_EXEC_INTERRUPTIBLE_WAIT ,0 );return 0 ; }drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c +2
-2@@ -70,7 +70,7 @@ int amdgpu_map_static_csa(struct amdgpu_device *adev, struct amdgpu_vm *vm,struct drm_exec exec;int r; -drm_exec_init (&exec,DRM_EXEC_INTERRUPTIBLE_WAIT ); +drm_exec_init (&exec,DRM_EXEC_INTERRUPTIBLE_WAIT ,0 ); drm_exec_until_all_locked(&exec) { r =amdgpu_vm_lock_pd (vm, &exec,0 );if (likely (!r))@@ -110,7 +110,7 @@ int amdgpu_unmap_static_csa(struct amdgpu_device *adev, struct amdgpu_vm *vm,struct drm_exec exec;int r; -drm_exec_init (&exec,DRM_EXEC_INTERRUPTIBLE_WAIT ); +drm_exec_init (&exec,DRM_EXEC_INTERRUPTIBLE_WAIT ,0 ); drm_exec_until_all_locked(&exec) { r =amdgpu_vm_lock_pd (vm, &exec,0 );if (likely (!r))drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c +2
-2@@ -203,7 +203,7 @@ static void amdgpu_gem_object_close(struct drm_gem_object *obj,struct drm_exec exec;long r; -drm_exec_init (&exec,DRM_EXEC_IGNORE_DUPLICATES ); +drm_exec_init (&exec,DRM_EXEC_IGNORE_DUPLICATES ,0 ); drm_exec_until_all_locked(&exec) { r =drm_exec_prepare_obj (&exec, &bo->tbo.base,1 );drm_exec_retry_on_contention (&exec);@@ -739,7 +739,7 @@ int amdgpu_gem_va_ioctl(struct drm_device *dev, void *data, }drm_exec_init (&exec,DRM_EXEC_INTERRUPTIBLE_WAIT | -DRM_EXEC_IGNORE_DUPLICATES ); +DRM_EXEC_IGNORE_DUPLICATES ,0 ); drm_exec_until_all_locked(&exec) {if (gobj) { r =drm_exec_lock_obj (&exec, gobj);drivers/gpu/drm/amd/amdgpu/amdgpu_mes.c +2
-2@@ -1105,7 +1105,7 @@ int amdgpu_mes_ctx_map_meta_data(struct amdgpu_device *adev,amdgpu_sync_create (&sync); -drm_exec_init (&exec,0 ); +drm_exec_init (&exec,0 ,0 ); drm_exec_until_all_locked(&exec) { r =drm_exec_lock_obj (&exec, &ctx_data->meta_data_obj->tbo.base);@@ -1176,7 +1176,7 @@ int amdgpu_mes_ctx_unmap_meta_data(struct amdgpu_device *adev,struct drm_exec exec;long r; -drm_exec_init (&exec,0 ); +drm_exec_init (&exec,0 ,0 ); drm_exec_until_all_locked(&exec) { r =drm_exec_lock_obj (&exec, &ctx_data->meta_data_obj->tbo.base);drivers/gpu/drm/drm_exec.c +6
-1@@ -69,16 +69,25 @@ static void drm_exec_unlock_all(struct drm_exec *exec)* drm_exec_init - initialize a drm_exec object * @exec: the drm_exec object to initialize * @flags: controls locking behavior, see DRM_EXEC_* defines +* @nr: the initial # of objects * * Initialize the object and make sure that we can track locked objects. +* +* If nr is non-zero then it is used as the initial objects table size. +* In either case, the table will grow (be re-allocated) on demand. */ -void drm_exec_init (struct drm_exec *exec,uint32_t flags) +void drm_exec_init (struct drm_exec *exec,uint32_t flags,unsigned nr) { +size_t sz =PAGE_SIZE ;Quoted 8 lines
drivers/gpu/drm/drm_exec.c static void drm_exec_unlock_all(struct drm_exec *exec) · line 87
Quoted 7 lines
drivers/gpu/drm/drm_exec.c +5
-1+ +if (nr) + sz = (size_t )nr *sizeof (void *); + exec->flags = flags; - exec->objects =kmalloc (PAGE_SIZE ,GFP_KERNEL ); + exec->objects =kmalloc (sz,GFP_KERNEL );Please use k*v*malloc() here since we can't predict how large that will be.
Quoted 2 lines
or
__GFP_NOWARN? If userspace (or kasan) is cheeky and asks for ~0 objects, we should probably just fail?Oh, good point! If this value is controlled by userspace we must be much more careful.
Instead of
__GFP_NOWARNor any other workaround we should usekvmalloc_array()here.
Quoted 7 lines
Maybe turn the code upside down, in other words something like this here:
if (!nr)
nr = PAGE_SIZE / sizeof(void *);
exec->objects=kvmalloc_array(nr, sizeof(void *), GFP_KERNEL);exec->max_objects=exec->objects? nr : 0;
oh, good point
BR,
-R
Quoted 60 lines
Regards,
Christian.Quoted 55 lines
BR,
-RQuoted 51 lines
With that fixed the patch is Reviewed-by: Christian König <[email protected]>.
Regards,
Christian.Quoted 45 lines
drivers/gpu/drm/drm_exec.c +1
-1/* If allocation here fails, just delay that till the first use */ - exec->max_objects = exec->objects ?PAGE_SIZE /sizeof (void *) :0 ; + exec->max_objects = exec->objects ? sz /sizeof (void *) :0 ; exec->num_objects =0 ; exec->contended =DRM_EXEC_DUMMY ; exec->prelocked =NULL ;drivers/gpu/drm/nouveau/nouveau_exec.c +1
-1@@ -103,7 +103,7 @@ nouveau_exec_job_submit(struct nouveau_job *job)nouveau_uvmm_lock (uvmm);drm_exec_init (exec,DRM_EXEC_INTERRUPTIBLE_WAIT | -DRM_EXEC_IGNORE_DUPLICATES ); +DRM_EXEC_IGNORE_DUPLICATES ,0 );drm_exec_until_all_locked (exec) {struct drm_gpuva *va;drivers/gpu/drm/nouveau/nouveau_uvmm.c +1
-1@@ -1288,7 +1288,7 @@ nouveau_uvmm_bind_job_submit(struct nouveau_job *job) }drm_exec_init (exec,DRM_EXEC_INTERRUPTIBLE_WAIT | -DRM_EXEC_IGNORE_DUPLICATES ); +DRM_EXEC_IGNORE_DUPLICATES ,0 );drm_exec_until_all_locked (exec) {list_for_each_op (op, &bind_job->ops) {struct drm_gpuva_op *va_op;include/drm/drm_exec.h +1
-1@@ -135,7 +135,7 @@ static inline bool drm_exec_is_contended(struct drm_exec *exec)return !!exec->contended; } -void drm_exec_init (struct drm_exec *exec,uint32_t flags); +void drm_exec_init (struct drm_exec *exec,uint32_t flags,unsigned nr);void drm_exec_fini (struct drm_exec *exec);bool drm_exec_cleanup (struct drm_exec *exec);int drm_exec_lock_obj (struct drm_exec *exec,struct drm_gem_object *obj);
PATCH 7/7 drm/msm/gem: Convert to drm_exec
In reply to Rob ClarkFrom: Rob Clark <[email protected]>
Replace the ww_mutex locking dance with the drm_exec helper.
Signed-off-by Rob Clark <[email protected]>
--- drivers/gpu/drm/msm/Kconfig | 1 + drivers/gpu/drm/msm/msm_gem.h | 5 +- drivers/gpu/drm/msm/msm_gem_submit.c | 117 +++++---------------------- 3 files changed, 24 insertions(+), 99 deletions(-)
drivers/gpu/drm/msm/Kconfig +1 -0
@@ -16,6 +16,7 @@ config DRM_MSM select DRM_DP_AUX_BUS select DRM_DISPLAY_DP_HELPER select DRM_DISPLAY_HELPER + select DRM_EXEC select DRM_KMS_HELPER select DRM_PANEL select DRM_BRIDGE
drivers/gpu/drm/msm/msm_gem.h +2 -3
@@ -9,6 +9,7 @@#include <linux/kref.h> #include <linux/dma-resv.h> +#include "drm/drm_exec.h" #include "drm/gpu_scheduler.h" #include "msm_drv.h"
@@ -254,7 +255,7 @@ struct msm_gem_submit {struct msm_gpu *gpu;struct msm_gem_address_space *aspace;struct list_head node;/* node in ring submit list */ -struct ww_acquire_ctx ticket; +struct drm_exec exec;uint32_t seqno;/* Sequence number of the submit on the ring */ /* Hw fence, which is created when the scheduler executes the job, and
@@ -287,8 +288,6 @@ struct msm_gem_submit {struct drm_msm_gem_submit_reloc *relocs; } *cmd;/* array of size nr_cmds */ struct { -/* make sure these don't conflict w/ MSM_SUBMIT_BO_x */ -#define BO_LOCKED 0x4000/* obj lock is held */ uint32_t flags;union {struct drm_gem_object *obj;
drivers/gpu/drm/msm/msm_gem_submit.c +21 -96
173 lines not on this page · raw message
Signature · 1 line
2.41.0