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

Farah Kassabri fkassabri at habana.ai
Sun May 26 13:10:43 UTC 2024


On 5/26/2024 13:32, Michal Wajdeczko wrote:
> [You don't often get email from michal.wajdeczko at intel.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> 
> 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 at 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;

I need the includes cannot just forward declaration.
for example: PCI_DVSEC_VENDOR_ID_CXL and other defines exist in cxlpci.h
to_pci_dev needs the xe_device.h also the drm_dbg/err ....
As to all other comments agreed and fixed.

BR,
Farah
> 
>> +
>> +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!) */



More information about the Intel-xe mailing list