On 11/1/23 10:56, Thomas Hellström wrote:
On Wed, 2023-11-01 at 10:41 +0100, Thomas Hellström wrote:
Hi, Danilo,
On Tue, 2023-10-31 at 18:52 +0100, Danilo Krummrich wrote:
On 10/31/23 17:45, Thomas Hellström wrote:
On Tue, 2023-10-31 at 17:39 +0100, Danilo Krummrich wrote:
On 10/31/23 12:25, Thomas Hellström wrote:
On Mon, 2023-10-23 at 22:16 +0200, Danilo Krummrich wrote:
Add an abstraction layer between the drm_gpuva mappings of
a
particular
drm_gem_object and this GEM object itself. The abstraction
represents
a
combination of a drm_gem_object and drm_gpuvm. The
drm_gem_object
holds
a list of drm_gpuvm_bo structures (the structure
representing
this
abstraction), while each drm_gpuvm_bo contains list of
mappings
of
this
GEM object.
This has multiple advantages:
1) We can use the drm_gpuvm_bo structure to attach it to
various
lists
of the drm_gpuvm. This is useful for tracking external
and
evicted
objects per VM, which is introduced in subsequent
patches.
2) Finding mappings of a certain drm_gem_object mapped in a
certain
drm_gpuvm becomes much cheaper.
3) Drivers can derive and extend the structure to easily
represent
driver specific states of a BO for a certain GPUVM.
The idea of this abstraction was taken from amdgpu, hence
the
credit
for
this idea goes to the developers of amdgpu.
Cc: Christian König <christian.koenig@xxxxxxx>
Signed-off-by: Danilo Krummrich <dakr@xxxxxxxxxx>
---
drivers/gpu/drm/drm_gpuvm.c | 335
+++++++++++++++++++++--
--
drivers/gpu/drm/nouveau/nouveau_uvmm.c | 64 +++--
include/drm/drm_gem.h | 32 +--
include/drm/drm_gpuvm.h | 188
+++++++++++++-
4 files changed, 533 insertions(+), 86 deletions(-)
That checkpatch.pl error still remains as well.
I guess you refer to:
ERROR: do not use assignment in if condition
#633: FILE: drivers/gpu/drm/nouveau/nouveau_uvmm.c:1165:
+ if (!(op->gem.obj = obj))
This was an intentional decision, since in this specific case
it
seems to
be more readable than the alternatives.
However, if we consider this to be a hard rule, which we never
ever
break,
I'm fine changing it too.
With the errors, sooner or later they are going to start generate
patches to "fix" them. In this particular case also Xe CI is
complaining and abort building when I submit the Xe adaptation,
so
it'd
be good to be checkpatch.pl conformant IMHO.
Ok, I will change this one.
However, in general my opinion on coding style is that we should
preserve us
the privilege to deviate from it when we agree it makes sense and
improves
the code quality.
Having a CI forcing people to *blindly* follow certain rules and
even
abort
building isn't very beneficial in that respect.
Also, consider patches which partially change a line of code that
already
contains a coding style "issue" - the CI would also block you on
that
one I
guess. Besides that it seems to block you on unrelated code, note
that the
assignment in question is from Nouveau and not from GPUVM.
Yes, I completely agree that having CI enforce error free coding
style
checks is bad, and I'll see if I can get that changed on Xe CI. To my
Knowledge It hasn't always been like that.
But OTOH my take on this is that if there are coding style rules and
recommendations we should try to follow them unless there are
*strong*
reasons not to. Sometimes that may result in code that may be a
little
harder to read, but OTOH a reviewer won't have to read up on the
component's style flavor before reviewing and it will avoid future
style fix patches.
Basically meaning I'll continue to point those out when reviewing in
case the author made an oversight, but won't require fixing for an R-B
if the component owner thinks otherwise.
Yeah, I fully agree on that. That's why I changed it. I still think it was
better as it was, but clearly way too minor to break the rules.
- Danilo
Thanks,
Thomas
Thanks,
Thomas
- Danilo
Thanks,
Thomas
Thanks,
Thomas
