Hi Jani,
> -----Original Message-----
> From: Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx>
> Sent: Wednesday, December 20, 2023 1:38 AM
> To: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>; Sripada, Radhakrishna
> <radhakrishna.sripada@xxxxxxxxx>
> Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> 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@xxxxxxxxxxxxxxx> 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@xxxxxxxxx>
> >> ---
> >> 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
