[PATCH 0/6] drm/i915: VLV/CHV backlight fixes

[PATCH 0/6] drm/i915: VLV/CHV backlight fixes

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

This series tries to fix the initial backlight setup on VLV/CHV so that
we read out the hardware state from the correct pipe's BLC registers.
Makes for a better experience on BSW where the BIOS always uses pipe B
for the eDP port.

I also noticed some weirdness with systemd due to the early backlight
device registration, so I went ahead and moved that to occur after the
modeset init has finished. That should fix the race between our state
readout and userspace backlight frobbery.

Ville Syrjälä (6):
  drm/i915: Warn if trying to poke a VLV backlight on invalid pipe
  drm/i915: Catch INVALID_PIPE in vlv_get_backlight()
  drm/i915: Don't deref NULL crtc in intel_get_pipe_from_connector()
  drm/i915: Pass the current pipe from eDP init to backlight setup
  drm/i915: Register the backlight device after the modeset init
  drm/i915: Remove most INVALID_PIPE checks from VLV backlight code

 drivers/gpu/drm/i915/i915_drv.h      |  2 +-
 drivers/gpu/drm/i915/intel_display.c |  6 ++-
 drivers/gpu/drm/i915/intel_dp.c      | 22 +++++++++-
 drivers/gpu/drm/i915/intel_drv.h     |  5 ++-
 drivers/gpu/drm/i915/intel_lvds.c    |  2 +-
 drivers/gpu/drm/i915/intel_panel.c   | 79 +++++++++++++++++++++++++-----------
 6 files changed, 87 insertions(+), 29 deletions(-)

-- 
2.0.4

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

[PATCH 1/6] drm/i915: Warn if trying to poke a VLV backlight on invalid pipe

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

VLV/CHV have backlight controls only on pipes A and B. Bail out
without touching registers that don't exist, and print a warning.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_panel.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
index e18b3f4..ef646b1 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -521,6 +521,9 @@ static u32 _vlv_get_backlight(struct drm_device *dev, enum pipe pipe)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
+	if (WARN_ON(pipe != PIPE_A && pipe != PIPE_B))
+		return 0;
+
 	return I915_READ(VLV_BLC_PWM_CTL(pipe)) & BACKLIGHT_DUTY_CYCLE_MASK;
 }
 
@@ -602,6 +605,9 @@ static void vlv_set_backlight(struct intel_connector *connector, u32 level)
 	enum pipe pipe = intel_get_pipe_from_connector(connector);
 	u32 tmp;
 
+	if (WARN_ON(pipe != PIPE_A && pipe != PIPE_B))
+		return;
+
 	tmp = I915_READ(VLV_BLC_PWM_CTL(pipe)) & ~BACKLIGHT_DUTY_CYCLE_MASK;
 	I915_WRITE(VLV_BLC_PWM_CTL(pipe), tmp | level);
 }
@@ -717,6 +723,9 @@ static void vlv_disable_backlight(struct intel_connector *connector)
 	enum pipe pipe = intel_get_pipe_from_connector(connector);
 	u32 tmp;
 
+	if (WARN_ON(pipe != PIPE_A && pipe != PIPE_B))
+		return;
+
 	intel_panel_actually_set_backlight(connector, 0);
 
 	tmp = I915_READ(VLV_BLC_PWM_CTL2(pipe));
@@ -906,6 +915,9 @@ static void vlv_enable_backlight(struct intel_connector *connector)
 	enum pipe pipe = intel_get_pipe_from_connector(connector);
 	u32 ctl, ctl2;
 
+	if (WARN_ON(pipe != PIPE_A && pipe != PIPE_B))
+		return;
+
 	ctl2 = I915_READ(VLV_BLC_PWM_CTL2(pipe));
 	if (ctl2 & BLM_PWM_ENABLE) {
 		DRM_DEBUG_KMS("backlight already enabled\n");
-- 
2.0.4

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

Re: [PATCH 1/6] drm/i915: Warn if trying to poke a VLV backlight on invalid pipe

[Jani Nikula]

On Fri, 07 Nov 2014, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> VLV/CHV have backlight controls only on pipes A and B. Bail out
> without touching registers that don't exist, and print a warning.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_panel.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
> index e18b3f4..ef646b1 100644
> --- a/drivers/gpu/drm/i915/intel_panel.c
> +++ b/drivers/gpu/drm/i915/intel_panel.c
> @@ -521,6 +521,9 @@ static u32 _vlv_get_backlight(struct drm_device *dev, enum pipe pipe)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  
> +	if (WARN_ON(pipe != PIPE_A && pipe != PIPE_B))
> +		return 0;
> +
>  	return I915_READ(VLV_BLC_PWM_CTL(pipe)) & BACKLIGHT_DUTY_CYCLE_MASK;
>  }
>  
> @@ -602,6 +605,9 @@ static void vlv_set_backlight(struct intel_connector *connector, u32 level)
>  	enum pipe pipe = intel_get_pipe_from_connector(connector);
>  	u32 tmp;
>  
> +	if (WARN_ON(pipe != PIPE_A && pipe != PIPE_B))
> +		return;
> +
>  	tmp = I915_READ(VLV_BLC_PWM_CTL(pipe)) & ~BACKLIGHT_DUTY_CYCLE_MASK;
>  	I915_WRITE(VLV_BLC_PWM_CTL(pipe), tmp | level);
>  }
> @@ -717,6 +723,9 @@ static void vlv_disable_backlight(struct intel_connector *connector)
>  	enum pipe pipe = intel_get_pipe_from_connector(connector);
>  	u32 tmp;
>  
> +	if (WARN_ON(pipe != PIPE_A && pipe != PIPE_B))
> +		return;
> +
>  	intel_panel_actually_set_backlight(connector, 0);
>  
>  	tmp = I915_READ(VLV_BLC_PWM_CTL2(pipe));
> @@ -906,6 +915,9 @@ static void vlv_enable_backlight(struct intel_connector *connector)
>  	enum pipe pipe = intel_get_pipe_from_connector(connector);
>  	u32 ctl, ctl2;
>  
> +	if (WARN_ON(pipe != PIPE_A && pipe != PIPE_B))
> +		return;
> +
>  	ctl2 = I915_READ(VLV_BLC_PWM_CTL2(pipe));
>  	if (ctl2 & BLM_PWM_ENABLE) {
>  		DRM_DEBUG_KMS("backlight already enabled\n");
> -- 
> 2.0.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/6] drm/i915: Catch INVALID_PIPE in vlv_get_backlight()

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

When the display is off, we can't read out the current backlight level
from the hardware since we have no pipe to do so. Catch the INVALID_PIPE
in vlv_get_backlight() rather than pass it to VLV_BLC_PWM_CTL() which
would obviously end accessing some bogus register.

This problem can be reproduced simply by reading the backlight device
actual_brightness file while the display is off.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_panel.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
index ef646b1..847d00f9 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -532,6 +532,9 @@ static u32 vlv_get_backlight(struct intel_connector *connector)
 	struct drm_device *dev = connector->base.dev;
 	enum pipe pipe = intel_get_pipe_from_connector(connector);
 
+	if (pipe == INVALID_PIPE)
+		return 0;
+
 	return _vlv_get_backlight(dev, pipe);
 }
 
-- 
2.0.4

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

Re: [PATCH 2/6] drm/i915: Catch INVALID_PIPE in vlv_get_backlight()

[Jani Nikula]

On Fri, 07 Nov 2014, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> When the display is off, we can't read out the current backlight level
> from the hardware since we have no pipe to do so. Catch the INVALID_PIPE
> in vlv_get_backlight() rather than pass it to VLV_BLC_PWM_CTL() which
> would obviously end accessing some bogus register.
>
> This problem can be reproduced simply by reading the backlight device
> actual_brightness file while the display is off.

I think we should check for panel->backlight.enabled in
intel_panel_get_backlight instead. I think it would be the right thing
to do independent of the issue you found, but I think it also fixes your
issue.

BR,
Jani.


>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_panel.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
> index ef646b1..847d00f9 100644
> --- a/drivers/gpu/drm/i915/intel_panel.c
> +++ b/drivers/gpu/drm/i915/intel_panel.c
> @@ -532,6 +532,9 @@ static u32 vlv_get_backlight(struct intel_connector *connector)
>  	struct drm_device *dev = connector->base.dev;
>  	enum pipe pipe = intel_get_pipe_from_connector(connector);
>  
> +	if (pipe == INVALID_PIPE)
> +		return 0;
> +
>  	return _vlv_get_backlight(dev, pipe);
>  }
>  
> -- 
> 2.0.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 2/6] drm/i915: Skip .get_backlight() when backlight isn't enabled

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

On VLV/CHV when the display is off, we can't read out the current
backlight level from the hardware since we have no pipe to do so.
Currently we end up reading a bigus register due to passing
INVALID_PIPE to VLV_BLC_PWM_CTL().

Skip the entire .get_backlight() call if the backlight isn't enabled
according to backlight.enabled.

