Thomas Hellström
2023-Oct-31 11:25 UTC
[Nouveau] [PATCH drm-misc-next v7 4/7] drm/gpuvm: add an abstraction for a VM / BO combination
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 at amd.com> > Signed-off-by: Danilo Krummrich <dakr at redhat.com> > --- > ?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. Thanks, Thomas
Danilo Krummrich
2023-Oct-31 16:39 UTC
[Nouveau] [PATCH drm-misc-next v7 4/7] drm/gpuvm: add an abstraction for a VM / BO combination
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 at amd.com> >> Signed-off-by: Danilo Krummrich <dakr at redhat.com> >> --- >> ?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.> > Thanks, > Thomas >