On 05.03.2025 08:15, Christian König wrote:
Am 04.03.25 um 16:39 schrieb Lis, Tomasz:
There was no NACK, and no further questions/comments for a month.
From that, we conclude that the proposed change is considered acceptable.
Well there was also not any comment from Arun nor Matthew who are the de facto maintainers for this particular code.
Saying that the whole change looks completely unecessary and overly complex to me.
Just maintain the offset for your VF separately to the drm_mm nodes and you don't need to change all nodes any more when that offset changes.
Going to comment on the other reply as well.
Regards,
Christian.
I will make sure to add Arun and Matthew in the further discussion.
On 05.03.2025 08:18, Christian König wrote:
Am 05.02.25 um 17:02 schrieb Lis, Tomasz:
On 05.02.2025 09:32, Christian König wrote:
Am 04.02.25 um 23:41 schrieb Tomasz Lis:
This RFC asks for introduction of an interface which allows to shift
a range managed by drm_mm instance without repeating the node list
creation.
What do you mean with "shift" here? As far as I can see from the code you just modify the start address of each node, e.g. you manipulate the offset.
Yes, that's the idea - move the address space to a different base.
Should I use different terminology?
Yes, absolutely. A shift is usually something different than an offset.
Ok, instead of "shifting a range" I will refer to that as "adding offset
to the range".
The long explanation:
Single Root I/O Virtualization is becoming a standard GFX feature
in server environments. Virtual Machines provided with direct access
to virtualized GFX hardware, in form of VFs, need to support the
standard set of features expected by Virtual Machine Managers.
These standard features include ability to save the VM state, and
later restore the VM, possibly on another machine with different
setup. For the restore to succeed, the GFX hardware model must match;
but for its configuration, some differences are often allowed. Such
alterations may include a different range of non-virtualized
resources assigned to the VF, including global address spaces.
If any non-virtualized address space is saved, as part of VM state,
on one machine and restored on another, it may happen that the target
range differs from source range. To shift the address space,
currently creating a new drm_mm object is required, and moving all
nodes to the new object while adding the shift.
GFX hardware handled by Xe driver contains Global Graphics
Translation Table, which is an example of such non-virtualized
resource. Should this interface change be accepted, a series which
utilizes this interface in Xe driver will be prepared.
Well that sounds exactly like what AMD is doing, but we just add the VRAM base to the MM node when calculating the final address in the MC address space.
On the other hand AMD hardware has different address spaces, e.g. VRAM in page table always starts at address 0 while in the MC address space it has a certain offset which differs from device to device.
We use the drm_mm in a similar manner - some address spaces start at 0, other use a narrower range. But we do not add any base after creation - we have the nodes at final offsets.
There is one more technique we use in few places to restrict range of valid addresses - we create "balloon" nodes which fill up the inaccessible areas. When the accessible range shifts, we then remove all notes, and add them back with balloons resized and the rest of nodes just moved by an offset.
Using the function proposed in this RFC will actually force us to abandon the ballooning approach for VF Global Gfx Translation Table address space and refactor the code to give only the accessible range to drm_mm. That is why I'm sending the RFC without use example - we will have to refactor the Xe code to use it.
Well that approach looks overly complex to me.
Just maintain the VF offsets separately to the drm_mm nodes and apply it in your address calculation whenever necessary. That's basically how all other drivers do it as far as I can see.
This way you don't need to update all nodes whenever your base offsets changes.
My current implementation, without drm_mm modification, involves
removing and re-adding all the nodes. The proposed implementation makes
the code simpler, not more complex.
For maintaining VF offsets separately - this could be done, if
Matthew/Arun agree to this extra addition within Xe driver. However, if
that is the preferred implementation, then why the `drm_mm_init()`
declaration contains a `start` parameter? Do we really want to have
support of offsets within `drm_mm`, but keep the support incomplete?
Are we sure that the other drivers you mention, went for a solution with
extra offset in addition to drm_mm `start` offset because that is the
best engineering solution? Maybe I'm missing something, but a solution
with multiple offsets stacking together looks to me like a workaround. A
workaround likely introduced to avoid spending the time required for
discussions and finding an agreement about improving the common drm_mm
utility.
Well, let me know what you think.
-Tomasz
Regards,
Christian.
-Tomasz
Regards,
Christian.
Signed-off-by: Tomasz Lis <tomasz.lis@xxxxxxxxx>
Tomasz Lis (1):
drm_mm: Introduce address space shifting
drivers/gpu/drm/drm_mm.c | 24 ++++++++++++++++++++++++
include/drm/drm_mm.h | 1 +
2 files changed, 25 insertions(+)
