All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Souza, Jose" <jose.souza@intel.com>
To: "Roper, Matthew D" <matthew.d.roper@intel.com>
Cc: "intel-gfx@lists.freedesktop.org" <intel-gfx@lists.freedesktop.org>
Subject: Re: [Intel-gfx] [PATCH v2 1/3] drm/i915/vbt: Fix backlight parsing for VBT 234+
Date: Thu, 8 Oct 2020 00:33:48 +0000	[thread overview]
Message-ID: <ec4d0b5cd6eb09acc53c374b4dac70fb9c41b4c9.camel@intel.com> (raw)
In-Reply-To: <20201007224539.GL2863995@mdroper-desk1.amr.corp.intel.com>

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
> 

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

  reply	other threads:[~2020-10-08  0:33 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-09-29 22:34 [Intel-gfx] [PATCH v2 1/3] drm/i915/vbt: Fix backlight parsing for VBT 234+ José Roberto de Souza
2020-09-29 22:34 ` [Intel-gfx] [PATCH v2 2/3] drm/i915/vbt: Update the version and expected size of BDB_GENERAL_DEFINITIONS map José Roberto de Souza
2020-10-07 22:49   ` Matt Roper
2020-09-29 22:34 ` [Intel-gfx] [PATCH v2 3/3] drm/i915/vbt: Add VRR VBT toggle José Roberto de Souza
2020-10-07 22:53   ` Matt Roper
2020-09-29 23:09 ` [Intel-gfx] ✓ Fi.CI.BAT: success for series starting with [v2,1/3] drm/i915/vbt: Fix backlight parsing for VBT 234+ Patchwork
2020-09-30 13:03 ` [Intel-gfx] ✓ Fi.CI.IGT: " Patchwork
2020-10-06  9:04 ` [Intel-gfx] [PATCH v2 1/3] " Jani Nikula
2020-10-06 17:10   ` Souza, Jose
2020-10-07 22:45 ` Matt Roper
2020-10-08  0:33   ` Souza, Jose [this message]
2020-10-08  3:28     ` Matt Roper

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=ec4d0b5cd6eb09acc53c374b4dac70fb9c41b4c9.camel@intel.com \
    --to=jose.souza@intel.com \
    --cc=intel-gfx@lists.freedesktop.org \
    --cc=matthew.d.roper@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.