On Wed, 06 Apr 2022, Ville Syrjala <ville.syrjala@xxxxxxxxxxxxxxx> wrote:
> From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxxxx>
> ---
> 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@xxxxxxxxx>
> }
>
> 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
