Re: [PATCH 10/14] drm/amdgpu/vce1: Implement VCE1 IP block

["Timur Kristóf" <timur.kristof@gmail.com>]

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