Re: [PATCH 2/3] drm/ttm: remove swap LRU v2

["Christian König" <ckoenig.leichtzumerken@gmail.com>]

Am 15.03.21 um 19:54 schrieb Matthew Auld:
> On Mon, 15 Mar 2021 at 16:04, Christian König
> <ckoenig.leichtzumerken@gmail.com> wrote:
>> [SNIP]
>> @@ -1193,6 +1164,10 @@ int ttm_bo_swapout(struct ttm_buffer_object *bo, struct ttm_operation_ctx *ctx,
>>          bool locked;
>>          int ret;
>>
>> +       if (!bo->ttm || bo->ttm->page_flags & (TTM_PAGE_FLAG_SG |
>> +                                              TTM_PAGE_FLAG_SWAPPED))
>> +               return false;
>> +
> return 0; ?
>
> Seems inconsistent to return zero here and not drop the lru lock? Or
> maybe turn this into a programmer error, since the current caller
> already checks for the above?

Thanks, that is just an artifact from rebasing and should be removed.

>> [SNIP]
>>
>> @@ -109,27 +106,60 @@ static int ttm_global_init(void)
>>   long ttm_global_swapout(struct ttm_operation_ctx *ctx, gfp_t gfp_flags)
>>   {
>>          struct ttm_global *glob = &ttm_glob;
>> +       struct ttm_device *bdev;
>> +       int ret = -EBUSY;
>> +
>> +       mutex_lock(&ttm_global_mutex);
>> +       list_for_each_entry(bdev, &glob->device_list, device_list) {
>> +               ret = ttm_device_swapout(bdev, ctx, gfp_flags);
> Mixing int and long for num_pages.
>
> Does ttm enforce a maximum page count somewhere for object sizes?

We should use 32 bit values for the number of pages in TTM, even signed 
values allow for 8TB large BOs.

And I really hope that we can get rid of the BO approach in general 
before we ever come close to that limit.

> Something like INT_MAX, since it doesn't look like ttm is consistently
> using the same type(unsigned long?) when representing the number of
> pages for an object?

I should probably add a check for that in the tt code, yes.

> [SNIP]
>   static void ttm_init_sysman(struct ttm_device *bdev)
>   {
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> index b991422e156c..0e82b0662d9e 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> @@ -1371,7 +1371,7 @@ static int vmw_pm_freeze(struct device *kdev)
>          vmw_execbuf_release_pinned_bo(dev_priv);
>          vmw_resource_evict_all(dev_priv);
>          vmw_release_device_early(dev_priv);
> -       while (ttm_global_swapout(&ctx, GFP_KERNEL) > 0);
> +       while (ttm_device_swapout(&dev_priv->bdev, &ctx, GFP_KERNEL) == 0);
> Is this the intended behaviour? ttm_device_swapout() still just
> returns num_pages if it swapped something out. I assume this wants to
> keep swapping stuff out, until it can't anymore. Or am I missing
> something?

Indeed that's a mix up. Thanks for pointing that out.

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