[RFC CABC v3 PATCH 0/2] CABC patch list

[RFC CABC v3 PATCH 0/2] CABC patch list

[Deepak M]

CABC stands for the Content Adaptive Backlight Control.
In the normal display the backlight which we see is due to the
backlight which is being modulated by the filter, which is inturn
dependent on the image. In brief the CABC does the histogram
analysis of the image and then controls the filter and backlight.
For example in CABC to display the dark image the backlight is dimmed
and then controlls the filter to allow more light, because of
which is power consuption will be reduced.

Below are the inital set of patches which supports the CABC.
A field exits in the mipi configuration of the VBT which
when enabled indiactes the CABC is supported. Depending on
this filed the backlight control function pointer are
initalized in the intel_panel.c file.

In case of dual link panels depending on the panel
the DCS commands have to be send to either PORT A,
PORT C or both PORT A and PORT C. Again a filed is
added in the VBT to get this data from the version 197 onwards.
One of the below patches parses these fields from the
VBT.

v3:
- removed the first patch from the list as it was not necessary
- addressed Daniel and Gaurav`s comments.

Deepak M (2):
  drm/i915: Parsing the PWM cntrl and CABC ON/OFF fileds in VBT
  drm/i915: CABC support for backlight control

 drivers/gpu/drm/i915/Makefile      |    1 +
 drivers/gpu/drm/i915/intel_bios.c  |   13 +++++++++++++
 drivers/gpu/drm/i915/intel_bios.h  |    5 ++++-
 drivers/gpu/drm/i915/intel_dsi.c   |   18 +++++++++++++++---
 drivers/gpu/drm/i915/intel_dsi.h   |    2 ++
 drivers/gpu/drm/i915/intel_panel.c |   23 +++++++++++++++++++----
 include/video/mipi_display.h       |    8 ++++++++
 7 files changed, 62 insertions(+), 8 deletions(-)

-- 
1.7.9.5

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

[RFC CABC v3 PATCH 1/2] drm/i915: Parsing the PWM cntrl and CABC ON/OFF fileds in VBT

[Deepak M]

For dual link panel scenarios there are new fileds added in the
VBT which indicate on which port the PWM cntrl and CABC ON/OFF
commands needs to be sent.

v2: Rebase
v3: Rebase

Signed-off-by: Deepak M <m.deepak@intel.com>
---
 drivers/gpu/drm/i915/intel_bios.c |   13 +++++++++++++
 drivers/gpu/drm/i915/intel_bios.h |    5 ++++-
 drivers/gpu/drm/i915/intel_dsi.h  |    2 ++
 3 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
index c8acc29..dacfadd 100644
--- a/drivers/gpu/drm/i915/intel_bios.c
+++ b/drivers/gpu/drm/i915/intel_bios.c
@@ -789,6 +789,19 @@ parse_mipi(struct drm_i915_private *dev_priv, const struct bdb_header *bdb)
 		return;
 	}
 
+	/*
+	 * These below bits will inform us on which port the panel blk_cntrl and
+	 * CABC ON/OFF commands needs to be sent in case of dual link panels
+	 *	u16 dl_cabc_port:2;
+	 *	u16 pwm_bkl_ctrl:2;
+	 * But these are introduced from the VBT version 197 onwards, so making
+	 * sure that these bits are zero in the pervious versions.
+	 */
+	if (dev_priv->vbt.dsi.config->dual_link && bdb->version < 197) {
+		dev_priv->vbt.dsi.config->dl_cabc_port = 0;
+		dev_priv->vbt.dsi.config->pwm_bkl_ctrl = 0;
+	}
+
 	/* We have mandatory mipi config blocks. Initialize as generic panel */
 	dev_priv->vbt.dsi.panel_id = MIPI_DSI_GENERIC_PANEL_ID;
 
diff --git a/drivers/gpu/drm/i915/intel_bios.h b/drivers/gpu/drm/i915/intel_bios.h
index 1b7417e..544b51d 100644
--- a/drivers/gpu/drm/i915/intel_bios.h
+++ b/drivers/gpu/drm/i915/intel_bios.h
@@ -834,7 +834,10 @@ struct mipi_config {
 	u16 dual_link:2;
 	u16 lane_cnt:2;
 	u16 pixel_overlap:3;
-	u16 rsvd3:9;
+	u16 rgb_flip:1;
+	u16 dl_cabc_port:2;
+	u16 pwm_bkl_ctrl:2;
+	u16 rsvd3:4;
 
 	u16 rsvd4;
 
diff --git a/drivers/gpu/drm/i915/intel_dsi.h b/drivers/gpu/drm/i915/intel_dsi.h
index 42a6859..c727d7b 100644
--- a/drivers/gpu/drm/i915/intel_dsi.h
+++ b/drivers/gpu/drm/i915/intel_dsi.h
@@ -74,6 +74,8 @@ struct intel_dsi {
 
 	u8 escape_clk_div;
 	u8 dual_link;
+	u8 dl_cabc_port;
+	u8 pwm_blk_ctrl;
 	u8 pixel_overlap;
 	u32 port_bits;
 	u32 bw_timer;
-- 
1.7.9.5

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

[RFC CABC v3 PATCH 2/2] drm/i915: CABC support for backlight control

[Deepak M]

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 \
 	  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

Re: [RFC CABC v3 PATCH 2/2] drm/i915: CABC support for backlight control

[Jani Nikula]

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

-- 
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: [RFC CABC v3 PATCH 2/2] drm/i915: CABC support for backlight control

[Daniel Vetter]

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
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC CABC v3 PATCH 2/2] drm/i915: CABC support for backlight control

[Jani Nikula]

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, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC CABC v3 PATCH 2/2] drm/i915: CABC support for backlight control

[Jani Nikula]

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

-- 
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: [RFC CABC v3 PATCH 2/2] drm/i915: CABC support for backlight control

[Daniel Vetter]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC CABC v3 PATCH 2/2] drm/i915: CABC support for backlight control

[Jani Nikula]

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

-- 
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: [RFC CABC v3 PATCH 2/2] drm/i915: CABC support for backlight control

[Daniel Vetter]

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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC CABC v3 PATCH 2/2] drm/i915: CABC support for backlight control

[Deepak, M]

>-----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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] drm/i915: CABC support for backlight control

[Deepak M]

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_dsi_cabc.c |  310 +++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_dsi_cabc.h |   46 +++++
 drivers/gpu/drm/i915/intel_panel.c    |   23 ++-
 include/video/mipi_display.h          |    8 +
 6 files changed, 399 insertions(+), 7 deletions(-)
 create mode 100644 drivers/gpu/drm/i915/intel_dsi_cabc.c
 create mode 100644 drivers/gpu/drm/i915/intel_dsi_cabc.h

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_dsi_cabc.c b/drivers/gpu/drm/i915/intel_dsi_cabc.c
new file mode 100644
index 0000000..c2fcd95
--- /dev/null
+++ b/drivers/gpu/drm/i915/intel_dsi_cabc.c
@@ -0,0 +1,310 @@
+/*
+ * Copyright © 2006-2010 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
+ * DEALINGS IN THE SOFTWARE.
+ *
+ * Author: Deepak M <m.deepak@intel.com>
+ */
+
+#include <linux/kernel.h>
+#include <linux/moduleparam.h>
+#include "intel_drv.h"
+#include "intel_dsi.h"
+#include "i915_drv.h"
+#include "intel_dsi_cabc.h"
+#include <video/mipi_display.h>
+#include <drm/drm_mipi_dsi.h>
+
+static inline enum port get_cabc_port(struct intel_dsi *intel_dsi)
+{
+	if (intel_dsi->dl_cabc_port == CABC_PORT_C)
+		return PORT_C;
+	else
+		return PORT_A;
+}
+
+int cabc_setup_backlight(struct drm_connector *connector,
+		enum pipe unused)
+{
+	struct drm_device *dev = connector->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_connector *intel_connector = to_intel_connector(connector);
+	struct intel_panel *panel = &intel_connector->panel;
+
+	if (dev_priv->vbt.backlight.present)
+		panel->backlight.present = true;
+	else {
+		DRM_ERROR("no backlight present per VBT\n");
+		return 0;
+	}
+
+	panel->backlight.max = CABC_MAX_VALUE;
+	panel->backlight.level = CABC_MAX_VALUE;
+
+	return 0;
+}
+
+u32 cabc_get_backlight(struct intel_connector *connector)
+{
+	struct drm_device *dev = connector->base.dev;
+	struct intel_dsi *intel_dsi = NULL;
+	struct drm_crtc *crtc = NULL;
+	struct intel_encoder *encoder = NULL;
+	struct mipi_dsi_device *dsi_device;
+	u8 data[2] = {0};
+	enum port port;
+	u32 ret;
+
+	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
+		for_each_encoder_on_crtc(dev, crtc, encoder) {
+			if (encoder->type == INTEL_OUTPUT_DSI) {
+				intel_dsi = enc_to_intel_dsi(&encoder->base);
+				break;
+			}
+		}
+		if (intel_dsi != NULL)
+			break;
+	}
+
+	for_each_dsi_port(port, intel_dsi->ports) {
+		if (intel_dsi->dual_link)
+			port = get_cabc_port(intel_dsi);
+
+		dsi_device = intel_dsi->dsi_hosts[port]->device;
+		mipi_dsi_dcs_read(dsi_device, MIPI_DCS_CABC_LEVEL_RD, data, 2);
+
+		if (intel_dsi->dual_link &&
+				!(intel_dsi->dl_cabc_port == CABC_PORT_A_AND_C))
+			break;
+	}
+
+	ret = data[1];
+
+	return ret;
+}
+
+void cabc_set_backlight(struct intel_connector *connector, u32 level)
+{
+	struct drm_device *dev = connector->base.dev;
+	struct intel_dsi *intel_dsi = NULL;
+	struct drm_crtc *crtc = NULL;
+	struct intel_encoder *encoder = NULL;
+	struct mipi_dsi_device *dsi_device;
+	u8 data[2] = {0};
+	enum port port;
+
+	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
+		for_each_encoder_on_crtc(dev, crtc, encoder) {
+			if (encoder->type == INTEL_OUTPUT_DSI) {
+				intel_dsi = enc_to_intel_dsi(&encoder->base);
+				break;
+			}
+		}
+		if (intel_dsi != NULL)
+			break;
+	}
+
+	for_each_dsi_port(port, intel_dsi->ports) {
+		if (intel_dsi->dual_link)
+			port = get_cabc_port(intel_dsi);
+
+		dsi_device = intel_dsi->dsi_hosts[port]->device;
+		data[1] = level;
+		data[0] = MIPI_DCS_CABC_LEVEL_WR;
+		mipi_dsi_dcs_write_buffer(dsi_device, data, 2);
+
+		if (intel_dsi->dual_link &&
+			!(intel_dsi->dl_cabc_port == CABC_PORT_A_AND_C))
+			break;
+	}
+}
+
+void cabc_enable_backlight(struct intel_connector *connector)
+{
+	struct drm_device *dev = connector->base.dev;
+	struct intel_dsi *intel_dsi = NULL;
+	struct drm_crtc *crtc = NULL;
+	struct intel_encoder *encoder = NULL;
+	struct intel_panel *panel = &connector->panel;
+	struct mipi_dsi_device *dsi_device;
+	enum port port;
+	u8 data[2] = {0};
+
+	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
+		for_each_encoder_on_crtc(dev, crtc, encoder) {
+			if (encoder->type == INTEL_OUTPUT_DSI) {
+				intel_dsi = enc_to_intel_dsi(&encoder->base);
+				break;
+			}
+		}
+		if (intel_dsi != NULL)
+			break;
+	}
+
+	for_each_dsi_port(port, intel_dsi->ports) {
+		if (intel_dsi->dual_link)
+			port = get_cabc_port(intel_dsi);
+
+		dsi_device = intel_dsi->dsi_hosts[port]->device;
+		data[0] = MIPI_DCS_CABC_CONTROL_BRIGHT_WR;
+		data[1] = CABC_BACKLIGHT | CABC_DIMMING_DISPLAY | CABC_BCTRL;
+		mipi_dsi_dcs_write_buffer(dsi_device, data, 2);
+		data[0] = MIPI_DCS_CABC_CONTROL_WR;
+		data[1] = CABC_STILL_PICTURE;
+		mipi_dsi_dcs_write_buffer(dsi_device, data, 2);
+
+		if (intel_dsi->dual_link &&
+				!(intel_dsi->dl_cabc_port == CABC_PORT_A_AND_C))
+			break;
+	}
+
+	cabc_set_backlight(connector, panel->backlight.level);
+}
+
+void cabc_disable_backlight(struct intel_connector *connector)
+{
+	struct drm_device *dev = connector->base.dev;
+	struct intel_dsi *intel_dsi = NULL;
+	struct drm_crtc *crtc = NULL;
+	struct intel_encoder *encoder = NULL;
+	struct mipi_dsi_device *dsi_device;
+	enum port port;
+	u8 data[2] = {0};
+
+	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
+		for_each_encoder_on_crtc(dev, crtc, encoder) {
+			if (encoder->type == INTEL_OUTPUT_DSI) {
+				intel_dsi = enc_to_intel_dsi(&encoder->base);
+				break;
+			}
+		}
+		if (intel_dsi != NULL)
+			break;
+	}
+
+	cabc_set_backlight(connector, 0);
+
+	for_each_dsi_port(port, intel_dsi->ports) {
+		if (intel_dsi->dual_link)
+			port = get_cabc_port(intel_dsi);
+		dsi_device = intel_dsi->dsi_hosts[port]->device;
+
+		data[1] = CABC_OFF;
+		data[0] = MIPI_DCS_CABC_CONTROL_WR;
+		mipi_dsi_dcs_write_buffer(dsi_device, data, 2);
+		data[0] = MIPI_DCS_CABC_CONTROL_BRIGHT_WR;
+		mipi_dsi_dcs_write_buffer(dsi_device, data, 2);
+
+		if (intel_dsi->dual_link &&
+				!(intel_dsi->dl_cabc_port == CABC_PORT_A_AND_C))
+			break;
+	}
+}
+static int cabc_backlight_device_update_status(struct backlight_device *bd)
+{
+	struct intel_connector *connector = bl_get_data(bd);
+	struct drm_device *dev = connector->base.dev;
+
+	drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
+	DRM_DEBUG_KMS("updating intel_backlight, brightness=%d/%d\n",
+			bd->props.brightness, bd->props.max_brightness);
+
+	cabc_set_backlight(connector, bd->props.brightness);
+
+	drm_modeset_unlock(&dev->mode_config.connection_mutex);
+
+	return 0;
+}
+
+static int cabc_backlight_device_get_brightness(struct backlight_device *bd)
+{
+	struct intel_connector *connector = bl_get_data(bd);
+	struct drm_device *dev = connector->base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	int ret;
+
+	intel_runtime_pm_get(dev_priv);
+	drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
+
+	ret = cabc_get_backlight(connector);
+
+	drm_modeset_unlock(&dev->mode_config.connection_mutex);
+	intel_runtime_pm_put(dev_priv);
+
+	return ret;
+}
+
+static const struct backlight_ops cabc_backlight_device_ops = {
+	.update_status = cabc_backlight_device_update_status,
+	.get_brightness = cabc_backlight_device_get_brightness,
+};
+
+int cabc_backlight_device_register(struct intel_connector *connector)
+{
+	struct intel_panel *panel = &connector->panel;
+	struct backlight_properties props;
+
+	if (WARN_ON(panel->backlight.device))
+		return -ENODEV;
+
+	if (!panel->backlight.present) {
+		DRM_ERROR("No backlight present\n");
+		return 0;
+	}
+
+	WARN_ON(panel->backlight.max == 0);
+
+	memset(&props, 0, sizeof(props));
+	props.type = BACKLIGHT_FIRMWARE;
+	props.max_brightness = panel->backlight.max;
+	props.brightness = props.max_brightness;
+
+	/*
+	 * Note: using the same name independent of the connector prevents
+	 * registration of multiple backlight devices in the driver.
+	 */
+	panel->backlight.device =
+		backlight_device_register("intel_backlight",
+				connector->base.kdev,
+				connector,
+				&cabc_backlight_device_ops, &props);
+
+	if (IS_ERR(panel->backlight.device)) {
+		DRM_ERROR("Failed to register backlight: %ld\n",
+				PTR_ERR(panel->backlight.device));
+		panel->backlight.device = NULL;
+		return -ENODEV;
+	}
+
+	DRM_DEBUG_KMS("Connector %s backlight sysfs interface registered\n",
+			connector->base.name);
+
+	return 0;
+}
+
+void cabc_backlight_device_unregister(struct intel_connector *connector)
+{
+	struct intel_panel *panel = &connector->panel;
+
+	if (panel->backlight.device) {
+		backlight_device_unregister(panel->backlight.device);
+		panel->backlight.device = NULL;
+	}
+}
diff --git a/drivers/gpu/drm/i915/intel_dsi_cabc.h b/drivers/gpu/drm/i915/intel_dsi_cabc.h
new file mode 100644
index 0000000..b1a79d9
--- /dev/null
+++ b/drivers/gpu/drm/i915/intel_dsi_cabc.h
@@ -0,0 +1,46 @@
+/*
+ * Copyright © 2013 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
+ * DEALINGS IN THE SOFTWARE.
+ *
+ * Author: Deepak M <m.deepak@intel.com>
+ */
+
+extern int cabc_backlight_device_register(struct intel_connector *connector);
+extern void cabc_backlight_device_unregister(struct intel_connector *connector);
+void cabc_set_backlight(struct intel_connector *connector, u32 level);
+u32 cabc_get_backlight(struct intel_connector *connector);
+void cabc_enable_backlight(struct intel_connector *connector);
+void cabc_disable_backlight(struct intel_connector *connector);
+int cabc_setup_backlight(struct drm_connector *connector, enum pipe unused);
+
+#define CABC_OFF			(0 << 0)
+#define CABC_USER_INTERFACE_IMAGE	(1 << 0)
+#define CABC_STILL_PICTURE		(2 << 0)
+#define CABC_VIDEO_MODE			(3 << 0)
+
+#define CABC_BACKLIGHT			(1 << 2)
+#define CABC_DIMMING_DISPLAY		(1 << 3)
+#define CABC_BCTRL			(1 << 5)
+
+#define CABC_PORT_A			0x00
+#define CABC_PORT_C			0x01
+#define CABC_PORT_A_AND_C		0x02
+#define CABC_MAX_VALUE			0xff
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

Re: [PATCH] drm/i915: CABC support for backlight control

[Jani Nikula]

On Mon, 14 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.

I think CABC is a confusing name for this. At the high level, this
should be called, say, DCS backlight control. I think CABC is a special
case of backlight control in the DSI panel; there are panels with
brightness control via DCS *without* CABC.

See further comments inline.

>
> 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_dsi_cabc.c |  310 +++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_dsi_cabc.h |   46 +++++
>  drivers/gpu/drm/i915/intel_panel.c    |   23 ++-
>  include/video/mipi_display.h          |    8 +
>  6 files changed, 399 insertions(+), 7 deletions(-)
>  create mode 100644 drivers/gpu/drm/i915/intel_dsi_cabc.c
>  create mode 100644 drivers/gpu/drm/i915/intel_dsi_cabc.h
>
> 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);

See my patch at [1] for how I think all of this should be handled.

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

>  }
>  
>  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_dsi_cabc.c b/drivers/gpu/drm/i915/intel_dsi_cabc.c
> new file mode 100644
> index 0000000..c2fcd95
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/intel_dsi_cabc.c
> @@ -0,0 +1,310 @@
> +/*
> + * Copyright © 2006-2010 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> + * DEALINGS IN THE SOFTWARE.
> + *
> + * Author: Deepak M <m.deepak@intel.com>
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/moduleparam.h>
> +#include "intel_drv.h"
> +#include "intel_dsi.h"
> +#include "i915_drv.h"
> +#include "intel_dsi_cabc.h"
> +#include <video/mipi_display.h>
> +#include <drm/drm_mipi_dsi.h>
> +
> +static inline enum port get_cabc_port(struct intel_dsi *intel_dsi)
> +{
> +	if (intel_dsi->dl_cabc_port == CABC_PORT_C)
> +		return PORT_C;
> +	else
> +		return PORT_A;
> +}
> +
> +int cabc_setup_backlight(struct drm_connector *connector,
> +		enum pipe unused)
> +{
> +	struct drm_device *dev = connector->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct intel_connector *intel_connector = to_intel_connector(connector);
> +	struct intel_panel *panel = &intel_connector->panel;
> +
> +	if (dev_priv->vbt.backlight.present)
> +		panel->backlight.present = true;
> +	else {
> +		DRM_ERROR("no backlight present per VBT\n");
> +		return 0;
> +	}
> +
> +	panel->backlight.max = CABC_MAX_VALUE;
> +	panel->backlight.level = CABC_MAX_VALUE;
> +
> +	return 0;
> +}
> +
> +u32 cabc_get_backlight(struct intel_connector *connector)
> +{
> +	struct drm_device *dev = connector->base.dev;
> +	struct intel_dsi *intel_dsi = NULL;
> +	struct drm_crtc *crtc = NULL;
> +	struct intel_encoder *encoder = NULL;
> +	struct mipi_dsi_device *dsi_device;
> +	u8 data[2] = {0};
> +	enum port port;
> +	u32 ret;
> +
> +	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
> +		for_each_encoder_on_crtc(dev, crtc, encoder) {
> +			if (encoder->type == INTEL_OUTPUT_DSI) {
> +				intel_dsi = enc_to_intel_dsi(&encoder->base);
> +				break;
> +			}
> +		}
> +		if (intel_dsi != NULL)
> +			break;
> +	}

You can get at intel_dsi via connector->encoder. No need for this kind
of loops.

Same for similar loops below.

> +
> +	for_each_dsi_port(port, intel_dsi->ports) {
> +		if (intel_dsi->dual_link)
> +			port = get_cabc_port(intel_dsi);
> +
> +		dsi_device = intel_dsi->dsi_hosts[port]->device;
> +		mipi_dsi_dcs_read(dsi_device, MIPI_DCS_CABC_LEVEL_RD, data, 2);
> +
> +		if (intel_dsi->dual_link &&
> +				!(intel_dsi->dl_cabc_port == CABC_PORT_A_AND_C))
> +			break;
> +	}

That is a really confusing way to do the dcs command on one or more
ports. How about making dl_cabc_port (hopefully renamed to something
referring to dcs rather than cabc) a bitmap of ports similar to
intel_dsi->ports, so you can make this loop simply:

	for_each_dsi_port(port, dcs_backlight_ports)
        	...

There'd be no need for special casing and conditional breaks out of the
loop or anything.

Same for similar loops below.

> +
> +	ret = data[1];
> +
> +	return ret;
> +}
> +
> +void cabc_set_backlight(struct intel_connector *connector, u32 level)
> +{
> +	struct drm_device *dev = connector->base.dev;
> +	struct intel_dsi *intel_dsi = NULL;
> +	struct drm_crtc *crtc = NULL;
> +	struct intel_encoder *encoder = NULL;
> +	struct mipi_dsi_device *dsi_device;
> +	u8 data[2] = {0};
> +	enum port port;
> +
> +	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
> +		for_each_encoder_on_crtc(dev, crtc, encoder) {
> +			if (encoder->type == INTEL_OUTPUT_DSI) {
> +				intel_dsi = enc_to_intel_dsi(&encoder->base);
> +				break;
> +			}
> +		}
> +		if (intel_dsi != NULL)
> +			break;
> +	}
> +
> +	for_each_dsi_port(port, intel_dsi->ports) {
> +		if (intel_dsi->dual_link)
> +			port = get_cabc_port(intel_dsi);
> +
> +		dsi_device = intel_dsi->dsi_hosts[port]->device;
> +		data[1] = level;
> +		data[0] = MIPI_DCS_CABC_LEVEL_WR;
> +		mipi_dsi_dcs_write_buffer(dsi_device, data, 2);
> +
> +		if (intel_dsi->dual_link &&
> +			!(intel_dsi->dl_cabc_port == CABC_PORT_A_AND_C))
> +			break;
> +	}
> +}
> +
> +void cabc_enable_backlight(struct intel_connector *connector)
> +{
> +	struct drm_device *dev = connector->base.dev;
> +	struct intel_dsi *intel_dsi = NULL;
> +	struct drm_crtc *crtc = NULL;
> +	struct intel_encoder *encoder = NULL;
> +	struct intel_panel *panel = &connector->panel;
> +	struct mipi_dsi_device *dsi_device;
> +	enum port port;
> +	u8 data[2] = {0};
> +
> +	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
> +		for_each_encoder_on_crtc(dev, crtc, encoder) {
> +			if (encoder->type == INTEL_OUTPUT_DSI) {
> +				intel_dsi = enc_to_intel_dsi(&encoder->base);
> +				break;
> +			}
> +		}
> +		if (intel_dsi != NULL)
> +			break;
> +	}
> +
> +	for_each_dsi_port(port, intel_dsi->ports) {
> +		if (intel_dsi->dual_link)
> +			port = get_cabc_port(intel_dsi);
> +
> +		dsi_device = intel_dsi->dsi_hosts[port]->device;
> +		data[0] = MIPI_DCS_CABC_CONTROL_BRIGHT_WR;
> +		data[1] = CABC_BACKLIGHT | CABC_DIMMING_DISPLAY | CABC_BCTRL;
> +		mipi_dsi_dcs_write_buffer(dsi_device, data, 2);
> +		data[0] = MIPI_DCS_CABC_CONTROL_WR;
> +		data[1] = CABC_STILL_PICTURE;
> +		mipi_dsi_dcs_write_buffer(dsi_device, data, 2);
> +
> +		if (intel_dsi->dual_link &&
> +				!(intel_dsi->dl_cabc_port == CABC_PORT_A_AND_C))
> +			break;
> +	}
> +
> +	cabc_set_backlight(connector, panel->backlight.level);
> +}
> +
> +void cabc_disable_backlight(struct intel_connector *connector)
> +{
> +	struct drm_device *dev = connector->base.dev;
> +	struct intel_dsi *intel_dsi = NULL;
> +	struct drm_crtc *crtc = NULL;
> +	struct intel_encoder *encoder = NULL;
> +	struct mipi_dsi_device *dsi_device;
> +	enum port port;
> +	u8 data[2] = {0};
> +
> +	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
> +		for_each_encoder_on_crtc(dev, crtc, encoder) {
> +			if (encoder->type == INTEL_OUTPUT_DSI) {
> +				intel_dsi = enc_to_intel_dsi(&encoder->base);
> +				break;
> +			}
> +		}
> +		if (intel_dsi != NULL)
> +			break;
> +	}
> +
> +	cabc_set_backlight(connector, 0);
> +
> +	for_each_dsi_port(port, intel_dsi->ports) {
> +		if (intel_dsi->dual_link)
> +			port = get_cabc_port(intel_dsi);
> +		dsi_device = intel_dsi->dsi_hosts[port]->device;
> +
> +		data[1] = CABC_OFF;
> +		data[0] = MIPI_DCS_CABC_CONTROL_WR;
> +		mipi_dsi_dcs_write_buffer(dsi_device, data, 2);
> +		data[0] = MIPI_DCS_CABC_CONTROL_BRIGHT_WR;
> +		mipi_dsi_dcs_write_buffer(dsi_device, data, 2);
> +
> +		if (intel_dsi->dual_link &&
> +				!(intel_dsi->dl_cabc_port == CABC_PORT_A_AND_C))
> +			break;
> +	}
> +}
> +static int cabc_backlight_device_update_status(struct backlight_device *bd)
> +{
> +	struct intel_connector *connector = bl_get_data(bd);
> +	struct drm_device *dev = connector->base.dev;
> +
> +	drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
> +	DRM_DEBUG_KMS("updating intel_backlight, brightness=%d/%d\n",
> +			bd->props.brightness, bd->props.max_brightness);
> +
> +	cabc_set_backlight(connector, bd->props.brightness);
> +
> +	drm_modeset_unlock(&dev->mode_config.connection_mutex);
> +
> +	return 0;
> +}
> +
> +static int cabc_backlight_device_get_brightness(struct backlight_device *bd)
> +{
> +	struct intel_connector *connector = bl_get_data(bd);
> +	struct drm_device *dev = connector->base.dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	int ret;
> +
> +	intel_runtime_pm_get(dev_priv);
> +	drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
> +
> +	ret = cabc_get_backlight(connector);
> +
> +	drm_modeset_unlock(&dev->mode_config.connection_mutex);
> +	intel_runtime_pm_put(dev_priv);
> +
> +	return ret;
> +}
> +
> +static const struct backlight_ops cabc_backlight_device_ops = {
> +	.update_status = cabc_backlight_device_update_status,
> +	.get_brightness = cabc_backlight_device_get_brightness,
> +};
> +
> +int cabc_backlight_device_register(struct intel_connector *connector)
> +{
> +	struct intel_panel *panel = &connector->panel;
> +	struct backlight_properties props;
> +
> +	if (WARN_ON(panel->backlight.device))
> +		return -ENODEV;
> +
> +	if (!panel->backlight.present) {
> +		DRM_ERROR("No backlight present\n");
> +		return 0;
> +	}
> +
> +	WARN_ON(panel->backlight.max == 0);
> +
> +	memset(&props, 0, sizeof(props));
> +	props.type = BACKLIGHT_FIRMWARE;
> +	props.max_brightness = panel->backlight.max;
> +	props.brightness = props.max_brightness;
> +
> +	/*
> +	 * Note: using the same name independent of the connector prevents
> +	 * registration of multiple backlight devices in the driver.
> +	 */
> +	panel->backlight.device =
> +		backlight_device_register("intel_backlight",
> +				connector->base.kdev,
> +				connector,
> +				&cabc_backlight_device_ops, &props);
> +
> +	if (IS_ERR(panel->backlight.device)) {
> +		DRM_ERROR("Failed to register backlight: %ld\n",
> +				PTR_ERR(panel->backlight.device));
> +		panel->backlight.device = NULL;
> +		return -ENODEV;
> +	}
> +
> +	DRM_DEBUG_KMS("Connector %s backlight sysfs interface registered\n",
> +			connector->base.name);
> +
> +	return 0;
> +}
> +
> +void cabc_backlight_device_unregister(struct intel_connector *connector)
> +{
> +	struct intel_panel *panel = &connector->panel;
> +
> +	if (panel->backlight.device) {
> +		backlight_device_unregister(panel->backlight.device);
> +		panel->backlight.device = NULL;
> +	}
> +}
> diff --git a/drivers/gpu/drm/i915/intel_dsi_cabc.h b/drivers/gpu/drm/i915/intel_dsi_cabc.h
> new file mode 100644
> index 0000000..b1a79d9
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/intel_dsi_cabc.h
> @@ -0,0 +1,46 @@
> +/*
> + * Copyright © 2013 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> + * DEALINGS IN THE SOFTWARE.
> + *
> + * Author: Deepak M <m.deepak@intel.com>
> + */
> +
> +extern int cabc_backlight_device_register(struct intel_connector *connector);
> +extern void cabc_backlight_device_unregister(struct intel_connector *connector);
> +void cabc_set_backlight(struct intel_connector *connector, u32 level);
> +u32 cabc_get_backlight(struct intel_connector *connector);
> +void cabc_enable_backlight(struct intel_connector *connector);
> +void cabc_disable_backlight(struct intel_connector *connector);
> +int cabc_setup_backlight(struct drm_connector *connector, enum pipe unused);
> +
> +#define CABC_OFF			(0 << 0)
> +#define CABC_USER_INTERFACE_IMAGE	(1 << 0)
> +#define CABC_STILL_PICTURE		(2 << 0)
> +#define CABC_VIDEO_MODE			(3 << 0)
> +
> +#define CABC_BACKLIGHT			(1 << 2)
> +#define CABC_DIMMING_DISPLAY		(1 << 3)
> +#define CABC_BCTRL			(1 << 5)
> +
> +#define CABC_PORT_A			0x00
> +#define CABC_PORT_C			0x01
> +#define CABC_PORT_A_AND_C		0x02
> +#define CABC_MAX_VALUE			0xff
> 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,

These changes need to be a separate patch. And what's the spec
reference, I can't find these.

BR,
Jani.


>  };
>  
>  /* 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

-- 
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] drm/i915: CABC support for backlight control

[Deepak, M]

>-----Original Message-----
>From: Jani Nikula [mailto:jani.nikula@linux.intel.com]
>Sent: Monday, September 14, 2015 5:51 PM
>To: Deepak, M; intel-gfx@lists.freedesktop.org
>Cc: Deepak, M
>Subject: Re: [Intel-gfx] [PATCH] drm/i915: CABC support for backlight control
>
>On Mon, 14 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.
>
>I think CABC is a confusing name for this. At the high level, this should be
>called, say, DCS backlight control. I think CABC is a special case of backlight
>control in the DSI panel; there are panels with brightness control via DCS
>*without* CABC.
>
>See further comments inline.
>
>>
>> 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_dsi_cabc.c |  310
>+++++++++++++++++++++++++++++++++
>>  drivers/gpu/drm/i915/intel_dsi_cabc.h |   46 +++++
>>  drivers/gpu/drm/i915/intel_panel.c    |   23 ++-
>>  include/video/mipi_display.h          |    8 +
>>  6 files changed, 399 insertions(+), 7 deletions(-)  create mode
>> 100644 drivers/gpu/drm/i915/intel_dsi_cabc.c
>>  create mode 100644 drivers/gpu/drm/i915/intel_dsi_cabc.h
>>
>> 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);
>
>See my patch at [1] for how I think all of this should be handled.
>
>[1] http://mid.gmane.org/cover.1442227790.git.jani.nikula@intel.com
>
[Deepak M] Will go through that patch. 
>>  }
>>
>>  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_dsi_cabc.c
>> b/drivers/gpu/drm/i915/intel_dsi_cabc.c
>> new file mode 100644
>> index 0000000..c2fcd95
>> --- /dev/null
>> +++ b/drivers/gpu/drm/i915/intel_dsi_cabc.c
>> @@ -0,0 +1,310 @@
>> +/*
>> + * Copyright © 2006-2010 Intel Corporation
>> + *
>> + * Permission is hereby granted, free of charge, to any person
>> +obtaining a
>> + * copy of this software and associated documentation files (the
>> +"Software"),
>> + * to deal in the Software without restriction, including without
>> +limitation
>> + * the rights to use, copy, modify, merge, publish, distribute,
>> +sublicense,
>> + * and/or sell copies of the Software, and to permit persons to whom
>> +the
>> + * Software is furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice (including
>> +the next
>> + * paragraph) shall be included in all copies or substantial portions
>> +of the
>> + * Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY
>KIND,
>> +EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
>> +MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO
>EVENT
>> +SHALL
>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,
>DAMAGES
>> +OR OTHER
>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
>> +ARISING
>> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE
>OR
>> +OTHER
>> + * DEALINGS IN THE SOFTWARE.
>> + *
>> + * Author: Deepak M <m.deepak@intel.com>  */
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/moduleparam.h>
>> +#include "intel_drv.h"
>> +#include "intel_dsi.h"
>> +#include "i915_drv.h"
>> +#include "intel_dsi_cabc.h"
>> +#include <video/mipi_display.h>
>> +#include <drm/drm_mipi_dsi.h>
>> +
>> +static inline enum port get_cabc_port(struct intel_dsi *intel_dsi) {
>> +	if (intel_dsi->dl_cabc_port == CABC_PORT_C)
>> +		return PORT_C;
>> +	else
>> +		return PORT_A;
>> +}
>> +
>> +int cabc_setup_backlight(struct drm_connector *connector,
>> +		enum pipe unused)
>> +{
>> +	struct drm_device *dev = connector->dev;
>> +	struct drm_i915_private *dev_priv = dev->dev_private;
>> +	struct intel_connector *intel_connector =
>to_intel_connector(connector);
>> +	struct intel_panel *panel = &intel_connector->panel;
>> +
>> +	if (dev_priv->vbt.backlight.present)
>> +		panel->backlight.present = true;
>> +	else {
>> +		DRM_ERROR("no backlight present per VBT\n");
>> +		return 0;
>> +	}
>> +
>> +	panel->backlight.max = CABC_MAX_VALUE;
>> +	panel->backlight.level = CABC_MAX_VALUE;
>> +
>> +	return 0;
>> +}
>> +
>> +u32 cabc_get_backlight(struct intel_connector *connector) {
>> +	struct drm_device *dev = connector->base.dev;
>> +	struct intel_dsi *intel_dsi = NULL;
>> +	struct drm_crtc *crtc = NULL;
>> +	struct intel_encoder *encoder = NULL;
>> +	struct mipi_dsi_device *dsi_device;
>> +	u8 data[2] = {0};
>> +	enum port port;
>> +	u32 ret;
>> +
>> +	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
>> +		for_each_encoder_on_crtc(dev, crtc, encoder) {
>> +			if (encoder->type == INTEL_OUTPUT_DSI) {
>> +				intel_dsi = enc_to_intel_dsi(&encoder-
>>base);
>> +				break;
>> +			}
>> +		}
>> +		if (intel_dsi != NULL)
>> +			break;
>> +	}
>
>You can get at intel_dsi via connector->encoder. No need for this kind of
>loops.
>
[Deepak M] Okay
>Same for similar loops below.
>
>> +
>> +	for_each_dsi_port(port, intel_dsi->ports) {
>> +		if (intel_dsi->dual_link)
>> +			port = get_cabc_port(intel_dsi);
>> +
>> +		dsi_device = intel_dsi->dsi_hosts[port]->device;
>> +		mipi_dsi_dcs_read(dsi_device, MIPI_DCS_CABC_LEVEL_RD,
>data, 2);
>> +
>> +		if (intel_dsi->dual_link &&
>> +				!(intel_dsi->dl_cabc_port ==
>CABC_PORT_A_AND_C))
>> +			break;
>> +	}
>
>That is a really confusing way to do the dcs command on one or more ports.
>How about making dl_cabc_port (hopefully renamed to something referring
>to dcs rather than cabc) a bitmap of ports similar to intel_dsi->ports, so you
>can make this loop simply:
>
>	for_each_dsi_port(port, dcs_backlight_ports)
>        	...
>
>There'd be no need for special casing and conditional breaks out of the loop or
>anything.
>
>Same for similar loops below.
>
[Deepak M] Nice, will try to implement this way
>> +
>> +	ret = data[1];
>> +
>> +	return ret;
>> +}
>> +
>> +void cabc_set_backlight(struct intel_connector *connector, u32 level)
>> +{
>> +	struct drm_device *dev = connector->base.dev;
>> +	struct intel_dsi *intel_dsi = NULL;
>> +	struct drm_crtc *crtc = NULL;
>> +	struct intel_encoder *encoder = NULL;
>> +	struct mipi_dsi_device *dsi_device;
>> +	u8 data[2] = {0};
>> +	enum port port;
>> +
>> +	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
>> +		for_each_encoder_on_crtc(dev, crtc, encoder) {
>> +			if (encoder->type == INTEL_OUTPUT_DSI) {
>> +				intel_dsi = enc_to_intel_dsi(&encoder-
>>base);
>> +				break;
>> +			}
>> +		}
>> +		if (intel_dsi != NULL)
>> +			break;
>> +	}
>> +
>> +	for_each_dsi_port(port, intel_dsi->ports) {
>> +		if (intel_dsi->dual_link)
>> +			port = get_cabc_port(intel_dsi);
>> +
>> +		dsi_device = intel_dsi->dsi_hosts[port]->device;
>> +		data[1] = level;
>> +		data[0] = MIPI_DCS_CABC_LEVEL_WR;
>> +		mipi_dsi_dcs_write_buffer(dsi_device, data, 2);
>> +
>> +		if (intel_dsi->dual_link &&
>> +			!(intel_dsi->dl_cabc_port == CABC_PORT_A_AND_C))
>> +			break;
>> +	}
>> +}
>> +
>> +void cabc_enable_backlight(struct intel_connector *connector) {
>> +	struct drm_device *dev = connector->base.dev;
>> +	struct intel_dsi *intel_dsi = NULL;
>> +	struct drm_crtc *crtc = NULL;
>> +	struct intel_encoder *encoder = NULL;
>> +	struct intel_panel *panel = &connector->panel;
>> +	struct mipi_dsi_device *dsi_device;
>> +	enum port port;
>> +	u8 data[2] = {0};
>> +
>> +	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
>> +		for_each_encoder_on_crtc(dev, crtc, encoder) {
>> +			if (encoder->type == INTEL_OUTPUT_DSI) {
>> +				intel_dsi = enc_to_intel_dsi(&encoder-
>>base);
>> +				break;
>> +			}
>> +		}
>> +		if (intel_dsi != NULL)
>> +			break;
>> +	}
>> +
>> +	for_each_dsi_port(port, intel_dsi->ports) {
>> +		if (intel_dsi->dual_link)
>> +			port = get_cabc_port(intel_dsi);
>> +
>> +		dsi_device = intel_dsi->dsi_hosts[port]->device;
>> +		data[0] = MIPI_DCS_CABC_CONTROL_BRIGHT_WR;
>> +		data[1] = CABC_BACKLIGHT | CABC_DIMMING_DISPLAY |
>CABC_BCTRL;
>> +		mipi_dsi_dcs_write_buffer(dsi_device, data, 2);
>> +		data[0] = MIPI_DCS_CABC_CONTROL_WR;
>> +		data[1] = CABC_STILL_PICTURE;
>> +		mipi_dsi_dcs_write_buffer(dsi_device, data, 2);
>> +
>> +		if (intel_dsi->dual_link &&
>> +				!(intel_dsi->dl_cabc_port ==
>CABC_PORT_A_AND_C))
>> +			break;
>> +	}
>> +
>> +	cabc_set_backlight(connector, panel->backlight.level); }
>> +
>> +void cabc_disable_backlight(struct intel_connector *connector) {
>> +	struct drm_device *dev = connector->base.dev;
>> +	struct intel_dsi *intel_dsi = NULL;
>> +	struct drm_crtc *crtc = NULL;
>> +	struct intel_encoder *encoder = NULL;
>> +	struct mipi_dsi_device *dsi_device;
>> +	enum port port;
>> +	u8 data[2] = {0};
>> +
>> +	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
>> +		for_each_encoder_on_crtc(dev, crtc, encoder) {
>> +			if (encoder->type == INTEL_OUTPUT_DSI) {
>> +				intel_dsi = enc_to_intel_dsi(&encoder-
>>base);
>> +				break;
>> +			}
>> +		}
>> +		if (intel_dsi != NULL)
>> +			break;
>> +	}
>> +
>> +	cabc_set_backlight(connector, 0);
>> +
>> +	for_each_dsi_port(port, intel_dsi->ports) {
>> +		if (intel_dsi->dual_link)
>> +			port = get_cabc_port(intel_dsi);
>> +		dsi_device = intel_dsi->dsi_hosts[port]->device;
>> +
>> +		data[1] = CABC_OFF;
>> +		data[0] = MIPI_DCS_CABC_CONTROL_WR;
>> +		mipi_dsi_dcs_write_buffer(dsi_device, data, 2);
>> +		data[0] = MIPI_DCS_CABC_CONTROL_BRIGHT_WR;
>> +		mipi_dsi_dcs_write_buffer(dsi_device, data, 2);
>> +
>> +		if (intel_dsi->dual_link &&
>> +				!(intel_dsi->dl_cabc_port ==
>CABC_PORT_A_AND_C))
>> +			break;
>> +	}
>> +}
>> +static int cabc_backlight_device_update_status(struct
>> +backlight_device *bd) {
>> +	struct intel_connector *connector = bl_get_data(bd);
>> +	struct drm_device *dev = connector->base.dev;
>> +
>> +	drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
>> +	DRM_DEBUG_KMS("updating intel_backlight, brightness=%d/%d\n",
>> +			bd->props.brightness, bd->props.max_brightness);
>> +
>> +	cabc_set_backlight(connector, bd->props.brightness);
>> +
>> +	drm_modeset_unlock(&dev->mode_config.connection_mutex);
>> +
>> +	return 0;
>> +}
>> +
>> +static int cabc_backlight_device_get_brightness(struct
>> +backlight_device *bd) {
>> +	struct intel_connector *connector = bl_get_data(bd);
>> +	struct drm_device *dev = connector->base.dev;
>> +	struct drm_i915_private *dev_priv = dev->dev_private;
>> +	int ret;
>> +
>> +	intel_runtime_pm_get(dev_priv);
>> +	drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
>> +
>> +	ret = cabc_get_backlight(connector);
>> +
>> +	drm_modeset_unlock(&dev->mode_config.connection_mutex);
>> +	intel_runtime_pm_put(dev_priv);
>> +
>> +	return ret;
>> +}
>> +
>> +static const struct backlight_ops cabc_backlight_device_ops = {
>> +	.update_status = cabc_backlight_device_update_status,
>> +	.get_brightness = cabc_backlight_device_get_brightness,
>> +};
>> +
>> +int cabc_backlight_device_register(struct intel_connector *connector)
>> +{
>> +	struct intel_panel *panel = &connector->panel;
>> +	struct backlight_properties props;
>> +
>> +	if (WARN_ON(panel->backlight.device))
>> +		return -ENODEV;
>> +
>> +	if (!panel->backlight.present) {
>> +		DRM_ERROR("No backlight present\n");
>> +		return 0;
>> +	}
>> +
>> +	WARN_ON(panel->backlight.max == 0);
>> +
>> +	memset(&props, 0, sizeof(props));
>> +	props.type = BACKLIGHT_FIRMWARE;
>> +	props.max_brightness = panel->backlight.max;
>> +	props.brightness = props.max_brightness;
>> +
>> +	/*
>> +	 * Note: using the same name independent of the connector prevents
>> +	 * registration of multiple backlight devices in the driver.
>> +	 */
>> +	panel->backlight.device =
>> +		backlight_device_register("intel_backlight",
>> +				connector->base.kdev,
>> +				connector,
>> +				&cabc_backlight_device_ops, &props);
>> +
>> +	if (IS_ERR(panel->backlight.device)) {
>> +		DRM_ERROR("Failed to register backlight: %ld\n",
>> +				PTR_ERR(panel->backlight.device));
>> +		panel->backlight.device = NULL;
>> +		return -ENODEV;
>> +	}
>> +
>> +	DRM_DEBUG_KMS("Connector %s backlight sysfs interface
>registered\n",
>> +			connector->base.name);
>> +
>> +	return 0;
>> +}
>> +
>> +void cabc_backlight_device_unregister(struct intel_connector
>> +*connector) {
>> +	struct intel_panel *panel = &connector->panel;
>> +
>> +	if (panel->backlight.device) {
>> +		backlight_device_unregister(panel->backlight.device);
>> +		panel->backlight.device = NULL;
>> +	}
>> +}
>> diff --git a/drivers/gpu/drm/i915/intel_dsi_cabc.h
>> b/drivers/gpu/drm/i915/intel_dsi_cabc.h
>> new file mode 100644
>> index 0000000..b1a79d9
>> --- /dev/null
>> +++ b/drivers/gpu/drm/i915/intel_dsi_cabc.h
>> @@ -0,0 +1,46 @@
>> +/*
>> + * Copyright © 2013 Intel Corporation
>> + *
>> + * Permission is hereby granted, free of charge, to any person
>> +obtaining a
>> + * copy of this software and associated documentation files (the
>> +"Software"),
>> + * to deal in the Software without restriction, including without
>> +limitation
>> + * the rights to use, copy, modify, merge, publish, distribute,
>> +sublicense,
>> + * and/or sell copies of the Software, and to permit persons to whom
>> +the
>> + * Software is furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice (including
>> +the next
>> + * paragraph) shall be included in all copies or substantial portions
>> +of the
>> + * Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY
>KIND,
>> +EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
>> +MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO
>EVENT
>> +SHALL
>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,
>DAMAGES
>> +OR OTHER
>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
>> +ARISING
>> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE
>OR
>> +OTHER
>> + * DEALINGS IN THE SOFTWARE.
>> + *
>> + * Author: Deepak M <m.deepak@intel.com>  */
>> +
>> +extern int cabc_backlight_device_register(struct intel_connector
>> +*connector); extern void cabc_backlight_device_unregister(struct
>> +intel_connector *connector); void cabc_set_backlight(struct
>> +intel_connector *connector, u32 level);
>> +u32 cabc_get_backlight(struct intel_connector *connector); void
>> +cabc_enable_backlight(struct intel_connector *connector); void
>> +cabc_disable_backlight(struct intel_connector *connector); int
>> +cabc_setup_backlight(struct drm_connector *connector, enum pipe
>> +unused);
>> +
>> +#define CABC_OFF			(0 << 0)
>> +#define CABC_USER_INTERFACE_IMAGE	(1 << 0)
>> +#define CABC_STILL_PICTURE		(2 << 0)
>> +#define CABC_VIDEO_MODE			(3 << 0)
>> +
>> +#define CABC_BACKLIGHT			(1 << 2)
>> +#define CABC_DIMMING_DISPLAY		(1 << 3)
>> +#define CABC_BCTRL			(1 << 5)
>> +
>> +#define CABC_PORT_A			0x00
>> +#define CABC_PORT_C			0x01
>> +#define CABC_PORT_A_AND_C		0x02
>> +#define CABC_MAX_VALUE			0xff
>> 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,
>
>These changes need to be a separate patch. And what's the spec reference, I
>can't find these.
>
[Deepak M] The HLD can be found in the link https://securewiki.ith.intel.com/display/GfxDisplay/MIPI+Display (it is under Intel firewall)
>BR,
>Jani.
>
>
>>  };
>>
>>  /* 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
>
>--
>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] drm/i915: CABC support for backlight control

[Jani Nikula]

On Mon, 14 Sep 2015, "Deepak, M" <m.deepak@intel.com> wrote:
>>-----Original Message-----
>>From: Jani Nikula [mailto:jani.nikula@linux.intel.com]
>>Sent: Monday, September 14, 2015 5:51 PM
>>To: Deepak, M; intel-gfx@lists.freedesktop.org
>>Cc: Deepak, M
>>Subject: Re: [Intel-gfx] [PATCH] drm/i915: CABC support for backlight control
>>
>>On Mon, 14 Sep 2015, Deepak M <m.deepak@intel.com> wrote:
>>> 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,
>>
>>These changes need to be a separate patch. And what's the spec reference, I
>>can't find these.
>>
> [Deepak M] The HLD can be found in the link https://securewiki.ith.intel.com/display/GfxDisplay/MIPI+Display (it is under Intel firewall)

If you want to add stuff to include/video/mipi_display.h it has to be
standard DCS commands, in actual MIPI issued specs, not something
internal to us.

Manufacturer specific commands (usually referred to as MCS) need to be
hidden in manufacturer specific files in the driver.

BR,
Jani.


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