[RFC,CABC,v3,2/2] drm/i915: CABC support for backlight control
diff mbox

Message ID 1441946868-4985-3-git-send-email-m.deepak@intel.com
State New
Headers show

Commit Message

Deepak M Sept. 11, 2015, 4:47 a.m. UTC
In CABC (Content Adaptive Brightness Control) content grey level
scale can be increased while simultaneously decreasing
brightness of the backlight to achieve same perceived brightness.

The CABC is not standardized and panel vendors are free to follow
their implementation. The CABC implementaion here assumes that the
panels use standard SW register for control.

In this design there will be no PWM signal from the SoC and DCS
commands are sent to enable and control the backlight brightness.

v2:
- Created a new backlight driver for cabc, which will be registered
  only when it cabc is supported by panel. (Daniel Vetter)
v3:
- Use for_each_dsi_port macro for handling port C also (Gaurav)
- Rebase

Signed-off-by: Deepak M <m.deepak@intel.com>
---
 drivers/gpu/drm/i915/Makefile      |    1 +
 drivers/gpu/drm/i915/intel_dsi.c   |   18 +++++++++++++++---
 drivers/gpu/drm/i915/intel_panel.c |   23 +++++++++++++++++++----
 include/video/mipi_display.h       |    8 ++++++++
 4 files changed, 43 insertions(+), 7 deletions(-)

Comments

Jani Nikula Sept. 11, 2015, 11:28 a.m. UTC | #1
On Fri, 11 Sep 2015, Deepak M <m.deepak@intel.com> wrote:
> In CABC (Content Adaptive Brightness Control) content grey level
> scale can be increased while simultaneously decreasing
> brightness of the backlight to achieve same perceived brightness.
>
> The CABC is not standardized and panel vendors are free to follow
> their implementation. The CABC implementaion here assumes that the
> panels use standard SW register for control.
>
> In this design there will be no PWM signal from the SoC and DCS
> commands are sent to enable and control the backlight brightness.
>
> v2:
> - Created a new backlight driver for cabc, which will be registered
>   only when it cabc is supported by panel. (Daniel Vetter)

I don't know what Daniel's been telling you, but I think this does need
to get bolted into the regular backlight control mechanism. We'll also
need eDP specific backlight control, and there's the VLV/CVH pwm driver
absed backlight control, so we need to cover this more gracefully than
an if-else ladder anyway.

My idea all along has been that the backlight hooks in dev_priv.display
need to be moved to connectors (probably intel_panel), and connector
backlight setup can do what they want with these. All the boilerplate
code for sysfs and scaling and so on would be there already.

I do not approve of the approach here.

BR,
Jani.



