[PATCH] drm/amdgpu: Fix UVD contiguous CS mapping problem

[Christian König]

Am 14.11.24 um 16:38 schrieb Paneer Selvam, Arunpravin:
>
> Hi Christian,
>
> On 11/11/2024 3:33 PM, Christian König wrote:
>> Am 11.11.24 um 09:05 schrieb Arunpravin Paneer Selvam:
>>> When starting the mpv player, Radeon R9 users are observing
>>> the below error in dmesg.
>>>
>>> [drm:amdgpu_uvd_cs_pass2 [amdgpu]]
>>> *ERROR* msg/fb buffer ff00f7c000-ff00f7e000 out of 256MB segment!
>>>
>>> The patch tries to set the TTM_PL_FLAG_CONTIGUOUS for both user
>>> flag(AMDGPU_GEM_CREATE_VRAM_CONTIGUOUS) set and not set cases.
>>>
>>> Closes:https://gitlab.freedesktop.org/drm/amd/-/issues/3599
>>> Closes:https://gitlab.freedesktop.org/drm/amd/-/issues/3501
>>> Signed-off-by: Arunpravin Paneer Selvam 
>>> <Arunpravin.PaneerSelvam at amd.com>
>>> Cc: stable at vger.kernel.org
>>> ---
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 18 +++++++++++-------
>>>   1 file changed, 11 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c 
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>> index d891ab779ca7..9f73f821054b 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>> @@ -1801,13 +1801,17 @@ int amdgpu_cs_find_mapping(struct 
>>> amdgpu_cs_parser *parser,
>>>       if (dma_resv_locking_ctx((*bo)->tbo.base.resv) != 
>>> &parser->exec.ticket)
>>>           return -EINVAL;
>>>   -    (*bo)->flags |= AMDGPU_GEM_CREATE_VRAM_CONTIGUOUS;
>>> -    amdgpu_bo_placement_from_domain(*bo, (*bo)->allowed_domains);
>>> -    for (i = 0; i < (*bo)->placement.num_placement; i++)
>>> -        (*bo)->placements[i].flags |= TTM_PL_FLAG_CONTIGUOUS;
>>> -    r = ttm_bo_validate(&(*bo)->tbo, &(*bo)->placement, &ctx);
>>> -    if (r)
>>> -        return r;
>>> +    if ((*bo)->flags & AMDGPU_GEM_CREATE_VRAM_CONTIGUOUS) {
>>> +        (*bo)->placements[0].flags |= TTM_PL_FLAG_CONTIGUOUS;
>>
>> That is a pretty clearly broken approach. (*bo)->placements[0].flags 
>> is just used temporary between the call to 
>> amdgpu_bo_placement_from_domain() and ttm_bo_validate().
>>
>> So setting the TTM_PL_FLAG_CONTIGUOUS here is certainly not correct. 
>> Why is that necessary?
> gitlab users reported that the buffers are out of 256MB segment, looks 
> like buffers are not contiguous, after making the
> contiguous allocation mandatory using the TTM_PL_FLAG_CONTIGUOUS flag, 
> they are not seeing this issue.

In that case we have a bug somewhere that we don't properly initialize 
the flags before validating something.

I fear you need to investigate further since that here clearly doesn't 
fix the bug but just hides it.

Regards,
Christian.

>
> Thanks,
> Arun.
>>
>> Regards,
>> Christian.
>>
>>> +    } else {
>>> +        (*bo)->flags |= AMDGPU_GEM_CREATE_VRAM_CONTIGUOUS;
>>> +        amdgpu_bo_placement_from_domain(*bo, (*bo)->allowed_domains);
>>> +        for (i = 0; i < (*bo)->placement.num_placement; i++)
>>> +            (*bo)->placements[i].flags |= TTM_PL_FLAG_CONTIGUOUS;
>>> +        r = ttm_bo_validate(&(*bo)->tbo, &(*bo)->placement, &ctx);
>>> +        if (r)
>>> +            return r;
>>> +    }
>>>         return amdgpu_ttm_alloc_gart(&(*bo)->tbo);
>>>   }
>>
>