Re: [PATCH 4/7] drm/radeon: Pin buffers while they are vmap'ed

[Thomas Zimmermann <tzimmermann@suse.de>]

Hi

Am 16.11.20 um 12:28 schrieb Christian König:
> Am 13.11.20 um 08:59 schrieb Thomas Zimmermann:
>> Hi Christian
>>
>> Am 12.11.20 um 18:16 schrieb Christian König:
>>> Am 12.11.20 um 14:21 schrieb Thomas Zimmermann:
>>>> In order to avoid eviction of vmap'ed buffers, pin them in their GEM
>>>> object's vmap implementation. Unpin them in the vunmap implementation.
>>>> This is needed to make generic fbdev support work reliably. Without,
>>>> the buffer object could be evicted while fbdev flushed its shadow
>>>> buffer.
>>>>
>>>> In difference to the PRIME pin/unpin functions, the vmap code does not
>>>> modify the BOs prime_shared_count, so a vmap-pinned BO does not
>>>> count as
>>>> shared.
>>>>
>>>> The actual pin location is not important as the vmap call returns
>>>> information on how to access the buffer. Callers that require a
>>>> specific location should explicitly pin the BO before vmapping it.
>>> Well is the buffer supposed to be scanned out?
>> No, not by the fbdev helper.
> 
> Ok in this case that should work.
> 
>>> If yes then the pin location is actually rather important since the
>>> hardware can only scan out from VRAM.
>> For relocatable BOs, fbdev uses a shadow buffer that makes all any
>> relocation transparent to userspace. It flushes the shadow fb into the
>> BO's memory if there are updates. The code is in
>> drm_fb_helper_dirty_work(). [1] During the flush operation, the vmap
>> call now pins the BO to wherever it is. The actual location does not
>> matter. It's vunmap'ed immediately afterwards.
> 
> The problem is what happens when it is prepared for scanout, but can't
> be moved to VRAM because it is vmapped?
> 
> When the shadow is never scanned out that isn't a problem, but we need
> to keep that in mind.

I sent out a patchset that addresses the issue in it's final patch. [1]
I'd appreciate your feedback. It also tested the patches with the
converted radeon driver.

Best regards
Thomas

[1] https://patchwork.freedesktop.org/series/83918/

> 
> Regards,
> Christian.
> 
>>
>> For dma-buf sharing, the regular procedure of pin + vmap still apply.
>> This should always move the BO into GTT-managed memory.
>>
>> Best regards
>> Thomas
>>
>> [1]
>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.kernel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Ftorvalds%2Flinux.git%2Ftree%2Fdrivers%2Fgpu%2Fdrm%2Fdrm_fb_helper.c%23n432&data=04%7C01%7Cchristian.koenig%40amd.com%7C31b890664ca7429fc45808d887aa0842%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637408511650629569%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=RLauuAuXkcl0rXwWWJ%2FrKP%2BsCr2wAzU1ejGV1bnQ80w%3D&reserved=0
>>
>>
>>> Regards,
>>> Christian.
>>>
>>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>>>> ---
>>>>    drivers/gpu/drm/radeon/radeon_gem.c | 51
>>>> +++++++++++++++++++++++++++--
>>>>    1 file changed, 49 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/radeon/radeon_gem.c
>>>> b/drivers/gpu/drm/radeon/radeon_gem.c
>>>> index d2876ce3bc9e..eaf7fc9a7b07 100644
>>>> --- a/drivers/gpu/drm/radeon/radeon_gem.c
>>>> +++ b/drivers/gpu/drm/radeon/radeon_gem.c
>>>> @@ -226,6 +226,53 @@ static int radeon_gem_handle_lockup(struct
>>>> radeon_device *rdev, int r)
>>>>        return r;
>>>>    }
>>>>    +static int radeon_gem_object_vmap(struct drm_gem_object *obj,
>>>> struct dma_buf_map *map)
>>>> +{
>>>> +    static const uint32_t any_domain = RADEON_GEM_DOMAIN_VRAM |
>>>> +                       RADEON_GEM_DOMAIN_GTT |
>>>> +                       RADEON_GEM_DOMAIN_CPU;
>>>> +
>>>> +    struct radeon_bo *bo = gem_to_radeon_bo(obj);
>>>> +    int ret;
>>>> +
>>>> +    ret = radeon_bo_reserve(bo, false);
>>>> +    if (ret)
>>>> +        return ret;
>>>> +
>>>> +    /* pin buffer at its current location */
>>>> +    ret = radeon_bo_pin(bo, any_domain, NULL);
>>>> +    if (ret)
>>>> +        goto err_radeon_bo_unreserve;
>>>> +
>>>> +    ret = drm_gem_ttm_vmap(obj, map);
>>>> +    if (ret)
>>>> +        goto err_radeon_bo_unpin;
>>>> +
>>>> +    radeon_bo_unreserve(bo);
>>>> +
>>>> +    return 0;
>>>> +
>>>> +err_radeon_bo_unpin:
>>>> +    radeon_bo_unpin(bo);
>>>> +err_radeon_bo_unreserve:
>>>> +    radeon_bo_unreserve(bo);
>>>> +    return ret;
>>>> +}
>>>> +
>>>> +static void radeon_gem_object_vunmap(struct drm_gem_object *obj,
>>>> struct dma_buf_map *map)
>>>> +{
>>>> +    struct radeon_bo *bo = gem_to_radeon_bo(obj);
>>>> +    int ret;
>>>> +
>>>> +    ret = radeon_bo_reserve(bo, false);
>>>> +    if (ret)
>>>> +        return;
>>>> +
>>>> +    drm_gem_ttm_vunmap(obj, map);
>>>> +    radeon_bo_unpin(bo);
>>>> +    radeon_bo_unreserve(bo);
>>>> +}
>>>> +
>>>>    static const struct drm_gem_object_funcs radeon_gem_object_funcs = {
>>>>        .free = radeon_gem_object_free,
>>>>        .open = radeon_gem_object_open,
>>>> @@ -234,8 +281,8 @@ static const struct drm_gem_object_funcs
>>>> radeon_gem_object_funcs = {
>>>>        .pin = radeon_gem_prime_pin,
>>>>        .unpin = radeon_gem_prime_unpin,
>>>>        .get_sg_table = radeon_gem_prime_get_sg_table,
>>>> -    .vmap = drm_gem_ttm_vmap,
>>>> -    .vunmap = drm_gem_ttm_vunmap,
>>>> +    .vmap = radeon_gem_object_vmap,
>>>> +    .vunmap = radeon_gem_object_vunmap,
>>>>    };
>>>>      /*
>>> _______________________________________________
>>> dri-devel mailing list
>>> dri-devel@lists.freedesktop.org
>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-devel&amp;data=04%7C01%7Cchristian.koenig%40amd.com%7C31b890664ca7429fc45808d887aa0842%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637408511650629569%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=h1U9Po83K7webxsiKpn3ZGFz9Fcg6SRkxtrXWZ1%2B%2FEc%3D&amp;reserved=0
>>>
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx