[Intel-gfx] [PATCH v1] drm/i915/bdb: Fix version check

[Lukasz Majczak]

pon., 20 wrz 2021 o 22:47 Souza, Jose <jose.souza at intel.com> 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 at vger.kernel.org> # 5.4+
> > Tested-by: Lukasz Majczak <lma at semihalf.com>
> > Signed-off-by: Lukasz Majczak <lma at semihalf.com>
> > ---
> >  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