Re: [PATCH v10 2/3] drm/i915: Add option to support dynamic backlight via DPCD

["Pandiyan, Dhinakaran" <dhinakaran.pandiyan@intel.com>]

On Wed, 2017-05-31 at 14:37 -0700, Puthikorn Voravootivat wrote:
> 
> 
> On Tue, May 30, 2017 at 9:18 PM, Pandiyan, Dhinakaran
> <dhinakaran.pandiyan@intel.com> wrote:
>         On Fri, 2017-05-26 at 18:42 -0700, Puthikorn Voravootivat
>         wrote:
>         > This patch adds option to enable dynamic backlight for eDP
>         > panel that supports this feature via DPCD register and
>         > set minimum / maximum brightness to 0% and 100% of the
>         > normal brightness.
>         >
>         > Signed-off-by: Puthikorn Voravootivat <puthik@chromium.org>
>         > ---
>         >  drivers/gpu/drm/i915/i915_params.c            |  5 ++++
>         >  drivers/gpu/drm/i915/i915_params.h            |  3 +-
>         >  drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 42
>         ++++++++++++++++++++++-----
>         >  3 files changed, 41 insertions(+), 9 deletions(-)
>         >
>         > diff --git a/drivers/gpu/drm/i915/i915_params.c
>         b/drivers/gpu/drm/i915/i915_params.c
>         > index 3758ae1f11b4..ce033d58134e 100644
>         > --- a/drivers/gpu/drm/i915/i915_params.c
>         > +++ b/drivers/gpu/drm/i915/i915_params.c
>         > @@ -65,6 +65,7 @@ struct i915_params i915 __read_mostly = {
>         >       .inject_load_failure = 0,
>         >       .enable_dpcd_backlight = -1,
>         >       .enable_gvt = false,
>         > +     .enable_dbc = false,
>         
>         Based on Daniel's earlier comments on module parameters,
>         shouldn't this
>         be enabled by default too?
> 
> 
> Yes. Will do.
>  
>         
>         Or even more importantly, is this the right approach to
>         enable/disable
>         dynamic back light control? The reason I recommended having
>         some sort of
>         control to disable/enable is that the eDP spec. says the
>         feature can
>         have user visible impact.
> 
> 
> I don't think we should expect end user to set this correctly. For
> power user,
> I think the i915_params is adequate. I don't want to add more
> complication
> to the driver.
>  

Sure, I am not requesting any changes here :) I meant to address the
question to Daniel in case he had any feedback.

