Re: [PATCH] drm/xe: Add device CXL capabilities identification

[Michal Wajdeczko <michal.wajdeczko@intel.com>]

Hi Farah,

On 26.05.2024 10:24, 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>
> ---
>  drivers/gpu/drm/xe/Makefile          |  3 ++-
>  drivers/gpu/drm/xe/xe_cxl.c          | 33 ++++++++++++++++++++++++++++
>  drivers/gpu/drm/xe/xe_cxl.h          | 14 ++++++++++++
>  drivers/gpu/drm/xe/xe_device.c       |  5 +++++
>  drivers/gpu/drm/xe/xe_device_types.h | 10 +++++++++
>  5 files changed, 64 insertions(+), 1 deletion(-)
>  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 c9f067b8f54d..faf40ff3c62e 100644
> --- a/drivers/gpu/drm/xe/Makefile
> +++ b/drivers/gpu/drm/xe/Makefile
> @@ -146,7 +146,8 @@ xe-y += xe_bb.o \
>  	xe_vram_freq.o \
>  	xe_wait_user_fence.o \
>  	xe_wa.o \
> -	xe_wopcm.o
> +	xe_wopcm.o \
> +	xe_cxl.o \

please keep .o list sorted alphabetically

>  
>  xe-$(CONFIG_HMM_MIRROR) += xe_hmm.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..da47a79ee2c2
> --- /dev/null
> +++ b/drivers/gpu/drm/xe/xe_cxl.c
> @@ -0,0 +1,33 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright © 2024 Intel Corporation
> + */
> +
> +#include "xe_cxl.h"
> +
> +int xe_init_cxl_capabilities(struct xe_device *xe, struct pci_dev *pdev)

please add kernel-doc for any new public function

and as this function is defined in xe_cxl.* then use "xe_cxl_" prefix

hmm, do you really need to explicitly pass the pdev as parameter ?
isn't it already setup and available as to_pci_dev(xe->drm.dev) ?

> +{
> +	int cxl_dvsec, rc;

pci_find_dvsec_capability() as defined as returning u16, so maybe
cxl_dvsec can be u16 too ?

> +	u16 ctrl;
> +
> +	cxl_dvsec = pci_find_dvsec_capability(pdev, PCI_DVSEC_VENDOR_ID_CXL, CXL_DVSEC_PCIE_DEVICE);
> +	if (cxl_dvsec) {

I guess the BKM is to do an early return if conditions are not met

	if (!cxl_dvsec)
		return 0;

	...
	return 0;

> +		xe->info.has_cxl_cap = true;
> +		xe->cxl.dvsec = cxl_dvsec;
> +
> +		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\n");

shouldn't we use "DVSEC" in any user facing messages ?
also maybe printing value of the rc would be good hint for diagnostics ?

> +			return rc;
> +		}
> +
> +		if (ctrl & CXL_DVSEC_MEM_ENABLE)
> +			xe->cxl.type = (ctrl & BIT(0)) ? 2 : 3;
> +		else
> +			xe->cxl.type = 1;

shouldn't we have some defines for these types, or the plan is to always
use plain numbers everywhere ?

> +
> +		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..971c6e9adc2f
> --- /dev/null
> +++ b/drivers/gpu/drm/xe/xe_cxl.h
> @@ -0,0 +1,14 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright © 2024 Intel Corporation
> + */
> +
> +#ifndef _XE_CXL_H_
> +#define _XE_CXL_H_
> +
> +#include "xe_device.h"
> +#include "../drivers/cxl/cxlpci.h"

you don't need to include these headers here,
just add forward declaration:

struct xe_device;

> +
> +int xe_init_cxl_capabilities(struct xe_device *xe, struct pci_dev *pdev);
> +
> +#endif /* _XE_CXL_H_ */

it was decided to not use include guard name in closing #endif

> diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
> index 5acf2c92789f..ca2e47a47b11 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_init_cxl_capabilities(xe, pdev);
> +	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 bc97990fd032..aad56b829ca4 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 */
> +		int dvsec;

do you need full 'int' here or maybe 'u16' is sufficient ?

> +		/** @cxl.type: cxl device type */

shouldn't we use "CXL" in documentation ?

> +		int type;

u16 ?

> +	} cxl;
> +
>  	/** @sriov: device level virtualization data */
>  	struct {
>  		/** @sriov.__mode: SR-IOV mode (Don't access directly!) */