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. Thanks, Thomas > > Thanks, > Thomas > > > > > > - Danilo > > > > > > > > Thanks, > > > Thomas > > > > > > > > > > > > > > > > > > > > > > > > > > Thanks, > > > > > Thomas > > > > > > > > > > > > > > >
