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

[Francois Dugast]

Hi,

First of all, thank you for the feedback and sorry for the delay.

On Tue, May 09, 2023 at 08:16:10AM +0200, Ohad Sharabi wrote:
> 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)?

This particular code is now obsolete anyway as the PAT code has been refactored.

> 
> 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.

Yes, this was demonstrated in the first revision of this series. A per-platform FP was introduced and initialized at the beginning of xe_device_probe. Please check it here: https://patchwork.freedesktop.org/patch/524916/?series=114630&rev=1 In the current revision however, the idea is to explore a per-compilation unit FP which could be a better fit for the current code.

> 
> 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)?

A reason for the per-compilation unit FP approach is to avoid exposing details of the internal implementation outside of the compilation unit. So while this proposal sounds doable technically, we probably need to refactor the code to use more generic lifecycle functions such as X_init(), X_fini() that would apply to multiple compilation units, in order to avoid multiplication of functions in the FP.

Francois

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