| Message ID | 1352728811-21860-3-git-send-email-maarten.lankhorst@canonical.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 11/12/2012 03:00 PM, Maarten Lankhorst wrote: > Just use the return error from ttm_mem_evict_first instead. Here driver need to be able to evict a memory type completely, because they might shut down the memory type or clear it for some legacy usage, suspending or whatever, so returning 0 on -EBUSY isn't sufficient, we need at least a list empty check, and a shared reservation at this point is illegal. This is a point where the mechanism to exclude other reservers is needed, as we discussed previously. vmwgfx is using the ttm lock, but will of course adapt if a new mechanism is emerging. Thanks, /Thomas > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@canonical.com> > --- > drivers/gpu/drm/ttm/ttm_bo.c | 27 ++++++++------------------- > 1 file changed, 8 insertions(+), 19 deletions(-) > > diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c > index 74d6e7c..a3383a7 100644 > --- a/drivers/gpu/drm/ttm/ttm_bo.c > +++ b/drivers/gpu/drm/ttm/ttm_bo.c > @@ -1302,29 +1302,18 @@ EXPORT_SYMBOL(ttm_bo_create); > static int ttm_bo_force_list_clean(struct ttm_bo_device *bdev, > unsigned mem_type, bool allow_errors) > { > - struct ttm_mem_type_manager *man = &bdev->man[mem_type]; > - struct ttm_bo_global *glob = bdev->glob; > - int ret; > - > - /* > - * Can't use standard list traversal since we're unlocking. > - */ > + int ret = 0; > > - spin_lock(&glob->lru_lock); > - while (!list_empty(&man->lru)) { > - spin_unlock(&glob->lru_lock); > + while (!ret) { > ret = ttm_mem_evict_first(bdev, mem_type, false, false, false); > - if (ret) { > - if (allow_errors) { > - return ret; > - } else { > - pr_err("Cleanup eviction failed\n"); > - } > + if (ret == -EBUSY) > + return 0; > + else if (ret && !allow_errors) { > + pr_err("Cleanup eviction failed\n"); > + ret = 0; > } > - spin_lock(&glob->lru_lock); > } > - spin_unlock(&glob->lru_lock); > - return 0; > + return ret; > } > > int ttm_bo_clean_mm(struct ttm_bo_device *bdev, unsigned mem_type)
Op 19-11-12 14:33, Thomas Hellstrom schreef: > On 11/12/2012 03:00 PM, Maarten Lankhorst wrote: >> Just use the return error from ttm_mem_evict_first instead. > > Here driver need to be able to evict a memory type completely, because they might shut down > the memory type or clear it for some legacy usage, suspending or whatever, so returning 0 on -EBUSY isn't sufficient, > we need at least a list empty check, and a shared reservation at this point is illegal. > > This is a point where the mechanism to exclude other reservers is needed, as we discussed previously. > vmwgfx is using the ttm lock, but will of course adapt if a new mechanism is emerging. Normally ttm_mem_evict_first only returns -EBUSY if the list is empty and no_wait = false, so I thought using the return code would be equivalent. We could do spin_lock(&glob->lru_lock); WARN_ON(!list_empty(&man->lru_lock)); spin_unlock(&glob->lru_lock); to handle this after -EBUSY. With a lot of objects on the lru list, this would save taking lru_lock twice for each object. ~Maarten
On 11/19/2012 03:10 PM, Maarten Lankhorst wrote: > Op 19-11-12 14:33, Thomas Hellstrom schreef: >> On 11/12/2012 03:00 PM, Maarten Lankhorst wrote: >>> Just use the return error from ttm_mem_evict_first instead. >> Here driver need to be able to evict a memory type completely, because they might shut down >> the memory type or clear it for some legacy usage, suspending or whatever, so returning 0 on -EBUSY isn't sufficient, >> we need at least a list empty check, and a shared reservation at this point is illegal. >> >> This is a point where the mechanism to exclude other reservers is needed, as we discussed previously. >> vmwgfx is using the ttm lock, but will of course adapt if a new mechanism is emerging. > Normally ttm_mem_evict_first only returns -EBUSY if the list is empty and no_wait = false, > so I thought using the return code would be equivalent. > > We could do spin_lock(&glob->lru_lock); WARN_ON(!list_empty(&man->lru_lock)); spin_unlock(&glob->lru_lock); to handle this after -EBUSY. > > With a lot of objects on the lru list, this would save taking lru_lock twice for each object. > > ~Maarten Sure, and in the allow_errors case we should return an error if the list isn't empty, to allow careful drivers to deal with that. /Thomas > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c index 74d6e7c..a3383a7 100644 --- a/drivers/gpu/drm/ttm/ttm_bo.c +++ b/drivers/gpu/drm/ttm/ttm_bo.c @@ -1302,29 +1302,18 @@ EXPORT_SYMBOL(ttm_bo_create); static int ttm_bo_force_list_clean(struct ttm_bo_device *bdev, unsigned mem_type, bool allow_errors) { - struct ttm_mem_type_manager *man = &bdev->man[mem_type]; - struct ttm_bo_global *glob = bdev->glob; - int ret; - - /* - * Can't use standard list traversal since we're unlocking. - */ + int ret = 0; - spin_lock(&glob->lru_lock); - while (!list_empty(&man->lru)) { - spin_unlock(&glob->lru_lock); + while (!ret) { ret = ttm_mem_evict_first(bdev, mem_type, false, false, false); - if (ret) { - if (allow_errors) { - return ret; - } else { - pr_err("Cleanup eviction failed\n"); - } + if (ret == -EBUSY) + return 0; + else if (ret && !allow_errors) { + pr_err("Cleanup eviction failed\n"); + ret = 0; } - spin_lock(&glob->lru_lock); } - spin_unlock(&glob->lru_lock); - return 0; + return ret; } int ttm_bo_clean_mm(struct ttm_bo_device *bdev, unsigned mem_type)
Just use the return error from ttm_mem_evict_first instead. Signed-off-by: Maarten Lankhorst <maarten.lankhorst@canonical.com> --- drivers/gpu/drm/ttm/ttm_bo.c | 27 ++++++++------------------- 1 file changed, 8 insertions(+), 19 deletions(-)