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

[Matt Roper]

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