pon., 20 wrz 2021 o 22:47 Souza, Jose <jose.souza@xxxxxxxxx> napisał(a):
>
> On Mon, 2021-09-20 at 16:11 +0200, Lukasz Majczak wrote:
> > With patch "drm/i915/vbt: Fix backlight parsing for VBT 234+"
> > the size of bdb_lfp_backlight_data structure has been increased,
> > causing if-statement in the parse_lfp_backlight function
> > that comapres this structure size to the one retrieved from BDB,
> > always to fail for older revisions.
> > This patch fixes it by comparing a total size of all fileds from
> > the structure (present before the change) with the value gathered from BDB.
> > Tested on Chromebook Pixelbook (Nocturne) (reports bdb->version = 221)
> >
> > Cc: <stable@xxxxxxxxxxxxxxx> # 5.4+
> > Tested-by: Lukasz Majczak <lma@xxxxxxxxxxxx>
> > Signed-off-by: Lukasz Majczak <lma@xxxxxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/display/intel_bios.c | 4 +++-
> > drivers/gpu/drm/i915/display/intel_vbt_defs.h | 5 +++++
> > 2 files changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c
> > index 3c25926092de..052a19b455d1 100644
> > --- a/drivers/gpu/drm/i915/display/intel_bios.c
> > +++ b/drivers/gpu/drm/i915/display/intel_bios.c
> > @@ -452,7 +452,9 @@ parse_lfp_backlight(struct drm_i915_private *i915,
> >
> > i915->vbt.backlight.type = INTEL_BACKLIGHT_DISPLAY_DDI;
> > if (bdb->version >= 191 &&
> > - get_blocksize(backlight_data) >= sizeof(*backlight_data)) {
> > + get_blocksize(backlight_data) >= (sizeof(backlight_data->entry_size) +
> > + sizeof(backlight_data->data) +
> > + sizeof(backlight_data->level))) {
>
> Missing sizeof(backlight_data->backlight_control) but this is getting very verbose.
> Would be better have a expected size variable set each version set in the beginning of this function.
>
> something like:
> switch (bdb->version) {
> case 191:
> expected_size = x;
> break;
> case 234:
> expected_size = x;
> break;
> case 236:
> default:
> expected_size = x;
> }
>
>
> > const struct lfp_backlight_control_method *method;
> >
> > method = &backlight_data->backlight_control[panel_type];
> > diff --git a/drivers/gpu/drm/i915/display/intel_vbt_defs.h b/drivers/gpu/drm/i915/display/intel_vbt_defs.h
> > index 330077c2e588..fff456bf8783 100644
> > --- a/drivers/gpu/drm/i915/display/intel_vbt_defs.h
> > +++ b/drivers/gpu/drm/i915/display/intel_vbt_defs.h
> > @@ -814,6 +814,11 @@ struct lfp_brightness_level {
> > u16 reserved;
> > } __packed;
> >
> > +/*
> > + * Changing struct bdb_lfp_backlight_data might affect its
> > + * size comparation to the value hold in BDB.
> > + * (e.g. in parse_lfp_backlight())
> > + */
>
> This is true for all the blocks so I don't think we need this comment.
>
> > struct bdb_lfp_backlight_data {
> > u8 entry_size;
> > struct lfp_backlight_data_entry data[16];
>
Hi Jose, Jani
Jani - you are right - I was working on 5.4 with a backported patch -
I'm sorry for this confusion.
Jose,
Regarding expected_size, I couldn't find documentation that could
described this structure size changes among revisions, so all I could
do is to do an educated guess, basing on comments at this structure,
like:
(gdb) ptype /o struct bdb_lfp_backlight_data
/* offset | size */ type = struct bdb_lfp_backlight_data {
/* 0 | 1 */ u8 entry_size;
/* 1 | 96 */ struct lfp_backlight_data_entry data[16];
/* 97 | 16 */ u8 level[16];
/* 113 | 16 */ struct lfp_backlight_control_method
backlight_control[16];
/* 129 | 64 */ struct lfp_brightness_level
brightness_level[16]; /* 234+ */
/* 193 | 64 */ struct lfp_brightness_level
brightness_min_level[16]; /* 234+ */
/* 257 | 16 */ u8 brightness_precision_bits[16]; /* 236+ */
/* total size (bytes): 273 */
}
if (revision <= 234)
expected_size = 129;
else if (revision > 234 && revision <=236)
expected_size = 257;
else /* revision > 236 */
expected_size = 273;
Is this approach ok? Otherwise I think I would need help from you to
get exact numbers for each revision...
Best regards,
Lukasz
