[Intel-gfx] [RFC PATCH v2 0/1] Splitting up platform-specific calls

[Intel-gfx] [RFC PATCH v2 0/1] Splitting up platform-specific calls

[Casey Bowman]

In this RFC I would like to ask the community their thoughts
on how we can best handle splitting architecture-specific
calls.

I would like to address the following:

1. How do we want to split architecture calls? Different object files
per platform? Separate function calls within the same object file?

2. How do we address dummy functions? If we have a function call that is
used for one or more platforms, but is not used in another, what should
we do for this case?

I've given an example of splitting an architecture call
in my patch with run_as_guest() being split into different
implementations for x86 and arm64 in separate object files, sharing
a single header.

Another suggestion from Michael (michael.cheng@intel.com) involved
using a single object file, a single header, and splitting various
functions calls via ifdefs in the header file.

I would appreciate any input on how we can avoid scaling issues when
including multiple architectures and multiple functions (as the number
of function calls will inevitably increase with more architectures).

v2: Revised to use kernel's platform-splitting scheme.

Casey Bowman (1):
  i915/drm: Split out x86 and arm64 functionality

 drivers/gpu/drm/i915/Makefile                         |  3 +++
 drivers/gpu/drm/i915/i915_drv.h                       |  7 ++-----
 drivers/gpu/drm/i915/platforms/Makefile               |  8 ++++++++
 .../arm64/include/platform/i915_hypervisor.h          | 11 +++++++++++
 .../platforms/x86/include/platform/i915_hypervisor.h  |  9 +++++++++
 5 files changed, 33 insertions(+), 5 deletions(-)
 create mode 100644 drivers/gpu/drm/i915/platforms/Makefile
 create mode 100644 drivers/gpu/drm/i915/platforms/arm64/include/platform/i915_hypervisor.h
 create mode 100644 drivers/gpu/drm/i915/platforms/x86/include/platform/i915_hypervisor.h

-- 
2.25.1

[Intel-gfx] [RFC PATCH v2 1/1] i915/drm: Split out x86 and arm64 functionality

[Casey Bowman]

Some x86 checks are unnecessary on arm64 systems, so they
are being split out to avoid being used.

We are starting the split with run_as_guest(), which has
an x86-specific usage, while on arm64, we aren't using it.

The split reflects the way the kernel currently handles
platform-specific libraries, but these calls are localized
to i915.

Signed-off-by: Casey Bowman <casey.g.bowman@intel.com>
---
 drivers/gpu/drm/i915/Makefile                         |  3 +++
 drivers/gpu/drm/i915/i915_drv.h                       |  7 ++-----
 drivers/gpu/drm/i915/platforms/Makefile               |  8 ++++++++
 .../arm64/include/platform/i915_hypervisor.h          | 11 +++++++++++
 .../platforms/x86/include/platform/i915_hypervisor.h  |  9 +++++++++
 5 files changed, 33 insertions(+), 5 deletions(-)
 create mode 100644 drivers/gpu/drm/i915/platforms/Makefile
 create mode 100644 drivers/gpu/drm/i915/platforms/arm64/include/platform/i915_hypervisor.h
 create mode 100644 drivers/gpu/drm/i915/platforms/x86/include/platform/i915_hypervisor.h

diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index 16cab8db012a..f69ed0c10e80 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -29,6 +29,9 @@ subdir-ccflags-y += -I$(srctree)/$(src)
 
 # Please keep these build lists sorted!
 
+# arch support
+include $(src)/platforms/Makefile
+
 # core driver code
 i915-y += i915_driver.o \
 	  i915_config.o \
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 78fa3340101b..ba2cfb4d9830 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -87,6 +87,8 @@
 #include "gt/intel_workarounds.h"
 #include "gt/uc/intel_uc.h"
 
+#include "platform/i915_hypervisor.h"
+
 #include "intel_device_info.h"
 #include "intel_memory_region.h"
 #include "intel_pch.h"
@@ -1498,11 +1500,6 @@ IS_SUBPLATFORM(const struct drm_i915_private *i915,
 #define INTEL_DISPLAY_ENABLED(dev_priv) \
 	(drm_WARN_ON(&(dev_priv)->drm, !HAS_DISPLAY(dev_priv)), !(dev_priv)->params.disable_display)
 
-static inline bool run_as_guest(void)
-{
-	return !hypervisor_is_type(X86_HYPER_NATIVE);
-}
-
 #define HAS_D12_PLANE_MINIMIZATION(dev_priv) (IS_ROCKETLAKE(dev_priv) || \
 					      IS_ALDERLAKE_S(dev_priv))
 
