Message-ID: <b0eab26f-23e8-49cb-b831-1188d5abda86@redhat.com>
Date:   Wed, 1 Nov 2023 18:23:35 +0100
From:   Danilo Krummrich <dakr@...hat.com>
To:     Thomas Hellström 
        <thomas.hellstrom@...ux.intel.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 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@....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.

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
>>>>>>
>>>>>
>>>>
>>>
>>
> 

Powered by blists - more mailing lists