[PATCH 1/2] drm/i915: keep backlight_level and backlight device brightness in sync

[PATCH 1/2] drm/i915: keep backlight_level and backlight device brightness in sync

[Jani Nikula]

A single point of truth would be better than two, but achieving that would
require more abstractions for CONFIG_BACKLIGHT_CLASS_DEVICE=n with not a
whole lot of real benefits. Take the short route and just keep the
backlight levels in sync. In particular, update backlight device brightness
on opregion brightness changes.

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

diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
index a3730e0..725d726 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -287,6 +287,9 @@ void intel_panel_set_backlight(struct drm_device *dev, u32 level)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
 	dev_priv->backlight_level = level;
+	if (dev_priv->backlight)
+		dev_priv->backlight->props.brightness = level;
+
 	if (dev_priv->backlight_enabled)
 		intel_panel_actually_set_backlight(dev, level);
 }
@@ -318,8 +321,12 @@ void intel_panel_enable_backlight(struct drm_device *dev,
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
-	if (dev_priv->backlight_level == 0)
+	if (dev_priv->backlight_level == 0) {
 		dev_priv->backlight_level = intel_panel_get_max_backlight(dev);
+		if (dev_priv->backlight)
+			dev_priv->backlight->props.brightness =
+				dev_priv->backlight_level;
+	}
 
 	dev_priv->backlight_enabled = true;
 	intel_panel_actually_set_backlight(dev, dev_priv->backlight_level);
@@ -427,6 +434,7 @@ int intel_panel_setup_backlight(struct drm_connector *connector)
 
 	memset(&props, 0, sizeof(props));
 	props.type = BACKLIGHT_RAW;
+	props.brightness = dev_priv->backlight_level;
 	props.max_brightness = _intel_panel_get_max_backlight(dev);
 	if (props.max_brightness == 0) {
 		DRM_DEBUG_DRIVER("Failed to get maximum backlight value\n");
@@ -443,7 +451,6 @@ int intel_panel_setup_backlight(struct drm_connector *connector)
 		dev_priv->backlight = NULL;
 		return -ENODEV;
 	}
-	dev_priv->backlight->props.brightness = intel_panel_get_backlight(dev);
 	return 0;
 }
 
-- 
1.7.9.5

[PATCH 2/2] drm/i915: return actual brightness to .get_brightness callback

[Jani Nikula]

The backlight device .get_brightness callback is supposed to return the
actual brightness by querying the hardware, not the cached value. See
Documentation/ABI/stable/sysfs-class-backlight. The callback is there to
support the actual_brightness sysfs file.

With the backlight_level and backlight device brightness now in sync, one
can reliably get the cached value through the brightness sysfs file.

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

diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
index 725d726..ca2179d 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -415,8 +415,7 @@ static int intel_panel_update_status(struct backlight_device *bd)
 static int intel_panel_get_brightness(struct backlight_device *bd)
 {
 	struct drm_device *dev = bl_get_data(bd);
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	return dev_priv->backlight_level;
+	return intel_panel_get_backlight(dev);
 }
 
 static const struct backlight_ops intel_panel_bl_ops = {
-- 
1.7.9.5

Re: [PATCH 1/2] drm/i915: keep backlight_level and backlight device brightness in sync

[Jani Nikula]

Any comments on these two patches?

BR,
Jani.


On Tue, 12 Mar 2013, Jani Nikula <jani.nikula@intel.com> wrote:
> A single point of truth would be better than two, but achieving that would
> require more abstractions for CONFIG_BACKLIGHT_CLASS_DEVICE=n with not a
> whole lot of real benefits. Take the short route and just keep the
> backlight levels in sync. In particular, update backlight device brightness
> on opregion brightness changes.
>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_panel.c |   11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
> index a3730e0..725d726 100644
> --- a/drivers/gpu/drm/i915/intel_panel.c
> +++ b/drivers/gpu/drm/i915/intel_panel.c
> @@ -287,6 +287,9 @@ void intel_panel_set_backlight(struct drm_device *dev, u32 level)
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  
>  	dev_priv->backlight_level = level;
> +	if (dev_priv->backlight)
> +		dev_priv->backlight->props.brightness = level;
> +
>  	if (dev_priv->backlight_enabled)
>  		intel_panel_actually_set_backlight(dev, level);
>  }
> @@ -318,8 +321,12 @@ void intel_panel_enable_backlight(struct drm_device *dev,
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  
> -	if (dev_priv->backlight_level == 0)
> +	if (dev_priv->backlight_level == 0) {
>  		dev_priv->backlight_level = intel_panel_get_max_backlight(dev);
> +		if (dev_priv->backlight)
> +			dev_priv->backlight->props.brightness =
> +				dev_priv->backlight_level;
> +	}
>  
>  	dev_priv->backlight_enabled = true;
>  	intel_panel_actually_set_backlight(dev, dev_priv->backlight_level);
> @@ -427,6 +434,7 @@ int intel_panel_setup_backlight(struct drm_connector *connector)
>  
>  	memset(&props, 0, sizeof(props));
>  	props.type = BACKLIGHT_RAW;
> +	props.brightness = dev_priv->backlight_level;
>  	props.max_brightness = _intel_panel_get_max_backlight(dev);
>  	if (props.max_brightness == 0) {
>  		DRM_DEBUG_DRIVER("Failed to get maximum backlight value\n");
> @@ -443,7 +451,6 @@ int intel_panel_setup_backlight(struct drm_connector *connector)
>  		dev_priv->backlight = NULL;
>  		return -ENODEV;
>  	}
> -	dev_priv->backlight->props.brightness = intel_panel_get_backlight(dev);
>  	return 0;
>  }
>  
> -- 
> 1.7.9.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915: keep backlight_level and backlight device brightness in sync

