Re: [PATCH v4] drm/i915: Show bios vbt when read from firmware/spi/oprom

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

On Thu, 29 Feb 2024, Radhakrishna Sripada <radhakrishna.sripada@intel.com> wrote:
> Make debugfs vbt only shows valid vbt when read from ACPI opregion.
> Make it work when read from firmware/spi/pci oprom cases.

The commit message (not just the changelog) should explain why the
runtime pm wakeref is needed.

I presume this only happens in spi_oprom_get_vbt() and/or
oprom_get_vbt() via debugfs, because we've not seen this before. So I'm
wondering about waking up the device for vbt debugfs for no reason in
the most common case, getting the vbt from opregion.

BR,
Jani.


>
> v2: Extract getting vbt from different sources to its own function.
>     Protect sysfs write with vbt check(Jani)
> v3: Fix CI error by probing bios vbt with runtime_pm wakeref
>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_bios.c | 62 ++++++++++++-----------
>  1 file changed, 33 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c
> index 44c9dfe86a00..3260aab63875 100644
> --- a/drivers/gpu/drm/i915/display/intel_bios.c
> +++ b/drivers/gpu/drm/i915/display/intel_bios.c
> @@ -3135,6 +3135,32 @@ static struct vbt_header *oprom_get_vbt(struct drm_i915_private *i915,
>  	return NULL;
>  }
>  
> +static const struct vbt_header *intel_bios_get_vbt(struct drm_i915_private *i915,
> +						   size_t *sizep)
> +{
> +	const struct vbt_header *vbt = NULL;
> +	intel_wakeref_t wakeref;
> +
> +	with_intel_runtime_pm(&i915->runtime_pm, wakeref) {
> +		vbt = firmware_get_vbt(i915, sizep);
> +
> +		if (!vbt)
> +			vbt = intel_opregion_get_vbt(i915, sizep);
> +
> +		/*
> +		 * If the OpRegion does not have VBT, look in SPI flash
> +		 * through MMIO or PCI mapping
> +		 */
> +		if (!vbt && IS_DGFX(i915))
> +			vbt = spi_oprom_get_vbt(i915, sizep);
> +
> +		if (!vbt)
> +			vbt = oprom_get_vbt(i915, sizep);
> +	}
> +
> +	return vbt;
> +}
> +
>  /**
>   * intel_bios_init - find VBT and initialize settings from the BIOS
>   * @i915: i915 device instance
> @@ -3146,7 +3172,6 @@ static struct vbt_header *oprom_get_vbt(struct drm_i915_private *i915,
>  void intel_bios_init(struct drm_i915_private *i915)
>  {
>  	const struct vbt_header *vbt;
> -	struct vbt_header *oprom_vbt = NULL;
>  	const struct bdb_header *bdb;
>  
>  	INIT_LIST_HEAD(&i915->display.vbt.display_devices);
> @@ -3160,27 +3185,7 @@ void intel_bios_init(struct drm_i915_private *i915)
>  
>  	init_vbt_defaults(i915);
>  
> -	oprom_vbt = firmware_get_vbt(i915, NULL);
> -	vbt = oprom_vbt;
> -
> -	if (!vbt) {
> -		oprom_vbt = intel_opregion_get_vbt(i915, NULL);
> -		vbt = oprom_vbt;
> -	}
> -
> -	/*
> -	 * If the OpRegion does not have VBT, look in SPI flash through MMIO or
> -	 * PCI mapping
> -	 */
> -	if (!vbt && IS_DGFX(i915)) {
> -		oprom_vbt = spi_oprom_get_vbt(i915, NULL);
> -		vbt = oprom_vbt;
> -	}
> -
> -	if (!vbt) {
> -		oprom_vbt = oprom_get_vbt(i915, NULL);
> -		vbt = oprom_vbt;
> -	}
> +	vbt = intel_bios_get_vbt(i915, NULL);
>  
>  	if (!vbt)
>  		goto out;
> @@ -3213,7 +3218,7 @@ void intel_bios_init(struct drm_i915_private *i915)
>  	parse_sdvo_device_mapping(i915);
>  	parse_ddi_ports(i915);
>  
> -	kfree(oprom_vbt);
> +	kfree(vbt);
>  }
>  
>  static void intel_bios_init_panel(struct drm_i915_private *i915,
> @@ -3743,13 +3748,12 @@ static int intel_bios_vbt_show(struct seq_file *m, void *unused)
>  	const void *vbt;
>  	size_t vbt_size;
>  
> -	/*
> -	 * FIXME: VBT might originate from other places than opregion, and then
> -	 * this would be incorrect.
> -	 */
> -	vbt = intel_opregion_get_vbt(i915, &vbt_size);
> -	if (vbt)
> +	vbt = intel_bios_get_vbt(i915, &vbt_size);
> +
> +	if (vbt) {
>  		seq_write(m, vbt, vbt_size);
> +		kfree(vbt);
> +	}
>  
>  	return 0;
>  }

-- 
Jani Nikula, Intel