| Message ID | f9531f26aa447398dfdf894d88c3df99ab5aea10.1365768957.git.jani.nikula@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Apr 12, 2013 at 03:18:37PM +0300, Jani Nikula wrote: > Backlight data and registers are fiddled through LVDS/eDP modeset > enable/disable hooks, backlight sysfs files, asle interrupts, and register > save/restore. Protect the backlight related registers and driver private > fields using a spinlock. > > The locking in register save/restore covers a little more than is strictly > necessary, including non-modeset case, for simplicity. > > v2: Cover register access, save/restore, i915_read_blc_pwm_ctl() and code > paths leading there. > > Signed-off-by: Jani Nikula <jani.nikula@intel.com> Looks reasonable. intel_panel_actually_set_backlight() should have a WARN_ON(!spinlocked); The irqness of the register writes scares me slightly - since the IRQ in question is from ACPI and we have a few bug reports along the lines of "backlight makes the entire system sluggish" i.e. commonly associated with bad interrupt handling. Whilst you are looking at updating the backlight programming, can you look at pushing the writes from out of the interrupt handler? -Chris
On Mon, 15 Apr 2013, Chris Wilson <chris@chris-wilson.co.uk> wrote: > On Fri, Apr 12, 2013 at 03:18:37PM +0300, Jani Nikula wrote: >> Backlight data and registers are fiddled through LVDS/eDP modeset >> enable/disable hooks, backlight sysfs files, asle interrupts, and register >> save/restore. Protect the backlight related registers and driver private >> fields using a spinlock. >> >> The locking in register save/restore covers a little more than is strictly >> necessary, including non-modeset case, for simplicity. >> >> v2: Cover register access, save/restore, i915_read_blc_pwm_ctl() and code >> paths leading there. >> >> Signed-off-by: Jani Nikula <jani.nikula@intel.com> > > Looks reasonable. > > intel_panel_actually_set_backlight() should have a WARN_ON(!spinlocked); > > The irqness of the register writes scares me slightly - since the IRQ in > question is from ACPI and we have a few bug reports along the lines of > "backlight makes the entire system sluggish" i.e. commonly associated > with bad interrupt handling. Whilst you are looking at updating the > backlight programming, can you look at pushing the writes from out > of the interrupt handler? So, add a work to do the register writes, and change the spinlock into a mutex while at it? Should be fairly simple, if you think that's the way to go. BR, Jani.
On Mon, Apr 15, 2013 at 6:38 PM, Jani Nikula <jani.nikula@intel.com> wrote: > On Mon, 15 Apr 2013, Chris Wilson <chris@chris-wilson.co.uk> wrote: >> On Fri, Apr 12, 2013 at 03:18:37PM +0300, Jani Nikula wrote: >>> Backlight data and registers are fiddled through LVDS/eDP modeset >>> enable/disable hooks, backlight sysfs files, asle interrupts, and register >>> save/restore. Protect the backlight related registers and driver private >>> fields using a spinlock. >>> >>> The locking in register save/restore covers a little more than is strictly >>> necessary, including non-modeset case, for simplicity. >>> >>> v2: Cover register access, save/restore, i915_read_blc_pwm_ctl() and code >>> paths leading there. >>> >>> Signed-off-by: Jani Nikula <jani.nikula@intel.com> >> >> Looks reasonable. >> >> intel_panel_actually_set_backlight() should have a WARN_ON(!spinlocked); >> >> The irqness of the register writes scares me slightly - since the IRQ in >> question is from ACPI and we have a few bug reports along the lines of >> "backlight makes the entire system sluggish" i.e. commonly associated >> with bad interrupt handling. Whilst you are looking at updating the >> backlight programming, can you look at pushing the writes from out >> of the interrupt handler? > > So, add a work to do the register writes, and change the spinlock into a > mutex while at it? Should be fairly simple, if you think that's the way > to go. I think I'll go ahead with the spinlock fix here for 3.10 and we can look into offloading this all for 3.11. Also, Chris do you remember one of these reports - I've kinda never noticed that particular kind of suck? -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
On Mon, Apr 15, 2013 at 06:59:58PM +0200, Daniel Vetter wrote: > On Mon, Apr 15, 2013 at 6:38 PM, Jani Nikula <jani.nikula@intel.com> wrote: > > On Mon, 15 Apr 2013, Chris Wilson <chris@chris-wilson.co.uk> wrote: > >> On Fri, Apr 12, 2013 at 03:18:37PM +0300, Jani Nikula wrote: > >>> Backlight data and registers are fiddled through LVDS/eDP modeset > >>> enable/disable hooks, backlight sysfs files, asle interrupts, and register > >>> save/restore. Protect the backlight related registers and driver private > >>> fields using a spinlock. > >>> > >>> The locking in register save/restore covers a little more than is strictly > >>> necessary, including non-modeset case, for simplicity. > >>> > >>> v2: Cover register access, save/restore, i915_read_blc_pwm_ctl() and code > >>> paths leading there. > >>> > >>> Signed-off-by: Jani Nikula <jani.nikula@intel.com> > >> > >> Looks reasonable. > >> > >> intel_panel_actually_set_backlight() should have a WARN_ON(!spinlocked); > >> > >> The irqness of the register writes scares me slightly - since the IRQ in > >> question is from ACPI and we have a few bug reports along the lines of > >> "backlight makes the entire system sluggish" i.e. commonly associated > >> with bad interrupt handling. Whilst you are looking at updating the > >> backlight programming, can you look at pushing the writes from out > >> of the interrupt handler? > > > > So, add a work to do the register writes, and change the spinlock into a > > mutex while at it? Should be fairly simple, if you think that's the way > > to go. > > I think I'll go ahead with the spinlock fix here for 3.10 and we can > look into offloading this all for 3.11. Also, Chris do you remember > one of these reports - I've kinda never noticed that particular kind > of suck? I maybe way off the mark and the cause is likely just to be a sucky ACPI firmware: https://bugs.launchpad.net/ubuntu/+source/xserver-xorg-video-intel/+bug/1019370 spinlock -> mutex + workqueue would mitigate against any bad firmware being run under irq context. -Chris
On Mon, Apr 15, 2013 at 10:49 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote: > On Mon, Apr 15, 2013 at 06:59:58PM +0200, Daniel Vetter wrote: >> On Mon, Apr 15, 2013 at 6:38 PM, Jani Nikula <jani.nikula@intel.com> wrote: >> > On Mon, 15 Apr 2013, Chris Wilson <chris@chris-wilson.co.uk> wrote: >> >> On Fri, Apr 12, 2013 at 03:18:37PM +0300, Jani Nikula wrote: >> >>> Backlight data and registers are fiddled through LVDS/eDP modeset >> >>> enable/disable hooks, backlight sysfs files, asle interrupts, and register >> >>> save/restore. Protect the backlight related registers and driver private >> >>> fields using a spinlock. >> >>> >> >>> The locking in register save/restore covers a little more than is strictly >> >>> necessary, including non-modeset case, for simplicity. >> >>> >> >>> v2: Cover register access, save/restore, i915_read_blc_pwm_ctl() and code >> >>> paths leading there. >> >>> >> >>> Signed-off-by: Jani Nikula <jani.nikula@intel.com> >> >> >> >> Looks reasonable. >> >> >> >> intel_panel_actually_set_backlight() should have a WARN_ON(!spinlocked); >> >> >> >> The irqness of the register writes scares me slightly - since the IRQ in >> >> question is from ACPI and we have a few bug reports along the lines of >> >> "backlight makes the entire system sluggish" i.e. commonly associated >> >> with bad interrupt handling. Whilst you are looking at updating the >> >> backlight programming, can you look at pushing the writes from out >> >> of the interrupt handler? >> > >> > So, add a work to do the register writes, and change the spinlock into a >> > mutex while at it? Should be fairly simple, if you think that's the way >> > to go. >> >> I think I'll go ahead with the spinlock fix here for 3.10 and we can >> look into offloading this all for 3.11. Also, Chris do you remember >> one of these reports - I've kinda never noticed that particular kind >> of suck? > > I maybe way off the mark and the cause is likely just to be a sucky ACPI > firmware: > https://bugs.launchpad.net/ubuntu/+source/xserver-xorg-video-intel/+bug/1019370 > > spinlock -> mutex + workqueue would mitigate against any bad firmware > being run under irq context. Hm, that's a funny one indeed. Although it's strange that it doesn't happen when not using X. Might be that the user doesn't notice it (since all the interactive stuff - blinking cursor - is done in irq context), but if this is indeed ACPI doing something crazy I have no idea what's it doing ... -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
On Fri, Apr 12, 2013 at 03:18:37PM +0300, Jani Nikula wrote: > Backlight data and registers are fiddled through LVDS/eDP modeset > enable/disable hooks, backlight sysfs files, asle interrupts, and register > save/restore. Protect the backlight related registers and driver private > fields using a spinlock. > > The locking in register save/restore covers a little more than is strictly > necessary, including non-modeset case, for simplicity. > > v2: Cover register access, save/restore, i915_read_blc_pwm_ctl() and code > paths leading there. > > Signed-off-by: Jani Nikula <jani.nikula@intel.com> Merged the first two patches here for next, I guess we can fix up any other issues here separately. Thanks, Daniel ---- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c index 3b315ba..7751660 100644 --- a/drivers/gpu/drm/i915/i915_dma.c +++ b/drivers/gpu/drm/i915/i915_dma.c @@ -1629,6 +1629,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags) spin_lock_init(&dev_priv->irq_lock); spin_lock_init(&dev_priv->gpu_error.lock); spin_lock_init(&dev_priv->rps.lock); + spin_lock_init(&dev_priv->backlight.lock); mutex_init(&dev_priv->dpio_lock); mutex_init(&dev_priv->rps.hw_lock); diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index b5a495a..9aa9d60 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -952,6 +952,7 @@ typedef struct drm_i915_private { struct { int level; bool enabled; + spinlock_t lock; /* bl registers and the above bl fields */ struct backlight_device *device; } backlight; diff --git a/drivers/gpu/drm/i915/i915_suspend.c b/drivers/gpu/drm/i915/i915_suspend.c index 41f0fde..88b9a66 100644 --- a/drivers/gpu/drm/i915/i915_suspend.c +++ b/drivers/gpu/drm/i915/i915_suspend.c @@ -192,6 +192,7 @@ static void i915_restore_vga(struct drm_device *dev) static void i915_save_display(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; + unsigned long flags; /* Display arbitration control */ if (INTEL_INFO(dev)->gen <= 4) @@ -202,6 +203,8 @@ static void i915_save_display(struct drm_device *dev) if (!drm_core_check_feature(dev, DRIVER_MODESET)) i915_save_display_reg(dev); + spin_lock_irqsave(&dev_priv->backlight.lock, flags); + /* LVDS state */ if (HAS_PCH_SPLIT(dev)) { dev_priv->regfile.savePP_CONTROL = I915_READ(PCH_PP_CONTROL); @@ -222,6 +225,8 @@ static void i915_save_display(struct drm_device *dev) dev_priv->regfile.saveLVDS = I915_READ(LVDS); } + spin_unlock_irqrestore(&dev_priv->backlight.lock, flags); + if (!IS_I830(dev) && !IS_845G(dev) && !HAS_PCH_SPLIT(dev)) dev_priv->regfile.savePFIT_CONTROL = I915_READ(PFIT_CONTROL); @@ -257,6 +262,7 @@ static void i915_restore_display(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; u32 mask = 0xffffffff; + unsigned long flags; /* Display arbitration */ if (INTEL_INFO(dev)->gen <= 4) @@ -265,6 +271,8 @@ static void i915_restore_display(struct drm_device *dev) if (!drm_core_check_feature(dev, DRIVER_MODESET)) i915_restore_display_reg(dev); + spin_lock_irqsave(&dev_priv->backlight.lock, flags); + /* LVDS state */ if (INTEL_INFO(dev)->gen >= 4 && !HAS_PCH_SPLIT(dev)) I915_WRITE(BLC_PWM_CTL2, dev_priv->regfile.saveBLC_PWM_CTL2); @@ -304,6 +312,8 @@ static void i915_restore_display(struct drm_device *dev) I915_WRITE(PP_CONTROL, dev_priv->regfile.savePP_CONTROL); } + spin_unlock_irqrestore(&dev_priv->backlight.lock, flags); + /* only restore FBC info on the platform that supports FBC*/ intel_disable_fbc(dev); if (I915_HAS_FBC(dev)) { diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c index acab67c..464c3d7 100644 --- a/drivers/gpu/drm/i915/intel_panel.c +++ b/drivers/gpu/drm/i915/intel_panel.c @@ -138,6 +138,8 @@ static u32 i915_read_blc_pwm_ctl(struct drm_device *dev) struct drm_i915_private *dev_priv = dev->dev_private; u32 val; + WARN_ON(!spin_is_locked(&dev_priv->backlight.lock)); + /* Restore the CTL value if it lost, e.g. GPU reset */ if (HAS_PCH_SPLIT(dev_priv->dev)) { @@ -218,6 +220,9 @@ static u32 intel_panel_get_backlight(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; u32 val; + unsigned long flags; + + spin_lock_irqsave(&dev_priv->backlight.lock, flags); if (HAS_PCH_SPLIT(dev)) { val = I915_READ(BLC_PWM_CPU_CTL) & BACKLIGHT_DUTY_CYCLE_MASK; @@ -235,6 +240,9 @@ static u32 intel_panel_get_backlight(struct drm_device *dev) } val = intel_panel_compute_brightness(dev, val); + + spin_unlock_irqrestore(&dev_priv->backlight.lock, flags); + DRM_DEBUG_DRIVER("get backlight PWM = %d\n", val); return val; } @@ -282,11 +290,14 @@ void intel_panel_set_backlight(struct drm_device *dev, u32 level, u32 max) { struct drm_i915_private *dev_priv = dev->dev_private; u32 freq; + unsigned long flags; + + spin_lock_irqsave(&dev_priv->backlight.lock, flags); freq = intel_panel_get_max_backlight(dev); if (!freq) { /* we are screwed, bail out */ - return; + goto out; } /* scale to hardware */ @@ -298,11 +309,16 @@ void intel_panel_set_backlight(struct drm_device *dev, u32 level, u32 max) if (dev_priv->backlight.enabled) intel_panel_actually_set_backlight(dev, level); +out: + spin_unlock_irqrestore(&dev_priv->backlight.lock, flags); } void intel_panel_disable_backlight(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; + unsigned long flags; + + spin_lock_irqsave(&dev_priv->backlight.lock, flags); dev_priv->backlight.enabled = false; intel_panel_actually_set_backlight(dev, 0); @@ -320,12 +336,17 @@ void intel_panel_disable_backlight(struct drm_device *dev) I915_WRITE(BLC_PWM_PCH_CTL1, tmp); } } + + spin_unlock_irqrestore(&dev_priv->backlight.lock, flags); } void intel_panel_enable_backlight(struct drm_device *dev, enum pipe pipe) { struct drm_i915_private *dev_priv = dev->dev_private; + unsigned long flags; + + spin_lock_irqsave(&dev_priv->backlight.lock, flags); if (dev_priv->backlight.level == 0) { dev_priv->backlight.level = intel_panel_get_max_backlight(dev); @@ -375,6 +396,8 @@ set_level: */ dev_priv->backlight.enabled = true; intel_panel_actually_set_backlight(dev, dev_priv->backlight.level); + + spin_unlock_irqrestore(&dev_priv->backlight.lock, flags); } static void intel_panel_init_backlight(struct drm_device *dev) @@ -432,13 +455,18 @@ int intel_panel_setup_backlight(struct drm_connector *connector) struct drm_device *dev = connector->dev; struct drm_i915_private *dev_priv = dev->dev_private; struct backlight_properties props; + unsigned long flags; intel_panel_init_backlight(dev); memset(&props, 0, sizeof(props)); props.type = BACKLIGHT_RAW; props.brightness = dev_priv->backlight.level; + + spin_lock_irqsave(&dev_priv->backlight.lock, flags); props.max_brightness = intel_panel_get_max_backlight(dev); + spin_unlock_irqrestore(&dev_priv->backlight.lock, flags); + if (props.max_brightness == 0) { DRM_DEBUG_DRIVER("Failed to get maximum backlight value\n"); return -ENODEV;
Backlight data and registers are fiddled through LVDS/eDP modeset enable/disable hooks, backlight sysfs files, asle interrupts, and register save/restore. Protect the backlight related registers and driver private fields using a spinlock. The locking in register save/restore covers a little more than is strictly necessary, including non-modeset case, for simplicity. v2: Cover register access, save/restore, i915_read_blc_pwm_ctl() and code paths leading there. Signed-off-by: Jani Nikula <jani.nikula@intel.com> --- drivers/gpu/drm/i915/i915_dma.c | 1 + drivers/gpu/drm/i915/i915_drv.h | 1 + drivers/gpu/drm/i915/i915_suspend.c | 10 ++++++++++ drivers/gpu/drm/i915/intel_panel.c | 30 +++++++++++++++++++++++++++++- 4 files changed, 41 insertions(+), 1 deletion(-)