[PATCH 0/4] drm/i915: backlight locking, cleanup

[PATCH 0/4] drm/i915: backlight locking, cleanup

[Jani Nikula]

Patches 1-2 are for locking, 3-4 are related only by being backlight fixes.

I haven't tested these much...

BR,
Jani.

Jani Nikula (4):
  drm/i915: keep max backlight internal to intel_panel.c
  drm/i915: protect backlight registers and data with a spinlock
  drm/i915: ensure single initialization and cleanup of backlight
    device
  drm/i915: hsw backlight registers need transcoder instead of pipe

 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_display.c  |    3 ++
 drivers/gpu/drm/i915/intel_dp.c       |    5 +-
 drivers/gpu/drm/i915/intel_drv.h      |    4 +-
 drivers/gpu/drm/i915/intel_lvds.c     |    1 -
 drivers/gpu/drm/i915/intel_opregion.c |    4 +-
 drivers/gpu/drm/i915/intel_panel.c    |   90 ++++++++++++++++++++++++---------
 9 files changed, 85 insertions(+), 34 deletions(-)

-- 
1.7.9.5

[PATCH 1/4] drm/i915: keep max backlight internal to intel_panel.c

[Jani Nikula]

In preparation of adding locking to backlight, make max backlight value
(the modulation frequency the PWM duty cycle value must not exceed)
internal to intel_panel.c.

Have intel_panel_set_backlight() accept a caller defined range for level,
and scale input to max backlight value internally.

Clean up intel_panel_get_max_backlight() and usage internally.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/intel_drv.h      |    4 +--
 drivers/gpu/drm/i915/intel_opregion.c |    4 +--
 drivers/gpu/drm/i915/intel_panel.c    |   51 +++++++++++++++++++--------------
 3 files changed, 32 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index d7bd031..d2238d1 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -520,8 +520,8 @@ extern void intel_pch_panel_fitting(struct drm_device *dev,
 				    int fitting_mode,
 				    const struct drm_display_mode *mode,
 				    struct drm_display_mode *adjusted_mode);
-extern u32 intel_panel_get_max_backlight(struct drm_device *dev);
-extern void intel_panel_set_backlight(struct drm_device *dev, u32 level);
+extern void intel_panel_set_backlight(struct drm_device *dev,
+				      u32 level, u32 max);
 extern int intel_panel_setup_backlight(struct drm_connector *connector);
 extern void intel_panel_enable_backlight(struct drm_device *dev,
 					 enum pipe pipe);
diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
index 4d33874..42faa58 100644
--- a/drivers/gpu/drm/i915/intel_opregion.c
+++ b/drivers/gpu/drm/i915/intel_opregion.c
@@ -152,7 +152,6 @@ static u32 asle_set_backlight(struct drm_device *dev, u32 bclp)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct opregion_asle __iomem *asle = dev_priv->opregion.asle;
-	u32 max;
 
 	DRM_DEBUG_DRIVER("bclp = 0x%08x\n", bclp);
 
@@ -163,8 +162,7 @@ static u32 asle_set_backlight(struct drm_device *dev, u32 bclp)
 	if (bclp > 255)
 		return ASLE_BACKLIGHT_FAILED;
 
-	max = intel_panel_get_max_backlight(dev);
-	intel_panel_set_backlight(dev, bclp * max / 255);
+	intel_panel_set_backlight(dev, bclp, 255);
 	iowrite32((bclp*0x64)/0xff | ASLE_CBLV_VALID, &asle->cblv);
 
 	return 0;
diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
index 7874cec..acab67c 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -130,6 +130,9 @@ static int is_backlight_combination_mode(struct drm_device *dev)
 	return 0;
 }
 
+/* XXX: query mode clock or hardware clock and program max PWM appropriately
+ * when it's 0.
+ */
 static u32 i915_read_blc_pwm_ctl(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -164,7 +167,7 @@ static u32 i915_read_blc_pwm_ctl(struct drm_device *dev)
 	return val;
 }
 