[Daniel Vetter]

On Mon, Mar 25, 2013 at 02:56:39PM +0200, Jani Nikula wrote:
> 
> Any comments on these two patches?

Both queued for -next, thanks for the patches. That means though that
you're volunteered for more cleanup ;-)
-Daniel

> 
> BR,
> Jani.
> 
> 
> On Tue, 12 Mar 2013, Jani Nikula <jani.nikula@intel.com> wrote:
> > A single point of truth would be better than two, but achieving that would
> > require more abstractions for CONFIG_BACKLIGHT_CLASS_DEVICE=n with not a
> > whole lot of real benefits. Take the short route and just keep the
> > backlight levels in sync. In particular, update backlight device brightness
> > on opregion brightness changes.
> >
> > Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_panel.c |   11 +++++++++--
> >  1 file changed, 9 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
> > index a3730e0..725d726 100644
> > --- a/drivers/gpu/drm/i915/intel_panel.c
> > +++ b/drivers/gpu/drm/i915/intel_panel.c
> > @@ -287,6 +287,9 @@ void intel_panel_set_backlight(struct drm_device *dev, u32 level)
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  
> >  	dev_priv->backlight_level = level;
> > +	if (dev_priv->backlight)
> > +		dev_priv->backlight->props.brightness = level;
> > +
> >  	if (dev_priv->backlight_enabled)
> >  		intel_panel_actually_set_backlight(dev, level);
> >  }
> > @@ -318,8 +321,12 @@ void intel_panel_enable_backlight(struct drm_device *dev,
> >  {
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  
> > -	if (dev_priv->backlight_level == 0)
> > +	if (dev_priv->backlight_level == 0) {
> >  		dev_priv->backlight_level = intel_panel_get_max_backlight(dev);
> > +		if (dev_priv->backlight)
> > +			dev_priv->backlight->props.brightness =
> > +				dev_priv->backlight_level;
> > +	}
> >  
> >  	dev_priv->backlight_enabled = true;
> >  	intel_panel_actually_set_backlight(dev, dev_priv->backlight_level);
> > @@ -427,6 +434,7 @@ int intel_panel_setup_backlight(struct drm_connector *connector)
> >  
> >  	memset(&props, 0, sizeof(props));
> >  	props.type = BACKLIGHT_RAW;
> > +	props.brightness = dev_priv->backlight_level;
> >  	props.max_brightness = _intel_panel_get_max_backlight(dev);
> >  	if (props.max_brightness == 0) {
> >  		DRM_DEBUG_DRIVER("Failed to get maximum backlight value\n");
> > @@ -443,7 +451,6 @@ int intel_panel_setup_backlight(struct drm_connector *connector)
> >  		dev_priv->backlight = NULL;
> >  		return -ENODEV;
> >  	}
> > -	dev_priv->backlight->props.brightness = intel_panel_get_backlight(dev);
> >  	return 0;
> >  }
> >  
> > -- 
> > 1.7.9.5
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> _______________________________________________
> 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