Re: 3.13 i915 brightness settings broken when going from docked -> undocked

[Jesse Barnes <jbarnes@virtuousgeek.org>]

On Sun, 23 Feb 2014 10:50:59 -0500
Josh Boyer <jwboyer@fedoraproject.org> wrote:

> On Thu, Feb 20, 2014 at 07:31:41PM -0500, Josh Boyer wrote:
> >On Wed, Feb 19, 2014 at 9:20 PM, Josh Boyer <jwboyer@fedoraproject.org> wrote:
> >> Hi All,
> >>
> >> We've had a rather weird report[1] of the brightness adjustments being
> >> broken in a specific case with Thinkpad x220 hardware (SandyBridge
> >> based).  If you boot the machine with it in a dock and then undock,
> >> the brightness adjustments do not work.  That is with either the FN
> >> keys or the GNOME brightness slider.
> >>
> >> I can see that the value of
> >> /sys/class/backlight/acpi_video0/brightness increases/decreases but
> >> /sys/class/backlight/intel_backlight/brightness doesn't reflect any
> >> changes.  With 3.12 this works, and oddly with 3.14-rc1 it works
> >> (specifically, it starts working around v3.13-10231-g53d8ab2 which is
> >> right after the first DRM merge for 3.14).  With 3.13, if I undock and
> >> echo a higher value in the intel_backlight_brightness sysfs entry, the
> >> brightness will actually increase so it can be done manually, but it
> >> does not work as you'd expect.
> >>
> >> I'm in the middle of trying to do a reverse bisect for which patch
> >> fixes it in the 3.14-rcX series, but that's taking a while.  I thought
> >> I'd email and see if anyone already knows about this situation, what
> >> patch in 3.13 broke this, and which one then fixed it again.  Thus far
> >> all I've gathered is that backlight handling is confusing.
> >
> >The reverse bisect between 3.13 and 3.14-rc1 didn't prove fruitful,
> >either because I messed it up or there's a combination of things that
> >fix the issue.  So instead I did a regular git bisect between 3.12 and
> >3.13 to see which commit _broke_ things and caused the above behavior.
> > That landed me at:
> >
> >Author: Jesse Barnes <jbarnes@virtuousgeek.org>
> >Date:   Thu Oct 31 18:55:49 2013 +0200
> >
> >    drm/i915: make backlight functions take a connector
> >
> >I have no idea if that makes sense or not, but it's at least something
> >that seems to be in a relevant area of code.  Does anyone involved in
> >that know why it would cause the above symptoms on 3.13, and which
> >commit(s) fix it in 3.14-rc1?
> 
> Since nobody is replying I poked around a bit myself.  The one commit
> that looks somewhat relevant in 3.14-rc1 seems to be:
> 
> commit c91c9f32843a1b433de5a1ead4789a6bc8d3d914
> Author: Jani Nikula <jani.nikula@intel.com>
> Date:   Fri Nov 8 16:48:55 2013 +0200
> 
>     drm/i915: make asle notifications update backlight on all connectors
> 
> That doesn't apply cleanly on 3.13 because of the other reworks that
> went in first, so I came up with the patch below.  It seems to fix it
> for my machine, but I'm waiting for confirmation from the original bug
> reporter still.  Maybe this will elicit some comments?
> 
> josh
> 
> Backport of upstream commit c91c9f328
> 
> ---
>  drivers/gpu/drm/i915/i915_drv.h       |  1 +
>  drivers/gpu/drm/i915/intel_opregion.c | 31 ++++++-------------------------
>  drivers/gpu/drm/i915/intel_panel.c    |  4 ++++
>  3 files changed, 11 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 221ac62..d6d4349 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1371,6 +1371,7 @@ typedef struct drm_i915_private {
>  
>  	/* backlight */
>  	struct {
> +		bool present;
>  		int level;
>  		bool enabled;
>  		spinlock_t lock; /* bl registers and the above bl fields */
> diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
> index 6d69a9b..b2a51ae 100644
> --- a/drivers/gpu/drm/i915/intel_opregion.c
> +++ b/drivers/gpu/drm/i915/intel_opregion.c
> @@ -414,38 +414,19 @@ static u32 asle_set_backlight(struct drm_device *dev, u32 bclp)
>  		return ASLC_BACKLIGHT_FAILED;
>  
>  	mutex_lock(&dev->mode_config.mutex);
> -	/*
> -	 * Could match the OpRegion connector here instead, but we'd also need
> -	 * to verify the connector could handle a backlight call.
> -	 */
> -	list_for_each_entry(encoder, &dev->mode_config.encoder_list, head)
> -		if (encoder->crtc == crtc) {
> -			found = true;
> -			break;
> -		}
> -
> -	if (!found) {
> -		ret = ASLC_BACKLIGHT_FAILED;
> -		goto out;
> -	}
>  
> -	list_for_each_entry(connector, &dev->mode_config.connector_list, head)
> -		if (connector->encoder == encoder)
> -			intel_connector = to_intel_connector(connector);
> -
> -	if (!intel_connector) {
> -		ret = ASLC_BACKLIGHT_FAILED;
> -		goto out;
> +	DRM_DEBUG_KMS("updating opregion backlight %d/255\n", bclp);
> +	list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
> +		intel_connector = to_intel_connector(connector);
> +		if (dev_priv->backlight.present)
> +			intel_panel_set_backlight(intel_connector, bclp, 255);
>  	}
>  
> -	DRM_DEBUG_KMS("updating opregion backlight %d/255\n", bclp);
> -	intel_panel_set_backlight(intel_connector, bclp, 255);
>  	iowrite32(DIV_ROUND_UP(bclp * 100, 255) | ASLE_CBLV_VALID, &asle->cblv);
>  
> -out:
>  	mutex_unlock(&dev->mode_config.mutex);
>  
> -	return ret;
> +	return 0;
>  }
>  
>  static u32 asle_set_als_illum(struct drm_device *dev, u32 alsi)
> diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
> index e6f782d..fa7b984 100644
> --- a/drivers/gpu/drm/i915/intel_panel.c
> +++ b/drivers/gpu/drm/i915/intel_panel.c
> @@ -832,6 +832,9 @@ int intel_panel_setup_backlight(struct drm_connector *connector)
>  		dev_priv->backlight.device = NULL;
>  		return -ENODEV;
>  	}
> +
> +	dev_priv->backlight.present = true;
> +
>  	return 0;
>  }
>  
> @@ -839,6 +842,7 @@ void intel_panel_destroy_backlight(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	if (dev_priv->backlight.device) {
> +		dev_priv->backlight.present = false;
>  		backlight_device_unregister(dev_priv->backlight.device);
>  		dev_priv->backlight.device = NULL;
>  	}

Yeah I think it looks reasonable, I was waiting for Jani to get back
since I think he's thought about this more.

Fundamentally, mapping from an OpRegion connector to a drm connector is
what we need, and your bits look a little closer to that than the
current code.

-- 
Jesse Barnes, Intel Open Source Technology Center