This problem can be reproduced simply by reading the backlight device
actual_brightness file while the display is off.

Cc: Jani Nikula <jani.nikula@intel.com>
Suggested-by: Jani Nikula <jani.nikula@intel.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_panel.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
index ef646b1..4ec6c2f 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -539,12 +539,15 @@ static u32 intel_panel_get_backlight(struct intel_connector *connector)
 {
 	struct drm_device *dev = connector->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	u32 val;
+	struct intel_panel *panel = &connector->panel;
+	u32 val = 0;
 
 	mutex_lock(&dev_priv->backlight_lock);
 
-	val = dev_priv->display.get_backlight(connector);
-	val = intel_panel_compute_brightness(connector, val);
+	if (panel->backlight.enabled) {
+		val = dev_priv->display.get_backlight(connector);
+		val = intel_panel_compute_brightness(connector, val);
+	}
 
 	mutex_unlock(&dev_priv->backlight_lock);
 
-- 
2.0.4

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

Re: [PATCH v2 2/6] drm/i915: Skip .get_backlight() when backlight isn't enabled

[Jani Nikula]

On Fri, 07 Nov 2014, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> On VLV/CHV when the display is off, we can't read out the current
> backlight level from the hardware since we have no pipe to do so.
> Currently we end up reading a bigus register due to passing
> INVALID_PIPE to VLV_BLC_PWM_CTL().
>
> Skip the entire .get_backlight() call if the backlight isn't enabled
> according to backlight.enabled.
>
> This problem can be reproduced simply by reading the backlight device
> actual_brightness file while the display is off.
>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Suggested-by: Jani Nikula <jani.nikula@intel.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_panel.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
> index ef646b1..4ec6c2f 100644
> --- a/drivers/gpu/drm/i915/intel_panel.c
> +++ b/drivers/gpu/drm/i915/intel_panel.c
> @@ -539,12 +539,15 @@ static u32 intel_panel_get_backlight(struct intel_connector *connector)
>  {
>  	struct drm_device *dev = connector->base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> -	u32 val;
> +	struct intel_panel *panel = &connector->panel;
> +	u32 val = 0;
>  
>  	mutex_lock(&dev_priv->backlight_lock);
>  
> -	val = dev_priv->display.get_backlight(connector);
> -	val = intel_panel_compute_brightness(connector, val);
> +	if (panel->backlight.enabled) {
> +		val = dev_priv->display.get_backlight(connector);
> +		val = intel_panel_compute_brightness(connector, val);
> +	}
>  
>  	mutex_unlock(&dev_priv->backlight_lock);
>  
> -- 
> 2.0.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 3/6] drm/i915: Don't deref NULL crtc in intel_get_pipe_from_connector()

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

If the connector would have an encoder but the encoder didn't have a
crtc we might dereference a NULL crtc here. I suppose that should never
happen due to intel_sanitize_encoder(), but let's be a bit paranoid
print a warning if we ever hit this and return INVALID_PIPE to the
caller.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 9ec1ab7..b9529d0 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12074,7 +12074,7 @@ enum pipe intel_get_pipe_from_connector(struct intel_connector *connector)
 
 	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
 
-	if (!encoder)
+	if (!encoder || WARN_ON(!encoder->crtc))
 		return INVALID_PIPE;
 
 	return to_intel_crtc(encoder->crtc)->pipe;
-- 
2.0.4

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

Re: [PATCH 3/6] drm/i915: Don't deref NULL crtc in intel_get_pipe_from_connector()

[Jani Nikula]

On Fri, 07 Nov 2014, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> If the connector would have an encoder but the encoder didn't have a
> crtc we might dereference a NULL crtc here. I suppose that should never
> happen due to intel_sanitize_encoder(), but let's be a bit paranoid
> print a warning if we ever hit this and return INVALID_PIPE to the
> caller.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_display.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 9ec1ab7..b9529d0 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12074,7 +12074,7 @@ enum pipe intel_get_pipe_from_connector(struct intel_connector *connector)
>  
>  	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
>  
> -	if (!encoder)
> +	if (!encoder || WARN_ON(!encoder->crtc))
>  		return INVALID_PIPE;
>  
>  	return to_intel_crtc(encoder->crtc)->pipe;
> -- 
> 2.0.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 4/6] drm/i915: Pass the current pipe from eDP init to backlight setup

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

On VLV/CHV both pipes A and B have their own backlight control
registers. In order to correctly read out the current hardware state at
init we need to know which pipe is driving the eDP port. Pass that
information down from the eDP init code into the backlight code.

To determine the correct pipe we first look at which pipe is currently
configured in the port control register, if that look invalid we look
at which pipe's PPS is currently controlling the port, and if that
too looks invalid we just assume pipe A.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h    |  2 +-
 drivers/gpu/drm/i915/intel_dp.c    | 22 +++++++++++++++++++++-
 drivers/gpu/drm/i915/intel_drv.h   |  2 +-
 drivers/gpu/drm/i915/intel_lvds.c  |  2 +-
 drivers/gpu/drm/i915/intel_panel.c | 31 +++++++++++++++++--------------
 5 files changed, 41 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 0f00e58..65e4e29 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -510,7 +510,7 @@ struct drm_i915_display_funcs {
 	/* display clock increase/decrease */
 	/* pll clock increase/decrease */
 
-	int (*setup_backlight)(struct intel_connector *connector);
+	int (*setup_backlight)(struct intel_connector *connector, enum pipe pipe);
 	uint32_t (*get_backlight)(struct intel_connector *connector);
 	void (*set_backlight)(struct intel_connector *connector,
 			      uint32_t level);
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index ceb528f..45b53ff 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -5232,6 +5232,7 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
 	bool has_dpcd;
 	struct drm_display_mode *scan;
 	struct edid *edid;
+	enum pipe pipe = INVALID_PIPE;
 
 	intel_dp->drrs_state.type = DRRS_NOT_SUPPORTED;
 
@@ -5300,11 +5301,30 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
 	if (IS_VALLEYVIEW(dev)) {
 		intel_dp->edp_notifier.notifier_call = edp_notify_handler;
 		register_reboot_notifier(&intel_dp->edp_notifier);
+
+		/*
+		 * Figure out the current pipe for the initial backlight setup.
+		 * If the current pipe isn't valid, try the PPS pipe, and if that
+		 * fails just assume pipe A.
+		 */
+		if (IS_CHERRYVIEW(dev))
+			pipe = DP_PORT_TO_PIPE_CHV(intel_dp->DP);
+		else
+			pipe = PORT_TO_PIPE(intel_dp->DP);
+
+		if (pipe != PIPE_A && pipe != PIPE_B)
+			pipe = intel_dp->pps_pipe;
+
+		if (pipe != PIPE_A && pipe != PIPE_B)
+			pipe = PIPE_A;
+
+		DRM_DEBUG_KMS("using pipe %c for initial backlight setup\n",
+			      pipe_name(pipe));
 	}
 
 	intel_panel_init(&intel_connector->panel, fixed_mode, downclock_mode);
 	intel_connector->panel.backlight_power = intel_edp_backlight_power;
-	intel_panel_setup_backlight(connector);
+	intel_panel_setup_backlight(connector, pipe);
 
 	return true;
 }
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index cb0e9db..0ff011e 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1103,7 +1103,7 @@ void intel_gmch_panel_fitting(struct intel_crtc *crtc,
 			      int fitting_mode);
 void intel_panel_set_backlight_acpi(struct intel_connector *connector,
 				    u32 level, u32 max);
-int intel_panel_setup_backlight(struct drm_connector *connector);
+int intel_panel_setup_backlight(struct drm_connector *connector, enum pipe pipe);
 void intel_panel_enable_backlight(struct intel_connector *connector);
 void intel_panel_disable_backlight(struct intel_connector *connector);
 void intel_panel_destroy_backlight(struct drm_connector *connector);
diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
index 2b50c98..c03d457 100644
--- a/drivers/gpu/drm/i915/intel_lvds.c
+++ b/drivers/gpu/drm/i915/intel_lvds.c
@@ -1116,7 +1116,7 @@ out:
 	drm_connector_register(connector);
 
 	intel_panel_init(&intel_connector->panel, fixed_mode, downclock_mode);
-	intel_panel_setup_backlight(connector);
+	intel_panel_setup_backlight(connector, INVALID_PIPE);
 
 	return;
 
diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
index 847d00f9..f6e8d23 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -1117,7 +1117,7 @@ static u32 get_backlight_min_vbt(struct intel_connector *connector)
 		     0, panel->backlight.max);
 }
 
-static int bdw_setup_backlight(struct intel_connector *connector)
+static int bdw_setup_backlight(struct intel_connector *connector, enum pipe unused)
 {
 	struct drm_device *dev = connector->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -1143,7 +1143,7 @@ static int bdw_setup_backlight(struct intel_connector *connector)
 	return 0;
 }
 
