[Intel-xe] [PATCH] drm/xe: follow up on the patch to fix pvc unload issue

Wed Apr 5 18:29:52 UTC 2023

On Wed, Apr 05, 2023 at 11:14:51AM -0700, Chang, Yu bruce wrote:
>
>
>> -----Original Message-----
>> From: De Marchi, Lucas <lucas.demarchi at intel.com>
>> Sent: Wednesday, April 5, 2023 10:29 AM
>> To: Chang, Yu bruce <yu.bruce.chang at intel.com>
>> Cc: intel-xe at lists.freedesktop.org; Vivi, Rodrigo <rodrigo.vivi at intel.com>
>> Subject: Re: [Intel-xe] [PATCH] drm/xe: follow up on the patch to fix pvc
>> unload issue
>>
>> On Wed, Apr 05, 2023 at 05:08:15PM +0000, Chang, Bruce wrote:
>> >created xe_ttm_sys_mgr.h as suggested by Lucas also fixed a compile
>> >warning found by Lucas
>>
>> Let's rather make this a fixup so we don't keep breaking and fixing the build.
>> We are still rebasing the tree, so let's keep this as part of the original commit.
>> Please commit with --fixup=01362fbe5e53
>>
>Thanks Rodrigo for helping fix this!
>
>> >
>> >Cc: Lucas De Marchi <lucas.demarchi at intel.com>
>> >Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
>> >Signed-off-by: Bruce Chang <yu.bruce.chang at intel.com>
>> >---
>> > drivers/gpu/drm/xe/xe_device.c      |  1 +
>> > drivers/gpu/drm/xe/xe_device.h      |  1 -
>> > drivers/gpu/drm/xe/xe_ttm_sys_mgr.c |  1 +
>> >drivers/gpu/drm/xe/xe_ttm_sys_mgr.h | 13 +++++++++++++
>> > 4 files changed, 15 insertions(+), 1 deletion(-)  create mode 100644
>> >drivers/gpu/drm/xe/xe_ttm_sys_mgr.h
>> >
>> >diff --git a/drivers/gpu/drm/xe/xe_device.c
>> >b/drivers/gpu/drm/xe/xe_device.c index 4fb9aff27686..85caf81ae686
>> >100644
>> >--- a/drivers/gpu/drm/xe/xe_device.c
>> >+++ b/drivers/gpu/drm/xe/xe_device.c
>> >@@ -27,6 +27,7 @@
>> > #include "xe_pcode.h"
>> > #include "xe_pm.h"
>> > #include "xe_query.h"
>> >+#include "xe_ttm_sys_mgr.h"
>> > #include "xe_ttm_stolen_mgr.h"
>>
>> needs to be sorted alphabetically. See
>> f4b0bb728995 ("drm/xe: Sort includes")
>>
>What if those headers have dependency? Such as ggtt will depend on
>either vram_mgr or sys_mgr.

headers are always self-contained. If they depend on something they
should either include or fwd declare. We prefer fwd declarations so we
keep header dependencies to a minimum.

>
>> > #include "xe_vm.h"
>> > #include "xe_vm_madvise.h"
>> >diff --git a/drivers/gpu/drm/xe/xe_device.h
>> >b/drivers/gpu/drm/xe/xe_device.h index d9d1b09a8e38..d277f8985f7b
>> >100644
>> >--- a/drivers/gpu/drm/xe/xe_device.h
>> >+++ b/drivers/gpu/drm/xe/xe_device.h
>> >@@ -116,5 +116,4 @@ static inline bool xe_device_has_flat_ccs(struct
>> >xe_device *xe)  }
>> >
>> > u32 xe_device_ccs_bytes(struct xe_device *xe, u64 size); -int
>> >xe_ttm_sys_mgr_init(struct xe_device *xe);
>>
>> there was a blank line here that needs to remain
>>
>So, there needs to be a blank line before #endif
>It seems the original file was missing the blank line already.

I thought it was there already before your patch. It's not a
requirement, but it's better to follow the same style everywhere

>
>I did a quick check, there are about 7 headers violates this.

let's add the newline in them

>
>Is there a way to enforce using a script to help insert a line before
>push/code review?

we are going to have hooks to plug in the CI infra. This can be one of
the checks. See https://gitlab.freedesktop.org/drm/xe/ci/-/merge_requests/10

>
>> > #endif
>> >diff --git a/drivers/gpu/drm/xe/xe_ttm_sys_mgr.c
>> >b/drivers/gpu/drm/xe/xe_ttm_sys_mgr.c
>> >index cf5f4f73d4dc..9c2f7d4936d8 100644
>> >--- a/drivers/gpu/drm/xe/xe_ttm_sys_mgr.c
>> >+++ b/drivers/gpu/drm/xe/xe_ttm_sys_mgr.c
>> >@@ -12,6 +12,7 @@
>> >
>> > #include "xe_bo.h"
>> > #include "xe_gt.h"
>> >+#include "xe_ttm_sys_mgr.h"
>>
>> some thing here, this is the header for this specific file. It should follow what
>> we do everywhere else and be the first include.
>> See f4b0bb728995 ("drm/xe: Sort includes")
>>
>This was from the original code as well.

original code? xe_ttm_sys_mgr.c is being added now

>
>I think it would be great to put this requirement in a coding standard so it will be easily to follow.
>Not sure if a script is a better idea instead of relying on developers.

eventually we are going to have that, when the CI hooks mentioned above.
Right now we rely on people following what is around, which is always a
great way to know how to follow the coding style.

>
>> >
>> > struct xe_ttm_sys_node {
>> > 	struct ttm_buffer_object *tbo;
>> >diff --git a/drivers/gpu/drm/xe/xe_ttm_sys_mgr.h
>> >b/drivers/gpu/drm/xe/xe_ttm_sys_mgr.h
>> >new file mode 100644
>> >index 000000000000..3d70c452a660
>> >--- /dev/null
>> >+++ b/drivers/gpu/drm/xe/xe_ttm_sys_mgr.h
>> >@@ -0,0 +1,13 @@
>> >+/* SPDX-License-Identifier: MIT */
>> >+/*
>> >+ *  * Copyright (c) 2022 Intel Corporation
>>
>> we are in 2023
>>
>Yes, I also realized this, I assume this needs to be cleaned up for all files. Maybe better to have a
>separate task to clean up all files every year? Some files are still in 2021.

not really. We don't bother updating them even if we could. But when we
add new files, we should put the year we are in.

thanks
Lucas De Marchi

>
>> Lucas De Marchi
>>
>> >+ *   */
>> >+
>> >+#ifndef _XE_TTM_SYS_MGR_H_
>> >+#define _XE_TTM_SYS_MGR_H_
>> >+
>> >+struct xe_device;
>> >+
>> >+int xe_ttm_sys_mgr_init(struct xe_device *xe);
>> >+
>> >+#endif
>> >--
>> >2.25.1
>> >