Re: [PATCH 3/6] drm/i915/display: add intel_display_run_as_guest()

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

On Thu, 30 Oct 2025, Luca Coelho <luca@coelho.fi> wrote:
> On Wed, 2025-10-22 at 18:17 +0300, Jani Nikula wrote:
>> Add intel_display_utils.c for display utilities that need more than a
>> header.
>> 
>> Start off with intel_display_run_as_guest(). The implementation is
>> intentional duplication of the i915_utils.h i915_run_as_guest(), with
>> the idea that it's small enough to not matter.
>> 
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> ---
>
>
>
>>  drivers/gpu/drm/i915/Makefile                  |  1 +
>>  .../gpu/drm/i915/display/intel_display_utils.c | 18 ++++++++++++++++++
>>  .../gpu/drm/i915/display/intel_display_utils.h |  6 ++++++
>>  drivers/gpu/drm/i915/display/intel_pch.c       |  4 ++--
>>  drivers/gpu/drm/xe/Makefile                    |  1 +
>>  5 files changed, 28 insertions(+), 2 deletions(-)
>>  create mode 100644 drivers/gpu/drm/i915/display/intel_display_utils.c
>> 
>> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
>> index 47bac9b2c611..046f9282fb65 100644
>> --- a/drivers/gpu/drm/i915/Makefile
>> +++ b/drivers/gpu/drm/i915/Makefile
>> @@ -255,6 +255,7 @@ i915-y += \
>>  	display/intel_display_rpm.o \
>>  	display/intel_display_rps.o \
>>  	display/intel_display_snapshot.o \
>> +	display/intel_display_utils.o \
>>  	display/intel_display_wa.o \
>>  	display/intel_dmc.o \
>>  	display/intel_dmc_wl.o \
>> diff --git a/drivers/gpu/drm/i915/display/intel_display_utils.c b/drivers/gpu/drm/i915/display/intel_display_utils.c
>> new file mode 100644
>> index 000000000000..13d3999dd580
>> --- /dev/null
>> +++ b/drivers/gpu/drm/i915/display/intel_display_utils.c
>> @@ -0,0 +1,18 @@
>> +// SPDX-License-Identifier: MIT
>> +/* Copyright © 2025 Intel Corporation */
>> +
>> +#ifdef CONFIG_X86
>> +#include <asm/hypervisor.h>
>> +#endif
>> +
>> +#include "intel_display_utils.h"
>> +
>> +bool intel_display_run_as_guest(struct intel_display *display)
>> +{
>> +#if IS_ENABLED(CONFIG_X86)
>> +	return !hypervisor_is_type(X86_HYPER_NATIVE);
>> +#else
>> +	/* Not supported yet */
>> +	return false;
>> +#endif
>> +}
>
> Why can't this be an inline in the header file?

I'll turn it around. I think there needs to be a rationale for inlining,
not the other way around. A regular function should be the default.

I think the primary reason for inlining would be performance, but I'll
accept small "superfluous" static inlines that don't require pulling in
other headers.

I don't think either is true here.

Additionally the static inline exposes all of that ifdef mess and the
implementation details in the header too. A change in asm/hypervisor.h
leads to a rebuild of everything that includes intel_display_utils.h,
making the header dependencies worse. (Maybe a change in
asm/hypervisor.h leads to a rebuild of everything anyway, but you get
the general point.)


BR,
Jani.


-- 
Jani Nikula, Intel