-static int pch_setup_backlight(struct intel_connector *connector)
+static int pch_setup_backlight(struct intel_connector *connector, enum pipe unused)
 {
 	struct drm_device *dev = connector->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -1170,7 +1170,7 @@ static int pch_setup_backlight(struct intel_connector *connector)
 	return 0;
 }
 
-static int i9xx_setup_backlight(struct intel_connector *connector)
+static int i9xx_setup_backlight(struct intel_connector *connector, enum pipe unused)
 {
 	struct drm_device *dev = connector->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -1202,7 +1202,7 @@ static int i9xx_setup_backlight(struct intel_connector *connector)
 	return 0;
 }
 
-static int i965_setup_backlight(struct intel_connector *connector)
+static int i965_setup_backlight(struct intel_connector *connector, enum pipe unused)
 {
 	struct drm_device *dev = connector->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -1232,37 +1232,40 @@ static int i965_setup_backlight(struct intel_connector *connector)
 	return 0;
 }
 
-static int vlv_setup_backlight(struct intel_connector *connector)
+static int vlv_setup_backlight(struct intel_connector *connector, enum pipe pipe)
 {
 	struct drm_device *dev = connector->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_panel *panel = &connector->panel;
-	enum pipe pipe;
+	enum pipe p;
 	u32 ctl, ctl2, val;
 
-	for_each_pipe(dev_priv, pipe) {
-		u32 cur_val = I915_READ(VLV_BLC_PWM_CTL(pipe));
+	for_each_pipe(dev_priv, p) {
+		u32 cur_val = I915_READ(VLV_BLC_PWM_CTL(p));
 
 		/* Skip if the modulation freq is already set */
 		if (cur_val & ~BACKLIGHT_DUTY_CYCLE_MASK)
 			continue;
 
 		cur_val &= BACKLIGHT_DUTY_CYCLE_MASK;
-		I915_WRITE(VLV_BLC_PWM_CTL(pipe), (0xf42 << 16) |
+		I915_WRITE(VLV_BLC_PWM_CTL(p), (0xf42 << 16) |
 			   cur_val);
 	}
 
-	ctl2 = I915_READ(VLV_BLC_PWM_CTL2(PIPE_A));
+	if (WARN_ON(pipe != PIPE_A && pipe != PIPE_B))
+		return -ENODEV;
+
+	ctl2 = I915_READ(VLV_BLC_PWM_CTL2(pipe));
 	panel->backlight.active_low_pwm = ctl2 & BLM_POLARITY_I965;
 
-	ctl = I915_READ(VLV_BLC_PWM_CTL(PIPE_A));
+	ctl = I915_READ(VLV_BLC_PWM_CTL(pipe));
 	panel->backlight.max = ctl >> 16;
 	if (!panel->backlight.max)
 		return -ENODEV;
 
 	panel->backlight.min = get_backlight_min_vbt(connector);
 
-	val = _vlv_get_backlight(dev, PIPE_A);
+	val = _vlv_get_backlight(dev, pipe);
 	panel->backlight.level = intel_panel_compute_brightness(connector, val);
 
 	panel->backlight.enabled = (ctl2 & BLM_PWM_ENABLE) &&
@@ -1271,7 +1274,7 @@ static int vlv_setup_backlight(struct intel_connector *connector)
 	return 0;
 }
 
-int intel_panel_setup_backlight(struct drm_connector *connector)
+int intel_panel_setup_backlight(struct drm_connector *connector, enum pipe pipe)
 {
 	struct drm_device *dev = connector->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -1290,7 +1293,7 @@ int intel_panel_setup_backlight(struct drm_connector *connector)
 
 	/* set level and max in panel struct */
 	mutex_lock(&dev_priv->backlight_lock);
-	ret = dev_priv->display.setup_backlight(intel_connector);
+	ret = dev_priv->display.setup_backlight(intel_connector, pipe);
 	mutex_unlock(&dev_priv->backlight_lock);
 
 	if (ret) {
-- 
2.0.4

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

Re: [PATCH 4/6] drm/i915: Pass the current pipe from eDP init to backlight setup

[Jani Nikula]

On Fri, 07 Nov 2014, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> On VLV/CHV both pipes A and B have their own backlight control
> registers. In order to correctly read out the current hardware state at
> init we need to know which pipe is driving the eDP port. Pass that
> information down from the eDP init code into the backlight code.
>
> To determine the correct pipe we first look at which pipe is currently
> configured in the port control register, if that look invalid we look
> at which pipe's PPS is currently controlling the port, and if that
> too looks invalid we just assume pipe A.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h    |  2 +-
>  drivers/gpu/drm/i915/intel_dp.c    | 22 +++++++++++++++++++++-
>  drivers/gpu/drm/i915/intel_drv.h   |  2 +-
>  drivers/gpu/drm/i915/intel_lvds.c  |  2 +-
>  drivers/gpu/drm/i915/intel_panel.c | 31 +++++++++++++++++--------------
>  5 files changed, 41 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 0f00e58..65e4e29 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -510,7 +510,7 @@ struct drm_i915_display_funcs {
>  	/* display clock increase/decrease */
>  	/* pll clock increase/decrease */
>  
> -	int (*setup_backlight)(struct intel_connector *connector);
> +	int (*setup_backlight)(struct intel_connector *connector, enum pipe pipe);
>  	uint32_t (*get_backlight)(struct intel_connector *connector);
>  	void (*set_backlight)(struct intel_connector *connector,
>  			      uint32_t level);
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index ceb528f..45b53ff 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -5232,6 +5232,7 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
>  	bool has_dpcd;
>  	struct drm_display_mode *scan;
>  	struct edid *edid;
> +	enum pipe pipe = INVALID_PIPE;
>  
>  	intel_dp->drrs_state.type = DRRS_NOT_SUPPORTED;
>  
> @@ -5300,11 +5301,30 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
>  	if (IS_VALLEYVIEW(dev)) {
>  		intel_dp->edp_notifier.notifier_call = edp_notify_handler;
>  		register_reboot_notifier(&intel_dp->edp_notifier);
> +
> +		/*
> +		 * Figure out the current pipe for the initial backlight setup.
> +		 * If the current pipe isn't valid, try the PPS pipe, and if that
> +		 * fails just assume pipe A.
> +		 */
> +		if (IS_CHERRYVIEW(dev))
> +			pipe = DP_PORT_TO_PIPE_CHV(intel_dp->DP);
> +		else
> +			pipe = PORT_TO_PIPE(intel_dp->DP);
> +
> +		if (pipe != PIPE_A && pipe != PIPE_B)
> +			pipe = intel_dp->pps_pipe;
> +
> +		if (pipe != PIPE_A && pipe != PIPE_B)
> +			pipe = PIPE_A;
> +
> +		DRM_DEBUG_KMS("using pipe %c for initial backlight setup\n",
> +			      pipe_name(pipe));
>  	}
>  
>  	intel_panel_init(&intel_connector->panel, fixed_mode, downclock_mode);
>  	intel_connector->panel.backlight_power = intel_edp_backlight_power;
> -	intel_panel_setup_backlight(connector);
> +	intel_panel_setup_backlight(connector, pipe);
>  
>  	return true;
>  }
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index cb0e9db..0ff011e 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1103,7 +1103,7 @@ void intel_gmch_panel_fitting(struct intel_crtc *crtc,
>  			      int fitting_mode);
>  void intel_panel_set_backlight_acpi(struct intel_connector *connector,
>  				    u32 level, u32 max);
> -int intel_panel_setup_backlight(struct drm_connector *connector);
> +int intel_panel_setup_backlight(struct drm_connector *connector, enum pipe pipe);
>  void intel_panel_enable_backlight(struct intel_connector *connector);
>  void intel_panel_disable_backlight(struct intel_connector *connector);
>  void intel_panel_destroy_backlight(struct drm_connector *connector);
> diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
> index 2b50c98..c03d457 100644
> --- a/drivers/gpu/drm/i915/intel_lvds.c
> +++ b/drivers/gpu/drm/i915/intel_lvds.c
> @@ -1116,7 +1116,7 @@ out:
>  	drm_connector_register(connector);
>  
>  	intel_panel_init(&intel_connector->panel, fixed_mode, downclock_mode);
> -	intel_panel_setup_backlight(connector);
> +	intel_panel_setup_backlight(connector, INVALID_PIPE);
>  
>  	return;
>  
> diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
> index 847d00f9..f6e8d23 100644
> --- a/drivers/gpu/drm/i915/intel_panel.c
> +++ b/drivers/gpu/drm/i915/intel_panel.c
> @@ -1117,7 +1117,7 @@ static u32 get_backlight_min_vbt(struct intel_connector *connector)
>  		     0, panel->backlight.max);
>  }
>  
> -static int bdw_setup_backlight(struct intel_connector *connector)
> +static int bdw_setup_backlight(struct intel_connector *connector, enum pipe unused)
>  {
>  	struct drm_device *dev = connector->base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -1143,7 +1143,7 @@ static int bdw_setup_backlight(struct intel_connector *connector)
>  	return 0;
>  }
>  
> -static int pch_setup_backlight(struct intel_connector *connector)
> +static int pch_setup_backlight(struct intel_connector *connector, enum pipe unused)
>  {
>  	struct drm_device *dev = connector->base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -1170,7 +1170,7 @@ static int pch_setup_backlight(struct intel_connector *connector)
>  	return 0;
>  }
>  
> -static int i9xx_setup_backlight(struct intel_connector *connector)
> +static int i9xx_setup_backlight(struct intel_connector *connector, enum pipe unused)
>  {
>  	struct drm_device *dev = connector->base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -1202,7 +1202,7 @@ static int i9xx_setup_backlight(struct intel_connector *connector)
>  	return 0;
>  }
>  
> -static int i965_setup_backlight(struct intel_connector *connector)
> +static int i965_setup_backlight(struct intel_connector *connector, enum pipe unused)
>  {
>  	struct drm_device *dev = connector->base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -1232,37 +1232,40 @@ static int i965_setup_backlight(struct intel_connector *connector)
>  	return 0;
>  }
>  
> -static int vlv_setup_backlight(struct intel_connector *connector)
> +static int vlv_setup_backlight(struct intel_connector *connector, enum pipe pipe)
>  {
>  	struct drm_device *dev = connector->base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_panel *panel = &connector->panel;
> -	enum pipe pipe;
> +	enum pipe p;
>  	u32 ctl, ctl2, val;
>  
> -	for_each_pipe(dev_priv, pipe) {
> -		u32 cur_val = I915_READ(VLV_BLC_PWM_CTL(pipe));
> +	for_each_pipe(dev_priv, p) {
> +		u32 cur_val = I915_READ(VLV_BLC_PWM_CTL(p));
>  
>  		/* Skip if the modulation freq is already set */
>  		if (cur_val & ~BACKLIGHT_DUTY_CYCLE_MASK)
>  			continue;
>  
>  		cur_val &= BACKLIGHT_DUTY_CYCLE_MASK;
> -		I915_WRITE(VLV_BLC_PWM_CTL(pipe), (0xf42 << 16) |
> +		I915_WRITE(VLV_BLC_PWM_CTL(p), (0xf42 << 16) |
>  			   cur_val);
>  	}

I think we could try a follow-up patch doing the above only for the pipe
passed in. That's the one we're using, and the regs will be written in
backlight enable if the pipe changes.

Anyway, I like this approach. I had a patch trying to figure all this
out harder in the backlight code, but really anyone calling setup
backlight is in a much better position to do it.

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

>  
> -	ctl2 = I915_READ(VLV_BLC_PWM_CTL2(PIPE_A));
> +	if (WARN_ON(pipe != PIPE_A && pipe != PIPE_B))
> +		return -ENODEV;
> +
> +	ctl2 = I915_READ(VLV_BLC_PWM_CTL2(pipe));
>  	panel->backlight.active_low_pwm = ctl2 & BLM_POLARITY_I965;
>  
> -	ctl = I915_READ(VLV_BLC_PWM_CTL(PIPE_A));
> +	ctl = I915_READ(VLV_BLC_PWM_CTL(pipe));
>  	panel->backlight.max = ctl >> 16;
>  	if (!panel->backlight.max)
>  		return -ENODEV;
>  
>  	panel->backlight.min = get_backlight_min_vbt(connector);
>  
> -	val = _vlv_get_backlight(dev, PIPE_A);
> +	val = _vlv_get_backlight(dev, pipe);
>  	panel->backlight.level = intel_panel_compute_brightness(connector, val);
>  
>  	panel->backlight.enabled = (ctl2 & BLM_PWM_ENABLE) &&
> @@ -1271,7 +1274,7 @@ static int vlv_setup_backlight(struct intel_connector *connector)
>  	return 0;
>  }
>  
> -int intel_panel_setup_backlight(struct drm_connector *connector)
> +int intel_panel_setup_backlight(struct drm_connector *connector, enum pipe pipe)
>  {
>  	struct drm_device *dev = connector->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -1290,7 +1293,7 @@ int intel_panel_setup_backlight(struct drm_connector *connector)
>  
>  	/* set level and max in panel struct */
>  	mutex_lock(&dev_priv->backlight_lock);
> -	ret = dev_priv->display.setup_backlight(intel_connector);
> +	ret = dev_priv->display.setup_backlight(intel_connector, pipe);
>  	mutex_unlock(&dev_priv->backlight_lock);
>  
>  	if (ret) {
> -- 
> 2.0.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 5/6] drm/i915: Register the backlight device after the modeset init

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Currently we register the backlight device as soon as we register the
connector. That means we can get backlight requestes from userspace
already before reading out the current modeset hardware state.

That means we don't yet know the current crtc->encoder->connector mapping,
which causes problems for VLV/CHV which need to know the current pipe in
order to figure out which BLC registers to poke. Currently we just
ignore such requestes fairly deep in the backlight code which means the
backlight device brightness property will get out of sync with our
backlight.level and the actual hardware state.

Fix the problem by delaying the backlight device registration until the
entire modeset init has been performed. And we also move the
backlight unregisteration to happen as the first thing during the
modeset cleanup so that we also won't be bothered with userspace
backlight requested during teardown.

This is a real world problem on machines using systemd, because systemd,
for some reason, wants to restore the backlight to the level it used last
time. And that happens as soon as it sees the backlight device appearing
in the system. Sometimes the userspace access makes it through before
the modeset init, sometimes not.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c |  4 ++++
 drivers/gpu/drm/i915/intel_drv.h     |  3 +++
 drivers/gpu/drm/i915/intel_panel.c   | 22 +++++++++++++++++++---
 3 files changed, 26 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index b9529d0..158d65b 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13378,6 +13378,8 @@ void intel_modeset_gem_init(struct drm_device *dev)
 		}
 	}
 	mutex_unlock(&dev->struct_mutex);
+
+	intel_backlight_register(dev);
 }
 
 void intel_connector_unregister(struct intel_connector *intel_connector)
@@ -13393,6 +13395,8 @@ void intel_modeset_cleanup(struct drm_device *dev)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_connector *connector;
 
+	intel_backlight_unregister(dev);
+
 	/*
 	 * Interrupts and polling as the first thing to avoid creating havoc.
 	 * Too much stuff here (turning of rps, connectors, ...) would
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 0ff011e..2a1f790 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1113,6 +1113,9 @@ extern struct drm_display_mode *intel_find_panel_downclock(
 				struct drm_device *dev,
 				struct drm_display_mode *fixed_mode,
 				struct drm_connector *connector);
+void intel_backlight_register(struct drm_device *dev);
+void intel_backlight_unregister(struct drm_device *dev);
+
 
 /* intel_runtime_pm.c */
 int intel_power_domains_init(struct drm_i915_private *);
diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
index f6e8d23..2bc3309 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -1041,6 +1041,9 @@ static int intel_backlight_device_register(struct intel_connector *connector)
 	if (WARN_ON(panel->backlight.device))
 		return -ENODEV;
 
+	if (!panel->backlight.present)
+		return 0;
+
 	WARN_ON(panel->backlight.max == 0);
 
 	memset(&props, 0, sizeof(props));
@@ -1302,8 +1305,6 @@ int intel_panel_setup_backlight(struct drm_connector *connector, enum pipe pipe)
 		return ret;
 	}
 
-	intel_backlight_device_register(intel_connector);
-
 	panel->backlight.present = true;
 
 	DRM_DEBUG_KMS("backlight initialized, %s, brightness %u/%u, "
@@ -1321,7 +1322,6 @@ void intel_panel_destroy_backlight(struct drm_connector *connector)
 	struct intel_panel *panel = &intel_connector->panel;
 
 	panel->backlight.present = false;
-	intel_backlight_device_unregister(intel_connector);
 }
 
 /* Set up chip specific backlight functions */
@@ -1384,3 +1384,19 @@ void intel_panel_fini(struct intel_panel *panel)
 		drm_mode_destroy(intel_connector->base.dev,
 				panel->downclock_mode);
 }