> v3:
> - Use for_each_dsi_port macro for handling port C also (Gaurav)
> - Rebase
>
> Signed-off-by: Deepak M <m.deepak@intel.com>
> ---
>  drivers/gpu/drm/i915/Makefile      |    1 +
>  drivers/gpu/drm/i915/intel_dsi.c   |   18 +++++++++++++++---
>  drivers/gpu/drm/i915/intel_panel.c |   23 +++++++++++++++++++----
>  include/video/mipi_display.h       |    8 ++++++++
>  4 files changed, 43 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index 44d290a..d87c690 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -82,6 +82,7 @@ i915-y += dvo_ch7017.o \
>  	  intel_dsi.o \
>  	  intel_dsi_panel_vbt.o \
>  	  intel_dsi_pll.o \
> +	  intel_dsi_cabc.o \
>  	  intel_dvo.o \
>  	  intel_hdmi.o \
>  	  intel_i2c.o \
> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
> index 781c267..1d98ed8 100644
> --- a/drivers/gpu/drm/i915/intel_dsi.c
> +++ b/drivers/gpu/drm/i915/intel_dsi.c
> @@ -35,6 +35,7 @@
>  #include "i915_drv.h"
>  #include "intel_drv.h"
>  #include "intel_dsi.h"
> +#include "intel_dsi_cabc.h"
>  
>  static const struct {
>  	u16 panel_id;
> @@ -398,7 +399,10 @@ static void intel_dsi_enable(struct intel_encoder *encoder)
>  		intel_dsi_port_enable(encoder);
>  	}
>  
> -	intel_panel_enable_backlight(intel_dsi->attached_connector);
> +	if (dev_priv->vbt.dsi.config->cabc_supported)
> +		cabc_enable_backlight(intel_dsi->attached_connector);
> +	else
> +		intel_panel_enable_backlight(intel_dsi->attached_connector);
>  }
>  
>  static void intel_dsi_pre_enable(struct intel_encoder *encoder)
> @@ -458,12 +462,17 @@ static void intel_dsi_enable_nop(struct intel_encoder *encoder)
>  
>  static void intel_dsi_pre_disable(struct intel_encoder *encoder)
>  {
> +	struct drm_device *dev = encoder->base.dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
>  	enum port port;
>  
>  	DRM_DEBUG_KMS("\n");
>  
> -	intel_panel_disable_backlight(intel_dsi->attached_connector);
> +	if (dev_priv->vbt.dsi.config->cabc_supported)
> +		cabc_disable_backlight(intel_dsi->attached_connector);
> +	else
> +		intel_panel_disable_backlight(intel_dsi->attached_connector);
>  
>  	if (is_vid_mode(intel_dsi)) {
>  		/* Send Shutdown command to the panel in LP mode */
> @@ -1133,7 +1142,10 @@ void intel_dsi_init(struct drm_device *dev)
>  	}
>  
>  	intel_panel_init(&intel_connector->panel, fixed_mode, NULL);
> -	intel_panel_setup_backlight(connector, INVALID_PIPE);
> +	if (dev_priv->vbt.dsi.config->cabc_supported)
> +		cabc_setup_backlight(connector, INVALID_PIPE);
> +	else
> +		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 e2ab3f6..ff2e586 100644
> --- a/drivers/gpu/drm/i915/intel_panel.c
> +++ b/drivers/gpu/drm/i915/intel_panel.c
> @@ -34,6 +34,7 @@
>  #include <linux/moduleparam.h>
>  #include <linux/pwm.h>
>  #include "intel_drv.h"
> +#include "intel_dsi_cabc.h"
>  
>  #define CRC_PMIC_PWM_PERIOD_NS	21333
>  
> @@ -1586,15 +1587,29 @@ void intel_panel_fini(struct intel_panel *panel)
>  void intel_backlight_register(struct drm_device *dev)
>  {
>  	struct intel_connector *connector;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
>  
> -	list_for_each_entry(connector, &dev->mode_config.connector_list, base.head)
> -		intel_backlight_device_register(connector);
> +	list_for_each_entry(connector, &dev->mode_config.connector_list,
> +					base.head) {
> +		if (connector->encoder->type == INTEL_OUTPUT_DSI &&
> +			dev_priv->vbt.dsi.config->cabc_supported)
> +			cabc_backlight_device_register(connector);
> +		else
> +			intel_backlight_device_register(connector);
> +	}
>  }
>  
>  void intel_backlight_unregister(struct drm_device *dev)
>  {
>  	struct intel_connector *connector;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
>  
> -	list_for_each_entry(connector, &dev->mode_config.connector_list, base.head)
> -		intel_backlight_device_unregister(connector);
> +	list_for_each_entry(connector, &dev->mode_config.connector_list,
> +					base.head) {
> +		if (connector->encoder->type == INTEL_OUTPUT_DSI &&
> +			dev_priv->vbt.dsi.config->cabc_supported)
> +			cabc_backlight_device_unregister(connector);
> +		else
> +			intel_backlight_device_unregister(connector);
> +	}
>  }
> diff --git a/include/video/mipi_display.h b/include/video/mipi_display.h
> index ddcc8ca..5b8eeec 100644
> --- a/include/video/mipi_display.h
> +++ b/include/video/mipi_display.h
> @@ -117,6 +117,14 @@ enum {
>  	MIPI_DCS_GET_SCANLINE		= 0x45,
>  	MIPI_DCS_READ_DDB_START		= 0xA1,
>  	MIPI_DCS_READ_DDB_CONTINUE	= 0xA8,
> +	MIPI_DCS_CABC_LEVEL_RD          = 0x52,
> +	MIPI_DCS_CABC_MIN_BRIGHTNESS_RD = 0x5F,
> +	MIPI_DCS_CABC_CONTROL_RD        = 0x56,
> +	MIPI_DCS_CABC_CONTROL_BRIGHT_RD = 0x54,
> +	MIPI_DCS_CABC_LEVEL_WR          = 0x51,
> +	MIPI_DCS_CABC_MIN_BRIGHTNESS_WR = 0x5E,
> +	MIPI_DCS_CABC_CONTROL_WR        = 0x55,
> +	MIPI_DCS_CABC_CONTROL_BRIGHT_WR = 0x53,
>  };
>  
>  /* MIPI DCS pixel formats */
> -- 
> 1.7.9.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Daniel Vetter Sept. 14, 2015, 9:39 a.m. UTC | #2
On Fri, Sep 11, 2015 at 02:28:02PM +0300, Jani Nikula wrote:
> On Fri, 11 Sep 2015, Deepak M <m.deepak@intel.com> wrote:
> > In CABC (Content Adaptive Brightness Control) content grey level
> > scale can be increased while simultaneously decreasing
> > brightness of the backlight to achieve same perceived brightness.
> >
> > The CABC is not standardized and panel vendors are free to follow
> > their implementation. The CABC implementaion here assumes that the
> > panels use standard SW register for control.
> >
> > In this design there will be no PWM signal from the SoC and DCS
> > commands are sent to enable and control the backlight brightness.
> >
> > v2:
> > - Created a new backlight driver for cabc, which will be registered
> >   only when it cabc is supported by panel. (Daniel Vetter)
> 
> I don't know what Daniel's been telling you, but I think this does need
> to get bolted into the regular backlight control mechanism. We'll also
> need eDP specific backlight control, and there's the VLV/CVH pwm driver
> absed backlight control, so we need to cover this more gracefully than
> an if-else ladder anyway.
> 
> My idea all along has been that the backlight hooks in dev_priv.display
> need to be moved to connectors (probably intel_panel), and connector
> backlight setup can do what they want with these. All the boilerplate
> code for sysfs and scaling and so on would be there already.
> 
> I do not approve of the approach here.

The current design of backlights in linux is that userspace picks the
right backlight device. We've enabled/disabled the backlight pwm
ourselves too, but that was always just for the native backlight power
thing, not any of the others.

Imo this is the right approach since it fits into the current design.
Fixing the current design so that i915 can get at the right backlight
driver should imo be a separate series. I.e. yes this is what I
recommended roughly (but I didn't check details nor can I test with
xf86-video-intel to make sure it actually works correctly).
-Daniel
Daniel Vetter Sept. 14, 2015, 9:41 a.m. UTC | #3
On Fri, Sep 11, 2015 at 10:17:48AM +0530, Deepak M wrote:
> In CABC (Content Adaptive Brightness Control) content grey level
> scale can be increased while simultaneously decreasing
> brightness of the backlight to achieve same perceived brightness.
> 
> The CABC is not standardized and panel vendors are free to follow
> their implementation. The CABC implementaion here assumes that the
> panels use standard SW register for control.
> 
> In this design there will be no PWM signal from the SoC and DCS
> commands are sent to enable and control the backlight brightness.
> 
> v2:
> - Created a new backlight driver for cabc, which will be registered
>   only when it cabc is supported by panel. (Daniel Vetter)
> v3:
> - Use for_each_dsi_port macro for handling port C also (Gaurav)
> - Rebase
> 
> Signed-off-by: Deepak M <m.deepak@intel.com>
> ---
>  drivers/gpu/drm/i915/Makefile      |    1 +
>  drivers/gpu/drm/i915/intel_dsi.c   |   18 +++++++++++++++---
>  drivers/gpu/drm/i915/intel_panel.c |   23 +++++++++++++++++++----
>  include/video/mipi_display.h       |    8 ++++++++
>  4 files changed, 43 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index 44d290a..d87c690 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -82,6 +82,7 @@ i915-y += dvo_ch7017.o \
>  	  intel_dsi.o \
>  	  intel_dsi_panel_vbt.o \
>  	  intel_dsi_pll.o \
> +	  intel_dsi_cabc.o \

You've forgotten to git add intel_dsi_cabc.c. Makes reviewing things a bit
hard ;-)
-Daniel

