Re: [RFC 00/15] VBT read Cleanup

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

On Mon, 08 Jan 2024, Radhakrishna Sripada <radhakrishna.sripada@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.

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.

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.

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