Messages in this thread | | | Date | Wed, 1 Nov 2023 18:23:35 +0100 | Subject | Re: [PATCH drm-misc-next v7 4/7] drm/gpuvm: add an abstraction for a VM / BO combination | From | Danilo Krummrich <> |
| |
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@amd.com> >>>>>>> Signed-off-by: Danilo Krummrich <dakr@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. >>>> >>>> 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 >>>>>> >>>>> >>>> >>> >> >
| |