+
+void intel_backlight_register(struct drm_device *dev)
+{
+	struct intel_connector *connector;
+
+	list_for_each_entry(connector, &dev->mode_config.connector_list, base.head)
+		intel_backlight_device_register(connector);
+}
+
+void intel_backlight_unregister(struct drm_device *dev)
+{
+	struct intel_connector *connector;
+
+	list_for_each_entry(connector, &dev->mode_config.connector_list, base.head)
+		intel_backlight_device_unregister(connector);
+}
-- 
2.0.4

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

Re: [PATCH 5/6] drm/i915: Register the backlight device after the modeset init

[Jani Nikula]

On Fri, 07 Nov 2014, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Currently we register the backlight device as soon as we register the
> connector. That means we can get backlight requestes from userspace
> already before reading out the current modeset hardware state.
>
> That means we don't yet know the current crtc->encoder->connector mapping,
> which causes problems for VLV/CHV which need to know the current pipe in
> order to figure out which BLC registers to poke. Currently we just
> ignore such requestes fairly deep in the backlight code which means the
> backlight device brightness property will get out of sync with our
> backlight.level and the actual hardware state.
>
> Fix the problem by delaying the backlight device registration until the
> entire modeset init has been performed. And we also move the
> backlight unregisteration to happen as the first thing during the
> modeset cleanup so that we also won't be bothered with userspace
> backlight requested during teardown.
>
> This is a real world problem on machines using systemd, because systemd,
> for some reason, wants to restore the backlight to the level it used last
> time. And that happens as soon as it sees the backlight device appearing
> in the system. Sometimes the userspace access makes it through before
> the modeset init, sometimes not.

