[Intel-xe] [RFC PATCH 0/4] Split code between common and platform specific

[Lucas De Marchi]

On Fri, Mar 03, 2023 at 03:50:11PM +0100, Francois Dugast wrote:
>This patch series explores a code split between common code and
>platform specific code, which is moved to separate files. The
>minimal hardware abstraction layer is implemented with a structure
>of function pointers, similarly to what is done in accel/habanalabs
>and in drm/amd/amdgpu.
>
>The objective is to ease the addition of new platforms by limiting
>the need to modify the common code and centralizing specific code in
>a platform file, potentially in its platform directory.
>
>For now only one function has been refactored as an example
>(setup_private_ppat) but this shows already how platform specific
>code and platform switches can be moved out of xe_gt.c.

I skimmed through the patches and I still have mixed feelings about how
this is done. I think we will need at least 2 more places converted
in order to really have a more informed opinion on what it would like.

My main concerns:

1) The xe driver is split in several small parts xe_gt.c, xe_wa.c,
xe_pcode.c, etc and each compilation unit encapsulates as much as
possible from that specific area. The rest of the driver doesn't need to
know the inner details about how things are implemented. By having a
global platform_funcs set only once, you have to expose this.

2) Having the platform functions in different files means it's harder to
share common helpers that are specific to that area - you'd have to expose
them. In your case you have a xe_platform_tgl.h exposing
tgl_setup_private_ppat(struct xe_gt *gt) for platforms to reuse that
hook. But what about the case half the platforms need a common part +
a specific one.

3) For the ppat we are trading 3 localized, static functions, that are
only called on init / reset, for a bunch of functions spread in the
platform files. Couldn't we rather have a global xe->funcs like you
have, but delegate setting the hooks to the individual compilation
units? Also this is probably worth only for things called throughout the
driver rather than the localized functions we have here.

Lucas De Marchi