Re: [Intel-gfx] [RFC PATCH v3 1/1] i915/drm: Split out x86/arm64 for run_as_guest

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

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