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

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Wed, Apr 05, 2023 at 07:52:17PM +0000, Chang, Yu bruce wrote:
> 
> 
> > -----Original Message-----
> > From: De Marchi, Lucas <lucas.demarchi@intel.com>
> > Sent: Wednesday, April 5, 2023 11:30 AM
> > To: Chang, Yu bruce <yu.bruce.chang@intel.com>
> > Cc: intel-xe@lists.freedesktop.org; Vivi, Rodrigo <rodrigo.vivi@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 11:14:51AM -0700, Chang, Yu bruce wrote:
> > >
> > >
> > >> -----Original Message-----
> > >> From: De Marchi, Lucas <lucas.demarchi@intel.com>
> > >> Sent: Wednesday, April 5, 2023 10:29 AM
> > >> To: Chang, Yu bruce <yu.bruce.chang@intel.com>
> > >> Cc: intel-xe@lists.freedesktop.org; Vivi, Rodrigo
> > >> <rodrigo.vivi@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@intel.com>
> > >> >Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > >> >Signed-off-by: Bruce Chang <yu.bruce.chang@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.
> > 
> Make sense!
> 
> > >
> > >> > #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
> > 
> Ok, I will do it
> 
> > >
> > >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
> >
> It was a rename: was xe_ttm_gtt_mgr.c.
> 
> I will change it.
>  
> > >
> > >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.
> > 
> This was also a rename from existing xe_ttm_gtt_mgr.h to xe_ttm_sys_mgr.h
> 
> I can make the change.

Lucas and Bruce, I'm really sorry for having rushed the patch and the fixup.
My bad.
So, let's have a final clean patch. Even if the best to do now is a double
revert with a clean patch on top, so on the next rebase we can remove the
previous versions and the reverts. Or another fixup!. Up to you and just
let me know so I can try to help here, but next time without rushing it out.

> 
> Thanks,
> Bruce
> 
> > 
> > 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
> > >> >