Message-ID: <bfcaf8f777c2c6f018423bb1840a58ab7c80d97f.camel@linux.intel.com>
Date:   Wed, 01 Nov 2023 10:56:14 +0100
From:   Thomas Hellström 
        <thomas.hellstrom@...ux.intel.com>
To:     Danilo Krummrich <dakr@...hat.com>, airlied@...il.com,
        daniel@...ll.ch, matthew.brost@...el.com, sarah.walker@...tec.com,
        donald.robson@...tec.com, boris.brezillon@...labora.com,
        christian.koenig@....com, faith@...strand.net
Cc:     dri-devel@...ts.freedesktop.org, nouveau@...ts.freedesktop.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH drm-misc-next v7 4/7] drm/gpuvm: add an abstraction for
 a VM / BO combination

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@....com>
> > > > > > Signed-off-by: Danilo Krummrich <dakr@...hat.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.
> > > 
> > > 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
> > > > > 
> > > > 
> > > 
> > 
> 

Powered by blists - more mailing lists