On Thu, Apr 07, 2022 at 08:07:24PM +0300, Jani Nikula wrote:
> 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[].
Sure.
>
> > + 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.
I think I had that at some point. But I guess I lost it during
one of the many rewrites I did to this stuff.
>
> > + 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*
Could work. I was also pondering making .min_size a vfunc, but that
would lead to excessive boilerplate for all the other blocks. If only
we had lambdas...
>
> > +
> > 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
--
Ville Syrjälä
Intel