diff --git a/drivers/gpu/drm/i915/platforms/Makefile b/drivers/gpu/drm/i915/platforms/Makefile
new file mode 100644
index 000000000000..03250b5704ea
--- /dev/null
+++ b/drivers/gpu/drm/i915/platforms/Makefile
@@ -0,0 +1,8 @@
+ifdef CONFIG_X86
+ccflags-y += -I $(srctree)/$(src)/platforms/x86/include/
+endif
+
+ifdef CONFIG_ARM64
+ccflags-y += -I $(srctree)/$(src)/platforms/arm64/include/
+endif
+
diff --git a/drivers/gpu/drm/i915/platforms/arm64/include/platform/i915_hypervisor.h b/drivers/gpu/drm/i915/platforms/arm64/include/platform/i915_hypervisor.h
new file mode 100644
index 000000000000..cd9050711dce
--- /dev/null
+++ b/drivers/gpu/drm/i915/platforms/arm64/include/platform/i915_hypervisor.h
@@ -0,0 +1,11 @@
+#ifndef _I915_HYPERVISOR_
+#define _I915_HYPERVISOR_
+
+
+/* Not supported for arm64, so we return  */
+static inline bool run_as_guest(void)
+{
+	return false;
+}
+
+#endif
diff --git a/drivers/gpu/drm/i915/platforms/x86/include/platform/i915_hypervisor.h b/drivers/gpu/drm/i915/platforms/x86/include/platform/i915_hypervisor.h
new file mode 100644
index 000000000000..5eab54f5e09f
--- /dev/null
+++ b/drivers/gpu/drm/i915/platforms/x86/include/platform/i915_hypervisor.h
@@ -0,0 +1,9 @@
+#ifndef _I915_HYPERVISOR_
+#define _I915_HYPERVISOR_
+
+static inline bool run_as_guest(void)
+{
+	return !hypervisor_is_type(X86_HYPER_NATIVE);
+}
+
+#endif
-- 
2.25.1

[Intel-gfx] ✗ Fi.CI.BUILD: failure for Splitting up platform-specific calls (rev2)

[Patchwork]

== Series Details ==

Series: Splitting up platform-specific calls (rev2)
URL   : https://patchwork.freedesktop.org/series/99126/
State : failure

== Summary ==

Applying: i915/drm: Split out x86 and arm64 functionality
.git/rebase-apply/patch:64: new blank line at EOF.
+
warning: 1 line adds whitespace errors.
Using index info to reconstruct a base tree...
M	drivers/gpu/drm/i915/Makefile
M	drivers/gpu/drm/i915/i915_drv.h
Falling back to patching base and 3-way merge...
Auto-merging drivers/gpu/drm/i915/i915_drv.h
CONFLICT (content): Merge conflict in drivers/gpu/drm/i915/i915_drv.h
Auto-merging drivers/gpu/drm/i915/Makefile
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 i915/drm: Split out x86 and arm64 functionality
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

Re: [Intel-gfx] [RFC PATCH v2 0/1] Splitting up platform-specific calls

[Jani Nikula]

On Thu, 10 Feb 2022, Casey Bowman <casey.g.bowman@intel.com> wrote:
> In this RFC I would like to ask the community their thoughts
> on how we can best handle splitting architecture-specific
> calls.
>
> I would like to address the following:
>
> 1. How do we want to split architecture calls? Different object files
> per platform? Separate function calls within the same object file?
>
> 2. How do we address dummy functions? If we have a function call that is
> used for one or more platforms, but is not used in another, what should
> we do for this case?
>
> I've given an example of splitting an architecture call
> in my patch with run_as_guest() being split into different
> implementations for x86 and arm64 in separate object files, sharing
> a single header.
>
> Another suggestion from Michael (michael.cheng@intel.com) involved
> using a single object file, a single header, and splitting various
> functions calls via ifdefs in the header file.
>
> I would appreciate any input on how we can avoid scaling issues when
> including multiple architectures and multiple functions (as the number
> of function calls will inevitably increase with more architectures).
>
> v2: Revised to use kernel's platform-splitting scheme.

I think this is overengineering.

Just add different implementations of the functions per architecture
next to where they are now, like I suggested before.

If we need to split them better later, it'll be a trivial undertaking,
and we'll be in a better position to do it because we'll know how many
functions there'll be and where they are and what they do.

Adding a bunch of overhead from the start seems like the wrong thing to
do.

BR,
Jani.





