* ✓ CI.Patch_applied: success for drm/xe: Add device CXL capabilities identification (rev3)
2024-05-27 8:40 [PATCH v3] drm/xe: Add device CXL capabilities identification Farah Kassabri
@ 2024-05-27 8:46 ` Patchwork
2024-05-27 8:46 ` ✗ CI.checkpatch: warning " Patchwork
` (3 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Patchwork @ 2024-05-27 8:46 UTC (permalink / raw)
To: Farah Kassabri; +Cc: intel-xe
== Series Details ==
Series: drm/xe: Add device CXL capabilities identification (rev3)
URL : https://patchwork.freedesktop.org/series/134047/
State : success
== Summary ==
=== Applying kernel patches on branch 'drm-tip' with base: ===
Base commit: 373526673480 drm-tip: 2024y-05m-27d-08h-37m-13s UTC integration manifest
=== git am output follows ===
Applying: drm/xe: Add device CXL capabilities identification
^ permalink raw reply [flat|nested] 6+ messages in thread* ✗ CI.checkpatch: warning for drm/xe: Add device CXL capabilities identification (rev3)
2024-05-27 8:40 [PATCH v3] drm/xe: Add device CXL capabilities identification Farah Kassabri
2024-05-27 8:46 ` ✓ CI.Patch_applied: success for drm/xe: Add device CXL capabilities identification (rev3) Patchwork
@ 2024-05-27 8:46 ` Patchwork
2024-05-27 8:46 ` ✗ CI.KUnit: failure " Patchwork
` (2 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Patchwork @ 2024-05-27 8:46 UTC (permalink / raw)
To: Farah Kassabri; +Cc: intel-xe
== Series Details ==
Series: drm/xe: Add device CXL capabilities identification (rev3)
URL : https://patchwork.freedesktop.org/series/134047/
State : warning
== Summary ==
+ KERNEL=/kernel
+ git clone https://gitlab.freedesktop.org/drm/maintainer-tools mt
Cloning into 'mt'...
warning: redirecting to https://gitlab.freedesktop.org/drm/maintainer-tools.git/
+ git -C mt rev-list -n1 origin/master
51ce9f6cd981d42d7467409d7dbc559a450abc1e
+ cd /kernel
+ git config --global --add safe.directory /kernel
+ git log -n1
commit 22e9dbbba0e9434378b90854ad98d51e971c38bc
Author: Farah Kassabri <fkassabri@habana.ai>
Date: Mon May 27 11:40:31 2024 +0300
drm/xe: Add device CXL capabilities identification
As future Intel GPUs will use CXL interface with the host
servers, this patch will add check if the xe device has CXL
capabilities or not, by reading the PCIe standard DVSEC register
and identify the CXL vendor id.
Signed-off-by: Farah Kassabri <fkassabri@habana.ai>
+ /mt/dim checkpatch 3735266734803d913dd698230e05c462806525aa drm-intel
22e9dbbba0e9 drm/xe: Add device CXL capabilities identification
Traceback (most recent call last):
File "scripts/spdxcheck.py", line 6, in <module>
from ply import lex, yacc
ModuleNotFoundError: No module named 'ply'
Traceback (most recent call last):
File "scripts/spdxcheck.py", line 6, in <module>
from ply import lex, yacc
ModuleNotFoundError: No module named 'ply'
-:26: WARNING:FILE_PATH_CHANGES: added, moved or deleted file(s), does MAINTAINERS need updating?
#26:
new file mode 100644
total: 0 errors, 1 warnings, 0 checks, 110 lines checked
^ permalink raw reply [flat|nested] 6+ messages in thread* ✗ CI.KUnit: failure for drm/xe: Add device CXL capabilities identification (rev3)
2024-05-27 8:40 [PATCH v3] drm/xe: Add device CXL capabilities identification Farah Kassabri
2024-05-27 8:46 ` ✓ CI.Patch_applied: success for drm/xe: Add device CXL capabilities identification (rev3) Patchwork
2024-05-27 8:46 ` ✗ CI.checkpatch: warning " Patchwork
@ 2024-05-27 8:46 ` Patchwork
2024-05-27 16:43 ` [PATCH v3] drm/xe: Add device CXL capabilities identification Michal Wajdeczko
2024-05-28 17:20 ` Rodrigo Vivi
4 siblings, 0 replies; 6+ messages in thread
From: Patchwork @ 2024-05-27 8:46 UTC (permalink / raw)
To: Farah Kassabri; +Cc: intel-xe
== Series Details ==
Series: drm/xe: Add device CXL capabilities identification (rev3)
URL : https://patchwork.freedesktop.org/series/134047/
State : failure
== Summary ==
+ trap cleanup EXIT
+ /kernel/tools/testing/kunit/kunit.py run --kunitconfig /kernel/drivers/gpu/drm/xe/.kunitconfig
ERROR:root:../drivers/gpu/drm/xe/xe_cxl.c: In function ‘xe_cxl_init_capabilities’:
../drivers/gpu/drm/xe/xe_cxl.c:30:46: error: ‘PCI_DVSEC_VENDOR_ID_CXL’ undeclared (first use in this function); did you mean ‘PCI_VENDOR_ID_CXL’?
30 | cxl_dvsec = pci_find_dvsec_capability(pdev, PCI_DVSEC_VENDOR_ID_CXL, CXL_DVSEC_PCIE_DEVICE);
| ^~~~~~~~~~~~~~~~~~~~~~~
| PCI_VENDOR_ID_CXL
../drivers/gpu/drm/xe/xe_cxl.c:30:46: note: each undeclared identifier is reported only once for each function it appears in
make[7]: *** [../scripts/Makefile.build:244: drivers/gpu/drm/xe/xe_cxl.o] Error 1
make[7]: *** Waiting for unfinished jobs....
make[6]: *** [../scripts/Makefile.build:485: drivers/gpu/drm/xe] Error 2
make[6]: *** Waiting for unfinished jobs....
make[5]: *** [../scripts/Makefile.build:485: drivers/gpu/drm] Error 2
make[4]: *** [../scripts/Makefile.build:485: drivers/gpu] Error 2
make[4]: *** Waiting for unfinished jobs....
make[3]: *** [../scripts/Makefile.build:485: drivers] Error 2
make[3]: *** Waiting for unfinished jobs....
../lib/iomap.c:156:5: warning: no previous prototype for ‘ioread64_lo_hi’ [-Wmissing-prototypes]
156 | u64 ioread64_lo_hi(const void __iomem *addr)
| ^~~~~~~~~~~~~~
../lib/iomap.c:163:5: warning: no previous prototype for ‘ioread64_hi_lo’ [-Wmissing-prototypes]
163 | u64 ioread64_hi_lo(const void __iomem *addr)
| ^~~~~~~~~~~~~~
../lib/iomap.c:170:5: warning: no previous prototype for ‘ioread64be_lo_hi’ [-Wmissing-prototypes]
170 | u64 ioread64be_lo_hi(const void __iomem *addr)
| ^~~~~~~~~~~~~~~~
../lib/iomap.c:178:5: warning: no previous prototype for ‘ioread64be_hi_lo’ [-Wmissing-prototypes]
178 | u64 ioread64be_hi_lo(const void __iomem *addr)
| ^~~~~~~~~~~~~~~~
../lib/iomap.c:264:6: warning: no previous prototype for ‘iowrite64_lo_hi’ [-Wmissing-prototypes]
264 | void iowrite64_lo_hi(u64 val, void __iomem *addr)
| ^~~~~~~~~~~~~~~
../lib/iomap.c:272:6: warning: no previous prototype for ‘iowrite64_hi_lo’ [-Wmissing-prototypes]
272 | void iowrite64_hi_lo(u64 val, void __iomem *addr)
| ^~~~~~~~~~~~~~~
../lib/iomap.c:280:6: warning: no previous prototype for ‘iowrite64be_lo_hi’ [-Wmissing-prototypes]
280 | void iowrite64be_lo_hi(u64 val, void __iomem *addr)
| ^~~~~~~~~~~~~~~~~
../lib/iomap.c:288:6: warning: no previous prototype for ‘iowrite64be_hi_lo’ [-Wmissing-prototypes]
288 | void iowrite64be_hi_lo(u64 val, void __iomem *addr)
| ^~~~~~~~~~~~~~~~~
make[2]: *** [/kernel/Makefile:1934: .] Error 2
make[1]: *** [/kernel/Makefile:240: __sub-make] Error 2
make: *** [Makefile:240: __sub-make] Error 2
[08:46:18] Configuring KUnit Kernel ...
Generating .config ...
Populating config with:
$ make ARCH=um O=.kunit olddefconfig
[08:46:23] Building KUnit Kernel ...
Populating config with:
$ make ARCH=um O=.kunit olddefconfig
Building with:
$ make ARCH=um O=.kunit --jobs=48
+ cleanup
++ stat -c %u:%g /kernel
+ chown -R 1003:1003 /kernel
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v3] drm/xe: Add device CXL capabilities identification
2024-05-27 8:40 [PATCH v3] drm/xe: Add device CXL capabilities identification Farah Kassabri
` (2 preceding siblings ...)
2024-05-27 8:46 ` ✗ CI.KUnit: failure " Patchwork
@ 2024-05-27 16:43 ` Michal Wajdeczko
2024-05-28 17:20 ` Rodrigo Vivi
4 siblings, 0 replies; 6+ messages in thread
From: Michal Wajdeczko @ 2024-05-27 16:43 UTC (permalink / raw)
To: Farah Kassabri, intel-xe
On 27.05.2024 10:40, Farah Kassabri wrote:
> As future Intel GPUs will use CXL interface with the host
> servers, this patch will add check if the xe device has CXL
> capabilities or not, by reading the PCIe standard DVSEC register
> and identify the CXL vendor id.
>
> Signed-off-by: Farah Kassabri <fkassabri@habana.ai>
> ---
> Changes in v3:
> fixes following review.
>
> drivers/gpu/drm/xe/Makefile | 1 +
> drivers/gpu/drm/xe/xe_cxl.c | 51 ++++++++++++++++++++++++++++
> drivers/gpu/drm/xe/xe_cxl.h | 13 +++++++
> drivers/gpu/drm/xe/xe_device.c | 5 +++
> drivers/gpu/drm/xe/xe_device_types.h | 10 ++++++
> 5 files changed, 80 insertions(+)
> create mode 100644 drivers/gpu/drm/xe/xe_cxl.c
> create mode 100644 drivers/gpu/drm/xe/xe_cxl.h
>
> diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
> index a5959bb7b1fb..65c0a0317ee7 100644
> --- a/drivers/gpu/drm/xe/Makefile
> +++ b/drivers/gpu/drm/xe/Makefile
> @@ -65,6 +65,7 @@ $(uses_generated_oob): $(generated_oob)
> xe-y += xe_bb.o \
> xe_bo.o \
> xe_bo_evict.o \
> + xe_cxl.o \
> xe_debugfs.o \
> xe_devcoredump.o \
> xe_device.o \
> diff --git a/drivers/gpu/drm/xe/xe_cxl.c b/drivers/gpu/drm/xe/xe_cxl.c
> new file mode 100644
> index 000000000000..78e4179e965a
> --- /dev/null
> +++ b/drivers/gpu/drm/xe/xe_cxl.c
> @@ -0,0 +1,51 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright © 2024 Intel Corporation
> + */
> +
> +#include "xe_cxl.h"
> +#include "xe_device.h"
btw, likely you only need "xe_device_types.h"
> +#include "../drivers/cxl/cxlpci.h"
and this still doesn't look good
can you double check with maintainers ?
> +
> +enum {
> + CXL_DEV_TYPE_ONE = 1,
> + CXL_DEV_TYPE_TWO,
> + CXL_DEV_TYPE_THREE
> +};
> +
> +/**
> + * xe_cxl_init_capabilities - set CXL device capabilities
> + * @xe: xe device structure
> + *
> + * Set CXL capabilities and information of the xe device.
> + *
> + * Return: 0 on success, negative value on failure.
usually on failure we return "negative error code (errno)"
but ...
> + */
> +int xe_cxl_init_capabilities(struct xe_device *xe)
> +{
> + struct pci_dev *pdev = to_pci_dev(xe->drm.dev);
> + u16 ctrl, cxl_dvsec;
> + int rc;
> +
> + cxl_dvsec = pci_find_dvsec_capability(pdev, PCI_DVSEC_VENDOR_ID_CXL, CXL_DVSEC_PCIE_DEVICE);
(btw, this line doesn't compile on KUnit)
> + if (!cxl_dvsec)
> + return -1;
... here it's a plain number (while likely should be an -errno value)
and ...
> +
> + rc = pci_read_config_word(pdev, cxl_dvsec + CXL_DVSEC_CTRL_OFFSET, &ctrl);
> + if (rc < 0) {
(btw, pci_read_config_word() will return > 0 on error)
> + drm_err(&xe->drm, "Failed to read DVSEC_CTRL register(%d)\n", rc);
> + return rc;
... here it will be non-negative PCI error code
> + }
> +
> + xe->info.has_cxl_cap = true;
> + xe->cxl.dvsec = cxl_dvsec;
> +
> + if (ctrl & CXL_DVSEC_MEM_ENABLE)
> + xe->cxl.type = (ctrl & BIT(0)) ? CXL_DEV_TYPE_TWO : CXL_DEV_TYPE_THREE;
> + else
> + xe->cxl.type = CXL_DEV_TYPE_ONE;
> +
> + drm_dbg(&xe->drm, "The device has CXL capability, type %u\n", xe->cxl.type);
> +
> + return 0;
> +}
> diff --git a/drivers/gpu/drm/xe/xe_cxl.h b/drivers/gpu/drm/xe/xe_cxl.h
> new file mode 100644
> index 000000000000..82e36285c383
> --- /dev/null
> +++ b/drivers/gpu/drm/xe/xe_cxl.h
> @@ -0,0 +1,13 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright © 2024 Intel Corporation
> + */
> +
> +#ifndef _XE_CXL_H_
> +#define _XE_CXL_H_
> +
> +struct xe_device;
> +
> +int xe_cxl_init_capabilities(struct xe_device *xe);
> +
> +#endif
> diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
> index 5acf2c92789f..04677c9ff10f 100644
> --- a/drivers/gpu/drm/xe/xe_device.c
> +++ b/drivers/gpu/drm/xe/xe_device.c
> @@ -50,6 +50,7 @@
> #include "xe_ttm_sys_mgr.h"
> #include "xe_vm.h"
> #include "xe_wait_user_fence.h"
> +#include "xe_cxl.h"
includes shall also be sorted
>
> static int xe_file_open(struct drm_device *dev, struct drm_file *file)
> {
> @@ -312,6 +313,10 @@ struct xe_device *xe_device_create(struct pci_dev *pdev,
> if (WARN_ON(err))
> goto err;
>
> + err = xe_cxl_init_capabilities(xe);
> + if (err)
> + goto err;
are you 100% sure that we should abort driver load on init failure?
and now it will fail on every platform without cxl caps
was this patch tested ?
> +
> return xe;
>
> err:
> diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
> index 8e7048ff3ee5..323c3d57fdb8 100644
> --- a/drivers/gpu/drm/xe/xe_device_types.h
> +++ b/drivers/gpu/drm/xe/xe_device_types.h
> @@ -285,6 +285,8 @@ struct xe_device {
> u8 has_atomic_enable_pte_bit:1;
> /** @info.has_device_atomics_on_smem: Supports device atomics on SMEM */
> u8 has_device_atomics_on_smem:1;
> + /** @info.has_cxl_cap: device has CXL capabilities */
> + u8 has_cxl_cap:1;
>
> #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
> struct {
> @@ -321,6 +323,14 @@ struct xe_device {
> struct ttm_resource_manager sys_mgr;
> } mem;
>
> + /** @cxl: CXL info for device */
> + struct {
> + /** @cxl.dvsec: offset to device DVSEC */
> + u16 dvsec;
> + /** @cxl.type: CXL device type */
> + u16 type;
> + } cxl;
> +
> /** @sriov: device level virtualization data */
> struct {
> /** @sriov.__mode: SR-IOV mode (Don't access directly!) */
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v3] drm/xe: Add device CXL capabilities identification
2024-05-27 8:40 [PATCH v3] drm/xe: Add device CXL capabilities identification Farah Kassabri
` (3 preceding siblings ...)
2024-05-27 16:43 ` [PATCH v3] drm/xe: Add device CXL capabilities identification Michal Wajdeczko
@ 2024-05-28 17:20 ` Rodrigo Vivi
4 siblings, 0 replies; 6+ messages in thread
From: Rodrigo Vivi @ 2024-05-28 17:20 UTC (permalink / raw)
To: Farah Kassabri; +Cc: intel-xe, Michal.Wajdeczko
On Mon, May 27, 2024 at 11:40:31AM +0300, Farah Kassabri wrote:
> As future Intel GPUs will use CXL interface with the host
> servers, this patch will add check if the xe device has CXL
> capabilities or not, by reading the PCIe standard DVSEC register
> and identify the CXL vendor id.
The biggest question of all, is why do we need this?
PCI also has a lot of capabilities and we (driver) don't have
to expose that. Why would it be different for CXL?
>
> Signed-off-by: Farah Kassabri <fkassabri@habana.ai>
> ---
> Changes in v3:
> fixes following review.
>
> drivers/gpu/drm/xe/Makefile | 1 +
> drivers/gpu/drm/xe/xe_cxl.c | 51 ++++++++++++++++++++++++++++
> drivers/gpu/drm/xe/xe_cxl.h | 13 +++++++
> drivers/gpu/drm/xe/xe_device.c | 5 +++
> drivers/gpu/drm/xe/xe_device_types.h | 10 ++++++
> 5 files changed, 80 insertions(+)
> create mode 100644 drivers/gpu/drm/xe/xe_cxl.c
> create mode 100644 drivers/gpu/drm/xe/xe_cxl.h
>
> diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
> index a5959bb7b1fb..65c0a0317ee7 100644
> --- a/drivers/gpu/drm/xe/Makefile
> +++ b/drivers/gpu/drm/xe/Makefile
> @@ -65,6 +65,7 @@ $(uses_generated_oob): $(generated_oob)
> xe-y += xe_bb.o \
> xe_bo.o \
> xe_bo_evict.o \
> + xe_cxl.o \
> xe_debugfs.o \
> xe_devcoredump.o \
> xe_device.o \
> diff --git a/drivers/gpu/drm/xe/xe_cxl.c b/drivers/gpu/drm/xe/xe_cxl.c
> new file mode 100644
> index 000000000000..78e4179e965a
> --- /dev/null
> +++ b/drivers/gpu/drm/xe/xe_cxl.c
> @@ -0,0 +1,51 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright © 2024 Intel Corporation
> + */
> +
> +#include "xe_cxl.h"
> +#include "xe_device.h"
> +#include "../drivers/cxl/cxlpci.h"
> +
> +enum {
> + CXL_DEV_TYPE_ONE = 1,
> + CXL_DEV_TYPE_TWO,
> + CXL_DEV_TYPE_THREE
> +};
> +
> +/**
> + * xe_cxl_init_capabilities - set CXL device capabilities
> + * @xe: xe device structure
> + *
> + * Set CXL capabilities and information of the xe device.
> + *
> + * Return: 0 on success, negative value on failure.
> + */
> +int xe_cxl_init_capabilities(struct xe_device *xe)
> +{
> + struct pci_dev *pdev = to_pci_dev(xe->drm.dev);
> + u16 ctrl, cxl_dvsec;
> + int rc;
> +
> + cxl_dvsec = pci_find_dvsec_capability(pdev, PCI_DVSEC_VENDOR_ID_CXL, CXL_DVSEC_PCIE_DEVICE);
> + if (!cxl_dvsec)
> + return -1;
> +
> + rc = pci_read_config_word(pdev, cxl_dvsec + CXL_DVSEC_CTRL_OFFSET, &ctrl);
> + if (rc < 0) {
> + drm_err(&xe->drm, "Failed to read DVSEC_CTRL register(%d)\n", rc);
> + return rc;
> + }
> +
> + xe->info.has_cxl_cap = true;
> + xe->cxl.dvsec = cxl_dvsec;
> +
> + if (ctrl & CXL_DVSEC_MEM_ENABLE)
> + xe->cxl.type = (ctrl & BIT(0)) ? CXL_DEV_TYPE_TWO : CXL_DEV_TYPE_THREE;
> + else
> + xe->cxl.type = CXL_DEV_TYPE_ONE;
> +
> + drm_dbg(&xe->drm, "The device has CXL capability, type %u\n", xe->cxl.type);
> +
> + return 0;
> +}
> diff --git a/drivers/gpu/drm/xe/xe_cxl.h b/drivers/gpu/drm/xe/xe_cxl.h
> new file mode 100644
> index 000000000000..82e36285c383
> --- /dev/null
> +++ b/drivers/gpu/drm/xe/xe_cxl.h
> @@ -0,0 +1,13 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright © 2024 Intel Corporation
> + */
> +
> +#ifndef _XE_CXL_H_
> +#define _XE_CXL_H_
> +
> +struct xe_device;
> +
> +int xe_cxl_init_capabilities(struct xe_device *xe);
> +
> +#endif
> diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
> index 5acf2c92789f..04677c9ff10f 100644
> --- a/drivers/gpu/drm/xe/xe_device.c
> +++ b/drivers/gpu/drm/xe/xe_device.c
> @@ -50,6 +50,7 @@
> #include "xe_ttm_sys_mgr.h"
> #include "xe_vm.h"
> #include "xe_wait_user_fence.h"
> +#include "xe_cxl.h"
>
> static int xe_file_open(struct drm_device *dev, struct drm_file *file)
> {
> @@ -312,6 +313,10 @@ struct xe_device *xe_device_create(struct pci_dev *pdev,
> if (WARN_ON(err))
> goto err;
>
> + err = xe_cxl_init_capabilities(xe);
> + if (err)
> + goto err;
> +
> return xe;
>
> err:
> diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
> index 8e7048ff3ee5..323c3d57fdb8 100644
> --- a/drivers/gpu/drm/xe/xe_device_types.h
> +++ b/drivers/gpu/drm/xe/xe_device_types.h
> @@ -285,6 +285,8 @@ struct xe_device {
> u8 has_atomic_enable_pte_bit:1;
> /** @info.has_device_atomics_on_smem: Supports device atomics on SMEM */
> u8 has_device_atomics_on_smem:1;
> + /** @info.has_cxl_cap: device has CXL capabilities */
> + u8 has_cxl_cap:1;
>
> #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
> struct {
> @@ -321,6 +323,14 @@ struct xe_device {
> struct ttm_resource_manager sys_mgr;
> } mem;
>
> + /** @cxl: CXL info for device */
> + struct {
> + /** @cxl.dvsec: offset to device DVSEC */
> + u16 dvsec;
> + /** @cxl.type: CXL device type */
> + u16 type;
> + } cxl;
> +
> /** @sriov: device level virtualization data */
> struct {
> /** @sriov.__mode: SR-IOV mode (Don't access directly!) */
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 6+ messages in thread