| Message ID | 20240703132602.4756-3-christian.koenig@amd.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/5] dma-buf/dma-resv: Introduce dma_resv_trylock_ctx() | expand |
On Wed, 2024-07-03 at 15:25 +0200, Christian König wrote: > Some contended objects might never be locked again in the case of > eviction > handling for example. > > Make sure that those doesn't show up in the list of locked objects > until > they are explicitely mentioned. Could you be a bit more specific in the commit message about in what situations that is bad? /Thomas > > Signed-off-by: Christian König <christian.koenig@amd.com> > --- > drivers/gpu/drm/drm_exec.c | 18 +++++++++--------- > 1 file changed, 9 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpu/drm/drm_exec.c b/drivers/gpu/drm/drm_exec.c > index 2da094bdf8a4..220df336fbd9 100644 > --- a/drivers/gpu/drm/drm_exec.c > +++ b/drivers/gpu/drm/drm_exec.c > @@ -61,8 +61,11 @@ static void drm_exec_unlock_all(struct drm_exec > *exec) > drm_gem_object_put(obj); > } > > - drm_gem_object_put(exec->prelocked); > - exec->prelocked = NULL; > + if (exec->prelocked) { > + dma_resv_unlock(exec->prelocked->resv); > + drm_gem_object_put(exec->prelocked); > + exec->prelocked = NULL; > + } > } > > /** > @@ -179,16 +182,9 @@ static int drm_exec_lock_contended(struct > drm_exec *exec) > dma_resv_lock_slow(obj->resv, &exec->ticket); > } > > - ret = drm_exec_obj_locked(exec, obj); > - if (unlikely(ret)) > - goto error_unlock; > - > exec->prelocked = obj; > return 0; > > -error_unlock: > - dma_resv_unlock(obj->resv); > - > error_dropref: > drm_gem_object_put(obj); > return ret; > @@ -214,6 +210,10 @@ int drm_exec_lock_obj(struct drm_exec *exec, > struct drm_gem_object *obj) > return ret; > > if (exec->prelocked == obj) { > + ret = drm_exec_obj_locked(exec, obj); > + if (unlikely(ret)) > + return ret; > + > drm_gem_object_put(exec->prelocked); > exec->prelocked = NULL; > return 0;
Am 03.07.24 um 17:51 schrieb Thomas Hellström: > On Wed, 2024-07-03 at 15:25 +0200, Christian König wrote: >> Some contended objects might never be locked again in the case of >> eviction >> handling for example. >> >> Make sure that those doesn't show up in the list of locked objects >> until >> they are explicitely mentioned. > Could you be a bit more specific in the commit message about in what > situations that is bad? The prelocked object is not necessarily expected to be in the list of locked objects. I ran into issues because amdgpu tried to validate all locked objects and so tried to also validate the prelocked one (which was only locked for eviction). That obviously didn't made much sense. Regards, Christian. > > /Thomas > > > >> Signed-off-by: Christian König <christian.koenig@amd.com> >> --- >> drivers/gpu/drm/drm_exec.c | 18 +++++++++--------- >> 1 file changed, 9 insertions(+), 9 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_exec.c b/drivers/gpu/drm/drm_exec.c >> index 2da094bdf8a4..220df336fbd9 100644 >> --- a/drivers/gpu/drm/drm_exec.c >> +++ b/drivers/gpu/drm/drm_exec.c >> @@ -61,8 +61,11 @@ static void drm_exec_unlock_all(struct drm_exec >> *exec) >> drm_gem_object_put(obj); >> } >> >> - drm_gem_object_put(exec->prelocked); >> - exec->prelocked = NULL; >> + if (exec->prelocked) { >> + dma_resv_unlock(exec->prelocked->resv); >> + drm_gem_object_put(exec->prelocked); >> + exec->prelocked = NULL; >> + } >> } >> >> /** >> @@ -179,16 +182,9 @@ static int drm_exec_lock_contended(struct >> drm_exec *exec) >> dma_resv_lock_slow(obj->resv, &exec->ticket); >> } >> >> - ret = drm_exec_obj_locked(exec, obj); >> - if (unlikely(ret)) >> - goto error_unlock; >> - >> exec->prelocked = obj; >> return 0; >> >> -error_unlock: >> - dma_resv_unlock(obj->resv); >> - >> error_dropref: >> drm_gem_object_put(obj); >> return ret; >> @@ -214,6 +210,10 @@ int drm_exec_lock_obj(struct drm_exec *exec, >> struct drm_gem_object *obj) >> return ret; >> >> if (exec->prelocked == obj) { >> + ret = drm_exec_obj_locked(exec, obj); >> + if (unlikely(ret)) >> + return ret; >> + >> drm_gem_object_put(exec->prelocked); >> exec->prelocked = NULL; >> return 0;
On Fri, 2024-07-05 at 14:41 +0200, Christian König wrote: > Am 03.07.24 um 17:51 schrieb Thomas Hellström: > > On Wed, 2024-07-03 at 15:25 +0200, Christian König wrote: > > > Some contended objects might never be locked again in the case of > > > eviction > > > handling for example. > > > > > > Make sure that those doesn't show up in the list of locked > > > objects > > > until > > > they are explicitely mentioned. > > Could you be a bit more specific in the commit message about in > > what > > situations that is bad? > > The prelocked object is not necessarily expected to be in the list of > locked objects. > > I ran into issues because amdgpu tried to validate all locked objects > and so tried to also validate the prelocked one (which was only > locked > for eviction). > > That obviously didn't made much sense. Indeed. Could you add a similar description to the commit message? /Thomas > > Regards, > Christian. > > > > > /Thomas > > > > > > > > > Signed-off-by: Christian König <christian.koenig@amd.com> > > > --- > > > drivers/gpu/drm/drm_exec.c | 18 +++++++++--------- > > > 1 file changed, 9 insertions(+), 9 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/drm_exec.c > > > b/drivers/gpu/drm/drm_exec.c > > > index 2da094bdf8a4..220df336fbd9 100644 > > > --- a/drivers/gpu/drm/drm_exec.c > > > +++ b/drivers/gpu/drm/drm_exec.c > > > @@ -61,8 +61,11 @@ static void drm_exec_unlock_all(struct > > > drm_exec > > > *exec) > > > drm_gem_object_put(obj); > > > } > > > > > > - drm_gem_object_put(exec->prelocked); > > > - exec->prelocked = NULL; > > > + if (exec->prelocked) { > > > + dma_resv_unlock(exec->prelocked->resv); > > > + drm_gem_object_put(exec->prelocked); > > > + exec->prelocked = NULL; > > > + } > > > } > > > > > > /** > > > @@ -179,16 +182,9 @@ static int drm_exec_lock_contended(struct > > > drm_exec *exec) > > > dma_resv_lock_slow(obj->resv, &exec->ticket); > > > } > > > > > > - ret = drm_exec_obj_locked(exec, obj); > > > - if (unlikely(ret)) > > > - goto error_unlock; > > > - > > > exec->prelocked = obj; > > > return 0; > > > > > > -error_unlock: > > > - dma_resv_unlock(obj->resv); > > > - > > > error_dropref: > > > drm_gem_object_put(obj); > > > return ret; > > > @@ -214,6 +210,10 @@ int drm_exec_lock_obj(struct drm_exec *exec, > > > struct drm_gem_object *obj) > > > return ret; > > > > > > if (exec->prelocked == obj) { > > > + ret = drm_exec_obj_locked(exec, obj); > > > + if (unlikely(ret)) > > > + return ret; > > > + > > > drm_gem_object_put(exec->prelocked); > > > exec->prelocked = NULL; > > > return 0; >
diff --git a/drivers/gpu/drm/drm_exec.c b/drivers/gpu/drm/drm_exec.c index 2da094bdf8a4..220df336fbd9 100644 --- a/drivers/gpu/drm/drm_exec.c +++ b/drivers/gpu/drm/drm_exec.c @@ -61,8 +61,11 @@ static void drm_exec_unlock_all(struct drm_exec *exec) drm_gem_object_put(obj); } - drm_gem_object_put(exec->prelocked); - exec->prelocked = NULL; + if (exec->prelocked) { + dma_resv_unlock(exec->prelocked->resv); + drm_gem_object_put(exec->prelocked); + exec->prelocked = NULL; + } } /** @@ -179,16 +182,9 @@ static int drm_exec_lock_contended(struct drm_exec *exec) dma_resv_lock_slow(obj->resv, &exec->ticket); } - ret = drm_exec_obj_locked(exec, obj); - if (unlikely(ret)) - goto error_unlock; - exec->prelocked = obj; return 0; -error_unlock: - dma_resv_unlock(obj->resv); - error_dropref: drm_gem_object_put(obj); return ret; @@ -214,6 +210,10 @@ int drm_exec_lock_obj(struct drm_exec *exec, struct drm_gem_object *obj) return ret; if (exec->prelocked == obj) { + ret = drm_exec_obj_locked(exec, obj); + if (unlikely(ret)) + return ret; + drm_gem_object_put(exec->prelocked); exec->prelocked = NULL; return 0;
Some contended objects might never be locked again in the case of eviction handling for example. Make sure that those doesn't show up in the list of locked objects until they are explicitely mentioned. Signed-off-by: Christian König <christian.koenig@amd.com> --- drivers/gpu/drm/drm_exec.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-)