On 10/30/25 14:47, Timur Kristóf wrote:
> On Thu, 2025-10-30 at 12:12 +0100, Christian König wrote:
>> On 10/29/25 23:48, Timur Kristóf wrote:
>>>>> + ASSERT(adev->vce.vcpu_bo);
>>>>
>>>> Please drop that.
>>>
>>> Sure, but can you say why?
>>
>> ASSERT either uses BUG_ON() or WARN_ON().
>>
>> BUG_ON() will crash the kernel immediately and WARN_ON will warn,
>> continue and then crash.
>>
>> The justification for a BUG_ON() is to prevent further data
>> corruption and that is not the case here.
>
> Thanks for explaining that. Technically the vcpu_bo should never be
> NULL, so I think I'll just go with your original suggestion and remove
> the assertion.
>
>>
>> What you can do is to use something like "if (WARN_ON(...)) return -
>> EINVAL;".
>>
>>>>
>>>>> +
>>>>> + r = amdgpu_bo_reserve(adev->vce.vcpu_bo, false);
>>>>> + if (r) {
>>>>> + dev_err(adev->dev, "%s (%d) failed to reserve
>>>>> VCE
>>>>> bo\n", __func__, r);
>>>>> + return r;
>>>>> + }
>>>>> +
>>>>> + r = amdgpu_bo_kmap(adev->vce.vcpu_bo, (void
>>>>> **)&cpu_addr);
>>>>> + if (r) {
>>>>> + amdgpu_bo_unreserve(adev->vce.vcpu_bo);
>>>>> + dev_err(adev->dev, "%s (%d) VCE map failed\n",
>>>>> __func__, r);
>>>>> + return r;
>>>>> + }
>>>>
>>>> That part is actually pretty pointless the cpu addr is already
>>>> available as adev->vce.cpu_addr.
>>>
>>> I don't think so. amdgpu_vce_resume actually unmaps and unreserves
>>> the
>>> VCE BO, so I think we need to map and reserve it again if we want
>>> to
>>> access it again. Am I misunderstanding something?
>>
>> Yeah, I see. But that is a totally pointless leftover from radeon as
>> well which we should probably be removed.
>>
>> The VCE BO needs to stay at the same location before and after resume
>> since the FW code is not relocateable once started.
>>
>> So we need to keep it pinned all the time and so can keep it CPU
>> mapped all the time as well.
>
> Right, that makes a lot of sense. I can do it, but I'd like to be
> careful about it because it sounds like this would affect all VCE
> versions and not just VCE1.
>
> Do you prefer that I add a patch to this series to deal with that, or
> would it be better to do that after this series lands?
Add a patch early into the series to clean that up.
It should be a pretty straight forward change and I can throw it into our CI
system which should have plenty of HW still using VCE (Polaris/Vega/MI*).
Thanks,
Christian.
>
> Thanks & best regards,
> Timur
>