-static u32 _intel_panel_get_max_backlight(struct drm_device *dev)
+static u32 intel_panel_get_max_backlight(struct drm_device *dev)
 {
 	u32 max;
 
@@ -182,23 +185,8 @@ static u32 _intel_panel_get_max_backlight(struct drm_device *dev)
 			max *= 0xff;
 	}
 
-	return max;
-}
-
-u32 intel_panel_get_max_backlight(struct drm_device *dev)
-{
-	u32 max;
-
-	max = _intel_panel_get_max_backlight(dev);
-	if (max == 0) {
-		/* XXX add code here to query mode clock or hardware clock
-		 * and program max PWM appropriately.
-		 */
-		pr_warn_once("fixme: max PWM is zero\n");
-		return 1;
-	}
-
 	DRM_DEBUG_DRIVER("max backlight PWM = %d\n", max);
+
 	return max;
 }
 
@@ -217,8 +205,11 @@ static u32 intel_panel_compute_brightness(struct drm_device *dev, u32 val)
 		return val;
 
 	if (i915_panel_invert_brightness > 0 ||
-	    dev_priv->quirks & QUIRK_INVERT_BRIGHTNESS)
-		return intel_panel_get_max_backlight(dev) - val;
+	    dev_priv->quirks & QUIRK_INVERT_BRIGHTNESS) {
+		u32 max = intel_panel_get_max_backlight(dev);
+		if (max)
+			return max - val;
+	}
 
 	return val;
 }
@@ -270,6 +261,10 @@ static void intel_panel_actually_set_backlight(struct drm_device *dev, u32 level
 		u32 max = intel_panel_get_max_backlight(dev);
 		u8 lbpc;
 
+		/* we're screwed, but keep behaviour backwards compatible */
+		if (!max)
+			max = 1;
+
 		lbpc = level * 0xfe / max + 1;
 		level /= lbpc;
 		pci_write_config_byte(dev->pdev, PCI_LBPC, lbpc);
@@ -282,9 +277,20 @@ static void intel_panel_actually_set_backlight(struct drm_device *dev, u32 level
 	I915_WRITE(BLC_PWM_CTL, tmp | level);
 }
 
-void intel_panel_set_backlight(struct drm_device *dev, u32 level)
+/* set backlight brightness to level in range [0..max] */
+void intel_panel_set_backlight(struct drm_device *dev, u32 level, u32 max)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	u32 freq;
+
+	freq = intel_panel_get_max_backlight(dev);
+	if (!freq) {
+		/* we are screwed, bail out */
+		return;
+	}
+
+	/* scale to hardware */
+	level = level * freq / max;
 
 	dev_priv->backlight.level = level;
 	if (dev_priv->backlight.device)
@@ -405,7 +411,8 @@ intel_panel_detect(struct drm_device *dev)
 static int intel_panel_update_status(struct backlight_device *bd)
 {
 	struct drm_device *dev = bl_get_data(bd);
-	intel_panel_set_backlight(dev, bd->props.brightness);
+	intel_panel_set_backlight(dev, bd->props.brightness,
+				  bd->props.max_brightness);
 	return 0;
 }
 
@@ -431,7 +438,7 @@ int intel_panel_setup_backlight(struct drm_connector *connector)
 	memset(&props, 0, sizeof(props));
 	props.type = BACKLIGHT_RAW;
 	props.brightness = dev_priv->backlight.level;
-	props.max_brightness = _intel_panel_get_max_backlight(dev);
+	props.max_brightness = intel_panel_get_max_backlight(dev);
 	if (props.max_brightness == 0) {
 		DRM_DEBUG_DRIVER("Failed to get maximum backlight value\n");
 		return -ENODEV;
-- 
1.7.9.5

Re: [PATCH 1/4] drm/i915: keep max backlight internal to intel_panel.c

[Chris Wilson]

On Fri, Apr 12, 2013 at 03:18:36PM +0300, Jani Nikula wrote:
> In preparation of adding locking to backlight, make max backlight value
> (the modulation frequency the PWM duty cycle value must not exceed)
> internal to intel_panel.c.
> 
> Have intel_panel_set_backlight() accept a caller defined range for level,
> and scale input to max backlight value internally.
> 
> Clean up intel_panel_get_max_backlight() and usage internally.
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>

Nice idea,
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH 2/4] drm/i915: protect backlight registers and data with a spinlock

[Jani Nikula]

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(-)

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;
-- 
1.7.9.5

Re: [PATCH 2/4] drm/i915: protect backlight registers and data with a spinlock

[Chris Wilson]

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

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 2/4] drm/i915: protect backlight registers and data with a spinlock

