Re: [Intel-gfx] [PATCH 1/2] drm/ttm: prevent moving of pinned BOs

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

Am 24.01.23 um 11:12 schrieb Matthew Auld:
> On Tue, 24 Jan 2023 at 09:51, Christian König
> <ckoenig.leichtzumerken@gmail.com> wrote:
>> Am 11.01.23 um 14:17 schrieb Matthew Auld:
>>> On Wed, 11 Jan 2023 at 11:43, Christian König
>>> <ckoenig.leichtzumerken@gmail.com> wrote:
>>>> We have checks for this in the individual drivers move callback, but
>>>> it's probably better to generally forbit that on a higher level.
>>>>
>>>> Also stops exporting ttm_resource_compat() since that's not necessary
>>>> any more after removing the extra checks in vmwgfx.
>>>>
>>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>>>> ---
>>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c |  4 ----
>>>>    drivers/gpu/drm/nouveau/nouveau_bo.c    |  3 ---
>>>>    drivers/gpu/drm/radeon/radeon_ttm.c     |  4 ----
>>>>    drivers/gpu/drm/ttm/ttm_bo.c            | 20 ++++++++++++--------
>>>>    drivers/gpu/drm/ttm/ttm_resource.c      |  1 -
>>>>    drivers/gpu/drm/vmwgfx/vmwgfx_bo.c      | 19 ++-----------------
>>>>    6 files changed, 14 insertions(+), 37 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>>> index 068c2d8495fd..677cd7d91687 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>>> @@ -466,11 +466,7 @@ static int amdgpu_bo_move(struct ttm_buffer_object *bo, bool evict,
>>>>                           return r;
>>>>           }
>>>>
>>>> -       /* Can't move a pinned BO */
>>>>           abo = ttm_to_amdgpu_bo(bo);
>>>> -       if (WARN_ON_ONCE(abo->tbo.pin_count > 0))
>>>> -               return -EINVAL;
>>>> -
>>>>           adev = amdgpu_ttm_adev(bo->bdev);
>>>>
>>>>           if (!old_mem || (old_mem->mem_type == TTM_PL_SYSTEM &&
>>>> diff --git a/drivers/gpu/drm/nouveau/nouveau_bo.c b/drivers/gpu/drm/nouveau/nouveau_bo.c
>>>> index 288eebc70a67..c2ec91cc845d 100644
>>>> --- a/drivers/gpu/drm/nouveau/nouveau_bo.c
>>>> +++ b/drivers/gpu/drm/nouveau/nouveau_bo.c
>>>> @@ -1015,9 +1015,6 @@ nouveau_bo_move(struct ttm_buffer_object *bo, bool evict,
>>>>           if (ret)
>>>>                   goto out_ntfy;
>>>>
>>>> -       if (nvbo->bo.pin_count)
>>>> -               NV_WARN(drm, "Moving pinned object %p!\n", nvbo);
>>>> -
>>>>           if (drm->client.device.info.family < NV_DEVICE_INFO_V0_TESLA) {
>>>>                   ret = nouveau_bo_vm_bind(bo, new_reg, &new_tile);
>>>>                   if (ret)
>>>> diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c b/drivers/gpu/drm/radeon/radeon_ttm.c
>>>> index 1e8e287e113c..67075c85f847 100644
>>>> --- a/drivers/gpu/drm/radeon/radeon_ttm.c
>>>> +++ b/drivers/gpu/drm/radeon/radeon_ttm.c
>>>> @@ -211,11 +211,7 @@ static int radeon_bo_move(struct ttm_buffer_object *bo, bool evict,
>>>>           if (r)
>>>>                   return r;
>>>>
>>>> -       /* Can't move a pinned BO */
>>>>           rbo = container_of(bo, struct radeon_bo, tbo);
>>>> -       if (WARN_ON_ONCE(rbo->tbo.pin_count > 0))
>>>> -               return -EINVAL;
>>>> -
>>>>           rdev = radeon_get_rdev(bo->bdev);
>>>>           if (old_mem->mem_type == TTM_PL_SYSTEM && bo->ttm == NULL) {
>>>>                   ttm_bo_move_null(bo, new_mem);
>>>> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
>>>> index 326a3d13a829..9baccb2f6e99 100644
>>>> --- a/drivers/gpu/drm/ttm/ttm_bo.c
>>>> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
>>>> @@ -894,14 +894,18 @@ int ttm_bo_validate(struct ttm_buffer_object *bo,
>>>>           if (!placement->num_placement && !placement->num_busy_placement)
>>>>                   return ttm_bo_pipeline_gutting(bo);
>>>>
>>>> -       /*
>>>> -        * Check whether we need to move buffer.
>>>> -        */
>>>> -       if (!bo->resource || !ttm_resource_compat(bo->resource, placement)) {
>>>> -               ret = ttm_bo_move_buffer(bo, placement, ctx);
>>>> -               if (ret)
>>>> -                       return ret;
>>>> -       }
>>>> +       /* Check whether we need to move buffer. */
>>>> +       if (bo->resource && ttm_resource_compat(bo->resource, placement))
>>>> +               return 0;
>>> Note this now skips the tt create below (intentional?). I think i915
>>> needed that, since it creates a dummy system resource initially for
>>> all objects, and then relies on ZERO_ALLOC being set for certain
>>> objects to know if the memory needs to be cleared or not when later
>>> doing the dummy -> vram. Thoughts?
>> That's unproblematic. On initial allocation bo->resource is NULL so we
>> never branch out here.
> Here is what I see in drm-tip, when first creating an object with ttm:
>
> ttm_bo_init_reserved() -> ttm_resource_alloc(PL_SYSTEM, &bo->resource)
> -> ttm_bo_validate()
>
> So bo->resource is for sure not NULL on initial allocation, and is
> pointing to PL_SYSTEM. And in i915 we initially stuff everything into
> SYSTEM as a dummy placement.
>
> IIRC you had a series that tried to address that (allowing NULL
> resource or so), but it never landed:
> https://patchwork.freedesktop.org/patch/500698/?series=107680&rev=2

Oh! My recollection was that this was done!

Sorry my memory failed me here, thanks for the notice.

Christian.

>
>> Christian.
>>
>>>> +
>>>> +       /* Moving of pinned BOs is forbidden */
>>>> +       if (bo->pin_count)
>>>> +               return -EINVAL;
>>>> +
>>>> +       ret = ttm_bo_move_buffer(bo, placement, ctx);
>>>> +       if (ret)
>>>> +               return ret;
>>>> +
>>>>           /*
>>>>            * We might need to add a TTM.
>>>>            */
>>>> diff --git a/drivers/gpu/drm/ttm/ttm_resource.c b/drivers/gpu/drm/ttm/ttm_resource.c
>>>> index b8a826a24fb2..7333f7a87a2f 100644
>>>> --- a/drivers/gpu/drm/ttm/ttm_resource.c
>>>> +++ b/drivers/gpu/drm/ttm/ttm_resource.c
>>>> @@ -361,7 +361,6 @@ bool ttm_resource_compat(struct ttm_resource *res,
>>>>
>>>>           return false;
>>>>    }
>>>> -EXPORT_SYMBOL(ttm_resource_compat);
>>>>
>>>>    void ttm_resource_set_bo(struct ttm_resource *res,
>>>>                            struct ttm_buffer_object *bo)
>>>> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c b/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
>>>> index 321c551784a1..dbcef460c452 100644
>>>> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
>>>> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
>>>> @@ -87,12 +87,7 @@ int vmw_bo_pin_in_placement(struct vmw_private *dev_priv,
>>>>           if (unlikely(ret != 0))
>>>>                   goto err;
>>>>
>>>> -       if (buf->base.pin_count > 0)
>>>> -               ret = ttm_resource_compat(bo->resource, placement)
>>>> -                       ? 0 : -EINVAL;
>>>> -       else
>>>> -               ret = ttm_bo_validate(bo, placement, &ctx);
>>>> -
>>>> +       ret = ttm_bo_validate(bo, placement, &ctx);
>>>>           if (!ret)
>>>>                   vmw_bo_pin_reserved(buf, true);
>>>>
>>>> @@ -128,12 +123,6 @@ int vmw_bo_pin_in_vram_or_gmr(struct vmw_private *dev_priv,
>>>>           if (unlikely(ret != 0))
>>>>                   goto err;
>>>>
>>>> -       if (buf->base.pin_count > 0) {
>>>> -               ret = ttm_resource_compat(bo->resource, &vmw_vram_gmr_placement)
>>>> -                       ? 0 : -EINVAL;
>>>> -               goto out_unreserve;
>>>> -       }
>>>> -
>>>>           ret = ttm_bo_validate(bo, &vmw_vram_gmr_placement, &ctx);
>>>>           if (likely(ret == 0) || ret == -ERESTARTSYS)
>>>>                   goto out_unreserve;
>>>> @@ -218,11 +207,7 @@ int vmw_bo_pin_in_start_of_vram(struct vmw_private *dev_priv,
>>>>                   (void) ttm_bo_validate(bo, &vmw_sys_placement, &ctx);
>>>>           }
>>>>
>>>> -       if (buf->base.pin_count > 0)
>>>> -               ret = ttm_resource_compat(bo->resource, &placement)
>>>> -                       ? 0 : -EINVAL;
>>>> -       else
>>>> -               ret = ttm_bo_validate(bo, &placement, &ctx);
>>>> +       ret = ttm_bo_validate(bo, &placement, &ctx);
>>>>
>>>>           /* For some reason we didn't end up at the start of vram */
>>>>           WARN_ON(ret == 0 && bo->resource->start != 0);
>>>> --
>>>> 2.34.1
>>>>