[RFC 00/15] VBT read Cleanup

[Sripada, Radhakrishna]

Hi Jani,

> -----Original Message-----
> From: Jani Nikula <jani.nikula at linux.intel.com>
> Sent: Wednesday, January 10, 2024 4:34 AM
> To: Sripada, Radhakrishna <radhakrishna.sripada at intel.com>; intel-
> gfx at lists.freedesktop.org
> Subject: Re: [RFC 00/15] VBT read Cleanup
> 
> On Mon, 08 Jan 2024, Radhakrishna Sripada <radhakrishna.sripada at intel.com>
> wrote:
> > This series does the VBT read cleanup. The series introduces new
> > intel_vbt structure to cache and collate vbt related info. Vbt
> > read from different sources viz. firmware/opregion/spi/oprom
> > needs to be cached for debug purposes and handled accordingly
> > during cleanup.
> 
> Mixed feelings. I think the goal is good, not convinced by the
> implementation.
> 
> First, i915->display.vbt.data.foo is just too much depth. It was
> borderline too much before, but now it definitely is.
Sure. I will cache and store the parsed fields under the same structure.

> 
> Second, whichever place allocates some stuff should also be responsible
> for freeing it. I don't like the idea that you have different places
> allocating and then you have a combined cleanup to take care of the
> alternatives.
Previously, allocations were happening under spi_get_oprom_vbt and
oprom_get_vbt but the de-allocation was happening immediately in
intel_bios_init. Because we need to cache vbt we will have to free the memory
only during driver remove phase.

I understand what you are suggesting here, the problem that I was thinking is
how to determine the sizes without extra reads, your series does it in a clean way.
 Will use that way to peek into the presence of vbt's in spi and oprom.

> 
> Possibly the first thing to do would be to put intel_bios_init() in
> charge of picking the VBT. Stop looking at opregion directly in
> intel_bios.c, and instead abstract that away. Also move firmware EDID
> loading there. Move debugfs there. Etc.
I think you meant firmware vbt here. I will rebase my work on top of your work
of opregion cleanup and come with a new series.

Thanks,
Radhakrishna(RK) Sripada
> 
> The opregion code could still figure out what its idea of VBT is, but
> intel_bios_init() would the place to ask opregion code about it only if
> needed.
> 
> 
> BR,
> Jani.
> 
> 
> 
> 
> 
> >
> > Radhakrishna Sripada (15):
> >   drm/i915: Extract display->vbt_data to a new vbt structure
> >   drm/i915: Move vbt fields from opregion to its own structure
> >   drm/i915: Cache opregion asls pointer
> >   drm/i915: Extract opregion vbt capture to its own function
> >   drm/i915: Init vbt fields when read from oprom/spi
> >   drm/i915: Classify vbt type based on its residence
> >   drm/i915: Collate vbt cleanup for different types
> >   drm/i915: Make intel_bios_init operate on intel_vbt
> >   drm/i915: Move vbt load from opregion to bios init
> >   drm/i915: Move vbt firmware load into intel_bios_init
> >   drm/i915: Make oprom_get_vbt operate on intel_vbt
> >   drm/i915: Make spi_oprom_get_vbt operate on intel_vbt
> >   drm/i915: Make intel_load_vbt_firmware operate on intel_vbt
> >   drm/i915: Kill reduntant vbt_firmware from intel_vbt
> >   drm/i915: Use vbt type to determine its validity
> >
> >  drivers/gpu/drm/i915/display/intel_bios.c     | 348 +++++++++++-------
> >  drivers/gpu/drm/i915/display/intel_crt.c      |   2 +-
> >  drivers/gpu/drm/i915/display/intel_display.c  |  10 +-
> >  .../gpu/drm/i915/display/intel_display_core.h |  16 +-
> >  .../drm/i915/display/intel_display_debugfs.c  |   6 +-
> >  drivers/gpu/drm/i915/display/intel_dp.c       |   2 +-
> >  drivers/gpu/drm/i915/display/intel_dpll.c     |  16 +-
> >  drivers/gpu/drm/i915/display/intel_dpll_mgr.c |  19 +-
> >  drivers/gpu/drm/i915/display/intel_dsi.c      |   2 +-
> >  drivers/gpu/drm/i915/display/intel_lvds.c     |   4 +-
> >  drivers/gpu/drm/i915/display/intel_opregion.c | 165 ++++-----
> >  drivers/gpu/drm/i915/display/intel_opregion.h |  13 +-
> >  drivers/gpu/drm/i915/display/intel_panel.c    |   2 +-
> >  .../gpu/drm/i915/display/intel_pch_refclk.c   |   2 +-
> >  drivers/gpu/drm/i915/display/intel_sdvo.c     |  18 +-
> >  drivers/gpu/drm/i915/intel_clock_gating.c     |   2 +-
> >  16 files changed, 348 insertions(+), 279 deletions(-)
> 
> --
> Jani Nikula, Intel