Makes sense.

Please update the debug print at the end of intel_panel_setup_backlight
accordingly, and add a debug print about sysfs registration at the end
of intel_backlight_device_register. With that done, this is

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

While at it, how about adding drm_get_connector_name(connector) in the
debug prints?

BR,
Jani.


>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c |  4 ++++
>  drivers/gpu/drm/i915/intel_drv.h     |  3 +++
>  drivers/gpu/drm/i915/intel_panel.c   | 22 +++++++++++++++++++---
>  3 files changed, 26 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index b9529d0..158d65b 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13378,6 +13378,8 @@ void intel_modeset_gem_init(struct drm_device *dev)
>  		}
>  	}
>  	mutex_unlock(&dev->struct_mutex);
> +
> +	intel_backlight_register(dev);
>  }
>  
>  void intel_connector_unregister(struct intel_connector *intel_connector)
> @@ -13393,6 +13395,8 @@ void intel_modeset_cleanup(struct drm_device *dev)
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct drm_connector *connector;
>  
> +	intel_backlight_unregister(dev);
> +
>  	/*
>  	 * Interrupts and polling as the first thing to avoid creating havoc.
>  	 * Too much stuff here (turning of rps, connectors, ...) would
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 0ff011e..2a1f790 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1113,6 +1113,9 @@ extern struct drm_display_mode *intel_find_panel_downclock(
>  				struct drm_device *dev,
>  				struct drm_display_mode *fixed_mode,
>  				struct drm_connector *connector);
> +void intel_backlight_register(struct drm_device *dev);
> +void intel_backlight_unregister(struct drm_device *dev);
> +
>  
>  /* intel_runtime_pm.c */
>  int intel_power_domains_init(struct drm_i915_private *);
> diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
> index f6e8d23..2bc3309 100644
> --- a/drivers/gpu/drm/i915/intel_panel.c
> +++ b/drivers/gpu/drm/i915/intel_panel.c
> @@ -1041,6 +1041,9 @@ static int intel_backlight_device_register(struct intel_connector *connector)
>  	if (WARN_ON(panel->backlight.device))
>  		return -ENODEV;
>  
> +	if (!panel->backlight.present)
> +		return 0;
> +
>  	WARN_ON(panel->backlight.max == 0);
>  
>  	memset(&props, 0, sizeof(props));
> @@ -1302,8 +1305,6 @@ int intel_panel_setup_backlight(struct drm_connector *connector, enum pipe pipe)
>  		return ret;
>  	}
>  
> -	intel_backlight_device_register(intel_connector);
> -
>  	panel->backlight.present = true;
>  
>  	DRM_DEBUG_KMS("backlight initialized, %s, brightness %u/%u, "
> @@ -1321,7 +1322,6 @@ void intel_panel_destroy_backlight(struct drm_connector *connector)
>  	struct intel_panel *panel = &intel_connector->panel;
>  
>  	panel->backlight.present = false;
> -	intel_backlight_device_unregister(intel_connector);
>  }
>  
>  /* Set up chip specific backlight functions */
> @@ -1384,3 +1384,19 @@ void intel_panel_fini(struct intel_panel *panel)
>  		drm_mode_destroy(intel_connector->base.dev,
>  				panel->downclock_mode);
>  }
> +
> +void intel_backlight_register(struct drm_device *dev)
> +{
> +	struct intel_connector *connector;
> +
> +	list_for_each_entry(connector, &dev->mode_config.connector_list, base.head)
> +		intel_backlight_device_register(connector);
> +}
> +
> +void intel_backlight_unregister(struct drm_device *dev)
> +{
> +	struct intel_connector *connector;
> +
> +	list_for_each_entry(connector, &dev->mode_config.connector_list, base.head)
> +		intel_backlight_device_unregister(connector);
> +}
> -- 
> 2.0.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/6] drm/i915: Register the backlight device after the modeset init

[Jani Nikula]

On Fri, 07 Nov 2014, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Also, s/requestes/requests/g below.

J.

> Currently we register the backlight device as soon as we register the
> connector. That means we can get backlight requestes from userspace
> already before reading out the current modeset hardware state.
>
> That means we don't yet know the current crtc->encoder->connector mapping,
> which causes problems for VLV/CHV which need to know the current pipe in
> order to figure out which BLC registers to poke. Currently we just
> ignore such requestes fairly deep in the backlight code which means the
> backlight device brightness property will get out of sync with our
> backlight.level and the actual hardware state.
>
> Fix the problem by delaying the backlight device registration until the
> entire modeset init has been performed. And we also move the
> backlight unregisteration to happen as the first thing during the
> modeset cleanup so that we also won't be bothered with userspace
> backlight requested during teardown.
>
> This is a real world problem on machines using systemd, because systemd,
> for some reason, wants to restore the backlight to the level it used last
> time. And that happens as soon as it sees the backlight device appearing
> in the system. Sometimes the userspace access makes it through before
> the modeset init, sometimes not.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c |  4 ++++
>  drivers/gpu/drm/i915/intel_drv.h     |  3 +++
>  drivers/gpu/drm/i915/intel_panel.c   | 22 +++++++++++++++++++---
>  3 files changed, 26 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index b9529d0..158d65b 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13378,6 +13378,8 @@ void intel_modeset_gem_init(struct drm_device *dev)
>  		}
>  	}
>  	mutex_unlock(&dev->struct_mutex);
> +
> +	intel_backlight_register(dev);
>  }
>  
>  void intel_connector_unregister(struct intel_connector *intel_connector)
> @@ -13393,6 +13395,8 @@ void intel_modeset_cleanup(struct drm_device *dev)
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct drm_connector *connector;
>  
> +	intel_backlight_unregister(dev);
> +
>  	/*
>  	 * Interrupts and polling as the first thing to avoid creating havoc.
>  	 * Too much stuff here (turning of rps, connectors, ...) would
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 0ff011e..2a1f790 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1113,6 +1113,9 @@ extern struct drm_display_mode *intel_find_panel_downclock(
>  				struct drm_device *dev,
>  				struct drm_display_mode *fixed_mode,
>  				struct drm_connector *connector);
> +void intel_backlight_register(struct drm_device *dev);
> +void intel_backlight_unregister(struct drm_device *dev);
> +
>  
>  /* intel_runtime_pm.c */
>  int intel_power_domains_init(struct drm_i915_private *);
> diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
> index f6e8d23..2bc3309 100644
> --- a/drivers/gpu/drm/i915/intel_panel.c
> +++ b/drivers/gpu/drm/i915/intel_panel.c
> @@ -1041,6 +1041,9 @@ static int intel_backlight_device_register(struct intel_connector *connector)
>  	if (WARN_ON(panel->backlight.device))
>  		return -ENODEV;
>  
> +	if (!panel->backlight.present)
> +		return 0;
> +
>  	WARN_ON(panel->backlight.max == 0);
>  
>  	memset(&props, 0, sizeof(props));
> @@ -1302,8 +1305,6 @@ int intel_panel_setup_backlight(struct drm_connector *connector, enum pipe pipe)
>  		return ret;
>  	}
>  
> -	intel_backlight_device_register(intel_connector);
> -
>  	panel->backlight.present = true;
>  
>  	DRM_DEBUG_KMS("backlight initialized, %s, brightness %u/%u, "
> @@ -1321,7 +1322,6 @@ void intel_panel_destroy_backlight(struct drm_connector *connector)
>  	struct intel_panel *panel = &intel_connector->panel;
>  
>  	panel->backlight.present = false;
> -	intel_backlight_device_unregister(intel_connector);
>  }
>  
>  /* Set up chip specific backlight functions */
> @@ -1384,3 +1384,19 @@ void intel_panel_fini(struct intel_panel *panel)
>  		drm_mode_destroy(intel_connector->base.dev,
>  				panel->downclock_mode);
>  }
> +
> +void intel_backlight_register(struct drm_device *dev)
> +{
> +	struct intel_connector *connector;
> +
> +	list_for_each_entry(connector, &dev->mode_config.connector_list, base.head)
> +		intel_backlight_device_register(connector);
> +}
> +
> +void intel_backlight_unregister(struct drm_device *dev)
> +{
> +	struct intel_connector *connector;
> +
> +	list_for_each_entry(connector, &dev->mode_config.connector_list, base.head)
> +		intel_backlight_device_unregister(connector);
> +}
> -- 
> 2.0.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 5/6] drm/i915: Register the backlight device after the modeset init

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Currently we register the backlight device as soon as we register the
connector. That means we can get backlight requests from userspace
already before reading out the current modeset hardware state.

