Re: [Intel-gfx] [PATCH v2 1/3] drm/i915/vbt: Fix backlight parsing for VBT 234+

[Matt Roper <matthew.d.roper@intel.com>]

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
> > 
> 

-- 
Matt Roper
Graphics Software Engineer
VTT-OSGC Platform Enablement
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx