Re: [Intel-gfx] [PATCH 02/11] drm/i915/bios: Make copies of VBT data blocks

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

On Fri, 18 Mar 2022, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Thu, Mar 17, 2022 at 09:02:46PM +0200, Jani Nikula wrote:
>> On Thu, 17 Mar 2022, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
>> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> >
>> > Make a copy of each VB data block with a guaranteed minimum
>> > size. The extra (if any) will just be left zeroed.
>> 
>> *VBT
>> 
>> >
>> > This means we don't have to worry about going out of bounds
>> > when accessing any of the structure members. Otherwise that
>> > could easliy happen if we simply get the version check wrong,
>> > or if the VBT is broken/malicious.
>> 
>> *easily
>> 
>> >
>> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> 
>> The high level question is if we really want to save the copies until
>> driver remove instead of just during parsing. The lifetime should be
>> mentioned in the commit message, with rationale if you have some.
>
> Sure, I'll amend the commit msg a bit.
>
> I think we at least must move away from the "parse VBT once at
> driver init" idea because that won't ever let us deal with the
> panel_type=0xff->check PnP ID thing. I think I even have a few
> real world VBTs in my stash which have panel_type=0xff, so it's
> not just some theoretical thing.
>
> So we must hang onto the blocks at least until we've finished the
> output setup. But I'm thinking we can just keep them permanently
> and even start thinking about moving away from the "parse once,
> stash results somewhere" mentality. Ie. we could just parse on
> demand instead.

That pretty much aligns with what my direction has been with the child
devices. So gradually start throwing away stuff from i915->vbt, and
instead have some accessors on the opaque list of bdb block entries. I
guess it's going to be a lot of functions, but at least their names can
be self-documenting and they can handle the VBT version differences
cleanly.

>
>> I was wondering about making the copies up front instead of as needed,
>> but that means setting up a list for the min sizes. It would clean up
>> the usage (avoids passing around any pointers to original data to the
>> parsers). Then you could use just find_section(i915, BDB_XXX). Dunno.
>
> At least if we go for the parse on demand apporach then we definitely
> want to do that. Avoids having to deal with kmalloc() fails/etc. while
> parsing.

Agreed.


BR,
Jani.

-- 
Jani Nikula, Intel Open Source Graphics Center