* [PATCH v10 1/3] drm/i915: Add heuristic to determine better way to adjust brightness
2017-05-27 1:42 [PATCH v10 0/3] Enhancement to intel_dp_aux_backlight driver Puthikorn Voravootivat
@ 2017-05-27 1:42 ` Puthikorn Voravootivat
2017-06-02 17:42 ` Pandiyan, Dhinakaran
2017-05-27 1:42 ` [PATCH v10 2/3] drm/i915: Add option to support dynamic backlight via DPCD Puthikorn Voravootivat
2017-05-27 1:42 ` [PATCH v10 3/3] drm/i915: Set PWM divider to match desired frequency in vbt Puthikorn Voravootivat
2 siblings, 1 reply; 14+ messages in thread
From: Puthikorn Voravootivat @ 2017-05-27 1:42 UTC (permalink / raw)
To: intel-gfx, Dhinakaran Pandiyan, Jani Nikula, Daniel Vetter
Cc: Puthikorn Voravootivat, dri-devel
Add heuristic to decide that AUX or PWM pin should use for
backlight brightness adjustment and modify i915 param description
to have auto, force disable, and force enable.
The heuristic to determine that using AUX pin is better than using
PWM pin is that the panel support any of the feature list here.
- Regional backlight brightness adjustment
- Backlight PWM frequency set
- More than 8 bits resolution of brightness level
- Backlight enablement via AUX and not by BL_ENABLE pin
Signed-off-by: Puthikorn Voravootivat <puthik@chromium.org>
---
drivers/gpu/drm/i915/i915_params.c | 7 +--
drivers/gpu/drm/i915/i915_params.h | 2 +-
drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 64 +++++++++++++++++++++++++--
3 files changed, 66 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
index b6a7e363d076..3758ae1f11b4 100644
--- a/drivers/gpu/drm/i915/i915_params.c
+++ b/drivers/gpu/drm/i915/i915_params.c
@@ -63,7 +63,7 @@ struct i915_params i915 __read_mostly = {
.huc_firmware_path = NULL,
.enable_dp_mst = true,
.inject_load_failure = 0,
- .enable_dpcd_backlight = false,
+ .enable_dpcd_backlight = -1,
.enable_gvt = false,
};
@@ -246,9 +246,10 @@ MODULE_PARM_DESC(enable_dp_mst,
module_param_named_unsafe(inject_load_failure, i915.inject_load_failure, uint, 0400);
MODULE_PARM_DESC(inject_load_failure,
"Force an error after a number of failure check points (0:disabled (default), N:force failure at the Nth failure check point)");
-module_param_named(enable_dpcd_backlight, i915.enable_dpcd_backlight, bool, 0600);
+module_param_named_unsafe(enable_dpcd_backlight, i915.enable_dpcd_backlight, int, 0600);
MODULE_PARM_DESC(enable_dpcd_backlight,
- "Enable support for DPCD backlight control (default:false)");
+ "Enable support for DPCD backlight control "
+ "(-1:auto (default), 0:force disable, 1:force enabled if supported");
module_param_named(enable_gvt, i915.enable_gvt, bool, 0400);
MODULE_PARM_DESC(enable_gvt,
diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
index 34148cc8637c..ac02efce6e22 100644
--- a/drivers/gpu/drm/i915/i915_params.h
+++ b/drivers/gpu/drm/i915/i915_params.h
@@ -66,7 +66,7 @@
func(bool, verbose_state_checks); \
func(bool, nuclear_pageflip); \
func(bool, enable_dp_mst); \
- func(bool, enable_dpcd_backlight); \
+ func(int, enable_dpcd_backlight); \
func(bool, enable_gvt)
#define MEMBER(T, member) T member
diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
index a0995c00fc84..c89aae804659 100644
--- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
+++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
@@ -43,6 +43,9 @@ static void set_aux_backlight_enable(struct intel_dp *intel_dp, bool enable)
else
reg_val &= ~(DP_EDP_BACKLIGHT_ENABLE);
+ /* TODO: If the panel also support enabling backlight via BL_ENABLE pin,
+ * the backlight will be enabled again in _intel_edp_backlight_on()
+ */
if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_EDP_DISPLAY_CONTROL_REGISTER,
reg_val) != 1) {
DRM_DEBUG_KMS("Failed to %s aux backlight\n",
@@ -168,15 +171,66 @@ intel_dp_aux_display_control_capable(struct intel_connector *connector)
/* Check the eDP Display control capabilities registers to determine if
* the panel can support backlight control over the aux channel
*/
- if (intel_dp->edp_dpcd[1] & DP_EDP_TCON_BACKLIGHT_ADJUSTMENT_CAP &&
- (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_AUX_SET_CAP) &&
- !(intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_PWM_PIN_CAP)) {
+ if ((intel_dp->edp_dpcd[1] & DP_EDP_TCON_BACKLIGHT_ADJUSTMENT_CAP) &&
+ (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_AUX_SET_CAP)) {
DRM_DEBUG_KMS("AUX Backlight Control Supported!\n");
return true;
}
return false;
}
+/*
+ * Heuristic function whether we should use AUX for backlight adjustment or not.
+ *
+ * We should use AUX for backlight brightness adjustment if panel doesn't this
+ * via PWM pin or using AUX is better than using PWM pin.
+ *
+ * The heuristic to determine that using AUX pin is better than using PWM pin is
+ * that the panel support any of the feature list here.
+ * - Regional backlight brightness adjustment
+ * - Backlight PWM frequency set
+ * - More than 8 bits resolution of brightness level
+ * - Backlight enablement via AUX and not by BL_ENABLE pin
+ *
+ * If all above are not true, assume that using PWM pin is better.
+ */
+static bool
+intel_dp_aux_display_control_heuristic(struct intel_connector *connector)
+{
+ struct intel_dp *intel_dp = enc_to_intel_dp(&connector->encoder->base);
+ uint8_t reg_val;
+
+ /* Panel doesn't support adjusting backlight brightness via PWN pin */
+ if (!(intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_BRIGHTNESS_PWM_PIN_CAP))
+ return true;
+
+ /* Panel supports regional backlight brightness adjustment */
+ if (drm_dp_dpcd_readb(&intel_dp->aux, DP_EDP_GENERAL_CAP_3,
+ ®_val) != 1) {
+ DRM_DEBUG_KMS("Failed to read DPCD register 0x%x\n",
+ DP_EDP_GENERAL_CAP_3);
+ return false;
+ }
+ if (reg_val > 0)
+ return true;
+
+ /* Panel supports backlight PWM frequency set */
+ if (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_FREQ_AUX_SET_CAP)
+ return true;
+
+ /* Panel supports more than 8 bits resolution of brightness level */
+ if (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_BYTE_COUNT)
+ return true;
+
+ /* Panel supports enabling backlight via AUX but not by BL_ENABLE pin */
+ if ((intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_AUX_ENABLE_CAP) &&
+ !(intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_PIN_ENABLE_CAP))
+ return true;
+
+ return false;
+
+}
+
int intel_dp_aux_init_backlight_funcs(struct intel_connector *intel_connector)
{
struct intel_panel *panel = &intel_connector->panel;
@@ -187,6 +241,10 @@ int intel_dp_aux_init_backlight_funcs(struct intel_connector *intel_connector)
if (!intel_dp_aux_display_control_capable(intel_connector))
return -ENODEV;
+ if (i915.enable_dpcd_backlight == -1 &&
+ !intel_dp_aux_display_control_heuristic(intel_connector))
+ return -ENODEV;
+
panel->backlight.setup = intel_dp_aux_setup_backlight;
panel->backlight.enable = intel_dp_aux_enable_backlight;
panel->backlight.disable = intel_dp_aux_disable_backlight;
--
2.13.0.219.gdb65acc882-goog
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH v10 1/3] drm/i915: Add heuristic to determine better way to adjust brightness
2017-05-27 1:42 ` [PATCH v10 1/3] drm/i915: Add heuristic to determine better way to adjust brightness Puthikorn Voravootivat
@ 2017-06-02 17:42 ` Pandiyan, Dhinakaran
2017-06-02 17:56 ` Pandiyan, Dhinakaran
0 siblings, 1 reply; 14+ messages in thread
From: Pandiyan, Dhinakaran @ 2017-06-02 17:42 UTC (permalink / raw)
To: puthik@chromium.org
Cc: dri-devel@lists.freedesktop.org, intel-gfx@lists.freedesktop.org
On Fri, 2017-05-26 at 18:42 -0700, Puthikorn Voravootivat wrote:
> Add heuristic to decide that AUX or PWM pin should use for
> backlight brightness adjustment and modify i915 param description
> to have auto, force disable, and force enable.
>
> The heuristic to determine that using AUX pin is better than using
> PWM pin is that the panel support any of the feature list here.
> - Regional backlight brightness adjustment
> - Backlight PWM frequency set
> - More than 8 bits resolution of brightness level
> - Backlight enablement via AUX and not by BL_ENABLE pin
>
> Signed-off-by: Puthikorn Voravootivat <puthik@chromium.org>
> ---
> drivers/gpu/drm/i915/i915_params.c | 7 +--
> drivers/gpu/drm/i915/i915_params.h | 2 +-
> drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 64 +++++++++++++++++++++++++--
> 3 files changed, 66 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
> index b6a7e363d076..3758ae1f11b4 100644
> --- a/drivers/gpu/drm/i915/i915_params.c
> +++ b/drivers/gpu/drm/i915/i915_params.c
> @@ -63,7 +63,7 @@ struct i915_params i915 __read_mostly = {
> .huc_firmware_path = NULL,
> .enable_dp_mst = true,
> .inject_load_failure = 0,
> - .enable_dpcd_backlight = false,
> + .enable_dpcd_backlight = -1,
> .enable_gvt = false,
> };
>
> @@ -246,9 +246,10 @@ MODULE_PARM_DESC(enable_dp_mst,
> module_param_named_unsafe(inject_load_failure, i915.inject_load_failure, uint, 0400);
> MODULE_PARM_DESC(inject_load_failure,
> "Force an error after a number of failure check points (0:disabled (default), N:force failure at the Nth failure check point)");
> -module_param_named(enable_dpcd_backlight, i915.enable_dpcd_backlight, bool, 0600);
> +module_param_named_unsafe(enable_dpcd_backlight, i915.enable_dpcd_backlight, int, 0600);
> MODULE_PARM_DESC(enable_dpcd_backlight,
> - "Enable support for DPCD backlight control (default:false)");
> + "Enable support for DPCD backlight control "
> + "(-1:auto (default), 0:force disable, 1:force enabled if supported");
>
> module_param_named(enable_gvt, i915.enable_gvt, bool, 0400);
> MODULE_PARM_DESC(enable_gvt,
> diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
> index 34148cc8637c..ac02efce6e22 100644
> --- a/drivers/gpu/drm/i915/i915_params.h
> +++ b/drivers/gpu/drm/i915/i915_params.h
> @@ -66,7 +66,7 @@
> func(bool, verbose_state_checks); \
> func(bool, nuclear_pageflip); \
> func(bool, enable_dp_mst); \
> - func(bool, enable_dpcd_backlight); \
> + func(int, enable_dpcd_backlight); \
> func(bool, enable_gvt)
>
> #define MEMBER(T, member) T member
> diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> index a0995c00fc84..c89aae804659 100644
> --- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> +++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> @@ -43,6 +43,9 @@ static void set_aux_backlight_enable(struct intel_dp *intel_dp, bool enable)
> else
> reg_val &= ~(DP_EDP_BACKLIGHT_ENABLE);
>
> + /* TODO: If the panel also support enabling backlight via BL_ENABLE pin,
> + * the backlight will be enabled again in _intel_edp_backlight_on()
> + */
Unrelated hunk, please remove. This should have been included in one of
the previous patches.
> if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_EDP_DISPLAY_CONTROL_REGISTER,
> reg_val) != 1) {
> DRM_DEBUG_KMS("Failed to %s aux backlight\n",
> @@ -168,15 +171,66 @@ intel_dp_aux_display_control_capable(struct intel_connector *connector)
> /* Check the eDP Display control capabilities registers to determine if
> * the panel can support backlight control over the aux channel
> */
> - if (intel_dp->edp_dpcd[1] & DP_EDP_TCON_BACKLIGHT_ADJUSTMENT_CAP &&
> - (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_AUX_SET_CAP) &&
> - !(intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_PWM_PIN_CAP)) {
> + if ((intel_dp->edp_dpcd[1] & DP_EDP_TCON_BACKLIGHT_ADJUSTMENT_CAP) &&
> + (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_AUX_SET_CAP)) {
> DRM_DEBUG_KMS("AUX Backlight Control Supported!\n");
> return true;
> }
> return false;
> }
>
> +/*
> + * Heuristic function whether we should use AUX for backlight adjustment or not.
> + *
> + * We should use AUX for backlight brightness adjustment if panel doesn't this
> + * via PWM pin or using AUX is better than using PWM pin.
> + *
> + * The heuristic to determine that using AUX pin is better than using PWM pin is
> + * that the panel support any of the feature list here.
> + * - Regional backlight brightness adjustment
> + * - Backlight PWM frequency set
> + * - More than 8 bits resolution of brightness level
> + * - Backlight enablement via AUX and not by BL_ENABLE pin
> + *
> + * If all above are not true, assume that using PWM pin is better.
> + */
> +static bool
> +intel_dp_aux_display_control_heuristic(struct intel_connector *connector)
> +{
> + struct intel_dp *intel_dp = enc_to_intel_dp(&connector->encoder->base);
> + uint8_t reg_val;
> +
> + /* Panel doesn't support adjusting backlight brightness via PWN pin */
> + if (!(intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_BRIGHTNESS_PWM_PIN_CAP))
Isn't BACKLIGHT_BRIGHTNESS_PWM_PIN_CAP in edp_dpcd[2] ?
> + return true;
> +
> + /* Panel supports regional backlight brightness adjustment */
> + if (drm_dp_dpcd_readb(&intel_dp->aux, DP_EDP_GENERAL_CAP_3,
Spec says this register is new to eDP 1.4. I don't see a check for
version.
> + ®_val) != 1) {
> + DRM_DEBUG_KMS("Failed to read DPCD register 0x%x\n",
> + DP_EDP_GENERAL_CAP_3);
> + return false;
> + }
> + if (reg_val > 0)
> + return true;
> +
> + /* Panel supports backlight PWM frequency set */
> + if (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_FREQ_AUX_SET_CAP)
> + return true;
> +
> + /* Panel supports more than 8 bits resolution of brightness level */
> + if (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_BYTE_COUNT)
> + return true;
I don't know if more than 8 bits is relatively better than the
resolution PWM pin allows. I guess, we could fix that later by comparing
the number of brightness levels.
> +
> + /* Panel supports enabling backlight via AUX but not by BL_ENABLE pin */
> + if ((intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_AUX_ENABLE_CAP) &&
> + !(intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_PIN_ENABLE_CAP))
> + return true;
> +
> + return false;
> +
> +}
> +
> int intel_dp_aux_init_backlight_funcs(struct intel_connector *intel_connector)
> {
> struct intel_panel *panel = &intel_connector->panel;
> @@ -187,6 +241,10 @@ int intel_dp_aux_init_backlight_funcs(struct intel_connector *intel_connector)
> if (!intel_dp_aux_display_control_capable(intel_connector))
> return -ENODEV;
>
> + if (i915.enable_dpcd_backlight == -1 &&
> + !intel_dp_aux_display_control_heuristic(intel_connector))
> + return -ENODEV;
> +
> panel->backlight.setup = intel_dp_aux_setup_backlight;
> panel->backlight.enable = intel_dp_aux_enable_backlight;
> panel->backlight.disable = intel_dp_aux_disable_backlight;
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH v10 1/3] drm/i915: Add heuristic to determine better way to adjust brightness
2017-06-02 17:42 ` Pandiyan, Dhinakaran
@ 2017-06-02 17:56 ` Pandiyan, Dhinakaran
2017-06-03 1:35 ` Puthikorn Voravootivat
0 siblings, 1 reply; 14+ messages in thread
From: Pandiyan, Dhinakaran @ 2017-06-02 17:56 UTC (permalink / raw)
To: puthik@chromium.org
Cc: daniel.vetter@ffwll.ch, intel-gfx@lists.freedesktop.org,
dri-devel@lists.freedesktop.org
On Fri, 2017-06-02 at 17:42 +0000, Pandiyan, Dhinakaran wrote:
Somehow the CC's got removed in my previous reply, adding them back. See
one additional comment below.
> On Fri, 2017-05-26 at 18:42 -0700, Puthikorn Voravootivat wrote:
> > Add heuristic to decide that AUX or PWM pin should use for
> > backlight brightness adjustment and modify i915 param description
> > to have auto, force disable, and force enable.
> >
> > The heuristic to determine that using AUX pin is better than using
> > PWM pin is that the panel support any of the feature list here.
> > - Regional backlight brightness adjustment
> > - Backlight PWM frequency set
> > - More than 8 bits resolution of brightness level
> > - Backlight enablement via AUX and not by BL_ENABLE pin
> >
> > Signed-off-by: Puthikorn Voravootivat <puthik@chromium.org>
> > ---
> > drivers/gpu/drm/i915/i915_params.c | 7 +--
> > drivers/gpu/drm/i915/i915_params.h | 2 +-
> > drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 64 +++++++++++++++++++++++++--
> > 3 files changed, 66 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
> > index b6a7e363d076..3758ae1f11b4 100644
> > --- a/drivers/gpu/drm/i915/i915_params.c
> > +++ b/drivers/gpu/drm/i915/i915_params.c
> > @@ -63,7 +63,7 @@ struct i915_params i915 __read_mostly = {
> > .huc_firmware_path = NULL,
> > .enable_dp_mst = true,
> > .inject_load_failure = 0,
> > - .enable_dpcd_backlight = false,
> > + .enable_dpcd_backlight = -1,
> > .enable_gvt = false,
> > };
> >
> > @@ -246,9 +246,10 @@ MODULE_PARM_DESC(enable_dp_mst,
> > module_param_named_unsafe(inject_load_failure, i915.inject_load_failure, uint, 0400);
> > MODULE_PARM_DESC(inject_load_failure,
> > "Force an error after a number of failure check points (0:disabled (default), N:force failure at the Nth failure check point)");
> > -module_param_named(enable_dpcd_backlight, i915.enable_dpcd_backlight, bool, 0600);
> > +module_param_named_unsafe(enable_dpcd_backlight, i915.enable_dpcd_backlight, int, 0600);
> > MODULE_PARM_DESC(enable_dpcd_backlight,
> > - "Enable support for DPCD backlight control (default:false)");
> > + "Enable support for DPCD backlight control "
> > + "(-1:auto (default), 0:force disable, 1:force enabled if supported");
> >
> > module_param_named(enable_gvt, i915.enable_gvt, bool, 0400);
> > MODULE_PARM_DESC(enable_gvt,
> > diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
> > index 34148cc8637c..ac02efce6e22 100644
> > --- a/drivers/gpu/drm/i915/i915_params.h
> > +++ b/drivers/gpu/drm/i915/i915_params.h
> > @@ -66,7 +66,7 @@
> > func(bool, verbose_state_checks); \
> > func(bool, nuclear_pageflip); \
> > func(bool, enable_dp_mst); \
> > - func(bool, enable_dpcd_backlight); \
> > + func(int, enable_dpcd_backlight); \
Please move this above the bools, see comment in code that's a few lines
above.
-DK
> > func(bool, enable_gvt)
> >
> > #define MEMBER(T, member) T member
> > diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> > index a0995c00fc84..c89aae804659 100644
> > --- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> > +++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> > @@ -43,6 +43,9 @@ static void set_aux_backlight_enable(struct intel_dp *intel_dp, bool enable)
> > else
> > reg_val &= ~(DP_EDP_BACKLIGHT_ENABLE);
> >
> > + /* TODO: If the panel also support enabling backlight via BL_ENABLE pin,
> > + * the backlight will be enabled again in _intel_edp_backlight_on()
> > + */
>
> Unrelated hunk, please remove. This should have been included in one of
> the previous patches.
>
> > if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_EDP_DISPLAY_CONTROL_REGISTER,
> > reg_val) != 1) {
> > DRM_DEBUG_KMS("Failed to %s aux backlight\n",
> > @@ -168,15 +171,66 @@ intel_dp_aux_display_control_capable(struct intel_connector *connector)
> > /* Check the eDP Display control capabilities registers to determine if
> > * the panel can support backlight control over the aux channel
> > */
> > - if (intel_dp->edp_dpcd[1] & DP_EDP_TCON_BACKLIGHT_ADJUSTMENT_CAP &&
> > - (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_AUX_SET_CAP) &&
> > - !(intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_PWM_PIN_CAP)) {
> > + if ((intel_dp->edp_dpcd[1] & DP_EDP_TCON_BACKLIGHT_ADJUSTMENT_CAP) &&
> > + (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_AUX_SET_CAP)) {
> > DRM_DEBUG_KMS("AUX Backlight Control Supported!\n");
> > return true;
> > }
> > return false;
> > }
> >
> > +/*
> > + * Heuristic function whether we should use AUX for backlight adjustment or not.
> > + *
> > + * We should use AUX for backlight brightness adjustment if panel doesn't this
> > + * via PWM pin or using AUX is better than using PWM pin.
> > + *
> > + * The heuristic to determine that using AUX pin is better than using PWM pin is
> > + * that the panel support any of the feature list here.
> > + * - Regional backlight brightness adjustment
> > + * - Backlight PWM frequency set
> > + * - More than 8 bits resolution of brightness level
> > + * - Backlight enablement via AUX and not by BL_ENABLE pin
> > + *
> > + * If all above are not true, assume that using PWM pin is better.
> > + */
> > +static bool
> > +intel_dp_aux_display_control_heuristic(struct intel_connector *connector)
> > +{
> > + struct intel_dp *intel_dp = enc_to_intel_dp(&connector->encoder->base);
> > + uint8_t reg_val;
> > +
> > + /* Panel doesn't support adjusting backlight brightness via PWN pin */
> > + if (!(intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_BRIGHTNESS_PWM_PIN_CAP))
>
> Isn't BACKLIGHT_BRIGHTNESS_PWM_PIN_CAP in edp_dpcd[2] ?
>
>
> > + return true;
> > +
> > + /* Panel supports regional backlight brightness adjustment */
> > + if (drm_dp_dpcd_readb(&intel_dp->aux, DP_EDP_GENERAL_CAP_3,
>
> Spec says this register is new to eDP 1.4. I don't see a check for
> version.
>
>
> > + ®_val) != 1) {
> > + DRM_DEBUG_KMS("Failed to read DPCD register 0x%x\n",
> > + DP_EDP_GENERAL_CAP_3);
> > + return false;
> > + }
> > + if (reg_val > 0)
> > + return true;
> > +
> > + /* Panel supports backlight PWM frequency set */
> > + if (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_FREQ_AUX_SET_CAP)
> > + return true;
> > +
> > + /* Panel supports more than 8 bits resolution of brightness level */
> > + if (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_BYTE_COUNT)
> > + return true;
>
> I don't know if more than 8 bits is relatively better than the
> resolution PWM pin allows. I guess, we could fix that later by comparing
> the number of brightness levels.
>
> > +
> > + /* Panel supports enabling backlight via AUX but not by BL_ENABLE pin */
> > + if ((intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_AUX_ENABLE_CAP) &&
> > + !(intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_PIN_ENABLE_CAP))
> > + return true;
> > +
> > + return false;
> > +
> > +}
> > +
> > int intel_dp_aux_init_backlight_funcs(struct intel_connector *intel_connector)
> > {
> > struct intel_panel *panel = &intel_connector->panel;
> > @@ -187,6 +241,10 @@ int intel_dp_aux_init_backlight_funcs(struct intel_connector *intel_connector)
> > if (!intel_dp_aux_display_control_capable(intel_connector))
> > return -ENODEV;
> >
> > + if (i915.enable_dpcd_backlight == -1 &&
> > + !intel_dp_aux_display_control_heuristic(intel_connector))
> > + return -ENODEV;
> > +
> > panel->backlight.setup = intel_dp_aux_setup_backlight;
> > panel->backlight.enable = intel_dp_aux_enable_backlight;
> > panel->backlight.disable = intel_dp_aux_disable_backlight;
>
> _______________________________________________
> 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
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH v10 1/3] drm/i915: Add heuristic to determine better way to adjust brightness
2017-06-02 17:56 ` Pandiyan, Dhinakaran
@ 2017-06-03 1:35 ` Puthikorn Voravootivat
0 siblings, 0 replies; 14+ messages in thread
From: Puthikorn Voravootivat @ 2017-06-03 1:35 UTC (permalink / raw)
To: Pandiyan, Dhinakaran
Cc: puthik@chromium.org, daniel.vetter@ffwll.ch,
intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org
[-- Attachment #1.1: Type: text/plain, Size: 8851 bytes --]
On Fri, Jun 2, 2017 at 10:56 AM, Pandiyan, Dhinakaran <
dhinakaran.pandiyan@intel.com> wrote:
> On Fri, 2017-06-02 at 17:42 +0000, Pandiyan, Dhinakaran wrote:
>
> Somehow the CC's got removed in my previous reply, adding them back. See
> one additional comment below.
>
>
> > On Fri, 2017-05-26 at 18:42 -0700, Puthikorn Voravootivat wrote:
> > > Add heuristic to decide that AUX or PWM pin should use for
> > > backlight brightness adjustment and modify i915 param description
> > > to have auto, force disable, and force enable.
> > >
> > > The heuristic to determine that using AUX pin is better than using
> > > PWM pin is that the panel support any of the feature list here.
> > > - Regional backlight brightness adjustment
> > > - Backlight PWM frequency set
> > > - More than 8 bits resolution of brightness level
> > > - Backlight enablement via AUX and not by BL_ENABLE pin
> > >
> > > Signed-off-by: Puthikorn Voravootivat <puthik@chromium.org>
> > > ---
> > > drivers/gpu/drm/i915/i915_params.c | 7 +--
> > > drivers/gpu/drm/i915/i915_params.h | 2 +-
> > > drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 64
> +++++++++++++++++++++++++--
> > > 3 files changed, 66 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_params.c
> b/drivers/gpu/drm/i915/i915_params.c
> > > index b6a7e363d076..3758ae1f11b4 100644
> > > --- a/drivers/gpu/drm/i915/i915_params.c
> > > +++ b/drivers/gpu/drm/i915/i915_params.c
> > > @@ -63,7 +63,7 @@ struct i915_params i915 __read_mostly = {
> > > .huc_firmware_path = NULL,
> > > .enable_dp_mst = true,
> > > .inject_load_failure = 0,
> > > - .enable_dpcd_backlight = false,
> > > + .enable_dpcd_backlight = -1,
> > > .enable_gvt = false,
> > > };
> > >
> > > @@ -246,9 +246,10 @@ MODULE_PARM_DESC(enable_dp_mst,
> > > module_param_named_unsafe(inject_load_failure,
> i915.inject_load_failure, uint, 0400);
> > > MODULE_PARM_DESC(inject_load_failure,
> > > "Force an error after a number of failure check points (0:disabled
> (default), N:force failure at the Nth failure check point)");
> > > -module_param_named(enable_dpcd_backlight,
> i915.enable_dpcd_backlight, bool, 0600);
> > > +module_param_named_unsafe(enable_dpcd_backlight,
> i915.enable_dpcd_backlight, int, 0600);
> > > MODULE_PARM_DESC(enable_dpcd_backlight,
> > > - "Enable support for DPCD backlight control (default:false)");
> > > + "Enable support for DPCD backlight control "
> > > + "(-1:auto (default), 0:force disable, 1:force enabled if
> supported");
> > >
> > > module_param_named(enable_gvt, i915.enable_gvt, bool, 0400);
> > > MODULE_PARM_DESC(enable_gvt,
> > > diff --git a/drivers/gpu/drm/i915/i915_params.h
> b/drivers/gpu/drm/i915/i915_params.h
> > > index 34148cc8637c..ac02efce6e22 100644
> > > --- a/drivers/gpu/drm/i915/i915_params.h
> > > +++ b/drivers/gpu/drm/i915/i915_params.h
> > > @@ -66,7 +66,7 @@
> > > func(bool, verbose_state_checks); \
> > > func(bool, nuclear_pageflip); \
> > > func(bool, enable_dp_mst); \
> > > - func(bool, enable_dpcd_backlight); \
> > > + func(int, enable_dpcd_backlight); \
>
>
> Please move this above the bools, see comment in code that's a few lines
> above.
>
> Will do
> -DK
>
> > > func(bool, enable_gvt)
> > >
> > > #define MEMBER(T, member) T member
> > > diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> > > index a0995c00fc84..c89aae804659 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> > > @@ -43,6 +43,9 @@ static void set_aux_backlight_enable(struct
> intel_dp *intel_dp, bool enable)
> > > else
> > > reg_val &= ~(DP_EDP_BACKLIGHT_ENABLE);
> > >
> > > + /* TODO: If the panel also support enabling backlight via
> BL_ENABLE pin,
> > > + * the backlight will be enabled again in _intel_edp_backlight_on()
> > > + */
> >
> > Unrelated hunk, please remove. This should have been included in one of
> > the previous patches.
>
Removed
> >
> > > if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_EDP_DISPLAY_CONTROL_
> REGISTER,
> > > reg_val) != 1) {
> > > DRM_DEBUG_KMS("Failed to %s aux backlight\n",
> > > @@ -168,15 +171,66 @@ intel_dp_aux_display_control_capable(struct
> intel_connector *connector)
> > > /* Check the eDP Display control capabilities registers to
> determine if
> > > * the panel can support backlight control over the aux channel
> > > */
> > > - if (intel_dp->edp_dpcd[1] & DP_EDP_TCON_BACKLIGHT_ADJUSTMENT_CAP
> &&
> > > - (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_AUX_SET_CAP)
> &&
> > > - !(intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_PWM_PIN_CAP))
> {
> > > + if ((intel_dp->edp_dpcd[1] & DP_EDP_TCON_BACKLIGHT_ADJUSTMENT_CAP)
> &&
> > > + (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_AUX_SET_CAP))
> {
> > > DRM_DEBUG_KMS("AUX Backlight Control Supported!\n");
> > > return true;
> > > }
> > > return false;
> > > }
> > >
> > > +/*
> > > + * Heuristic function whether we should use AUX for backlight
> adjustment or not.
> > > + *
> > > + * We should use AUX for backlight brightness adjustment if panel
> doesn't this
> > > + * via PWM pin or using AUX is better than using PWM pin.
> > > + *
> > > + * The heuristic to determine that using AUX pin is better than using
> PWM pin is
> > > + * that the panel support any of the feature list here.
> > > + * - Regional backlight brightness adjustment
> > > + * - Backlight PWM frequency set
> > > + * - More than 8 bits resolution of brightness level
> > > + * - Backlight enablement via AUX and not by BL_ENABLE pin
> > > + *
> > > + * If all above are not true, assume that using PWM pin is better.
> > > + */
> > > +static bool
> > > +intel_dp_aux_display_control_heuristic(struct intel_connector
> *connector)
> > > +{
> > > + struct intel_dp *intel_dp = enc_to_intel_dp(&connector->
> encoder->base);
> > > + uint8_t reg_val;
> > > +
> > > + /* Panel doesn't support adjusting backlight brightness via PWN
> pin */
> > > + if (!(intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_BRIGHTNESS_
> PWM_PIN_CAP))
> >
> > Isn't BACKLIGHT_BRIGHTNESS_PWM_PIN_CAP in edp_dpcd[2] ?
> >
>
Thank you for catching this.
> >
> > > + return true;
> > > +
> > > + /* Panel supports regional backlight brightness adjustment */
> > > + if (drm_dp_dpcd_readb(&intel_dp->aux, DP_EDP_GENERAL_CAP_3,
> >
> > Spec says this register is new to eDP 1.4. I don't see a check for
> > version.
> >
>
There is no need to check this. In not supported eDP panel, this register
will read all 0s.
> >
> > > + ®_val) != 1) {
> > > + DRM_DEBUG_KMS("Failed to read DPCD register 0x%x\n",
> > > + DP_EDP_GENERAL_CAP_3);
> > > + return false;
> > > + }
> > > + if (reg_val > 0)
> > > + return true;
> > > +
> > > + /* Panel supports backlight PWM frequency set */
> > > + if (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_FREQ_AUX_SET_CAP)
> > > + return true;
> > > +
> > > + /* Panel supports more than 8 bits resolution of brightness level
> */
> > > + if (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_
> BYTE_COUNT)
> > > + return true;
> >
> > I don't know if more than 8 bits is relatively better than the
> > resolution PWM pin allows. I guess, we could fix that later by comparing
> > the number of brightness levels.
> >
> > > +
> > > + /* Panel supports enabling backlight via AUX but not by BL_ENABLE
> pin */
> > > + if ((intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_AUX_ENABLE_CAP) &&
> > > + !(intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_PIN_ENABLE_CAP))
> > > + return true;
> > > +
> > > + return false;
> > > +
> > > +}
> > > +
> > > int intel_dp_aux_init_backlight_funcs(struct intel_connector
> *intel_connector)
> > > {
> > > struct intel_panel *panel = &intel_connector->panel;
> > > @@ -187,6 +241,10 @@ int intel_dp_aux_init_backlight_funcs(struct
> intel_connector *intel_connector)
> > > if (!intel_dp_aux_display_control_capable(intel_connector))
> > > return -ENODEV;
> > >
> > > + if (i915.enable_dpcd_backlight == -1 &&
> > > + !intel_dp_aux_display_control_heuristic(intel_connector))
> > > + return -ENODEV;
> > > +
> > > panel->backlight.setup = intel_dp_aux_setup_backlight;
> > > panel->backlight.enable = intel_dp_aux_enable_backlight;
> > > panel->backlight.disable = intel_dp_aux_disable_backlight;
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>
[-- Attachment #1.2: Type: text/html, Size: 12387 bytes --]
[-- Attachment #2: Type: text/plain, Size: 160 bytes --]
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v10 2/3] drm/i915: Add option to support dynamic backlight via DPCD
2017-05-27 1:42 [PATCH v10 0/3] Enhancement to intel_dp_aux_backlight driver Puthikorn Voravootivat
2017-05-27 1:42 ` [PATCH v10 1/3] drm/i915: Add heuristic to determine better way to adjust brightness Puthikorn Voravootivat
@ 2017-05-27 1:42 ` Puthikorn Voravootivat
2017-05-31 4:18 ` Pandiyan, Dhinakaran
2017-06-02 18:25 ` Pandiyan, Dhinakaran
2017-05-27 1:42 ` [PATCH v10 3/3] drm/i915: Set PWM divider to match desired frequency in vbt Puthikorn Voravootivat
2 siblings, 2 replies; 14+ messages in thread
From: Puthikorn Voravootivat @ 2017-05-27 1:42 UTC (permalink / raw)
To: intel-gfx, Dhinakaran Pandiyan, Jani Nikula, Daniel Vetter
Cc: Puthikorn Voravootivat, dri-devel
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,
};
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);
}
--
2.13.0.219.gdb65acc882-goog
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH v10 2/3] drm/i915: Add option to support dynamic backlight via DPCD
2017-05-27 1:42 ` [PATCH v10 2/3] drm/i915: Add option to support dynamic backlight via DPCD Puthikorn Voravootivat
@ 2017-05-31 4:18 ` Pandiyan, Dhinakaran
2017-05-31 21:37 ` Puthikorn Voravootivat
2017-06-02 18:25 ` Pandiyan, Dhinakaran
1 sibling, 1 reply; 14+ messages in thread
From: Pandiyan, Dhinakaran @ 2017-05-31 4:18 UTC (permalink / raw)
To: puthik@chromium.org
Cc: dri-devel@lists.freedesktop.org, intel-gfx@lists.freedesktop.org
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?
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.
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
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH v10 2/3] drm/i915: Add option to support dynamic backlight via DPCD
2017-05-31 4:18 ` Pandiyan, Dhinakaran
@ 2017-05-31 21:37 ` Puthikorn Voravootivat
2017-05-31 21:52 ` Pandiyan, Dhinakaran
0 siblings, 1 reply; 14+ messages in thread
From: Puthikorn Voravootivat @ 2017-05-31 21:37 UTC (permalink / raw)
To: Pandiyan, Dhinakaran
Cc: puthik@chromium.org, intel-gfx@lists.freedesktop.org,
dri-devel@lists.freedesktop.org
[-- Attachment #1.1: Type: text/plain, Size: 6786 bytes --]
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.
>
> 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);
> > }
>
>
[-- Attachment #1.2: Type: text/html, Size: 8878 bytes --]
[-- Attachment #2: Type: text/plain, Size: 160 bytes --]
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH v10 2/3] drm/i915: Add option to support dynamic backlight via DPCD
2017-05-31 21:37 ` Puthikorn Voravootivat
@ 2017-05-31 21:52 ` Pandiyan, Dhinakaran
0 siblings, 0 replies; 14+ messages in thread
From: Pandiyan, Dhinakaran @ 2017-05-31 21:52 UTC (permalink / raw)
To: puthik@chromium.org
Cc: daniel.vetter@ffwll.ch, intel-gfx@lists.freedesktop.org,
dri-devel@lists.freedesktop.org
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v10 2/3] drm/i915: Add option to support dynamic backlight via DPCD
2017-05-27 1:42 ` [PATCH v10 2/3] drm/i915: Add option to support dynamic backlight via DPCD Puthikorn Voravootivat
2017-05-31 4:18 ` Pandiyan, Dhinakaran
@ 2017-06-02 18:25 ` Pandiyan, Dhinakaran
2017-06-03 1:56 ` Puthikorn Voravootivat
1 sibling, 1 reply; 14+ messages in thread
From: Pandiyan, Dhinakaran @ 2017-06-02 18:25 UTC (permalink / raw)
To: puthik@chromium.org
Cc: dri-devel@lists.freedesktop.org, intel-gfx@lists.freedesktop.org
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,
> };
>
> 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");
> + }
> +
Just curious, does DBC require DP AUX brightness control?
-DK
> + 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
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH v10 2/3] drm/i915: Add option to support dynamic backlight via DPCD
2017-06-02 18:25 ` Pandiyan, Dhinakaran
@ 2017-06-03 1:56 ` Puthikorn Voravootivat
0 siblings, 0 replies; 14+ messages in thread
From: Puthikorn Voravootivat @ 2017-06-03 1:56 UTC (permalink / raw)
To: Pandiyan, Dhinakaran
Cc: puthik@chromium.org, intel-gfx@lists.freedesktop.org,
dri-devel@lists.freedesktop.org
[-- Attachment #1.1: Type: text/plain, Size: 5982 bytes --]
On Fri, Jun 2, 2017 at 11:25 AM, 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,
> > };
> >
> > 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");
> > + }
> > +
>
>
> Just curious, does DBC require DP AUX brightness control?
>
> No. Here is quote from the spec.
"The Dynamic Backlight Control (DBC) and color engine modes are not listed
in Table 10-1,
because when they are available, they are available for all supported
modes."
> -DK
>
> > + 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);
> > }
>
>
[-- Attachment #1.2: Type: text/html, Size: 8151 bytes --]
[-- Attachment #2: Type: text/plain, Size: 160 bytes --]
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v10 3/3] drm/i915: Set PWM divider to match desired frequency in vbt
2017-05-27 1:42 [PATCH v10 0/3] Enhancement to intel_dp_aux_backlight driver Puthikorn Voravootivat
2017-05-27 1:42 ` [PATCH v10 1/3] drm/i915: Add heuristic to determine better way to adjust brightness Puthikorn Voravootivat
2017-05-27 1:42 ` [PATCH v10 2/3] drm/i915: Add option to support dynamic backlight via DPCD Puthikorn Voravootivat
@ 2017-05-27 1:42 ` Puthikorn Voravootivat
2017-05-31 3:40 ` Pandiyan, Dhinakaran
2 siblings, 1 reply; 14+ messages in thread
From: Puthikorn Voravootivat @ 2017-05-27 1:42 UTC (permalink / raw)
To: intel-gfx, Dhinakaran Pandiyan, Jani Nikula, Daniel Vetter
Cc: Puthikorn Voravootivat, dri-devel
Read desired PWM frequency from panel vbt and calculate the
value for divider in DPCD address 0x724 and 0x728 to have
as many bits as possible for PWM duty cyle for granularity of
brightness adjustment while the frequency divisor is still
within 25% of the desired value.
Signed-off-by: Puthikorn Voravootivat <puthik@chromium.org>
---
drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 79 +++++++++++++++++++++++++++
1 file changed, 79 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
index f55af41ce3bd..a8d485a29f29 100644
--- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
+++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
@@ -116,6 +116,81 @@ intel_dp_aux_set_dynamic_backlight_percent(struct intel_dp *intel_dp,
}
}
+/*
+ * Set PWM Frequency divider to match desired frequency in vbt.
+ * The PWM Frequency is calculated as 27Mhz / (F x P).
+ * - Where F = PWM Frequency Pre-Divider value programmed by field 7:0 of the
+ * EDP_BACKLIGHT_FREQ_SET register (DPCD Address 00728h)
+ * - Where P = 2^Pn, where Pn is the value programmed by field 4:0 of the
+ * EDP_PWMGEN_BIT_COUNT register (DPCD Address 00724h)
+ */
+static int intel_dp_aux_set_pwm_freq(struct intel_connector *connector)
+{
+ struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
+ struct intel_dp *intel_dp = enc_to_intel_dp(&connector->encoder->base);
+ int freq, fxp, fxp_min, fxp_max, fxp_actual, f = 1;
+ u8 pn, pn_min, pn_max;
+
+ /* Find desired value of (F x P)
+ * Note that, if F x P is out of supported range, the maximum value or
+ * minimum value will applied automatically. So no need to check that.
+ */
+ freq = dev_priv->vbt.backlight.pwm_freq_hz;
+ DRM_DEBUG_KMS("VBT defined backlight frequency %u Hz\n", freq);
+ if (!freq) {
+ DRM_DEBUG_KMS("Use panel default backlight frequency\n");
+ return -1;
+ }
+
+ fxp = DIV_ROUND_CLOSEST(KHz(DP_EDP_BACKLIGHT_FREQ_BASE_KHZ), freq);
+
+ /* Use highest possible value of Pn for more granularity of brightness
+ * adjustment while satifying the conditions below.
+ * - Pn is in the range of Pn_min and Pn_max
+ * - F is in the range of 1 and 255
+ * - FxP is within 25% of desired value.
+ * Note: 25% is arbitrary value and may need some tweak.
+ */
+ if (drm_dp_dpcd_readb(&intel_dp->aux,
+ DP_EDP_PWMGEN_BIT_COUNT_CAP_MIN, &pn_min) != 1) {
+ DRM_DEBUG_KMS("Failed to read pwmgen bit count cap min\n");
+ return -EIO;
+ }
+ if (drm_dp_dpcd_readb(&intel_dp->aux,
+ DP_EDP_PWMGEN_BIT_COUNT_CAP_MAX, &pn_max) != 1) {
+ DRM_DEBUG_KMS("Failed to read pwmgen bit count cap max\n");
+ return -EIO;
+ }
+ pn_min &= DP_EDP_PWMGEN_BIT_COUNT_MASK;
+ pn_max &= DP_EDP_PWMGEN_BIT_COUNT_MASK;
+
+ fxp_min = DIV_ROUND_CLOSEST(fxp * 3, 4);
+ fxp_max = DIV_ROUND_CLOSEST(fxp * 5, 4);
+ if (fxp_min < (1 << pn_min) || (255 << pn_max) < fxp_max) {
+ DRM_DEBUG_KMS("VBT defined backlight frequency out of range\n");
+ return -ERANGE;
+ }
+
+ for (pn = pn_max; pn >= pn_min; pn--) {
+ f = clamp(DIV_ROUND_CLOSEST(fxp, 1 << pn), 1, 255);
+ fxp_actual = f << pn;
+ if (fxp_min <= fxp_actual && fxp_actual <= fxp_max)
+ break;
+ }
+
+ if (drm_dp_dpcd_writeb(&intel_dp->aux,
+ DP_EDP_PWMGEN_BIT_COUNT, pn) < 0) {
+ DRM_DEBUG_KMS("Failed to write aux pwmgen bit count\n");
+ return -EIO;
+ }
+ if (drm_dp_dpcd_writeb(&intel_dp->aux,
+ DP_EDP_BACKLIGHT_FREQ_SET, (u8) f) < 0) {
+ DRM_DEBUG_KMS("Failed to write aux backlight freq\n");
+ return -EIO;
+ }
+ return 0;
+}
+
static void intel_dp_aux_enable_backlight(struct intel_connector *connector)
{
struct intel_dp *intel_dp = enc_to_intel_dp(&connector->encoder->base);
@@ -152,6 +227,10 @@ static void intel_dp_aux_enable_backlight(struct intel_connector *connector)
DRM_DEBUG_KMS("Enable dynamic brightness.\n");
}
+ if (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_FREQ_AUX_SET_CAP)
+ if (!intel_dp_aux_set_pwm_freq(connector))
+ new_dpcd_buf |= DP_EDP_BACKLIGHT_FREQ_AUX_SET_ENABLE;
+
if (new_dpcd_buf != dpcd_buf) {
if (drm_dp_dpcd_writeb(&intel_dp->aux,
DP_EDP_BACKLIGHT_MODE_SET_REGISTER, new_dpcd_buf) < 0) {
--
2.13.0.219.gdb65acc882-goog
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH v10 3/3] drm/i915: Set PWM divider to match desired frequency in vbt
2017-05-27 1:42 ` [PATCH v10 3/3] drm/i915: Set PWM divider to match desired frequency in vbt Puthikorn Voravootivat
@ 2017-05-31 3:40 ` Pandiyan, Dhinakaran
2017-05-31 21:44 ` Puthikorn Voravootivat
0 siblings, 1 reply; 14+ messages in thread
From: Pandiyan, Dhinakaran @ 2017-05-31 3:40 UTC (permalink / raw)
To: puthik@chromium.org
Cc: dri-devel@lists.freedesktop.org, intel-gfx@lists.freedesktop.org
The patch looks good overall, it would have been easier to merge if
you'd sent this as the first patch in this version. Some comments
inline.
On Fri, 2017-05-26 at 18:42 -0700, Puthikorn Voravootivat wrote:
> Read desired PWM frequency from panel vbt and calculate the
> value for divider in DPCD address 0x724 and 0x728 to have
> as many bits as possible for PWM duty cyle for granularity of
> brightness adjustment while the frequency divisor is still
> within 25% of the desired value.
>
> Signed-off-by: Puthikorn Voravootivat <puthik@chromium.org>
> ---
> drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 79 +++++++++++++++++++++++++++
> 1 file changed, 79 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> index f55af41ce3bd..a8d485a29f29 100644
> --- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> +++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> @@ -116,6 +116,81 @@ intel_dp_aux_set_dynamic_backlight_percent(struct intel_dp *intel_dp,
> }
> }
>
> +/*
> + * Set PWM Frequency divider to match desired frequency in vbt.
> + * The PWM Frequency is calculated as 27Mhz / (F x P).
> + * - Where F = PWM Frequency Pre-Divider value programmed by field 7:0 of the
> + * EDP_BACKLIGHT_FREQ_SET register (DPCD Address 00728h)
> + * - Where P = 2^Pn, where Pn is the value programmed by field 4:0 of the
> + * EDP_PWMGEN_BIT_COUNT register (DPCD Address 00724h)
> + */
> +static int intel_dp_aux_set_pwm_freq(struct intel_connector *connector)
> +{
> + struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> + struct intel_dp *intel_dp = enc_to_intel_dp(&connector->encoder->base);
> + int freq, fxp, fxp_min, fxp_max, fxp_actual, f = 1;
nit: unnecessary initialization for f?
> + u8 pn, pn_min, pn_max;
> +
> + /* Find desired value of (F x P)
> + * Note that, if F x P is out of supported range, the maximum value or
> + * minimum value will applied automatically. So no need to check that.
> + */
> + freq = dev_priv->vbt.backlight.pwm_freq_hz;
> + DRM_DEBUG_KMS("VBT defined backlight frequency %u Hz\n", freq);
> + if (!freq) {
> + DRM_DEBUG_KMS("Use panel default backlight frequency\n");
> + return -1;
> + }
> +
> + fxp = DIV_ROUND_CLOSEST(KHz(DP_EDP_BACKLIGHT_FREQ_BASE_KHZ), freq);
> +
> + /* Use highest possible value of Pn for more granularity of brightness
> + * adjustment while satifying the conditions below.
> + * - Pn is in the range of Pn_min and Pn_max
> + * - F is in the range of 1 and 255
> + * - FxP is within 25% of desired value.
> + * Note: 25% is arbitrary value and may need some tweak.
> + */
> + if (drm_dp_dpcd_readb(&intel_dp->aux,
> + DP_EDP_PWMGEN_BIT_COUNT_CAP_MIN, &pn_min) != 1) {
> + DRM_DEBUG_KMS("Failed to read pwmgen bit count cap min\n");
> + return -EIO;
The error numbers are not propagated outside, so I don't see a real need
for them. bool should suffice.
> + }
> + if (drm_dp_dpcd_readb(&intel_dp->aux,
> + DP_EDP_PWMGEN_BIT_COUNT_CAP_MAX, &pn_max) != 1) {
> + DRM_DEBUG_KMS("Failed to read pwmgen bit count cap max\n");
> + return -EIO;
> + }
> + pn_min &= DP_EDP_PWMGEN_BIT_COUNT_MASK;
> + pn_max &= DP_EDP_PWMGEN_BIT_COUNT_MASK;
> +
> + fxp_min = DIV_ROUND_CLOSEST(fxp * 3, 4);
> + fxp_max = DIV_ROUND_CLOSEST(fxp * 5, 4);
> + if (fxp_min < (1 << pn_min) || (255 << pn_max) < fxp_max) {
> + DRM_DEBUG_KMS("VBT defined backlight frequency out of range\n");
> + return -ERANGE;
> + }
> +
> + for (pn = pn_max; pn >= pn_min; pn--) {
> + f = clamp(DIV_ROUND_CLOSEST(fxp, 1 << pn), 1, 255);
> + fxp_actual = f << pn;
> + if (fxp_min <= fxp_actual && fxp_actual <= fxp_max)
> + break;
> + }
> +
> + if (drm_dp_dpcd_writeb(&intel_dp->aux,
> + DP_EDP_PWMGEN_BIT_COUNT, pn) < 0) {
> + DRM_DEBUG_KMS("Failed to write aux pwmgen bit count\n");
> + return -EIO;
> + }
> + if (drm_dp_dpcd_writeb(&intel_dp->aux,
> + DP_EDP_BACKLIGHT_FREQ_SET, (u8) f) < 0) {
> + DRM_DEBUG_KMS("Failed to write aux backlight freq\n");
> + return -EIO;
> + }
> + return 0;
> +}
> +
> static void intel_dp_aux_enable_backlight(struct intel_connector *connector)
> {
> struct intel_dp *intel_dp = enc_to_intel_dp(&connector->encoder->base);
> @@ -152,6 +227,10 @@ static void intel_dp_aux_enable_backlight(struct intel_connector *connector)
> DRM_DEBUG_KMS("Enable dynamic brightness.\n");
> }
>
> + if (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_FREQ_AUX_SET_CAP)
> + if (!intel_dp_aux_set_pwm_freq(connector))
nit: This doesn't read right to me, looking at the function name it
seems like you are proceeding when it failed.
> + new_dpcd_buf |= DP_EDP_BACKLIGHT_FREQ_AUX_SET_ENABLE;
> +
> if (new_dpcd_buf != dpcd_buf) {
> if (drm_dp_dpcd_writeb(&intel_dp->aux,
> DP_EDP_BACKLIGHT_MODE_SET_REGISTER, new_dpcd_buf) < 0) {
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH v10 3/3] drm/i915: Set PWM divider to match desired frequency in vbt
2017-05-31 3:40 ` Pandiyan, Dhinakaran
@ 2017-05-31 21:44 ` Puthikorn Voravootivat
0 siblings, 0 replies; 14+ messages in thread
From: Puthikorn Voravootivat @ 2017-05-31 21:44 UTC (permalink / raw)
To: Pandiyan, Dhinakaran
Cc: puthik@chromium.org, intel-gfx@lists.freedesktop.org,
dri-devel@lists.freedesktop.org
[-- Attachment #1.1: Type: text/plain, Size: 5959 bytes --]
On Tue, May 30, 2017 at 8:40 PM, Pandiyan, Dhinakaran <
dhinakaran.pandiyan@intel.com> wrote:
> The patch looks good overall, it would have been easier to merge if
> you'd sent this as the first patch in this version. Some comments
> inline.
>
>
> Will re-order to make this the first patch in the next version.
>
> On Fri, 2017-05-26 at 18:42 -0700, Puthikorn Voravootivat wrote:
> > Read desired PWM frequency from panel vbt and calculate the
> > value for divider in DPCD address 0x724 and 0x728 to have
> > as many bits as possible for PWM duty cyle for granularity of
> > brightness adjustment while the frequency divisor is still
> > within 25% of the desired value.
> >
> > Signed-off-by: Puthikorn Voravootivat <puthik@chromium.org>
> > ---
> > drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 79
> +++++++++++++++++++++++++++
> > 1 file changed, 79 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> > index f55af41ce3bd..a8d485a29f29 100644
> > --- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> > +++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> > @@ -116,6 +116,81 @@ intel_dp_aux_set_dynamic_backlight_percent(struct
> intel_dp *intel_dp,
> > }
> > }
> >
> > +/*
> > + * Set PWM Frequency divider to match desired frequency in vbt.
> > + * The PWM Frequency is calculated as 27Mhz / (F x P).
> > + * - Where F = PWM Frequency Pre-Divider value programmed by field 7:0
> of the
> > + * EDP_BACKLIGHT_FREQ_SET register (DPCD Address 00728h)
> > + * - Where P = 2^Pn, where Pn is the value programmed by field 4:0 of
> the
> > + * EDP_PWMGEN_BIT_COUNT register (DPCD Address 00724h)
> > + */
> > +static int intel_dp_aux_set_pwm_freq(struct intel_connector *connector)
> > +{
> > + struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> > + struct intel_dp *intel_dp = enc_to_intel_dp(&connector->
> encoder->base);
> > + int freq, fxp, fxp_min, fxp_max, fxp_actual, f = 1;
>
> nit: unnecessary initialization for f?
>
No. We only set f in for (pn = pn_max; pn >= pn_min; pn--) but compiler
wouldn't
know that pn_max >= pn_min so f might be uninitialized.
> > + u8 pn, pn_min, pn_max;
> > +
> > + /* Find desired value of (F x P)
> > + * Note that, if F x P is out of supported range, the maximum
> value or
> > + * minimum value will applied automatically. So no need to check
> that.
> > + */
> > + freq = dev_priv->vbt.backlight.pwm_freq_hz;
> > + DRM_DEBUG_KMS("VBT defined backlight frequency %u Hz\n", freq);
> > + if (!freq) {
> > + DRM_DEBUG_KMS("Use panel default backlight frequency\n");
> > + return -1;
> > + }
> > +
> > + fxp = DIV_ROUND_CLOSEST(KHz(DP_EDP_BACKLIGHT_FREQ_BASE_KHZ),
> freq);
> > +
> > + /* Use highest possible value of Pn for more granularity of
> brightness
> > + * adjustment while satifying the conditions below.
> > + * - Pn is in the range of Pn_min and Pn_max
> > + * - F is in the range of 1 and 255
> > + * - FxP is within 25% of desired value.
> > + * Note: 25% is arbitrary value and may need some tweak.
> > + */
> > + if (drm_dp_dpcd_readb(&intel_dp->aux,
> > + DP_EDP_PWMGEN_BIT_COUNT_CAP_MIN, &pn_min)
> != 1) {
> > + DRM_DEBUG_KMS("Failed to read pwmgen bit count cap min\n");
> > + return -EIO;
>
> The error numbers are not propagated outside, so I don't see a real need
> for them. bool should suffice.
>
> Sure.
>
> > + }
> > + if (drm_dp_dpcd_readb(&intel_dp->aux,
> > + DP_EDP_PWMGEN_BIT_COUNT_CAP_MAX, &pn_max)
> != 1) {
> > + DRM_DEBUG_KMS("Failed to read pwmgen bit count cap max\n");
> > + return -EIO;
> > + }
> > + pn_min &= DP_EDP_PWMGEN_BIT_COUNT_MASK;
> > + pn_max &= DP_EDP_PWMGEN_BIT_COUNT_MASK;
> > +
> > + fxp_min = DIV_ROUND_CLOSEST(fxp * 3, 4);
> > + fxp_max = DIV_ROUND_CLOSEST(fxp * 5, 4);
> > + if (fxp_min < (1 << pn_min) || (255 << pn_max) < fxp_max) {
> > + DRM_DEBUG_KMS("VBT defined backlight frequency out of
> range\n");
> > + return -ERANGE;
> > + }
> > +
> > + for (pn = pn_max; pn >= pn_min; pn--) {
> > + f = clamp(DIV_ROUND_CLOSEST(fxp, 1 << pn), 1, 255);
> > + fxp_actual = f << pn;
> > + if (fxp_min <= fxp_actual && fxp_actual <= fxp_max)
> > + break;
> > + }
> > +
> > + if (drm_dp_dpcd_writeb(&intel_dp->aux,
> > + DP_EDP_PWMGEN_BIT_COUNT, pn) < 0) {
> > + DRM_DEBUG_KMS("Failed to write aux pwmgen bit count\n");
> > + return -EIO;
> > + }
> > + if (drm_dp_dpcd_writeb(&intel_dp->aux,
> > + DP_EDP_BACKLIGHT_FREQ_SET, (u8) f) < 0) {
> > + DRM_DEBUG_KMS("Failed to write aux backlight freq\n");
> > + return -EIO;
> > + }
> > + return 0;
> > +}
> > +
> > static void intel_dp_aux_enable_backlight(struct intel_connector
> *connector)
> > {
> > struct intel_dp *intel_dp = enc_to_intel_dp(&connector->
> encoder->base);
> > @@ -152,6 +227,10 @@ static void intel_dp_aux_enable_backlight(struct
> intel_connector *connector)
> > DRM_DEBUG_KMS("Enable dynamic brightness.\n");
> > }
> >
> > + if (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_FREQ_AUX_SET_CAP)
> > + if (!intel_dp_aux_set_pwm_freq(connector))
>
> nit: This doesn't read right to me, looking at the function name it
> seems like you are proceeding when it failed.
>
> OK.
>
> > + new_dpcd_buf |= DP_EDP_BACKLIGHT_FREQ_AUX_SET_
> ENABLE;
> > +
> > if (new_dpcd_buf != dpcd_buf) {
> > if (drm_dp_dpcd_writeb(&intel_dp->aux,
> > DP_EDP_BACKLIGHT_MODE_SET_REGISTER, new_dpcd_buf)
> < 0) {
>
>
[-- Attachment #1.2: Type: text/html, Size: 8823 bytes --]
[-- Attachment #2: Type: text/plain, Size: 160 bytes --]
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 14+ messages in thread