[PATCH 1/2] drm/i915: check VBT for supported backlight type

[PATCH 1/2] drm/i915: check VBT for supported backlight type

[Jani Nikula]

The only supported types are none and PWM. Other values are obsolete or
reserved, don't add them.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h   |    1 +
 drivers/gpu/drm/i915/intel_bios.c |   10 ++++++++++
 drivers/gpu/drm/i915/intel_bios.h |    3 +++
 3 files changed, 14 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 55addaaa8222..03f4c866bf3e 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1154,6 +1154,7 @@ struct intel_vbt_data {
 	struct edp_power_seq edp_pps;
 
 	struct {
+		bool present;
 		u16 pwm_freq_hz;
 		bool active_low_pwm;
 	} backlight;
diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
index 9b986775c4b0..862ca04f7a44 100644
--- a/drivers/gpu/drm/i915/intel_bios.c
+++ b/drivers/gpu/drm/i915/intel_bios.c
@@ -309,6 +309,9 @@ parse_lfp_backlight(struct drm_i915_private *dev_priv, struct bdb_header *bdb)
 	const struct bdb_lfp_backlight_data *backlight_data;
 	const struct bdb_lfp_backlight_data_entry *entry;
 
+	/* Err to enabling backlight if no backlight block. */
+	dev_priv->vbt.backlight.present = true;
+
 	backlight_data = find_section(bdb, BDB_LVDS_BACKLIGHT);
 	if (!backlight_data)
 		return;
@@ -321,6 +324,13 @@ parse_lfp_backlight(struct drm_i915_private *dev_priv, struct bdb_header *bdb)
 
 	entry = &backlight_data->data[panel_type];
 
+	dev_priv->vbt.backlight.present = entry->type == BDB_BACKLIGHT_TYPE_PWM;
+	if (!dev_priv->vbt.backlight.present) {
+		DRM_DEBUG_KMS("PWM backlight not present in VBT (type %u)\n",
+			      entry->type);
+		return;
+	}
+
 	dev_priv->vbt.backlight.pwm_freq_hz = entry->pwm_freq_hz;
 	dev_priv->vbt.backlight.active_low_pwm = entry->active_low_pwm;
 	DRM_DEBUG_KMS("VBT backlight PWM modulation frequency %u Hz, "
diff --git a/drivers/gpu/drm/i915/intel_bios.h b/drivers/gpu/drm/i915/intel_bios.h
index d02e5f93c362..036a79922391 100644
--- a/drivers/gpu/drm/i915/intel_bios.h
+++ b/drivers/gpu/drm/i915/intel_bios.h
@@ -389,6 +389,9 @@ struct bdb_lvds_lfp_data {
 	struct bdb_lvds_lfp_data_entry data[16];
 } __packed;
 
+#define BDB_BACKLIGHT_TYPE_NONE	0
+#define BDB_BACKLIGHT_TYPE_PWM	2
+
 struct bdb_lfp_backlight_data_entry {
 	u8 type:2;
 	u8 active_low_pwm:1;
-- 
1.7.9.5

[PATCH 2/2] drm/i915: do not setup backlight if not available according to VBT

[Jani Nikula]

Some machines use an external EC for controlling the backlight. Info
about this is present in the VBT. Do not setup native backlight control
if no PWM backlight is available or supported according to VBT. The
acpi_backlight interface appears to work for the EC control.

In most cases there has been no harm done, but it looks like there are
machines out there that have both an EC and our PWM line connected to
the same wire. This, obviously, does not end well.

This should fix the regression caused by
commit bc0bb9fd1c7810407ab810d204bbaecb255fddde
Author: Jani Nikula <jani.nikula@intel.com>
Date:   Thu Nov 14 12:14:29 2013 +0200

    drm/i915: remove QUIRK_NO_PCH_PWM_ENABLE

AFAICT the quirk removed by the above commit effectively resulted in
i915 not driving the backlight PWM output, thus not messing things up.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=76276
Reference: https://bugzilla.kernel.org/show_bug.cgi?id=47941
CC: Aaron Lu <aaron.lu@intel.com>
CC: Kamal Mostafa <kamal@canonical.com>
CC: Eric Griffith <EGriffith92@gmail.com>
CC: Kent Baxley <kent.baxley@canonical.com>
CC: stable@vger.kernel.org [v3.14+]
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/intel_panel.c |    5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
index cb058408c70e..0eead16aeda7 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -1065,6 +1065,11 @@ int intel_panel_setup_backlight(struct drm_connector *connector)
 	unsigned long flags;
 	int ret;
 
+	if (!dev_priv->vbt.backlight.present) {
+		DRM_DEBUG_KMS("native backlight control not available per VBT\n");
+		return 0;
+	}
+
 	/* set level and max in panel struct */
 	spin_lock_irqsave(&dev_priv->backlight_lock, flags);
 	ret = dev_priv->display.setup_backlight(intel_connector);
-- 
1.7.9.5

Re: [PATCH 2/2] drm/i915: do not setup backlight if not available according to VBT

[Daniel Vetter]

On Wed, Apr 09, 2014 at 01:35:35PM +0300, Jani Nikula wrote:
> Some machines use an external EC for controlling the backlight. Info
> about this is present in the VBT. Do not setup native backlight control
> if no PWM backlight is available or supported according to VBT. The
> acpi_backlight interface appears to work for the EC control.
> 
> In most cases there has been no harm done, but it looks like there are
> machines out there that have both an EC and our PWM line connected to
> the same wire. This, obviously, does not end well.
> 
> This should fix the regression caused by
> commit bc0bb9fd1c7810407ab810d204bbaecb255fddde
> Author: Jani Nikula <jani.nikula@intel.com>
> Date:   Thu Nov 14 12:14:29 2013 +0200
> 
>     drm/i915: remove QUIRK_NO_PCH_PWM_ENABLE
> 
> AFAICT the quirk removed by the above commit effectively resulted in
> i915 not driving the backlight PWM output, thus not messing things up.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=76276
> Reference: https://bugzilla.kernel.org/show_bug.cgi?id=47941
> CC: Aaron Lu <aaron.lu@intel.com>
> CC: Kamal Mostafa <kamal@canonical.com>
> CC: Eric Griffith <EGriffith92@gmail.com>
> CC: Kent Baxley <kent.baxley@canonical.com>
> CC: stable@vger.kernel.org [v3.14+]

Imo this part is a bit frisky - if we get positive testing on the xps13
machines I'd shovel this into 3.15-fixes and then resurrect the old hack
for stable kernels. Imo this is risky enough that we should attempt to
backport it before 3.15 is actually released.
-Daniel

> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_panel.c |    5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
> index cb058408c70e..0eead16aeda7 100644
> --- a/drivers/gpu/drm/i915/intel_panel.c
> +++ b/drivers/gpu/drm/i915/intel_panel.c
> @@ -1065,6 +1065,11 @@ int intel_panel_setup_backlight(struct drm_connector *connector)
>  	unsigned long flags;
>  	int ret;
>  
> +	if (!dev_priv->vbt.backlight.present) {
> +		DRM_DEBUG_KMS("native backlight control not available per VBT\n");
> +		return 0;
> +	}
> +
>  	/* set level and max in panel struct */
>  	spin_lock_irqsave(&dev_priv->backlight_lock, flags);
>  	ret = dev_priv->display.setup_backlight(intel_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

Re: [PATCH 2/2] drm/i915: do not setup backlight if not available according to VBT

[Jani Nikula]

On Wed, 09 Apr 2014, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Wed, Apr 09, 2014 at 01:35:35PM +0300, Jani Nikula wrote:
>> Some machines use an external EC for controlling the backlight. Info
>> about this is present in the VBT. Do not setup native backlight control
>> if no PWM backlight is available or supported according to VBT. The
>> acpi_backlight interface appears to work for the EC control.
>> 
>> In most cases there has been no harm done, but it looks like there are
>> machines out there that have both an EC and our PWM line connected to
>> the same wire. This, obviously, does not end well.
>> 
>> This should fix the regression caused by
>> commit bc0bb9fd1c7810407ab810d204bbaecb255fddde
>> Author: Jani Nikula <jani.nikula@intel.com>
>> Date:   Thu Nov 14 12:14:29 2013 +0200
>> 
>>     drm/i915: remove QUIRK_NO_PCH_PWM_ENABLE
>> 
>> AFAICT the quirk removed by the above commit effectively resulted in
>> i915 not driving the backlight PWM output, thus not messing things up.
>> 
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=76276
>> Reference: https://bugzilla.kernel.org/show_bug.cgi?id=47941
>> CC: Aaron Lu <aaron.lu@intel.com>
>> CC: Kamal Mostafa <kamal@canonical.com>
>> CC: Eric Griffith <EGriffith92@gmail.com>
>> CC: Kent Baxley <kent.baxley@canonical.com>
>> CC: stable@vger.kernel.org [v3.14+]
>
> Imo this part is a bit frisky - if we get positive testing on the xps13
> machines I'd shovel this into 3.15-fixes and then resurrect the old hack
> for stable kernels. Imo this is risky enough that we should attempt to
> backport it before 3.15 is actually released.

I pushed the patches to -fixes per the test reports, and dropped cc:
stable. IOW, this will cover 3.15 and beyond, but we'll still need to do
something about 3.14, the only affected stable. Either way, it'll have
to wait until these get merged to Linus' tree.

Thanks for the review and testing.

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH 2/2] drm/i915: do not setup backlight if not available according to VBT

[Kamal Mostafa]

[-- Attachment #1.1: Type: text/plain, Size: 2331 bytes --]

On Wed, 2014-04-09 at 13:35 +0300, Jani Nikula wrote:
> Some machines use an external EC for controlling the backlight. Info
> about this is present in the VBT. Do not setup native backlight control
> if no PWM backlight is available or supported according to VBT. The
> acpi_backlight interface appears to work for the EC control.
> 
> In most cases there has been no harm done, but it looks like there are
> machines out there that have both an EC and our PWM line connected to
> the same wire. This, obviously, does not end well.
> 
> This should fix the regression caused by
> commit bc0bb9fd1c7810407ab810d204bbaecb255fddde
> Author: Jani Nikula <jani.nikula@intel.com>
> Date:   Thu Nov 14 12:14:29 2013 +0200
> 
>     drm/i915: remove QUIRK_NO_PCH_PWM_ENABLE


Works great!  These patches do indeed re-fix backlight control in v3.14,
on both of the two XPS13 models that had needed the NO_PCH_PWM_ENABLE
quirk.

Tested-by: Kamal Mostafa <kamal@canonical.com>

Thanks Jani!

 -Kamal


> AFAICT the quirk removed by the above commit effectively resulted in
> i915 not driving the backlight PWM output, thus not messing things up.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=76276
> Reference: https://bugzilla.kernel.org/show_bug.cgi?id=47941
> CC: Aaron Lu <aaron.lu@intel.com>
> CC: Kamal Mostafa <kamal@canonical.com>
> CC: Eric Griffith <EGriffith92@gmail.com>
> CC: Kent Baxley <kent.baxley@canonical.com>
> CC: stable@vger.kernel.org [v3.14+]
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_panel.c |    5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
> index cb058408c70e..0eead16aeda7 100644
> --- a/drivers/gpu/drm/i915/intel_panel.c
> +++ b/drivers/gpu/drm/i915/intel_panel.c
> @@ -1065,6 +1065,11 @@ int intel_panel_setup_backlight(struct drm_connector *connector)
>  	unsigned long flags;
>  	int ret;
>  
> +	if (!dev_priv->vbt.backlight.present) {
> +		DRM_DEBUG_KMS("native backlight control not available per VBT\n");
> +		return 0;
> +	}
> +
>  	/* set level and max in panel struct */
>  	spin_lock_irqsave(&dev_priv->backlight_lock, flags);
>  	ret = dev_priv->display.setup_backlight(intel_connector);


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [Intel-gfx] [PATCH 1/2] drm/i915: check VBT for supported backlight type

[Ville Syrjälä]

On Wed, Apr 09, 2014 at 01:35:34PM +0300, Jani Nikula wrote:
> The only supported types are none and PWM. Other values are obsolete or
> reserved, don't add them.
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h   |    1 +
>  drivers/gpu/drm/i915/intel_bios.c |   10 ++++++++++
>  drivers/gpu/drm/i915/intel_bios.h |    3 +++
>  3 files changed, 14 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 55addaaa8222..03f4c866bf3e 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1154,6 +1154,7 @@ struct intel_vbt_data {
>  	struct edp_power_seq edp_pps;
>  
>  	struct {
> +		bool present;
>  		u16 pwm_freq_hz;
>  		bool active_low_pwm;

Could pack the bools next to each other.

Otherwise the series looks good. Obviously I can't tell whether the VBT
can be trusted on this, but at least the code matches the spec I have.

So for the series:
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

>  	} backlight;
> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
> index 9b986775c4b0..862ca04f7a44 100644
> --- a/drivers/gpu/drm/i915/intel_bios.c
> +++ b/drivers/gpu/drm/i915/intel_bios.c
> @@ -309,6 +309,9 @@ parse_lfp_backlight(struct drm_i915_private *dev_priv, struct bdb_header *bdb)
>  	const struct bdb_lfp_backlight_data *backlight_data;
>  	const struct bdb_lfp_backlight_data_entry *entry;
>  
> +	/* Err to enabling backlight if no backlight block. */
> +	dev_priv->vbt.backlight.present = true;
> +
>  	backlight_data = find_section(bdb, BDB_LVDS_BACKLIGHT);
>  	if (!backlight_data)
>  		return;
> @@ -321,6 +324,13 @@ parse_lfp_backlight(struct drm_i915_private *dev_priv, struct bdb_header *bdb)
>  
>  	entry = &backlight_data->data[panel_type];
>  
> +	dev_priv->vbt.backlight.present = entry->type == BDB_BACKLIGHT_TYPE_PWM;
> +	if (!dev_priv->vbt.backlight.present) {
> +		DRM_DEBUG_KMS("PWM backlight not present in VBT (type %u)\n",
> +			      entry->type);
> +		return;
> +	}
> +
>  	dev_priv->vbt.backlight.pwm_freq_hz = entry->pwm_freq_hz;
>  	dev_priv->vbt.backlight.active_low_pwm = entry->active_low_pwm;
>  	DRM_DEBUG_KMS("VBT backlight PWM modulation frequency %u Hz, "
> diff --git a/drivers/gpu/drm/i915/intel_bios.h b/drivers/gpu/drm/i915/intel_bios.h
> index d02e5f93c362..036a79922391 100644
> --- a/drivers/gpu/drm/i915/intel_bios.h
> +++ b/drivers/gpu/drm/i915/intel_bios.h
> @@ -389,6 +389,9 @@ struct bdb_lvds_lfp_data {
>  	struct bdb_lvds_lfp_data_entry data[16];
>  } __packed;
>  
> +#define BDB_BACKLIGHT_TYPE_NONE	0
> +#define BDB_BACKLIGHT_TYPE_PWM	2
> +
>  struct bdb_lfp_backlight_data_entry {
>  	u8 type:2;
>  	u8 active_low_pwm:1;
> -- 
> 1.7.9.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC