| Message ID | 5da7c92cb5634c7b7349065fe82f46c0564a6715.1383920621.git.jani.nikula@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, 2013-11-08 at 16:48 +0200, Jani Nikula wrote: > Signed-off-by: Jani Nikula <jani.nikula@intel.com> > --- > drivers/gpu/drm/i915/intel_panel.c | 10 +++++++--- > 1 file changed, 7 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c > index a821949..e82b2dd 100644 > --- a/drivers/gpu/drm/i915/intel_panel.c > +++ b/drivers/gpu/drm/i915/intel_panel.c > @@ -555,7 +555,7 @@ static void i9xx_set_backlight(struct intel_connector *connector, u32 level) > { > struct drm_device *dev = connector->base.dev; > struct drm_i915_private *dev_priv = dev->dev_private; > - u32 tmp; > + u32 tmp, mask; > > if (is_backlight_combination_mode(dev)) { > u32 max = intel_panel_get_max_backlight(connector); > @@ -570,10 +570,14 @@ static void i9xx_set_backlight(struct intel_connector *connector, u32 level) > pci_write_config_byte(dev->pdev, PCI_LBPC, lbpc); > } > > - if (INTEL_INFO(dev)->gen < 4) > + if (IS_GEN4(dev)) { > + mask = BACKLIGHT_DUTY_CYCLE_MASK; > + } else { > level <<= 1; > + mask = BACKLIGHT_DUTY_CYCLE_MASK_PNV; > + } According to the gen2/3 bspec I have, the correct mask is BACKLIGHT_DUTY_CYCLE_MASK_PNV only in case of IS_PINEVIEW(dev), for everything else it's BACKLIGHT_DUTY_CYCLE_MASK. --Imre > > - tmp = I915_READ(BLC_PWM_CTL) & ~BACKLIGHT_DUTY_CYCLE_MASK; > + tmp = I915_READ(BLC_PWM_CTL) & ~mask; > I915_WRITE(BLC_PWM_CTL, tmp | level); > } >
On Wed, 13 Nov 2013, Imre Deak <imre.deak@intel.com> wrote: > On Fri, 2013-11-08 at 16:48 +0200, Jani Nikula wrote: >> Signed-off-by: Jani Nikula <jani.nikula@intel.com> >> --- >> drivers/gpu/drm/i915/intel_panel.c | 10 +++++++--- >> 1 file changed, 7 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c >> index a821949..e82b2dd 100644 >> --- a/drivers/gpu/drm/i915/intel_panel.c >> +++ b/drivers/gpu/drm/i915/intel_panel.c >> @@ -555,7 +555,7 @@ static void i9xx_set_backlight(struct intel_connector *connector, u32 level) >> { >> struct drm_device *dev = connector->base.dev; >> struct drm_i915_private *dev_priv = dev->dev_private; >> - u32 tmp; >> + u32 tmp, mask; >> >> if (is_backlight_combination_mode(dev)) { >> u32 max = intel_panel_get_max_backlight(connector); >> @@ -570,10 +570,14 @@ static void i9xx_set_backlight(struct intel_connector *connector, u32 level) >> pci_write_config_byte(dev->pdev, PCI_LBPC, lbpc); >> } >> >> - if (INTEL_INFO(dev)->gen < 4) >> + if (IS_GEN4(dev)) { >> + mask = BACKLIGHT_DUTY_CYCLE_MASK; >> + } else { >> level <<= 1; >> + mask = BACKLIGHT_DUTY_CYCLE_MASK_PNV; >> + } > > According to the gen2/3 bspec I have, the correct mask is > BACKLIGHT_DUTY_CYCLE_MASK_PNV only in case of IS_PINEVIEW(dev), for > everything else it's BACKLIGHT_DUTY_CYCLE_MASK. What you say is correct, but we've treated all gen2/3 similar to PNV since commit ca88479c1c3b7b1a9f94320745f5331e1de77f80 Author: Keith Packard <keithp@keithp.com> Date: Fri Nov 18 11:09:24 2011 -0800 drm/i915: Treat pre-gen4 backlight duty cycle value consistently i.e. we only use the high 15 bits for all gen2/3. For non-PNV this just means the lowest bit is always zero. For PNV the lowest bit has a different meaning in both the PWM freq and duty cycle fields. I don't want to take any chances by changing this behaviour. I realize there's zero comments about this in the code; would you like me to add some? BR, Jani. > > --Imre > >> >> - tmp = I915_READ(BLC_PWM_CTL) & ~BACKLIGHT_DUTY_CYCLE_MASK; >> + tmp = I915_READ(BLC_PWM_CTL) & ~mask; >> I915_WRITE(BLC_PWM_CTL, tmp | level); >> } >> > >
On Wed, Nov 13, 2013 at 10:27:38AM +0200, Jani Nikula wrote: > On Wed, 13 Nov 2013, Imre Deak <imre.deak@intel.com> wrote: > > On Fri, 2013-11-08 at 16:48 +0200, Jani Nikula wrote: > >> Signed-off-by: Jani Nikula <jani.nikula@intel.com> > >> --- > >> drivers/gpu/drm/i915/intel_panel.c | 10 +++++++--- > >> 1 file changed, 7 insertions(+), 3 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c > >> index a821949..e82b2dd 100644 > >> --- a/drivers/gpu/drm/i915/intel_panel.c > >> +++ b/drivers/gpu/drm/i915/intel_panel.c > >> @@ -555,7 +555,7 @@ static void i9xx_set_backlight(struct intel_connector *connector, u32 level) > >> { > >> struct drm_device *dev = connector->base.dev; > >> struct drm_i915_private *dev_priv = dev->dev_private; > >> - u32 tmp; > >> + u32 tmp, mask; > >> > >> if (is_backlight_combination_mode(dev)) { > >> u32 max = intel_panel_get_max_backlight(connector); > >> @@ -570,10 +570,14 @@ static void i9xx_set_backlight(struct intel_connector *connector, u32 level) > >> pci_write_config_byte(dev->pdev, PCI_LBPC, lbpc); > >> } > >> > >> - if (INTEL_INFO(dev)->gen < 4) > >> + if (IS_GEN4(dev)) { > >> + mask = BACKLIGHT_DUTY_CYCLE_MASK; > >> + } else { > >> level <<= 1; > >> + mask = BACKLIGHT_DUTY_CYCLE_MASK_PNV; > >> + } > > > > According to the gen2/3 bspec I have, the correct mask is > > BACKLIGHT_DUTY_CYCLE_MASK_PNV only in case of IS_PINEVIEW(dev), for > > everything else it's BACKLIGHT_DUTY_CYCLE_MASK. > > What you say is correct, but we've treated all gen2/3 similar to PNV > since > > commit ca88479c1c3b7b1a9f94320745f5331e1de77f80 > Author: Keith Packard <keithp@keithp.com> > Date: Fri Nov 18 11:09:24 2011 -0800 > > drm/i915: Treat pre-gen4 backlight duty cycle value consistently > > i.e. we only use the high 15 bits for all gen2/3. For non-PNV this just > means the lowest bit is always zero. For PNV the lowest bit has a > different meaning in both the PWM freq and duty cycle fields. > > I don't want to take any chances by changing this behaviour. I realize > there's zero comments about this in the code; would you like me to add > some? Yeah, I think some comment here would be good. Or maybe a follow-up patch to differentiate between pnv and everything else on gen2/3. -Daniel > > > BR, > Jani. > > > > > > --Imre > > > >> > >> - tmp = I915_READ(BLC_PWM_CTL) & ~BACKLIGHT_DUTY_CYCLE_MASK; > >> + tmp = I915_READ(BLC_PWM_CTL) & ~mask; > >> I915_WRITE(BLC_PWM_CTL, tmp | level); > >> } > >> > > > > > > -- > Jani Nikula, Intel Open Source Technology Center > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, 2013-11-13 at 10:27 +0200, Jani Nikula wrote: > On Wed, 13 Nov 2013, Imre Deak <imre.deak@intel.com> wrote: > > On Fri, 2013-11-08 at 16:48 +0200, Jani Nikula wrote: > >> Signed-off-by: Jani Nikula <jani.nikula@intel.com> > >> --- > >> drivers/gpu/drm/i915/intel_panel.c | 10 +++++++--- > >> 1 file changed, 7 insertions(+), 3 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c > >> index a821949..e82b2dd 100644 > >> --- a/drivers/gpu/drm/i915/intel_panel.c > >> +++ b/drivers/gpu/drm/i915/intel_panel.c > >> @@ -555,7 +555,7 @@ static void i9xx_set_backlight(struct intel_connector *connector, u32 level) > >> { > >> struct drm_device *dev = connector->base.dev; > >> struct drm_i915_private *dev_priv = dev->dev_private; > >> - u32 tmp; > >> + u32 tmp, mask; > >> > >> if (is_backlight_combination_mode(dev)) { > >> u32 max = intel_panel_get_max_backlight(connector); > >> @@ -570,10 +570,14 @@ static void i9xx_set_backlight(struct intel_connector *connector, u32 level) > >> pci_write_config_byte(dev->pdev, PCI_LBPC, lbpc); > >> } > >> > >> - if (INTEL_INFO(dev)->gen < 4) > >> + if (IS_GEN4(dev)) { > >> + mask = BACKLIGHT_DUTY_CYCLE_MASK; > >> + } else { > >> level <<= 1; > >> + mask = BACKLIGHT_DUTY_CYCLE_MASK_PNV; > >> + } > > > > According to the gen2/3 bspec I have, the correct mask is > > BACKLIGHT_DUTY_CYCLE_MASK_PNV only in case of IS_PINEVIEW(dev), for > > everything else it's BACKLIGHT_DUTY_CYCLE_MASK. > > What you say is correct, but we've treated all gen2/3 similar to PNV > since > > commit ca88479c1c3b7b1a9f94320745f5331e1de77f80 > Author: Keith Packard <keithp@keithp.com> > Date: Fri Nov 18 11:09:24 2011 -0800 > > drm/i915: Treat pre-gen4 backlight duty cycle value consistently > > i.e. we only use the high 15 bits for all gen2/3. For non-PNV this just > means the lowest bit is always zero. For PNV the lowest bit has a > different meaning in both the PWM freq and duty cycle fields. > > I don't want to take any chances by changing this behaviour. I realize > there's zero comments about this in the code; would you like me to add > some? Yea, looking at the log would've been useful.. I see now from that commit that there was a problem with setting bit 0 on some old HW, so I'm ok to leave this as-is. A comment would be nice, but either way: Reviewed-by: Imre Deak <imre.deak@intel.com>
diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c index a821949..e82b2dd 100644 --- a/drivers/gpu/drm/i915/intel_panel.c +++ b/drivers/gpu/drm/i915/intel_panel.c @@ -555,7 +555,7 @@ static void i9xx_set_backlight(struct intel_connector *connector, u32 level) { struct drm_device *dev = connector->base.dev; struct drm_i915_private *dev_priv = dev->dev_private; - u32 tmp; + u32 tmp, mask; if (is_backlight_combination_mode(dev)) { u32 max = intel_panel_get_max_backlight(connector); @@ -570,10 +570,14 @@ static void i9xx_set_backlight(struct intel_connector *connector, u32 level) pci_write_config_byte(dev->pdev, PCI_LBPC, lbpc); } - if (INTEL_INFO(dev)->gen < 4) + if (IS_GEN4(dev)) { + mask = BACKLIGHT_DUTY_CYCLE_MASK; + } else { level <<= 1; + mask = BACKLIGHT_DUTY_CYCLE_MASK_PNV; + } - tmp = I915_READ(BLC_PWM_CTL) & ~BACKLIGHT_DUTY_CYCLE_MASK; + tmp = I915_READ(BLC_PWM_CTL) & ~mask; I915_WRITE(BLC_PWM_CTL, tmp | level); }
Signed-off-by: Jani Nikula <jani.nikula@intel.com> --- drivers/gpu/drm/i915/intel_panel.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)