>  	  intel_dvo.o \
>  	  intel_hdmi.o \
>  	  intel_i2c.o \
> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
> index 781c267..1d98ed8 100644
> --- a/drivers/gpu/drm/i915/intel_dsi.c
> +++ b/drivers/gpu/drm/i915/intel_dsi.c
> @@ -35,6 +35,7 @@
>  #include "i915_drv.h"
>  #include "intel_drv.h"
>  #include "intel_dsi.h"
> +#include "intel_dsi_cabc.h"
>  
>  static const struct {
>  	u16 panel_id;
> @@ -398,7 +399,10 @@ static void intel_dsi_enable(struct intel_encoder *encoder)
>  		intel_dsi_port_enable(encoder);
>  	}
>  
> -	intel_panel_enable_backlight(intel_dsi->attached_connector);
> +	if (dev_priv->vbt.dsi.config->cabc_supported)
> +		cabc_enable_backlight(intel_dsi->attached_connector);
> +	else
> +		intel_panel_enable_backlight(intel_dsi->attached_connector);
>  }
>  
>  static void intel_dsi_pre_enable(struct intel_encoder *encoder)
> @@ -458,12 +462,17 @@ static void intel_dsi_enable_nop(struct intel_encoder *encoder)
>  
>  static void intel_dsi_pre_disable(struct intel_encoder *encoder)
>  {
> +	struct drm_device *dev = encoder->base.dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
>  	enum port port;
>  
>  	DRM_DEBUG_KMS("\n");
>  
> -	intel_panel_disable_backlight(intel_dsi->attached_connector);
> +	if (dev_priv->vbt.dsi.config->cabc_supported)
> +		cabc_disable_backlight(intel_dsi->attached_connector);
> +	else
> +		intel_panel_disable_backlight(intel_dsi->attached_connector);
>  
>  	if (is_vid_mode(intel_dsi)) {
>  		/* Send Shutdown command to the panel in LP mode */
> @@ -1133,7 +1142,10 @@ void intel_dsi_init(struct drm_device *dev)
>  	}
>  
>  	intel_panel_init(&intel_connector->panel, fixed_mode, NULL);
> -	intel_panel_setup_backlight(connector, INVALID_PIPE);
> +	if (dev_priv->vbt.dsi.config->cabc_supported)
> +		cabc_setup_backlight(connector, INVALID_PIPE);
> +	else
> +		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 e2ab3f6..ff2e586 100644
> --- a/drivers/gpu/drm/i915/intel_panel.c
> +++ b/drivers/gpu/drm/i915/intel_panel.c
> @@ -34,6 +34,7 @@
>  #include <linux/moduleparam.h>
>  #include <linux/pwm.h>
>  #include "intel_drv.h"
> +#include "intel_dsi_cabc.h"
>  
>  #define CRC_PMIC_PWM_PERIOD_NS	21333
>  
> @@ -1586,15 +1587,29 @@ void intel_panel_fini(struct intel_panel *panel)
>  void intel_backlight_register(struct drm_device *dev)
>  {
>  	struct intel_connector *connector;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
>  
> -	list_for_each_entry(connector, &dev->mode_config.connector_list, base.head)
> -		intel_backlight_device_register(connector);
> +	list_for_each_entry(connector, &dev->mode_config.connector_list,
> +					base.head) {
> +		if (connector->encoder->type == INTEL_OUTPUT_DSI &&
> +			dev_priv->vbt.dsi.config->cabc_supported)
> +			cabc_backlight_device_register(connector);
> +		else
> +			intel_backlight_device_register(connector);
> +	}
>  }
>  
>  void intel_backlight_unregister(struct drm_device *dev)
>  {
>  	struct intel_connector *connector;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
>  
> -	list_for_each_entry(connector, &dev->mode_config.connector_list, base.head)
> -		intel_backlight_device_unregister(connector);
> +	list_for_each_entry(connector, &dev->mode_config.connector_list,
> +					base.head) {
> +		if (connector->encoder->type == INTEL_OUTPUT_DSI &&
> +			dev_priv->vbt.dsi.config->cabc_supported)
> +			cabc_backlight_device_unregister(connector);
> +		else
> +			intel_backlight_device_unregister(connector);
> +	}
>  }
> diff --git a/include/video/mipi_display.h b/include/video/mipi_display.h
> index ddcc8ca..5b8eeec 100644
> --- a/include/video/mipi_display.h
> +++ b/include/video/mipi_display.h
> @@ -117,6 +117,14 @@ enum {
>  	MIPI_DCS_GET_SCANLINE		= 0x45,
>  	MIPI_DCS_READ_DDB_START		= 0xA1,
>  	MIPI_DCS_READ_DDB_CONTINUE	= 0xA8,
> +	MIPI_DCS_CABC_LEVEL_RD          = 0x52,
> +	MIPI_DCS_CABC_MIN_BRIGHTNESS_RD = 0x5F,
> +	MIPI_DCS_CABC_CONTROL_RD        = 0x56,
> +	MIPI_DCS_CABC_CONTROL_BRIGHT_RD = 0x54,
> +	MIPI_DCS_CABC_LEVEL_WR          = 0x51,
> +	MIPI_DCS_CABC_MIN_BRIGHTNESS_WR = 0x5E,
> +	MIPI_DCS_CABC_CONTROL_WR        = 0x55,
> +	MIPI_DCS_CABC_CONTROL_BRIGHT_WR = 0x53,
>  };
>  
>  /* MIPI DCS pixel formats */
> -- 
> 1.7.9.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Deepak M Sept. 14, 2015, 9:41 a.m. UTC | #4
>-----Original Message-----
>From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel
>Vetter
>Sent: Monday, September 14, 2015 3:12 PM
>To: Deepak, M
>Cc: intel-gfx@lists.freedesktop.org
>Subject: Re: [Intel-gfx] [RFC CABC v3 PATCH 2/2] drm/i915: CABC support for
>backlight control
>
>On Fri, Sep 11, 2015 at 10:17:48AM +0530, Deepak M wrote:
>> In CABC (Content Adaptive Brightness Control) content grey level scale
>> can be increased while simultaneously decreasing brightness of the
>> backlight to achieve same perceived brightness.
>>
>> The CABC is not standardized and panel vendors are free to follow
>> their implementation. The CABC implementaion here assumes that the
>> panels use standard SW register for control.
>>
>> In this design there will be no PWM signal from the SoC and DCS
>> commands are sent to enable and control the backlight brightness.
>>
>> v2:
>> - Created a new backlight driver for cabc, which will be registered
>>   only when it cabc is supported by panel. (Daniel Vetter)
>> v3:
>> - Use for_each_dsi_port macro for handling port C also (Gaurav)
>> - Rebase
>>
>> Signed-off-by: Deepak M <m.deepak@intel.com>
>> ---
>>  drivers/gpu/drm/i915/Makefile      |    1 +
>>  drivers/gpu/drm/i915/intel_dsi.c   |   18 +++++++++++++++---
>>  drivers/gpu/drm/i915/intel_panel.c |   23 +++++++++++++++++++----
>>  include/video/mipi_display.h       |    8 ++++++++
>>  4 files changed, 43 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/Makefile
>> b/drivers/gpu/drm/i915/Makefile index 44d290a..d87c690 100644
>> --- a/drivers/gpu/drm/i915/Makefile
>> +++ b/drivers/gpu/drm/i915/Makefile
>> @@ -82,6 +82,7 @@ i915-y += dvo_ch7017.o \
>>  	  intel_dsi.o \
>>  	  intel_dsi_panel_vbt.o \
>>  	  intel_dsi_pll.o \
>> +	  intel_dsi_cabc.o \
>
>You've forgotten to git add intel_dsi_cabc.c. Makes reviewing things a bit hard
>;-) -Daniel
[Deepak M] Extremely sorry, git am didn't worked had did patch -p1 but got forgot to do a git am :), will update the patch.
>
>>  	  intel_dvo.o \
>>  	  intel_hdmi.o \
>>  	  intel_i2c.o \
>> diff --git a/drivers/gpu/drm/i915/intel_dsi.c
>> b/drivers/gpu/drm/i915/intel_dsi.c
>> index 781c267..1d98ed8 100644
>> --- a/drivers/gpu/drm/i915/intel_dsi.c
>> +++ b/drivers/gpu/drm/i915/intel_dsi.c
>> @@ -35,6 +35,7 @@
>>  #include "i915_drv.h"
>>  #include "intel_drv.h"
>>  #include "intel_dsi.h"
>> +#include "intel_dsi_cabc.h"
>>
>>  static const struct {
>>  	u16 panel_id;
>> @@ -398,7 +399,10 @@ static void intel_dsi_enable(struct intel_encoder
>*encoder)
>>  		intel_dsi_port_enable(encoder);
>>  	}
>>
>> -	intel_panel_enable_backlight(intel_dsi->attached_connector);
>> +	if (dev_priv->vbt.dsi.config->cabc_supported)
>> +		cabc_enable_backlight(intel_dsi->attached_connector);
>> +	else
>> +		intel_panel_enable_backlight(intel_dsi-
>>attached_connector);
>>  }
>>
>>  static void intel_dsi_pre_enable(struct intel_encoder *encoder) @@
>> -458,12 +462,17 @@ static void intel_dsi_enable_nop(struct
>> intel_encoder *encoder)
>>
>>  static void intel_dsi_pre_disable(struct intel_encoder *encoder)  {
>> +	struct drm_device *dev = encoder->base.dev;
>> +	struct drm_i915_private *dev_priv = dev->dev_private;
>>  	struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
>>  	enum port port;
>>
>>  	DRM_DEBUG_KMS("\n");
>>
>> -	intel_panel_disable_backlight(intel_dsi->attached_connector);
>> +	if (dev_priv->vbt.dsi.config->cabc_supported)
>> +		cabc_disable_backlight(intel_dsi->attached_connector);
>> +	else
>> +		intel_panel_disable_backlight(intel_dsi-
>>attached_connector);
>>
>>  	if (is_vid_mode(intel_dsi)) {
>>  		/* Send Shutdown command to the panel in LP mode */ @@ -
>1133,7
>> +1142,10 @@ void intel_dsi_init(struct drm_device *dev)
>>  	}
>>
>>  	intel_panel_init(&intel_connector->panel, fixed_mode, NULL);
>> -	intel_panel_setup_backlight(connector, INVALID_PIPE);
>> +	if (dev_priv->vbt.dsi.config->cabc_supported)
>> +		cabc_setup_backlight(connector, INVALID_PIPE);
>> +	else
>> +		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 e2ab3f6..ff2e586 100644
>> --- a/drivers/gpu/drm/i915/intel_panel.c
>> +++ b/drivers/gpu/drm/i915/intel_panel.c
>> @@ -34,6 +34,7 @@
>>  #include <linux/moduleparam.h>
>>  #include <linux/pwm.h>
>>  #include "intel_drv.h"
>> +#include "intel_dsi_cabc.h"
>>
>>  #define CRC_PMIC_PWM_PERIOD_NS	21333
>>
>> @@ -1586,15 +1587,29 @@ void intel_panel_fini(struct intel_panel
>> *panel)  void intel_backlight_register(struct drm_device *dev)  {
>>  	struct intel_connector *connector;
>> +	struct drm_i915_private *dev_priv = dev->dev_private;
>>
>> -	list_for_each_entry(connector, &dev->mode_config.connector_list,
>base.head)
>> -		intel_backlight_device_register(connector);
>> +	list_for_each_entry(connector, &dev->mode_config.connector_list,
>> +					base.head) {
>> +		if (connector->encoder->type == INTEL_OUTPUT_DSI &&
>> +			dev_priv->vbt.dsi.config->cabc_supported)
>> +			cabc_backlight_device_register(connector);
>> +		else
>> +			intel_backlight_device_register(connector);
>> +	}
>>  }
>>
>>  void intel_backlight_unregister(struct drm_device *dev)  {
>>  	struct intel_connector *connector;
>> +	struct drm_i915_private *dev_priv = dev->dev_private;
>>
>> -	list_for_each_entry(connector, &dev->mode_config.connector_list,
>base.head)
>> -		intel_backlight_device_unregister(connector);
>> +	list_for_each_entry(connector, &dev->mode_config.connector_list,
>> +					base.head) {
>> +		if (connector->encoder->type == INTEL_OUTPUT_DSI &&
>> +			dev_priv->vbt.dsi.config->cabc_supported)
>> +			cabc_backlight_device_unregister(connector);
>> +		else
>> +			intel_backlight_device_unregister(connector);
>> +	}
>>  }
>> diff --git a/include/video/mipi_display.h
>> b/include/video/mipi_display.h index ddcc8ca..5b8eeec 100644
>> --- a/include/video/mipi_display.h
>> +++ b/include/video/mipi_display.h
>> @@ -117,6 +117,14 @@ enum {
>>  	MIPI_DCS_GET_SCANLINE		= 0x45,
>>  	MIPI_DCS_READ_DDB_START		= 0xA1,
>>  	MIPI_DCS_READ_DDB_CONTINUE	= 0xA8,
>> +	MIPI_DCS_CABC_LEVEL_RD          = 0x52,
>> +	MIPI_DCS_CABC_MIN_BRIGHTNESS_RD = 0x5F,
>> +	MIPI_DCS_CABC_CONTROL_RD        = 0x56,
>> +	MIPI_DCS_CABC_CONTROL_BRIGHT_RD = 0x54,
>> +	MIPI_DCS_CABC_LEVEL_WR          = 0x51,
>> +	MIPI_DCS_CABC_MIN_BRIGHTNESS_WR = 0x5E,
>> +	MIPI_DCS_CABC_CONTROL_WR        = 0x55,
>> +	MIPI_DCS_CABC_CONTROL_BRIGHT_WR = 0x53,
>>  };
>>
>>  /* MIPI DCS pixel formats */
>> --
>> 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
>http://blog.ffwll.ch
Jani Nikula Sept. 14, 2015, 10:10 a.m. UTC | #5
On Mon, 14 Sep 2015, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Fri, Sep 11, 2015 at 02:28:02PM +0300, Jani Nikula wrote:
>> On Fri, 11 Sep 2015, Deepak M <m.deepak@intel.com> wrote:
>> > In CABC (Content Adaptive Brightness Control) content grey level
>> > scale can be increased while simultaneously decreasing
>> > brightness of the backlight to achieve same perceived brightness.
>> >
>> > The CABC is not standardized and panel vendors are free to follow
>> > their implementation. The CABC implementaion here assumes that the
>> > panels use standard SW register for control.
>> >
>> > In this design there will be no PWM signal from the SoC and DCS
>> > commands are sent to enable and control the backlight brightness.
>> >
>> > v2:
>> > - Created a new backlight driver for cabc, which will be registered
>> >   only when it cabc is supported by panel. (Daniel Vetter)
>> 
>> I don't know what Daniel's been telling you, but I think this does need
>> to get bolted into the regular backlight control mechanism. We'll also
>> need eDP specific backlight control, and there's the VLV/CVH pwm driver
>> absed backlight control, so we need to cover this more gracefully than
>> an if-else ladder anyway.
>> 
>> My idea all along has been that the backlight hooks in dev_priv.display
>> need to be moved to connectors (probably intel_panel), and connector
>> backlight setup can do what they want with these. All the boilerplate
>> code for sysfs and scaling and so on would be there already.
>> 
>> I do not approve of the approach here.
>
> The current design of backlights in linux is that userspace picks the
> right backlight device. We've enabled/disabled the backlight pwm
> ourselves too, but that was always just for the native backlight power
> thing, not any of the others.

What does that have to do with this series?

> Imo this is the right approach since it fits into the current design.

No, it does *not* fit into the current design, which you can see from
the if ladders there.

> Fixing the current design so that i915 can get at the right backlight
> driver should imo be a separate series. I.e. yes this is what I
> recommended roughly (but I didn't check details nor can I test with
> xf86-video-intel to make sure it actually works correctly).

It is important this gets done the right way up front, because there's
now two series in flight to rip up the backlight code that's now neat
and tidy.

I am *not* volunteering to clean up the backlight code again. I've
already done it once.

BR,
Jani.


> -Daniel
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
Jani Nikula Sept. 14, 2015, 12:34 p.m. UTC | #6
On Mon, 14 Sep 2015, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> On Mon, 14 Sep 2015, Daniel Vetter <daniel@ffwll.ch> wrote:
>> On Fri, Sep 11, 2015 at 02:28:02PM +0300, Jani Nikula wrote:
>>> On Fri, 11 Sep 2015, Deepak M <m.deepak@intel.com> wrote:
>>> > In CABC (Content Adaptive Brightness Control) content grey level
>>> > scale can be increased while simultaneously decreasing
>>> > brightness of the backlight to achieve same perceived brightness.
>>> >
>>> > The CABC is not standardized and panel vendors are free to follow
>>> > their implementation. The CABC implementaion here assumes that the
>>> > panels use standard SW register for control.
>>> >
>>> > In this design there will be no PWM signal from the SoC and DCS
>>> > commands are sent to enable and control the backlight brightness.
>>> >
>>> > v2:
>>> > - Created a new backlight driver for cabc, which will be registered
>>> >   only when it cabc is supported by panel. (Daniel Vetter)
>>> 
>>> I don't know what Daniel's been telling you, but I think this does need
>>> to get bolted into the regular backlight control mechanism. We'll also
>>> need eDP specific backlight control, and there's the VLV/CVH pwm driver
>>> absed backlight control, so we need to cover this more gracefully than
>>> an if-else ladder anyway.
>>> 
>>> My idea all along has been that the backlight hooks in dev_priv.display
>>> need to be moved to connectors (probably intel_panel), and connector
>>> backlight setup can do what they want with these. All the boilerplate
>>> code for sysfs and scaling and so on would be there already.
>>> 
>>> I do not approve of the approach here.
>>
>> The current design of backlights in linux is that userspace picks the
>> right backlight device. We've enabled/disabled the backlight pwm
>> ourselves too, but that was always just for the native backlight power
>> thing, not any of the others.
>
> What does that have to do with this series?
>
>> Imo this is the right approach since it fits into the current design.
>
> No, it does *not* fit into the current design, which you can see from
> the if ladders there.
>
>> Fixing the current design so that i915 can get at the right backlight
>> driver should imo be a separate series. I.e. yes this is what I
>> recommended roughly (but I didn't check details nor can I test with
>> xf86-video-intel to make sure it actually works correctly).
>
> It is important this gets done the right way up front, because there's
> now two series in flight to rip up the backlight code that's now neat
> and tidy.
>
> I am *not* volunteering to clean up the backlight code again. I've
> already done it once.

Here's how you can actually do this nicely [1].

BR,
Jani.


[1] http://mid.gmane.org/cover.1442227790.git.jani.nikula@intel.com



>
> BR,
> Jani.
>
>
>> -Daniel
>> -- 
>> Daniel Vetter
>> Software Engineer, Intel Corporation
>> http://blog.ffwll.ch
>
> -- 
> Jani Nikula, Intel Open Source Technology Center
Daniel Vetter Sept. 14, 2015, 1:19 p.m. UTC | #7
On Mon, Sep 14, 2015 at 01:10:01PM +0300, Jani Nikula wrote:
> On Mon, 14 Sep 2015, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Fri, Sep 11, 2015 at 02:28:02PM +0300, Jani Nikula wrote:
> >> On Fri, 11 Sep 2015, Deepak M <m.deepak@intel.com> wrote:
> >> > In CABC (Content Adaptive Brightness Control) content grey level
> >> > scale can be increased while simultaneously decreasing
> >> > brightness of the backlight to achieve same perceived brightness.
> >> >
> >> > The CABC is not standardized and panel vendors are free to follow
> >> > their implementation. The CABC implementaion here assumes that the
> >> > panels use standard SW register for control.
> >> >
> >> > In this design there will be no PWM signal from the SoC and DCS
> >> > commands are sent to enable and control the backlight brightness.
> >> >
> >> > v2:
> >> > - Created a new backlight driver for cabc, which will be registered
> >> >   only when it cabc is supported by panel. (Daniel Vetter)
> >> 
> >> I don't know what Daniel's been telling you, but I think this does need
> >> to get bolted into the regular backlight control mechanism. We'll also
> >> need eDP specific backlight control, and there's the VLV/CVH pwm driver
> >> absed backlight control, so we need to cover this more gracefully than
> >> an if-else ladder anyway.
> >> 
> >> My idea all along has been that the backlight hooks in dev_priv.display
> >> need to be moved to connectors (probably intel_panel), and connector
> >> backlight setup can do what they want with these. All the boilerplate
> >> code for sysfs and scaling and so on would be there already.
> >> 
> >> I do not approve of the approach here.
> >
> > The current design of backlights in linux is that userspace picks the
> > right backlight device. We've enabled/disabled the backlight pwm
> > ourselves too, but that was always just for the native backlight power
> > thing, not any of the others.
> 
> What does that have to do with this series?
> 
> > Imo this is the right approach since it fits into the current design.
> 
> No, it does *not* fit into the current design, which you can see from
> the if ladders there.

Well those if ladders are partially breaking the current design, we've
never done that for any of the other backlight drivers. For those
userspace is expected to call them before a modeset.

Adding if ladders to the kernel is probably not needed if userspace holds
up it's part of the deal.

> > Fixing the current design so that i915 can get at the right backlight
> > driver should imo be a separate series. I.e. yes this is what I
> > recommended roughly (but I didn't check details nor can I test with
> > xf86-video-intel to make sure it actually works correctly).
> 
> It is important this gets done the right way up front, because there's
> now two series in flight to rip up the backlight code that's now neat
> and tidy.
> 
> I am *not* volunteering to clean up the backlight code again. I've
> already done it once.

This isn't about cleaning up the i915 backlight code, it's about pulling
the backlight selection logic from libbacklight into the kernel. At least
for the if ladders in the encoder enable/disable hooks. I assumed that
neither for capc and for the edp dpcd backlight we'd need those (userspace
should frob the backlight for us).
-Daniel
Jani Nikula Sept. 14, 2015, 1:59 p.m. UTC | #8
On Mon, 14 Sep 2015, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Mon, Sep 14, 2015 at 01:10:01PM +0300, Jani Nikula wrote:
>> On Mon, 14 Sep 2015, Daniel Vetter <daniel@ffwll.ch> wrote:
>> > On Fri, Sep 11, 2015 at 02:28:02PM +0300, Jani Nikula wrote:
>> >> On Fri, 11 Sep 2015, Deepak M <m.deepak@intel.com> wrote:
>> >> > In CABC (Content Adaptive Brightness Control) content grey level
>> >> > scale can be increased while simultaneously decreasing
>> >> > brightness of the backlight to achieve same perceived brightness.
>> >> >
>> >> > The CABC is not standardized and panel vendors are free to follow
>> >> > their implementation. The CABC implementaion here assumes that the
>> >> > panels use standard SW register for control.
>> >> >
>> >> > In this design there will be no PWM signal from the SoC and DCS
>> >> > commands are sent to enable and control the backlight brightness.
>> >> >
>> >> > v2:
>> >> > - Created a new backlight driver for cabc, which will be registered
>> >> >   only when it cabc is supported by panel. (Daniel Vetter)
>> >> 
>> >> I don't know what Daniel's been telling you, but I think this does need
>> >> to get bolted into the regular backlight control mechanism. We'll also
>> >> need eDP specific backlight control, and there's the VLV/CVH pwm driver
>> >> absed backlight control, so we need to cover this more gracefully than
>> >> an if-else ladder anyway.
>> >> 
>> >> My idea all along has been that the backlight hooks in dev_priv.display
>> >> need to be moved to connectors (probably intel_panel), and connector
>> >> backlight setup can do what they want with these. All the boilerplate
>> >> code for sysfs and scaling and so on would be there already.
>> >> 
>> >> I do not approve of the approach here.
>> >
>> > The current design of backlights in linux is that userspace picks the
>> > right backlight device. We've enabled/disabled the backlight pwm
>> > ourselves too, but that was always just for the native backlight power
>> > thing, not any of the others.
>> 
>> What does that have to do with this series?
>> 
>> > Imo this is the right approach since it fits into the current design.
>> 
>> No, it does *not* fit into the current design, which you can see from
>> the if ladders there.
>
> Well those if ladders are partially breaking the current design, we've
> never done that for any of the other backlight drivers. For those
> userspace is expected to call them before a modeset.
>
> Adding if ladders to the kernel is probably not needed if userspace holds
> up it's part of the deal.
>
>> > Fixing the current design so that i915 can get at the right backlight
>> > driver should imo be a separate series. I.e. yes this is what I
>> > recommended roughly (but I didn't check details nor can I test with
>> > xf86-video-intel to make sure it actually works correctly).
>> 
>> It is important this gets done the right way up front, because there's
>> now two series in flight to rip up the backlight code that's now neat
>> and tidy.
>> 
>> I am *not* volunteering to clean up the backlight code again. I've
>> already done it once.
>
> This isn't about cleaning up the i915 backlight code, it's about pulling
> the backlight selection logic from libbacklight into the kernel. At least
> for the if ladders in the encoder enable/disable hooks. I assumed that
> neither for capc and for the edp dpcd backlight we'd need those (userspace
> should frob the backlight for us).

