[PATCH] drm/i915: Piggyback opregion vbt to store vbt read from flash/oprom

Sripada, Radhakrishna radhakrishna.sripada at intel.com
Wed Dec 20 22:56:07 UTC 2023


Hi Jani,

> -----Original Message-----
> From: Jani Nikula <jani.nikula at linux.intel.com>
> Sent: Wednesday, December 20, 2023 1:38 AM
> To: Ville Syrjälä <ville.syrjala at linux.intel.com>; Sripada, Radhakrishna
> <radhakrishna.sripada at intel.com>
> Cc: intel-gfx at lists.freedesktop.org
> Subject: Re: [PATCH] drm/i915: Piggyback opregion vbt to store vbt read from
> flash/oprom
> 
> On Wed, 20 Dec 2023, Ville Syrjälä <ville.syrjala at linux.intel.com> wrote:
> > On Tue, Dec 19, 2023 at 05:49:52PM -0800, Radhakrishna Sripada wrote:
> >> Discrete cards do not have ACPI opregion. The vbt is stored in a special
> >> flash accessible by the display controller. In order to access the vbt
> >> in such cases, re-use the vbt, vbt_size fields in the opregion structure.
> >
> > Why?
> 
> The i915_vbt debugfs file probably does not work for VBT from SPI
> flash. We should fix that.
> 
> But this patch is not the way to go. If the VBT does not come from
> opregion, it shouldn't be stored in the opregion structures. Maybe this
> needs another abstraction layer. *grin*.
> 
> Specifically, one of the problems here is that the allocation and free
> of the VBT originating from SPI flash happens at completely different
> modules and abstraction layers. That's usually a recipe for disaster
> later.

I fully agree with you. I have thus made the statement in the original commit
that we would need to move away from storing vbt in opregion. I will try to
come up with a new patchset the stores vbt in the vbt structure itself and not in
the opregion.

