| Message ID | 1380317977-1009-1-git-send-email-jbarnes@virtuousgeek.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, 28 Sep 2013, Jesse Barnes <jbarnes@virtuousgeek.org> wrote: > On VLV/BYT, backlight controls a per-pipe, so when adjusting the > backlight we need to pass the correct info. So make the externally > visible backlight functions take a connector argument, which can be used > internally to figure out the pipe backlight to adjust. > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> > --- > drivers/gpu/drm/i915/intel_display.c | 8 +++++ > drivers/gpu/drm/i915/intel_dp.c | 5 ++- > drivers/gpu/drm/i915/intel_drv.h | 8 +++-- > drivers/gpu/drm/i915/intel_lvds.c | 9 +++-- > drivers/gpu/drm/i915/intel_opregion.c | 28 ++++++++++++++- > drivers/gpu/drm/i915/intel_panel.c | 66 +++++++++++++++++++++-------------- > 6 files changed, 88 insertions(+), 36 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 9a7136c..d6a1868 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -9716,6 +9716,14 @@ static void intel_crtc_init(struct drm_device *dev, int pipe) > drm_crtc_helper_add(&intel_crtc->base, &intel_helper_funcs); > } > > +enum pipe intel_get_pipe_from_connector(struct intel_connector *connector) > +{ > + struct intel_encoder *encoder = connector->encoder; > + struct intel_crtc *crtc = to_intel_crtc(encoder->base.crtc); > + > + return crtc->pipe; What if crtc == NULL? I guess this shouldn't happen on the backlight enable/disable call paths through encoder callbacks, but what about backlight sysfs and opregion? > +} > + > int intel_get_pipe_from_crtc_id(struct drm_device *dev, void *data, > struct drm_file *file) > { > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 95a3159..ef69d31 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -1252,7 +1252,6 @@ void ironlake_edp_backlight_on(struct intel_dp *intel_dp) > struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); > struct drm_device *dev = intel_dig_port->base.base.dev; > struct drm_i915_private *dev_priv = dev->dev_private; > - int pipe = to_intel_crtc(intel_dig_port->base.base.crtc)->pipe; > u32 pp; > u32 pp_ctrl_reg; > > @@ -1275,7 +1274,7 @@ void ironlake_edp_backlight_on(struct intel_dp *intel_dp) > I915_WRITE(pp_ctrl_reg, pp); > POSTING_READ(pp_ctrl_reg); > > - intel_panel_enable_backlight(dev, pipe); > + intel_panel_enable_backlight(intel_dp->attached_connector); > } > > void ironlake_edp_backlight_off(struct intel_dp *intel_dp) > @@ -1288,7 +1287,7 @@ void ironlake_edp_backlight_off(struct intel_dp *intel_dp) > if (!is_edp(intel_dp)) > return; > > - intel_panel_disable_backlight(dev); > + intel_panel_disable_backlight(intel_dp->attached_connector); > > DRM_DEBUG_KMS("\n"); > pp = ironlake_get_pp_control(intel_dp); > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index a17a86a..e9b7540 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -619,6 +619,7 @@ void intel_connector_attach_encoder(struct intel_connector *connector, > struct drm_encoder *intel_best_encoder(struct drm_connector *connector); > struct drm_display_mode *intel_crtc_mode_get(struct drm_device *dev, > struct drm_crtc *crtc); > +enum pipe intel_get_pipe_from_connector(struct intel_connector *connector); > int intel_get_pipe_from_crtc_id(struct drm_device *dev, void *data, > struct drm_file *file_priv); > enum transcoder intel_pipe_to_cpu_transcoder(struct drm_i915_private *dev_priv, > @@ -767,10 +768,11 @@ void intel_pch_panel_fitting(struct intel_crtc *crtc, > void intel_gmch_panel_fitting(struct intel_crtc *crtc, > struct intel_crtc_config *pipe_config, > int fitting_mode); > -void intel_panel_set_backlight(struct drm_device *dev, u32 level, u32 max); > +void intel_panel_set_backlight(struct intel_connector *connector, u32 level, > + u32 max); > int intel_panel_setup_backlight(struct drm_connector *connector); > -void intel_panel_enable_backlight(struct drm_device *dev, enum pipe pipe); > -void intel_panel_disable_backlight(struct drm_device *dev); > +void intel_panel_enable_backlight(struct intel_connector *connector); > +void intel_panel_disable_backlight(struct intel_connector *connector); > void intel_panel_destroy_backlight(struct drm_device *dev); > enum drm_connector_status intel_panel_detect(struct drm_device *dev); > > diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c > index fb3f8ef..f3069ae2 100644 > --- a/drivers/gpu/drm/i915/intel_lvds.c > +++ b/drivers/gpu/drm/i915/intel_lvds.c > @@ -206,7 +206,8 @@ static void intel_enable_lvds(struct intel_encoder *encoder) > { > struct drm_device *dev = encoder->base.dev; > struct intel_lvds_encoder *lvds_encoder = to_lvds_encoder(&encoder->base); > - struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc); > + struct intel_connector *intel_connector = > + &lvds_encoder->attached_connector->base; > struct drm_i915_private *dev_priv = dev->dev_private; > u32 ctl_reg, stat_reg; > > @@ -225,13 +226,15 @@ static void intel_enable_lvds(struct intel_encoder *encoder) > if (wait_for((I915_READ(stat_reg) & PP_ON) != 0, 1000)) > DRM_ERROR("timed out waiting for panel to power on\n"); > > - intel_panel_enable_backlight(dev, intel_crtc->pipe); > + intel_panel_enable_backlight(intel_connector); > } > > static void intel_disable_lvds(struct intel_encoder *encoder) > { > struct drm_device *dev = encoder->base.dev; > struct intel_lvds_encoder *lvds_encoder = to_lvds_encoder(&encoder->base); > + struct intel_connector *intel_connector = > + &lvds_encoder->attached_connector->base; > struct drm_i915_private *dev_priv = dev->dev_private; > u32 ctl_reg, stat_reg; > > @@ -243,7 +246,7 @@ static void intel_disable_lvds(struct intel_encoder *encoder) > stat_reg = PP_STATUS; > } > > - intel_panel_disable_backlight(dev); > + intel_panel_disable_backlight(intel_connector); > > I915_WRITE(ctl_reg, I915_READ(ctl_reg) & ~POWER_TARGET_ON); > if (wait_for((I915_READ(stat_reg) & PP_ON) == 0, 1000)) > diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c > index 2acf5ca..2404e1c 100644 > --- a/drivers/gpu/drm/i915/intel_opregion.c > +++ b/drivers/gpu/drm/i915/intel_opregion.c > @@ -395,6 +395,11 @@ static u32 asle_set_backlight(struct drm_device *dev, u32 bclp) > { > struct drm_i915_private *dev_priv = dev->dev_private; > struct opregion_asle __iomem *asle = dev_priv->opregion.asle; > + struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[0]; > + struct drm_encoder *encoder; > + struct drm_connector *connector; > + struct intel_connector *intel_connector = NULL; > + bool found = false; > > DRM_DEBUG_DRIVER("bclp = 0x%08x\n", bclp); > > @@ -405,7 +410,28 @@ static u32 asle_set_backlight(struct drm_device *dev, u32 bclp) > if (bclp > 255) > return ASLC_BACKLIGHT_FAILED; > > - intel_panel_set_backlight(dev, bclp, 255); > + /* > + * Could match the OpRegion connector here instead, but we'd > + * also need to verify the connector could handle a backlight > + * call. > + */ Connector callbacks for backlight is the future? > + list_for_each_entry(encoder, &dev->mode_config.encoder_list, head) > + if (encoder->crtc == crtc) { > + found = true; > + break; > + } > + > + if (!found) > + return ASLC_BACKLIGHT_FAILED; > + > + list_for_each_entry(connector, &dev->mode_config.connector_list, head) > + if (connector->encoder == encoder) > + intel_connector = to_intel_connector(connector); > + > + if (!found) > + return ASLC_BACKLIGHT_FAILED; ITYM if (intel_connector == NULL). It aches a little to realize you pick a fixed pipe here, go through all the trouble of finding the corresponding connector, only to have intel_panel_set_backlight() dig out the pipe from the connector again... An alternative (not necessarily better, just different) approach might be changing backlight on all enabled pipes on requests coming from opregion or sysfs (and maybe disabling backlight on disabled pipes). This would let the backlight be adjusted via sysfs/opregion also on pipe B, which doesn't happen with the proposed patch. > + > + intel_panel_set_backlight(intel_connector, bclp, 255); > iowrite32(DIV_ROUND_UP(bclp * 100, 255) | ASLE_CBLV_VALID, &asle->cblv); > > return 0; > diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c > index 5468416..5122f58 100644 > --- a/drivers/gpu/drm/i915/intel_panel.c > +++ b/drivers/gpu/drm/i915/intel_panel.c > @@ -341,7 +341,7 @@ static int is_backlight_combination_mode(struct drm_device *dev) > /* XXX: query mode clock or hardware clock and program max PWM appropriately > * when it's 0. > */ > -static u32 i915_read_blc_pwm_ctl(struct drm_device *dev) > +static u32 i915_read_blc_pwm_ctl(struct drm_device *dev, enum pipe pipe) > { > struct drm_i915_private *dev_priv = dev->dev_private; > u32 val; > @@ -380,11 +380,12 @@ static u32 i915_read_blc_pwm_ctl(struct drm_device *dev) > return val; > } > > -static u32 intel_panel_get_max_backlight(struct drm_device *dev) > +static u32 intel_panel_get_max_backlight(struct drm_device *dev, > + enum pipe pipe) > { > u32 max; > > - max = i915_read_blc_pwm_ctl(dev); > + max = i915_read_blc_pwm_ctl(dev, pipe); Unrelated to this patch: This reminds me again we should probably just pick an arbitrary backlight range we expose to the userspace, and scale right before we write the value to the duty cycle. There's a (somewhat theoretical) chance the two backlight PWMs happen to have different max values, and the max we expose shouldn't change depending on the pipe. Also we can't support changing the PWM frequency if we expose that as the backlight device max value. BR, Jani. > if (HAS_PCH_SPLIT(dev)) { > max >>= 16; > @@ -410,7 +411,8 @@ MODULE_PARM_DESC(invert_brightness, "Invert backlight brightness " > "to dri-devel@lists.freedesktop.org, if your machine needs it. " > "It will then be included in an upcoming module version."); > module_param_named(invert_brightness, i915_panel_invert_brightness, int, 0600); > -static u32 intel_panel_compute_brightness(struct drm_device *dev, u32 val) > +static u32 intel_panel_compute_brightness(struct drm_device *dev, > + enum pipe pipe, u32 val) > { > struct drm_i915_private *dev_priv = dev->dev_private; > > @@ -419,7 +421,7 @@ static u32 intel_panel_compute_brightness(struct drm_device *dev, u32 val) > > if (i915_panel_invert_brightness > 0 || > dev_priv->quirks & QUIRK_INVERT_BRIGHTNESS) { > - u32 max = intel_panel_get_max_backlight(dev); > + u32 max = intel_panel_get_max_backlight(dev, pipe); > if (max) > return max - val; > } > @@ -427,7 +429,8 @@ static u32 intel_panel_compute_brightness(struct drm_device *dev, u32 val) > return val; > } > > -static u32 intel_panel_get_backlight(struct drm_device *dev) > +static u32 intel_panel_get_backlight(struct drm_device *dev, > + enum pipe pipe) > { > struct drm_i915_private *dev_priv = dev->dev_private; > u32 val; > @@ -450,7 +453,7 @@ static u32 intel_panel_get_backlight(struct drm_device *dev) > } > } > > - val = intel_panel_compute_brightness(dev, val); > + val = intel_panel_compute_brightness(dev, pipe, val); > > spin_unlock_irqrestore(&dev_priv->backlight.lock, flags); > > @@ -466,19 +469,19 @@ static void intel_pch_panel_set_backlight(struct drm_device *dev, u32 level) > } > > static void intel_panel_actually_set_backlight(struct drm_device *dev, > - u32 level) > + enum pipe pipe, u32 level) > { > struct drm_i915_private *dev_priv = dev->dev_private; > u32 tmp; > > DRM_DEBUG_DRIVER("set backlight PWM = %d\n", level); > - level = intel_panel_compute_brightness(dev, level); > + level = intel_panel_compute_brightness(dev, pipe, level); > > if (HAS_PCH_SPLIT(dev)) > return intel_pch_panel_set_backlight(dev, level); > > if (is_backlight_combination_mode(dev)) { > - u32 max = intel_panel_get_max_backlight(dev); > + u32 max = intel_panel_get_max_backlight(dev, pipe); > u8 lbpc; > > /* we're screwed, but keep behaviour backwards compatible */ > @@ -498,15 +501,18 @@ static void intel_panel_actually_set_backlight(struct drm_device *dev, > } > > /* set backlight brightness to level in range [0..max] */ > -void intel_panel_set_backlight(struct drm_device *dev, u32 level, u32 max) > +void intel_panel_set_backlight(struct intel_connector *connector, u32 level, > + u32 max) > { > + struct drm_device *dev = connector->base.dev; > struct drm_i915_private *dev_priv = dev->dev_private; > + enum pipe pipe = intel_get_pipe_from_connector(connector); > u32 freq; > unsigned long flags; > > spin_lock_irqsave(&dev_priv->backlight.lock, flags); > > - freq = intel_panel_get_max_backlight(dev); > + freq = intel_panel_get_max_backlight(dev, pipe); > if (!freq) { > /* we are screwed, bail out */ > goto out; > @@ -523,14 +529,16 @@ void intel_panel_set_backlight(struct drm_device *dev, u32 level, u32 max) > dev_priv->backlight.device->props.brightness = level; > > if (dev_priv->backlight.enabled) > - intel_panel_actually_set_backlight(dev, level); > + intel_panel_actually_set_backlight(dev, pipe, level); > out: > spin_unlock_irqrestore(&dev_priv->backlight.lock, flags); > } > > -void intel_panel_disable_backlight(struct drm_device *dev) > +void intel_panel_disable_backlight(struct intel_connector *connector) > { > + struct drm_device *dev = connector->base.dev; > struct drm_i915_private *dev_priv = dev->dev_private; > + enum pipe pipe = intel_get_pipe_from_connector(connector); > unsigned long flags; > > /* > @@ -547,7 +555,7 @@ void intel_panel_disable_backlight(struct drm_device *dev) > spin_lock_irqsave(&dev_priv->backlight.lock, flags); > > dev_priv->backlight.enabled = false; > - intel_panel_actually_set_backlight(dev, 0); > + intel_panel_actually_set_backlight(dev, pipe, 0); > > if (INTEL_INFO(dev)->gen >= 4) { > uint32_t reg, tmp; > @@ -566,10 +574,11 @@ void intel_panel_disable_backlight(struct drm_device *dev) > spin_unlock_irqrestore(&dev_priv->backlight.lock, flags); > } > > -void intel_panel_enable_backlight(struct drm_device *dev, > - enum pipe pipe) > +void intel_panel_enable_backlight(struct intel_connector *connector) > { > + struct drm_device *dev = connector->base.dev; > struct drm_i915_private *dev_priv = dev->dev_private; > + enum pipe pipe = intel_get_pipe_from_connector(connector); > enum transcoder cpu_transcoder = > intel_pipe_to_cpu_transcoder(dev_priv, pipe); > unsigned long flags; > @@ -577,7 +586,8 @@ void intel_panel_enable_backlight(struct drm_device *dev, > spin_lock_irqsave(&dev_priv->backlight.lock, flags); > > if (dev_priv->backlight.level == 0) { > - dev_priv->backlight.level = intel_panel_get_max_backlight(dev); > + dev_priv->backlight.level = intel_panel_get_max_backlight(dev, > + pipe); > if (dev_priv->backlight.device) > dev_priv->backlight.device->props.brightness = > dev_priv->backlight.level; > @@ -627,7 +637,8 @@ set_level: > * registers are set. > */ > dev_priv->backlight.enabled = true; > - intel_panel_actually_set_backlight(dev, dev_priv->backlight.level); > + intel_panel_actually_set_backlight(dev, pipe, > + dev_priv->backlight.level); > > spin_unlock_irqrestore(&dev_priv->backlight.lock, flags); > } > @@ -650,7 +661,7 @@ static void intel_panel_init_backlight(struct drm_device *dev) > > intel_panel_init_backlight_regs(dev); > > - dev_priv->backlight.level = intel_panel_get_backlight(dev); > + dev_priv->backlight.level = intel_panel_get_backlight(dev, 0); > dev_priv->backlight.enabled = dev_priv->backlight.level != 0; > } > > @@ -679,16 +690,17 @@ intel_panel_detect(struct drm_device *dev) > #if IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE) > static int intel_panel_update_status(struct backlight_device *bd) > { > - struct drm_device *dev = bl_get_data(bd); > - intel_panel_set_backlight(dev, bd->props.brightness, > + struct intel_connector *connector = bl_get_data(bd); > + intel_panel_set_backlight(connector, bd->props.brightness, > bd->props.max_brightness); > return 0; > } > > static int intel_panel_get_brightness(struct backlight_device *bd) > { > - struct drm_device *dev = bl_get_data(bd); > - return intel_panel_get_backlight(dev); > + struct intel_connector *connector = bl_get_data(bd); > + enum pipe pipe = intel_get_pipe_from_connector(connector); > + return intel_panel_get_backlight(connector->base.dev, pipe); > } > > static const struct backlight_ops intel_panel_bl_ops = { > @@ -708,12 +720,13 @@ int intel_panel_setup_backlight(struct drm_connector *connector) > if (WARN_ON(dev_priv->backlight.device)) > return -ENODEV; > > + /* Just create a device for pipe 0 */ > memset(&props, 0, sizeof(props)); > props.type = BACKLIGHT_RAW; > props.brightness = dev_priv->backlight.level; > > spin_lock_irqsave(&dev_priv->backlight.lock, flags); > - props.max_brightness = intel_panel_get_max_backlight(dev); > + props.max_brightness = intel_panel_get_max_backlight(dev, 0); > spin_unlock_irqrestore(&dev_priv->backlight.lock, flags); > > if (props.max_brightness == 0) { > @@ -722,7 +735,8 @@ int intel_panel_setup_backlight(struct drm_connector *connector) > } > dev_priv->backlight.device = > backlight_device_register("intel_backlight", > - &connector->kdev, dev, > + &connector->kdev, > + to_intel_connector(connector), > &intel_panel_bl_ops, &props); > > if (IS_ERR(dev_priv->backlight.device)) { > -- > 1.8.3.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Mon, Sep 30, 2013 at 11:18:28AM +0300, Jani Nikula wrote: > On Sat, 28 Sep 2013, Jesse Barnes <jbarnes@virtuousgeek.org> wrote: > > On VLV/BYT, backlight controls a per-pipe, so when adjusting the > > backlight we need to pass the correct info. So make the externally > > visible backlight functions take a connector argument, which can be used > > internally to figure out the pipe backlight to adjust. > > > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> > > --- > > drivers/gpu/drm/i915/intel_display.c | 8 +++++ > > drivers/gpu/drm/i915/intel_dp.c | 5 ++- > > drivers/gpu/drm/i915/intel_drv.h | 8 +++-- > > drivers/gpu/drm/i915/intel_lvds.c | 9 +++-- > > drivers/gpu/drm/i915/intel_opregion.c | 28 ++++++++++++++- > > drivers/gpu/drm/i915/intel_panel.c | 66 +++++++++++++++++++++-------------- > > 6 files changed, 88 insertions(+), 36 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > index 9a7136c..d6a1868 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -9716,6 +9716,14 @@ static void intel_crtc_init(struct drm_device *dev, int pipe) > > drm_crtc_helper_add(&intel_crtc->base, &intel_helper_funcs); > > } > > > > +enum pipe intel_get_pipe_from_connector(struct intel_connector *connector) > > +{ > > + struct intel_encoder *encoder = connector->encoder; > > + struct intel_crtc *crtc = to_intel_crtc(encoder->base.crtc); > > + > > + return crtc->pipe; > > What if crtc == NULL? > > I guess this shouldn't happen on the backlight enable/disable call paths > through encoder callbacks, but what about backlight sysfs and opregion? I was discussing things a bit with Jani, and I'll repeat my thoughts here for others to see. Feel free to kick me if I get things totally wrong as I don't know the backlight code, nor do I really want to. We may need to handle backlight configuration requests when there's no pipe attached to the port, and we also want to be able to change the pipe<->port mapping, so my idea would be to shadow the backlight registers in the connector, and when we assign a pipe to the port, we write all the regs from the shadow copies to the real pipe blc registers. Otherwise I'm worried about all the rmw stuff we do to the registers, and leaving bits around in the registers from when the pipe was still used with some other port.
On Mon, Sep 30, 2013 at 2:15 PM, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > I was discussing things a bit with Jani, and I'll repeat my thoughts > here for others to see. Feel free to kick me if I get things totally > wrong as I don't know the backlight code, nor do I really want to. > > We may need to handle backlight configuration requests when there's no > pipe attached to the port, and we also want to be able to change the > pipe<->port mapping, so my idea would be to shadow the backlight > registers in the connector, and when we assign a pipe to the port, > we write all the regs from the shadow copies to the real pipe blc > registers. Otherwise I'm worried about all the rmw stuff we do to the > registers, and leaving bits around in the registers from when the pipe > was still used with some other port. I think that's the sanest approach (maybe keep the shadow values around in a backligh struct or intel_panel). Then we could also drop our reliance on the save/restore logic in i915_suspend.c, which will kill the 2nd copy of the fragile backlight register read/write code. Finally if we initialize the shadow values at module load time we can also ditch the imo fragile delayed register saving logic we currently have. -Daniel
On Mon, 30 Sep 2013 11:18:28 +0300 Jani Nikula <jani.nikula@linux.intel.com> wrote: > On Sat, 28 Sep 2013, Jesse Barnes <jbarnes@virtuousgeek.org> wrote: > > On VLV/BYT, backlight controls a per-pipe, so when adjusting the > > backlight we need to pass the correct info. So make the externally > > visible backlight functions take a connector argument, which can be used > > internally to figure out the pipe backlight to adjust. > > > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> > > --- > > drivers/gpu/drm/i915/intel_display.c | 8 +++++ > > drivers/gpu/drm/i915/intel_dp.c | 5 ++- > > drivers/gpu/drm/i915/intel_drv.h | 8 +++-- > > drivers/gpu/drm/i915/intel_lvds.c | 9 +++-- > > drivers/gpu/drm/i915/intel_opregion.c | 28 ++++++++++++++- > > drivers/gpu/drm/i915/intel_panel.c | 66 +++++++++++++++++++++-------------- > > 6 files changed, 88 insertions(+), 36 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > index 9a7136c..d6a1868 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -9716,6 +9716,14 @@ static void intel_crtc_init(struct drm_device *dev, int pipe) > > drm_crtc_helper_add(&intel_crtc->base, &intel_helper_funcs); > > } > > > > +enum pipe intel_get_pipe_from_connector(struct intel_connector *connector) > > +{ > > + struct intel_encoder *encoder = connector->encoder; > > + struct intel_crtc *crtc = to_intel_crtc(encoder->base.crtc); > > + > > + return crtc->pipe; > > What if crtc == NULL? > > I guess this shouldn't happen on the backlight enable/disable call paths > through encoder callbacks, but what about backlight sysfs and opregion? Yeah, it's definitely worth checking on a generic function like this. I suppose we should return a negative value in that case and make sure the callers check for it. > > + /* > > + * Could match the OpRegion connector here instead, but we'd > > + * also need to verify the connector could handle a backlight > > + * call. > > + */ > > Connector callbacks for backlight is the future? Yeah we definitely need to deal with this case better somehow... > > > + list_for_each_entry(encoder, &dev->mode_config.encoder_list, head) > > + if (encoder->crtc == crtc) { > > + found = true; > > + break; > > + } > > + > > + if (!found) > > + return ASLC_BACKLIGHT_FAILED; > > + > > + list_for_each_entry(connector, &dev->mode_config.connector_list, head) > > + if (connector->encoder == encoder) > > + intel_connector = to_intel_connector(connector); > > + > > + if (!found) > > + return ASLC_BACKLIGHT_FAILED; > > ITYM if (intel_connector == NULL). Oops yeah, cult & paste fail. > It aches a little to realize you pick a fixed pipe here, go through all > the trouble of finding the corresponding connector, only to have > intel_panel_set_backlight() dig out the pipe from the connector again... > > An alternative (not necessarily better, just different) approach might > be changing backlight on all enabled pipes on requests coming from > opregion or sysfs (and maybe disabling backlight on disabled > pipes). This would let the backlight be adjusted via sysfs/opregion also > on pipe B, which doesn't happen with the proposed patch. Yeah that might be better; I don't know how real hw will be wired up, but on the prototype board I have that would work fine (and should continue to work until someone builds a device with two panels and want independent backlight control). > > u32 max; > > > > - max = i915_read_blc_pwm_ctl(dev); > > + max = i915_read_blc_pwm_ctl(dev, pipe); > > Unrelated to this patch: > > This reminds me again we should probably just pick an arbitrary > backlight range we expose to the userspace, and scale right before we > write the value to the duty cycle. There's a (somewhat theoretical) > chance the two backlight PWMs happen to have different max values, and > the max we expose shouldn't change depending on the pipe. Also we can't > support changing the PWM frequency if we expose that as the backlight > device max value. Yeah that's a good point. Yet another thing to clean up in the backlight code. Jesse
On Mon, 30 Sep 2013 15:15:12 +0300 Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > On Mon, Sep 30, 2013 at 11:18:28AM +0300, Jani Nikula wrote: > > On Sat, 28 Sep 2013, Jesse Barnes <jbarnes@virtuousgeek.org> wrote: > > > On VLV/BYT, backlight controls a per-pipe, so when adjusting the > > > backlight we need to pass the correct info. So make the externally > > > visible backlight functions take a connector argument, which can be used > > > internally to figure out the pipe backlight to adjust. > > > > > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> > > > --- > > > drivers/gpu/drm/i915/intel_display.c | 8 +++++ > > > drivers/gpu/drm/i915/intel_dp.c | 5 ++- > > > drivers/gpu/drm/i915/intel_drv.h | 8 +++-- > > > drivers/gpu/drm/i915/intel_lvds.c | 9 +++-- > > > drivers/gpu/drm/i915/intel_opregion.c | 28 ++++++++++++++- > > > drivers/gpu/drm/i915/intel_panel.c | 66 +++++++++++++++++++++-------------- > > > 6 files changed, 88 insertions(+), 36 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > > index 9a7136c..d6a1868 100644 > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > @@ -9716,6 +9716,14 @@ static void intel_crtc_init(struct drm_device *dev, int pipe) > > > drm_crtc_helper_add(&intel_crtc->base, &intel_helper_funcs); > > > } > > > > > > +enum pipe intel_get_pipe_from_connector(struct intel_connector *connector) > > > +{ > > > + struct intel_encoder *encoder = connector->encoder; > > > + struct intel_crtc *crtc = to_intel_crtc(encoder->base.crtc); > > > + > > > + return crtc->pipe; > > > > What if crtc == NULL? > > > > I guess this shouldn't happen on the backlight enable/disable call paths > > through encoder callbacks, but what about backlight sysfs and opregion? > > I was discussing things a bit with Jani, and I'll repeat my thoughts > here for others to see. Feel free to kick me if I get things totally > wrong as I don't know the backlight code, nor do I really want to. > > We may need to handle backlight configuration requests when there's no > pipe attached to the port, and we also want to be able to change the > pipe<->port mapping, so my idea would be to shadow the backlight > registers in the connector, and when we assign a pipe to the port, > we write all the regs from the shadow copies to the real pipe blc > registers. Otherwise I'm worried about all the rmw stuff we do to the > registers, and leaving bits around in the registers from when the pipe > was still used with some other port. Yeah it's definitely a gap with this hw design. Tracking those bits in the connector makes more sense than having the backlight level change if the panel gets moved between pipes. Jesse
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 9a7136c..d6a1868 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -9716,6 +9716,14 @@ static void intel_crtc_init(struct drm_device *dev, int pipe) drm_crtc_helper_add(&intel_crtc->base, &intel_helper_funcs); } +enum pipe intel_get_pipe_from_connector(struct intel_connector *connector) +{ + struct intel_encoder *encoder = connector->encoder; + struct intel_crtc *crtc = to_intel_crtc(encoder->base.crtc); + + return crtc->pipe; +} + int intel_get_pipe_from_crtc_id(struct drm_device *dev, void *data, struct drm_file *file) { diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 95a3159..ef69d31 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -1252,7 +1252,6 @@ void ironlake_edp_backlight_on(struct intel_dp *intel_dp) struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); struct drm_device *dev = intel_dig_port->base.base.dev; struct drm_i915_private *dev_priv = dev->dev_private; - int pipe = to_intel_crtc(intel_dig_port->base.base.crtc)->pipe; u32 pp; u32 pp_ctrl_reg; @@ -1275,7 +1274,7 @@ void ironlake_edp_backlight_on(struct intel_dp *intel_dp) I915_WRITE(pp_ctrl_reg, pp); POSTING_READ(pp_ctrl_reg); - intel_panel_enable_backlight(dev, pipe); + intel_panel_enable_backlight(intel_dp->attached_connector); } void ironlake_edp_backlight_off(struct intel_dp *intel_dp) @@ -1288,7 +1287,7 @@ void ironlake_edp_backlight_off(struct intel_dp *intel_dp) if (!is_edp(intel_dp)) return; - intel_panel_disable_backlight(dev); + intel_panel_disable_backlight(intel_dp->attached_connector); DRM_DEBUG_KMS("\n"); pp = ironlake_get_pp_control(intel_dp); diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index a17a86a..e9b7540 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -619,6 +619,7 @@ void intel_connector_attach_encoder(struct intel_connector *connector, struct drm_encoder *intel_best_encoder(struct drm_connector *connector); struct drm_display_mode *intel_crtc_mode_get(struct drm_device *dev, struct drm_crtc *crtc); +enum pipe intel_get_pipe_from_connector(struct intel_connector *connector); int intel_get_pipe_from_crtc_id(struct drm_device *dev, void *data, struct drm_file *file_priv); enum transcoder intel_pipe_to_cpu_transcoder(struct drm_i915_private *dev_priv, @@ -767,10 +768,11 @@ void intel_pch_panel_fitting(struct intel_crtc *crtc, void intel_gmch_panel_fitting(struct intel_crtc *crtc, struct intel_crtc_config *pipe_config, int fitting_mode); -void intel_panel_set_backlight(struct drm_device *dev, u32 level, u32 max); +void intel_panel_set_backlight(struct intel_connector *connector, u32 level, + u32 max); int intel_panel_setup_backlight(struct drm_connector *connector); -void intel_panel_enable_backlight(struct drm_device *dev, enum pipe pipe); -void intel_panel_disable_backlight(struct drm_device *dev); +void intel_panel_enable_backlight(struct intel_connector *connector); +void intel_panel_disable_backlight(struct intel_connector *connector); void intel_panel_destroy_backlight(struct drm_device *dev); enum drm_connector_status intel_panel_detect(struct drm_device *dev); diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c index fb3f8ef..f3069ae2 100644 --- a/drivers/gpu/drm/i915/intel_lvds.c +++ b/drivers/gpu/drm/i915/intel_lvds.c @@ -206,7 +206,8 @@ static void intel_enable_lvds(struct intel_encoder *encoder) { struct drm_device *dev = encoder->base.dev; struct intel_lvds_encoder *lvds_encoder = to_lvds_encoder(&encoder->base); - struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc); + struct intel_connector *intel_connector = + &lvds_encoder->attached_connector->base; struct drm_i915_private *dev_priv = dev->dev_private; u32 ctl_reg, stat_reg; @@ -225,13 +226,15 @@ static void intel_enable_lvds(struct intel_encoder *encoder) if (wait_for((I915_READ(stat_reg) & PP_ON) != 0, 1000)) DRM_ERROR("timed out waiting for panel to power on\n"); - intel_panel_enable_backlight(dev, intel_crtc->pipe); + intel_panel_enable_backlight(intel_connector); } static void intel_disable_lvds(struct intel_encoder *encoder) { struct drm_device *dev = encoder->base.dev; struct intel_lvds_encoder *lvds_encoder = to_lvds_encoder(&encoder->base); + struct intel_connector *intel_connector = + &lvds_encoder->attached_connector->base; struct drm_i915_private *dev_priv = dev->dev_private; u32 ctl_reg, stat_reg; @@ -243,7 +246,7 @@ static void intel_disable_lvds(struct intel_encoder *encoder) stat_reg = PP_STATUS; } - intel_panel_disable_backlight(dev); + intel_panel_disable_backlight(intel_connector); I915_WRITE(ctl_reg, I915_READ(ctl_reg) & ~POWER_TARGET_ON); if (wait_for((I915_READ(stat_reg) & PP_ON) == 0, 1000)) diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c index 2acf5ca..2404e1c 100644 --- a/drivers/gpu/drm/i915/intel_opregion.c +++ b/drivers/gpu/drm/i915/intel_opregion.c @@ -395,6 +395,11 @@ static u32 asle_set_backlight(struct drm_device *dev, u32 bclp) { struct drm_i915_private *dev_priv = dev->dev_private; struct opregion_asle __iomem *asle = dev_priv->opregion.asle; + struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[0]; + struct drm_encoder *encoder; + struct drm_connector *connector; + struct intel_connector *intel_connector = NULL; + bool found = false; DRM_DEBUG_DRIVER("bclp = 0x%08x\n", bclp); @@ -405,7 +410,28 @@ static u32 asle_set_backlight(struct drm_device *dev, u32 bclp) if (bclp > 255) return ASLC_BACKLIGHT_FAILED; - intel_panel_set_backlight(dev, bclp, 255); + /* + * Could match the OpRegion connector here instead, but we'd + * also need to verify the connector could handle a backlight + * call. + */ + list_for_each_entry(encoder, &dev->mode_config.encoder_list, head) + if (encoder->crtc == crtc) { + found = true; + break; + } + + if (!found) + return ASLC_BACKLIGHT_FAILED; + + list_for_each_entry(connector, &dev->mode_config.connector_list, head) + if (connector->encoder == encoder) + intel_connector = to_intel_connector(connector); + + if (!found) + return ASLC_BACKLIGHT_FAILED; + + intel_panel_set_backlight(intel_connector, bclp, 255); iowrite32(DIV_ROUND_UP(bclp * 100, 255) | ASLE_CBLV_VALID, &asle->cblv); return 0; diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c index 5468416..5122f58 100644 --- a/drivers/gpu/drm/i915/intel_panel.c +++ b/drivers/gpu/drm/i915/intel_panel.c @@ -341,7 +341,7 @@ static int is_backlight_combination_mode(struct drm_device *dev) /* XXX: query mode clock or hardware clock and program max PWM appropriately * when it's 0. */ -static u32 i915_read_blc_pwm_ctl(struct drm_device *dev) +static u32 i915_read_blc_pwm_ctl(struct drm_device *dev, enum pipe pipe) { struct drm_i915_private *dev_priv = dev->dev_private; u32 val; @@ -380,11 +380,12 @@ static u32 i915_read_blc_pwm_ctl(struct drm_device *dev) return val; } -static u32 intel_panel_get_max_backlight(struct drm_device *dev) +static u32 intel_panel_get_max_backlight(struct drm_device *dev, + enum pipe pipe) { u32 max; - max = i915_read_blc_pwm_ctl(dev); + max = i915_read_blc_pwm_ctl(dev, pipe); if (HAS_PCH_SPLIT(dev)) { max >>= 16; @@ -410,7 +411,8 @@ MODULE_PARM_DESC(invert_brightness, "Invert backlight brightness " "to dri-devel@lists.freedesktop.org, if your machine needs it. " "It will then be included in an upcoming module version."); module_param_named(invert_brightness, i915_panel_invert_brightness, int, 0600); -static u32 intel_panel_compute_brightness(struct drm_device *dev, u32 val) +static u32 intel_panel_compute_brightness(struct drm_device *dev, + enum pipe pipe, u32 val) { struct drm_i915_private *dev_priv = dev->dev_private; @@ -419,7 +421,7 @@ static u32 intel_panel_compute_brightness(struct drm_device *dev, u32 val) if (i915_panel_invert_brightness > 0 || dev_priv->quirks & QUIRK_INVERT_BRIGHTNESS) { - u32 max = intel_panel_get_max_backlight(dev); + u32 max = intel_panel_get_max_backlight(dev, pipe); if (max) return max - val; } @@ -427,7 +429,8 @@ static u32 intel_panel_compute_brightness(struct drm_device *dev, u32 val) return val; } -static u32 intel_panel_get_backlight(struct drm_device *dev) +static u32 intel_panel_get_backlight(struct drm_device *dev, + enum pipe pipe) { struct drm_i915_private *dev_priv = dev->dev_private; u32 val; @@ -450,7 +453,7 @@ static u32 intel_panel_get_backlight(struct drm_device *dev) } } - val = intel_panel_compute_brightness(dev, val); + val = intel_panel_compute_brightness(dev, pipe, val); spin_unlock_irqrestore(&dev_priv->backlight.lock, flags); @@ -466,19 +469,19 @@ static void intel_pch_panel_set_backlight(struct drm_device *dev, u32 level) } static void intel_panel_actually_set_backlight(struct drm_device *dev, - u32 level) + enum pipe pipe, u32 level) { struct drm_i915_private *dev_priv = dev->dev_private; u32 tmp; DRM_DEBUG_DRIVER("set backlight PWM = %d\n", level); - level = intel_panel_compute_brightness(dev, level); + level = intel_panel_compute_brightness(dev, pipe, level); if (HAS_PCH_SPLIT(dev)) return intel_pch_panel_set_backlight(dev, level); if (is_backlight_combination_mode(dev)) { - u32 max = intel_panel_get_max_backlight(dev); + u32 max = intel_panel_get_max_backlight(dev, pipe); u8 lbpc; /* we're screwed, but keep behaviour backwards compatible */ @@ -498,15 +501,18 @@ static void intel_panel_actually_set_backlight(struct drm_device *dev, } /* set backlight brightness to level in range [0..max] */ -void intel_panel_set_backlight(struct drm_device *dev, u32 level, u32 max) +void intel_panel_set_backlight(struct intel_connector *connector, u32 level, + u32 max) { + struct drm_device *dev = connector->base.dev; struct drm_i915_private *dev_priv = dev->dev_private; + enum pipe pipe = intel_get_pipe_from_connector(connector); u32 freq; unsigned long flags; spin_lock_irqsave(&dev_priv->backlight.lock, flags); - freq = intel_panel_get_max_backlight(dev); + freq = intel_panel_get_max_backlight(dev, pipe); if (!freq) { /* we are screwed, bail out */ goto out; @@ -523,14 +529,16 @@ void intel_panel_set_backlight(struct drm_device *dev, u32 level, u32 max) dev_priv->backlight.device->props.brightness = level; if (dev_priv->backlight.enabled) - intel_panel_actually_set_backlight(dev, level); + intel_panel_actually_set_backlight(dev, pipe, level); out: spin_unlock_irqrestore(&dev_priv->backlight.lock, flags); } -void intel_panel_disable_backlight(struct drm_device *dev) +void intel_panel_disable_backlight(struct intel_connector *connector) { + struct drm_device *dev = connector->base.dev; struct drm_i915_private *dev_priv = dev->dev_private; + enum pipe pipe = intel_get_pipe_from_connector(connector); unsigned long flags; /* @@ -547,7 +555,7 @@ void intel_panel_disable_backlight(struct drm_device *dev) spin_lock_irqsave(&dev_priv->backlight.lock, flags); dev_priv->backlight.enabled = false; - intel_panel_actually_set_backlight(dev, 0); + intel_panel_actually_set_backlight(dev, pipe, 0); if (INTEL_INFO(dev)->gen >= 4) { uint32_t reg, tmp; @@ -566,10 +574,11 @@ void intel_panel_disable_backlight(struct drm_device *dev) spin_unlock_irqrestore(&dev_priv->backlight.lock, flags); } -void intel_panel_enable_backlight(struct drm_device *dev, - enum pipe pipe) +void intel_panel_enable_backlight(struct intel_connector *connector) { + struct drm_device *dev = connector->base.dev; struct drm_i915_private *dev_priv = dev->dev_private; + enum pipe pipe = intel_get_pipe_from_connector(connector); enum transcoder cpu_transcoder = intel_pipe_to_cpu_transcoder(dev_priv, pipe); unsigned long flags; @@ -577,7 +586,8 @@ void intel_panel_enable_backlight(struct drm_device *dev, spin_lock_irqsave(&dev_priv->backlight.lock, flags); if (dev_priv->backlight.level == 0) { - dev_priv->backlight.level = intel_panel_get_max_backlight(dev); + dev_priv->backlight.level = intel_panel_get_max_backlight(dev, + pipe); if (dev_priv->backlight.device) dev_priv->backlight.device->props.brightness = dev_priv->backlight.level; @@ -627,7 +637,8 @@ set_level: * registers are set. */ dev_priv->backlight.enabled = true; - intel_panel_actually_set_backlight(dev, dev_priv->backlight.level); + intel_panel_actually_set_backlight(dev, pipe, + dev_priv->backlight.level); spin_unlock_irqrestore(&dev_priv->backlight.lock, flags); } @@ -650,7 +661,7 @@ static void intel_panel_init_backlight(struct drm_device *dev) intel_panel_init_backlight_regs(dev); - dev_priv->backlight.level = intel_panel_get_backlight(dev); + dev_priv->backlight.level = intel_panel_get_backlight(dev, 0); dev_priv->backlight.enabled = dev_priv->backlight.level != 0; } @@ -679,16 +690,17 @@ intel_panel_detect(struct drm_device *dev) #if IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE) static int intel_panel_update_status(struct backlight_device *bd) { - struct drm_device *dev = bl_get_data(bd); - intel_panel_set_backlight(dev, bd->props.brightness, + struct intel_connector *connector = bl_get_data(bd); + intel_panel_set_backlight(connector, bd->props.brightness, bd->props.max_brightness); return 0; } static int intel_panel_get_brightness(struct backlight_device *bd) { - struct drm_device *dev = bl_get_data(bd); - return intel_panel_get_backlight(dev); + struct intel_connector *connector = bl_get_data(bd); + enum pipe pipe = intel_get_pipe_from_connector(connector); + return intel_panel_get_backlight(connector->base.dev, pipe); } static const struct backlight_ops intel_panel_bl_ops = { @@ -708,12 +720,13 @@ int intel_panel_setup_backlight(struct drm_connector *connector) if (WARN_ON(dev_priv->backlight.device)) return -ENODEV; + /* Just create a device for pipe 0 */ memset(&props, 0, sizeof(props)); props.type = BACKLIGHT_RAW; props.brightness = dev_priv->backlight.level; spin_lock_irqsave(&dev_priv->backlight.lock, flags); - props.max_brightness = intel_panel_get_max_backlight(dev); + props.max_brightness = intel_panel_get_max_backlight(dev, 0); spin_unlock_irqrestore(&dev_priv->backlight.lock, flags); if (props.max_brightness == 0) { @@ -722,7 +735,8 @@ int intel_panel_setup_backlight(struct drm_connector *connector) } dev_priv->backlight.device = backlight_device_register("intel_backlight", - &connector->kdev, dev, + &connector->kdev, + to_intel_connector(connector), &intel_panel_bl_ops, &props); if (IS_ERR(dev_priv->backlight.device)) {
On VLV/BYT, backlight controls a per-pipe, so when adjusting the backlight we need to pass the correct info. So make the externally visible backlight functions take a connector argument, which can be used internally to figure out the pipe backlight to adjust. Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> --- drivers/gpu/drm/i915/intel_display.c | 8 +++++ drivers/gpu/drm/i915/intel_dp.c | 5 ++- drivers/gpu/drm/i915/intel_drv.h | 8 +++-- drivers/gpu/drm/i915/intel_lvds.c | 9 +++-- drivers/gpu/drm/i915/intel_opregion.c | 28 ++++++++++++++- drivers/gpu/drm/i915/intel_panel.c | 66 +++++++++++++++++++++-------------- 6 files changed, 88 insertions(+), 36 deletions(-)