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.
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.
Regards,
Christian.