[Jani Nikula]

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.

Re: [PATCH 2/4] drm/i915: protect backlight registers and data with a spinlock

[Daniel Vetter]

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

Re: [PATCH 2/4] drm/i915: protect backlight registers and data with a spinlock

[Chris Wilson]

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

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 2/4] drm/i915: protect backlight registers and data with a spinlock

[Daniel Vetter]

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

Re: [PATCH 2/4] drm/i915: protect backlight registers and data with a spinlock

[Daniel Vetter]

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

[PATCH 3/4] drm/i915: ensure single initialization and cleanup of backlight device

[Jani Nikula]

Backlight cleanup in the eDP connector destroy callback caused the
backlight device to be removed on some systems that first initialized LVDS
and then attempted to initialize eDP. Prevent multiple backlight
initializations, and ensure backlight cleanup is only done once by moving
it to modeset cleanup.

A small wrinkle is the introduced asymmetry in backlight
setup/cleanup. This could be solved by adding refcounting, but it seems
overkill considering that there should only ever be one backlight device.

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=55701
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c |    3 +++
 drivers/gpu/drm/i915/intel_dp.c      |    5 +----
 drivers/gpu/drm/i915/intel_lvds.c    |    1 -
 drivers/gpu/drm/i915/intel_panel.c   |    7 ++++++-
 4 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 457a0a0..712b0af 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -9419,6 +9419,9 @@ void intel_modeset_cleanup(struct drm_device *dev)
 	/* flush any delayed tasks or pending work */
 	flush_scheduled_work();
 