I can't be bothered to summarize the rather frustrated IRC discussion on
this, but the bottom line is that the kernel implementation should be
done using the backlight hooks approach I suggested in [1]. That does
not exclude the possibility of exposing more backlight interfaces if
needed, but it keeps the kernel implementation neat.

BR,
Jani.


[1] http://mid.gmane.org/cover.1442227790.git.jani.nikula@intel.com



> -Daniel
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

Patch
diff mbox

diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index 44d290a..d87c690 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -82,6 +82,7 @@  i915-y += dvo_ch7017.o \
 	  intel_dsi.o \
 	  intel_dsi_panel_vbt.o \
 	  intel_dsi_pll.o \
+	  intel_dsi_cabc.o \
 	  intel_dvo.o \
 	  intel_hdmi.o \
 	  intel_i2c.o \
diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
index 781c267..1d98ed8 100644
--- a/drivers/gpu/drm/i915/intel_dsi.c
+++ b/drivers/gpu/drm/i915/intel_dsi.c
@@ -35,6 +35,7 @@ 
 #include "i915_drv.h"
 #include "intel_drv.h"
 #include "intel_dsi.h"
+#include "intel_dsi_cabc.h"
 
 static const struct {
 	u16 panel_id;
@@ -398,7 +399,10 @@  static void intel_dsi_enable(struct intel_encoder *encoder)
 		intel_dsi_port_enable(encoder);
 	}
 
