On Thu, 10 Sep 2015, Yetunde Adebisi <yetundex.adebisi@xxxxxxxxx> 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.
> 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.
> 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.
>
> - 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.
>
> Cc: Bob Paauwe <bob.j.paauwe@xxxxxxxxx>
> Signed-off-by: Yetunde Adebisi <yetundex.adebisi@xxxxxxxxx>
> ---
> 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?
> 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..@source_max] scaled to range
> * [@target_min..@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.
> {
> - 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@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
