Re: [Intel-gfx] [PATCH 2/2] drm/i915/bios: Use hardcoded fp_timing size for generating LFP data pointers

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

On Thu, 18 Aug 2022, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> The current scheme for generating the LFP data table pointers
> (when the block including them is missing from the VBT) expects
> the 0xffff sequence to only appear in the fp_timing terminator
> entries. However some VBTs also have extra 0xffff sequences
> elsewhere in the LFP data. When looking for the terminators
> we may end up finding those extra sequeneces insted, which means
> we deduce the wrong size for the fp_timing table. The code
> then notices the inconsistent looking values and gives up on
> the generated data table pointers, preventing us from parsing
> the LFP data table entirely.
>
> Let's give up on the "search for the terminators" approach
> and instead just hardcode the expected size for the fp_timing
> table.
>
> We have enough sanity checks in place to make sure we
> shouldn't end up parsing total garbage even if that size
> should change in the future (although that seems unlikely
> as the fp_timing and dvo_timing tables have been declared
> obsolete as of VBT version 229).
>
> Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/6592
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

What a mess.

Could debug log about missing data ptrs on vbt version < 155, but no
biggie.

Reviewed-by: Jani Nikula <jani.nikula@intel.com>


> ---
>  drivers/gpu/drm/i915/display/intel_bios.c | 46 +++++++++--------------
>  1 file changed, 18 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c
> index f1f861da9e93..f54a1843924e 100644
> --- a/drivers/gpu/drm/i915/display/intel_bios.c
> +++ b/drivers/gpu/drm/i915/display/intel_bios.c
> @@ -337,18 +337,6 @@ static bool fixup_lfp_data_ptrs(const void *bdb, void *ptrs_block)
>  	return validate_lfp_data_ptrs(bdb, ptrs);
>  }
>  
> -static const void *find_fp_timing_terminator(const u8 *data, int size)
> -{
> -	int i;
> -
> -	for (i = 0; i < size - 1; i++) {
> -		if (data[i] == 0xff && data[i+1] == 0xff)
> -			return &data[i];
> -	}
> -
> -	return NULL;
> -}
> -
>  static int make_lfp_data_ptr(struct lvds_lfp_data_ptr_table *table,
>  			     int table_size, int total_size)
>  {
> @@ -372,11 +360,22 @@ static void next_lfp_data_ptr(struct lvds_lfp_data_ptr_table *next,
>  static void *generate_lfp_data_ptrs(struct drm_i915_private *i915,
>  				    const void *bdb)
>  {
> -	int i, size, table_size, block_size, offset;
> -	const void *t0, *t1, *block;
> +	int i, size, table_size, block_size, offset, fp_timing_size;
>  	struct bdb_lvds_lfp_data_ptrs *ptrs;
> +	const void *block;
>  	void *ptrs_block;
>  
> +	/*
> +	 * The hardcoded fp_timing_size is only valid for
> +	 * modernish VBTs. All older VBTs definitely should
> +	 * include block 41 and thus we don't need to
> +	 * generate one.
> +	 */
> +	if (i915->vbt.version < 155)
> +		return NULL;
> +
> +	fp_timing_size = 38;
> +
>  	block = find_raw_section(bdb, BDB_LVDS_LFP_DATA);
>  	if (!block)
>  		return NULL;
> @@ -385,17 +384,8 @@ static void *generate_lfp_data_ptrs(struct drm_i915_private *i915,
>  
>  	block_size = get_blocksize(block);
>  
> -	size = block_size;
> -	t0 = find_fp_timing_terminator(block, size);
> -	if (!t0)
> -		return NULL;
> -
> -	size -= t0 - block - 2;
> -	t1 = find_fp_timing_terminator(t0 + 2, size);
> -	if (!t1)
> -		return NULL;
> -
> -	size = t1 - t0;
> +	size = fp_timing_size + sizeof(struct lvds_dvo_timing) +
> +		sizeof(struct lvds_pnp_id);
>  	if (size * 16 > block_size)
>  		return NULL;
>  
> @@ -413,7 +403,7 @@ static void *generate_lfp_data_ptrs(struct drm_i915_private *i915,
>  	table_size = sizeof(struct lvds_dvo_timing);
>  	size = make_lfp_data_ptr(&ptrs->ptr[0].dvo_timing, table_size, size);
>  
> -	table_size = t0 - block + 2;
> +	table_size = fp_timing_size;
>  	size = make_lfp_data_ptr(&ptrs->ptr[0].fp_timing, table_size, size);
>  
>  	if (ptrs->ptr[0].fp_timing.table_size)
> @@ -428,14 +418,14 @@ static void *generate_lfp_data_ptrs(struct drm_i915_private *i915,
>  		return NULL;
>  	}
>  
> -	size = t1 - t0;
> +	size = fp_timing_size + sizeof(struct lvds_dvo_timing) +
> +		sizeof(struct lvds_pnp_id);
>  	for (i = 1; i < 16; i++) {
>  		next_lfp_data_ptr(&ptrs->ptr[i].fp_timing, &ptrs->ptr[i-1].fp_timing, size);
>  		next_lfp_data_ptr(&ptrs->ptr[i].dvo_timing, &ptrs->ptr[i-1].dvo_timing, size);
>  		next_lfp_data_ptr(&ptrs->ptr[i].panel_pnp_id, &ptrs->ptr[i-1].panel_pnp_id, size);
>  	}
>  
> -	size = t1 - t0;
>  	table_size = sizeof(struct lvds_lfp_panel_name);
>  
>  	if (16 * (size + table_size) <= block_size) {

-- 
Jani Nikula, Intel Open Source Graphics Center