Re: [Intel-gfx] [PATCH v3 09/22] drm/i915/bios: Get access to the tail end of the LFP data block

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

On Wed, 06 Apr 2022, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> We need to start parsing stuff from the tail end of the LFP data block.
> This is made awkward by the fact that the fp_timing table has variable
> size. So we must use a bit more finesse to get the tail end, and to
> make sure we allocate enough memory for it to make sure our struct
> representation fits.
>
> v2: Rebase due to the preallocation of BDB blocks
> v3: Rebase due to min_size WARN relocation
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_bios.c     | 39 ++++++++++++++++++-
>  drivers/gpu/drm/i915/display/intel_vbt_defs.h | 17 ++++++++
>  2 files changed, 55 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c
> index d32091dad1b0..9a14d55b636c 100644
> --- a/drivers/gpu/drm/i915/display/intel_bios.c
> +++ b/drivers/gpu/drm/i915/display/intel_bios.c
> @@ -188,7 +188,7 @@ static const struct {
>  	{ .section_id = BDB_LVDS_LFP_DATA_PTRS,
>  	  .min_size = sizeof(struct bdb_lvds_lfp_data_ptrs), },
>  	{ .section_id = BDB_LVDS_LFP_DATA,
> -	  .min_size = sizeof(struct bdb_lvds_lfp_data), },
> +	  .min_size = 0, /* special case */ },
>  	{ .section_id = BDB_LVDS_BACKLIGHT,
>  	  .min_size = sizeof(struct bdb_lfp_backlight_data), },
>  	{ .section_id = BDB_LFP_POWER,
> @@ -203,6 +203,23 @@ static const struct {
>  	  .min_size = sizeof(struct bdb_generic_dtd), },
>  };
>  
> +static size_t lfp_data_min_size(struct drm_i915_private *i915)
> +{
> +	const struct bdb_lvds_lfp_data_ptrs *ptrs;
> +	size_t size;
> +
> +	ptrs = find_section(i915, BDB_LVDS_LFP_DATA_PTRS);

This depends on that block having been initialized before. Maybe the
ordering requirement deserves a comment in bdb_blocks[].

> +	if (!ptrs)
> +		return 0;
> +
> +	size = sizeof(struct bdb_lvds_lfp_data);

Basically that and the struct definition are bogus, though? They assume
a rigid structure. It might be true for some specific platforms, but
generally likely not.

Or we could of course just add a comment about that in intel_vbt_defs.h.

> +	if (ptrs->panel_name.table_size)
> +		size = max(size, ptrs->panel_name.offset +
> +			   sizeof(struct bdb_lvds_lfp_data_tail));
> +
> +	return size;
> +}
> +
>  static bool validate_lfp_data_ptrs(const void *bdb,
>  				   const struct bdb_lvds_lfp_data_ptrs *ptrs)
>  {
> @@ -492,6 +509,9 @@ static void init_bdb_blocks(struct drm_i915_private *i915,
>  		enum bdb_block_id section_id = bdb_blocks[i].section_id;
>  		size_t min_size = bdb_blocks[i].min_size;
>  
> +		if (section_id == BDB_LVDS_LFP_DATA)
> +			min_size = lfp_data_min_size(i915);

Nitpick, could also leave the "default" min size in bdb_blocks[], have
lfp_data_min_size() return the other value or 0, and have the max()
here. *shrug*

> +
>  		init_bdb_block(i915, bdb, section_id, min_size);
>  	}
>  }
> @@ -562,6 +582,16 @@ get_lvds_fp_timing(const struct bdb_lvds_lfp_data *data,
>  	return (const void *)data + ptrs->ptr[index].fp_timing.offset;
>  }
>  
> +static const struct bdb_lvds_lfp_data_tail *
> +get_lfp_data_tail(const struct bdb_lvds_lfp_data *data,
> +		  const struct bdb_lvds_lfp_data_ptrs *ptrs)
> +{
> +	if (ptrs->panel_name.table_size)
> +		return (const void *)data + ptrs->panel_name.offset;
> +	else
> +		return NULL;
> +}
> +
>  /* Parse general panel options */
>  static void
>  parse_panel_options(struct drm_i915_private *i915)
> @@ -666,6 +696,7 @@ static void
>  parse_lfp_data(struct drm_i915_private *i915)
>  {
>  	const struct bdb_lvds_lfp_data *data;
> +	const struct bdb_lvds_lfp_data_tail *tail;
>  	const struct bdb_lvds_lfp_data_ptrs *ptrs;
>  
>  	ptrs = find_section(i915, BDB_LVDS_LFP_DATA_PTRS);
> @@ -678,6 +709,12 @@ parse_lfp_data(struct drm_i915_private *i915)
>  
>  	if (!i915->vbt.lfp_lvds_vbt_mode)
>  		parse_lfp_panel_dtd(i915, data, ptrs);
> +
> +	tail = get_lfp_data_tail(data, ptrs);
> +	if (!tail)
> +		return;
> +
> +	(void)tail;

Mmmkay.

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

>  }
>  
>  static void
> diff --git a/drivers/gpu/drm/i915/display/intel_vbt_defs.h b/drivers/gpu/drm/i915/display/intel_vbt_defs.h
> index e4a11c3e3f3e..64551d206aeb 100644
> --- a/drivers/gpu/drm/i915/display/intel_vbt_defs.h
> +++ b/drivers/gpu/drm/i915/display/intel_vbt_defs.h
> @@ -783,6 +783,23 @@ struct lvds_lfp_panel_name {
>  	u8 name[13];
>  } __packed;
>  
> +struct lvds_lfp_black_border {
> +	u8 top; /*  227 */
> +	u8 bottom; /*  227 */
> +	u8 left; /* 238 */
> +	u8 right; /* 238 */
> +} __packed;
> +
> +struct bdb_lvds_lfp_data_tail {
> +	struct lvds_lfp_panel_name panel_name[16]; /* 156-163? */
> +	u16 scaling_enable; /* 187 */
> +	u8 seamless_drrs_min_refresh_rate[16]; /* 188 */
> +	u8 pixel_overlap_count[16]; /* 208 */
> +	struct lvds_lfp_black_border black_border[16]; /* 227 */
> +	u16 dual_lfp_port_sync_enable; /* 231 */
> +	u16 gpu_dithering_for_banding_artifacts; /* 245 */
> +} __packed;
> +
>  /*
>   * Block 43 - LFP Backlight Control Data Block
>   */

-- 
Jani Nikula, Intel Open Source Graphics Center