| Message ID | 1441946868-4985-3-git-send-email-m.deepak@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
>-----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
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
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
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
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
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 */
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(-)