RE: [PATCH 5/6] drm/ttm: enable eviction for Per-VM-BO

[He, Roger]

-Original Message-
From: Christian König [mailto:ckoenig.leichtzumer...@gmail.com] 
Sent: Tuesday, December 12, 2017 6:40 PM
To: He, Roger <hongbo...@amd.com>; amd-...@lists.freedesktop.org; 
dri-devel@lists.freedesktop.org
Subject: Re: [PATCH 5/6] drm/ttm: enable eviction for Per-VM-BO

Am 12.12.2017 um 10:33 schrieb Roger He:
> Change-Id: I491d4ceb8c98bb3d8e6e0ddef2330284ce2fe5f6
> Signed-off-by: Roger He <hongbo...@amd.com>

I would squash this one with patch #6.

> ---
>   drivers/gpu/drm/ttm/ttm_bo.c | 7 +++
>   1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c 
> b/drivers/gpu/drm/ttm/ttm_bo.c index eb8c568..22b6ca5 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
> @@ -722,10 +722,9 @@ static int ttm_mem_evict_first(struct ttm_bo_device 
> *bdev,
>   spin_lock(>lru_lock);
>   for (i = 0; i < TTM_MAX_BO_PRIORITY; ++i) {
>   list_for_each_entry(bo, >lru[i], lru) {
> - if (bo->resv == resv) {
> - if (list_empty(>ddestroy))
> - continue;
> - } else {
> + if (!ctx ||
> + !(ctx->on_alloc_stage &&
> + bo->resv == ctx->resv)) {

Coding style: The lines stating with "!(ctx" and "bo->resv" are to far 
to the right.

Additional to that I think ctx is mandatory and doesn't need a check 
(but might be wrong). If it isn't it's probably time to make itmandatory.

And I would use (ctx->on_alloc_stage || list_empty(>ddestroy)) as 
check, we probably still want to be able to handledeleted BOs here 
during CS.

Change that as below, I think it can cover the case you described. No matter 
the Bo is in deleted list or not.
if (!ctx->allow_reserved_eviction || bo->resv != ctx->resv) {
locked = 
kcl_reservation_object_trylock(bo->resv);
if (!locked)
continue;
}

Thanks
Roger(Hongbo.He)



Christian.

>   locked = reservation_object_trylock(bo->resv);
>   if (!locked)
>   continue;

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 5/6] drm/ttm: enable eviction for Per-VM-BO

[Christian König]

Am 12.12.2017 um 10:33 schrieb Roger He:

Change-Id: I491d4ceb8c98bb3d8e6e0ddef2330284ce2fe5f6
Signed-off-by: Roger He 


I would squash this one with patch #6.


---
  drivers/gpu/drm/ttm/ttm_bo.c | 7 +++
  1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index eb8c568..22b6ca5 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -722,10 +722,9 @@ static int ttm_mem_evict_first(struct ttm_bo_device *bdev,
spin_lock(>lru_lock);
for (i = 0; i < TTM_MAX_BO_PRIORITY; ++i) {
list_for_each_entry(bo, >lru[i], lru) {
-   if (bo->resv == resv) {
-   if (list_empty(>ddestroy))
-   continue;
-   } else {
+   if (!ctx ||
+   !(ctx->on_alloc_stage &&
+   bo->resv == ctx->resv)) {


Coding style: The lines stating with "!(ctx" and "bo->resv" are to far 
to the right.


Additional to that I think ctx is mandatory and doesn't need a check 
(but might be wrong). If it isn't it's probably time to make it mandatory.


And I would use (ctx->on_alloc_stage || list_empty(>ddestroy)) as 
check, we probably still want to be able to handle deleted BOs here 
during CS.


Christian.


locked = reservation_object_trylock(bo->resv);
if (!locked)
continue;


___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel