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

Wed Apr 5 19:52:17 UTC 2023

> -----Original Message-----
> From: De Marchi, Lucas <lucas.demarchi at intel.com>
> Sent: Wednesday, April 5, 2023 11:30 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 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.
> 
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.

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