>
> Casey Bowman (1):
>   i915/drm: Split out x86 and arm64 functionality
>
>  drivers/gpu/drm/i915/Makefile                         |  3 +++
>  drivers/gpu/drm/i915/i915_drv.h                       |  7 ++-----
>  drivers/gpu/drm/i915/platforms/Makefile               |  8 ++++++++
>  .../arm64/include/platform/i915_hypervisor.h          | 11 +++++++++++
>  .../platforms/x86/include/platform/i915_hypervisor.h  |  9 +++++++++
>  5 files changed, 33 insertions(+), 5 deletions(-)
>  create mode 100644 drivers/gpu/drm/i915/platforms/Makefile
>  create mode 100644 drivers/gpu/drm/i915/platforms/arm64/include/platform/i915_hypervisor.h
>  create mode 100644 drivers/gpu/drm/i915/platforms/x86/include/platform/i915_hypervisor.h

-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [Intel-gfx] [RFC PATCH v2 0/1] Splitting up platform-specific calls

[Tvrtko Ursulin]

On 11/02/2022 11:55, Jani Nikula wrote:
> On Thu, 10 Feb 2022, Casey Bowman <casey.g.bowman@intel.com> wrote:
>> In this RFC I would like to ask the community their thoughts
>> on how we can best handle splitting architecture-specific
>> calls.
>>
>> I would like to address the following:
>>
>> 1. How do we want to split architecture calls? Different object files
>> per platform? Separate function calls within the same object file?
>>
>> 2. How do we address dummy functions? If we have a function call that is
>> used for one or more platforms, but is not used in another, what should
>> we do for this case?
>>
>> I've given an example of splitting an architecture call
>> in my patch with run_as_guest() being split into different
>> implementations for x86 and arm64 in separate object files, sharing
>> a single header.
>>
>> Another suggestion from Michael (michael.cheng@intel.com) involved
>> using a single object file, a single header, and splitting various
>> functions calls via ifdefs in the header file.
>>
>> I would appreciate any input on how we can avoid scaling issues when
>> including multiple architectures and multiple functions (as the number
>> of function calls will inevitably increase with more architectures).
>>
>> v2: Revised to use kernel's platform-splitting scheme.
> 
> I think this is overengineering.
> 
> Just add different implementations of the functions per architecture
> next to where they are now, like I suggested before.
> 
> If we need to split them better later, it'll be a trivial undertaking,
> and we'll be in a better position to do it because we'll know how many
> functions there'll be and where they are and what they do.
> 
> Adding a bunch of overhead from the start seems like the wrong thing to
> do.

I don't see it adds real complexity, which would normally be associated 
with over-engineering. As a benefit I see it helping with driving the 
clean re-design (during the porting effort) in a way that it will be 
easy to spot is something is overly hacky, split on the wrong level, or 
incorrectly placed.

And it moves run_as_guest outside of intel_vtd.[hc] which IMO shows 
immediate benefit, since it has nothing to do with intel_vtd.

I suggested to add clflush as well, since I think going for 
drm_flush_virt_range everywhere is a bit lazy given how it is a clear 
regression for older platforms.

But after that I indeed don't have a crystal ball to show me how many 
more appropriate low-level primitives would be to use the pattern.

So my vote would be to go with it, although the main thing is probably 
to solve the conflicting asks and let guys focus on the port. Put it to 
voting then? :)

Regards,

Tvrtko

Re: [Intel-gfx] [RFC PATCH v2 0/1] Splitting up platform-specific calls

[Casey Bowman]

On 2/11/22 05:51, Tvrtko Ursulin wrote:
>
> On 11/02/2022 11:55, Jani Nikula wrote:
>> On Thu, 10 Feb 2022, Casey Bowman <casey.g.bowman@intel.com> wrote:
>>> In this RFC I would like to ask the community their thoughts
>>> on how we can best handle splitting architecture-specific
>>> calls.
>>>
>>> I would like to address the following:
>>>
>>> 1. How do we want to split architecture calls? Different object files
>>> per platform? Separate function calls within the same object file?
>>>
>>> 2. How do we address dummy functions? If we have a function call 
>>> that is
>>> used for one or more platforms, but is not used in another, what should
>>> we do for this case?
>>>
>>> I've given an example of splitting an architecture call
>>> in my patch with run_as_guest() being split into different
>>> implementations for x86 and arm64 in separate object files, sharing
>>> a single header.
>>>
>>> Another suggestion from Michael (michael.cheng@intel.com) involved
>>> using a single object file, a single header, and splitting various
>>> functions calls via ifdefs in the header file.
>>>
>>> I would appreciate any input on how we can avoid scaling issues when
>>> including multiple architectures and multiple functions (as the number
>>> of function calls will inevitably increase with more architectures).
>>>
>>> v2: Revised to use kernel's platform-splitting scheme.
>>
>> I think this is overengineering.
>>
>> Just add different implementations of the functions per architecture
>> next to where they are now, like I suggested before.
>>
>> If we need to split them better later, it'll be a trivial undertaking,
>> and we'll be in a better position to do it because we'll know how many
>> functions there'll be and where they are and what they do.
>>
>> Adding a bunch of overhead from the start seems like the wrong thing to
>> do.
>
> I don't see it adds real complexity, which would normally be 
> associated with over-engineering. As a benefit I see it helping with 
> driving the clean re-design (during the porting effort) in a way that 
> it will be easy to spot is something is overly hacky, split on the 
> wrong level, or incorrectly placed.
>
> And it moves run_as_guest outside of intel_vtd.[hc] which IMO shows 
> immediate benefit, since it has nothing to do with intel_vtd.
>
> I suggested to add clflush as well, since I think going for 
> drm_flush_virt_range everywhere is a bit lazy given how it is a clear 
> regression for older platforms.
>
> But after that I indeed don't have a crystal ball to show me how many 
> more appropriate low-level primitives would be to use the pattern.
>
> So my vote would be to go with it, although the main thing is probably 
> to solve the conflicting asks and let guys focus on the port. Put it 
> to voting then? :)
>
If we can get someone else to weigh in here to break the tie, that'd be 
helpful :)

