[RFC PATCH 2/2] drm/amdgpu/uvd: Ensure vcpu bos are within the uvd segment

[Christian König]

On 6/3/25 16:34, John Olender wrote:
>>> Oh, that's a very interesting find. Could you try to turn around the way the patch works?
>>>
>>> E.g. instead of forcing the UVD FW into the first segment, change amdgpu_uvd_force_into_uvd_segment() so that the BOs are forced into the same segment as the UVD firmware.
>>>
> 
> I started implementing this and I realized two main problems with this
> approach.
> 
> First, there's currently no guarantee the UVD FW does not cross a 256MB
> boundary.

There actually is a guarantee for that.

FW BOs are always allocated contiguously, and the allocation backend makes sure that for example an 16MiB (rounded up) FW is always aligned to a 16MiB boundary.

So FW memory never crosses a segment boundary larger than it's rounded up size. Without that tons of things in our driver would fall apart, not just UVD :)

  Checking for this and providing a fallback is going to make
> this patch... not really any less complex than the original.
> 
> Second, most of time this is just going to end up selecting the first
> segment anyway.  I'll go more into this below.

That's fine with me. I'm just concerned about that 1% of people where it potentially worked before and we are now breaking it.

> 
>>> That would resolve my concern that this could overload the first segment. The feedback and message BO are usually rather small (4 or 128k IIRC), but the firmware is a couple of megabytes in size.
>>>
>>> When we have other FW and VGA emulation buffers in the first segment as well then that could result into clashing that segment to much.
>>>
> 
> During my initial investigation, I found out that the UVD FW got placed
> in the first segment *because* things were already placed there.  This
> is why adding a 'stolen_vga_memory' substitute was an effective workaround.
> 
> So, CIK is *already* forced to deal with an overloaded first segment
> and, with the inverted approach, will continue to do so for typical use
> cases.  Explicitly placing the UVD FW into the first segment just makes
> this guaranteed.
> 
> I did implement a module parameter for testing that allows designating a
> specific 256MB segment as the legacy UVD segment.  I can polish this up
> so the user has the option to manually relieve some of the first segment
> pressure on SI and CIK devices.

The module parameter is fine for your testing, but that is not something I would accept upstream.

As soon as things are not working automatically any more people will start to complain. That we have a manual work around is only feasible to a minority.

Regards,
Christian.

> 
> I haven't run into a situation where I've needed this during normal use,
> but I can certainly appreciate it being available.
> 
> Thanks,
> John