* [PATCH] drm/i915: Backlight Control over AUX feature
@ 2015-09-10 14:11 Yetunde Adebisi
2015-09-10 15:20 ` Daniel Vetter
2015-09-14 10:00 ` Jani Nikula
0 siblings, 2 replies; 8+ messages in thread
From: Yetunde Adebisi @ 2015-09-10 14:11 UTC (permalink / raw)
To: Intel-gfx; +Cc: Yetunde Adebisi
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.
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
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
- 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.
Cc: Bob Paauwe <bob.j.paauwe@intel.com>
Signed-off-by: Yetunde Adebisi <yetundex.adebisi@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 +
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
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)
{
- 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@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/i915: Backlight Control over AUX feature
2015-09-10 14:11 [PATCH] drm/i915: Backlight Control over AUX feature Yetunde Adebisi
@ 2015-09-10 15:20 ` Daniel Vetter
2015-09-14 16:29 ` Adebisi, YetundeX
2015-09-14 10:00 ` Jani Nikula
1 sibling, 1 reply; 8+ messages in thread
From: Daniel Vetter @ 2015-09-10 15:20 UTC (permalink / raw)
To: Yetunde Adebisi; +Cc: Intel-gfx
On Thu, Sep 10, 2015 at 03:11:42PM +0100, Yetunde Adebisi 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.
>
> 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
>
> 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
>
> - 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.
>
> Cc: Bob Paauwe <bob.j.paauwe@intel.com>
> Signed-off-by: Yetunde Adebisi <yetundex.adebisi@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 +
> 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 \
Shouldn't we have most of this implemented as a generic helper on top of
the core drm_dp_aux abstraction in the drm_dp_helper.c code? And drivers
would then just call the overall backlight_register/unregister functions
provided by those helpers.
The other bit from my high-level review is that the kerneldoc needs more
polish and needs to be pulled into drm.tmpl at a suitable place.
-Daniel
> 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
> 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)
> {
> - 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@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/i915: Backlight Control over AUX feature
2015-09-10 14:11 [PATCH] drm/i915: Backlight Control over AUX feature Yetunde Adebisi
2015-09-10 15:20 ` Daniel Vetter
@ 2015-09-14 10:00 ` Jani Nikula
2015-09-14 13:15 ` Daniel Vetter
2015-09-14 14:40 ` Adebisi, YetundeX
1 sibling, 2 replies; 8+ messages in thread
From: Jani Nikula @ 2015-09-14 10:00 UTC (permalink / raw)
To: Intel-gfx; +Cc: Yetunde Adebisi
On Thu, 10 Sep 2015, Yetunde Adebisi <yetundex.adebisi@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.
> 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@intel.com>
> Signed-off-by: Yetunde Adebisi <yetundex.adebisi@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?
> 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@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/i915: Backlight Control over AUX feature
2015-09-14 10:00 ` Jani Nikula
@ 2015-09-14 13:15 ` Daniel Vetter
2015-09-14 14:40 ` Adebisi, YetundeX
1 sibling, 0 replies; 8+ messages in thread
From: Daniel Vetter @ 2015-09-14 13:15 UTC (permalink / raw)
To: Jani Nikula; +Cc: Yetunde Adebisi, Intel-gfx
On Mon, Sep 14, 2015 at 01:00:26PM +0300, Jani Nikula wrote:
> On Thu, 10 Sep 2015, Yetunde Adebisi <yetundex.adebisi@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.
>
> > 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.
Registering piles of backlight controllers and letting userspace pick it
is how it works currently. Ugly, but well that's how it is. Fixing that up
would be a bit of work ...
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/i915: Backlight Control over AUX feature
2015-09-14 10:00 ` Jani Nikula
2015-09-14 13:15 ` Daniel Vetter
@ 2015-09-14 14:40 ` Adebisi, YetundeX
2015-09-15 7:26 ` Jani Nikula
1 sibling, 1 reply; 8+ messages in thread
From: Adebisi, YetundeX @ 2015-09-14 14:40 UTC (permalink / raw)
To: Jani Nikula, Intel-gfx@lists.freedesktop.org
> -----Original Message-----
> From: Jani Nikula [mailto:jani.nikula@linux.intel.com]
> Sent: Monday, September 14, 2015 11:00 AM
> To: Adebisi, YetundeX; Intel-gfx@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@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@intel.com>
> > Signed-off-by: Yetunde Adebisi <yetundex.adebisi@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..@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.
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@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/i915: Backlight Control over AUX feature
2015-09-10 15:20 ` Daniel Vetter
@ 2015-09-14 16:29 ` Adebisi, YetundeX
2015-09-14 17:18 ` Daniel Vetter
0 siblings, 1 reply; 8+ messages in thread
From: Adebisi, YetundeX @ 2015-09-14 16:29 UTC (permalink / raw)
To: Daniel Vetter; +Cc: Intel-gfx@lists.freedesktop.org
> -----Original Message-----
> From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel
> Vetter
> Sent: Thursday, September 10, 2015 4:21 PM
> To: Adebisi, YetundeX
> Cc: Intel-gfx@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH] drm/i915: Backlight Control over AUX feature
>
> On Thu, Sep 10, 2015 at 03:11:42PM +0100, Yetunde Adebisi 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.
> >
> > 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
> >
> > 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
> >
> > - 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.
> >
> > Cc: Bob Paauwe <bob.j.paauwe@intel.com>
> > Signed-off-by: Yetunde Adebisi <yetundex.adebisi@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 +
> > 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 \
>
> Shouldn't we have most of this implemented as a generic helper on top of
> the core drm_dp_aux abstraction in the drm_dp_helper.c code? And drivers
> would then just call the overall backlight_register/unregister functions
> provided by those helpers.
I can see that it makes sense to add helper functions for getting and setting the backlight values.
Can you please clarify what you mean about the backlight_register/unregister functions?
Apologies if this is obvious.
Thank you.
>
> The other bit from my high-level review is that the kerneldoc needs more
> polish and needs to be pulled into drm.tmpl at a suitable place.
> -Daniel
>
> > 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
> > 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 (c) 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)
> > {
> > - 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@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/i915: Backlight Control over AUX feature
2015-09-14 16:29 ` Adebisi, YetundeX
@ 2015-09-14 17:18 ` Daniel Vetter
0 siblings, 0 replies; 8+ messages in thread
From: Daniel Vetter @ 2015-09-14 17:18 UTC (permalink / raw)
To: Adebisi, YetundeX; +Cc: Intel-gfx@lists.freedesktop.org
On Mon, Sep 14, 2015 at 04:29:05PM +0000, Adebisi, YetundeX wrote:
>
>
> > -----Original Message-----
> > From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel
> > Vetter
> > Sent: Thursday, September 10, 2015 4:21 PM
> > To: Adebisi, YetundeX
> > Cc: Intel-gfx@lists.freedesktop.org
> > Subject: Re: [Intel-gfx] [PATCH] drm/i915: Backlight Control over AUX feature
> >
> > On Thu, Sep 10, 2015 at 03:11:42PM +0100, Yetunde Adebisi 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.
> > >
> > > 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
> > >
> > > 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
> > >
> > > - 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.
> > >
> > > Cc: Bob Paauwe <bob.j.paauwe@intel.com>
> > > Signed-off-by: Yetunde Adebisi <yetundex.adebisi@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 +
> > > 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 \
> >
> > Shouldn't we have most of this implemented as a generic helper on top of
> > the core drm_dp_aux abstraction in the drm_dp_helper.c code? And drivers
> > would then just call the overall backlight_register/unregister functions
> > provided by those helpers.
> I can see that it makes sense to add helper functions for getting and setting the backlight values.
>
> Can you please clarify what you mean about the backlight_register/unregister functions?
>
> Apologies if this is obvious.
See Jani's latest mail with the outcome from the irc discussion I had with
Jani and what the plan is. My mail here is before that discussion, so you
can entirely disregard it.
Thanks, Daniel
>
> Thank you.
> >
> > The other bit from my high-level review is that the kerneldoc needs more
> > polish and needs to be pulled into drm.tmpl at a suitable place.
> > -Daniel
> >
> > > 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
> > > 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 (c) 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)
> > > {
> > > - 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@lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/i915: Backlight Control over AUX feature
2015-09-14 14:40 ` Adebisi, YetundeX
@ 2015-09-15 7:26 ` Jani Nikula
0 siblings, 0 replies; 8+ messages in thread
From: Jani Nikula @ 2015-09-15 7:26 UTC (permalink / raw)
To: Adebisi, YetundeX, Intel-gfx@lists.freedesktop.org
On Mon, 14 Sep 2015, "Adebisi, YetundeX" <yetundex.adebisi@intel.com> wrote:
>> -----Original Message-----
>> From: Jani Nikula [mailto:jani.nikula@linux.intel.com]
>> Sent: Monday, September 14, 2015 11:00 AM
>> To: Adebisi, YetundeX; Intel-gfx@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@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)
>
Understood. However the first priority should be to get the design right
for the many orders of magnitude more common case of standards compliant
eDP DPCD backlight control.
Indeed the Chrontel datasheet also says it's eDP-to-LVDS, so you're
getting deeper in the unknown standards incompliance territory...
>>
>> > 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.
Okay. For the normal eDP case the solution would be to probe for
backlight capabilities in intel_dp.c *before* the call to
intel_panel_init(), and have the backlight hooks initialized
accordingly.
For this "eDP-to-LVDS converter in normal DP" case, I am frankly not
sure if it has a place in the upstream driver. With the eDP DPCD support
in place we can get pretty close to where you want to be, but then you'd
have to additionally setup DPCD backlight control for all DP
connectors. That may be something you have to carry out-of-tree, but I'm
pretty confident it's a rather small diff.
So please separate your work to separate patches along those lines. The
eDP case is not controversial and we'll want that no matter what, and it
is required prep work for your customer case anyway. And that's like
95+% of the whole thing.
BR,
Jani.
>
>> >
>> > - 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@intel.com>
>> > Signed-off-by: Yetunde Adebisi <yetundex.adebisi@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..@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.
>
> 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@lists.freedesktop.org
>> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>
>> --
>> Jani Nikula, Intel Open Source Technology Center
--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2015-09-15 7:23 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-09-10 14:11 [PATCH] drm/i915: Backlight Control over AUX feature Yetunde Adebisi
2015-09-10 15:20 ` Daniel Vetter
2015-09-14 16:29 ` Adebisi, YetundeX
2015-09-14 17:18 ` Daniel Vetter
2015-09-14 10:00 ` Jani Nikula
2015-09-14 13:15 ` Daniel Vetter
2015-09-14 14:40 ` Adebisi, YetundeX
2015-09-15 7:26 ` Jani Nikula
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.