Thanks,
Radhakrishan(RK) Sripada
> 
> 
> BR,
> Jani.
> 
> 
> >
> >>
> >> We should move away from storing the vbt in the opregion and store it,
> >> if required in the vbt structure.
> >>
> >> Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada at intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/display/intel_bios.c     | 44 ++++++++++---------
> >>  drivers/gpu/drm/i915/display/intel_opregion.c |  7 ++-
> >>  2 files changed, 29 insertions(+), 22 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/display/intel_bios.c
> b/drivers/gpu/drm/i915/display/intel_bios.c
> >> index 736499a6e2c7..cbfbc56ff5b2 100644
> >> --- a/drivers/gpu/drm/i915/display/intel_bios.c
> >> +++ b/drivers/gpu/drm/i915/display/intel_bios.c
> >> @@ -2930,12 +2930,11 @@ static u32 intel_spi_read(struct intel_uncore
> *uncore, u32 offset)
> >>  	return intel_uncore_read(uncore, PRIMARY_SPI_TRIGGER);
> >>  }
> >>
> >> -static struct vbt_header *spi_oprom_get_vbt(struct drm_i915_private *i915)
> >> +static struct vbt_header *spi_oprom_get_vbt(struct drm_i915_private *i915,
> u16 *vbt_size)
> >>  {
> >>  	u32 count, data, found, store = 0;
> >>  	u32 static_region, oprom_offset;
> >>  	u32 oprom_size = 0x200000;
> >> -	u16 vbt_size;
> >>  	u32 *vbt;
> >>
> >>  	static_region = intel_uncore_read(&i915->uncore, SPI_STATIC_REGIONS);
> >> @@ -2957,18 +2956,18 @@ static struct vbt_header
> *spi_oprom_get_vbt(struct drm_i915_private *i915)
> >>  		goto err_not_found;
> >>
> >>  	/* Get VBT size and allocate space for the VBT */
> >> -	vbt_size = intel_spi_read(&i915->uncore,
> >> +	*vbt_size = intel_spi_read(&i915->uncore,
> >>  				  found + offsetof(struct vbt_header, vbt_size));
> >> -	vbt_size &= 0xffff;
> >> +	*vbt_size &= 0xffff;
> >>
> >> -	vbt = kzalloc(round_up(vbt_size, 4), GFP_KERNEL);
> >> +	vbt = kzalloc(round_up(*vbt_size, 4), GFP_KERNEL);
> >>  	if (!vbt)
> >>  		goto err_not_found;
> >>
> >> -	for (count = 0; count < vbt_size; count += 4)
> >> +	for (count = 0; count < *vbt_size; count += 4)
> >>  		*(vbt + store++) = intel_spi_read(&i915->uncore, found + count);
> >>
> >> -	if (!intel_bios_is_valid_vbt(vbt, vbt_size))
> >> +	if (!intel_bios_is_valid_vbt(vbt, *vbt_size))
> >>  		goto err_free_vbt;
> >>
> >>  	drm_dbg_kms(&i915->drm, "Found valid VBT in SPI flash\n");
> >> @@ -2977,16 +2976,16 @@ static struct vbt_header
> *spi_oprom_get_vbt(struct drm_i915_private *i915)
> >>
> >>  err_free_vbt:
> >>  	kfree(vbt);
> >> +	*vbt_size = 0;
> >>  err_not_found:
> >>  	return NULL;
> >>  }
> >>
> >> -static struct vbt_header *oprom_get_vbt(struct drm_i915_private *i915)
> >> +static struct vbt_header *oprom_get_vbt(struct drm_i915_private *i915, u16
> *vbt_size)
> >>  {
> >>  	struct pci_dev *pdev = to_pci_dev(i915->drm.dev);
> >>  	void __iomem *p = NULL, *oprom;
> >>  	struct vbt_header *vbt;
> >> -	u16 vbt_size;
> >>  	size_t i, size;
> >>
> >>  	oprom = pci_map_rom(pdev, &size);
> >> @@ -3011,21 +3010,21 @@ static struct vbt_header *oprom_get_vbt(struct
> drm_i915_private *i915)
> >>  		goto err_unmap_oprom;
> >>  	}
> >>
> >> -	vbt_size = ioread16(p + offsetof(struct vbt_header, vbt_size));
> >> -	if (vbt_size > size) {
> >> +	*vbt_size = ioread16(p + offsetof(struct vbt_header, vbt_size));
> >> +	if (*vbt_size > size) {
> >>  		drm_dbg(&i915->drm,
> >> -			"VBT incomplete (vbt_size overflows)\n");
> >> +			"VBT incomplete (*vbt_size overflows)\n");
> >>  		goto err_unmap_oprom;
> >>  	}
> >>
> >>  	/* The rest will be validated by intel_bios_is_valid_vbt() */
> >> -	vbt = kmalloc(vbt_size, GFP_KERNEL);
> >> +	vbt = kmalloc(*vbt_size, GFP_KERNEL);
> >>  	if (!vbt)
> >>  		goto err_unmap_oprom;
> >>
> >> -	memcpy_fromio(vbt, p, vbt_size);
> >> +	memcpy_fromio(vbt, p, *vbt_size);
> >>
> >> -	if (!intel_bios_is_valid_vbt(vbt, vbt_size))
> >> +	if (!intel_bios_is_valid_vbt(vbt, *vbt_size))
> >>  		goto err_free_vbt;
> >>
> >>  	pci_unmap_rom(pdev, oprom);
> >> @@ -3036,6 +3035,7 @@ static struct vbt_header *oprom_get_vbt(struct
> drm_i915_private *i915)
> >>
> >>  err_free_vbt:
> >>  	kfree(vbt);
> >> +	*vbt_size = 0;
> >>  err_unmap_oprom:
> >>  	pci_unmap_rom(pdev, oprom);
> >>
> >> @@ -3052,8 +3052,10 @@ static struct vbt_header *oprom_get_vbt(struct
> drm_i915_private *i915)
> >>   */
> >>  void intel_bios_init(struct drm_i915_private *i915)
> >>  {
> >> +	struct intel_opregion *opregion = &i915->display.opregion;
> >>  	const struct vbt_header *vbt = i915->display.opregion.vbt;
> >>  	struct vbt_header *oprom_vbt = NULL;
> >> +	u16 vbt_size;
> >>  	const struct bdb_header *bdb;
> >>
> >>  	INIT_LIST_HEAD(&i915->display.vbt.display_devices);
> >> @@ -3072,13 +3074,15 @@ void intel_bios_init(struct drm_i915_private
> *i915)
> >>  	 * PCI mapping
> >>  	 */
> >>  	if (!vbt && IS_DGFX(i915)) {
> >> -		oprom_vbt = spi_oprom_get_vbt(i915);
> >> -		vbt = oprom_vbt;
> >> +		oprom_vbt = spi_oprom_get_vbt(i915, &vbt_size);
> >> +		opregion->vbt = vbt = oprom_vbt;
> >> +		opregion->vbt_size = (u32)vbt_size;
> >>  	}
> >>
> >>  	if (!vbt) {
> >> -		oprom_vbt = oprom_get_vbt(i915);
> >> -		vbt = oprom_vbt;
> >> +		oprom_vbt = oprom_get_vbt(i915, &vbt_size);
> >> +		opregion->vbt = vbt = oprom_vbt;
> >> +		opregion->vbt_size = (u32)vbt_size;
> >>  	}
> >>
> >>  	if (!vbt)
> >> @@ -3111,8 +3115,6 @@ void intel_bios_init(struct drm_i915_private *i915)
> >>  	/* Further processing on pre-parsed or generated child device data */
> >>  	parse_sdvo_device_mapping(i915);
> >>  	parse_ddi_ports(i915);
> >> -
> >> -	kfree(oprom_vbt);
> >>  }
> >>
> >>  static void intel_bios_init_panel(struct drm_i915_private *i915,
> >> diff --git a/drivers/gpu/drm/i915/display/intel_opregion.c
> b/drivers/gpu/drm/i915/display/intel_opregion.c
> >> index 1ce785db6a5e..20b2160e9d0e 100644
> >> --- a/drivers/gpu/drm/i915/display/intel_opregion.c
> >> +++ b/drivers/gpu/drm/i915/display/intel_opregion.c
> >> @@ -1244,8 +1244,13 @@ void intel_opregion_cleanup(struct
> drm_i915_private *i915)
> >>  {
> >>  	struct intel_opregion *opregion = &i915->display.opregion;
> >>
> >> -	if (!opregion->header)
> >> +	if (!opregion->header) {
> >> +		if (opregion->vbt) {
> >> +			kfree(opregion->vbt);
> >> +			opregion->vbt_size = 0;
> >> +		}
> >>  		return;
> >> +	}
> >>
> >>  	/* just clear all opregion memory pointers now */
> >>  	memunmap(opregion->header);
> >> --
> >> 2.34.1
> 
> --
> Jani Nikula, Intel


More information about the Intel-gfx mailing list