+	/* destroy backlight, if any, before the connectors */
+	intel_panel_destroy_backlight(dev);
+
 	drm_mode_config_cleanup(dev);
 
 	intel_cleanup_overlay(dev);
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 173add1..8845e82 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -2474,17 +2474,14 @@ done:
 static void
 intel_dp_destroy(struct drm_connector *connector)
 {
-	struct drm_device *dev = connector->dev;
 	struct intel_dp *intel_dp = intel_attached_dp(connector);
 	struct intel_connector *intel_connector = to_intel_connector(connector);
 
 	if (!IS_ERR_OR_NULL(intel_connector->edid))
 		kfree(intel_connector->edid);
 
-	if (is_edp(intel_dp)) {
-		intel_panel_destroy_backlight(dev);
+	if (is_edp(intel_dp))
 		intel_panel_fini(&intel_connector->panel);
-	}
 
 	drm_sysfs_connector_remove(connector);
 	drm_connector_cleanup(connector);
diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
index ca2d903..f36f1ba 100644
--- a/drivers/gpu/drm/i915/intel_lvds.c
+++ b/drivers/gpu/drm/i915/intel_lvds.c
@@ -631,7 +631,6 @@ static void intel_lvds_destroy(struct drm_connector *connector)
 	if (!IS_ERR_OR_NULL(lvds_connector->base.edid))
 		kfree(lvds_connector->base.edid);
 
-	intel_panel_destroy_backlight(connector->dev);
 	intel_panel_fini(&lvds_connector->base.panel);
 
 	drm_sysfs_connector_remove(connector);
diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
index 464c3d7..5d3e9d7 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -459,6 +459,9 @@ int intel_panel_setup_backlight(struct drm_connector *connector)
 
 	intel_panel_init_backlight(dev);
 
+	if (WARN_ON(dev_priv->backlight.device))
+		return -ENODEV;
+
 	memset(&props, 0, sizeof(props));
 	props.type = BACKLIGHT_RAW;
 	props.brightness = dev_priv->backlight.level;
@@ -488,8 +491,10 @@ int intel_panel_setup_backlight(struct drm_connector *connector)
 void intel_panel_destroy_backlight(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	if (dev_priv->backlight.device)
+	if (dev_priv->backlight.device) {
 		backlight_device_unregister(dev_priv->backlight.device);
+		dev_priv->backlight.device = NULL;
+	}
 }
 #else
 int intel_panel_setup_backlight(struct drm_connector *connector)
-- 
1.7.9.5

Re: [PATCH 3/4] drm/i915: ensure single initialization and cleanup of backlight device

[Jani Nikula]

On Fri, 12 Apr 2013, Jani Nikula <jani.nikula@intel.com> wrote:
> Backlight cleanup in the eDP connector destroy callback caused the
> backlight device to be removed on some systems that first initialized LVDS
> and then attempted to initialize eDP. Prevent multiple backlight
> initializations, and ensure backlight cleanup is only done once by moving
> it to modeset cleanup.
>
> A small wrinkle is the introduced asymmetry in backlight
> setup/cleanup. This could be solved by adding refcounting, but it seems
> overkill considering that there should only ever be one backlight device.
>
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=55701

Via bugzilla,

Tested-by: Peter Verthez <peter.verthez@skynet.be>

CC: Stable?

> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c |    3 +++
>  drivers/gpu/drm/i915/intel_dp.c      |    5 +----
>  drivers/gpu/drm/i915/intel_lvds.c    |    1 -
>  drivers/gpu/drm/i915/intel_panel.c   |    7 ++++++-
>  4 files changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 457a0a0..712b0af 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -9419,6 +9419,9 @@ void intel_modeset_cleanup(struct drm_device *dev)
>  	/* flush any delayed tasks or pending work */
>  	flush_scheduled_work();
>  
> +	/* destroy backlight, if any, before the connectors */
> +	intel_panel_destroy_backlight(dev);
> +
>  	drm_mode_config_cleanup(dev);
>  
>  	intel_cleanup_overlay(dev);
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 173add1..8845e82 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -2474,17 +2474,14 @@ done:
>  static void
>  intel_dp_destroy(struct drm_connector *connector)
>  {
> -	struct drm_device *dev = connector->dev;
>  	struct intel_dp *intel_dp = intel_attached_dp(connector);
>  	struct intel_connector *intel_connector = to_intel_connector(connector);
>  
>  	if (!IS_ERR_OR_NULL(intel_connector->edid))
>  		kfree(intel_connector->edid);
>  
> -	if (is_edp(intel_dp)) {
> -		intel_panel_destroy_backlight(dev);
> +	if (is_edp(intel_dp))
>  		intel_panel_fini(&intel_connector->panel);
> -	}
>  
>  	drm_sysfs_connector_remove(connector);
>  	drm_connector_cleanup(connector);
> diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
> index ca2d903..f36f1ba 100644
> --- a/drivers/gpu/drm/i915/intel_lvds.c
> +++ b/drivers/gpu/drm/i915/intel_lvds.c
> @@ -631,7 +631,6 @@ static void intel_lvds_destroy(struct drm_connector *connector)
>  	if (!IS_ERR_OR_NULL(lvds_connector->base.edid))
>  		kfree(lvds_connector->base.edid);
>  
> -	intel_panel_destroy_backlight(connector->dev);
>  	intel_panel_fini(&lvds_connector->base.panel);
>  
>  	drm_sysfs_connector_remove(connector);
> diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
> index 464c3d7..5d3e9d7 100644
> --- a/drivers/gpu/drm/i915/intel_panel.c
> +++ b/drivers/gpu/drm/i915/intel_panel.c
> @@ -459,6 +459,9 @@ int intel_panel_setup_backlight(struct drm_connector *connector)
>  
>  	intel_panel_init_backlight(dev);
>  
> +	if (WARN_ON(dev_priv->backlight.device))
> +		return -ENODEV;
> +
>  	memset(&props, 0, sizeof(props));
>  	props.type = BACKLIGHT_RAW;
>  	props.brightness = dev_priv->backlight.level;
> @@ -488,8 +491,10 @@ int intel_panel_setup_backlight(struct drm_connector *connector)
>  void intel_panel_destroy_backlight(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> -	if (dev_priv->backlight.device)
> +	if (dev_priv->backlight.device) {
>  		backlight_device_unregister(dev_priv->backlight.device);
> +		dev_priv->backlight.device = NULL;
> +	}
>  }
>  #else
>  int intel_panel_setup_backlight(struct drm_connector *connector)
> -- 
> 1.7.9.5

Re: [PATCH 3/4] drm/i915: ensure single initialization and cleanup of backlight device

[Daniel Vetter]

On Mon, Apr 15, 2013 at 09:33:13AM +0300, Jani Nikula wrote:
> On Fri, 12 Apr 2013, Jani Nikula <jani.nikula@intel.com> wrote:
> > Backlight cleanup in the eDP connector destroy callback caused the
> > backlight device to be removed on some systems that first initialized LVDS
> > and then attempted to initialize eDP. Prevent multiple backlight
> > initializations, and ensure backlight cleanup is only done once by moving
> > it to modeset cleanup.
> >
> > A small wrinkle is the introduced asymmetry in backlight
> > setup/cleanup. This could be solved by adding refcounting, but it seems
> > overkill considering that there should only ever be one backlight device.
> >
> > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=55701
> 
> Via bugzilla,
> 
> Tested-by: Peter Verthez <peter.verthez@skynet.be>
> 
> CC: Stable?

Yeah, agreed. Queued for -next, thanks for the patch.
-Daniel
> 
> > Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c |    3 +++
> >  drivers/gpu/drm/i915/intel_dp.c      |    5 +----
> >  drivers/gpu/drm/i915/intel_lvds.c    |    1 -
> >  drivers/gpu/drm/i915/intel_panel.c   |    7 ++++++-
> >  4 files changed, 10 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 457a0a0..712b0af 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -9419,6 +9419,9 @@ void intel_modeset_cleanup(struct drm_device *dev)
> >  	/* flush any delayed tasks or pending work */
> >  	flush_scheduled_work();
> >  
> > +	/* destroy backlight, if any, before the connectors */
> > +	intel_panel_destroy_backlight(dev);
> > +
> >  	drm_mode_config_cleanup(dev);
> >  
> >  	intel_cleanup_overlay(dev);
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index 173add1..8845e82 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -2474,17 +2474,14 @@ done:
> >  static void
> >  intel_dp_destroy(struct drm_connector *connector)
> >  {
> > -	struct drm_device *dev = connector->dev;
> >  	struct intel_dp *intel_dp = intel_attached_dp(connector);
> >  	struct intel_connector *intel_connector = to_intel_connector(connector);
> >  
> >  	if (!IS_ERR_OR_NULL(intel_connector->edid))
> >  		kfree(intel_connector->edid);
> >  
> > -	if (is_edp(intel_dp)) {
> > -		intel_panel_destroy_backlight(dev);
> > +	if (is_edp(intel_dp))
> >  		intel_panel_fini(&intel_connector->panel);
> > -	}
> >  
> >  	drm_sysfs_connector_remove(connector);
> >  	drm_connector_cleanup(connector);
> > diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
> > index ca2d903..f36f1ba 100644
> > --- a/drivers/gpu/drm/i915/intel_lvds.c
> > +++ b/drivers/gpu/drm/i915/intel_lvds.c
> > @@ -631,7 +631,6 @@ static void intel_lvds_destroy(struct drm_connector *connector)
> >  	if (!IS_ERR_OR_NULL(lvds_connector->base.edid))
> >  		kfree(lvds_connector->base.edid);
> >  
> > -	intel_panel_destroy_backlight(connector->dev);
> >  	intel_panel_fini(&lvds_connector->base.panel);
> >  
> >  	drm_sysfs_connector_remove(connector);
> > diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
> > index 464c3d7..5d3e9d7 100644
> > --- a/drivers/gpu/drm/i915/intel_panel.c
> > +++ b/drivers/gpu/drm/i915/intel_panel.c
> > @@ -459,6 +459,9 @@ int intel_panel_setup_backlight(struct drm_connector *connector)
> >  
> >  	intel_panel_init_backlight(dev);
> >  
> > +	if (WARN_ON(dev_priv->backlight.device))
> > +		return -ENODEV;
> > +
> >  	memset(&props, 0, sizeof(props));
> >  	props.type = BACKLIGHT_RAW;
> >  	props.brightness = dev_priv->backlight.level;
> > @@ -488,8 +491,10 @@ int intel_panel_setup_backlight(struct drm_connector *connector)
> >  void intel_panel_destroy_backlight(struct drm_device *dev)
> >  {
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > -	if (dev_priv->backlight.device)
> > +	if (dev_priv->backlight.device) {
> >  		backlight_device_unregister(dev_priv->backlight.device);
> > +		dev_priv->backlight.device = NULL;
> > +	}
> >  }
> >  #else
> >  int intel_panel_setup_backlight(struct drm_connector *connector)
> > -- 
> > 1.7.9.5
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH 4/4] drm/i915: hsw backlight registers need transcoder instead of pipe

[Jani Nikula]

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/intel_panel.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
index 5d3e9d7..0362f5c 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -344,6 +344,8 @@ void intel_panel_enable_backlight(struct drm_device *dev,
 				  enum pipe pipe)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	enum transcoder cpu_transcoder =
+		intel_pipe_to_cpu_transcoder(dev_priv, pipe);
 	unsigned long flags;
 
 	spin_lock_irqsave(&dev_priv->backlight.lock, flags);
@@ -374,7 +376,7 @@ void intel_panel_enable_backlight(struct drm_device *dev,
 		else
 			tmp &= ~BLM_PIPE_SELECT;
 
-		tmp |= BLM_PIPE(pipe);
+		tmp |= BLM_PIPE(cpu_transcoder);
 		tmp &= ~BLM_PWM_ENABLE;
 
 		I915_WRITE(reg, tmp);
-- 
1.7.9.5

Re: [PATCH 4/4] drm/i915: hsw backlight registers need transcoder instead of pipe

[Imre Deak]

On Fri, 2013-04-12 at 15:18 +0300, Jani Nikula wrote:
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_panel.c |    4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
> index 5d3e9d7..0362f5c 100644
> --- a/drivers/gpu/drm/i915/intel_panel.c
> +++ b/drivers/gpu/drm/i915/intel_panel.c
> @@ -344,6 +344,8 @@ void intel_panel_enable_backlight(struct drm_device *dev,
>  				  enum pipe pipe)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> +	enum transcoder cpu_transcoder =
> +		intel_pipe_to_cpu_transcoder(dev_priv, pipe);
>  	unsigned long flags;
>  
>  	spin_lock_irqsave(&dev_priv->backlight.lock, flags);
> @@ -374,7 +376,7 @@ void intel_panel_enable_backlight(struct drm_device *dev,
>  		else
>  			tmp &= ~BLM_PIPE_SELECT;
>  
> -		tmp |= BLM_PIPE(pipe);
> +		tmp |= BLM_PIPE(cpu_transcoder);

Imo BLM_PIPE would be clearer checking for TRANSCODER_EDP explicitly,
but the code works anyway, so:
Reviewed-by: Imre Deak <imre.deak@intel.com>

>  		tmp &= ~BLM_PWM_ENABLE;
>  
>  		I915_WRITE(reg, tmp);

Re: [PATCH 4/4] drm/i915: hsw backlight registers need transcoder instead of pipe

[Daniel Vetter]

On Thu, Apr 25, 2013 at 04:14:23PM +0300, Imre Deak wrote:
> On Fri, 2013-04-12 at 15:18 +0300, Jani Nikula wrote:
> > Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_panel.c |    4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
> > index 5d3e9d7..0362f5c 100644
> > --- a/drivers/gpu/drm/i915/intel_panel.c
> > +++ b/drivers/gpu/drm/i915/intel_panel.c
> > @@ -344,6 +344,8 @@ void intel_panel_enable_backlight(struct drm_device *dev,
> >  				  enum pipe pipe)
> >  {
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > +	enum transcoder cpu_transcoder =
> > +		intel_pipe_to_cpu_transcoder(dev_priv, pipe);
> >  	unsigned long flags;
> >  
> >  	spin_lock_irqsave(&dev_priv->backlight.lock, flags);
> > @@ -374,7 +376,7 @@ void intel_panel_enable_backlight(struct drm_device *dev,
> >  		else
> >  			tmp &= ~BLM_PIPE_SELECT;
> >  
> > -		tmp |= BLM_PIPE(pipe);
> > +		tmp |= BLM_PIPE(cpu_transcoder);
> 
> Imo BLM_PIPE would be clearer checking for TRANSCODER_EDP explicitly,
> but the code works anyway, so:
> Reviewed-by: Imre Deak <imre.deak@intel.com>

Atm all the difference between pipe and cpu_transcoder is about transcoder
edp. So this feels explicit enough for me.

Queued for -next, thanks for the patch.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 4/4] drm/i915: hsw backlight registers need transcoder instead of pipe

[Daniel Vetter]

On Thu, Apr 25, 2013 at 03:34:26PM +0200, Daniel Vetter wrote:
> On Thu, Apr 25, 2013 at 04:14:23PM +0300, Imre Deak wrote:
> > On Fri, 2013-04-12 at 15:18 +0300, Jani Nikula wrote:
> > > Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_panel.c |    4 +++-
> > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
> > > index 5d3e9d7..0362f5c 100644
> > > --- a/drivers/gpu/drm/i915/intel_panel.c
> > > +++ b/drivers/gpu/drm/i915/intel_panel.c
> > > @@ -344,6 +344,8 @@ void intel_panel_enable_backlight(struct drm_device *dev,
> > >  				  enum pipe pipe)
> > >  {
> > >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > > +	enum transcoder cpu_transcoder =
> > > +		intel_pipe_to_cpu_transcoder(dev_priv, pipe);
> > >  	unsigned long flags;
> > >  
> > >  	spin_lock_irqsave(&dev_priv->backlight.lock, flags);
> > > @@ -374,7 +376,7 @@ void intel_panel_enable_backlight(struct drm_device *dev,
> > >  		else
> > >  			tmp &= ~BLM_PIPE_SELECT;
> > >  
> > > -		tmp |= BLM_PIPE(pipe);
> > > +		tmp |= BLM_PIPE(cpu_transcoder);
> > 
> > Imo BLM_PIPE would be clearer checking for TRANSCODER_EDP explicitly,
> > but the code works anyway, so:
> > Reviewed-by: Imre Deak <imre.deak@intel.com>
> 
> Atm all the difference between pipe and cpu_transcoder is about transcoder
> edp. So this feels explicit enough for me.

Imre pointed out that his concern is about the truncation, since for eDP
this evaluates to (0xf << 29). Which truncates to the right value, but I
agree it's a bit too much working-by-accident. Dropped the patch again.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH v2] drm/i915: hsw backlight registers need transcoder instead of pipe

[Jani Nikula]

v2: Make TRANSCODER_EDP handling more explicit. (Imre)

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h    |    4 ++++
 drivers/gpu/drm/i915/intel_panel.c |    7 ++++++-
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 60e0e19..e79669f 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -2087,6 +2087,10 @@
 #define   BLM_PIPE_A			(0 << 29)
 #define   BLM_PIPE_B			(1 << 29)
 #define   BLM_PIPE_C			(2 << 29) /* ivb + */
+#define   BLM_TRANSCODER_A		BLM_PIPE_A /* hsw */
+#define   BLM_TRANSCODER_B		BLM_PIPE_B
+#define   BLM_TRANSCODER_C		BLM_PIPE_C
+#define   BLM_TRANSCODER_EDP		(3 << 29)
 #define   BLM_PIPE(pipe)		((pipe) << 29)
 #define   BLM_POLARITY_I965		(1 << 28) /* gen4 only */
 #define   BLM_PHASE_IN_INTERUPT_STATUS	(1 << 26)
diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
index 5d3e9d7..7f6141d 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -344,6 +344,8 @@ void intel_panel_enable_backlight(struct drm_device *dev,
 				  enum pipe pipe)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	enum transcoder cpu_transcoder =
+		intel_pipe_to_cpu_transcoder(dev_priv, pipe);
 	unsigned long flags;
 
 	spin_lock_irqsave(&dev_priv->backlight.lock, flags);
@@ -374,7 +376,10 @@ void intel_panel_enable_backlight(struct drm_device *dev,
 		else
 			tmp &= ~BLM_PIPE_SELECT;
 
-		tmp |= BLM_PIPE(pipe);
+		if (cpu_transcoder == TRANSCODER_EDP)
+			tmp |= BLM_TRANSCODER_EDP;
+		else
+			tmp |= BLM_PIPE(cpu_transcoder);
 		tmp &= ~BLM_PWM_ENABLE;
 
 		I915_WRITE(reg, tmp);
-- 
1.7.9.5

Re: [PATCH v2] drm/i915: hsw backlight registers need transcoder instead of pipe

[Imre Deak]

On Thu, 2013-04-25 at 16:49 +0300, Jani Nikula wrote:
> v2: Make TRANSCODER_EDP handling more explicit. (Imre)
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>

Looks ok, so still applies my r-b.

> --
>  drivers/gpu/drm/i915/i915_reg.h    |    4 ++++
>  drivers/gpu/drm/i915/intel_panel.c |    7 ++++++-
>  2 files changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 60e0e19..e79669f 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -2087,6 +2087,10 @@
>  #define   BLM_PIPE_A			(0 << 29)
>  #define   BLM_PIPE_B			(1 << 29)
>  #define   BLM_PIPE_C			(2 << 29) /* ivb + */
> +#define   BLM_TRANSCODER_A		BLM_PIPE_A /* hsw */
> +#define   BLM_TRANSCODER_B		BLM_PIPE_B
> +#define   BLM_TRANSCODER_C		BLM_PIPE_C
> +#define   BLM_TRANSCODER_EDP		(3 << 29)
>  #define   BLM_PIPE(pipe)		((pipe) << 29)
>  #define   BLM_POLARITY_I965		(1 << 28) /* gen4 only */
>  #define   BLM_PHASE_IN_INTERUPT_STATUS	(1 << 26)
> diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
> index 5d3e9d7..7f6141d 100644
> --- a/drivers/gpu/drm/i915/intel_panel.c
> +++ b/drivers/gpu/drm/i915/intel_panel.c
> @@ -344,6 +344,8 @@ void intel_panel_enable_backlight(struct drm_device *dev,
>  				  enum pipe pipe)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> +	enum transcoder cpu_transcoder =
> +		intel_pipe_to_cpu_transcoder(dev_priv, pipe);
>  	unsigned long flags;
>  
>  	spin_lock_irqsave(&dev_priv->backlight.lock, flags);
> @@ -374,7 +376,10 @@ void intel_panel_enable_backlight(struct drm_device *dev,
>  		else
>  			tmp &= ~BLM_PIPE_SELECT;
>  
> -		tmp |= BLM_PIPE(pipe);
> +		if (cpu_transcoder == TRANSCODER_EDP)
> +			tmp |= BLM_TRANSCODER_EDP;
> +		else
> +			tmp |= BLM_PIPE(cpu_transcoder);
>  		tmp &= ~BLM_PWM_ENABLE;
>  
>  		I915_WRITE(reg, tmp);

Re: [PATCH v2] drm/i915: hsw backlight registers need transcoder instead of pipe

[Daniel Vetter]

On Thu, Apr 25, 2013 at 05:07:55PM +0300, Imre Deak wrote:
> On Thu, 2013-04-25 at 16:49 +0300, Jani Nikula wrote:
> > v2: Make TRANSCODER_EDP handling more explicit. (Imre)
> > 
> > Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> 
> Looks ok, so still applies my r-b.

Queued for -next, thanks for the patch.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch