From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jesse Barnes Subject: Re: [PATCH 3/3] drm/i915/vlv: use min brightness from VBT Date: Tue, 1 Apr 2014 09:16:07 -0700 Message-ID: <20140401091607.72e644c5@jbarnes-desktop> References: <1396289637-1013-1-git-send-email-jbarnes@virtuousgeek.org> <1396289637-1013-3-git-send-email-jbarnes@virtuousgeek.org> <8761mteaky.fsf@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from gproxy3-pub.mail.unifiedlayer.com (gproxy3-pub.mail.unifiedlayer.com [69.89.30.42]) by gabe.freedesktop.org (Postfix) with SMTP id 34D5B6E79E for ; Tue, 1 Apr 2014 09:14:59 -0700 (PDT) In-Reply-To: <8761mteaky.fsf@intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Jani Nikula Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Tue, 01 Apr 2014 11:08:13 +0300 Jani Nikula wrote: > On Mon, 31 Mar 2014, Jesse Barnes wrote: > > Going below the minimum value may affect the BLC_EN line, so try to use > > the VBT provided minimum where possible, otherwise use an experimentally > > derived value to prevent the panel from coming up. > > > > Signed-off-by: Jesse Barnes > > --- > > drivers/gpu/drm/i915/i915_drv.h | 1 + > > drivers/gpu/drm/i915/intel_bios.c | 3 ++- > > drivers/gpu/drm/i915/intel_drv.h | 1 + > > drivers/gpu/drm/i915/intel_panel.c | 10 +++++++++- > > 4 files changed, 13 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > > index ff02225..3c40dcb 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.h > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > @@ -1148,6 +1148,7 @@ struct intel_vbt_data { > > struct { > > u16 pwm_freq_hz; > > bool active_low_pwm; > > + u8 min_brightness; > > } backlight; > > > > /* MIPI DSI */ > > diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c > > index 4867f4c..e8dedf5 100644 > > --- a/drivers/gpu/drm/i915/intel_bios.c > > +++ b/drivers/gpu/drm/i915/intel_bios.c > > @@ -301,11 +301,12 @@ parse_lfp_backlight(struct drm_i915_private *dev_priv, struct bdb_header *bdb) > > > > 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; > > DRM_DEBUG_KMS("VBT backlight PWM modulation frequency %u Hz, " > > "active %s, min brightness %u, level %u\n", > > dev_priv->vbt.backlight.pwm_freq_hz, > > dev_priv->vbt.backlight.active_low_pwm ? "low" : "high", > > - entry->min_brightness, > > + dev_priv->vbt.backlight.min_brightness, > > backlight_data->level[panel_type]); > > } > > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > > index 0e91c40..053a968 100644 > > --- a/drivers/gpu/drm/i915/intel_drv.h > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > @@ -166,6 +166,7 @@ struct intel_panel { > > bool present; > > u32 level; > > u32 max; > > + u32 min; > > bool enabled; > > bool combination_mode; /* gen 2/4 only */ > > bool active_low_pwm; > > diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c > > index 21c5e6f..27d7508 100644 > > --- a/drivers/gpu/drm/i915/intel_panel.c > > +++ b/drivers/gpu/drm/i915/intel_panel.c > > @@ -510,6 +510,9 @@ void intel_panel_set_backlight(struct intel_connector *connector, u32 level, > > else > > level = freq / max * level; > > > > + if (level < panel->backlight.min) > > + level = panel->backlight.min; > > Are you sure the VBT minimum is the *absolute* minimum duty cycle value? > The spec I have says, "This field specifies the minimum brightness", > which could be an absolute value, percentage, multiplier, anything > really. I most doubt it's an absolute value because you have to go out > of your way to figure out what the max is because it's defined in terms > of the PWM frequency in Hz. > > No matter what the unit is, this is wrong for any gen 2/4 platform where > combination mode is enabled. > > --- > > I'm also hesitant about the change. First, what does it mean for the > userspace that 0 no longer switches off the backlight? (Which I realize > was sort of broken due to not really disabling the PWM since gen4, but > that's what the perceived effect was anyway.) This may not be a big > issue since 0 is not necessarily off for acpi backlight. > > Second, what's the usability implication of backlight change requests > potentially having no effect at the low values? Imagine maybe 10-20 > levels of brightness in the UI but no change between the lowest > levels. I fear we might get regression reports for this. The only way to > handle this would be scaling of some sort I think. > > I suppose our original thinking was mechanism not policy, and just > exposed the full range to userspace. This is very different from the > acpi backlight approach, which looks at vbt/opregion and exposes I think > up to 20 discrete levels with 0 being min, not necessarily off. The > opregion tables may also have a non-linear mapping from backlight > percentage to duty cycle. Maybe we should bite the bullet and follow > suit? Yeah I should have marked this one as RFC; the VBT usage is new and we really do need to scale things. The main bug fix here is to prevent the BYT duty cycle from dropping below ~64. As you say, to do that we really shouldn't expose those lower 64 values to userspace as they'll do nothing. -- Jesse Barnes, Intel Open Source Technology Center