[Intel-gfx] [MIPI CABC 2/2] drm/i915: CABC support for backlight control
Jani Nikula
jani.nikula at intel.com
Wed Mar 23 11:50:14 UTC 2016
On Wed, 23 Mar 2016, "Deepak, M" <m.deepak at intel.com> wrote:
>> -----Original Message-----
>> From: Nikula, Jani
>> Sent: Tuesday, March 22, 2016 7:19 PM
>> To: Deepak, M <m.deepak at intel.com>; intel-gfx at lists.freedesktop.org
>> Cc: Deepak, M <m.deepak at intel.com>; Vetter, Daniel
>> <daniel.vetter at intel.com>; Adebisi, YetundeX
>> <yetundex.adebisi at intel.com>
>> Subject: Re: [MIPI CABC 2/2] drm/i915: CABC support for backlight control
>>
>> On Tue, 22 Mar 2016, Deepak M <m.deepak at 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: Moving the CABC bkl functions to new file.(Jani)
>> >
>> > v3: Rebase
>> >
>> > v4: Rebase
>> >
>> > Cc: Jani Nikula <jani.nikula at intel.com>
>> > Cc: Daniel Vetter <daniel.vetter at intel.com>
>> > Cc: Yetunde Adebisi <yetundex.adebisi at intel.com>
>> > Signed-off-by: Deepak M <m.deepak at intel.com>
>> > ---
>> > drivers/gpu/drm/i915/Makefile | 1 +
>> > drivers/gpu/drm/i915/i915_drv.h | 2 +-
>> > drivers/gpu/drm/i915/intel_dsi.c | 19 +++-
>> > drivers/gpu/drm/i915/intel_dsi.h | 4 +
>> > drivers/gpu/drm/i915/intel_dsi_cabc.c | 179
>> ++++++++++++++++++++++++++++++++++
>> > drivers/gpu/drm/i915/intel_panel.c | 4 +
>> > 6 files changed, 206 insertions(+), 3 deletions(-) create mode
>> > 100644 drivers/gpu/drm/i915/intel_dsi_cabc.c
>> >
>> > diff --git a/drivers/gpu/drm/i915/Makefile
>> > b/drivers/gpu/drm/i915/Makefile index 7ffb51b..065c410 100644
>> > --- a/drivers/gpu/drm/i915/Makefile
>> > +++ b/drivers/gpu/drm/i915/Makefile
>> > @@ -83,6 +83,7 @@ i915-y += dvo_ch7017.o \
>> > intel_dp_mst.o \
>> > intel_dp.o \
>> > intel_dsi.o \
>> > + intel_dsi_cabc.o \
>> > intel_dsi_panel_vbt.o \
>> > intel_dsi_pll.o \
>> > intel_dvo.o \
>> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
>> > b/drivers/gpu/drm/i915/i915_drv.h index 050d860..d196404 100644
>> > --- a/drivers/gpu/drm/i915/i915_drv.h
>> > +++ b/drivers/gpu/drm/i915/i915_drv.h
>> > @@ -3489,7 +3489,7 @@ void intel_sbi_write(struct drm_i915_private
>> *dev_priv, u16 reg, u32 value,
>> > enum intel_sbi_destination destination);
>> > u32 vlv_flisdsi_read(struct drm_i915_private *dev_priv, u32 reg);
>> > void vlv_flisdsi_write(struct drm_i915_private *dev_priv, u32 reg, u32
>> > val);
>> > -
>> > +int intel_dsi_cabc_init_backlight_funcs(struct intel_connector
>> > +*intel_connector);
>>
>> This probably fits better in intel_drv.h under a /* intel_dsi_cabc.c */
>> comment, see the file for examples.
>>
>> > int intel_gpu_freq(struct drm_i915_private *dev_priv, int val); int
>> > intel_freq_opcode(struct drm_i915_private *dev_priv, int val);
>> >
>> > diff --git a/drivers/gpu/drm/i915/intel_dsi.c
>> > b/drivers/gpu/drm/i915/intel_dsi.c
>> > index 456676c..7aa707f 100644
>> > --- a/drivers/gpu/drm/i915/intel_dsi.c
>> > +++ b/drivers/gpu/drm/i915/intel_dsi.c
>> > @@ -1209,10 +1209,25 @@ void intel_dsi_init(struct drm_device *dev)
>> > else
>> > intel_encoder->crtc_mask = BIT(PIPE_B);
>> >
>> > - if (dev_priv->vbt.dsi.config->dual_link)
>> > + if (dev_priv->vbt.dsi.config->dual_link) {
>> > intel_dsi->ports = BIT(PORT_A) | BIT(PORT_C);
>> > - else
>> > + switch (dev_priv->vbt.dsi.config->dl_cabc_port) {
>> > + case CABC_PORT_A:
>> > + intel_dsi->bkl_dcs_ports = BIT(PORT_A);
>> > + break;
>> > + case CABC_PORT_C:
>> > + intel_dsi->bkl_dcs_ports = BIT(PORT_C);
>> > + break;
>> > + case CABC_PORT_A_AND_C:
>> > + intel_dsi->bkl_dcs_ports = BIT(PORT_A) |
>> BIT(PORT_C);
>> > + break;
>> > + default:
>> > + DRM_ERROR("Unknown MIPI ports for sending
>> DCS\n");
>> > + }
>> > + } else {
>> > intel_dsi->ports = BIT(port);
>> > + intel_dsi->bkl_dcs_ports = BIT(port);
>> > + }
>> >
>> > /* Create a DSI host (and a device) for each port. */
>> > for_each_dsi_port(port, intel_dsi->ports) { diff --git
>> > a/drivers/gpu/drm/i915/intel_dsi.h b/drivers/gpu/drm/i915/intel_dsi.h
>> > index 0e758f1..5c07d59 100644
>> > --- a/drivers/gpu/drm/i915/intel_dsi.h
>> > +++ b/drivers/gpu/drm/i915/intel_dsi.h
>> > @@ -34,6 +34,10 @@
>> > #define DSI_DUAL_LINK_FRONT_BACK 1
>> > #define DSI_DUAL_LINK_PIXEL_ALT 2
>> >
>> > +#define CABC_PORT_A 0x00
>> > +#define CABC_PORT_C 0x01
>> > +#define CABC_PORT_A_AND_C 0x02
>> > +
>> > struct intel_dsi_host;
>> >
>> > struct intel_dsi {
>> > 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..d14a669
>> > --- /dev/null
>> > +++ b/drivers/gpu/drm/i915/intel_dsi_cabc.c
>> > @@ -0,0 +1,179 @@
>> > +/*
>> > + * Copyright © 2006-2010 Intel Corporation
>>
>> 2016
>>
>> > + *
>> > + * 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 at intel.com> */
>> > +
>> > +#include "intel_drv.h"
>> > +#include "intel_dsi.h"
>> > +#include "i915_drv.h"
>> > +#include <drm/drm_mipi_dsi.h>
>> > +
>> > +#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_MAX_VALUE 0xFF
>> > +
>>
>> The defines below should be added to the relevant enum in
>> include/video/mipi_display.h using the MIPI DCS specification names. I'll
>> copy them below. Please create a separate prep patch and send it to dri-
>> devel.
>>
>> > +#define MIPI_DCS_CABC_LEVEL_RD 0x52
>>
>> MIPI_DCS_GET_DISPLAY_BRIGHTNESS
>>
>> > +#define MIPI_DCS_CABC_MIN_BRIGHTNESS_RD 0x5F
>>
>> MIPI_DCS_GET_CABC_MIN_BRIGHTNESS
>>
>> > +#define MIPI_DCS_CABC_CONTROL_RD 0x56
>>
>> MIPI_DCS_GET_POWER_SAVE
>>
>> > +#define MIPI_DCS_CABC_CONTROL_BRIGHT_RD 0x54
>>
>> MIPI_DCS_GET_CONTROL_DISPLAY
>>
>> > +#define MIPI_DCS_CABC_LEVEL_WR 0x51
>>
>> MIPI_DCS_SET_DISPLAY_BRIGHTNESS
>>
>> > +#define MIPI_DCS_CABC_MIN_BRIGHTNESS_WR 0x5E
>>
>> MIPI_DCS_SET_CABC_MIN_BRIGHTNESS
>>
>> > +#define MIPI_DCS_CABC_CONTROL_WR 0x55
>>
>> MIPI_DCS_WRITE_POWER_SAVE
>>
>> > +#define MIPI_DCS_CABC_CONTROL_BRIGHT_WR 0x53
>>
>> MIPI_DCS_WRITE_CONTROL_DISPLAY
>>
>> > +
>> > +static u32 cabc_get_backlight(struct intel_connector *connector) {
>> struct intel_encoder *encoder = connector->encoder;
>> struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
>>
>> Same for all functions below.
>>
>> > + struct intel_dsi *intel_dsi = NULL;
>> > + struct intel_encoder *encoder = NULL;
>> > + struct mipi_dsi_device *dsi_device;
>> > + u8 data[2] = {0};
>> > + enum port port;
>> > +
>> > + encoder = connector->encoder;
>> > + intel_dsi = enc_to_intel_dsi(&encoder->base);
>> > +
>> > + for_each_dsi_port(port, intel_dsi->bkl_dcs_ports) {
>> > + dsi_device = intel_dsi->dsi_hosts[port]->device;
>> > + mipi_dsi_dcs_read(dsi_device, MIPI_DCS_CABC_LEVEL_RD,
>> data, 2);
>> > + }
>> > +
>> > + return data[1];
>>
>> Hmm, it's up to the manufacturer whether the this is 8 or 16 bits. I guess
>> you're just assuming 8 bits? I wonder if this should be more generic wrt 8 vs.
>> 16 bits.
>>
> [Deepak, M] Need to think on how to make it generic wrt 8 vs 16 bits,
> problem here is to somehow we have to identify the no of parameters or
> we have to pass this info in VBT...
Look at the maximum value to decide? That's what needs to be set in
setup if the size changes, so no need to add another variable.
BR,
Jani.
>> > +}
>> > +
>> > +static void cabc_set_backlight(struct intel_connector *connector, u32
>> > +level) {
>> > + struct intel_dsi *intel_dsi = NULL;
>> > + struct intel_encoder *encoder = NULL;
>> > + struct mipi_dsi_device *dsi_device;
>> > + u8 data[2] = {0};
>> > + enum port port;
>> > +
>> > + encoder = connector->encoder;
>> > + intel_dsi = enc_to_intel_dsi(&encoder->base);
>> > +
>> > + for_each_dsi_port(port, intel_dsi->bkl_dcs_ports) {
>> > + 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);
>>
>> Please use mipi_dsi_dcs_write(). Same for all functions below.
>>
>> > + }
>> > +}
>> > +
>> > +static void cabc_disable_backlight(struct intel_connector *connector)
>> > +{
>> > + struct intel_dsi *intel_dsi = NULL;
>> > + struct intel_encoder *encoder = NULL;
>> > + struct mipi_dsi_device *dsi_device;
>> > + enum port port;
>> > + u8 data[2] = {0};
>> > +
>> > + encoder = connector->encoder;
>> > + intel_dsi = enc_to_intel_dsi(&encoder->base);
>> > +
>> > + cabc_set_backlight(connector, 0);
>> > +
>> > + for_each_dsi_port(port, intel_dsi->bkl_dcs_ports) {
>> > + 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);
>> > + }
>> > +}
>> > +
>> > +static void cabc_enable_backlight(struct intel_connector *connector)
>> > +{
>> > + struct intel_dsi *intel_dsi = 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};
>> > +
>> > + encoder = connector->encoder;
>> > + intel_dsi = enc_to_intel_dsi(&encoder->base);
>> > +
>> > + for_each_dsi_port(port, intel_dsi->bkl_dcs_ports) {
>> > + 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);
>> > + }
>> > +
>> > + cabc_set_backlight(connector, panel->backlight.level); }
>> > +
>> > +static int cabc_setup_backlight(struct intel_connector *connector,
>> > + enum pipe unused)
>> > +{
>> > + struct drm_device *dev = connector->base.dev;
>> > + struct drm_i915_private *dev_priv = dev->dev_private;
>> > + struct intel_panel *panel = &connector->panel;
>> > +
>> > + if (dev_priv->vbt.backlight.present)
>> > + panel->backlight.present = true;
>> > + else {
>> > + DRM_ERROR("no backlight present per VBT\n");
>> > + return 0;
>> > + }
>>
>> None of the above is needed.
>>
>> dev_priv->vbt.backlight.present is checked higher level in
>> intel_panel_setup_backlight(), and panel->backlight.present = true is set
>> there as well if this hook returns 0.
>>
>> > +
>> > + panel->backlight.max = CABC_MAX_VALUE;
>> > + panel->backlight.level = CABC_MAX_VALUE;
>> > +
>> > + return 0;
>> > +}
>> > +
>> > +int intel_dsi_cabc_init_backlight_funcs(struct intel_connector
>> > +*intel_connector) {
>> > + struct drm_device *dev = intel_connector->base.dev;
>> > + struct drm_i915_private *dev_priv = dev->dev_private;
>> > + struct intel_encoder *encoder = intel_connector->encoder;
>> > + struct intel_panel *panel = &intel_connector->panel;
>> > +
>> > + if (!dev_priv->vbt.dsi.config->cabc_supported)
>> > + return -EINVAL;
>>
>> -ENODEV seems more appropriate.
>>
>> > +
>> > + if (encoder->type != INTEL_OUTPUT_DSI) {
>> > + DRM_ERROR("Use DSI encoder for CABC\n");
>> > + return -EINVAL;
>> > + }
>>
>> if (WARN_ON(encoder->type != INTEL_OUTPUT_DSI))
>> return -EINVAL;
>>
>> > +
>> > + panel->backlight.setup = cabc_setup_backlight;
>> > + panel->backlight.enable = cabc_enable_backlight;
>> > + panel->backlight.disable = cabc_disable_backlight;
>> > + panel->backlight.set = cabc_set_backlight;
>> > + panel->backlight.get = cabc_get_backlight;
>> > +
>> > + return 0;
>> > +}
>> > diff --git a/drivers/gpu/drm/i915/intel_panel.c
>> > b/drivers/gpu/drm/i915/intel_panel.c
>> > index 8c8996f..0f9bf80 100644
>> > --- a/drivers/gpu/drm/i915/intel_panel.c
>> > +++ b/drivers/gpu/drm/i915/intel_panel.c
>> > @@ -1718,6 +1718,10 @@ intel_panel_init_backlight_funcs(struct
>> intel_panel *panel)
>> > container_of(panel, struct intel_connector, panel);
>> > struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
>> >
>> > + if (connector->base.connector_type ==
>> DRM_MODE_CONNECTOR_DSI &&
>> > + intel_dsi_cabc_init_backlight_funcs(connector) == 0)
>>
>> Indentation is not quite right.
>>
>> > + return;
>> > +
>> > if (IS_BROXTON(dev_priv)) {
>> > panel->backlight.setup = bxt_setup_backlight;
>> > panel->backlight.enable = bxt_enable_backlight;
>>
>> --
>> Jani Nikula, Intel Open Source Technology Center
--
Jani Nikula, Intel Open Source Technology Center
More information about the Intel-gfx
mailing list