-	intel_panel_enable_backlight(intel_dsi->attached_connector);
+	if (dev_priv->vbt.dsi.config->cabc_supported)
+		cabc_enable_backlight(intel_dsi->attached_connector);
+	else
+		intel_panel_enable_backlight(intel_dsi->attached_connector);
 }
 
 static void intel_dsi_pre_enable(struct intel_encoder *encoder)
@@ -458,12 +462,17 @@  static void intel_dsi_enable_nop(struct intel_encoder *encoder)
 
 static void intel_dsi_pre_disable(struct intel_encoder *encoder)
 {
+	struct drm_device *dev = encoder->base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
 	enum port port;
 
 	DRM_DEBUG_KMS("\n");
 
-	intel_panel_disable_backlight(intel_dsi->attached_connector);
+	if (dev_priv->vbt.dsi.config->cabc_supported)
+		cabc_disable_backlight(intel_dsi->attached_connector);
+	else
+		intel_panel_disable_backlight(intel_dsi->attached_connector);
 
 	if (is_vid_mode(intel_dsi)) {
 		/* Send Shutdown command to the panel in LP mode */
@@ -1133,7 +1142,10 @@  void intel_dsi_init(struct drm_device *dev)
 	}
 
 	intel_panel_init(&intel_connector->panel, fixed_mode, NULL);
-	intel_panel_setup_backlight(connector, INVALID_PIPE);
+	if (dev_priv->vbt.dsi.config->cabc_supported)
+		cabc_setup_backlight(connector, INVALID_PIPE);
+	else
+		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 e2ab3f6..ff2e586 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -34,6 +34,7 @@ 
 #include <linux/moduleparam.h>
 #include <linux/pwm.h>
 #include "intel_drv.h"
+#include "intel_dsi_cabc.h"
 
 #define CRC_PMIC_PWM_PERIOD_NS	21333
 
@@ -1586,15 +1587,29 @@  void intel_panel_fini(struct intel_panel *panel)
 void intel_backlight_register(struct drm_device *dev)
 {
 	struct intel_connector *connector;
+	struct drm_i915_private *dev_priv = dev->dev_private;
 
-	list_for_each_entry(connector, &dev->mode_config.connector_list, base.head)
-		intel_backlight_device_register(connector);
+	list_for_each_entry(connector, &dev->mode_config.connector_list,
+					base.head) {
+		if (connector->encoder->type == INTEL_OUTPUT_DSI &&
+			dev_priv->vbt.dsi.config->cabc_supported)
+			cabc_backlight_device_register(connector);
+		else
+			intel_backlight_device_register(connector);
+	}
 }
 
 void intel_backlight_unregister(struct drm_device *dev)
 {
 	struct intel_connector *connector;
+	struct drm_i915_private *dev_priv = dev->dev_private;
 
-	list_for_each_entry(connector, &dev->mode_config.connector_list, base.head)
-		intel_backlight_device_unregister(connector);
+	list_for_each_entry(connector, &dev->mode_config.connector_list,
+					base.head) {
+		if (connector->encoder->type == INTEL_OUTPUT_DSI &&
+			dev_priv->vbt.dsi.config->cabc_supported)
+			cabc_backlight_device_unregister(connector);
+		else
+			intel_backlight_device_unregister(connector);
+	}
 }
diff --git a/include/video/mipi_display.h b/include/video/mipi_display.h
index ddcc8ca..5b8eeec 100644
--- a/include/video/mipi_display.h
+++ b/include/video/mipi_display.h
@@ -117,6 +117,14 @@  enum {
 	MIPI_DCS_GET_SCANLINE		= 0x45,
 	MIPI_DCS_READ_DDB_START		= 0xA1,
 	MIPI_DCS_READ_DDB_CONTINUE	= 0xA8,
+	MIPI_DCS_CABC_LEVEL_RD          = 0x52,
+	MIPI_DCS_CABC_MIN_BRIGHTNESS_RD = 0x5F,
+	MIPI_DCS_CABC_CONTROL_RD        = 0x56,
+	MIPI_DCS_CABC_CONTROL_BRIGHT_RD = 0x54,
+	MIPI_DCS_CABC_LEVEL_WR          = 0x51,
+	MIPI_DCS_CABC_MIN_BRIGHTNESS_WR = 0x5E,
+	MIPI_DCS_CABC_CONTROL_WR        = 0x55,
+	MIPI_DCS_CABC_CONTROL_BRIGHT_WR = 0x53,
 };
 
 /* MIPI DCS pixel formats */