On Sat, 28 Sep 2013, Jesse Barnes <jbarnes@xxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxxxxxxxx, 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@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
