On Wed, 30 Sep 2015, Yetunde Adebisi <yetundex.adebisi@xxxxxxxxx> wrote:
> This patch adds support for eDP backlight control using DPCD registers to
> backlight hooks in intel_panel.
>
> It checks for backlight control over AUX channel capability and sets up
> function pointers to get and set the backlight brightness level if
> supported.
>
> Cc: Bob Paauwe <bob.j.paauwe@xxxxxxxxx>
> Cc: Jani Nikula <jani.nikula@xxxxxxxxx>
> Cc: Deepak M <m.deepak@xxxxxxxxx>
> Signed-off-by: Yetunde Adebisi <yetundex.adebisi@xxxxxxxxx>
> ---
> drivers/gpu/drm/i915/intel_dp.c | 185 +++++++++++++++++++++++++++++++++++++
> drivers/gpu/drm/i915/intel_drv.h | 3 +
> drivers/gpu/drm/i915/intel_panel.c | 5 +-
> include/drm/drm_dp_helper.h | 6 ++
> 4 files changed, 198 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index fa1a524..fc4b896 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -6259,3 +6259,188 @@ void intel_dp_mst_resume(struct drm_device *dev)
> }
> }
> }
I think I'd like all of the below in a separate file. intel_dp.c is
already way too crowded; this is a logical set to have separately.
intel_dp_aux_backlight.c ?
> +
> +static uint8_t read_aux_backlight_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);
You only need this once, I'd just open code it where it's needed.
> +
> + return dpcd_buf;
> +}
> +
> +static bool get_aux_backlight_enable(struct drm_dp_aux *aux)
At a quick read _enable at the end is confusing, when you're not
enabling but rather getting the enabled status.
is_aux_backlight_enabled() ?
> +{
> + 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 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");
> + }
> +}
> +
> +/**
> + * Read the current backlight value from DPCD register(s) based
> + * on if 8-bit(MSB) or 16-bit(MSB and LSB) values are supported
This is not kernel-doc style comment, please use /* instead of /**.
> + */
> +static uint32_t intel_dp_aux_get_backlight(struct intel_connector *connector)
> +{
> + struct intel_dp *intel_dp = intel_attached_dp(&connector->base);
> + 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_blc_use_lsb) {
> + 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);
> + }
Please rewrite this to use only one read. Maybe parameterize the length
to read (1 or 2 bytes) so you only have one call to dpcd_read, and then
use either 1 or 2 bytes from the result buf.
> +
> + return read_val;
> +}
> +
> +/**
> + * Sends the current backlight level over the aux channel, checking if its using
> + * 8-bit or 16 bit value (MSB and LSB)
This is not kernel-doc style comment, please use /* instead of /**.
> + */
> +static void
> +intel_dp_aux_set_backlight(struct intel_connector *connector, u32 level)
> +{
> + struct intel_dp *intel_dp = intel_attached_dp(&connector->base);
> + 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_blc_use_lsb) {
> + vals[0] = (level & 0xFF00) >> 8;
> + vals[1] = (level & 0xFF);
> + if (drm_dp_dpcd_writeb(&intel_dp->aux,
> + DP_EDP_BACKLIGHT_BRIGHTNESS_LSB, 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;
> + }
Please rewrite this to use only one write, same as for read.
> +}
> +
> +static void intel_dp_aux_enable_backlight(struct intel_connector *connector)
> +{
> + set_aux_backlight_enable(intel_attached_dp(&connector->base), true);
> +}
> +
> +static void intel_dp_aux_disable_backlight(struct intel_connector *connector)
> +{
> + set_aux_backlight_enable(intel_attached_dp(&connector->base), false);
> +}
> +
> +static int intel_dp_aux_setup_backlight(struct intel_connector *connector,
> + enum pipe pipe)
> +{
> + struct intel_dp *intel_dp = intel_attached_dp(&connector->base);
> + struct intel_panel *panel = &connector->panel;
> +
> + if (!intel_dp_aux_display_control_capable(connector)) {
> + DRM_DEBUG_KMS("Backlight control over AUX not supported\n");
> + return -ENODEV;
> + }
The above shouldn't happen because you don't end up here unless the call
returns true in the first place. You do the aux reads and logging
twice. Please remove the above check.
> +
> + if (intel_dp->aux_blc_use_lsb)
> + panel->backlight.max = 0xFFFF;
> + else
> + panel->backlight.max = 0xFF;
> +
> + panel->backlight.min = 0;
> +
> + panel->backlight.level = intel_dp_aux_get_backlight(connector);
> + panel->backlight.enabled = get_aux_backlight_enable(&intel_dp->aux);
> +
> + return 0;
> +}
> +
> +void intel_dp_aux_init_backlight_funcs(struct intel_connector *intel_connector)
> +{
> + struct intel_panel *panel = &intel_connector->panel;
> +
> + panel->backlight.setup = intel_dp_aux_setup_backlight;
> + panel->backlight.enable = intel_dp_aux_enable_backlight;
> + panel->backlight.disable = intel_dp_aux_disable_backlight;
> + panel->backlight.set = intel_dp_aux_set_backlight;
> + panel->backlight.get = intel_dp_aux_get_backlight;
> +}
> +
> +bool intel_dp_aux_display_control_capable(struct intel_connector *connector)
> +{
> + struct intel_dp *intel_dp = intel_attached_dp(&connector->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;
> + }
I think it would be more useful to read 3 bytes when intel_dp_get_dpcd
reads DP_EDP_DPCD_REV, and store that in intel_dp, maybe as ->dpcd_edp
or something. It's faster to have fewer reads, and it's more generic to
have that instead of adding intel_dp->aux_blc_use_lsb.
> + DRM_DEBUG_KMS("EDP DPCD : %*ph\n", (int) sizeof(dpcd_edp), dpcd_edp);
Also do this at intel_dp_get_dpcd.
> +
> + 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_aux_backlight_mode_set_reg(intel_dp) &
> + DP_EDP_BACKLIGHT_BRIGHTNESS_CTL_MODE_DPCD_MASK))) {
This will enable aux backlight control also for the pwm * dpcd product
control mode. I don't think you handle that yet, so please check (reg &
3) == 2, with the proper #defines of course.
> +
> + DRM_DEBUG_KMS("AUX Backlight Control Supported!\n");
> +
> + if (dpcd_edp[1] & DP_EDP_BACKLIGHT_BRIGHTNESS_BYTE_COUNT)
> + intel_dp->aux_blc_use_lsb = true;
> +
> + return true;
> + }
> + return false;
> +}
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 35a65ca..dacbc37 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -747,6 +747,7 @@ struct intel_dp {
> unsigned long last_power_cycle;
> unsigned long last_power_on;
> unsigned long last_backlight_off;
> + bool aux_blc_use_lsb;
>
> struct notifier_block edp_notifier;
>
> @@ -1221,6 +1222,8 @@ 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_aux_display_control_capable(struct intel_connector *connector);
> +void intel_dp_aux_init_backlight_funcs(struct intel_connector *intel_connector);
>
> /* intel_dp_mst.c */
> int intel_dp_mst_encoder_init(struct intel_digital_port *intel_dig_port, int conn_id);
> diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
> index 9adb62b..04eff34 100644
> --- a/drivers/gpu/drm/i915/intel_panel.c
> +++ b/drivers/gpu/drm/i915/intel_panel.c
> @@ -1685,7 +1685,10 @@ intel_panel_init_backlight_funcs(struct intel_panel *panel)
> struct drm_device *dev = intel_connector->base.dev;
> struct drm_i915_private *dev_priv = dev->dev_private;
>
> - if (IS_BROXTON(dev)) {
> + if (intel_connector->base.connector_type == DRM_MODE_CONNECTOR_eDP &&
> + intel_dp_aux_display_control_capable(intel_connector)) {
> + intel_dp_aux_init_backlight_funcs(intel_connector);
I'd make this step separate from the platform specific if ladded, and if
the aux init function returns without errors, you return early from
intel_panel_init_backlight_funcs. The function could internally check
for capabilities etc. so you only need to expose one func outside of the
module.
> + } else if (IS_BROXTON(dev)) {
> panel->backlight.setup = bxt_setup_backlight;
> panel->backlight.enable = bxt_enable_backlight;
> panel->backlight.disable = bxt_disable_backlight;
> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> index 9ec4716..7367e1a 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
As I said in my earlier review, the changes to this file must be
separate, and need to be posted to dri-devel mailing list. I've now done
this, adding all of the other relevant defines as well:
http://patchwork.freedesktop.org/patch/msgid/1446109388-4301-1-git-send-email-jani.nikula@xxxxxxxxx
BR,
Jani.
--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