That means we don't yet know the current crtc->encoder->connector mapping,
which causes problems for VLV/CHV which need to know the current pipe in
order to figure out which BLC registers to poke. Currently we just
ignore such requests fairly deep in the backlight code which means the
backlight device brightness property will get out of sync with our
backlight.level and the actual hardware state.

Fix the problem by delaying the backlight device registration until the
entire modeset init has been performed. And we also move the
backlight unregisteration to happen as the first thing during the
modeset cleanup so that we also won't be bothered with userspace
backlight requested during teardown.

This is a real world problem on machines using systemd, because systemd,
for some reason, wants to restore the backlight to the level it used last
time. And that happens as soon as it sees the backlight device appearing
in the system. Sometimes the userspace access makes it through before
the modeset init, sometimes not.

v2: Do not lie to the user in the debug prints (Jani)
    Include connector name in the prints (Jani)
    Fix a typo in the commit message (Jani)

Reviewed-by: Jani Nikula <jani.nikula@intel.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c |  4 ++++
 drivers/gpu/drm/i915/intel_drv.h     |  3 +++
 drivers/gpu/drm/i915/intel_panel.c   | 33 ++++++++++++++++++++++++++-------
 3 files changed, 33 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index b9529d0..158d65b 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13378,6 +13378,8 @@ void intel_modeset_gem_init(struct drm_device *dev)
 		}
 	}
 	mutex_unlock(&dev->struct_mutex);
+
+	intel_backlight_register(dev);
 }
 
 void intel_connector_unregister(struct intel_connector *intel_connector)
@@ -13393,6 +13395,8 @@ void intel_modeset_cleanup(struct drm_device *dev)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_connector *connector;
 
+	intel_backlight_unregister(dev);
+
 	/*
 	 * Interrupts and polling as the first thing to avoid creating havoc.
 	 * Too much stuff here (turning of rps, connectors, ...) would
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 0ff011e..2a1f790 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1113,6 +1113,9 @@ extern struct drm_display_mode *intel_find_panel_downclock(
 				struct drm_device *dev,
 				struct drm_display_mode *fixed_mode,
 				struct drm_connector *connector);
+void intel_backlight_register(struct drm_device *dev);
+void intel_backlight_unregister(struct drm_device *dev);
+
 
 /* intel_runtime_pm.c */
 int intel_power_domains_init(struct drm_i915_private *);
diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
index 69bbfba..708642a 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -1041,6 +1041,9 @@ static int intel_backlight_device_register(struct intel_connector *connector)
 	if (WARN_ON(panel->backlight.device))
 		return -ENODEV;
 
+	if (!panel->backlight.present)
+		return 0;
+
 	WARN_ON(panel->backlight.max == 0);
 
 	memset(&props, 0, sizeof(props));
@@ -1076,6 +1079,10 @@ static int intel_backlight_device_register(struct intel_connector *connector)
 		panel->backlight.device = NULL;
 		return -ENODEV;
 	}
+
+	DRM_DEBUG_KMS("Connector %s backlight sysfs interface registered\n",
+		      connector->base.name);
+
 	return 0;
 }
 
@@ -1302,15 +1309,12 @@ int intel_panel_setup_backlight(struct drm_connector *connector, enum pipe pipe)
 		return ret;
 	}
 
-	intel_backlight_device_register(intel_connector);
-
 	panel->backlight.present = true;
 
-	DRM_DEBUG_KMS("backlight initialized, %s, brightness %u/%u, "
-		      "sysfs interface %sregistered\n",
+	DRM_DEBUG_KMS("Connector %s backlight initialized, %s, brightness %u/%u\n",
+		      connector->name,
 		      panel->backlight.enabled ? "enabled" : "disabled",
-		      panel->backlight.level, panel->backlight.max,
-		      panel->backlight.device ? "" : "not ");
+		      panel->backlight.level, panel->backlight.max);
 
 	return 0;
 }
@@ -1321,7 +1325,6 @@ void intel_panel_destroy_backlight(struct drm_connector *connector)
 	struct intel_panel *panel = &intel_connector->panel;
 
 	panel->backlight.present = false;
-	intel_backlight_device_unregister(intel_connector);
 }
 
 /* Set up chip specific backlight functions */
@@ -1384,3 +1387,19 @@ void intel_panel_fini(struct intel_panel *panel)
 		drm_mode_destroy(intel_connector->base.dev,
 				panel->downclock_mode);
 }
+
+void intel_backlight_register(struct drm_device *dev)
+{
+	struct intel_connector *connector;
+
+	list_for_each_entry(connector, &dev->mode_config.connector_list, base.head)
+		intel_backlight_device_register(connector);
+}
+
+void intel_backlight_unregister(struct drm_device *dev)
+{
+	struct intel_connector *connector;
+
+	list_for_each_entry(connector, &dev->mode_config.connector_list, base.head)
+		intel_backlight_device_unregister(connector);
+}
-- 
2.0.4

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

Re: [PATCH v2 5/6] drm/i915: Register the backlight device after the modeset init

[Jani Nikula]

On Fri, 07 Nov 2014, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Currently we register the backlight device as soon as we register the
> connector. That means we can get backlight requests from userspace
> already before reading out the current modeset hardware state.
>
> That means we don't yet know the current crtc->encoder->connector mapping,
> which causes problems for VLV/CHV which need to know the current pipe in
> order to figure out which BLC registers to poke. Currently we just
> ignore such requests fairly deep in the backlight code which means the
> backlight device brightness property will get out of sync with our
> backlight.level and the actual hardware state.
>
> Fix the problem by delaying the backlight device registration until the
> entire modeset init has been performed. And we also move the
> backlight unregisteration to happen as the first thing during the
> modeset cleanup so that we also won't be bothered with userspace
> backlight requested during teardown.
>
> This is a real world problem on machines using systemd, because systemd,
> for some reason, wants to restore the backlight to the level it used last
> time. And that happens as soon as it sees the backlight device appearing
> in the system. Sometimes the userspace access makes it through before
> the modeset init, sometimes not.
>
> v2: Do not lie to the user in the debug prints (Jani)
>     Include connector name in the prints (Jani)
>     Fix a typo in the commit message (Jani)
>
> Reviewed-by: Jani Nikula <jani.nikula@intel.com>

Yup.

> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c |  4 ++++
>  drivers/gpu/drm/i915/intel_drv.h     |  3 +++
>  drivers/gpu/drm/i915/intel_panel.c   | 33 ++++++++++++++++++++++++++-------
>  3 files changed, 33 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index b9529d0..158d65b 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13378,6 +13378,8 @@ void intel_modeset_gem_init(struct drm_device *dev)
>  		}
>  	}
>  	mutex_unlock(&dev->struct_mutex);
> +
> +	intel_backlight_register(dev);
>  }
>  
>  void intel_connector_unregister(struct intel_connector *intel_connector)
> @@ -13393,6 +13395,8 @@ void intel_modeset_cleanup(struct drm_device *dev)
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct drm_connector *connector;
>  
> +	intel_backlight_unregister(dev);
> +
>  	/*
>  	 * Interrupts and polling as the first thing to avoid creating havoc.
>  	 * Too much stuff here (turning of rps, connectors, ...) would
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 0ff011e..2a1f790 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1113,6 +1113,9 @@ extern struct drm_display_mode *intel_find_panel_downclock(
>  				struct drm_device *dev,
>  				struct drm_display_mode *fixed_mode,
>  				struct drm_connector *connector);
> +void intel_backlight_register(struct drm_device *dev);
> +void intel_backlight_unregister(struct drm_device *dev);
> +
>  
>  /* intel_runtime_pm.c */
>  int intel_power_domains_init(struct drm_i915_private *);
> diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
> index 69bbfba..708642a 100644
> --- a/drivers/gpu/drm/i915/intel_panel.c
> +++ b/drivers/gpu/drm/i915/intel_panel.c
> @@ -1041,6 +1041,9 @@ static int intel_backlight_device_register(struct intel_connector *connector)
>  	if (WARN_ON(panel->backlight.device))
>  		return -ENODEV;
>  
> +	if (!panel->backlight.present)
> +		return 0;
> +
>  	WARN_ON(panel->backlight.max == 0);
>  
>  	memset(&props, 0, sizeof(props));
> @@ -1076,6 +1079,10 @@ static int intel_backlight_device_register(struct intel_connector *connector)
>  		panel->backlight.device = NULL;
>  		return -ENODEV;
>  	}
> +
> +	DRM_DEBUG_KMS("Connector %s backlight sysfs interface registered\n",
> +		      connector->base.name);
> +
>  	return 0;
>  }
>  
> @@ -1302,15 +1309,12 @@ int intel_panel_setup_backlight(struct drm_connector *connector, enum pipe pipe)
>  		return ret;
>  	}
>  
> -	intel_backlight_device_register(intel_connector);
> -
>  	panel->backlight.present = true;
>  
> -	DRM_DEBUG_KMS("backlight initialized, %s, brightness %u/%u, "
> -		      "sysfs interface %sregistered\n",
> +	DRM_DEBUG_KMS("Connector %s backlight initialized, %s, brightness %u/%u\n",
> +		      connector->name,
>  		      panel->backlight.enabled ? "enabled" : "disabled",
> -		      panel->backlight.level, panel->backlight.max,
> -		      panel->backlight.device ? "" : "not ");
> +		      panel->backlight.level, panel->backlight.max);
>  
>  	return 0;
>  }
> @@ -1321,7 +1325,6 @@ void intel_panel_destroy_backlight(struct drm_connector *connector)
>  	struct intel_panel *panel = &intel_connector->panel;
>  
>  	panel->backlight.present = false;
> -	intel_backlight_device_unregister(intel_connector);
>  }
>  
>  /* Set up chip specific backlight functions */
> @@ -1384,3 +1387,19 @@ void intel_panel_fini(struct intel_panel *panel)
>  		drm_mode_destroy(intel_connector->base.dev,
>  				panel->downclock_mode);
>  }
> +
> +void intel_backlight_register(struct drm_device *dev)
> +{
> +	struct intel_connector *connector;
> +
> +	list_for_each_entry(connector, &dev->mode_config.connector_list, base.head)
> +		intel_backlight_device_register(connector);
> +}
> +
> +void intel_backlight_unregister(struct drm_device *dev)
> +{
> +	struct intel_connector *connector;
> +
> +	list_for_each_entry(connector, &dev->mode_config.connector_list, base.head)
> +		intel_backlight_device_unregister(connector);
> +}
> -- 
> 2.0.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 6/6] drm/i915: Remove most INVALID_PIPE checks from VLV backlight code

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Now that the backlight device no longer gets registered too early we
should be able to drop most of the INVALID_PIPE checks form the VLV/CHV
backlight code.

