[Intel-gfx] [PATCH] drm/i915: Backlight Control over AUX feature
Adebisi, YetundeX
yetundex.adebisi at intel.com
Mon Sep 14 07:40:19 PDT 2015
> -----Original Message-----
> From: Jani Nikula [mailto:jani.nikula at linux.intel.com]
> Sent: Monday, September 14, 2015 11:00 AM
> To: Adebisi, YetundeX; Intel-gfx at lists.freedesktop.org
> Cc: Adebisi, YetundeX
> Subject: Re: [Intel-gfx] [PATCH] drm/i915: Backlight Control over AUX feature
>
> On Thu, 10 Sep 2015, Yetunde Adebisi <yetundex.adebisi at intel.com> wrote:
> > This patch adds support for Backlight Control over the AUX channel for
> > DP and eDP connectors. It allows the backlight of DP and eDP connected
> > displays to be controlled from software using sysfs interface.
>
> Which spec says there's DPCD backlight control support on non-embedded
> Display Port? AFAICT it's just eDP.
DPCD Backlight control is only specified in eDP.
However, this patch is also to address a customer use case for Chrontel's DP to LVDS add-on card (http://www.chrontel.com/index.php/ch7511b-lvds-displayport)
>
> > The code first checks if the DP/eDP display has the capability for
> > backlight control by reading Display Control DPCD registers as defined
> > by the eDP v1.3 VESA specs.
> > It then registers a /sys/backlight device if backlight control is
> > supported.
> >
> > It provides functions to
> > - Register a sysfs backlight interface if the eDP/DP connnector is
> > capable of aux backlight control
> > - Read the current backlight level from DPCD register 0x722
> > - Change the backlight level
> > - Disable/Enable the backlight by writing to DPCD register 0x720
>
> IMO the design should be to move dev_priv->display backlight hooks to be
> connector specific, in intel_panel, and it would be transparent whether
> backlight control is AUX based, GPU PWM based, PMIC PWM based, or DSI
> command mode based. I've given the exact same feedback about the recent
> DSI command mode backlight code.
>
> Most of the boilerplate code in intel_panel.c should remain intact, just the
> hooks would be in a different struct, and would, in DP AUX case, point at
> different functions. This patch duplicates way too much code for no good
> reason.
>
I will have a look at separating out the eDP AUX Backlight control as suggested.
> > Usage:
> > Backlight level ranges from 0(min)-240(max) 0x0-0xF0
> > - To change Backlight level to 50
> > echo 50 > /sys/class/backlight/intel_aux_backlight-DP-3/brightness
>
> IIUC it should be enough to have just one intel_backlight. And if not, I don't
> think tying the backlight to the connector name like this is a good idea.
> Userspace can already map the backlight interfaces to the connectors based
> on the parent kobject.
>
The customer use case is to be able to control the backlight of the connected displays independently.
The displays are connected using the DP/eDP to LVDS add-on card.
This is why there are multiple intel_backlight interface, there might be better approaches, I will appreciate your suggestions.
> >
> > - To disable backlight
> > echo 4 > /sys/class/backlight/intel_aux_backlight-DP-3/bl_power
> > - To enable backlight
> > echo 0 > /sys/class/backlight/intel_aux_backlight-DP-3/bl_power
> >
> > v2:
> > - Code clean up
> > - Avoid code duplication by merging the backlight device
> > register/unregister function with the existing one for internal
> > displays
> >
> > v3:
> > Further changes to re-use existing code by adding bl_name and
> > backlight_ops variables to the intel_backlight structure.
>
> Where were the previous versions? Somehow I don't think I would have
> missed them if they had been posted on the mailing lists.
Sorry, this was initially sent to isg-gms. I will remove them.
>
> >
> > Cc: Bob Paauwe <bob.j.paauwe at intel.com>
> > Signed-off-by: Yetunde Adebisi <yetundex.adebisi at intel.com>
> > ---
> > drivers/gpu/drm/i915/Makefile | 1 +
> > drivers/gpu/drm/i915/intel_display.c | 1 +
> > drivers/gpu/drm/i915/intel_dp.c | 6 +-
> > drivers/gpu/drm/i915/intel_dp_aux_backlight_ctl.c | 324
> ++++++++++++++++++++++
> > drivers/gpu/drm/i915/intel_drv.h | 46 ++-
> > drivers/gpu/drm/i915/intel_panel.c | 111 +++++---
> > include/drm/drm_dp_helper.h | 6 +
>
> Changes to this file must be a separate patch, posted to dri-devel list.
>
> Please find some further notes below.
>
> > 7 files changed, 443 insertions(+), 52 deletions(-) create mode
> > 100644 drivers/gpu/drm/i915/intel_dp_aux_backlight_ctl.c
> >
> > diff --git a/drivers/gpu/drm/i915/Makefile
> > b/drivers/gpu/drm/i915/Makefile index 44d290a..260be03 100644
> > --- a/drivers/gpu/drm/i915/Makefile
> > +++ b/drivers/gpu/drm/i915/Makefile
> > @@ -77,6 +77,7 @@ i915-y += dvo_ch7017.o \
> > dvo_tfp410.o \
> > intel_crt.o \
> > intel_ddi.o \
> > + intel_dp_aux_backlight_ctl.o \
> > intel_dp_mst.o \
> > intel_dp.o \
> > intel_dsi.o \
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > index e629a1b..2937432 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -15340,6 +15340,7 @@ void intel_connector_unregister(struct
> intel_connector *intel_connector)
> > struct drm_connector *connector = &intel_connector->base;
> >
> > intel_panel_destroy_backlight(connector);
> > + intel_dp_aux_backlight_destroy(connector);
> > drm_connector_unregister(connector);
> > }
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > b/drivers/gpu/drm/i915/intel_dp.c index 45ab25e..975a836 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -3018,7 +3018,7 @@ static void chv_dp_post_pll_disable(struct
> intel_encoder *encoder)
> > * Sinks are *supposed* to come up within 1ms from an off state, but
> we're also
> > * supposed to retry 3 times per the spec.
> > */
> > -static ssize_t
> > +ssize_t
> > intel_dp_dpcd_read_wake(struct drm_dp_aux *aux, unsigned int offset,
> > void *buffer, size_t size)
> > {
> > @@ -3969,7 +3969,7 @@ intel_dp_link_down(struct intel_dp *intel_dp)
> > msleep(intel_dp->panel_power_down_delay);
> > }
> >
> > -static bool
> > +bool
>
> Why?
It gets called from the newly added file intel_dp_aux_backlight_ctl.c. Might not be needed if I change it as you suggested.
>
> > intel_dp_get_dpcd(struct intel_dp *intel_dp) {
> > struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp); @@
> > -6125,6 +6125,8 @@ intel_dp_init_connector(struct intel_digital_port
> *intel_dig_port,
> > return false;
> > }
> >
> > + intel_dp_aux_backlight_init(intel_dp, connector);
> > +
> > intel_dp_add_properties(intel_dp, connector);
> >
> > /* For G4X desktop chip, PEG_BAND_GAP_DATA 3:0 must first be
> written
> > diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight_ctl.c
> > b/drivers/gpu/drm/i915/intel_dp_aux_backlight_ctl.c
> > new file mode 100644
> > index 0000000..a8ef960
> > --- /dev/null
> > +++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight_ctl.c
> > @@ -0,0 +1,324 @@
> > +/*
> > + * Copyright © 2015 Intel Corporation
> > + *
> > + * Permission is hereby granted, free of charge, to any person
> > +obtaining a
> > + * copy of this software and associated documentation files (the
> > +"Software"),
> > + * to deal in the Software without restriction, including without
> > +limitation
> > + * the rights to use, copy, modify, merge, publish, distribute,
> > +sublicense,
> > + * and/or sell copies of the Software, and to permit persons to whom
> > +the
> > + * Software is furnished to do so, subject to the following conditions:
> > + *
> > + * The above copyright notice and this permission notice (including
> > +the next
> > + * paragraph) shall be included in all copies or substantial portions
> > +of the
> > + * Software.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY
> KIND,
> > +EXPRESS OR
> > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> > +MERCHANTABILITY,
> > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN
> NO EVENT
> > +SHALL
> > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,
> DAMAGES
> > +OR OTHER
> > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
> OTHERWISE,
> > +ARISING
> > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE
> OR
> > +OTHER DEALINGS
> > + * IN THE SOFTWARE.
> > + *
> > + *
> > + */
> > +
> > +#include "intel_drv.h"
> > +
> > +
> > +#define MAX_AUX_BL_HW_LEVEL 0xF0
> > +#define MIN_AUX_BL_HW_LEVEL 0x00
> > +
> > +
> > +static uint8_t read_mode_set_reg(struct intel_dp *intel_dp) {
> > + uint8_t dpcd_buf = 0;
> > +
> > + if (intel_dp_dpcd_read_wake(&intel_dp->aux,
> > + DP_EDP_BACKLIGHT_MODE_SET_REGISTER,
> > + &dpcd_buf, sizeof(dpcd_buf)) < 0)
> > + DRM_DEBUG_KMS("Failed to read DPCD register 0x%x\n",
> > + DP_EDP_BACKLIGHT_MODE_SET_REGISTER);
> > +
> > + return dpcd_buf;
> > +}
> > +
> > +/**
> > + * Read the current backlight value from DPCD register(s) based
> > + * on if 8-bit(MSB) or 16-bit(MSB and LSB) values are supported */
> > +static uint16_t read_aux_backlight_level(struct intel_dp *intel_dp) {
> > + uint16_t read_val = 0;
> > +
> > + if (intel_dp_dpcd_read_wake(&intel_dp->aux,
> > + DP_EDP_BACKLIGHT_BRIGHTNESS_MSB,
> > + &read_val, sizeof(read_val)) < 0) {
> > + DRM_DEBUG_KMS("Failed to read DPCD register 0x%x\n",
> > + DP_EDP_BACKLIGHT_BRIGHTNESS_MSB);
> > + return 0;
> > + }
> > + if (intel_dp->aux_backlight.use_lsb_reg) {
> > + uint8_t val_lsb = 0;
> > +
> > + if (intel_dp_dpcd_read_wake(&intel_dp->aux,
> > + DP_EDP_BACKLIGHT_BRIGHTNESS_LSB,
> > + &val_lsb, sizeof(val_lsb)) < 0) {
> > + DRM_DEBUG_KMS("Failed to read DPCD register
> 0x%x\n",
> > +
> DP_EDP_BACKLIGHT_BRIGHTNESS_LSB);
> > + return 0;
> > + }
> > + read_val = (read_val << 8 | val_lsb);
> > + }
> > +
> > + return read_val;
> > +}
> > +
> > +
> > +static bool get_aux_backlight_enable(struct drm_dp_aux *aux) {
> > + uint8_t read_val = 0;
> > +
> > + if (intel_dp_dpcd_read_wake(aux,
> DP_EDP_DISPLAY_CONTROL_REGISTER,
> > + &read_val, sizeof(read_val)) < 0) {
> > + DRM_DEBUG_KMS("Failed to read DPCD register 0x%x\n",
> > + DP_EDP_DISPLAY_CONTROL_REGISTER);
> > + }
> > + return read_val & DP_EDP_BACKLIGHT_ENABLE; }
> > +
> > +
> > +static bool is_aux_display_control_capable(struct drm_connector
> > +*connector) {
> > + struct intel_encoder *intel_encoder =
> intel_attached_encoder(connector);
> > + struct intel_dp *intel_dp = enc_to_intel_dp(&intel_encoder->base);
> > + uint8_t dpcd_edp[2] = { 0x0 };
> > +
> > + /* Check the eDP Display control capabilities registers to determine if
> > + * the panel can support backlight control over the aux channel*/
> > + if (intel_dp_dpcd_read_wake(&intel_dp->aux,
> DP_EDP_GENERAL_CAP_1,
> > + dpcd_edp, sizeof(dpcd_edp)) < 0) {
> > + DRM_DEBUG_KMS("Failed to read DPCD Display Control
> registers\n");
> > + return false;
> > + }
> > + DRM_DEBUG_KMS("EDP DPCD : %*ph\n", (int) sizeof(dpcd_edp),
> > +dpcd_edp);
> > +
> > + if (dpcd_edp[0] &
> DP_EDP_TCON_BACKLIGHT_ADJUSTMENT_CAPABLE &&
> > + (dpcd_edp[0] &
> DP_EDP_BACKLIGHT_AUX_ENABLE_CAPABLE) &&
> > + (dpcd_edp[1] &
> DP_EDP_BACKLIGHT_BRIGHTNESS_AUX_SET_CAPABLE) &&
> > + ((read_mode_set_reg(intel_dp) &
> > +
> DP_EDP_BACKLIGHT_BRIGHTNESS_CTL_MODE_DPCD_MASK))) {
> > +
> > + DRM_DEBUG_KMS("AUX Backlight Control Supported!\n");
> > +
> > + if (dpcd_edp[1] &
> DP_EDP_BACKLIGHT_BRIGHTNESS_BYTE_COUNT)
> > + intel_dp->aux_backlight.use_lsb_reg = true;
> > +
> > + return true;
> > + }
> > + return false;
> > +}
> > +
> > +#if IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE)
> > +
> > +/**
> > + * Sends the current backlight level over the aux channel, checking
> > +if its using
> > + * 8-bit or 16 bit value (MSB and LSB) */ static void
> > +write_aux_backlight_level(struct intel_dp *intel_dp, uint16_t level)
> > +{
> > + uint8_t vals[2] = { 0x0 };
> > +
> > + vals[0] = level;
> > + DRM_DEBUG_KMS("Level 0x%x\n", level);
> > +
> > + /* Write the MSB and/or LSB */
> > + if (intel_dp->aux_backlight.use_lsb_reg) {
> > + vals[0] = (level & 0xFF00) >> 8;
> > + vals[1] = (level & 0xFF);
> > + if (drm_dp_dpcd_writeb(&intel_dp->aux,
> > + DP_EDP_BACKLIGHT_BRIGHTNESS_MSB,
> vals[1]) < 0) {
> > + DRM_DEBUG_KMS("Failed to write aux backlight
> level\n");
> > + return;
> > + }
> > + }
> > + if (drm_dp_dpcd_writeb(&intel_dp->aux,
> DP_EDP_BACKLIGHT_BRIGHTNESS_MSB,
> > + vals[0]) < 0) {
> > + DRM_DEBUG_KMS("Failed to write aux backlight level\n");
> > + return;
> > + }
> > + intel_dp->aux_backlight.level = level; }
> > +
> > +static void set_aux_backlight_enable(struct intel_dp *intel_dp, bool
> > +enable) {
> > + uint8_t reg_val = 0;
> > +
> > + if (intel_dp_dpcd_read_wake(&intel_dp->aux,
> > + DP_EDP_DISPLAY_CONTROL_REGISTER,
> > + ®_val, sizeof(reg_val)) < 0) {
> > + DRM_DEBUG_KMS("Failed to read DPCD register 0x%x\n",
> > + DP_EDP_DISPLAY_CONTROL_REGISTER);
> > + return;
> > + }
> > + if (enable)
> > + reg_val |= DP_EDP_BACKLIGHT_ENABLE;
> > + else
> > + reg_val &= ~(DP_EDP_BACKLIGHT_ENABLE);
> > +
> > + if (drm_dp_dpcd_writeb(&intel_dp->aux,
> DP_EDP_DISPLAY_CONTROL_REGISTER,
> > + reg_val) < 0) {
> > + DRM_DEBUG_KMS("Failed to %s aux backlight\n",
> > + enable ? "enable" : "disable");
> > + return;
> > + }
> > + intel_dp->aux_backlight.enabled = enable; }
> > +
> > +/**
> > + * intel_dp_aux_backlight_device_update_status
> > + * Writes to the sysfs backlight interface will come here to set the
> > + * backlight level or disable/enable the backlight */ static int
> > +intel_dp_aux_backlight_device_update_status(struct backlight_device
> > +*bd) {
> > + struct intel_connector *intel_connector = bl_get_data(bd);
> > + struct drm_connector *connector = &intel_connector->base;
> > + struct intel_encoder *intel_encoder =
> intel_attached_encoder(connector);
> > + struct intel_dp *intel_dp = enc_to_intel_dp(&intel_encoder->base);
> > + struct drm_device *dev = connector->dev;
> > + uint32_t hw_level;
> > + bool bl_enable = false;
> > +
> > + DRM_DEBUG_KMS("Updating intel_aux_backlight-DP,
> brightness=%d/%d, bl_power %d\n",
> > + bd->props.brightness, bd->props.max_brightness,
> > + bd->props.power);
> > +
> > + if (connector->status != connector_status_connected)
> > + return -ENODEV;
> > +
> > + WARN_ON(intel_dp->aux_backlight.max == 0);
> > +
> > + drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
> > +
> > + hw_level = scale(bd->props.brightness,
> > + 0, bd->props.max_brightness,
> > + intel_dp->aux_backlight.min, intel_dp-
> >aux_backlight.max);
> > +
> > + if (intel_dp->aux_backlight.level != hw_level)
> > + write_aux_backlight_level(intel_dp, hw_level);
> > +
> > + /*
> > + * This gets called when bl_power is written to as well. Check if the
> > + * bl_power state has changed and write it
> > + */
> > + bl_enable = (bd->props.power == FB_BLANK_UNBLANK);
> > + if (intel_dp->aux_backlight.enabled != bl_enable)
> > + set_aux_backlight_enable(intel_dp, bl_enable);
> > +
> > + drm_modeset_unlock(&dev->mode_config.connection_mutex);
> > + return 0;
> > +}
> > +
> > +/**
> > + * intel_dp_aux_backlight_device_get_brightness
> > + * Called on read of the sysfs backlight interface to get the current
> > +backlight
> > + * value from the AUX channel.
> > + */
> > +static int
> > +intel_dp_aux_backlight_device_get_brightness(struct backlight_device
> > +*bd) {
> > + struct intel_connector *intel_connector = bl_get_data(bd);
> > + struct drm_connector *connector = &intel_connector->base;
> > + struct drm_device *dev = connector->dev;
> > + struct intel_encoder *intel_encoder =
> intel_attached_encoder(connector);
> > + struct intel_dp *intel_dp = enc_to_intel_dp(&intel_encoder->base);
> > + int ret;
> > +
> > + if (connector->status != connector_status_connected)
> > + return -ENODEV;
> > +
> > + drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
> > +
> > + intel_dp->aux_backlight.level = read_aux_backlight_level(intel_dp);
> > +
> > + ret = scale(intel_dp->aux_backlight.level,
> > + intel_dp->aux_backlight.min, intel_dp-
> >aux_backlight.max,
> > + 0, bd->props.max_brightness);
> > +
> > + drm_modeset_unlock(&dev->mode_config.connection_mutex);
> > +
> > + return ret;
> > +}
> > +
> > +const struct backlight_ops intel_dp_backlight_device_ops = {
> > + .update_status = intel_dp_aux_backlight_device_update_status,
> > + .get_brightness = intel_dp_aux_backlight_device_get_brightness,
> > +};
> > +
> > +static void setup_backlight_ops(struct intel_backlight *backlight) {
> > + backlight->bl_ops = &intel_dp_backlight_device_ops; } #else
> > +
> > +static void setup_backlight_ops(struct intel_backlight *backlight) {
> > +
> > +}
> > +#endif /* CONFIG_BACKLIGHT_CLASS_DEVICE */
> > +
> > +
> > +static void _aux_backlight_init(struct intel_dp *intel_dp) {
> > + struct drm_connector *connector =
> > +&intel_dp->attached_connector->base;
> > +
> > + intel_dp->aux_backlight.max = MAX_AUX_BL_HW_LEVEL;
> > + intel_dp->aux_backlight.min = MIN_AUX_BL_HW_LEVEL;
> > +
> > + intel_dp->aux_backlight.level = read_aux_backlight_level(intel_dp);
> > + intel_dp->aux_backlight.enabled =
> > +get_aux_backlight_enable(&intel_dp->aux);
> > +
> > + setup_backlight_ops(&intel_dp->aux_backlight);
> > + /*
> > + * For AUX backlight, using different name for each DP/eDP connector
> > + * will produce registration of multiple DP-AUX backlight devices in
> > + * the driver.
> > + */
> > + snprintf(intel_dp->aux_backlight.bl_name,
> INTEL_BACKLIGHT_NAME_LEN,
> > + "intel_aux_backlight-%s", connector->name); }
> > +
> > +/**
> > + * Called on connector initialization to check for aux backlight
> > +control
> > + * capability and if present, initialize it.
> > + */
> > +
> > +void intel_dp_aux_backlight_init(struct intel_dp *intel_dp,
> > + struct drm_connector *connector)
> > +{
> > + if (!is_aux_display_control_capable(connector)) {
> > + DRM_DEBUG_KMS("Backlight control over AUX not
> supported\n");
> > + return;
> > + }
> > + intel_dp->aux_backlight.present = true;
> > +
> > + _aux_backlight_init(intel_dp);
> > +
> > + DRM_DEBUG_KMS("Connector %s backlight %s, brightness
> %u/%u\n",
> > + connector->name,
> > + intel_dp->aux_backlight.enabled ? "enabled" :
> "disabled",
> > + intel_dp->aux_backlight.level, intel_dp-
> >aux_backlight.max); }
> > +/**
> > + * Cleanup aux backlight control data */ void
> > +intel_dp_aux_backlight_destroy(struct drm_connector *connector) {
> > + struct intel_encoder *intel_encoder =
> intel_attached_encoder(connector);
> > + struct intel_dp *intel_dp = enc_to_intel_dp(&intel_encoder->base);
> > +
> > + intel_dp->aux_backlight.present = false; }
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > b/drivers/gpu/drm/i915/intel_drv.h
> > index 46484e4..438a87b 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -163,26 +163,31 @@ struct intel_encoder {
> > enum hpd_pin hpd_pin;
> > };
> >
> > +#define INTEL_BACKLIGHT_NAME_LEN 50
> > +struct intel_backlight {
> > + bool present;
> > + u32 level;
> > + u32 min;
> > + u32 max;
> > + bool enabled;
> > + bool combination_mode; /* gen 2/4 only */
> > + bool active_low_pwm;
> > +
> > + /* PWM chip */
> > + struct pwm_device *pwm;
> > +
> > + struct backlight_device *device;
> > + const struct backlight_ops *bl_ops;
> > + char bl_name[INTEL_BACKLIGHT_NAME_LEN];
> > + bool use_lsb_reg; /* aux backlight only */
> > +};
> > +
> > struct intel_panel {
> > struct drm_display_mode *fixed_mode;
> > struct drm_display_mode *downclock_mode;
> > int fitting_mode;
> >
> > - /* backlight */
> > - struct {
> > - bool present;
> > - u32 level;
> > - u32 min;
> > - u32 max;
> > - bool enabled;
> > - bool combination_mode; /* gen 2/4 only */
> > - bool active_low_pwm;
> > -
> > - /* PWM chip */
> > - struct pwm_device *pwm;
> > -
> > - struct backlight_device *device;
> > - } backlight;
> > + struct intel_backlight backlight;
> >
> > void (*backlight_power)(struct intel_connector *, bool enable); };
> > @@ -771,6 +776,8 @@ struct intel_dp {
> > unsigned long compliance_test_type;
> > unsigned long compliance_test_data;
> > bool compliance_test_active;
> > +
> > + struct intel_backlight aux_backlight;
> > };
> >
> > struct intel_digital_port {
> > @@ -1210,6 +1217,11 @@ void intel_edp_drrs_invalidate(struct
> drm_device *dev,
> > unsigned frontbuffer_bits);
> > void intel_edp_drrs_flush(struct drm_device *dev, unsigned
> > frontbuffer_bits); void hsw_dp_set_ddi_pll_sel(struct
> > intel_crtc_state *pipe_config);
> > +bool intel_dp_get_dpcd(struct intel_dp *intel_dp); ssize_t
> > +intel_dp_dpcd_read_wake(struct drm_dp_aux *aux, unsigned int offset,
> > + void *buffer, size_t size);
> > +uint32_t scale(uint32_t source_val, uint32_t source_min, uint32_t
> source_max,
> > + uint32_t target_min, uint32_t target_max);
> >
> > /* intel_dp_mst.c */
> > int intel_dp_mst_encoder_init(struct intel_digital_port
> > *intel_dig_port, int conn_id); @@ -1328,6 +1340,10 @@ extern struct
> > drm_display_mode *intel_find_panel_downclock( void
> > intel_backlight_register(struct drm_device *dev); void
> > intel_backlight_unregister(struct drm_device *dev);
> >
> > +/* intel_dp_aux_backlight_ctl.c */
> > +void intel_dp_aux_backlight_init(struct intel_dp *intel_dp,
> > + struct drm_connector *connector);
> > +void intel_dp_aux_backlight_destroy(struct drm_connector *connector);
> >
> > /* intel_psr.c */
> > void intel_psr_enable(struct intel_dp *intel_dp); diff --git
> > a/drivers/gpu/drm/i915/intel_panel.c
> > b/drivers/gpu/drm/i915/intel_panel.c
> > index 2034438a..c61fb92 100644
> > --- a/drivers/gpu/drm/i915/intel_panel.c
> > +++ b/drivers/gpu/drm/i915/intel_panel.c
> > @@ -410,7 +410,7 @@ intel_panel_detect(struct drm_device *dev)
> > * Return @source_val in range [@source_min.. at source_max] scaled to
> range
> > * [@target_min.. at target_max].
> > */
> > -static uint32_t scale(uint32_t source_val,
> > +uint32_t scale(uint32_t source_val,
> > uint32_t source_min, uint32_t source_max,
> > uint32_t target_min, uint32_t target_max) { @@ -1151,18
> > +1151,23 @@ static const struct backlight_ops intel_backlight_device_ops
> = {
> > .get_brightness = intel_backlight_device_get_brightness,
> > };
> >
> > -static int intel_backlight_device_register(struct intel_connector
> > *connector)
> > +static void setup_backlight_ops(struct intel_backlight *backlight) {
> > + backlight->bl_ops = &intel_backlight_device_ops; }
> > +
> > +static int intel_backlight_device_register(struct intel_connector
> *connector,
> > + struct intel_backlight *backlight)
>
> Again, AFAICT only eDP can have this, so intel_panel is the place to keep
> intel_backlight, and you don't have to modify the signature of this function.
>
> Even if regular DP supports this, I'd go with a clean implementation on eDP
> only first, getting that working and right, and the considering the implications
> of having backlight devices for external displays.
That makes sense, I will look at the patch you sent for setting up the eDP Backlight control over AUX.
>
> > {
> > - struct intel_panel *panel = &connector->panel;
> > struct backlight_properties props;
> >
> > - if (WARN_ON(panel->backlight.device))
> > + if (WARN_ON(backlight->device))
> > return -ENODEV;
> >
> > - if (!panel->backlight.present)
> > + if (!backlight->present)
> > return 0;
> >
> > - WARN_ON(panel->backlight.max == 0);
> > + WARN_ON(backlight->max == 0);
> >
> > memset(&props, 0, sizeof(props));
> > props.type = BACKLIGHT_RAW;
> > @@ -1171,30 +1176,25 @@ static int intel_backlight_device_register(struct
> intel_connector *connector)
> > * Note: Everything should work even if the backlight device max
> > * presented to the userspace is arbitrarily chosen.
> > */
> > - props.max_brightness = panel->backlight.max;
> > - props.brightness = scale_hw_to_user(connector,
> > - panel->backlight.level,
> > - props.max_brightness);
> > + props.max_brightness = backlight->max;
> > + props.brightness = scale(backlight->level, backlight->min, backlight-
> >max,
> > + 0, props.max_brightness);
> >
> > - if (panel->backlight.enabled)
> > + if (backlight->enabled)
> > props.power = FB_BLANK_UNBLANK;
> > else
> > props.power = FB_BLANK_POWERDOWN;
> >
> > - /*
> > - * Note: using the same name independent of the connector
> prevents
> > - * registration of multiple backlight devices in the driver.
> > - */
> > - panel->backlight.device =
> > - backlight_device_register("intel_backlight",
> > + backlight->device =
> > + backlight_device_register(backlight->bl_name,
> > connector->base.kdev,
> > connector,
> > - &intel_backlight_device_ops,
> &props);
> > + backlight->bl_ops, &props);
> >
> > - if (IS_ERR(panel->backlight.device)) {
> > + if (IS_ERR(backlight->device)) {
> > DRM_ERROR("Failed to register backlight: %ld\n",
> > - PTR_ERR(panel->backlight.device));
> > - panel->backlight.device = NULL;
> > + PTR_ERR(backlight->device));
> > + backlight->device = NULL;
> > return -ENODEV;
> > }
> >
> > @@ -1204,21 +1204,25 @@ static int intel_backlight_device_register(struct
> intel_connector *connector)
> > return 0;
> > }
> >
> > -static void intel_backlight_device_unregister(struct intel_connector
> > *connector)
> > +static void intel_backlight_device_unregister(struct intel_backlight
> > +*backlight)
> > {
> > - struct intel_panel *panel = &connector->panel;
> > -
> > - if (panel->backlight.device) {
> > - backlight_device_unregister(panel->backlight.device);
> > - panel->backlight.device = NULL;
> > + if (backlight->device) {
> > + backlight_device_unregister(backlight->device);
> > + backlight->device = NULL;
> > }
> > }
> > +
> > #else /* CONFIG_BACKLIGHT_CLASS_DEVICE */ -static int
> > intel_backlight_device_register(struct intel_connector *connector)
> > +
> > +static int intel_backlight_device_register(struct intel_connector
> *connector,
> > + struct intel_backlight *backlight)
> > {
> > return 0;
> > }
> > -static void intel_backlight_device_unregister(struct intel_connector
> > *connector)
> > +static void intel_backlight_device_unregister(struct intel_backlight
> > +*backlight) { } static void setup_backlight_ops(struct
> > +intel_backlight *backlight)
> > {
> > }
> > #endif /* CONFIG_BACKLIGHT_CLASS_DEVICE */ @@ -1652,6 +1656,17
> @@ int
> > intel_panel_setup_backlight(struct drm_connector *connector, enum pipe
> > pipe)
> >
> > panel->backlight.present = true;
> >
> > + setup_backlight_ops(&panel->backlight);
> > +
> > + /*
> > + * Note: For internal displays, using the same name independent of
> the
> > + * connector prevents registration of multiple backlight devices in the
> > + * driver.
> > + */
> > + strncpy(panel->backlight.bl_name, "intel_backlight",
> > + INTEL_BACKLIGHT_NAME_LEN);
> > + panel->backlight.bl_name[INTEL_BACKLIGHT_NAME_LEN - 1] = '\0';
> > +
> > DRM_DEBUG_KMS("Connector %s backlight initialized, %s,
> brightness %u/%u\n",
> > connector->name,
> > panel->backlight.enabled ? "enabled" : "disabled", @@
> > -1757,16 +1772,42 @@ void intel_panel_fini(struct intel_panel *panel)
> >
> > void intel_backlight_register(struct drm_device *dev) {
> > - struct intel_connector *connector;
> > + struct intel_connector *intel_connector;
> > +
> > + list_for_each_entry(intel_connector, &dev-
> >mode_config.connector_list,
> > + base.head) {
> > + struct drm_connector *connector = &intel_connector->base;
> > +
> > + intel_backlight_device_register(intel_connector,
> > + &intel_connector->panel.backlight);
> > +
> > + if (connector->connector_type ==
> DRM_MODE_CONNECTOR_DisplayPort ||
> > + connector->connector_type ==
> DRM_MODE_CONNECTOR_eDP) {
> > + struct intel_encoder *encoder =
> intel_attached_encoder(connector);
> > + struct intel_dp *intel_dp =
> enc_to_intel_dp(&encoder->base);
> >
> > - list_for_each_entry(connector, &dev->mode_config.connector_list,
> base.head)
> > - intel_backlight_device_register(connector);
> > + intel_backlight_device_register(intel_connector,
> > + &intel_dp->aux_backlight);
> > + }
> > + }
> > }
> >
> > void intel_backlight_unregister(struct drm_device *dev) {
> > - struct intel_connector *connector;
> > + struct intel_connector *intel_connector;
> > +
> > + list_for_each_entry(intel_connector, &dev-
> >mode_config.connector_list,
> > + base.head) {
> > + struct drm_connector *connector = &intel_connector->base;
> > +
> > +
> > +intel_backlight_device_unregister(&intel_connector->panel.backlight);
> >
> > - list_for_each_entry(connector, &dev->mode_config.connector_list,
> base.head)
> > - intel_backlight_device_unregister(connector);
> > + if (connector->connector_type ==
> DRM_MODE_CONNECTOR_DisplayPort ||
> > + connector->connector_type ==
> DRM_MODE_CONNECTOR_eDP) {
> > + struct intel_encoder *encoder =
> intel_attached_encoder(connector);
> > + struct intel_dp *intel_dp =
> enc_to_intel_dp(&encoder->base);
> > +
> > + intel_backlight_device_unregister(&intel_dp-
> >aux_backlight);
> > + }
> > + }
> > }
> > diff --git a/include/drm/drm_dp_helper.h
> b/include/drm/drm_dp_helper.h
> > index 8c52d0ef1..5826183 100644
> > --- a/include/drm/drm_dp_helper.h
> > +++ b/include/drm/drm_dp_helper.h
> > @@ -455,16 +455,22 @@
> > # define DP_EDP_14 0x03
> >
> > #define DP_EDP_GENERAL_CAP_1 0x701
> > +#define DP_EDP_TCON_BACKLIGHT_ADJUSTMENT_CAPABLE (1 << 0)
> > +#define DP_EDP_BACKLIGHT_AUX_ENABLE_CAPABLE (1 << 2)
> >
> > #define DP_EDP_BACKLIGHT_ADJUSTMENT_CAP 0x702
> > +#define DP_EDP_BACKLIGHT_BRIGHTNESS_AUX_SET_CAPABLE (1 << 1)
> > +#define DP_EDP_BACKLIGHT_BRIGHTNESS_BYTE_COUNT (1 << 2)
> >
> > #define DP_EDP_GENERAL_CAP_2 0x703
> >
> > #define DP_EDP_GENERAL_CAP_3 0x704 /* eDP 1.4 */
> >
> > #define DP_EDP_DISPLAY_CONTROL_REGISTER 0x720
> > +#define DP_EDP_BACKLIGHT_ENABLE (1 << 0)
> >
> > #define DP_EDP_BACKLIGHT_MODE_SET_REGISTER 0x721
> > +#define DP_EDP_BACKLIGHT_BRIGHTNESS_CTL_MODE_DPCD_MASK 0x2
> >
> > #define DP_EDP_BACKLIGHT_BRIGHTNESS_MSB 0x722
> > #define DP_EDP_BACKLIGHT_BRIGHTNESS_LSB 0x723
> > --
> > 1.9.3
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Jani Nikula, Intel Open Source Technology Center
More information about the Intel-gfx
mailing list