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

[Jani Nikula <jani.nikula@linux.intel.com>]

On Tue, 14 Mar 2023, Francois Dugast <francois.dugast@intel.com> 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@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_device_types.h |  1 +
>  drivers/gpu/drm/xe/xe_gt.c           | 46 +++++++++++++++++++---------
>  2 files changed, 33 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
> index 034e0956f4ea..411e7a62d28a 100644
> --- a/drivers/gpu/drm/xe/xe_device_types.h
> +++ b/drivers/gpu/drm/xe/xe_device_types.h
> @@ -311,6 +311,7 @@ struct xe_device {
>  		u32 lvds_channel_mode;
>  	} params;
>  #endif
> +	struct xe_gt_platform_funcs *gt_funcs;

IMO this should be a const pointer.

>  };
>  
>  /**
> diff --git a/drivers/gpu/drm/xe/xe_gt.c b/drivers/gpu/drm/xe/xe_gt.c
> index daa433d0f2f5..3b092f951378 100644
> --- a/drivers/gpu/drm/xe/xe_gt.c
> +++ b/drivers/gpu/drm/xe/xe_gt.c
> @@ -42,6 +42,10 @@
>  #include "xe_wa.h"
>  #include "xe_wopcm.h"
>  
> +struct xe_gt_platform_funcs {
> +	void (*setup_private_ppat)(struct xe_gt *gt);
> +};
> +
>  struct xe_gt *xe_find_full_gt(struct xe_gt *gt)
>  {
>  	struct xe_gt *search;
> @@ -157,18 +161,6 @@ static void mtl_setup_private_ppat(struct xe_gt *gt)
>  			MTL_PPAT_0_WB | MTL_3_COH_2W);
>  }
>  
> -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);
> -}

IMO keep this as a wrapper that calls the function pointer.

> -
>  static int gt_ttm_mgr_init(struct xe_gt *gt)
>  {
>  	struct xe_device *xe = gt_to_xe(gt);
> @@ -380,6 +372,28 @@ 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);
> +
> +	xe->gt_funcs = kzalloc(sizeof(*xe->gt_funcs), GFP_KERNEL);
> +	switch (xe->info.platform) {
> +	case XE_METEORLAKE:
> +		xe->gt_funcs->setup_private_ppat = mtl_setup_private_ppat;
> +		break;
> +	case XE_PVC:
> +		xe->gt_funcs->setup_private_ppat = pvc_setup_private_ppat;
> +		break;
> +	case XE_ROCKETLAKE:
> +	case XE_DG1:
> +	case XE_DG2:
> +	case XE_ALDERLAKE_S:
> +	case XE_ALDERLAKE_P:
> +	case XE_TIGERLAKE:
> +		xe->gt_funcs->setup_private_ppat = tgl_setup_private_ppat;
> +		break;
> +	default:
> +		DRM_ERROR("Unsupported platform\n");
> +		break;
> +	}

IMO you should always aim to have static const initialized structs with
the function pointers, assign the const pointer to the struct, and avoid
assigning individual function pointers members.

It's a bad practise to allocate the function pointer struct.

>  
>  	xe_force_wake_init_gt(gt, gt_to_fw(gt));
>  
> @@ -441,13 +455,15 @@ int xe_gt_init_noalloc(struct xe_gt *gt)
>  static int gt_fw_domain_init(struct xe_gt *gt)
>  {
>  	int err, i;
> +	struct xe_device *xe = gt_to_xe(gt);
>  
>  	xe_device_mem_access_get(gt_to_xe(gt));
>  	err = xe_force_wake_get(gt_to_fw(gt), XE_FW_GT);
>  	if (err)
>  		goto err_hw_fence_irq;
>  
> -	setup_private_ppat(gt);
> +	if (xe->gt_funcs->setup_private_ppat)
> +		xe->gt_funcs->setup_private_ppat(gt);

IME it's better to keep functions like setup_private_ppat() as wrappers
to the function calls. The caller does not need to know the
implementation details i.e. whether it's a function pointer or not.

BR,
Jani.

>  
>  	if (!xe_gt_is_media_type(gt)) {
>  		err = xe_ggtt_init(gt, gt->mem.ggtt);
> @@ -630,8 +646,10 @@ static int do_gt_restart(struct xe_gt *gt)
>  	struct xe_hw_engine *hwe;
>  	enum xe_hw_engine_id id;
>  	int err;
> +	struct xe_device *xe = gt_to_xe(gt);
>  
> -	setup_private_ppat(gt);
> +	if (xe->gt_funcs->setup_private_ppat)
> +		xe->gt_funcs->setup_private_ppat(gt);
>  
>  	xe_gt_mcr_set_implicit_defaults(gt);
>  	xe_reg_sr_apply_mmio(&gt->reg_sr, gt);

-- 
Jani Nikula, Intel Open Source Graphics Center