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

[Chang, Yu bruce]

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

> > #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 did a quick check, there are about 7 headers violates this.

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

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

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.

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