On Wed, 2020-10-07 at 15:45 -0700, Matt Roper wrote:
> On Tue, Sep 29, 2020 at 03:34:17PM -0700, José Roberto de Souza wrote:
> > Child min_brightness is obsolete from VBT 234+, instead the new
> > min_brightness field in the main structure should be used.
> >
> > This new field is 16 bits wide, so backlight_precision_bits is needed
> > to check if value needs to be scaled down but it is only available in
> > VBT 236+ so working around it by using the also new backlight_level
> > in the main struct.
> >
> > v2:
> > - missed that backlight_data->level is also obsolete
> >
> > BSpec: 20149
> > Signed-off-by: José Roberto de Souza <jose.souza@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/display/intel_bios.c | 30 +++++++++++++++++--
> > drivers/gpu/drm/i915/display/intel_vbt_defs.h | 12 ++++++--
> > 2 files changed, 38 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c
> > index 4716484af62d..58e5657a77bb 100644
> > --- a/drivers/gpu/drm/i915/display/intel_bios.c
> > +++ b/drivers/gpu/drm/i915/display/intel_bios.c
> > @@ -425,6 +425,7 @@ parse_lfp_backlight(struct drm_i915_private *dev_priv,
> > const struct bdb_lfp_backlight_data *backlight_data;
> > const struct lfp_backlight_data_entry *entry;
> > int panel_type = dev_priv->vbt.panel_type;
> > + u16 level;
> >
> >
> >
> >
> > backlight_data = find_section(bdb, BDB_LVDS_BACKLIGHT);
> > if (!backlight_data)
> > @@ -459,14 +460,39 @@ parse_lfp_backlight(struct drm_i915_private *dev_priv,
> >
> >
> >
> >
> > dev_priv->vbt.backlight.pwm_freq_hz = entry->pwm_freq_hz;
> > dev_priv->vbt.backlight.active_low_pwm = entry->active_low_pwm;
> > - dev_priv->vbt.backlight.min_brightness = entry->min_brightness;
> > +
> > + if (bdb->version >= 234) {
> > + bool scale = false;
> > + u16 min_level;
> > +
> > + level = backlight_data->backlight_level[panel_type].level;
> > + min_level = backlight_data->backlight_min_level[panel_type].level;
> > +
> > + if (bdb->version >= 236)
> > + scale = backlight_data->backlight_precision_bits[panel_type] == 16;
> > + else
> > + scale = level > 255;
>
> I'm not sure I follow the 'else' arm here. On version 234/235 we'd have
> 16-bit level values. In the absence of any other precision information
> wouldn't we assume that all the bits are used and that we have a full
> 16-bit precision? If the level is < 256 (or for that matter if we have
> any value where level & 0xFF is non-zero) wouldn't that definitely mean
> that there are 16-bits of precision since otherwise those low bits would
> have to be 0's?
My understand is that in version 234 or 235 all brightness levels could be set as 16bits or 8bits wide by vendors and it did not had a clear way for
driver to know what it is, then version 236 came fixing it.
So working around it by using the regular brightness level(supposed the one that vendor wants the panel to have by default) and we can suppose that it
will be higher than the minimum so for 16bits of precision it will be higher than 255.
Anyways I doubt that any production product will have VBT version 234 or 235.
>
> > +
> > + if (scale)
> > + min_level = min_level / 255;
> > +
> > + if (min_level > 255) {
> > + drm_warn(&dev_priv->drm, "Backlight min level > 255\n");
> > + level = 255;
> > + }
> > + dev_priv->vbt.backlight.min_brightness = min_level;
> > + } else {
> > + level = backlight_data->level[panel_type];
> > + dev_priv->vbt.backlight.min_brightness = entry->min_brightness;
> > + }
> > +
> > drm_dbg_kms(&dev_priv->drm,
> > "VBT backlight PWM modulation frequency %u Hz, "
> > "active %s, min brightness %u, level %u, controller %u\n",
> > dev_priv->vbt.backlight.pwm_freq_hz,
> > dev_priv->vbt.backlight.active_low_pwm ? "low" : "high",
> > dev_priv->vbt.backlight.min_brightness,
> > - backlight_data->level[panel_type],
> > + level,
> > dev_priv->vbt.backlight.controller);
> > }
> >
> >
> >
> >
> >
> >
> >
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_vbt_defs.h b/drivers/gpu/drm/i915/display/intel_vbt_defs.h
> > index 54bcc6a6947c..b4742c4fde97 100644
> > --- a/drivers/gpu/drm/i915/display/intel_vbt_defs.h
> > +++ b/drivers/gpu/drm/i915/display/intel_vbt_defs.h
> > @@ -782,7 +782,7 @@ struct lfp_backlight_data_entry {
> > u8 active_low_pwm:1;
> > u8 obsolete1:5;
> > u16 pwm_freq_hz;
> > - u8 min_brightness;
> > + u8 min_brightness; /* Obsolete from 234+ */
> > u8 obsolete2;
> > u8 obsolete3;
> > } __packed;
> > @@ -792,11 +792,19 @@ struct lfp_backlight_control_method {
> > u8 controller:4;
> > } __packed;
> >
> >
> >
> >
> >
> >
> >
> >
> > +struct lfp_backlight_level {
> > + u32 level : 16;
> > + u32 reserved : 16;
> > +} __packed;
> > +
> > struct bdb_lfp_backlight_data {
> > u8 entry_size;
> > struct lfp_backlight_data_entry data[16];
> > - u8 level[16];
> > + u8 level[16]; /* Obsolete from 234+ */
> > struct lfp_backlight_control_method backlight_control[16];
> > + struct lfp_backlight_level backlight_level[16]; /* 234+ */
> > + struct lfp_backlight_level backlight_min_level[16]; /* 234+ */
>
> Technically these two are described as "brightness level" rather than
> "backlight level" in the spec. Matching the spec's terminology might
> make this slightly easier to follow when people look at it in the
> future, but up to you.
Okay will rename those, take a look in the comment above so we have an agreement for the the next version.
thanks
>
>
> Matt
>
> > + u8 backlight_precision_bits[16]; /* 236+ */
> > } __packed;
> >
> >
> >
> >
> >
> >
> >
> >
> > /*
> > --
> > 2.28.0
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