If we still manage to get here with INVALID_PIPE we will now get a WARN
from the lower level functions and can then actually investigate further.

vlv_get_backlight() still needs the check since that gets called in
response to userspace actual_brightness reads.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_panel.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
index 2bc3309..0e2cb12 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -634,10 +634,9 @@ static void intel_panel_set_backlight(struct intel_connector *connector,
 	struct drm_device *dev = connector->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_panel *panel = &connector->panel;
-	enum pipe pipe = intel_get_pipe_from_connector(connector);
 	u32 hw_level;
 
-	if (!panel->backlight.present || pipe == INVALID_PIPE)
+	if (!panel->backlight.present)
 		return;
 
 	mutex_lock(&dev_priv->backlight_lock);
@@ -662,10 +661,9 @@ void intel_panel_set_backlight_acpi(struct intel_connector *connector,
 	struct drm_device *dev = connector->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_panel *panel = &connector->panel;
-	enum pipe pipe = intel_get_pipe_from_connector(connector);
 	u32 hw_level;
 
-	if (!panel->backlight.present || pipe == INVALID_PIPE)
+	if (!panel->backlight.present)
 		return;
 
 	mutex_lock(&dev_priv->backlight_lock);
@@ -740,9 +738,8 @@ void intel_panel_disable_backlight(struct intel_connector *connector)
 	struct drm_device *dev = connector->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_panel *panel = &connector->panel;
-	enum pipe pipe = intel_get_pipe_from_connector(connector);
 
-	if (!panel->backlight.present || pipe == INVALID_PIPE)
+	if (!panel->backlight.present)
 		return;
 
 	/*
@@ -949,7 +946,7 @@ void intel_panel_enable_backlight(struct intel_connector *connector)
 	struct intel_panel *panel = &connector->panel;
 	enum pipe pipe = intel_get_pipe_from_connector(connector);
 
-	if (!panel->backlight.present || pipe == INVALID_PIPE)
+	if (!panel->backlight.present)
 		return;
 
 	DRM_DEBUG_KMS("pipe %c\n", pipe_name(pipe));
-- 
2.0.4

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

Re: [PATCH 6/6] drm/i915: Remove most INVALID_PIPE checks from VLV backlight code

[Jani Nikula]

On Fri, 07 Nov 2014, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Now that the backlight device no longer gets registered too early we
> should be able to drop most of the INVALID_PIPE checks form the VLV/CHV
> backlight code.

The subject and this paragraph refer to VLV/CHV but this isn't really
specific to those platforms.

> If we still manage to get here with INVALID_PIPE we will now get a WARN
> from the lower level functions and can then actually investigate further.
>
> vlv_get_backlight() still needs the check since that gets called in
> response to userspace actual_brightness reads.

IIUC this bit won't be true if you add the backlight.enabled check as I
suggested earlier.

>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_panel.c | 11 ++++-------
>  1 file changed, 4 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
> index 2bc3309..0e2cb12 100644
> --- a/drivers/gpu/drm/i915/intel_panel.c
> +++ b/drivers/gpu/drm/i915/intel_panel.c
> @@ -634,10 +634,9 @@ static void intel_panel_set_backlight(struct intel_connector *connector,
>  	struct drm_device *dev = connector->base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_panel *panel = &connector->panel;
> -	enum pipe pipe = intel_get_pipe_from_connector(connector);
>  	u32 hw_level;
>  
> -	if (!panel->backlight.present || pipe == INVALID_PIPE)
> +	if (!panel->backlight.present)
>  		return;
>  
>  	mutex_lock(&dev_priv->backlight_lock);
> @@ -662,10 +661,9 @@ void intel_panel_set_backlight_acpi(struct intel_connector *connector,
>  	struct drm_device *dev = connector->base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_panel *panel = &connector->panel;
> -	enum pipe pipe = intel_get_pipe_from_connector(connector);
>  	u32 hw_level;
>  
> -	if (!panel->backlight.present || pipe == INVALID_PIPE)
> +	if (!panel->backlight.present)
>  		return;

I have a feeling we may get these requests from the BIOS whenever. In
theory we should use the opregion ARDY field or somesuch to communicate
whether we're ready or not (we always say we're ready like a scout) but
even so we can't trust the BIOS to listen to what we say. Long story
short we should probably leave this check in.

With those fixed this LGTM.

BR,
Jani.


>  
>  	mutex_lock(&dev_priv->backlight_lock);
> @@ -740,9 +738,8 @@ void intel_panel_disable_backlight(struct intel_connector *connector)
>  	struct drm_device *dev = connector->base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_panel *panel = &connector->panel;
> -	enum pipe pipe = intel_get_pipe_from_connector(connector);
>  
> -	if (!panel->backlight.present || pipe == INVALID_PIPE)
> +	if (!panel->backlight.present)
>  		return;
>  
>  	/*
> @@ -949,7 +946,7 @@ void intel_panel_enable_backlight(struct intel_connector *connector)
>  	struct intel_panel *panel = &connector->panel;
>  	enum pipe pipe = intel_get_pipe_from_connector(connector);
>  
> -	if (!panel->backlight.present || pipe == INVALID_PIPE)
> +	if (!panel->backlight.present)
>  		return;
>  
>  	DRM_DEBUG_KMS("pipe %c\n", pipe_name(pipe));
> -- 
> 2.0.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 6/6] drm/i915: Remove most INVALID_PIPE checks from VLV backlight code

[Ville Syrjälä]

On Fri, Nov 07, 2014 at 02:24:46PM +0200, Jani Nikula wrote:
> On Fri, 07 Nov 2014, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Now that the backlight device no longer gets registered too early we
> > should be able to drop most of the INVALID_PIPE checks form the VLV/CHV
> > backlight code.
> 
> The subject and this paragraph refer to VLV/CHV but this isn't really
> specific to those platforms.

Hmm. My assumption was that we can't get these with other platforms, but
the opregion might be a bit special I agree.

> 
> > If we still manage to get here with INVALID_PIPE we will now get a WARN
> > from the lower level functions and can then actually investigate further.
> >
> > vlv_get_backlight() still needs the check since that gets called in
> > response to userspace actual_brightness reads.
> 
> IIUC this bit won't be true if you add the backlight.enabled check as I
> suggested earlier.

Yeah, I think that should cover it.

> 
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_panel.c | 11 ++++-------
> >  1 file changed, 4 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
> > index 2bc3309..0e2cb12 100644
> > --- a/drivers/gpu/drm/i915/intel_panel.c
> > +++ b/drivers/gpu/drm/i915/intel_panel.c
> > @@ -634,10 +634,9 @@ static void intel_panel_set_backlight(struct intel_connector *connector,
> >  	struct drm_device *dev = connector->base.dev;
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	struct intel_panel *panel = &connector->panel;
> > -	enum pipe pipe = intel_get_pipe_from_connector(connector);
> >  	u32 hw_level;
> >  
> > -	if (!panel->backlight.present || pipe == INVALID_PIPE)
> > +	if (!panel->backlight.present)
> >  		return;
> >  
> >  	mutex_lock(&dev_priv->backlight_lock);
> > @@ -662,10 +661,9 @@ void intel_panel_set_backlight_acpi(struct intel_connector *connector,
> >  	struct drm_device *dev = connector->base.dev;
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	struct intel_panel *panel = &connector->panel;
> > -	enum pipe pipe = intel_get_pipe_from_connector(connector);
> >  	u32 hw_level;
> >  
> > -	if (!panel->backlight.present || pipe == INVALID_PIPE)
> > +	if (!panel->backlight.present)
> >  		return;
> 
> I have a feeling we may get these requests from the BIOS whenever. In
> theory we should use the opregion ARDY field or somesuch to communicate
> whether we're ready or not (we always say we're ready like a scout) but
> even so we can't trust the BIOS to listen to what we say. Long story
> short we should probably leave this check in.

We do check backlight.enabled here as well, and I think we grab all the
required locks when servicing the opregion requests. So I'm thinking we
don't need the check here either. Or am I missing something?

> 
> With those fixed this LGTM.
> 
> BR,
> Jani.
> 
> 
> >  
> >  	mutex_lock(&dev_priv->backlight_lock);
> > @@ -740,9 +738,8 @@ void intel_panel_disable_backlight(struct intel_connector *connector)
> >  	struct drm_device *dev = connector->base.dev;
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	struct intel_panel *panel = &connector->panel;
> > -	enum pipe pipe = intel_get_pipe_from_connector(connector);
> >  
> > -	if (!panel->backlight.present || pipe == INVALID_PIPE)
> > +	if (!panel->backlight.present)
> >  		return;
> >  
> >  	/*
> > @@ -949,7 +946,7 @@ void intel_panel_enable_backlight(struct intel_connector *connector)
> >  	struct intel_panel *panel = &connector->panel;
> >  	enum pipe pipe = intel_get_pipe_from_connector(connector);
> >  
> > -	if (!panel->backlight.present || pipe == INVALID_PIPE)
> > +	if (!panel->backlight.present)
> >  		return;
> >  
> >  	DRM_DEBUG_KMS("pipe %c\n", pipe_name(pipe));
> > -- 
> > 2.0.4
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 6/6] drm/i915: Remove most INVALID_PIPE checks from the backlight code

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Now that the backlight device no longer gets registered too early we
should be able to drop most of the INVALID_PIPE checks from the backlight
code.

The only exceptio is the opregion stuff where we may (in theory at
least) get a request from the BIOS already during driver init as soon as
the backlight setup has been done. In which case we can still get the
INVALID_PIPE from intel_get_pipe_from_connector(). So leave that check
in place, and add a comment explaining why.

For the rest, if we still manage to get here with INVALID_PIPE on
VLV/CHV we will now get a WARN from the lower level functions and
can then actually investigate further.

v2: Leave the check in the BIOS related code (Jani)

Cc: Jani Nikula <jani.nikula@intel.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_panel.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
index 708642a..c45b127 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -634,10 +634,9 @@ static void intel_panel_set_backlight(struct intel_connector *connector,
 	struct drm_device *dev = connector->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_panel *panel = &connector->panel;
-	enum pipe pipe = intel_get_pipe_from_connector(connector);
 	u32 hw_level;
 
-	if (!panel->backlight.present || pipe == INVALID_PIPE)
+	if (!panel->backlight.present)
 		return;
 
 	mutex_lock(&dev_priv->backlight_lock);
@@ -665,6 +664,12 @@ void intel_panel_set_backlight_acpi(struct intel_connector *connector,
 	enum pipe pipe = intel_get_pipe_from_connector(connector);
 	u32 hw_level;
 
+	/*
+	 * INVALID_PIPE may occur during driver init because
+	 * connection_mutex isn't held across the entire backlight
+	 * setup + modeset readout, and the BIOS can issue the
+	 * requests at any time.
+	 */
 	if (!panel->backlight.present || pipe == INVALID_PIPE)
 		return;
 
@@ -740,9 +745,8 @@ void intel_panel_disable_backlight(struct intel_connector *connector)
 	struct drm_device *dev = connector->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_panel *panel = &connector->panel;
-	enum pipe pipe = intel_get_pipe_from_connector(connector);
 
-	if (!panel->backlight.present || pipe == INVALID_PIPE)
+	if (!panel->backlight.present)
 		return;
 
 	/*
@@ -949,7 +953,7 @@ void intel_panel_enable_backlight(struct intel_connector *connector)
 	struct intel_panel *panel = &connector->panel;
 	enum pipe pipe = intel_get_pipe_from_connector(connector);
 
-	if (!panel->backlight.present || pipe == INVALID_PIPE)
+	if (!panel->backlight.present)
 		return;
 
 	DRM_DEBUG_KMS("pipe %c\n", pipe_name(pipe));
-- 
2.0.4

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

Re: [PATCH v2 6/6] drm/i915: Remove most INVALID_PIPE checks from the backlight code

[Jani Nikula]

On Fri, 07 Nov 2014, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Now that the backlight device no longer gets registered too early we
> should be able to drop most of the INVALID_PIPE checks from the backlight
> code.
>
> The only exceptio is the opregion stuff where we may (in theory at
> least) get a request from the BIOS already during driver init as soon as
> the backlight setup has been done. In which case we can still get the
> INVALID_PIPE from intel_get_pipe_from_connector(). So leave that check
> in place, and add a comment explaining why.
>
> For the rest, if we still manage to get here with INVALID_PIPE on
> VLV/CHV we will now get a WARN from the lower level functions and
> can then actually investigate further.
>
> v2: Leave the check in the BIOS related code (Jani)
>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_panel.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
> index 708642a..c45b127 100644
> --- a/drivers/gpu/drm/i915/intel_panel.c
> +++ b/drivers/gpu/drm/i915/intel_panel.c
> @@ -634,10 +634,9 @@ static void intel_panel_set_backlight(struct intel_connector *connector,
>  	struct drm_device *dev = connector->base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_panel *panel = &connector->panel;
> -	enum pipe pipe = intel_get_pipe_from_connector(connector);
>  	u32 hw_level;
>  
> -	if (!panel->backlight.present || pipe == INVALID_PIPE)
> +	if (!panel->backlight.present)
>  		return;
>  
>  	mutex_lock(&dev_priv->backlight_lock);
> @@ -665,6 +664,12 @@ void intel_panel_set_backlight_acpi(struct intel_connector *connector,
>  	enum pipe pipe = intel_get_pipe_from_connector(connector);
>  	u32 hw_level;
>  
> +	/*
> +	 * INVALID_PIPE may occur during driver init because
> +	 * connection_mutex isn't held across the entire backlight
> +	 * setup + modeset readout, and the BIOS can issue the
> +	 * requests at any time.
> +	 */
>  	if (!panel->backlight.present || pipe == INVALID_PIPE)
>  		return;
>  
> @@ -740,9 +745,8 @@ void intel_panel_disable_backlight(struct intel_connector *connector)
>  	struct drm_device *dev = connector->base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_panel *panel = &connector->panel;
> -	enum pipe pipe = intel_get_pipe_from_connector(connector);
>  
> -	if (!panel->backlight.present || pipe == INVALID_PIPE)
> +	if (!panel->backlight.present)
>  		return;
>  
>  	/*
> @@ -949,7 +953,7 @@ void intel_panel_enable_backlight(struct intel_connector *connector)
>  	struct intel_panel *panel = &connector->panel;
>  	enum pipe pipe = intel_get_pipe_from_connector(connector);
>  
> -	if (!panel->backlight.present || pipe == INVALID_PIPE)
> +	if (!panel->backlight.present)
>  		return;
>  
>  	DRM_DEBUG_KMS("pipe %c\n", pipe_name(pipe));
> -- 
> 2.0.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 6/6] drm/i915: Remove most INVALID_PIPE checks from the backlight code

[Daniel Vetter]

On Fri, Nov 07, 2014 at 03:26:32PM +0200, Jani Nikula wrote:
> On Fri, 07 Nov 2014, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Now that the backlight device no longer gets registered too early we
> > should be able to drop most of the INVALID_PIPE checks from the backlight
> > code.
> >
> > The only exceptio is the opregion stuff where we may (in theory at
> > least) get a request from the BIOS already during driver init as soon as
> > the backlight setup has been done. In which case we can still get the
> > INVALID_PIPE from intel_get_pipe_from_connector(). So leave that check
> > in place, and add a comment explaining why.
> >
> > For the rest, if we still manage to get here with INVALID_PIPE on
> > VLV/CHV we will now get a WARN from the lower level functions and
> > can then actually investigate further.
> >
> > v2: Leave the check in the BIOS related code (Jani)
> >
> > Cc: Jani Nikula <jani.nikula@intel.com>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Reviewed-by: Jani Nikula <jani.nikula@intel.com>

Merged everything from this series, thanks for patches&review.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx