| Message ID | 20200929223419.146925-1-jose.souza@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/3] drm/i915/vbt: Fix backlight parsing for VBT 234+ | expand |
On Tue, 29 Sep 2020, José Roberto de Souza <jose.souza@intel.com> 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@intel.com> > --- > 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; > + > + 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; That looks like two u16's to me. ;) BR, Jani. > +} __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+ */ > + u8 backlight_precision_bits[16]; /* 236+ */ > } __packed; > > /*
On Tue, 2020-10-06 at 12:04 +0300, Jani Nikula wrote: > On Tue, 29 Sep 2020, José Roberto de Souza <jose.souza@intel.com> 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@intel.com> > > --- > > 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; > > + > > + 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; > > That looks like two u16's to me. ;) Okay, anything else to be changed in the next version? > > BR, > Jani. > > > > +} __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+ */ > > + u8 backlight_precision_bits[16]; /* 236+ */ > > } __packed; > > > > > > > > > > /* >
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@intel.com> > --- > 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? > + > + 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. Matt > + u8 backlight_precision_bits[16]; /* 236+ */ > } __packed; > > /* > -- > 2.28.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
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@intel.com> > > --- > > 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@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx >
On Wed, Oct 07, 2020 at 05:33:48PM -0700, Souza, Jose wrote: > 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@intel.com> > > > --- > > > 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. Okay. I guess since it was described with the term "precision" in the spec that made me think of it as "only the highest 8 bits in use" rather than an actual 8-bit range (i.e., just lower bits), but I guess there wouldn't really be a need to specify it if that were the case. So I think your logic here is probably correct. With the s/backlight/brightness/ rename and the u32 -> u16 simplification suggested by Jani, Reviewed-by: Matt Roper <matthew.d.roper@intel.com> > > > > > > + > > > + 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@lists.freedesktop.org > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > >
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; + + 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+ */ + u8 backlight_precision_bits[16]; /* 236+ */ } __packed; /*
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@intel.com> --- 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(-)