-DK
>         
>         Table 10-3  Display Control Capabilities
>         
>         "While the DBC control bit’s function is defined in this
>         Standard, DBC implementation specifics are not defined,
>         including
>         interaction with other DPCD register settings. The DBC
>         implementation,
>         visual performance, and power savings characteristics can
>         differ between
>         specific panels."
>         
>         -DK
>         
>         >  };
>         >
>         >  module_param_named(modeset, i915.modeset, int, 0400);
>         > @@ -254,3 +255,7 @@ MODULE_PARM_DESC(enable_dpcd_backlight,
>         >  module_param_named(enable_gvt, i915.enable_gvt, bool,
>         0400);
>         >  MODULE_PARM_DESC(enable_gvt,
>         >       "Enable support for Intel GVT-g graphics
>         virtualization host support(default:false)");
>         > +
>         > +module_param_named_unsafe(enable_dbc, i915.enable_dbc,
>         bool, 0600);
>         > +MODULE_PARM_DESC(enable_dbc,
>         > +     "Enable support for dynamic backlight control
>         (default:false)");
>         > diff --git a/drivers/gpu/drm/i915/i915_params.h
>         b/drivers/gpu/drm/i915/i915_params.h
>         > index ac02efce6e22..2de3e2850b54 100644
>         > --- a/drivers/gpu/drm/i915/i915_params.h
>         > +++ b/drivers/gpu/drm/i915/i915_params.h
>         > @@ -67,7 +67,8 @@
>         >       func(bool, nuclear_pageflip); \
>         >       func(bool, enable_dp_mst); \
>         >       func(int, enable_dpcd_backlight); \
>         > -     func(bool, enable_gvt)
>         > +     func(bool, enable_gvt); \
>         > +     func(bool, enable_dbc)
>         >
>         >  #define MEMBER(T, member) T member
>         >  struct i915_params {
>         > diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
>         b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
>         > index c89aae804659..f55af41ce3bd 100644
>         > --- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
>         > +++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
>         > @@ -100,11 +100,26 @@ intel_dp_aux_set_backlight(struct
>         intel_connector *connector, u32 level)
>         >       }
>         >  }
>         >
>         > +/*
>         > + * Set minimum / maximum dynamic brightness percentage.
>         This value is expressed
>         > + * as the percentage of normal brightness in 5% increments.
>         > + */
>         > +static void
>         > +intel_dp_aux_set_dynamic_backlight_percent(struct intel_dp
>         *intel_dp,
>         > +                                        u32 min, u32 max)
>         > +{
>         > +     u8 dbc[] = { DIV_ROUND_CLOSEST(min, 5),
>         DIV_ROUND_CLOSEST(max, 5) };
>         > +
>         > +     if (drm_dp_dpcd_write(&intel_dp->aux,
>         DP_EDP_DBC_MINIMUM_BRIGHTNESS_SET,
>         > +                       dbc, sizeof(dbc)) < 0) {
>         > +             DRM_DEBUG_KMS("Failed to write aux DBC
>         brightness level\n");
>         > +     }
>         > +}
>         > +
>         >  static void intel_dp_aux_enable_backlight(struct
>         intel_connector *connector)
>         >  {
>         >       struct intel_dp *intel_dp =
>         enc_to_intel_dp(&connector->encoder->base);
>         > -     uint8_t dpcd_buf = 0;
>         > -     uint8_t edp_backlight_mode = 0;
>         > +     uint8_t dpcd_buf, new_dpcd_buf, edp_backlight_mode;
>         >
>         >       if (drm_dp_dpcd_readb(&intel_dp->aux,
>         >                       DP_EDP_BACKLIGHT_MODE_SET_REGISTER,
>         &dpcd_buf) != 1) {
>         > @@ -113,18 +128,15 @@ static void
>         intel_dp_aux_enable_backlight(struct intel_connector
>         *connector)
>         >               return;
>         >       }
>         >
>         > +     new_dpcd_buf = dpcd_buf;
>         >       edp_backlight_mode = dpcd_buf &
>         DP_EDP_BACKLIGHT_CONTROL_MODE_MASK;
>         >
>         >       switch (edp_backlight_mode) {
>         >       case DP_EDP_BACKLIGHT_CONTROL_MODE_PWM:
>         >       case DP_EDP_BACKLIGHT_CONTROL_MODE_PRESET:
>         >       case DP_EDP_BACKLIGHT_CONTROL_MODE_PRODUCT:
>         > -             dpcd_buf &=
>         ~DP_EDP_BACKLIGHT_CONTROL_MODE_MASK;
>         > -             dpcd_buf |=
>         DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD;
>         > -             if (drm_dp_dpcd_writeb(&intel_dp->aux,
>         > -                     DP_EDP_BACKLIGHT_MODE_SET_REGISTER,
>         dpcd_buf) < 0) {
>         > -                     DRM_DEBUG_KMS("Failed to write aux
>         backlight mode\n");
>         > -             }
>         > +             new_dpcd_buf &=
>         ~DP_EDP_BACKLIGHT_CONTROL_MODE_MASK;
>         > +             new_dpcd_buf |=
>         DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD;
>         >               break;
>         >
>         >       /* Do nothing when it is already DPCD mode */
>         > @@ -133,6 +145,20 @@ static void
>         intel_dp_aux_enable_backlight(struct intel_connector
>         *connector)
>         >               break;
>         >       }
>         >
>         > +     if (i915.enable_dbc &&
>         > +         (intel_dp->edp_dpcd[2] &
>         DP_EDP_DYNAMIC_BACKLIGHT_CAP)) {
>         > +             new_dpcd_buf |=
>         DP_EDP_DYNAMIC_BACKLIGHT_ENABLE;
>         > +
>          intel_dp_aux_set_dynamic_backlight_percent(intel_dp, 0, 100);
>         > +             DRM_DEBUG_KMS("Enable dynamic brightness.\n");
>         > +     }
>         > +
>         > +     if (new_dpcd_buf != dpcd_buf) {
>         > +             if (drm_dp_dpcd_writeb(&intel_dp->aux,
>         > +                     DP_EDP_BACKLIGHT_MODE_SET_REGISTER,
>         new_dpcd_buf) < 0) {
>         > +                     DRM_DEBUG_KMS("Failed to write aux
>         backlight mode\n");
>         > +             }
>         > +     }
>         > +
>         >       set_aux_backlight_enable(intel_dp, true);
>         >       intel_dp_aux_set_backlight(connector,
>         connector->panel.backlight.level);
>         >  }
>         
>         
> 
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx