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?
Thanks & best regards,
Timur
