* [Intel-gfx] [RFC PATCH v3 0/1] Splitting up platform-specific calls @ 2022-02-15 23:41 Casey Bowman 2022-02-15 23:41 ` [Intel-gfx] [RFC PATCH v3 1/1] i915/drm: Split out x86/arm64 for run_as_guest Casey Bowman 2022-02-17 2:12 ` [Intel-gfx] ✗ Fi.CI.BUILD: failure for Splitting up platform-specific calls (rev3) Patchwork 0 siblings, 2 replies; 14+ messages in thread From: Casey Bowman @ 2022-02-15 23:41 UTC (permalink / raw) To: intel-gfx; +Cc: michael.cheng, jani.nikula, lucas.demarchi, daniel.vetter 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. v3: Revised to use simple if-else structure. Casey Bowman (1): i915/drm: Split out x86/arm64 for run_as_guest drivers/gpu/drm/i915/i915_drv.h | 8 ++++++++ 1 file changed, 8 insertions(+) -- 2.25.1 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [Intel-gfx] [RFC PATCH v3 1/1] i915/drm: Split out x86/arm64 for run_as_guest 2022-02-15 23:41 [Intel-gfx] [RFC PATCH v3 0/1] Splitting up platform-specific calls Casey Bowman @ 2022-02-15 23:41 ` Casey Bowman 2022-03-21 23:34 ` Casey Bowman 2022-02-17 2:12 ` [Intel-gfx] ✗ Fi.CI.BUILD: failure for Splitting up platform-specific calls (rev3) Patchwork 1 sibling, 1 reply; 14+ messages in thread From: Casey Bowman @ 2022-02-15 23:41 UTC (permalink / raw) To: intel-gfx; +Cc: michael.cheng, jani.nikula, lucas.demarchi, daniel.vetter Splitting out run_as_guest into platform-specific functions as arm64 does not support this functionality. Signed-off-by: Casey Bowman <casey.g.bowman@intel.com> --- drivers/gpu/drm/i915/i915_drv.h | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 1bca510a946d..fdec2b025540 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1381,10 +1381,18 @@ 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) +#if IS_ENABLED(CONFIG_X86) static inline bool run_as_guest(void) { return !hypervisor_is_type(X86_HYPER_NATIVE); } +#elif IS_ENABLED(CONFIG_ARM64) +static inline bool run_as_guest(void) +{ + /* Not supported for arm64, so we return false */ + return false; +} +#endif #define HAS_D12_PLANE_MINIMIZATION(dev_priv) (IS_ROCKETLAKE(dev_priv) || \ IS_ALDERLAKE_S(dev_priv)) -- 2.25.1 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [Intel-gfx] [RFC PATCH v3 1/1] i915/drm: Split out x86/arm64 for run_as_guest 2022-02-15 23:41 ` [Intel-gfx] [RFC PATCH v3 1/1] i915/drm: Split out x86/arm64 for run_as_guest Casey Bowman @ 2022-03-21 23:34 ` Casey Bowman 2022-03-22 2:01 ` Lucas De Marchi 0 siblings, 1 reply; 14+ messages in thread From: Casey Bowman @ 2022-03-21 23:34 UTC (permalink / raw) To: intel-gfx; +Cc: jani.nikula, lucas.demarchi, michael.cheng, daniel.vetter Wanted to ping this older thread to find out where we stand with this patch, Are we OK with the current state of these changes? With more recent information gathered from feedback on other patches, would we prefer changing this to a more arch-neutral control flow? e.g. #if IS_ENABLED(CONFIG_X86) ... #else ... #endif Would we also prefer this RFC series be merged or would it be preferred to create a new series instead? Regards, Casey On 2/15/22 15:41, Casey Bowman wrote: > Splitting out run_as_guest into platform-specific functions > as arm64 does not support this functionality. > > Signed-off-by: Casey Bowman <casey.g.bowman@intel.com> > --- > drivers/gpu/drm/i915/i915_drv.h | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 1bca510a946d..fdec2b025540 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -1381,10 +1381,18 @@ 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) > > +#if IS_ENABLED(CONFIG_X86) > static inline bool run_as_guest(void) > { > return !hypervisor_is_type(X86_HYPER_NATIVE); > } > +#elif IS_ENABLED(CONFIG_ARM64) > +static inline bool run_as_guest(void) > +{ > + /* Not supported for arm64, so we return false */ > + return false; > +} > +#endif > > #define HAS_D12_PLANE_MINIMIZATION(dev_priv) (IS_ROCKETLAKE(dev_priv) || \ > IS_ALDERLAKE_S(dev_priv)) ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Intel-gfx] [RFC PATCH v3 1/1] i915/drm: Split out x86/arm64 for run_as_guest 2022-03-21 23:34 ` Casey Bowman @ 2022-03-22 2:01 ` Lucas De Marchi 2022-03-22 10:21 ` Jani Nikula 0 siblings, 1 reply; 14+ messages in thread From: Lucas De Marchi @ 2022-03-22 2:01 UTC (permalink / raw) To: Casey Bowman; +Cc: jani.nikula, daniel.vetter, intel-gfx, michael.cheng On Mon, Mar 21, 2022 at 04:34:49PM -0700, Casey Bowman wrote: >Wanted to ping this older thread to find out where we stand with this patch, >Are we OK with the current state of these changes? > >With more recent information gathered from feedback on other patches, would >we prefer changing this to a more arch-neutral control flow? > >e.g. >#if IS_ENABLED(CONFIG_X86) >... >#else >... >#endif > >Would we also prefer this RFC series be merged or would it be preferred to >create a new series instead? for this specific function, that is used in only 2 places I think it's ok to do: static inline bool run_as_guest(void) { #if IS_ENABLED(CONFIG_X86) return !hypervisor_is_type(X86_HYPER_NATIVE); #else /* Not supported yet */ return false; #endif } For PCH it doesn't really matter as we don't execute that function for discrete. For intel_vtd_active() I figure anything other than x86 would be fine with false here. Jani, that this look good to you? Lucas De Marchi > >Regards, >Casey > >On 2/15/22 15:41, Casey Bowman wrote: >>Splitting out run_as_guest into platform-specific functions >>as arm64 does not support this functionality. >> >>Signed-off-by: Casey Bowman <casey.g.bowman@intel.com> >>--- >> drivers/gpu/drm/i915/i915_drv.h | 8 ++++++++ >> 1 file changed, 8 insertions(+) >> >>diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h >>index 1bca510a946d..fdec2b025540 100644 >>--- a/drivers/gpu/drm/i915/i915_drv.h >>+++ b/drivers/gpu/drm/i915/i915_drv.h >>@@ -1381,10 +1381,18 @@ 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) >>+#if IS_ENABLED(CONFIG_X86) >> static inline bool run_as_guest(void) >> { >> return !hypervisor_is_type(X86_HYPER_NATIVE); >> } >>+#elif IS_ENABLED(CONFIG_ARM64) >>+static inline bool run_as_guest(void) >>+{ >>+ /* Not supported for arm64, so we return false */ >>+ return false; >>+} >>+#endif >> #define HAS_D12_PLANE_MINIMIZATION(dev_priv) (IS_ROCKETLAKE(dev_priv) || \ >> IS_ALDERLAKE_S(dev_priv)) > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Intel-gfx] [RFC PATCH v3 1/1] i915/drm: Split out x86/arm64 for run_as_guest 2022-03-22 2:01 ` Lucas De Marchi @ 2022-03-22 10:21 ` Jani Nikula 2022-03-22 14:27 ` Lucas De Marchi 0 siblings, 1 reply; 14+ messages in thread From: Jani Nikula @ 2022-03-22 10:21 UTC (permalink / raw) To: Lucas De Marchi, Casey Bowman; +Cc: daniel.vetter, intel-gfx, michael.cheng On Mon, 21 Mar 2022, Lucas De Marchi <lucas.demarchi@intel.com> wrote: > On Mon, Mar 21, 2022 at 04:34:49PM -0700, Casey Bowman wrote: >>Wanted to ping this older thread to find out where we stand with this patch, >>Are we OK with the current state of these changes? >> >>With more recent information gathered from feedback on other patches, would >>we prefer changing this to a more arch-neutral control flow? >> >>e.g. >>#if IS_ENABLED(CONFIG_X86) >>... >>#else >>... >>#endif >> >>Would we also prefer this RFC series be merged or would it be preferred to >>create a new series instead? > > for this specific function, that is used in only 2 places I think it's > ok to do: > > static inline bool run_as_guest(void) > { > #if IS_ENABLED(CONFIG_X86) > return !hypervisor_is_type(X86_HYPER_NATIVE); > #else > /* Not supported yet */ > return false; > #endif > } > > For PCH it doesn't really matter as we don't execute that function > for discrete. For intel_vtd_active() I figure anything other than > x86 would be fine with false here. > > Jani, that this look good to you? It's more important to me to get this out of i915_drv.h, which is not supposed to be a collection of random stuff anymore. I've sent patches to this effect but they've stalled a bit. In general I like the preprocessor control flow outside of functions, i.e. completely separate function definitions, but for one-line function implementations I guess this is fine. This is less important for me than the first point. BR, Jani. > > Lucas De Marchi > >> >>Regards, >>Casey >> >>On 2/15/22 15:41, Casey Bowman wrote: >>>Splitting out run_as_guest into platform-specific functions >>>as arm64 does not support this functionality. >>> >>>Signed-off-by: Casey Bowman <casey.g.bowman@intel.com> >>>--- >>> drivers/gpu/drm/i915/i915_drv.h | 8 ++++++++ >>> 1 file changed, 8 insertions(+) >>> >>>diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h >>>index 1bca510a946d..fdec2b025540 100644 >>>--- a/drivers/gpu/drm/i915/i915_drv.h >>>+++ b/drivers/gpu/drm/i915/i915_drv.h >>>@@ -1381,10 +1381,18 @@ 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) >>>+#if IS_ENABLED(CONFIG_X86) >>> static inline bool run_as_guest(void) >>> { >>> return !hypervisor_is_type(X86_HYPER_NATIVE); >>> } >>>+#elif IS_ENABLED(CONFIG_ARM64) >>>+static inline bool run_as_guest(void) >>>+{ >>>+ /* Not supported for arm64, so we return false */ >>>+ return false; >>>+} >>>+#endif >>> #define HAS_D12_PLANE_MINIMIZATION(dev_priv) (IS_ROCKETLAKE(dev_priv) || \ >>> IS_ALDERLAKE_S(dev_priv)) >> -- Jani Nikula, Intel Open Source Graphics Center ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Intel-gfx] [RFC PATCH v3 1/1] i915/drm: Split out x86/arm64 for run_as_guest 2022-03-22 10:21 ` Jani Nikula @ 2022-03-22 14:27 ` Lucas De Marchi 2022-03-22 14:49 ` Jani Nikula 0 siblings, 1 reply; 14+ messages in thread From: Lucas De Marchi @ 2022-03-22 14:27 UTC (permalink / raw) To: Jani Nikula; +Cc: daniel.vetter, intel-gfx, michael.cheng On Tue, Mar 22, 2022 at 12:21:59PM +0200, Jani Nikula wrote: >On Mon, 21 Mar 2022, Lucas De Marchi <lucas.demarchi@intel.com> wrote: >> On Mon, Mar 21, 2022 at 04:34:49PM -0700, Casey Bowman wrote: >>>Wanted to ping this older thread to find out where we stand with this patch, >>>Are we OK with the current state of these changes? >>> >>>With more recent information gathered from feedback on other patches, would >>>we prefer changing this to a more arch-neutral control flow? >>> >>>e.g. >>>#if IS_ENABLED(CONFIG_X86) >>>... >>>#else >>>... >>>#endif >>> >>>Would we also prefer this RFC series be merged or would it be preferred to >>>create a new series instead? >> >> for this specific function, that is used in only 2 places I think it's >> ok to do: >> >> static inline bool run_as_guest(void) >> { >> #if IS_ENABLED(CONFIG_X86) >> return !hypervisor_is_type(X86_HYPER_NATIVE); >> #else >> /* Not supported yet */ >> return false; >> #endif >> } >> >> For PCH it doesn't really matter as we don't execute that function >> for discrete. For intel_vtd_active() I figure anything other than >> x86 would be fine with false here. >> >> Jani, that this look good to you? > >It's more important to me to get this out of i915_drv.h, which is not >supposed to be a collection of random stuff anymore. I've sent patches >to this effect but they've stalled a bit. do you have a patch moving this particular one? got a link? Lucas De Marchi ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Intel-gfx] [RFC PATCH v3 1/1] i915/drm: Split out x86/arm64 for run_as_guest 2022-03-22 14:27 ` Lucas De Marchi @ 2022-03-22 14:49 ` Jani Nikula 2022-03-22 15:18 ` Tvrtko Ursulin 2022-03-22 16:50 ` Lucas De Marchi 0 siblings, 2 replies; 14+ messages in thread From: Jani Nikula @ 2022-03-22 14:49 UTC (permalink / raw) To: Lucas De Marchi; +Cc: daniel.vetter, intel-gfx, michael.cheng On Tue, 22 Mar 2022, Lucas De Marchi <lucas.demarchi@intel.com> wrote: > On Tue, Mar 22, 2022 at 12:21:59PM +0200, Jani Nikula wrote: >>On Mon, 21 Mar 2022, Lucas De Marchi <lucas.demarchi@intel.com> wrote: >>> On Mon, Mar 21, 2022 at 04:34:49PM -0700, Casey Bowman wrote: >>>>Wanted to ping this older thread to find out where we stand with this patch, >>>>Are we OK with the current state of these changes? >>>> >>>>With more recent information gathered from feedback on other patches, would >>>>we prefer changing this to a more arch-neutral control flow? >>>> >>>>e.g. >>>>#if IS_ENABLED(CONFIG_X86) >>>>... >>>>#else >>>>... >>>>#endif >>>> >>>>Would we also prefer this RFC series be merged or would it be preferred to >>>>create a new series instead? >>> >>> for this specific function, that is used in only 2 places I think it's >>> ok to do: >>> >>> static inline bool run_as_guest(void) >>> { >>> #if IS_ENABLED(CONFIG_X86) >>> return !hypervisor_is_type(X86_HYPER_NATIVE); >>> #else >>> /* Not supported yet */ >>> return false; >>> #endif >>> } >>> >>> For PCH it doesn't really matter as we don't execute that function >>> for discrete. For intel_vtd_active() I figure anything other than >>> x86 would be fine with false here. >>> >>> Jani, that this look good to you? >> >>It's more important to me to get this out of i915_drv.h, which is not >>supposed to be a collection of random stuff anymore. I've sent patches >>to this effect but they've stalled a bit. > > do you have a patch moving this particular one? got a link? Yeah, but it was basically shot down by Tvrtko [1], and I stalled there. I'd just like to get all this cruft out of i915_drv.h. Whenever we have a file where the name isn't super specific, we seem to have a tendency of turning it into a dumping ground for random crap. So I'd really like to move this out of there *before* expanding on it. BR, Jani. [1] https://patchwork.freedesktop.org/series/99852/ -- Jani Nikula, Intel Open Source Graphics Center ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Intel-gfx] [RFC PATCH v3 1/1] i915/drm: Split out x86/arm64 for run_as_guest 2022-03-22 14:49 ` Jani Nikula @ 2022-03-22 15:18 ` Tvrtko Ursulin 2022-03-22 15:23 ` Tvrtko Ursulin 2022-03-22 15:26 ` Jani Nikula 2022-03-22 16:50 ` Lucas De Marchi 1 sibling, 2 replies; 14+ messages in thread From: Tvrtko Ursulin @ 2022-03-22 15:18 UTC (permalink / raw) To: Jani Nikula, Lucas De Marchi; +Cc: daniel.vetter, intel-gfx, michael.cheng On 22/03/2022 14:49, Jani Nikula wrote: > On Tue, 22 Mar 2022, Lucas De Marchi <lucas.demarchi@intel.com> wrote: >> On Tue, Mar 22, 2022 at 12:21:59PM +0200, Jani Nikula wrote: >>> On Mon, 21 Mar 2022, Lucas De Marchi <lucas.demarchi@intel.com> wrote: >>>> On Mon, Mar 21, 2022 at 04:34:49PM -0700, Casey Bowman wrote: >>>>> Wanted to ping this older thread to find out where we stand with this patch, >>>>> Are we OK with the current state of these changes? >>>>> >>>>> With more recent information gathered from feedback on other patches, would >>>>> we prefer changing this to a more arch-neutral control flow? >>>>> >>>>> e.g. >>>>> #if IS_ENABLED(CONFIG_X86) >>>>> ... >>>>> #else >>>>> ... >>>>> #endif >>>>> >>>>> Would we also prefer this RFC series be merged or would it be preferred to >>>>> create a new series instead? >>>> >>>> for this specific function, that is used in only 2 places I think it's >>>> ok to do: >>>> >>>> static inline bool run_as_guest(void) >>>> { >>>> #if IS_ENABLED(CONFIG_X86) >>>> return !hypervisor_is_type(X86_HYPER_NATIVE); >>>> #else >>>> /* Not supported yet */ >>>> return false; >>>> #endif >>>> } >>>> >>>> For PCH it doesn't really matter as we don't execute that function >>>> for discrete. For intel_vtd_active() I figure anything other than >>>> x86 would be fine with false here. >>>> >>>> Jani, that this look good to you? >>> >>> It's more important to me to get this out of i915_drv.h, which is not >>> supposed to be a collection of random stuff anymore. I've sent patches >>> to this effect but they've stalled a bit. >> >> do you have a patch moving this particular one? got a link? > > Yeah, but it was basically shot down by Tvrtko [1], and I stalled there. > > I'd just like to get all this cruft out of i915_drv.h. Whenever we have > a file where the name isn't super specific, we seem to have a tendency > of turning it into a dumping ground for random crap. So I'd really like > to move this out of there *before* expanding on it. Sounds like we had agreement on what tweaks to make and I conceded to live for now with the IMO wrongly named intel_vtd_run_as_guest. (I mean I really disagree with file name being trumps, which I think this example illustrates - this is i915 asking whether the kernel is running as guest so intel_vtd_ prefix is just wrong. Intel VT-d is the iommu thingy so it makes no sense when called from PCH detection. But I have no better ideas at the moment. We can call it i915_run_as_guest, to signify function belongs to i915, but then we lose the first parameter names the function rule.) But in any case I don't see that I created any blockers in this thread. AFAICS just a respin with intel_vtd_active taking struct device is needed and job done. Regards, Tvrtko ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Intel-gfx] [RFC PATCH v3 1/1] i915/drm: Split out x86/arm64 for run_as_guest 2022-03-22 15:18 ` Tvrtko Ursulin @ 2022-03-22 15:23 ` Tvrtko Ursulin 2022-03-22 15:26 ` Jani Nikula 1 sibling, 0 replies; 14+ messages in thread From: Tvrtko Ursulin @ 2022-03-22 15:23 UTC (permalink / raw) To: Jani Nikula, Lucas De Marchi; +Cc: daniel.vetter, intel-gfx, michael.cheng On 22/03/2022 15:18, Tvrtko Ursulin wrote: > > On 22/03/2022 14:49, Jani Nikula wrote: >> On Tue, 22 Mar 2022, Lucas De Marchi <lucas.demarchi@intel.com> wrote: >>> On Tue, Mar 22, 2022 at 12:21:59PM +0200, Jani Nikula wrote: >>>> On Mon, 21 Mar 2022, Lucas De Marchi <lucas.demarchi@intel.com> wrote: >>>>> On Mon, Mar 21, 2022 at 04:34:49PM -0700, Casey Bowman wrote: >>>>>> Wanted to ping this older thread to find out where we stand with >>>>>> this patch, >>>>>> Are we OK with the current state of these changes? >>>>>> >>>>>> With more recent information gathered from feedback on other >>>>>> patches, would >>>>>> we prefer changing this to a more arch-neutral control flow? >>>>>> >>>>>> e.g. >>>>>> #if IS_ENABLED(CONFIG_X86) >>>>>> ... >>>>>> #else >>>>>> ... >>>>>> #endif >>>>>> >>>>>> Would we also prefer this RFC series be merged or would it be >>>>>> preferred to >>>>>> create a new series instead? >>>>> >>>>> for this specific function, that is used in only 2 places I think it's >>>>> ok to do: >>>>> >>>>> static inline bool run_as_guest(void) >>>>> { >>>>> #if IS_ENABLED(CONFIG_X86) >>>>> return !hypervisor_is_type(X86_HYPER_NATIVE); >>>>> #else >>>>> /* Not supported yet */ >>>>> return false; >>>>> #endif >>>>> } >>>>> >>>>> For PCH it doesn't really matter as we don't execute that function >>>>> for discrete. For intel_vtd_active() I figure anything other than >>>>> x86 would be fine with false here. >>>>> >>>>> Jani, that this look good to you? >>>> >>>> It's more important to me to get this out of i915_drv.h, which is not >>>> supposed to be a collection of random stuff anymore. I've sent patches >>>> to this effect but they've stalled a bit. >>> >>> do you have a patch moving this particular one? got a link? >> >> Yeah, but it was basically shot down by Tvrtko [1], and I stalled there. >> >> I'd just like to get all this cruft out of i915_drv.h. Whenever we have >> a file where the name isn't super specific, we seem to have a tendency >> of turning it into a dumping ground for random crap. So I'd really like >> to move this out of there *before* expanding on it. > > Sounds like we had agreement on what tweaks to make and I conceded to > live for now with the IMO wrongly named intel_vtd_run_as_guest. > > (I mean I really disagree with file name being trumps, which I think > this example illustrates - this is i915 asking whether the kernel is > running as guest so intel_vtd_ prefix is just wrong. Intel VT-d is the > iommu thingy so it makes no sense when called from PCH detection. But I > have no better ideas at the moment. We can call it i915_run_as_guest, to > signify function belongs to i915, but then we lose the first parameter > names the function rule.) > > But in any case I don't see that I created any blockers in this thread. > AFAICS just a respin with intel_vtd_active taking struct device is > needed and job done. Sorry now I see I also suggested moving intel_scanout_needs_vtd_wa, intel_ggtt_update_needs_vtd_wa and intel_vm_no_concurrent_access_wa all to their respective files. Which I think is also correct. They are all higher components which are asking intel_vtd a question and basing a decision upon the answer. I don't think intel_vtd.h should contain knowledge about a mix of other driver components. Regards, Tvrtko ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Intel-gfx] [RFC PATCH v3 1/1] i915/drm: Split out x86/arm64 for run_as_guest 2022-03-22 15:18 ` Tvrtko Ursulin 2022-03-22 15:23 ` Tvrtko Ursulin @ 2022-03-22 15:26 ` Jani Nikula 2022-03-22 15:46 ` Tvrtko Ursulin 1 sibling, 1 reply; 14+ messages in thread From: Jani Nikula @ 2022-03-22 15:26 UTC (permalink / raw) To: Tvrtko Ursulin, Lucas De Marchi; +Cc: daniel.vetter, intel-gfx, michael.cheng On Tue, 22 Mar 2022, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote: > On 22/03/2022 14:49, Jani Nikula wrote: >> On Tue, 22 Mar 2022, Lucas De Marchi <lucas.demarchi@intel.com> wrote: >>> On Tue, Mar 22, 2022 at 12:21:59PM +0200, Jani Nikula wrote: >>>> On Mon, 21 Mar 2022, Lucas De Marchi <lucas.demarchi@intel.com> wrote: >>>>> On Mon, Mar 21, 2022 at 04:34:49PM -0700, Casey Bowman wrote: >>>>>> Wanted to ping this older thread to find out where we stand with this patch, >>>>>> Are we OK with the current state of these changes? >>>>>> >>>>>> With more recent information gathered from feedback on other patches, would >>>>>> we prefer changing this to a more arch-neutral control flow? >>>>>> >>>>>> e.g. >>>>>> #if IS_ENABLED(CONFIG_X86) >>>>>> ... >>>>>> #else >>>>>> ... >>>>>> #endif >>>>>> >>>>>> Would we also prefer this RFC series be merged or would it be preferred to >>>>>> create a new series instead? >>>>> >>>>> for this specific function, that is used in only 2 places I think it's >>>>> ok to do: >>>>> >>>>> static inline bool run_as_guest(void) >>>>> { >>>>> #if IS_ENABLED(CONFIG_X86) >>>>> return !hypervisor_is_type(X86_HYPER_NATIVE); >>>>> #else >>>>> /* Not supported yet */ >>>>> return false; >>>>> #endif >>>>> } >>>>> >>>>> For PCH it doesn't really matter as we don't execute that function >>>>> for discrete. For intel_vtd_active() I figure anything other than >>>>> x86 would be fine with false here. >>>>> >>>>> Jani, that this look good to you? >>>> >>>> It's more important to me to get this out of i915_drv.h, which is not >>>> supposed to be a collection of random stuff anymore. I've sent patches >>>> to this effect but they've stalled a bit. >>> >>> do you have a patch moving this particular one? got a link? >> >> Yeah, but it was basically shot down by Tvrtko [1], and I stalled there. >> >> I'd just like to get all this cruft out of i915_drv.h. Whenever we have >> a file where the name isn't super specific, we seem to have a tendency >> of turning it into a dumping ground for random crap. So I'd really like >> to move this out of there *before* expanding on it. > > Sounds like we had agreement on what tweaks to make and I conceded to > live for now with the IMO wrongly named intel_vtd_run_as_guest. > > (I mean I really disagree with file name being trumps, which I think > this example illustrates - this is i915 asking whether the kernel is > running as guest so intel_vtd_ prefix is just wrong. Intel VT-d is the > iommu thingy so it makes no sense when called from PCH detection. But I > have no better ideas at the moment. We can call it i915_run_as_guest, to > signify function belongs to i915, but then we lose the first parameter > names the function rule.) I think the "first parameter names the function" rule has backfired in gem/gt land, because it's pretty difficult to figure out *where* you'd expect to find or place functions. BR, Jani. > > But in any case I don't see that I created any blockers in this thread. > AFAICS just a respin with intel_vtd_active taking struct device is > needed and job done. > > Regards, > > Tvrtko -- Jani Nikula, Intel Open Source Graphics Center ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Intel-gfx] [RFC PATCH v3 1/1] i915/drm: Split out x86/arm64 for run_as_guest 2022-03-22 15:26 ` Jani Nikula @ 2022-03-22 15:46 ` Tvrtko Ursulin 2022-03-22 15:52 ` Jani Nikula 0 siblings, 1 reply; 14+ messages in thread From: Tvrtko Ursulin @ 2022-03-22 15:46 UTC (permalink / raw) To: Jani Nikula, Lucas De Marchi; +Cc: daniel.vetter, intel-gfx, michael.cheng On 22/03/2022 15:26, Jani Nikula wrote: > On Tue, 22 Mar 2022, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote: >> On 22/03/2022 14:49, Jani Nikula wrote: >>> On Tue, 22 Mar 2022, Lucas De Marchi <lucas.demarchi@intel.com> wrote: >>>> On Tue, Mar 22, 2022 at 12:21:59PM +0200, Jani Nikula wrote: >>>>> On Mon, 21 Mar 2022, Lucas De Marchi <lucas.demarchi@intel.com> wrote: >>>>>> On Mon, Mar 21, 2022 at 04:34:49PM -0700, Casey Bowman wrote: >>>>>>> Wanted to ping this older thread to find out where we stand with this patch, >>>>>>> Are we OK with the current state of these changes? >>>>>>> >>>>>>> With more recent information gathered from feedback on other patches, would >>>>>>> we prefer changing this to a more arch-neutral control flow? >>>>>>> >>>>>>> e.g. >>>>>>> #if IS_ENABLED(CONFIG_X86) >>>>>>> ... >>>>>>> #else >>>>>>> ... >>>>>>> #endif >>>>>>> >>>>>>> Would we also prefer this RFC series be merged or would it be preferred to >>>>>>> create a new series instead? >>>>>> >>>>>> for this specific function, that is used in only 2 places I think it's >>>>>> ok to do: >>>>>> >>>>>> static inline bool run_as_guest(void) >>>>>> { >>>>>> #if IS_ENABLED(CONFIG_X86) >>>>>> return !hypervisor_is_type(X86_HYPER_NATIVE); >>>>>> #else >>>>>> /* Not supported yet */ >>>>>> return false; >>>>>> #endif >>>>>> } >>>>>> >>>>>> For PCH it doesn't really matter as we don't execute that function >>>>>> for discrete. For intel_vtd_active() I figure anything other than >>>>>> x86 would be fine with false here. >>>>>> >>>>>> Jani, that this look good to you? >>>>> >>>>> It's more important to me to get this out of i915_drv.h, which is not >>>>> supposed to be a collection of random stuff anymore. I've sent patches >>>>> to this effect but they've stalled a bit. >>>> >>>> do you have a patch moving this particular one? got a link? >>> >>> Yeah, but it was basically shot down by Tvrtko [1], and I stalled there. >>> >>> I'd just like to get all this cruft out of i915_drv.h. Whenever we have >>> a file where the name isn't super specific, we seem to have a tendency >>> of turning it into a dumping ground for random crap. So I'd really like >>> to move this out of there *before* expanding on it. >> >> Sounds like we had agreement on what tweaks to make and I conceded to >> live for now with the IMO wrongly named intel_vtd_run_as_guest. >> >> (I mean I really disagree with file name being trumps, which I think >> this example illustrates - this is i915 asking whether the kernel is >> running as guest so intel_vtd_ prefix is just wrong. Intel VT-d is the >> iommu thingy so it makes no sense when called from PCH detection. But I >> have no better ideas at the moment. We can call it i915_run_as_guest, to >> signify function belongs to i915, but then we lose the first parameter >> names the function rule.) > > I think the "first parameter names the function" rule has backfired in > gem/gt land, because it's pretty difficult to figure out *where* you'd > expect to find or place functions. Hey surely is not that bad. And I am sure if I tried to add some display feature that there's a chance I'd manage to misplace something. :)) No scheme is perfect and there are always edge cases, ambiguities and always work to cleanup further because it all evolved rather than started from scratch. If you know what the function you wrote is about, surely you can place it into a file whose name suggests it is the right area. If you want the example of GT, there is a nice collection of files per functional area.. intel_gt_<suffix>.. interrupts, power management, requests, debugfs, etc. And if you look for functions you did not write, I certainly suggest using ctags rather than try opening random files. I think driver is just too big for the latter approach. Regards, Tvrtko > BR, > Jani. > >> >> But in any case I don't see that I created any blockers in this thread. >> AFAICS just a respin with intel_vtd_active taking struct device is >> needed and job done. >> >> Regards, >> >> Tvrtko > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Intel-gfx] [RFC PATCH v3 1/1] i915/drm: Split out x86/arm64 for run_as_guest 2022-03-22 15:46 ` Tvrtko Ursulin @ 2022-03-22 15:52 ` Jani Nikula 0 siblings, 0 replies; 14+ messages in thread From: Jani Nikula @ 2022-03-22 15:52 UTC (permalink / raw) To: Tvrtko Ursulin, Lucas De Marchi; +Cc: daniel.vetter, intel-gfx, michael.cheng On Tue, 22 Mar 2022, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote: > On 22/03/2022 15:26, Jani Nikula wrote: >> On Tue, 22 Mar 2022, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote: >>> On 22/03/2022 14:49, Jani Nikula wrote: >>>> On Tue, 22 Mar 2022, Lucas De Marchi <lucas.demarchi@intel.com> wrote: >>>>> On Tue, Mar 22, 2022 at 12:21:59PM +0200, Jani Nikula wrote: >>>>>> On Mon, 21 Mar 2022, Lucas De Marchi <lucas.demarchi@intel.com> wrote: >>>>>>> On Mon, Mar 21, 2022 at 04:34:49PM -0700, Casey Bowman wrote: >>>>>>>> Wanted to ping this older thread to find out where we stand with this patch, >>>>>>>> Are we OK with the current state of these changes? >>>>>>>> >>>>>>>> With more recent information gathered from feedback on other patches, would >>>>>>>> we prefer changing this to a more arch-neutral control flow? >>>>>>>> >>>>>>>> e.g. >>>>>>>> #if IS_ENABLED(CONFIG_X86) >>>>>>>> ... >>>>>>>> #else >>>>>>>> ... >>>>>>>> #endif >>>>>>>> >>>>>>>> Would we also prefer this RFC series be merged or would it be preferred to >>>>>>>> create a new series instead? >>>>>>> >>>>>>> for this specific function, that is used in only 2 places I think it's >>>>>>> ok to do: >>>>>>> >>>>>>> static inline bool run_as_guest(void) >>>>>>> { >>>>>>> #if IS_ENABLED(CONFIG_X86) >>>>>>> return !hypervisor_is_type(X86_HYPER_NATIVE); >>>>>>> #else >>>>>>> /* Not supported yet */ >>>>>>> return false; >>>>>>> #endif >>>>>>> } >>>>>>> >>>>>>> For PCH it doesn't really matter as we don't execute that function >>>>>>> for discrete. For intel_vtd_active() I figure anything other than >>>>>>> x86 would be fine with false here. >>>>>>> >>>>>>> Jani, that this look good to you? >>>>>> >>>>>> It's more important to me to get this out of i915_drv.h, which is not >>>>>> supposed to be a collection of random stuff anymore. I've sent patches >>>>>> to this effect but they've stalled a bit. >>>>> >>>>> do you have a patch moving this particular one? got a link? >>>> >>>> Yeah, but it was basically shot down by Tvrtko [1], and I stalled there. >>>> >>>> I'd just like to get all this cruft out of i915_drv.h. Whenever we have >>>> a file where the name isn't super specific, we seem to have a tendency >>>> of turning it into a dumping ground for random crap. So I'd really like >>>> to move this out of there *before* expanding on it. >>> >>> Sounds like we had agreement on what tweaks to make and I conceded to >>> live for now with the IMO wrongly named intel_vtd_run_as_guest. >>> >>> (I mean I really disagree with file name being trumps, which I think >>> this example illustrates - this is i915 asking whether the kernel is >>> running as guest so intel_vtd_ prefix is just wrong. Intel VT-d is the >>> iommu thingy so it makes no sense when called from PCH detection. But I >>> have no better ideas at the moment. We can call it i915_run_as_guest, to >>> signify function belongs to i915, but then we lose the first parameter >>> names the function rule.) >> >> I think the "first parameter names the function" rule has backfired in >> gem/gt land, because it's pretty difficult to figure out *where* you'd >> expect to find or place functions. > > Hey surely is not that bad. And I am sure if I tried to add some display > feature that there's a chance I'd manage to misplace something. :)) > > No scheme is perfect and there are always edge cases, ambiguities and > always work to cleanup further because it all evolved rather than > started from scratch. If you know what the function you wrote is about, > surely you can place it into a file whose name suggests it is the right > area. If you want the example of GT, there is a nice collection of files > per functional area.. intel_gt_<suffix>.. interrupts, power management, > requests, debugfs, etc. > > And if you look for functions you did not write, I certainly suggest > using ctags rather than try opening random files. I think driver is just > too big for the latter approach. Obviously I use code tagging and search. The point was more about the surprise in expecting to find a function in some place, and finding it in a totally different place. And obviously for finding the right place for new functions. BR, Jani. > > Regards, > > Tvrtko > > >> BR, >> Jani. >> >>> >>> But in any case I don't see that I created any blockers in this thread. >>> AFAICS just a respin with intel_vtd_active taking struct device is >>> needed and job done. >>> >>> Regards, >>> >>> Tvrtko >> -- Jani Nikula, Intel Open Source Graphics Center ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Intel-gfx] [RFC PATCH v3 1/1] i915/drm: Split out x86/arm64 for run_as_guest 2022-03-22 14:49 ` Jani Nikula 2022-03-22 15:18 ` Tvrtko Ursulin @ 2022-03-22 16:50 ` Lucas De Marchi 1 sibling, 0 replies; 14+ messages in thread From: Lucas De Marchi @ 2022-03-22 16:50 UTC (permalink / raw) To: Jani Nikula; +Cc: daniel.vetter, intel-gfx, michael.cheng On Tue, Mar 22, 2022 at 04:49:53PM +0200, Jani Nikula wrote: >On Tue, 22 Mar 2022, Lucas De Marchi <lucas.demarchi@intel.com> wrote: >> On Tue, Mar 22, 2022 at 12:21:59PM +0200, Jani Nikula wrote: >>>On Mon, 21 Mar 2022, Lucas De Marchi <lucas.demarchi@intel.com> wrote: >>>> On Mon, Mar 21, 2022 at 04:34:49PM -0700, Casey Bowman wrote: >>>>>Wanted to ping this older thread to find out where we stand with this patch, >>>>>Are we OK with the current state of these changes? >>>>> >>>>>With more recent information gathered from feedback on other patches, would >>>>>we prefer changing this to a more arch-neutral control flow? >>>>> >>>>>e.g. >>>>>#if IS_ENABLED(CONFIG_X86) >>>>>... >>>>>#else >>>>>... >>>>>#endif >>>>> >>>>>Would we also prefer this RFC series be merged or would it be preferred to >>>>>create a new series instead? >>>> >>>> for this specific function, that is used in only 2 places I think it's >>>> ok to do: >>>> >>>> static inline bool run_as_guest(void) >>>> { >>>> #if IS_ENABLED(CONFIG_X86) >>>> return !hypervisor_is_type(X86_HYPER_NATIVE); >>>> #else >>>> /* Not supported yet */ >>>> return false; >>>> #endif >>>> } >>>> >>>> For PCH it doesn't really matter as we don't execute that function >>>> for discrete. For intel_vtd_active() I figure anything other than >>>> x86 would be fine with false here. >>>> >>>> Jani, that this look good to you? >>> >>>It's more important to me to get this out of i915_drv.h, which is not >>>supposed to be a collection of random stuff anymore. I've sent patches >>>to this effect but they've stalled a bit. >> >> do you have a patch moving this particular one? got a link? > >Yeah, but it was basically shot down by Tvrtko [1], and I stalled there. > >I'd just like to get all this cruft out of i915_drv.h. Whenever we have >a file where the name isn't super specific, we seem to have a tendency >of turning it into a dumping ground for random crap. So I'd really like >to move this out of there *before* expanding on it. ok, I took a look there and it seems there is still some discussion on where to move it. The place you moved it to is not ideal as `run_as_guest()` has nothing to do with vt-d, even if it's used by one of those functions. It's also used by the PCH detection/fallback code. So, I think this is very much orthogonal to moving it and we are not really extending: this just doesn't make much sense for other archs. So my proposal is that we continue with this as is and move it when we have a rough agreement on where to move it to. As much as I hate the i915_utils.[ch] and I've been trying to reduce it, I can't think of a best place. Other than of course come up with an arch-neutral API to add to the kernel From some quick grep, include/linux/hypervisor.h might be a good place. But again, I think it should be orthogonal to what this is doing. thanks Lucas De Marchi > >BR, >Jani. > > >[1] https://patchwork.freedesktop.org/series/99852/ > > >-- >Jani Nikula, Intel Open Source Graphics Center ^ permalink raw reply [flat|nested] 14+ messages in thread
* [Intel-gfx] ✗ Fi.CI.BUILD: failure for Splitting up platform-specific calls (rev3) 2022-02-15 23:41 [Intel-gfx] [RFC PATCH v3 0/1] Splitting up platform-specific calls Casey Bowman 2022-02-15 23:41 ` [Intel-gfx] [RFC PATCH v3 1/1] i915/drm: Split out x86/arm64 for run_as_guest Casey Bowman @ 2022-02-17 2:12 ` Patchwork 1 sibling, 0 replies; 14+ messages in thread From: Patchwork @ 2022-02-17 2:12 UTC (permalink / raw) To: Casey Bowman; +Cc: intel-gfx == Series Details == Series: Splitting up platform-specific calls (rev3) URL : https://patchwork.freedesktop.org/series/99126/ State : failure == Summary == Applying: i915/drm: Split out x86/arm64 for run_as_guest Using index info to reconstruct a base tree... 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 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/arm64 for run_as_guest 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". ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2022-03-22 16:50 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-02-15 23:41 [Intel-gfx] [RFC PATCH v3 0/1] Splitting up platform-specific calls Casey Bowman 2022-02-15 23:41 ` [Intel-gfx] [RFC PATCH v3 1/1] i915/drm: Split out x86/arm64 for run_as_guest Casey Bowman 2022-03-21 23:34 ` Casey Bowman 2022-03-22 2:01 ` Lucas De Marchi 2022-03-22 10:21 ` Jani Nikula 2022-03-22 14:27 ` Lucas De Marchi 2022-03-22 14:49 ` Jani Nikula 2022-03-22 15:18 ` Tvrtko Ursulin 2022-03-22 15:23 ` Tvrtko Ursulin 2022-03-22 15:26 ` Jani Nikula 2022-03-22 15:46 ` Tvrtko Ursulin 2022-03-22 15:52 ` Jani Nikula 2022-03-22 16:50 ` Lucas De Marchi 2022-02-17 2:12 ` [Intel-gfx] ✗ Fi.CI.BUILD: failure for Splitting up platform-specific calls (rev3) Patchwork
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.