| Message ID | 20220406134002.30761-1-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | None | expand |
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 > */
On Thu, Apr 07, 2022 at 08:07:24PM +0300, Jani Nikula wrote: > 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[]. 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@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
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); + if (!ptrs) + return 0; + + size = sizeof(struct bdb_lvds_lfp_data); + 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); + 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; } 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 */