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