[Intel-xe] [RFC PATCH v3 1/3] drm/xe: Introduce function pointers in xe_gt.c with local structure

[Ohad Sharabi]

On 20/03/2023 10:12, Francois Dugast wrote:
> A local structure of function pointers is used as a minimal
> hardware abstraction layer to split between common and platform
> specific code. This structure is initialized once with the
> functions matching the platform, then it is used in the common
> code with no need for switch or if/else to detect the platform.
> For now only the function setup_private_ppat() is refactored.
>
> Signed-off-by: Francois Dugast <francois.dugast at intel.com>
> ---
>   drivers/gpu/drm/xe/xe_device_types.h |  1 +
>   drivers/gpu/drm/xe/xe_gt.c           | 49 ++++++++++++++++++++++++----
>   2 files changed, 44 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
> index 4c65598652e8..7d72fb3ff08d 100644
> --- a/drivers/gpu/drm/xe/xe_device_types.h
> +++ b/drivers/gpu/drm/xe/xe_device_types.h
> @@ -313,6 +313,7 @@ struct xe_device {
>   		u32 lvds_channel_mode;
>   	} params;
>   #endif
> +	const struct xe_gt_platform_funcs *gt_funcs;
>   };
>   
>   /**
> diff --git a/drivers/gpu/drm/xe/xe_gt.c b/drivers/gpu/drm/xe/xe_gt.c
> index daa433d0f2f5..718cd18b6421 100644
> --- a/drivers/gpu/drm/xe/xe_gt.c
> +++ b/drivers/gpu/drm/xe/xe_gt.c
> @@ -42,6 +42,26 @@
>   #include "xe_wa.h"
>   #include "xe_wopcm.h"
>   
> +struct xe_gt_platform_funcs {
> +	void (*setup_private_ppat)(struct xe_gt *gt);
> +};
> +
> +static void mtl_setup_private_ppat(struct xe_gt *gt);
> +static void pvc_setup_private_ppat(struct xe_gt *gt);
> +static void tgl_setup_private_ppat(struct xe_gt *gt);
> +
> +static const struct xe_gt_platform_funcs mtl_funcs = {
> +	.setup_private_ppat = mtl_setup_private_ppat,
> +};
> +
> +static const struct xe_gt_platform_funcs pvc_funcs = {
> +	.setup_private_ppat = pvc_setup_private_ppat,
> +};
> +
> +static const struct xe_gt_platform_funcs tgl_funcs = {
> +	.setup_private_ppat = tgl_setup_private_ppat,
> +};
> +
>   struct xe_gt *xe_find_full_gt(struct xe_gt *gt)
>   {
>   	struct xe_gt *search;
> @@ -161,12 +181,8 @@ static void setup_private_ppat(struct xe_gt *gt)
>   {
>   	struct xe_device *xe = gt_to_xe(gt);
>   
> -	if (xe->info.platform == XE_METEORLAKE)
> -		mtl_setup_private_ppat(gt);
> -	else if (xe->info.platform == XE_PVC)
> -		pvc_setup_private_ppat(gt);
> -	else
> -		tgl_setup_private_ppat(gt);
> +	if (xe->gt_funcs->setup_private_ppat)
> +		xe->gt_funcs->setup_private_ppat(gt);
>   }
>   
>   static int gt_ttm_mgr_init(struct xe_gt *gt)
> @@ -380,6 +396,27 @@ int xe_gt_record_default_lrcs(struct xe_gt *gt)
>   int xe_gt_init_early(struct xe_gt *gt)
>   {
>   	int err;
> +	struct xe_device *xe = gt_to_xe(gt);
> +
> +	switch (xe->info.platform) {
> +	case XE_METEORLAKE:
> +		xe->gt_funcs = &mtl_funcs;
> +		break;
> +	case XE_PVC:
> +		xe->gt_funcs = &pvc_funcs;
> +		break;
> +	case XE_ROCKETLAKE:
> +	case XE_DG1:
> +	case XE_DG2:
> +	case XE_ALDERLAKE_S:
> +	case XE_ALDERLAKE_P:
> +	case XE_TIGERLAKE:
> +		xe->gt_funcs = &tgl_funcs;
> +		break;
> +	default:
> +		DRM_ERROR("Unsupported platform\n");
> +		break;
> +	}

Won't initializing the function pointer here would do it per number of 
tiles (but over-writing the same pointer)?

Maybe consider moving all FP init to the very beginning of 
xe_device_probe (sort of sw_init)? We may need some future functions at 
an early stage.

In Addition, if I understood correctly, this switch-case would be 
present for each FP group.
Do you think it's possible to get this info directly from the descriptor 
(the driver data in pci_device_id)?

>   
>   	xe_force_wake_init_gt(gt, gt_to_fw(gt));
>   

Thanks,

Ohad