Regards,
Casey

Re: [Intel-gfx] [RFC PATCH v2 0/1] Splitting up platform-specific calls

[Lucas De Marchi]

On Mon, Feb 14, 2022 at 10:05:56PM -0800, Casey Bowman wrote:
>
>On 2/11/22 05:51, Tvrtko Ursulin wrote:
>>
>>On 11/02/2022 11:55, Jani Nikula wrote:
>>>On Thu, 10 Feb 2022, Casey Bowman <casey.g.bowman@intel.com> wrote:
>>>>In this RFC I would like to ask the community their thoughts
>>>>on how we can best handle splitting architecture-specific
>>>>calls.
>>>>
>>>>I would like to address the following:
>>>>
>>>>1. How do we want to split architecture calls? Different object files
>>>>per platform? Separate function calls within the same object file?
>>>>
>>>>2. How do we address dummy functions? If we have a function call 
>>>>that is
>>>>used for one or more platforms, but is not used in another, what should
>>>>we do for this case?
>>>>
>>>>I've given an example of splitting an architecture call
>>>>in my patch with run_as_guest() being split into different
>>>>implementations for x86 and arm64 in separate object files, sharing
>>>>a single header.
>>>>
>>>>Another suggestion from Michael (michael.cheng@intel.com) involved
>>>>using a single object file, a single header, and splitting various
>>>>functions calls via ifdefs in the header file.
>>>>
>>>>I would appreciate any input on how we can avoid scaling issues when
>>>>including multiple architectures and multiple functions (as the number
>>>>of function calls will inevitably increase with more architectures).
>>>>
>>>>v2: Revised to use kernel's platform-splitting scheme.
>>>
>>>I think this is overengineering.
>>>
>>>Just add different implementations of the functions per architecture
>>>next to where they are now, like I suggested before.
>>>
>>>If we need to split them better later, it'll be a trivial undertaking,
>>>and we'll be in a better position to do it because we'll know how many
>>>functions there'll be and where they are and what they do.
>>>
>>>Adding a bunch of overhead from the start seems like the wrong thing to
>>>do.
>>
>>I don't see it adds real complexity, which would normally be 
>>associated with over-engineering. As a benefit I see it helping with 
>>driving the clean re-design (during the porting effort) in a way 
>>that it will be easy to spot is something is overly hacky, split on 
>>the wrong level, or incorrectly placed.
>>
>>And it moves run_as_guest outside of intel_vtd.[hc] which IMO shows 
>>immediate benefit, since it has nothing to do with intel_vtd.
>>
>>I suggested to add clflush as well, since I think going for 
>>drm_flush_virt_range everywhere is a bit lazy given how it is a 
>>clear regression for older platforms.
>>
>>But after that I indeed don't have a crystal ball to show me how 
>>many more appropriate low-level primitives would be to use the 
>>pattern.
>>
>>So my vote would be to go with it, although the main thing is 
>>probably to solve the conflicting asks and let guys focus on the 
>>port. Put it to voting then? :)
>>
>If we can get someone else to weigh in here to break the tie, that'd 
>be helpful :)

I don't like much the split with platforms because a) I don't think we
have too many users to deserve that, b) if we do have something that is
common and should be abstracted in that way, it should probably be
outside of i915: find somewhere in the kernel that is the proper place
to add that and c) usually we will have "do one thing for x86, do
another for all the rest" - and the split per platform forces us to add
an implementation for each platform (or add a generic/ to account for
the absence of something). There will usually be one (x86) that will
be very different than the rest.

So my vote is to go with Jani's proposal.

Lucas De Marchi

>
>Regards,
>Casey
>