[PATCH v7 0/9] Enhancement to intel_dp_aux_backlight driver

[PATCH v7 0/9] Enhancement to intel_dp_aux_backlight driver

[Puthikorn Voravootivat]

This patch set contain 9 patches.
- First five patches fix bug in the driver and allow choosing which
  way to adjust brightness if both PWM pin and AUX are supported
- Next patch adds enable DBC by default
- Next patch makes the driver restore last brightness level after
  turning display off and on.
- Last two patches set the PWM freqency to match data in panel vbt.

This patch set is reviewed by Dhinakaran in patch #1, 2, 5, 7, 8

Change log:
v7:
- Add check in intel_dp_pwm_pin_display_control_capable in patch #4
- Add option in patch #6 to enable DPCD or not
- Change definition in patch #8 and implementation in #9 to use Khz
- Fix compiler warning from build bot in patch #9

v6:
- Address review from Dhinakaran
- Make PWM frequency to have highest value of Pn that make the
  frequency still within 25% of desired frequency.

v5:
- Split first patch in v4 to 3 patches
- Bump priority for "Correctly enable backlight brightness adjustment via DPCD"
- Make logic clearer for the case that both PWM pin and AUX are supported
- Add more log when write to register fail
- Add log when enable DBC

v4:
- Rebase / minor typo fix.

v3:
- Add new implementation of PWM frequency patch

v2:
- Drop PWM frequency patch
- Address suggestion from Jani Nikula


Puthikorn Voravootivat (9):
  drm/i915: Fix cap check for intel_dp_aux_backlight driver
  drm/i915: Correctly enable backlight brightness adjustment via DPCD
  drm/i915: Drop AUX backlight enable check for backlight control
  drm/i915: Allow choosing how to adjust brightness if both supported
  drm/i915: Set backlight mode before enable backlight
  drm/i915: Add option to support dynamic backlight via DPCD
  drm/i915: Restore brightness level in aux backlight driver
  drm: Add definition for eDP backlight frequency
  drm/i915: Set PWM divider to match desired frequency in vbt

 drivers/gpu/drm/i915/i915_params.c            |  13 +-
 drivers/gpu/drm/i915/i915_params.h            |   5 +-
 drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 173 ++++++++++++++++++++++++--
 include/drm/drm_dp_helper.h                   |   2 +
 4 files changed, 176 insertions(+), 17 deletions(-)

-- 
2.13.0.rc2.291.g57267f2277-goog

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

[PATCH v7 1/9] drm/i915: Fix cap check for intel_dp_aux_backlight driver

[Puthikorn Voravootivat]

intel_dp_aux_backlight driver should check for the
DP_EDP_BACKLIGHT_BRIGHTNESS_AUX_SET_CAP before enable the driver.

Signed-off-by: Puthikorn Voravootivat <puthik@chromium.org>
Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
 drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
index 6532e226db29..341bf2cb0c25 100644
--- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
+++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
@@ -144,6 +144,7 @@ intel_dp_aux_display_control_capable(struct intel_connector *connector)
 	 */
 	if (intel_dp->edp_dpcd[1] & DP_EDP_TCON_BACKLIGHT_ADJUSTMENT_CAP &&
 	    (intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_AUX_ENABLE_CAP) &&
+	    (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_AUX_SET_CAP) &&
 	    !((intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_PIN_ENABLE_CAP) ||
 	      (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_PWM_PIN_CAP))) {
 		DRM_DEBUG_KMS("AUX Backlight Control Supported!\n");
-- 
2.13.0.rc2.291.g57267f2277-goog

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

[PATCH v7 2/9] drm/i915: Correctly enable backlight brightness adjustment via DPCD

[Puthikorn Voravootivat]

intel_dp_aux_enable_backlight() assumed that the register
BACKLIGHT_BRIGHTNESS_CONTROL_MODE can only has value 01
(DP_EDP_BACKLIGHT_CONTROL_MODE_PRESET) when initialize.

This patch fixed that by handling all cases of that register.

Signed-off-by: Puthikorn Voravootivat <puthik@chromium.org>
Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
 drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 33 ++++++++++++++++++++++-----
 1 file changed, 27 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
index 341bf2cb0c25..870c03fc0f3a 100644
--- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
+++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
@@ -97,15 +97,36 @@ 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;
 
 	set_aux_backlight_enable(intel_dp, true);
 
-	if ((drm_dp_dpcd_readb(&intel_dp->aux,
-			       DP_EDP_BACKLIGHT_MODE_SET_REGISTER, &dpcd_buf) == 1) &&
-	    ((dpcd_buf & DP_EDP_BACKLIGHT_CONTROL_MODE_MASK) ==
-	     DP_EDP_BACKLIGHT_CONTROL_MODE_PRESET))
-		drm_dp_dpcd_writeb(&intel_dp->aux, DP_EDP_BACKLIGHT_MODE_SET_REGISTER,
-				   (dpcd_buf | DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD));
+	if (drm_dp_dpcd_readb(&intel_dp->aux,
+			DP_EDP_BACKLIGHT_MODE_SET_REGISTER, &dpcd_buf) != 1) {
+		DRM_DEBUG_KMS("Failed to read DPCD register 0x%x\n",
+			      DP_EDP_BACKLIGHT_MODE_SET_REGISTER);
+		return;
+	}
+
+	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");
+		}
+		break;
+
+	/* Do nothing when it is already DPCD mode */
+	case DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD:
+	default:
+		break;
+	}
 }
 
 static void intel_dp_aux_disable_backlight(struct intel_connector *connector)
-- 
2.13.0.rc2.291.g57267f2277-goog

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

[PATCH v7 3/9] drm/i915: Drop AUX backlight enable check for backlight control

[Puthikorn Voravootivat]

There are some panel that
(1) does not support display backlight enable via AUX
(2) support display backlight adjustment via AUX
(3) support display backlight enable via eDP BL_ENABLE pin

The current driver required that (1) must be support to enable (2).
This patch drops that requirement.

Signed-off-by: Puthikorn Voravootivat <puthik@chromium.org>
---
 drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
index 870c03fc0f3a..c22712762957 100644
--- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
+++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
@@ -28,6 +28,10 @@ static void set_aux_backlight_enable(struct intel_dp *intel_dp, bool enable)
 {
 	uint8_t reg_val = 0;
 
+       /* Early return when display use other mechanism to enable backlight. */
+	if (!(intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_AUX_ENABLE_CAP))
+		return;
+
 	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_EDP_DISPLAY_CONTROL_REGISTER,
 			      &reg_val) < 0) {
 		DRM_DEBUG_KMS("Failed to read DPCD register 0x%x\n",
@@ -164,7 +168,6 @@ intel_dp_aux_display_control_capable(struct intel_connector *connector)
 	 * 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[1] & DP_EDP_BACKLIGHT_AUX_ENABLE_CAP) &&
 	    (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_AUX_SET_CAP) &&
 	    !((intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_PIN_ENABLE_CAP) ||
 	      (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_PWM_PIN_CAP))) {
-- 
2.13.0.rc2.291.g57267f2277-goog

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

Re: [PATCH v7 3/9] drm/i915: Drop AUX backlight enable check for backlight control

[Pandiyan, Dhinakaran]

On Thu, 2017-05-11 at 16:02 -0700, Puthikorn Voravootivat wrote:
> There are some panel that
> (1) does not support display backlight enable via AUX
> (2) support display backlight adjustment via AUX
> (3) support display backlight enable via eDP BL_ENABLE pin
> 
> The current driver required that (1) must be support to enable (2).
> This patch drops that requirement.
> 

You sent this version before I finished my follow-up questions, copying
the conversation here for context. 


DK: Won't DP_EDP_BACKLIGHT_AUX_ENABLE_CAP be 1 always? The code below,
in
intel_dp_aux_display_control_capable(), makes sure
DP_EDP_BACKLIGHT_PIN_ENABLE_CAP=0. The spec says at least one of these
has to be 1.

Puthikorn: We will drop the  DP_EDP_BACKLIGHT_PIN_ENABLE_CAP != 0 check
in next patch set.
This patch adds check here to prepare for that.


1) So, this patch does not really fix what the commit message claims
because it is dependent on the following patch. Does it make sense to
remove this check in this patch? That way, this patch by itself is the
fix that the commit message says.

-           !((intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_PIN_ENABLE_CAP) 


2) If a panel supports backlight enable via AUX and BL_ENABLE pin, this
patch (along with the next) enables backlight twice, doesn't it?
_intel_edp_backlight_on(intel_dp) in intel_dp.c is called
unconditionally after intel_dp_aux_enable_backlight(). I don't know how
likely this configuration is or if it's alright to enable via both AUX
and BL_ENABLE pin.




> Signed-off-by: Puthikorn Voravootivat <puthik@chromium.org>
> ---
>  drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> index 870c03fc0f3a..c22712762957 100644
> --- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> +++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> @@ -28,6 +28,10 @@ static void set_aux_backlight_enable(struct intel_dp *intel_dp, bool enable)
>  {
>  	uint8_t reg_val = 0;
>  
> +       /* Early return when display use other mechanism to enable backlight. */
> +	if (!(intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_AUX_ENABLE_CAP))
> +		return;
> +
>  	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_EDP_DISPLAY_CONTROL_REGISTER,
>  			      &reg_val) < 0) {
>  		DRM_DEBUG_KMS("Failed to read DPCD register 0x%x\n",
> @@ -164,7 +168,6 @@ intel_dp_aux_display_control_capable(struct intel_connector *connector)
>  	 * 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[1] & DP_EDP_BACKLIGHT_AUX_ENABLE_CAP) &&
>  	    (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_AUX_SET_CAP) &&
>  	    !((intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_PIN_ENABLE_CAP) ||
>  	      (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_PWM_PIN_CAP))) {

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

Re: [PATCH v7 3/9] drm/i915: Drop AUX backlight enable check for backlight control

[Jani Nikula]

On Fri, 12 May 2017, "Pandiyan, Dhinakaran" <dhinakaran.pandiyan@intel.com> wrote:
> On Thu, 2017-05-11 at 16:02 -0700, Puthikorn Voravootivat wrote:
>> There are some panel that
>> (1) does not support display backlight enable via AUX
>> (2) support display backlight adjustment via AUX
>> (3) support display backlight enable via eDP BL_ENABLE pin
>> 
>> The current driver required that (1) must be support to enable (2).
>> This patch drops that requirement.
>> 
>
> You sent this version before I finished my follow-up questions, copying
> the conversation here for context. 

Puthikorn, please don't send new versions before the review is
addressed.

Pushed patches 1, 2, 5, and 7. Thanks for the patches and review.

BR,
Jani.

> DK: Won't DP_EDP_BACKLIGHT_AUX_ENABLE_CAP be 1 always? The code below,
> in
> intel_dp_aux_display_control_capable(), makes sure
> DP_EDP_BACKLIGHT_PIN_ENABLE_CAP=0. The spec says at least one of these
> has to be 1.
>
> Puthikorn: We will drop the  DP_EDP_BACKLIGHT_PIN_ENABLE_CAP != 0 check
> in next patch set.
> This patch adds check here to prepare for that.
>
>
> 1) So, this patch does not really fix what the commit message claims
> because it is dependent on the following patch. Does it make sense to
> remove this check in this patch? That way, this patch by itself is the
> fix that the commit message says.
>
> -           !((intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_PIN_ENABLE_CAP) 
>
>
> 2) If a panel supports backlight enable via AUX and BL_ENABLE pin, this
> patch (along with the next) enables backlight twice, doesn't it?
> _intel_edp_backlight_on(intel_dp) in intel_dp.c is called
> unconditionally after intel_dp_aux_enable_backlight(). I don't know how
> likely this configuration is or if it's alright to enable via both AUX
> and BL_ENABLE pin.
>
>
>
>
>> Signed-off-by: Puthikorn Voravootivat <puthik@chromium.org>
>> ---
>>  drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
>> index 870c03fc0f3a..c22712762957 100644
>> --- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
>> +++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
>> @@ -28,6 +28,10 @@ static void set_aux_backlight_enable(struct intel_dp *intel_dp, bool enable)
>>  {
>>  	uint8_t reg_val = 0;
>>  
>> +       /* Early return when display use other mechanism to enable backlight. */
>> +	if (!(intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_AUX_ENABLE_CAP))
>> +		return;
>> +
>>  	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_EDP_DISPLAY_CONTROL_REGISTER,
>>  			      &reg_val) < 0) {
>>  		DRM_DEBUG_KMS("Failed to read DPCD register 0x%x\n",
>> @@ -164,7 +168,6 @@ intel_dp_aux_display_control_capable(struct intel_connector *connector)
>>  	 * 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[1] & DP_EDP_BACKLIGHT_AUX_ENABLE_CAP) &&
>>  	    (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_AUX_SET_CAP) &&
>>  	    !((intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_PIN_ENABLE_CAP) ||
>>  	      (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_PWM_PIN_CAP))) {
>

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v7 3/9] drm/i915: Drop AUX backlight enable check for backlight control

[Puthikorn Voravootivat]

[-- Attachment #1.1: Type: text/plain, Size: 3931 bytes --]

On Fri, May 12, 2017 at 6:14 AM, Jani Nikula <jani.nikula@linux.intel.com>
wrote:

> On Fri, 12 May 2017, "Pandiyan, Dhinakaran" <dhinakaran.pandiyan@intel.com>
> wrote:
> > On Thu, 2017-05-11 at 16:02 -0700, Puthikorn Voravootivat wrote:
> >> There are some panel that
> >> (1) does not support display backlight enable via AUX
> >> (2) support display backlight adjustment via AUX
> >> (3) support display backlight enable via eDP BL_ENABLE pin
> >>
> >> The current driver required that (1) must be support to enable (2).
> >> This patch drops that requirement.
> >>
> >
> > You sent this version before I finished my follow-up questions, copying
> > the conversation here for context.
>
> Puthikorn, please don't send new versions before the review is
> addressed.
>
Sorry I thought I was explained it clear enough.

>
> Pushed patches 1, 2, 5, and 7. Thanks for the patches and review.
>
> BR,
> Jani.
>
> > DK: Won't DP_EDP_BACKLIGHT_AUX_ENABLE_CAP be 1 always? The code below,
> > in
> > intel_dp_aux_display_control_capable(), makes sure
> > DP_EDP_BACKLIGHT_PIN_ENABLE_CAP=0. The spec says at least one of these
> > has to be 1.
> >
> > Puthikorn: We will drop the  DP_EDP_BACKLIGHT_PIN_ENABLE_CAP != 0 check
> > in next patch set.
> > This patch adds check here to prepare for that.
> >
> >
> > 1) So, this patch does not really fix what the commit message claims
> > because it is dependent on the following patch. Does it make sense to
> > remove this check in this patch? That way, this patch by itself is the
> > fix that the commit message says.
> >
> > -           !((intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_PIN_ENABLE_CAP)
> >
>
Sure. I can remove this here and adds it in next patch instead.

>
> > 2) If a panel supports backlight enable via AUX and BL_ENABLE pin, this
> > patch (along with the next) enables backlight twice, doesn't it?
> > _intel_edp_backlight_on(intel_dp) in intel_dp.c is called
> > unconditionally after intel_dp_aux_enable_backlight(). I don't know how
> > likely this configuration is or if it's alright to enable via both AUX
> > and BL_ENABLE pin.
> >
>
The eDP spec did not mention this case explicitly.
But it should not hurt to enable backlight twice as we want the backlight
to be enabled anyway.


> >
> >
> >
> >> Signed-off-by: Puthikorn Voravootivat <puthik@chromium.org>
> >> ---
> >>  drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 5 ++++-
> >>  1 file changed, 4 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> >> index 870c03fc0f3a..c22712762957 100644
> >> --- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> >> +++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> >> @@ -28,6 +28,10 @@ static void set_aux_backlight_enable(struct
> intel_dp *intel_dp, bool enable)
> >>  {
> >>      uint8_t reg_val = 0;
> >>
> >> +       /* Early return when display use other mechanism to enable
> backlight. */
> >> +    if (!(intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_AUX_ENABLE_CAP))
> >> +            return;
> >> +
> >>      if (drm_dp_dpcd_readb(&intel_dp->aux, DP_EDP_DISPLAY_CONTROL_
> REGISTER,
> >>                            &reg_val) < 0) {
> >>              DRM_DEBUG_KMS("Failed to read DPCD register 0x%x\n",
> >> @@ -164,7 +168,6 @@ intel_dp_aux_display_control_capable(struct
> intel_connector *connector)
> >>       * 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[1] & DP_EDP_BACKLIGHT_AUX_ENABLE_CAP) &&
> >>          (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_AUX_SET_CAP)
> &&
> >>          !((intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_PIN_ENABLE_CAP) ||
> >>            (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_PWM_PIN_CAP)))
> {
> >
>
> --
> Jani Nikula, Intel Open Source Technology Center
>

[-- Attachment #1.2: Type: text/html, Size: 5883 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

Re: [PATCH v7 3/9] drm/i915: Drop AUX backlight enable check for backlight control

[Pandiyan, Dhinakaran]

On Fri, 2017-05-12 at 11:10 -0700, Puthikorn Voravootivat wrote:
> 
> 
> On Fri, May 12, 2017 at 6:14 AM, Jani Nikula
> <jani.nikula@linux.intel.com> wrote:
>         On Fri, 12 May 2017, "Pandiyan, Dhinakaran"
>         <dhinakaran.pandiyan@intel.com> wrote:
>         > On Thu, 2017-05-11 at 16:02 -0700, Puthikorn Voravootivat
>         wrote:
>         >> There are some panel that
>         >> (1) does not support display backlight enable via AUX
>         >> (2) support display backlight adjustment via AUX
>         >> (3) support display backlight enable via eDP BL_ENABLE pin
>         >>
>         >> The current driver required that (1) must be support to
>         enable (2).
>         >> This patch drops that requirement.
>         >>
>         >
>         > You sent this version before I finished my follow-up
>         questions, copying
>         > the conversation here for context.
>         
>         Puthikorn, please don't send new versions before the review is
>         addressed.
> Sorry I thought I was explained it clear enough.  
>         
>         Pushed patches 1, 2, 5, and 7. Thanks for the patches and
>         review.
>         
>         BR,
>         Jani.
>         
>         > DK: Won't DP_EDP_BACKLIGHT_AUX_ENABLE_CAP be 1 always? The
>         code below,
>         > in
>         > intel_dp_aux_display_control_capable(), makes sure
>         > DP_EDP_BACKLIGHT_PIN_ENABLE_CAP=0. The spec says at least
>         one of these
>         > has to be 1.
>         >
>         > Puthikorn: We will drop the
>         DP_EDP_BACKLIGHT_PIN_ENABLE_CAP != 0 check
>         > in next patch set.
>         > This patch adds check here to prepare for that.
>         >
>         >
>         > 1) So, this patch does not really fix what the commit
>         message claims
>         > because it is dependent on the following patch. Does it make
>         sense to
>         > remove this check in this patch? That way, this patch by
>         itself is the
>         > fix that the commit message says.
>         >
>         > -           !((intel_dp->edp_dpcd[1] &
>         DP_EDP_BACKLIGHT_PIN_ENABLE_CAP)
>         >
>         
> Sure. I can remove this here and adds it in next patch instead.
> 
> 
>         >
>         > 2) If a panel supports backlight enable via AUX and
>         BL_ENABLE pin, this
>         > patch (along with the next) enables backlight twice, doesn't
>         it?
>         > _intel_edp_backlight_on(intel_dp) in intel_dp.c is called
>         > unconditionally after intel_dp_aux_enable_backlight(). I
>         don't know how
>         > likely this configuration is or if it's alright to enable
>         via both AUX
>         > and BL_ENABLE pin.
>         >
>         
> The eDP spec did not mention this case explicitly.
> But it should not hurt to enable backlight twice as we want the
> backlight to be enabled anyway.
>  

Hope so, at least a TODO would be a reasonable warning.

>         >
>         >
>         >
>         >> Signed-off-by: Puthikorn Voravootivat <puthik@chromium.org>
>         >> ---
>         >>  drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 5 ++++-
>         >>  1 file changed, 4 insertions(+), 1 deletion(-)
>         >>
>         >> diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
>         b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
>         >> index 870c03fc0f3a..c22712762957 100644
>         >> --- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
>         >> +++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
>         >> @@ -28,6 +28,10 @@ static void
>         set_aux_backlight_enable(struct intel_dp *intel_dp, bool
>         enable)
>         >>  {
>         >>      uint8_t reg_val = 0;
>         >>
>         >> +       /* Early return when display use other mechanism to
>         enable backlight. */
>         >> +    if (!(intel_dp->edp_dpcd[1] &
>         DP_EDP_BACKLIGHT_AUX_ENABLE_CAP))
>         >> +            return;
>         >> +
>         >>      if (drm_dp_dpcd_readb(&intel_dp->aux,
>         DP_EDP_DISPLAY_CONTROL_REGISTER,
>         >>                            &reg_val) < 0) {
>         >>              DRM_DEBUG_KMS("Failed to read DPCD register 0x
>         %x\n",
>         >> @@ -164,7 +168,6 @@
>         intel_dp_aux_display_control_capable(struct intel_connector
>         *connector)
>         >>       * 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[1] &
>         DP_EDP_BACKLIGHT_AUX_ENABLE_CAP) &&
>         >>          (intel_dp->edp_dpcd[2] &
>         DP_EDP_BACKLIGHT_BRIGHTNESS_AUX_SET_CAP) &&
>         >>          !((intel_dp->edp_dpcd[1] &
>         DP_EDP_BACKLIGHT_PIN_ENABLE_CAP) ||
>         >>            (intel_dp->edp_dpcd[2] &
>         DP_EDP_BACKLIGHT_BRIGHTNESS_PWM_PIN_CAP))) {
>         >
>         
>         
>         --
>         Jani Nikula, Intel Open Source Technology Center
>         
> 
> 

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

[PATCH v7 4/9] drm/i915: Allow choosing how to adjust brightness if both supported

[Puthikorn Voravootivat]

Add option to allow choosing how to adjust brightness if
panel supports both PWM pin and AUX channel.

Signed-off-by: Puthikorn Voravootivat <puthik@chromium.org>
---
 drivers/gpu/drm/i915/i915_params.c            |  8 +++++---
 drivers/gpu/drm/i915/i915_params.h            |  2 +-
 drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 24 +++++++++++++++++++-----
 3 files changed, 25 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
index b6a7e363d076..13cf3f1572ab 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,11 @@ 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(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:disable (default), 0:Use PWM pin if both supported, "
+	"1:Use DPCD if both 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 c22712762957..5ee1d90a3263 100644
--- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
+++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
@@ -167,21 +167,35 @@ 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[1] & DP_EDP_BACKLIGHT_PIN_ENABLE_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;
 }
 
+static bool
+intel_dp_pwm_pin_display_control_capable(struct intel_connector *connector)
+{
+	struct intel_dp *intel_dp = enc_to_intel_dp(&connector->encoder->base);
+
+	/* Check the  eDP Display control capabilities registers to determine if
+	 * the panel can support backlight control via BL_PWM_DIM eDP pin
+	 */
+	return (intel_dp->edp_dpcd[1] & DP_EDP_TCON_BACKLIGHT_ADJUSTMENT_CAP) &&
+	       (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_PWM_PIN_CAP);
+}
+
 int intel_dp_aux_init_backlight_funcs(struct intel_connector *intel_connector)
 {
 	struct intel_panel *panel = &intel_connector->panel;
 
-	if (!i915.enable_dpcd_backlight)
+	if (i915.enable_dpcd_backlight == -1)
+		return -ENODEV;
+
+	if (i915.enable_dpcd_backlight == 0 &&
+	    intel_dp_pwm_pin_display_control_capable(intel_connector))
 		return -ENODEV;
 
 	if (!intel_dp_aux_display_control_capable(intel_connector))
-- 
2.13.0.rc2.291.g57267f2277-goog

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

[PATCH v7 5/9] drm/i915: Set backlight mode before enable backlight

[Puthikorn Voravootivat]

We should set backlight mode register before set register to
enable the backlight.

Signed-off-by: Puthikorn Voravootivat <puthik@chromium.org>
Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
 drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
index 5ee1d90a3263..a72893da78d0 100644
--- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
+++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
@@ -103,8 +103,6 @@ static void intel_dp_aux_enable_backlight(struct intel_connector *connector)
 	uint8_t dpcd_buf = 0;
 	uint8_t edp_backlight_mode = 0;
 
-	set_aux_backlight_enable(intel_dp, true);
-
 	if (drm_dp_dpcd_readb(&intel_dp->aux,
 			DP_EDP_BACKLIGHT_MODE_SET_REGISTER, &dpcd_buf) != 1) {
 		DRM_DEBUG_KMS("Failed to read DPCD register 0x%x\n",
@@ -131,6 +129,8 @@ static void intel_dp_aux_enable_backlight(struct intel_connector *connector)
 	default:
 		break;
 	}
+
+	set_aux_backlight_enable(intel_dp, true);
 }
 
 static void intel_dp_aux_disable_backlight(struct intel_connector *connector)
-- 
2.13.0.rc2.291.g57267f2277-goog

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

[PATCH v7 6/9] drm/i915: Add option to support dynamic backlight via DPCD

[Puthikorn Voravootivat]

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 | 40 +++++++++++++++++++++++----
 3 files changed, 41 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
index 13cf3f1572ab..6eaf660e74da 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);
@@ -255,3 +256,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(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 a72893da78d0..1c5459bc20ae 100644
--- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
+++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
@@ -97,10 +97,27 @@ 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 new_dpcd_buf = 0;
 	uint8_t edp_backlight_mode = 0;
 
 	if (drm_dp_dpcd_readb(&intel_dp->aux,
@@ -110,18 +127,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 */
@@ -130,6 +144,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);
 }
 
-- 
2.13.0.rc2.291.g57267f2277-goog

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

Re: [PATCH v7 6/9] drm/i915: Add option to support dynamic backlight via DPCD

[Pandiyan, Dhinakaran]

On Thu, 2017-05-11 at 16:02 -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 | 40 +++++++++++++++++++++++----
>  3 files changed, 41 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
> index 13cf3f1572ab..6eaf660e74da 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,

Thanks for adding this switch, however I am not sure about doing this
via a kernel parameter. Controlling this via a drm_property is another
way. But, that would require user space changes. 

Jani, 
What are your thoughts on adding another kernel parameter. iirc there
were some concerns in the mailing list about adding new params a while
back. 


-DK


>  };
>  
>  module_param_named(modeset, i915.modeset, int, 0400);
> @@ -255,3 +256,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(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 a72893da78d0..1c5459bc20ae 100644
> --- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> +++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> @@ -97,10 +97,27 @@ 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 new_dpcd_buf = 0;
>  	uint8_t edp_backlight_mode = 0;
>  
>  	if (drm_dp_dpcd_readb(&intel_dp->aux,
> @@ -110,18 +127,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 */
> @@ -130,6 +144,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-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v7 7/9] drm/i915: Restore brightness level in aux backlight driver

[Puthikorn Voravootivat]

Some panel will default to zero brightness when turning the
panel off and on again. This patch restores last brightness
level back when panel is turning back on.

Signed-off-by: Puthikorn Voravootivat <puthik@chromium.org>
Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
 drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
index 1c5459bc20ae..0b48851013cc 100644
--- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
+++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
@@ -159,6 +159,7 @@ static void intel_dp_aux_enable_backlight(struct intel_connector *connector)
 	}
 
 	set_aux_backlight_enable(intel_dp, true);
+	intel_dp_aux_set_backlight(connector, connector->panel.backlight.level);
 }
 
 static void intel_dp_aux_disable_backlight(struct intel_connector *connector)
-- 
2.13.0.rc2.291.g57267f2277-goog

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

[PATCH v7 8/9] drm: Add definition for eDP backlight frequency

[Puthikorn Voravootivat]

This patch adds the following definition
- Bit mask for EDP_PWMGEN_BIT_COUNT and min/max cap
  register which only use bit 0:4
- Base frequency (27 MHz) for backlight PWM frequency
  generator.

Signed-off-by: Puthikorn Voravootivat <puthik@chromium.org>
Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
 include/drm/drm_dp_helper.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index c0bd0d7651a9..eaa307f6ae8c 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -572,10 +572,12 @@
 #define DP_EDP_PWMGEN_BIT_COUNT             0x724
 #define DP_EDP_PWMGEN_BIT_COUNT_CAP_MIN     0x725
 #define DP_EDP_PWMGEN_BIT_COUNT_CAP_MAX     0x726
+# define  DP_EDP_PWMGEN_BIT_COUNT_MASK      (0x1f << 0)
 
 #define DP_EDP_BACKLIGHT_CONTROL_STATUS     0x727
 
 #define DP_EDP_BACKLIGHT_FREQ_SET           0x728
+# define DP_EDP_BACKLIGHT_FREQ_BASE_KHZ     27000
 
 #define DP_EDP_BACKLIGHT_FREQ_CAP_MIN_MSB   0x72a
 #define DP_EDP_BACKLIGHT_FREQ_CAP_MIN_MID   0x72b
-- 
2.13.0.rc2.291.g57267f2277-goog

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

[PATCH v7 9/9] drm/i915: Set PWM divider to match desired frequency in vbt

[Puthikorn Voravootivat]

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 is still within 25%
of the desired frequency.

Signed-off-by: Puthikorn Voravootivat <puthik@chromium.org>
---
 drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 81 +++++++++++++++++++++++++++
 1 file changed, 81 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
index 0b48851013cc..6f10a2f1ab76 100644
--- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
+++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
@@ -113,12 +113,86 @@ 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 void 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;
+	}
+
+	fxp = 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
+	 * - Effective frequency is within 25% of desired frequency.
+	 */
+	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;
+	}
+	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;
+	}
+	pn_min &= DP_EDP_PWMGEN_BIT_COUNT_MASK;
+	pn_max &= DP_EDP_PWMGEN_BIT_COUNT_MASK;
+
+	fxp_min = fxp * 3 / 4;
+	fxp_max = 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;
+	}
+
+	for (pn = pn_max; pn > pn_min; pn--) {
+		f = clamp(fxp >> 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;
+	}
+	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;
+	}
+}
+
 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 new_dpcd_buf = 0;
 	uint8_t edp_backlight_mode = 0;
+	bool freq_cap;
 
 	if (drm_dp_dpcd_readb(&intel_dp->aux,
 			DP_EDP_BACKLIGHT_MODE_SET_REGISTER, &dpcd_buf) != 1) {
@@ -151,6 +225,10 @@ static void intel_dp_aux_enable_backlight(struct intel_connector *connector)
 		DRM_DEBUG_KMS("Enable dynamic brightness.\n");
 	}
 
+	freq_cap = intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_FREQ_AUX_SET_CAP;
+	if (freq_cap)
+		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) {
@@ -158,6 +236,9 @@ static void intel_dp_aux_enable_backlight(struct intel_connector *connector)
 		}
 	}
 
+	if (freq_cap)
+		intel_dp_aux_set_pwm_freq(connector);
+
 	set_aux_backlight_enable(intel_dp, true);
 	intel_dp_aux_set_backlight(connector, connector->panel.backlight.level);
 }
-- 
2.13.0.rc2.291.g57267f2277-goog

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

Re: [PATCH v7 9/9] drm/i915: Set PWM divider to match desired frequency in vbt

[Pandiyan, Dhinakaran]

On Thu, 2017-05-11 at 16:02 -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 is still within 25%
> of the desired frequency.

I read a few eDP panel data sheets, the PWM frequencies all start from
~200Hz. If the VBT chooses this lowest value to allow for more
brightness control, and then this patch lowers the value by another 25%,
we'll end up below the panel allowed PWM frequency. 

In fact, one of the systems I checked had PWM frequency as 200Hz in VBT
and the panel datasheet also had PWM frequency range starting from
200Hz. Have you considered this case?

-DK
> 
> Signed-off-by: Puthikorn Voravootivat <puthik@chromium.org>
> ---
>  drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 81 +++++++++++++++++++++++++++
>  1 file changed, 81 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> index 0b48851013cc..6f10a2f1ab76 100644
> --- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> +++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> @@ -113,12 +113,86 @@ 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 void 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;
> +	}
> +
> +	fxp = 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
> +	 * - Effective frequency is within 25% of desired frequency.
> +	 */
> +	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;
> +	}
> +	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;
> +	}
> +	pn_min &= DP_EDP_PWMGEN_BIT_COUNT_MASK;
> +	pn_max &= DP_EDP_PWMGEN_BIT_COUNT_MASK;
> +
> +	fxp_min = fxp * 3 / 4;
> +	fxp_max = fxp * 5 / 4;

You are allowing fxp between +/- 25% of the actual. This isn't same as
the "Effective frequency is within 25% of desired frequency." right? The
frequency can vary between -20% and +33%.


> +	if (fxp_min < (1 << pn_min) || (255 << pn_max) < fxp_max) {
> +		DRM_DEBUG_KMS("VBT defined backlight frequency out of range\n");
> +		return;
> +	}
> +
> +	for (pn = pn_max; pn > pn_min; pn--) {
> +		f = clamp(fxp >> 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;
> +	}
> +	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;
> +	}
> +}
> +
>  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 new_dpcd_buf = 0;
>  	uint8_t edp_backlight_mode = 0;
> +	bool freq_cap;
>  
>  	if (drm_dp_dpcd_readb(&intel_dp->aux,
>  			DP_EDP_BACKLIGHT_MODE_SET_REGISTER, &dpcd_buf) != 1) {
> @@ -151,6 +225,10 @@ static void intel_dp_aux_enable_backlight(struct intel_connector *connector)
>  		DRM_DEBUG_KMS("Enable dynamic brightness.\n");
>  	}
>  
> +	freq_cap = intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_FREQ_AUX_SET_CAP;
> +	if (freq_cap)
> +		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) {
> @@ -158,6 +236,9 @@ static void intel_dp_aux_enable_backlight(struct intel_connector *connector)
>  		}
>  	}
>  
> +	if (freq_cap)
> +		intel_dp_aux_set_pwm_freq(connector);
> +
>  	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

Re: [PATCH v7 9/9] drm/i915: Set PWM divider to match desired frequency in vbt

[Puthikorn Voravootivat]

[-- Attachment #1.1: Type: text/plain, Size: 6785 bytes --]

On Fri, May 12, 2017 at 5:12 PM, Pandiyan, Dhinakaran <
dhinakaran.pandiyan@intel.com> wrote:

> On Thu, 2017-05-11 at 16:02 -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 is still within 25%
> > of the desired frequency.
>
> I read a few eDP panel data sheets, the PWM frequencies all start from
> ~200Hz. If the VBT chooses this lowest value to allow for more
> brightness control, and then this patch lowers the value by another 25%,
> we'll end up below the panel allowed PWM frequency.
>
> In fact, one of the systems I checked had PWM frequency as 200Hz in VBT
> and the panel datasheet also had PWM frequency range starting from
> 200Hz. Have you considered this case?
>
> The spec said "A given LCD panel typically has a limited range of
backlight frequency capability.
To limit the programmable frequency range, limitations are placed on the
allowable total divider ratio with the Sink device"
 So I think it should be auto cap to 200Hz in this case.


> -DK
> >
> > Signed-off-by: Puthikorn Voravootivat <puthik@chromium.org>
> > ---
> >  drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 81
> +++++++++++++++++++++++++++
> >  1 file changed, 81 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> > index 0b48851013cc..6f10a2f1ab76 100644
> > --- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> > +++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> > @@ -113,12 +113,86 @@ 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 void 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;
> > +     }
> > +
> > +     fxp = 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
> > +      * - Effective frequency is within 25% of desired frequency.
> > +      */
> > +     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;
> > +     }
> > +     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;
> > +     }
> > +     pn_min &= DP_EDP_PWMGEN_BIT_COUNT_MASK;
> > +     pn_max &= DP_EDP_PWMGEN_BIT_COUNT_MASK;
> > +
> > +     fxp_min = fxp * 3 / 4;
> > +     fxp_max = fxp * 5 / 4;
>
> You are allowing fxp between +/- 25% of the actual. This isn't same as
> the "Effective frequency is within 25% of desired frequency." right? The
> frequency can vary between -20% and +33%.
>
> You are right.
You want me to change commit message to reflect this or change the code to
match the commit message?

>
> > +     if (fxp_min < (1 << pn_min) || (255 << pn_max) < fxp_max) {
> > +             DRM_DEBUG_KMS("VBT defined backlight frequency out of
> range\n");
> > +             return;
> > +     }
> > +
> > +     for (pn = pn_max; pn > pn_min; pn--) {
> > +             f = clamp(fxp >> 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;
> > +     }
> > +     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;
> > +     }
> > +}
> > +
> >  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 new_dpcd_buf = 0;
> >       uint8_t edp_backlight_mode = 0;
> > +     bool freq_cap;
> >
> >       if (drm_dp_dpcd_readb(&intel_dp->aux,
> >                       DP_EDP_BACKLIGHT_MODE_SET_REGISTER, &dpcd_buf) !=
> 1) {
> > @@ -151,6 +225,10 @@ static void intel_dp_aux_enable_backlight(struct
> intel_connector *connector)
> >               DRM_DEBUG_KMS("Enable dynamic brightness.\n");
> >       }
> >
> > +     freq_cap = intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_FREQ_AUX_SET_
> CAP;
> > +     if (freq_cap)
> > +             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) {
> > @@ -158,6 +236,9 @@ static void intel_dp_aux_enable_backlight(struct
> intel_connector *connector)
> >               }
> >       }
> >
> > +     if (freq_cap)
> > +             intel_dp_aux_set_pwm_freq(connector);
> > +
> >       set_aux_backlight_enable(intel_dp, true);
> >       intel_dp_aux_set_backlight(connector, connector->panel.backlight.
> level);
> >  }
>
>

[-- Attachment #1.2: Type: text/html, Size: 9363 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

Re: [PATCH v7 9/9] drm/i915: Set PWM divider to match desired frequency in vbt

[Pandiyan, Dhinakaran]

On Fri, 2017-05-12 at 17:31 -0700, Puthikorn Voravootivat wrote:
> 
> 
> 
> On Fri, May 12, 2017 at 5:12 PM, Pandiyan, Dhinakaran
> <dhinakaran.pandiyan@intel.com> wrote:
>         On Thu, 2017-05-11 at 16:02 -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 is still within
>         25%
>         > of the desired frequency.
>         
>         I read a few eDP panel data sheets, the PWM frequencies all
>         start from
>         ~200Hz. If the VBT chooses this lowest value to allow for more
>         brightness control, and then this patch lowers the value by
>         another 25%,
>         we'll end up below the panel allowed PWM frequency.
>         
>         In fact, one of the systems I checked had PWM frequency as
>         200Hz in VBT
>         and the panel datasheet also had PWM frequency range starting
>         from
>         200Hz. Have you considered this case?
>         
> The spec said "A given LCD panel typically has a limited range of
> backlight frequency capability. 
> To limit the programmable frequency range, limitations are placed on
> the allowable total divider ratio with the Sink device"
>  So I think it should be auto cap to 200Hz in this case.
>  
>         -DK
>         >
>         > Signed-off-by: Puthikorn Voravootivat <puthik@chromium.org>
>         > ---
>         >  drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 81
>         +++++++++++++++++++++++++++
>         >  1 file changed, 81 insertions(+)
>         >
>         > diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
>         b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
>         > index 0b48851013cc..6f10a2f1ab76 100644
>         > --- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
>         > +++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
>         > @@ -113,12 +113,86 @@
>         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 void 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;
>         > +     }
>         > +
>         > +     fxp = 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
>         > +      * - Effective frequency is within 25% of desired
>         frequency.
>         > +      */
>         > +     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;
>         > +     }
>         > +     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;
>         > +     }
>         > +     pn_min &= DP_EDP_PWMGEN_BIT_COUNT_MASK;
>         > +     pn_max &= DP_EDP_PWMGEN_BIT_COUNT_MASK;
>         > +
>         > +     fxp_min = fxp * 3 / 4;
>         > +     fxp_max = fxp * 5 / 4;
>         
>         
>         You are allowing fxp between +/- 25% of the actual. This isn't
>         same as
>         the "Effective frequency is within 25% of desired frequency."
>         right? The
>         frequency can vary between -20% and +33%.
>         
>         
> You are right.
> You want me to change commit message to reflect this or change the
> code to
> match the commit message?  

I am okay with fixing the comment and commit message. Is the 25%
arbitrary or based on the distances between the possible values? Please
make a note in the comment if it's the former.


>         > +     if (fxp_min < (1 << pn_min) || (255 << pn_max) <
>         fxp_max) {



>         > +             DRM_DEBUG_KMS("VBT defined backlight frequency
>         out of range\n");
>         > +             return;
>         > +     }
>         > +
>         > +     for (pn = pn_max; pn > pn_min; pn--) {

Is there a reason this is not pn >= pn_min?


>         > +             f = clamp(fxp >> 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");


If the number of brightness control bits are changing, the max.
brightness value changes too.

Please see intel_dp_aux_setup_backlight().



>         > +             return;
>         > +     }
>         > +     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;
>         > +     }
>         > +}
>         > +
>         >  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 new_dpcd_buf = 0;
>         >       uint8_t edp_backlight_mode = 0;
>         > +     bool freq_cap;
>         >
>         >       if (drm_dp_dpcd_readb(&intel_dp->aux,
>         >                       DP_EDP_BACKLIGHT_MODE_SET_REGISTER,
>         &dpcd_buf) != 1) {
>         > @@ -151,6 +225,10 @@ static void
>         intel_dp_aux_enable_backlight(struct intel_connector
>         *connector)
>         >               DRM_DEBUG_KMS("Enable dynamic brightness.\n");
>         >       }
>         >
>         > +     freq_cap = intel_dp->edp_dpcd[2] &
>         DP_EDP_BACKLIGHT_FREQ_AUX_SET_CAP;
>         > +     if (freq_cap)
>         > +             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) {
>         > @@ -158,6 +236,9 @@ static void
>         intel_dp_aux_enable_backlight(struct intel_connector
>         *connector)
>         >               }
>         >       }
>         >
>         > +     if (freq_cap)
>         > +             intel_dp_aux_set_pwm_freq(connector);
>         > +
>         >       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

Re: [PATCH v7 9/9] drm/i915: Set PWM divider to match desired frequency in vbt

[Puthikorn Voravootivat]

[-- Attachment #1.1: Type: text/plain, Size: 9862 bytes --]

On Mon, May 15, 2017 at 4:07 PM, Pandiyan, Dhinakaran <
dhinakaran.pandiyan@intel.com> wrote:

> On Fri, 2017-05-12 at 17:31 -0700, Puthikorn Voravootivat wrote:
> >
> >
> >
> > On Fri, May 12, 2017 at 5:12 PM, Pandiyan, Dhinakaran
> > <dhinakaran.pandiyan@intel.com> wrote:
> >         On Thu, 2017-05-11 at 16:02 -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 is still within
> >         25%
> >         > of the desired frequency.
> >
> >         I read a few eDP panel data sheets, the PWM frequencies all
> >         start from
> >         ~200Hz. If the VBT chooses this lowest value to allow for more
> >         brightness control, and then this patch lowers the value by
> >         another 25%,
> >         we'll end up below the panel allowed PWM frequency.
> >
> >         In fact, one of the systems I checked had PWM frequency as
> >         200Hz in VBT
> >         and the panel datasheet also had PWM frequency range starting
> >         from
> >         200Hz. Have you considered this case?
> >
> > The spec said "A given LCD panel typically has a limited range of
> > backlight frequency capability.
> > To limit the programmable frequency range, limitations are placed on
> > the allowable total divider ratio with the Sink device"
> >  So I think it should be auto cap to 200Hz in this case.
> >
> >         -DK
> >         >
> >         > Signed-off-by: Puthikorn Voravootivat <puthik@chromium.org>
> >         > ---
> >         >  drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 81
> >         +++++++++++++++++++++++++++
> >         >  1 file changed, 81 insertions(+)
> >         >
> >         > diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> >         b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> >         > index 0b48851013cc..6f10a2f1ab76 100644
> >         > --- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> >         > +++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> >         > @@ -113,12 +113,86 @@
> >         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 void 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;
> >         > +     }
> >         > +
> >         > +     fxp = 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
> >         > +      * - Effective frequency is within 25% of desired
> >         frequency.
> >         > +      */
> >         > +     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;
> >         > +     }
> >         > +     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;
> >         > +     }
> >         > +     pn_min &= DP_EDP_PWMGEN_BIT_COUNT_MASK;
> >         > +     pn_max &= DP_EDP_PWMGEN_BIT_COUNT_MASK;
> >         > +
> >         > +     fxp_min = fxp * 3 / 4;
> >         > +     fxp_max = fxp * 5 / 4;
> >
> >
> >         You are allowing fxp between +/- 25% of the actual. This isn't
> >         same as
> >         the "Effective frequency is within 25% of desired frequency."
> >         right? The
> >         frequency can vary between -20% and +33%.
> >
> >
> > You are right.
> > You want me to change commit message to reflect this or change the
> > code to
> > match the commit message?
>
> I am okay with fixing the comment and commit message. Is the 25%
> arbitrary or based on the distances between the possible values? Please
> make a note in the comment if it's the former.
>
>
> >         > +     if (fxp_min < (1 << pn_min) || (255 << pn_max) <
> >         fxp_max) {
>
>
>
> >         > +             DRM_DEBUG_KMS("VBT defined backlight frequency
> >         out of range\n");
> >         > +             return;
> >         > +     }
> >         > +
> >         > +     for (pn = pn_max; pn > pn_min; pn--) {
>
> Is there a reason this is not pn >= pn_min?
>
This is bug because f value will be incorrect in the case that pn == pn_min.
Thanks for catching this.


>
> >         > +             f = clamp(fxp >> 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");
>
>
> If the number of brightness control bits are changing, the max.
> brightness value changes too.
>
> Please see intel_dp_aux_setup_backlight().
>
> I think this is fine because
- intel_dp_aux_setup_backlight() will
called intel_dp_aux_enable_backlight() which
called intel_dp_aux_set_pwm_freq() first before determine the max
brightness value.
- Also, the panel I tested does not change BACKLIGHT_BRIGHTNESS_BYTE_COUNT
when changing the frequency.

>
>
> >         > +             return;
> >         > +     }
> >         > +     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;
> >         > +     }
> >         > +}
> >         > +
> >         >  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 new_dpcd_buf = 0;
> >         >       uint8_t edp_backlight_mode = 0;
> >         > +     bool freq_cap;
> >         >
> >         >       if (drm_dp_dpcd_readb(&intel_dp->aux,
> >         >                       DP_EDP_BACKLIGHT_MODE_SET_REGISTER,
> >         &dpcd_buf) != 1) {
> >         > @@ -151,6 +225,10 @@ static void
> >         intel_dp_aux_enable_backlight(struct intel_connector
> >         *connector)
> >         >               DRM_DEBUG_KMS("Enable dynamic brightness.\n");
> >         >       }
> >         >
> >         > +     freq_cap = intel_dp->edp_dpcd[2] &
> >         DP_EDP_BACKLIGHT_FREQ_AUX_SET_CAP;
> >         > +     if (freq_cap)
> >         > +             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) {
> >         > @@ -158,6 +236,9 @@ static void
> >         intel_dp_aux_enable_backlight(struct intel_connector
> >         *connector)
> >         >               }
> >         >       }
> >         >
> >         > +     if (freq_cap)
> >         > +             intel_dp_aux_set_pwm_freq(connector);
> >         > +
> >         >       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
>
>

[-- Attachment #1.2: Type: text/html, Size: 14821 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

Re: [PATCH v7 9/9] drm/i915: Set PWM divider to match desired frequency in vbt

[Pandiyan, Dhinakaran]

On Mon, 2017-05-15 at 17:43 -0700, Puthikorn Voravootivat wrote:
> 
> 
> On Mon, May 15, 2017 at 4:07 PM, Pandiyan, Dhinakaran
> <dhinakaran.pandiyan@intel.com> wrote:
>         On Fri, 2017-05-12 at 17:31 -0700, Puthikorn Voravootivat
>         wrote:
>         >
>         >
>         >
>         > On Fri, May 12, 2017 at 5:12 PM, Pandiyan, Dhinakaran
>         > <dhinakaran.pandiyan@intel.com> wrote:
>         >         On Thu, 2017-05-11 at 16:02 -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 is still
>         within
>         >         25%
>         >         > of the desired frequency.
>         >
>         >         I read a few eDP panel data sheets, the PWM
>         frequencies all
>         >         start from
>         >         ~200Hz. If the VBT chooses this lowest value to
>         allow for more
>         >         brightness control, and then this patch lowers the
>         value by
>         >         another 25%,
>         >         we'll end up below the panel allowed PWM frequency.
>         >
>         >         In fact, one of the systems I checked had PWM
>         frequency as
>         >         200Hz in VBT
>         >         and the panel datasheet also had PWM frequency range
>         starting
>         >         from
>         >         200Hz. Have you considered this case?
>         >
>         > The spec said "A given LCD panel typically has a limited
>         range of
>         > backlight frequency capability.
>         > To limit the programmable frequency range, limitations are
>         placed on
>         > the allowable total divider ratio with the Sink device"
>         >  So I think it should be auto cap to 200Hz in this case.
>         >
>         >         -DK
>         >         >
>         >         > Signed-off-by: Puthikorn Voravootivat
>         <puthik@chromium.org>
>         >         > ---
>         >         >  drivers/gpu/drm/i915/intel_dp_aux_backlight.c |
>         81
>         >         +++++++++++++++++++++++++++
>         >         >  1 file changed, 81 insertions(+)
>         >         >
>         >         > diff --git
>         a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
>         >         b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
>         >         > index 0b48851013cc..6f10a2f1ab76 100644
>         >         > ---
>         a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
>         >         > +++
>         b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
>         >         > @@ -113,12 +113,86 @@
>         >         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 void 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;
>         >         > +     }
>         >         > +
>         >         > +     fxp = 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
>         >         > +      * - Effective frequency is within 25% of
>         desired
>         >         frequency.
>         >         > +      */
>         >         > +     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;
>         >         > +     }
>         >         > +     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;
>         >         > +     }
>         >         > +     pn_min &= DP_EDP_PWMGEN_BIT_COUNT_MASK;
>         >         > +     pn_max &= DP_EDP_PWMGEN_BIT_COUNT_MASK;
>         >         > +
>         >         > +     fxp_min = fxp * 3 / 4;
>         >         > +     fxp_max = fxp * 5 / 4;
>         >
>         >
>         >         You are allowing fxp between +/- 25% of the actual.
>         This isn't
>         >         same as
>         >         the "Effective frequency is within 25% of desired
>         frequency."
>         >         right? The
>         >         frequency can vary between -20% and +33%.
>         >
>         >
>         > You are right.
>         > You want me to change commit message to reflect this or
>         change the
>         > code to
>         > match the commit message?
>         
>         
>         I am okay with fixing the comment and commit message. Is the
>         25%
>         arbitrary or based on the distances between the possible
>         values? Please
>         make a note in the comment if it's the former.
>         
>         
>         >         > +     if (fxp_min < (1 << pn_min) || (255 <<
>         pn_max) <
>         >         fxp_max) {
>         
>         
>         
>         >         > +             DRM_DEBUG_KMS("VBT defined backlight
>         frequency
>         >         out of range\n");
>         >         > +             return;
>         >         > +     }
>         >         > +
>         >         > +     for (pn = pn_max; pn > pn_min; pn--) {
>         
>         Is there a reason this is not pn >= pn_min?
> This is bug because f value will be incorrect in the case that pn ==
> pn_min.
> Thanks for catching this.
>  

Isn't that a side-effect using the right shift operation for division?
Would DIV_ROUND_CLOSEST allow you to use pn_min?

>         
>         >         > +             f = clamp(fxp >> 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");
>         
>         
>         If the number of brightness control bits are changing, the
>         max.
>         brightness value changes too.
>         
>         Please see intel_dp_aux_setup_backlight().
>         
>         
> I think this is fine because
> - intel_dp_aux_setup_backlight() will
> called intel_dp_aux_enable_backlight() which 
> called intel_dp_aux_set_pwm_freq() first before determine the max
> brightness value.
> - Also, the panel I tested does not change
> BACKLIGHT_BRIGHTNESS_BYTE_COUNT
> 
> when changing the frequency. 

The only values I see being set for max brightness are 0xFFFF and 0xFF  

if (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_BYTE_COUNT)
	panel->backlight.max = 0xFFFF;
else
	panel->backlight.max = 0xFF;

I can't see where you are setting this based on Pn. Can you please point
out? From what I understand, max should be 2^Pn.

Also, won't this function be called every time _enable_backlight() is
called? Isn't doing this from setup sufficient? I guess, fixing it is an
optimization that'd be nice to have but not necessary.
>         
>         
>         >         > +             return;
>         >         > +     }
>         >         > +     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;
>         >         > +     }
>         >         > +}
>         >         > +
>         >         >  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 new_dpcd_buf = 0;
>         >         >       uint8_t edp_backlight_mode = 0;
>         >         > +     bool freq_cap;
>         >         >
>         >         >       if (drm_dp_dpcd_readb(&intel_dp->aux,
>         >         >
>          DP_EDP_BACKLIGHT_MODE_SET_REGISTER,
>         >         &dpcd_buf) != 1) {
>         >         > @@ -151,6 +225,10 @@ static void
>         >         intel_dp_aux_enable_backlight(struct intel_connector
>         >         *connector)
>         >         >               DRM_DEBUG_KMS("Enable dynamic
>         brightness.\n");
>         >         >       }
>         >         >
>         >         > +     freq_cap = intel_dp->edp_dpcd[2] &
>         >         DP_EDP_BACKLIGHT_FREQ_AUX_SET_CAP;
>         >         > +     if (freq_cap)
>         >         > +             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) {
>         >         > @@ -158,6 +236,9 @@ static void
>         >         intel_dp_aux_enable_backlight(struct intel_connector
>         >         *connector)
>         >         >               }
>         >         >       }
>         >         >
>         >         > +     if (freq_cap)
>         >         > +
>          intel_dp_aux_set_pwm_freq(connector);
>         >         > +
>         >         >       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

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

Re: [PATCH v7 9/9] drm/i915: Set PWM divider to match desired frequency in vbt

[Puthikorn Voravootivat]

[-- Attachment #1.1: Type: text/plain, Size: 15407 bytes --]

On Mon, May 15, 2017 at 11:21 PM, Pandiyan, Dhinakaran <
dhinakaran.pandiyan@intel.com> wrote:

> On Mon, 2017-05-15 at 17:43 -0700, Puthikorn Voravootivat wrote:
> >
> >
> > On Mon, May 15, 2017 at 4:07 PM, Pandiyan, Dhinakaran
> > <dhinakaran.pandiyan@intel.com> wrote:
> >         On Fri, 2017-05-12 at 17:31 -0700, Puthikorn Voravootivat
> >         wrote:
> >         >
> >         >
> >         >
> >         > On Fri, May 12, 2017 at 5:12 PM, Pandiyan, Dhinakaran
> >         > <dhinakaran.pandiyan@intel.com> wrote:
> >         >         On Thu, 2017-05-11 at 16:02 -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 is still
> >         within
> >         >         25%
> >         >         > of the desired frequency.
> >         >
> >         >         I read a few eDP panel data sheets, the PWM
> >         frequencies all
> >         >         start from
> >         >         ~200Hz. If the VBT chooses this lowest value to
> >         allow for more
> >         >         brightness control, and then this patch lowers the
> >         value by
> >         >         another 25%,
> >         >         we'll end up below the panel allowed PWM frequency.
> >         >
> >         >         In fact, one of the systems I checked had PWM
> >         frequency as
> >         >         200Hz in VBT
> >         >         and the panel datasheet also had PWM frequency range
> >         starting
> >         >         from
> >         >         200Hz. Have you considered this case?
> >         >
> >         > The spec said "A given LCD panel typically has a limited
> >         range of
> >         > backlight frequency capability.
> >         > To limit the programmable frequency range, limitations are
> >         placed on
> >         > the allowable total divider ratio with the Sink device"
> >         >  So I think it should be auto cap to 200Hz in this case.
> >         >
> >         >         -DK
> >         >         >
> >         >         > Signed-off-by: Puthikorn Voravootivat
> >         <puthik@chromium.org>
> >         >         > ---
> >         >         >  drivers/gpu/drm/i915/intel_dp_aux_backlight.c |
> >         81
> >         >         +++++++++++++++++++++++++++
> >         >         >  1 file changed, 81 insertions(+)
> >         >         >
> >         >         > diff --git
> >         a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> >         >         b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> >         >         > index 0b48851013cc..6f10a2f1ab76 100644
> >         >         > ---
> >         a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> >         >         > +++
> >         b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> >         >         > @@ -113,12 +113,86 @@
> >         >         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 void 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;
> >         >         > +     }
> >         >         > +
> >         >         > +     fxp = 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
> >         >         > +      * - Effective frequency is within 25% of
> >         desired
> >         >         frequency.
> >         >         > +      */
> >         >         > +     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;
> >         >         > +     }
> >         >         > +     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;
> >         >         > +     }
> >         >         > +     pn_min &= DP_EDP_PWMGEN_BIT_COUNT_MASK;
> >         >         > +     pn_max &= DP_EDP_PWMGEN_BIT_COUNT_MASK;
> >         >         > +
> >         >         > +     fxp_min = fxp * 3 / 4;
> >         >         > +     fxp_max = fxp * 5 / 4;
> >         >
> >         >
> >         >         You are allowing fxp between +/- 25% of the actual.
> >         This isn't
> >         >         same as
> >         >         the "Effective frequency is within 25% of desired
> >         frequency."
> >         >         right? The
> >         >         frequency can vary between -20% and +33%.
> >         >
> >         >
> >         > You are right.
> >         > You want me to change commit message to reflect this or
> >         change the
> >         > code to
> >         > match the commit message?
> >
> >
> >         I am okay with fixing the comment and commit message. Is the
> >         25%
> >         arbitrary or based on the distances between the possible
> >         values? Please
> >         make a note in the comment if it's the former.
> >
> >
> >         >         > +     if (fxp_min < (1 << pn_min) || (255 <<
> >         pn_max) <
> >         >         fxp_max) {
> >
> >
> >
> >         >         > +             DRM_DEBUG_KMS("VBT defined backlight
> >         frequency
> >         >         out of range\n");
> >         >         > +             return;
> >         >         > +     }
> >         >         > +
> >         >         > +     for (pn = pn_max; pn > pn_min; pn--) {
> >
> >         Is there a reason this is not pn >= pn_min?
> > This is bug because f value will be incorrect in the case that pn ==
> > pn_min.
> > Thanks for catching this.
> >
>
> Isn't that a side-effect using the right shift operation for division?
>
The bug is just for loop that exit too soon.

Would DIV_ROUND_CLOSEST allow you to use pn_min?
>
It does not related to the point above, but DIV_ROUND_CLOSEST still
better than just using right shift. I will change to that in next version.


>
> >
> >         >         > +             f = clamp(fxp >> 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");
> >
> >
> >         If the number of brightness control bits are changing, the
> >         max.
> >         brightness value changes too.
> >
> >         Please see intel_dp_aux_setup_backlight().
> >
> >
> > I think this is fine because
> > - intel_dp_aux_setup_backlight() will
> > called intel_dp_aux_enable_backlight() which
> > called intel_dp_aux_set_pwm_freq() first before determine the max
> > brightness value.
> > - Also, the panel I tested does not change
> > BACKLIGHT_BRIGHTNESS_BYTE_COUNT
> >
> > when changing the frequency.
>
> The only values I see being set for max brightness are 0xFFFF and 0xFF
>
> if (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_BYTE_COUNT)
>         panel->backlight.max = 0xFFFF;
> else
>         panel->backlight.max = 0xFF;



> I can't see where you are setting this based on Pn. Can you please point
> out? From what I understand, max should be 2^Pn.
>
>
It is suppose to be 0xFF or 0xFFFF only.

>From eDP spec:
0x702 bit 2 BACKLIGHT_BRIGHTNESS_BYTE_COUNT
0 = Indicates that the Sink device supports a 1-byte setting for backlight
brightness
1 = Indicates that the Sink device supports a 2-byte setting for the
backlight brightness

0x722 EDP_BACKLIGHT_BRIGHTNESS_MSB
The actual number of assigned bits for the backlight brightness PWM
generator is set by
field 4:0 of the EDP_PWMGEN_BIT_COUNT register (DPCD Address 00724h).
Assigned bits are allocated to the MSB of the enabled register combination

This means that if PWM_GEN_BIT_COUNT is less than 16 then the panel will
ignore
the least significant bit in EDP_BACKLIGHT_BRIGHTNESS register.

For example,
if  BACKLIGHT_BRIGHTNESS_BYTE_COUNT == 1 and PWM_GEN_BIT_COUNT = 16
and EDP_BACKLIGHT_BRIGHTNESS == 0xabcd
In this case, all bits in EDP_BACKLIGHT_BRIGHTNESS will be used.
0xabcd means 0xabcd / 0xffff or (43981 / 65535) or 67.111%

if  BACKLIGHT_BRIGHTNESS_BYTE_COUNT == 1 and PWM_GEN_BIT_COUNT = 12
and EDP_BACKLIGHT_BRIGHTNESS == 0xabcd
In this case, the last 4 bits will be discarded.
0xabcd means 0xabc / 0xfff or 2748 / 4095 or 67.106%

I think it should be fine to just have 8 or 16 bits of the max brightness
and let panel drop
the unneed bit to simplify the driver code.



> Also, won't this function be called every time _enable_backlight() is
> called? Isn't doing this from setup sufficient? I guess, fixing it is an
> optimization that'd be nice to have but not necessary.
>

Lets get this patch set done first. I can send in another patch after this
is go in to optimize this.
Probably need to add new member in struct drm_i915_private


> >
> >
> >         >         > +             return;
> >         >         > +     }
> >         >         > +     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;
> >         >         > +     }
> >         >         > +}
> >         >         > +
> >         >         >  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 new_dpcd_buf = 0;
> >         >         >       uint8_t edp_backlight_mode = 0;
> >         >         > +     bool freq_cap;
> >         >         >
> >         >         >       if (drm_dp_dpcd_readb(&intel_dp->aux,
> >         >         >
> >          DP_EDP_BACKLIGHT_MODE_SET_REGISTER,
> >         >         &dpcd_buf) != 1) {
> >         >         > @@ -151,6 +225,10 @@ static void
> >         >         intel_dp_aux_enable_backlight(struct intel_connector
> >         >         *connector)
> >         >         >               DRM_DEBUG_KMS("Enable dynamic
> >         brightness.\n");
> >         >         >       }
> >         >         >
> >         >         > +     freq_cap = intel_dp->edp_dpcd[2] &
> >         >         DP_EDP_BACKLIGHT_FREQ_AUX_SET_CAP;
> >         >         > +     if (freq_cap)
> >         >         > +             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) {
> >         >         > @@ -158,6 +236,9 @@ static void
> >         >         intel_dp_aux_enable_backlight(struct intel_connector
> >         >         *connector)
> >         >         >               }
> >         >         >       }
> >         >         >
> >         >         > +     if (freq_cap)
> >         >         > +
> >          intel_dp_aux_set_pwm_freq(connector);
> >         >         > +
> >         >         >       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
>
>

[-- Attachment #1.2: Type: text/html, Size: 25280 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

Re: [PATCH v7 9/9] drm/i915: Set PWM divider to match desired frequency in vbt

[Pandiyan, Dhinakaran]

On Tue, 2017-05-16 at 11:07 -0700, Puthikorn Voravootivat wrote:
> 
> 
> On Mon, May 15, 2017 at 11:21 PM, Pandiyan, Dhinakaran
> <dhinakaran.pandiyan@intel.com> wrote:
>         On Mon, 2017-05-15 at 17:43 -0700, Puthikorn Voravootivat
>         wrote:
>         >
>         >
>         > On Mon, May 15, 2017 at 4:07 PM, Pandiyan, Dhinakaran
>         > <dhinakaran.pandiyan@intel.com> wrote:
>         >         On Fri, 2017-05-12 at 17:31 -0700, Puthikorn
>         Voravootivat
>         >         wrote:
>         >         >
>         >         >
>         >         >
>         >         > On Fri, May 12, 2017 at 5:12 PM, Pandiyan,
>         Dhinakaran
>         >         > <dhinakaran.pandiyan@intel.com> wrote:
>         >         >         On Thu, 2017-05-11 at 16:02 -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 is still
>         >         within
>         >         >         25%
>         >         >         > of the desired frequency.
>         >         >
>         >         >         I read a few eDP panel data sheets, the
>         PWM
>         >         frequencies all
>         >         >         start from
>         >         >         ~200Hz. If the VBT chooses this lowest
>         value to
>         >         allow for more
>         >         >         brightness control, and then this patch
>         lowers the
>         >         value by
>         >         >         another 25%,
>         >         >         we'll end up below the panel allowed PWM
>         frequency.
>         >         >
>         >         >         In fact, one of the systems I checked had
>         PWM
>         >         frequency as
>         >         >         200Hz in VBT
>         >         >         and the panel datasheet also had PWM
>         frequency range
>         >         starting
>         >         >         from
>         >         >         200Hz. Have you considered this case?
>         >         >
>         >         > The spec said "A given LCD panel typically has a
>         limited
>         >         range of
>         >         > backlight frequency capability.
>         >         > To limit the programmable frequency range,
>         limitations are
>         >         placed on
>         >         > the allowable total divider ratio with the Sink
>         device"
>         >         >  So I think it should be auto cap to 200Hz in this
>         case.
>         >         >
>         >         >         -DK
>         >         >         >
>         >         >         > Signed-off-by: Puthikorn Voravootivat
>         >         <puthik@chromium.org>
>         >         >         > ---
>         >         >         >
>         drivers/gpu/drm/i915/intel_dp_aux_backlight.c |
>         >         81
>         >         >         +++++++++++++++++++++++++++
>         >         >         >  1 file changed, 81 insertions(+)
>         >         >         >
>         >         >         > diff --git
>         >         a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
>         >         >
>          b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
>         >         >         > index 0b48851013cc..6f10a2f1ab76 100644
>         >         >         > ---
>         >         a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
>         >         >         > +++
>         >         b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
>         >         >         > @@ -113,12 +113,86 @@
>         >         >
>          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 void
>         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;
>         >         >         > +     }
>         >         >         > +
>         >         >         > +     fxp =
>         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
>         >         >         > +      * - Effective frequency is within
>         25% of
>         >         desired
>         >         >         frequency.
>         >         >         > +      */
>         >         >         > +     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;
>         >         >         > +     }
>         >         >         > +     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;
>         >         >         > +     }
>         >         >         > +     pn_min &=
>         DP_EDP_PWMGEN_BIT_COUNT_MASK;
>         >         >         > +     pn_max &=
>         DP_EDP_PWMGEN_BIT_COUNT_MASK;
>         >         >         > +
>         >         >         > +     fxp_min = fxp * 3 / 4;
>         >         >         > +     fxp_max = fxp * 5 / 4;
>         >         >
>         >         >
>         >         >         You are allowing fxp between +/- 25% of
>         the actual.
>         >         This isn't
>         >         >         same as
>         >         >         the "Effective frequency is within 25% of
>         desired
>         >         frequency."
>         >         >         right? The
>         >         >         frequency can vary between -20% and +33%.
>         >         >
>         >         >
>         >         > You are right.
>         >         > You want me to change commit message to reflect
>         this or
>         >         change the
>         >         > code to
>         >         > match the commit message?
>         >
>         >
>         >         I am okay with fixing the comment and commit
>         message. Is the
>         >         25%
>         >         arbitrary or based on the distances between the
>         possible
>         >         values? Please
>         >         make a note in the comment if it's the former.
>         >
>         >
>         >         >         > +     if (fxp_min < (1 << pn_min) ||
>         (255 <<
>         >         pn_max) <
>         >         >         fxp_max) {
>         >
>         >
>         >
>         >         >         > +             DRM_DEBUG_KMS("VBT defined
>         backlight
>         >         frequency
>         >         >         out of range\n");
>         >         >         > +             return;
>         >         >         > +     }
>         >         >         > +
>         >         >         > +     for (pn = pn_max; pn > pn_min;
>         pn--) {
>         >
>         >         Is there a reason this is not pn >= pn_min?
>         > This is bug because f value will be incorrect in the case
>         that pn ==
>         > pn_min.
>         > Thanks for catching this.
>         >
>         
>         
>         Isn't that a side-effect using the right shift operation for
>         division?
> The bug is just for loop that exit too soon. 
> 
Not sure I got that, what's the invalid "f" case that you see with
pn==pn_min?



>         Would DIV_ROUND_CLOSEST allow you to use pn_min?
> It does not related to the point above, but DIV_ROUND_CLOSEST still
> better than just using right shift. I will change to that in next
> version.
>  
>         
>         >
>         >         >         > +             f = clamp(fxp >> 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");
>         >
>         >
>         >         If the number of brightness control bits are
>         changing, the
>         >         max.
>         >         brightness value changes too.
>         >
>         >         Please see intel_dp_aux_setup_backlight().
>         >
>         >
>         > I think this is fine because
>         > - intel_dp_aux_setup_backlight() will
>         > called intel_dp_aux_enable_backlight() which
>         > called intel_dp_aux_set_pwm_freq() first before determine
>         the max
>         > brightness value.
>         > - Also, the panel I tested does not change
>         > BACKLIGHT_BRIGHTNESS_BYTE_COUNT
>         >
>         > when changing the frequency.
>         
>         
>         The only values I see being set for max brightness are 0xFFFF
>         and 0xFF
>         
>         if (intel_dp->edp_dpcd[2] &
>         DP_EDP_BACKLIGHT_BRIGHTNESS_BYTE_COUNT)
>                 panel->backlight.max = 0xFFFF;
>         else
>                 panel->backlight.max = 0xFF;
>  
>         I can't see where you are setting this based on Pn. Can you
>         please point
>         out? From what I understand, max should be 2^Pn.
>         
> 
> It is suppose to be 0xFF or 0xFFFF only.
> 
> 
> From eDP spec:
> 0x702 bit 2 BACKLIGHT_BRIGHTNESS_BYTE_COUNT
> 0 = Indicates that the Sink device supports a 1-byte setting for
> backlight brightness
> 1 = Indicates that the Sink device supports a 2-byte setting for the
> backlight brightness
> 
> 
> 0x722 EDP_BACKLIGHT_BRIGHTNESS_MSB
> The actual number of assigned bits for the backlight brightness PWM
> generator is set by
> field 4:0 of the EDP_PWMGEN_BIT_COUNT register (DPCD Address 00724h).
> Assigned bits are allocated to the MSB of the enabled register
> combination
> 
^This makes it somewhat clear but gets confusing again.
> 
> This means that if PWM_GEN_BIT_COUNT is less than 16 then the panel
> will ignore
> the least significant bit in EDP_BACKLIGHT_BRIGHTNESS register.
> 
> 
> For example, 
> if  BACKLIGHT_BRIGHTNESS_BYTE_COUNT == 1 and PWM_GEN_BIT_COUNT = 16
> and EDP_BACKLIGHT_BRIGHTNESS == 0xabcd
> In this case, all bits in EDP_BACKLIGHT_BRIGHTNESS will be used.
> 
> 0xabcd means 0xabcd / 0xffff or (43981 / 65535) or 67.111%
> 
> 
> if  BACKLIGHT_BRIGHTNESS_BYTE_COUNT == 1 and PWM_GEN_BIT_COUNT = 12
> and EDP_BACKLIGHT_BRIGHTNESS == 0xabcd
> 
> In this case, the last 4 bits will be discarded.
> 0xabcd means 0xabc / 0xfff or 2748 / 4095 or 67.106%
> 
> 
> I think it should be fine to just have 8 or 16 bits of the max
> brightness and let panel drop
> the unneed bit to simplify the driver code.
> 

Note:
The number of assigned or controllable bits will have the ability to
provide full brightness control. Examples:
•
 For 10-bit operation, 000h = 0% brightness, and 3FFh = 100% brightness.

Going by the logic you stated, for a 10-bit control, the MSB and LSB
registers have to be written FFFFh to set 100% brightness with the last
6 bits dropped off by the hardware.

I don't like the way the spec leaves this to interpretation. Have you
experimented with both 0x0ABC and 0xABCD for the 12-bit, BYTE_COUNT==1
example you gave?


>  
> 
>         Also, won't this function be called every time
>         _enable_backlight() is
>         called? Isn't doing this from setup sufficient? I guess,
>         fixing it is an
>         optimization that'd be nice to have but not necessary.
>  
> Lets get this patch set done first. I can send in another patch after
> this is go in to optimize this.

Sure. This patch should be good to go with answers to my questions. The
only thing remaining would be getting an ACK from Jani for adding a new
parameter for dynamic backlight control.


-DK

> Probably need to add new member in struct drm_i915_private
>  
>         >
>         >
>         >         >         > +             return;
>         >         >         > +     }
>         >         >         > +     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;
>         >         >         > +     }
>         >         >         > +}
>         >         >         > +
>         >         >         >  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 new_dpcd_buf = 0;
>         >         >         >       uint8_t edp_backlight_mode = 0;
>         >         >         > +     bool freq_cap;
>         >         >         >
>         >         >         >       if
>         (drm_dp_dpcd_readb(&intel_dp->aux,
>         >         >         >
>         >          DP_EDP_BACKLIGHT_MODE_SET_REGISTER,
>         >         >         &dpcd_buf) != 1) {
>         >         >         > @@ -151,6 +225,10 @@ static void
>         >         >         intel_dp_aux_enable_backlight(struct
>         intel_connector
>         >         >         *connector)
>         >         >         >               DRM_DEBUG_KMS("Enable
>         dynamic
>         >         brightness.\n");
>         >         >         >       }
>         >         >         >
>         >         >         > +     freq_cap = intel_dp->edp_dpcd[2] &
>         >         >         DP_EDP_BACKLIGHT_FREQ_AUX_SET_CAP;
>         >         >         > +     if (freq_cap)
>         >         >         > +             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) {
>         >         >         > @@ -158,6 +236,9 @@ static void
>         >         >         intel_dp_aux_enable_backlight(struct
>         intel_connector
>         >         >         *connector)
>         >         >         >               }
>         >         >         >       }
>         >         >         >
>         >         >         > +     if (freq_cap)
>         >         >         > +
>         >          intel_dp_aux_set_pwm_freq(connector);
>         >         >         > +
>         >         >         >       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
>         
>         
> 
> 
> _______________________________________________
> 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

Re: [PATCH v7 9/9] drm/i915: Set PWM divider to match desired frequency in vbt

[Puthikorn Voravootivat]

[-- Attachment #1.1: Type: text/plain, Size: 22514 bytes --]

On Tue, May 16, 2017 at 1:29 PM, Pandiyan, Dhinakaran <
dhinakaran.pandiyan@intel.com> wrote:

> On Tue, 2017-05-16 at 11:07 -0700, Puthikorn Voravootivat wrote:
> >
> >
> > On Mon, May 15, 2017 at 11:21 PM, Pandiyan, Dhinakaran
> > <dhinakaran.pandiyan@intel.com> wrote:
> >         On Mon, 2017-05-15 at 17:43 -0700, Puthikorn Voravootivat
> >         wrote:
> >         >
> >         >
> >         > On Mon, May 15, 2017 at 4:07 PM, Pandiyan, Dhinakaran
> >         > <dhinakaran.pandiyan@intel.com> wrote:
> >         >         On Fri, 2017-05-12 at 17:31 -0700, Puthikorn
> >         Voravootivat
> >         >         wrote:
> >         >         >
> >         >         >
> >         >         >
> >         >         > On Fri, May 12, 2017 at 5:12 PM, Pandiyan,
> >         Dhinakaran
> >         >         > <dhinakaran.pandiyan@intel.com> wrote:
> >         >         >         On Thu, 2017-05-11 at 16:02 -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 is still
> >         >         within
> >         >         >         25%
> >         >         >         > of the desired frequency.
> >         >         >
> >         >         >         I read a few eDP panel data sheets, the
> >         PWM
> >         >         frequencies all
> >         >         >         start from
> >         >         >         ~200Hz. If the VBT chooses this lowest
> >         value to
> >         >         allow for more
> >         >         >         brightness control, and then this patch
> >         lowers the
> >         >         value by
> >         >         >         another 25%,
> >         >         >         we'll end up below the panel allowed PWM
> >         frequency.
> >         >         >
> >         >         >         In fact, one of the systems I checked had
> >         PWM
> >         >         frequency as
> >         >         >         200Hz in VBT
> >         >         >         and the panel datasheet also had PWM
> >         frequency range
> >         >         starting
> >         >         >         from
> >         >         >         200Hz. Have you considered this case?
> >         >         >
> >         >         > The spec said "A given LCD panel typically has a
> >         limited
> >         >         range of
> >         >         > backlight frequency capability.
> >         >         > To limit the programmable frequency range,
> >         limitations are
> >         >         placed on
> >         >         > the allowable total divider ratio with the Sink
> >         device"
> >         >         >  So I think it should be auto cap to 200Hz in this
> >         case.
> >         >         >
> >         >         >         -DK
> >         >         >         >
> >         >         >         > Signed-off-by: Puthikorn Voravootivat
> >         >         <puthik@chromium.org>
> >         >         >         > ---
> >         >         >         >
> >         drivers/gpu/drm/i915/intel_dp_aux_backlight.c |
> >         >         81
> >         >         >         +++++++++++++++++++++++++++
> >         >         >         >  1 file changed, 81 insertions(+)
> >         >         >         >
> >         >         >         > diff --git
> >         >         a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> >         >         >
> >          b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> >         >         >         > index 0b48851013cc..6f10a2f1ab76 100644
> >         >         >         > ---
> >         >         a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> >         >         >         > +++
> >         >         b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> >         >         >         > @@ -113,12 +113,86 @@
> >         >         >
> >          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 void
> >         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;
> >         >         >         > +     }
> >         >         >         > +
> >         >         >         > +     fxp =
> >         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
> >         >         >         > +      * - Effective frequency is within
> >         25% of
> >         >         desired
> >         >         >         frequency.
> >         >         >         > +      */
> >         >         >         > +     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;
> >         >         >         > +     }
> >         >         >         > +     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;
> >         >         >         > +     }
> >         >         >         > +     pn_min &=
> >         DP_EDP_PWMGEN_BIT_COUNT_MASK;
> >         >         >         > +     pn_max &=
> >         DP_EDP_PWMGEN_BIT_COUNT_MASK;
> >         >         >         > +
> >         >         >         > +     fxp_min = fxp * 3 / 4;
> >         >         >         > +     fxp_max = fxp * 5 / 4;
> >         >         >
> >         >         >
> >         >         >         You are allowing fxp between +/- 25% of
> >         the actual.
> >         >         This isn't
> >         >         >         same as
> >         >         >         the "Effective frequency is within 25% of
> >         desired
> >         >         frequency."
> >         >         >         right? The
> >         >         >         frequency can vary between -20% and +33%.
> >         >         >
> >         >         >
> >         >         > You are right.
> >         >         > You want me to change commit message to reflect
> >         this or
> >         >         change the
> >         >         > code to
> >         >         > match the commit message?
> >         >
> >         >
> >         >         I am okay with fixing the comment and commit
> >         message. Is the
> >         >         25%
> >         >         arbitrary or based on the distances between the
> >         possible
> >         >         values? Please
> >         >         make a note in the comment if it's the former.
> >         >
> >         >
> >         >         >         > +     if (fxp_min < (1 << pn_min) ||
> >         (255 <<
> >         >         pn_max) <
> >         >         >         fxp_max) {
> >         >
> >         >
> >         >
> >         >         >         > +             DRM_DEBUG_KMS("VBT defined
> >         backlight
> >         >         frequency
> >         >         >         out of range\n");
> >         >         >         > +             return;
> >         >         >         > +     }
> >         >         >         > +
> >         >         >         > +     for (pn = pn_max; pn > pn_min;
> >         pn--) {
> >         >
> >         >         Is there a reason this is not pn >= pn_min?
> >         > This is bug because f value will be incorrect in the case
> >         that pn ==
> >         > pn_min.
> >         > Thanks for catching this.
> >         >
> >
> >
> >         Isn't that a side-effect using the right shift operation for
> >         division?
> > The bug is just for loop that exit too soon.
> >
> Not sure I got that, what's the invalid "f" case that you see with
> pn==pn_min?
>
>
> if the for loop has pn > pn_min, when pn == pn_min we will exit the loop
first
without running the  f = clamp(fxp >> pn, 1, 255); in the next line.

This would fix with pn >= pn_min in for loop.

We guarantee that the break statement will be called and pn won't be pn_min
- 1
because we check (fxp_min < (1 << pn_min) || (255 << pn_max) < fxp_max)
before
entering this for loop.


> >         Would DIV_ROUND_CLOSEST allow you to use pn_min?
> > It does not related to the point above, but DIV_ROUND_CLOSEST still
> > better than just using right shift. I will change to that in next
> > version.
> >
> >
> >         >
> >         >         >         > +             f = clamp(fxp >> 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");
> >         >
> >         >
> >         >         If the number of brightness control bits are
> >         changing, the
> >         >         max.
> >         >         brightness value changes too.
> >         >
> >         >         Please see intel_dp_aux_setup_backlight().
> >         >
> >         >
> >         > I think this is fine because
> >         > - intel_dp_aux_setup_backlight() will
> >         > called intel_dp_aux_enable_backlight() which
> >         > called intel_dp_aux_set_pwm_freq() first before determine
> >         the max
> >         > brightness value.
> >         > - Also, the panel I tested does not change
> >         > BACKLIGHT_BRIGHTNESS_BYTE_COUNT
> >         >
> >         > when changing the frequency.
> >
> >
> >         The only values I see being set for max brightness are 0xFFFF
> >         and 0xFF
> >
> >         if (intel_dp->edp_dpcd[2] &
> >         DP_EDP_BACKLIGHT_BRIGHTNESS_BYTE_COUNT)
> >                 panel->backlight.max = 0xFFFF;
> >         else
> >                 panel->backlight.max = 0xFF;
> >
> >         I can't see where you are setting this based on Pn. Can you
> >         please point
> >         out? From what I understand, max should be 2^Pn.
> >
> >
> > It is suppose to be 0xFF or 0xFFFF only.
> >
> >
> > From eDP spec:
> > 0x702 bit 2 BACKLIGHT_BRIGHTNESS_BYTE_COUNT
> > 0 = Indicates that the Sink device supports a 1-byte setting for
> > backlight brightness
> > 1 = Indicates that the Sink device supports a 2-byte setting for the
> > backlight brightness
> >
> >
> > 0x722 EDP_BACKLIGHT_BRIGHTNESS_MSB
> > The actual number of assigned bits for the backlight brightness PWM
> > generator is set by
> > field 4:0 of the EDP_PWMGEN_BIT_COUNT register (DPCD Address 00724h).
> > Assigned bits are allocated to the MSB of the enabled register
> > combination
> >
> ^This makes it somewhat clear but gets confusing again.
> >
> > This means that if PWM_GEN_BIT_COUNT is less than 16 then the panel
> > will ignore
> > the least significant bit in EDP_BACKLIGHT_BRIGHTNESS register.
> >
> >
> > For example,
> > if  BACKLIGHT_BRIGHTNESS_BYTE_COUNT == 1 and PWM_GEN_BIT_COUNT = 16
> > and EDP_BACKLIGHT_BRIGHTNESS == 0xabcd
> > In this case, all bits in EDP_BACKLIGHT_BRIGHTNESS will be used.
> >
> > 0xabcd means 0xabcd / 0xffff or (43981 / 65535) or 67.111%
> >
> >
> > if  BACKLIGHT_BRIGHTNESS_BYTE_COUNT == 1 and PWM_GEN_BIT_COUNT = 12
> > and EDP_BACKLIGHT_BRIGHTNESS == 0xabcd
> >
> > In this case, the last 4 bits will be discarded.
> > 0xabcd means 0xabc / 0xfff or 2748 / 4095 or 67.106%
> >
> >
> > I think it should be fine to just have 8 or 16 bits of the max
> > brightness and let panel drop
> > the unneed bit to simplify the driver code.
> >
>
> Note:
> The number of assigned or controllable bits will have the ability to
> provide full brightness control. Examples:
> •
>  For 10-bit operation, 000h = 0% brightness, and 3FFh = 100% brightness.
>
> Going by the logic you stated, for a 10-bit control, the MSB and LSB
> registers have to be written FFFFh to set 100% brightness with the last
> 6 bits dropped off by the hardware.
>
> I don't like the way the spec leaves this to interpretation. Have you
> experimented with both 0x0ABC and 0xABCD for the 12-bit, BYTE_COUNT==1
> example you gave?
>
> Yes, already test with the real panel.

Here is better test case than 0xABCD
0xFFFF with 6, 12, 16 shows very bright screen with same brightness in all
case. (All 100%)
0x0FFF with 6, 12, 16 bits shows dim screen. 6 bit case is slightly dimmer
than other case.
Note that the brightness are 4.76% / 6.23% / 6.25% in 6 / 12 / 16 bits case.

You can apply this patch for easy testing
https://patchwork.kernel.org/patch/6241481/


>
> >
> >
> >         Also, won't this function be called every time
> >         _enable_backlight() is
> >         called? Isn't doing this from setup sufficient? I guess,
> >         fixing it is an
> >         optimization that'd be nice to have but not necessary.
> >
> > Lets get this patch set done first. I can send in another patch after
> > this is go in to optimize this.
>
> Sure. This patch should be good to go with answers to my questions. The
> only thing remaining would be getting an ACK from Jani for adding a new
> parameter for dynamic backlight control.
>
>
> -DK
>
> > Probably need to add new member in struct drm_i915_private
> >
> >         >
> >         >
> >         >         >         > +             return;
> >         >         >         > +     }
> >         >         >         > +     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;
> >         >         >         > +     }
> >         >         >         > +}
> >         >         >         > +
> >         >         >         >  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 new_dpcd_buf = 0;
> >         >         >         >       uint8_t edp_backlight_mode = 0;
> >         >         >         > +     bool freq_cap;
> >         >         >         >
> >         >         >         >       if
> >         (drm_dp_dpcd_readb(&intel_dp->aux,
> >         >         >         >
> >         >          DP_EDP_BACKLIGHT_MODE_SET_REGISTER,
> >         >         >         &dpcd_buf) != 1) {
> >         >         >         > @@ -151,6 +225,10 @@ static void
> >         >         >         intel_dp_aux_enable_backlight(struct
> >         intel_connector
> >         >         >         *connector)
> >         >         >         >               DRM_DEBUG_KMS("Enable
> >         dynamic
> >         >         brightness.\n");
> >         >         >         >       }
> >         >         >         >
> >         >         >         > +     freq_cap = intel_dp->edp_dpcd[2] &
> >         >         >         DP_EDP_BACKLIGHT_FREQ_AUX_SET_CAP;
> >         >         >         > +     if (freq_cap)
> >         >         >         > +             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) {
> >         >         >         > @@ -158,6 +236,9 @@ static void
> >         >         >         intel_dp_aux_enable_backlight(struct
> >         intel_connector
> >         >         >         *connector)
> >         >         >         >               }
> >         >         >         >       }
> >         >         >         >
> >         >         >         > +     if (freq_cap)
> >         >         >         > +
> >         >          intel_dp_aux_set_pwm_freq(connector);
> >         >         >         > +
> >         >         >         >       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
> >
> >
> >
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>

[-- Attachment #1.2: Type: text/html, Size: 34788 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

Re: [PATCH v7 9/9] drm/i915: Set PWM divider to match desired frequency in vbt

[Pandiyan, Dhinakaran]

On Tue, 2017-05-16 at 13:56 -0700, Puthikorn Voravootivat wrote:
> 
> 
> On Tue, May 16, 2017 at 1:29 PM, Pandiyan, Dhinakaran
> <dhinakaran.pandiyan@intel.com> wrote:
>         On Tue, 2017-05-16 at 11:07 -0700, Puthikorn Voravootivat
>         wrote:
>         >
>         >
>         > On Mon, May 15, 2017 at 11:21 PM, Pandiyan, Dhinakaran
>         > <dhinakaran.pandiyan@intel.com> wrote:
>         >         On Mon, 2017-05-15 at 17:43 -0700, Puthikorn
>         Voravootivat
>         >         wrote:
>         >         >
>         >         >
>         >         > On Mon, May 15, 2017 at 4:07 PM, Pandiyan,
>         Dhinakaran
>         >         > <dhinakaran.pandiyan@intel.com> wrote:
>         >         >         On Fri, 2017-05-12 at 17:31 -0700,
>         Puthikorn
>         >         Voravootivat
>         >         >         wrote:
>         >         >         >
>         >         >         >
>         >         >         >
>         >         >         > On Fri, May 12, 2017 at 5:12 PM,
>         Pandiyan,
>         >         Dhinakaran
>         >         >         > <dhinakaran.pandiyan@intel.com> wrote:
>         >         >         >         On Thu, 2017-05-11 at 16:02
>         -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 is still
>         >         >         within
>         >         >         >         25%
>         >         >         >         > of the desired frequency.
>         >         >         >
>         >         >         >         I read a few eDP panel data
>         sheets, the
>         >         PWM
>         >         >         frequencies all
>         >         >         >         start from
>         >         >         >         ~200Hz. If the VBT chooses this
>         lowest
>         >         value to
>         >         >         allow for more
>         >         >         >         brightness control, and then
>         this patch
>         >         lowers the
>         >         >         value by
>         >         >         >         another 25%,
>         >         >         >         we'll end up below the panel
>         allowed PWM
>         >         frequency.
>         >         >         >
>         >         >         >         In fact, one of the systems I
>         checked had
>         >         PWM
>         >         >         frequency as
>         >         >         >         200Hz in VBT
>         >         >         >         and the panel datasheet also had
>         PWM
>         >         frequency range
>         >         >         starting
>         >         >         >         from
>         >         >         >         200Hz. Have you considered this
>         case?
>         >         >         >
>         >         >         > The spec said "A given LCD panel
>         typically has a
>         >         limited
>         >         >         range of
>         >         >         > backlight frequency capability.
>         >         >         > To limit the programmable frequency
>         range,
>         >         limitations are
>         >         >         placed on
>         >         >         > the allowable total divider ratio with
>         the Sink
>         >         device"
>         >         >         >  So I think it should be auto cap to
>         200Hz in this
>         >         case.
>         >         >         >
>         >         >         >         -DK
>         >         >         >         >
>         >         >         >         > Signed-off-by: Puthikorn
>         Voravootivat
>         >         >         <puthik@chromium.org>
>         >         >         >         > ---
>         >         >         >         >
>         >         drivers/gpu/drm/i915/intel_dp_aux_backlight.c |
>         >         >         81
>         >         >         >         +++++++++++++++++++++++++++
>         >         >         >         >  1 file changed, 81
>         insertions(+)
>         >         >         >         >
>         >         >         >         > diff --git
>         >         >
>          a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
>         >         >         >
>         >          b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
>         >         >         >         > index
>         0b48851013cc..6f10a2f1ab76 100644
>         >         >         >         > ---
>         >         >
>          a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
>         >         >         >         > +++
>         >         >
>          b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
>         >         >         >         > @@ -113,12 +113,86 @@
>         >         >         >
>         >          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 void
>         >         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;
>         >         >         >         > +     }
>         >         >         >         > +
>         >         >         >         > +     fxp =
>         >         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
>         >         >         >         > +      * - Effective frequency
>         is within
>         >         25% of
>         >         >         desired
>         >         >         >         frequency.
>         >         >         >         > +      */
>         >         >         >         > +     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;
>         >         >         >         > +     }
>         >         >         >         > +     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;
>         >         >         >         > +     }
>         >         >         >         > +     pn_min &=
>         >         DP_EDP_PWMGEN_BIT_COUNT_MASK;
>         >         >         >         > +     pn_max &=
>         >         DP_EDP_PWMGEN_BIT_COUNT_MASK;
>         >         >         >         > +
>         >         >         >         > +     fxp_min = fxp * 3 / 4;
>         >         >         >         > +     fxp_max = fxp * 5 / 4;
>         >         >         >
>         >         >         >
>         >         >         >         You are allowing fxp between +/-
>         25% of
>         >         the actual.
>         >         >         This isn't
>         >         >         >         same as
>         >         >         >         the "Effective frequency is
>         within 25% of
>         >         desired
>         >         >         frequency."
>         >         >         >         right? The
>         >         >         >         frequency can vary between -20%
>         and +33%.
>         >         >         >
>         >         >         >
>         >         >         > You are right.
>         >         >         > You want me to change commit message to
>         reflect
>         >         this or
>         >         >         change the
>         >         >         > code to
>         >         >         > match the commit message?
>         >         >
>         >         >
>         >         >         I am okay with fixing the comment and
>         commit
>         >         message. Is the
>         >         >         25%
>         >         >         arbitrary or based on the distances
>         between the
>         >         possible
>         >         >         values? Please
>         >         >         make a note in the comment if it's the
>         former.
>         >         >
>         >         >
>         >         >         >         > +     if (fxp_min < (1 <<
>         pn_min) ||
>         >         (255 <<
>         >         >         pn_max) <
>         >         >         >         fxp_max) {
>         >         >
>         >         >
>         >         >
>         >         >         >         > +
>          DRM_DEBUG_KMS("VBT defined
>         >         backlight
>         >         >         frequency
>         >         >         >         out of range\n");
>         >         >         >         > +             return;
>         >         >         >         > +     }
>         >         >         >         > +
>         >         >         >         > +     for (pn = pn_max; pn >
>         pn_min;
>         >         pn--) {
>         >         >
>         >         >         Is there a reason this is not pn >=
>         pn_min?
>         >         > This is bug because f value will be incorrect in
>         the case
>         >         that pn ==
>         >         > pn_min.
>         >         > Thanks for catching this.
>         >         >
>         >
>         >
>         >         Isn't that a side-effect using the right shift
>         operation for
>         >         division?
>         > The bug is just for loop that exit too soon.
>         >
>         
>         Not sure I got that, what's the invalid "f" case that you see
>         with
>         pn==pn_min?
>         
>         
>         
> if the for loop has pn > pn_min, when pn == pn_min we will exit the
> loop first
> without running the  f = clamp(fxp >> pn, 1, 255); in the next line.
> 
> 
> This would fix with pn >= pn_min in for loop.
> 

Ah! I thought you were giving me a reason to not change it to pn >=
pn_min .


> 
> We guarantee that the break statement will be called and pn won't be
> pn_min - 1
> because we check (fxp_min < (1 << pn_min) || (255 << pn_max) <
> fxp_max) before
> entering this for loop.
> 
> 
>         
>         >         Would DIV_ROUND_CLOSEST allow you to use pn_min?
>         > It does not related to the point above, but
>         DIV_ROUND_CLOSEST still
>         > better than just using right shift. I will change to that in
>         next
>         > version.
>         >
>         >
>         >         >
>         >         >         >         > +             f = clamp(fxp >>
>         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");
>         >         >
>         >         >
>         >         >         If the number of brightness control bits
>         are
>         >         changing, the
>         >         >         max.
>         >         >         brightness value changes too.
>         >         >
>         >         >         Please see intel_dp_aux_setup_backlight().
>         >         >
>         >         >
>         >         > I think this is fine because
>         >         > - intel_dp_aux_setup_backlight() will
>         >         > called intel_dp_aux_enable_backlight() which
>         >         > called intel_dp_aux_set_pwm_freq() first before
>         determine
>         >         the max
>         >         > brightness value.
>         >         > - Also, the panel I tested does not change
>         >         > BACKLIGHT_BRIGHTNESS_BYTE_COUNT
>         >         >
>         >         > when changing the frequency.
>         >
>         >
>         >         The only values I see being set for max brightness
>         are 0xFFFF
>         >         and 0xFF
>         >
>         >         if (intel_dp->edp_dpcd[2] &
>         >         DP_EDP_BACKLIGHT_BRIGHTNESS_BYTE_COUNT)
>         >                 panel->backlight.max = 0xFFFF;
>         >         else
>         >                 panel->backlight.max = 0xFF;
>         >
>         >         I can't see where you are setting this based on Pn.
>         Can you
>         >         please point
>         >         out? From what I understand, max should be 2^Pn.
>         >
>         >
>         > It is suppose to be 0xFF or 0xFFFF only.
>         >
>         >
>         > From eDP spec:
>         > 0x702 bit 2 BACKLIGHT_BRIGHTNESS_BYTE_COUNT
>         > 0 = Indicates that the Sink device supports a 1-byte setting
>         for
>         > backlight brightness
>         > 1 = Indicates that the Sink device supports a 2-byte setting
>         for the
>         > backlight brightness
>         >
>         >
>         > 0x722 EDP_BACKLIGHT_BRIGHTNESS_MSB
>         > The actual number of assigned bits for the backlight
>         brightness PWM
>         > generator is set by
>         > field 4:0 of the EDP_PWMGEN_BIT_COUNT register (DPCD Address
>         00724h).
>         > Assigned bits are allocated to the MSB of the enabled
>         register
>         > combination
>         >
>         
>         ^This makes it somewhat clear but gets confusing again.
>         >
>         > This means that if PWM_GEN_BIT_COUNT is less than 16 then
>         the panel
>         > will ignore
>         > the least significant bit in EDP_BACKLIGHT_BRIGHTNESS
>         register.
>         >
>         >
>         > For example,
>         > if  BACKLIGHT_BRIGHTNESS_BYTE_COUNT == 1 and
>         PWM_GEN_BIT_COUNT = 16
>         > and EDP_BACKLIGHT_BRIGHTNESS == 0xabcd
>         > In this case, all bits in EDP_BACKLIGHT_BRIGHTNESS will be
>         used.
>         >
>         > 0xabcd means 0xabcd / 0xffff or (43981 / 65535) or 67.111%
>         >
>         >
>         > if  BACKLIGHT_BRIGHTNESS_BYTE_COUNT == 1 and
>         PWM_GEN_BIT_COUNT = 12
>         > and EDP_BACKLIGHT_BRIGHTNESS == 0xabcd
>         >
>         > In this case, the last 4 bits will be discarded.
>         > 0xabcd means 0xabc / 0xfff or 2748 / 4095 or 67.106%
>         >
>         >
>         > I think it should be fine to just have 8 or 16 bits of the
>         max
>         > brightness and let panel drop
>         > the unneed bit to simplify the driver code.
>         >
>         
>         Note:
>         The number of assigned or controllable bits will have the
>         ability to
>         provide full brightness control. Examples:
>         •
>          For 10-bit operation, 000h = 0% brightness, and 3FFh = 100%
>         brightness.
>         
>         Going by the logic you stated, for a 10-bit control, the MSB
>         and LSB
>         registers have to be written FFFFh to set 100% brightness with
>         the last
>         6 bits dropped off by the hardware.
>         
>         I don't like the way the spec leaves this to interpretation.
>         Have you
>         experimented with both 0x0ABC and 0xABCD for the 12-bit,
>         BYTE_COUNT==1
>         example you gave?
>         
> Yes, already test with the real panel.
> 
> 
> Here is better test case than 0xABCD
> 0xFFFF with 6, 12, 16 shows very bright screen with same brightness in
> all case. (All 100%)
> 0x0FFF with 6, 12, 16 bits shows dim screen. 6 bit case is slightly
> dimmer than other case.
> Note that the brightness are 4.76% / 6.23% / 6.25% in 6 / 12 / 16 bits
> case.
> 

Great! Thanks for confirming.

> You can apply this patch for easy testing
> https://patchwork.kernel.org/patch/6241481/
> 
>  
>         
>         >
>         >
>         >         Also, won't this function be called every time
>         >         _enable_backlight() is
>         >         called? Isn't doing this from setup sufficient? I
>         guess,
>         >         fixing it is an
>         >         optimization that'd be nice to have but not
>         necessary.
>         >
>         > Lets get this patch set done first. I can send in another
>         patch after
>         > this is go in to optimize this.
>         
>         Sure. This patch should be good to go with answers to my
>         questions. The
>         only thing remaining would be getting an ACK from Jani for
>         adding a new
>         parameter for dynamic backlight control.
>         
>         
>         -DK
>         
>         > Probably need to add new member in struct drm_i915_private
>         >
>         >         >
>         >         >
>         >         >         >         > +             return;
>         >         >         >         > +     }
>         >         >         >         > +     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;
>         >         >         >         > +     }
>         >         >         >         > +}
>         >         >         >         > +
>         >         >         >         >  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 new_dpcd_buf =
>         0;
>         >         >         >         >       uint8_t
>         edp_backlight_mode = 0;
>         >         >         >         > +     bool freq_cap;
>         >         >         >         >
>         >         >         >         >       if
>         >         (drm_dp_dpcd_readb(&intel_dp->aux,
>         >         >         >         >
>         >         >          DP_EDP_BACKLIGHT_MODE_SET_REGISTER,
>         >         >         >         &dpcd_buf) != 1) {
>         >         >         >         > @@ -151,6 +225,10 @@ static
>         void
>         >         >         >
>          intel_dp_aux_enable_backlight(struct
>         >         intel_connector
>         >         >         >         *connector)
>         >         >         >         >
>          DRM_DEBUG_KMS("Enable
>         >         dynamic
>         >         >         brightness.\n");
>         >         >         >         >       }
>         >         >         >         >
>         >         >         >         > +     freq_cap =
>         intel_dp->edp_dpcd[2] &
>         >         >         >
>          DP_EDP_BACKLIGHT_FREQ_AUX_SET_CAP;
>         >         >         >         > +     if (freq_cap)
>         >         >         >         > +             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) {
>         >         >         >         > @@ -158,6 +236,9 @@ static
>         void
>         >         >         >
>          intel_dp_aux_enable_backlight(struct
>         >         intel_connector
>         >         >         >         *connector)
>         >         >         >         >               }
>         >         >         >         >       }
>         >         >         >         >
>         >         >         >         > +     if (freq_cap)
>         >         >         >         > +
>         >         >          intel_dp_aux_set_pwm_freq(connector);
>         >         >         >         > +
>         >         >         >         >
>          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
>         >
>         >
>         >
>         >
>         > _______________________________________________
>         > 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

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

Re: [PATCH v7 9/9] drm/i915: Set PWM divider to match desired frequency in vbt

[Puthikorn Voravootivat]

[-- Attachment #1.1: Type: text/plain, Size: 29792 bytes --]

On Tue, May 16, 2017 at 2:21 PM, Pandiyan, Dhinakaran <
dhinakaran.pandiyan@intel.com> wrote:

> On Tue, 2017-05-16 at 13:56 -0700, Puthikorn Voravootivat wrote:
> >
> >
> > On Tue, May 16, 2017 at 1:29 PM, Pandiyan, Dhinakaran
> > <dhinakaran.pandiyan@intel.com> wrote:
> >         On Tue, 2017-05-16 at 11:07 -0700, Puthikorn Voravootivat
> >         wrote:
> >         >
> >         >
> >         > On Mon, May 15, 2017 at 11:21 PM, Pandiyan, Dhinakaran
> >         > <dhinakaran.pandiyan@intel.com> wrote:
> >         >         On Mon, 2017-05-15 at 17:43 -0700, Puthikorn
> >         Voravootivat
> >         >         wrote:
> >         >         >
> >         >         >
> >         >         > On Mon, May 15, 2017 at 4:07 PM, Pandiyan,
> >         Dhinakaran
> >         >         > <dhinakaran.pandiyan@intel.com> wrote:
> >         >         >         On Fri, 2017-05-12 at 17:31 -0700,
> >         Puthikorn
> >         >         Voravootivat
> >         >         >         wrote:
> >         >         >         >
> >         >         >         >
> >         >         >         >
> >         >         >         > On Fri, May 12, 2017 at 5:12 PM,
> >         Pandiyan,
> >         >         Dhinakaran
> >         >         >         > <dhinakaran.pandiyan@intel.com> wrote:
> >         >         >         >         On Thu, 2017-05-11 at 16:02
> >         -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 is still
> >         >         >         within
> >         >         >         >         25%
> >         >         >         >         > of the desired frequency.
> >         >         >         >
> >         >         >         >         I read a few eDP panel data
> >         sheets, the
> >         >         PWM
> >         >         >         frequencies all
> >         >         >         >         start from
> >         >         >         >         ~200Hz. If the VBT chooses this
> >         lowest
> >         >         value to
> >         >         >         allow for more
> >         >         >         >         brightness control, and then
> >         this patch
> >         >         lowers the
> >         >         >         value by
> >         >         >         >         another 25%,
> >         >         >         >         we'll end up below the panel
> >         allowed PWM
> >         >         frequency.
> >         >         >         >
> >         >         >         >         In fact, one of the systems I
> >         checked had
> >         >         PWM
> >         >         >         frequency as
> >         >         >         >         200Hz in VBT
> >         >         >         >         and the panel datasheet also had
> >         PWM
> >         >         frequency range
> >         >         >         starting
> >         >         >         >         from
> >         >         >         >         200Hz. Have you considered this
> >         case?
> >         >         >         >
> >         >         >         > The spec said "A given LCD panel
> >         typically has a
> >         >         limited
> >         >         >         range of
> >         >         >         > backlight frequency capability.
> >         >         >         > To limit the programmable frequency
> >         range,
> >         >         limitations are
> >         >         >         placed on
> >         >         >         > the allowable total divider ratio with
> >         the Sink
> >         >         device"
> >         >         >         >  So I think it should be auto cap to
> >         200Hz in this
> >         >         case.
> >         >         >         >
> >         >         >         >         -DK
> >         >         >         >         >
> >         >         >         >         > Signed-off-by: Puthikorn
> >         Voravootivat
> >         >         >         <puthik@chromium.org>
> >         >         >         >         > ---
> >         >         >         >         >
> >         >         drivers/gpu/drm/i915/intel_dp_aux_backlight.c |
> >         >         >         81
> >         >         >         >         +++++++++++++++++++++++++++
> >         >         >         >         >  1 file changed, 81
> >         insertions(+)
> >         >         >         >         >
> >         >         >         >         > diff --git
> >         >         >
> >          a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> >         >         >         >
> >         >          b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> >         >         >         >         > index
> >         0b48851013cc..6f10a2f1ab76 100644
> >         >         >         >         > ---
> >         >         >
> >          a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> >         >         >         >         > +++
> >         >         >
> >          b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> >         >         >         >         > @@ -113,12 +113,86 @@
> >         >         >         >
> >         >          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 void
> >         >         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;
> >         >         >         >         > +     }
> >         >         >         >         > +
> >         >         >         >         > +     fxp =
> >         >         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
> >         >         >         >         > +      * - Effective frequency
> >         is within
> >         >         25% of
> >         >         >         desired
> >         >         >         >         frequency.
> >         >         >         >         > +      */
> >         >         >         >         > +     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;
> >         >         >         >         > +     }
> >         >         >         >         > +     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;
> >         >         >         >         > +     }
> >         >         >         >         > +     pn_min &=
> >         >         DP_EDP_PWMGEN_BIT_COUNT_MASK;
> >         >         >         >         > +     pn_max &=
> >         >         DP_EDP_PWMGEN_BIT_COUNT_MASK;
> >         >         >         >         > +
> >         >         >         >         > +     fxp_min = fxp * 3 / 4;
> >         >         >         >         > +     fxp_max = fxp * 5 / 4;
> >         >         >         >
> >         >         >         >
> >         >         >         >         You are allowing fxp between +/-
> >         25% of
> >         >         the actual.
> >         >         >         This isn't
> >         >         >         >         same as
> >         >         >         >         the "Effective frequency is
> >         within 25% of
> >         >         desired
> >         >         >         frequency."
> >         >         >         >         right? The
> >         >         >         >         frequency can vary between -20%
> >         and +33%.
> >         >         >         >
> >         >         >         >
> >         >         >         > You are right.
> >         >         >         > You want me to change commit message to
> >         reflect
> >         >         this or
> >         >         >         change the
> >         >         >         > code to
> >         >         >         > match the commit message?
> >         >         >
> >         >         >
> >         >         >         I am okay with fixing the comment and
> >         commit
> >         >         message. Is the
> >         >         >         25%
> >         >         >         arbitrary or based on the distances
> >         between the
> >         >         possible
> >         >         >         values? Please
> >         >         >         make a note in the comment if it's the
> >         former.
> >         >         >
> >         >         >
> >         >         >         >         > +     if (fxp_min < (1 <<
> >         pn_min) ||
> >         >         (255 <<
> >         >         >         pn_max) <
> >         >         >         >         fxp_max) {
> >         >         >
> >         >         >
> >         >         >
> >         >         >         >         > +
> >          DRM_DEBUG_KMS("VBT defined
> >         >         backlight
> >         >         >         frequency
> >         >         >         >         out of range\n");
> >         >         >         >         > +             return;
> >         >         >         >         > +     }
> >         >         >         >         > +
> >         >         >         >         > +     for (pn = pn_max; pn >
> >         pn_min;
> >         >         pn--) {
> >         >         >
> >         >         >         Is there a reason this is not pn >=
> >         pn_min?
> >         >         > This is bug because f value will be incorrect in
> >         the case
> >         >         that pn ==
> >         >         > pn_min.
> >         >         > Thanks for catching this.
> >         >         >
> >         >
> >         >
> >         >         Isn't that a side-effect using the right shift
> >         operation for
> >         >         division?
> >         > The bug is just for loop that exit too soon.
> >         >
> >
> >         Not sure I got that, what's the invalid "f" case that you see
> >         with
> >         pn==pn_min?
> >
> >
> >
> > if the for loop has pn > pn_min, when pn == pn_min we will exit the
> > loop first
> > without running the  f = clamp(fxp >> pn, 1, 255); in the next line.
> >
> >
> > This would fix with pn >= pn_min in for loop.
> >
>
> Ah! I thought you were giving me a reason to not change it to pn >=
> pn_min .
>
>
> >
> > We guarantee that the break statement will be called and pn won't be
> > pn_min - 1
> > because we check (fxp_min < (1 << pn_min) || (255 << pn_max) <
> > fxp_max) before
> > entering this for loop.
> >
> >
> >
> >         >         Would DIV_ROUND_CLOSEST allow you to use pn_min?
> >         > It does not related to the point above, but
> >         DIV_ROUND_CLOSEST still
> >         > better than just using right shift. I will change to that in
> >         next
> >         > version.
> >         >
> >         >
> >         >         >
> >         >         >         >         > +             f = clamp(fxp >>
> >         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");
> >         >         >
> >         >         >
> >         >         >         If the number of brightness control bits
> >         are
> >         >         changing, the
> >         >         >         max.
> >         >         >         brightness value changes too.
> >         >         >
> >         >         >         Please see intel_dp_aux_setup_backlight().
> >         >         >
> >         >         >
> >         >         > I think this is fine because
> >         >         > - intel_dp_aux_setup_backlight() will
> >         >         > called intel_dp_aux_enable_backlight() which
> >         >         > called intel_dp_aux_set_pwm_freq() first before
> >         determine
> >         >         the max
> >         >         > brightness value.
> >         >         > - Also, the panel I tested does not change
> >         >         > BACKLIGHT_BRIGHTNESS_BYTE_COUNT
> >         >         >
> >         >         > when changing the frequency.
> >         >
> >         >
> >         >         The only values I see being set for max brightness
> >         are 0xFFFF
> >         >         and 0xFF
> >         >
> >         >         if (intel_dp->edp_dpcd[2] &
> >         >         DP_EDP_BACKLIGHT_BRIGHTNESS_BYTE_COUNT)
> >         >                 panel->backlight.max = 0xFFFF;
> >         >         else
> >         >                 panel->backlight.max = 0xFF;
> >         >
> >         >         I can't see where you are setting this based on Pn.
> >         Can you
> >         >         please point
> >         >         out? From what I understand, max should be 2^Pn.
> >         >
> >         >
> >         > It is suppose to be 0xFF or 0xFFFF only.
> >         >
> >         >
> >         > From eDP spec:
> >         > 0x702 bit 2 BACKLIGHT_BRIGHTNESS_BYTE_COUNT
> >         > 0 = Indicates that the Sink device supports a 1-byte setting
> >         for
> >         > backlight brightness
> >         > 1 = Indicates that the Sink device supports a 2-byte setting
> >         for the
> >         > backlight brightness
> >         >
> >         >
> >         > 0x722 EDP_BACKLIGHT_BRIGHTNESS_MSB
> >         > The actual number of assigned bits for the backlight
> >         brightness PWM
> >         > generator is set by
> >         > field 4:0 of the EDP_PWMGEN_BIT_COUNT register (DPCD Address
> >         00724h).
> >         > Assigned bits are allocated to the MSB of the enabled
> >         register
> >         > combination
> >         >
> >
> >         ^This makes it somewhat clear but gets confusing again.
> >         >
> >         > This means that if PWM_GEN_BIT_COUNT is less than 16 then
> >         the panel
> >         > will ignore
> >         > the least significant bit in EDP_BACKLIGHT_BRIGHTNESS
> >         register.
> >         >
> >         >
> >         > For example,
> >         > if  BACKLIGHT_BRIGHTNESS_BYTE_COUNT == 1 and
> >         PWM_GEN_BIT_COUNT = 16
> >         > and EDP_BACKLIGHT_BRIGHTNESS == 0xabcd
> >         > In this case, all bits in EDP_BACKLIGHT_BRIGHTNESS will be
> >         used.
> >         >
> >         > 0xabcd means 0xabcd / 0xffff or (43981 / 65535) or 67.111%
> >         >
> >         >
> >         > if  BACKLIGHT_BRIGHTNESS_BYTE_COUNT == 1 and
> >         PWM_GEN_BIT_COUNT = 12
> >         > and EDP_BACKLIGHT_BRIGHTNESS == 0xabcd
> >         >
> >         > In this case, the last 4 bits will be discarded.
> >         > 0xabcd means 0xabc / 0xfff or 2748 / 4095 or 67.106%
> >         >
> >         >
> >         > I think it should be fine to just have 8 or 16 bits of the
> >         max
> >         > brightness and let panel drop
> >         > the unneed bit to simplify the driver code.
> >         >
> >
> >         Note:
> >         The number of assigned or controllable bits will have the
> >         ability to
> >         provide full brightness control. Examples:
> >         •
> >          For 10-bit operation, 000h = 0% brightness, and 3FFh = 100%
> >         brightness.
> >
> >         Going by the logic you stated, for a 10-bit control, the MSB
> >         and LSB
> >         registers have to be written FFFFh to set 100% brightness with
> >         the last
> >         6 bits dropped off by the hardware.
> >
> >         I don't like the way the spec leaves this to interpretation.
> >         Have you
> >         experimented with both 0x0ABC and 0xABCD for the 12-bit,
> >         BYTE_COUNT==1
> >         example you gave?
> >
> > Yes, already test with the real panel.
> >
> >
> > Here is better test case than 0xABCD
> > 0xFFFF with 6, 12, 16 shows very bright screen with same brightness in
> > all case. (All 100%)
> > 0x0FFF with 6, 12, 16 bits shows dim screen. 6 bit case is slightly
> > dimmer than other case.
> > Note that the brightness are 4.76% / 6.23% / 6.25% in 6 / 12 / 16 bits
> > case.
> >
>
> Great! Thanks for confirming.
>
> > You can apply this patch for easy testing
> > https://patchwork.kernel.org/patch/6241481/
> >
> >
> >
> >         >
> >         >
> >         >         Also, won't this function be called every time
> >         >         _enable_backlight() is
> >         >         called? Isn't doing this from setup sufficient? I
> >         guess,
>
I looked at this a bit and look like we need to call this every time
_enable() is called.
One panel that I have reset the DP_EDP_PWMGEN_BIT_COUNT (pn) to the lowest
value
every time panel backlight is turned off. So doing this from setup is
insufficient.


> >         >         fixing it is an
> >         >         optimization that'd be nice to have but not
> >         necessary.
> >         >
> >         > Lets get this patch set done first. I can send in another
> >         patch after
> >         > this is go in to optimize this.
> >
> >         Sure. This patch should be good to go with answers to my
> >         questions. The
> >         only thing remaining would be getting an ACK from Jani for
> >         adding a new
> >         parameter for dynamic backlight control.
> >
> >
> >         -DK
> >
> >         > Probably need to add new member in struct drm_i915_private
> >         >
> >         >         >
> >         >         >
> >         >         >         >         > +             return;
> >         >         >         >         > +     }
> >         >         >         >         > +     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;
> >         >         >         >         > +     }
> >         >         >         >         > +}
> >         >         >         >         > +
> >         >         >         >         >  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 new_dpcd_buf =
> >         0;
> >         >         >         >         >       uint8_t
> >         edp_backlight_mode = 0;
> >         >         >         >         > +     bool freq_cap;
> >         >         >         >         >
> >         >         >         >         >       if
> >         >         (drm_dp_dpcd_readb(&intel_dp->aux,
> >         >         >         >         >
> >         >         >          DP_EDP_BACKLIGHT_MODE_SET_REGISTER,
> >         >         >         >         &dpcd_buf) != 1) {
> >         >         >         >         > @@ -151,6 +225,10 @@ static
> >         void
> >         >         >         >
> >          intel_dp_aux_enable_backlight(struct
> >         >         intel_connector
> >         >         >         >         *connector)
> >         >         >         >         >
> >          DRM_DEBUG_KMS("Enable
> >         >         dynamic
> >         >         >         brightness.\n");
> >         >         >         >         >       }
> >         >         >         >         >
> >         >         >         >         > +     freq_cap =
> >         intel_dp->edp_dpcd[2] &
> >         >         >         >
> >          DP_EDP_BACKLIGHT_FREQ_AUX_SET_CAP;
> >         >         >         >         > +     if (freq_cap)
> >         >         >         >         > +             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) {
> >         >         >         >         > @@ -158,6 +236,9 @@ static
> >         void
> >         >         >         >
> >          intel_dp_aux_enable_backlight(struct
> >         >         intel_connector
> >         >         >         >         *connector)
> >         >         >         >         >               }
> >         >         >         >         >       }
> >         >         >         >         >
> >         >         >         >         > +     if (freq_cap)
> >         >         >         >         > +
> >         >         >          intel_dp_aux_set_pwm_freq(connector);
> >         >         >         >         > +
> >         >         >         >         >
> >          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
> >         >
> >         >
> >         >
> >         >
> >         > _______________________________________________
> >         > 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
>
>

[-- Attachment #1.2: Type: text/html, Size: 47005 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

Re: [PATCH v7 9/9] drm/i915: Set PWM divider to match desired frequency in vbt

[Pandiyan, Dhinakaran]

On Tue, 2017-05-16 at 17:39 -0700, Puthikorn Voravootivat wrote:
> 
> 
> On Tue, May 16, 2017 at 2:21 PM, Pandiyan, Dhinakaran
> <dhinakaran.pandiyan@intel.com> wrote:
>         On Tue, 2017-05-16 at 13:56 -0700, Puthikorn Voravootivat
>         wrote:
>         >
>         >
>         > On Tue, May 16, 2017 at 1:29 PM, Pandiyan, Dhinakaran
>         > <dhinakaran.pandiyan@intel.com> wrote:
>         >         On Tue, 2017-05-16 at 11:07 -0700, Puthikorn
>         Voravootivat
>         >         wrote:
>         >         >
>         >         >
>         >         > On Mon, May 15, 2017 at 11:21 PM, Pandiyan,
>         Dhinakaran
>         >         > <dhinakaran.pandiyan@intel.com> wrote:
>         >         >         On Mon, 2017-05-15 at 17:43 -0700,
>         Puthikorn
>         >         Voravootivat
>         >         >         wrote:
>         >         >         >
>         >         >         >
>         >         >         > On Mon, May 15, 2017 at 4:07 PM,
>         Pandiyan,
>         >         Dhinakaran
>         >         >         > <dhinakaran.pandiyan@intel.com> wrote:
>         >         >         >         On Fri, 2017-05-12 at 17:31
>         -0700,
>         >         Puthikorn
>         >         >         Voravootivat
>         >         >         >         wrote:
>         >         >         >         >
>         >         >         >         >
>         >         >         >         >
>         >         >         >         > On Fri, May 12, 2017 at 5:12
>         PM,
>         >         Pandiyan,
>         >         >         Dhinakaran
>         >         >         >         >
>         <dhinakaran.pandiyan@intel.com> wrote:
>         >         >         >         >         On Thu, 2017-05-11 at
>         16:02
>         >         -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 is still
>         >         >         >         within
>         >         >         >         >         25%
>         >         >         >         >         > of the desired
>         frequency.
>         >         >         >         >
>         >         >         >         >         I read a few eDP panel
>         data
>         >         sheets, the
>         >         >         PWM
>         >         >         >         frequencies all
>         >         >         >         >         start from
>         >         >         >         >         ~200Hz. If the VBT
>         chooses this
>         >         lowest
>         >         >         value to
>         >         >         >         allow for more
>         >         >         >         >         brightness control,
>         and then
>         >         this patch
>         >         >         lowers the
>         >         >         >         value by
>         >         >         >         >         another 25%,
>         >         >         >         >         we'll end up below the
>         panel
>         >         allowed PWM
>         >         >         frequency.
>         >         >         >         >
>         >         >         >         >         In fact, one of the
>         systems I
>         >         checked had
>         >         >         PWM
>         >         >         >         frequency as
>         >         >         >         >         200Hz in VBT
>         >         >         >         >         and the panel
>         datasheet also had
>         >         PWM
>         >         >         frequency range
>         >         >         >         starting
>         >         >         >         >         from
>         >         >         >         >         200Hz. Have you
>         considered this
>         >         case?
>         >         >         >         >
>         >         >         >         > The spec said "A given LCD
>         panel
>         >         typically has a
>         >         >         limited
>         >         >         >         range of
>         >         >         >         > backlight frequency
>         capability.
>         >         >         >         > To limit the programmable
>         frequency
>         >         range,
>         >         >         limitations are
>         >         >         >         placed on
>         >         >         >         > the allowable total divider
>         ratio with
>         >         the Sink
>         >         >         device"
>         >         >         >         >  So I think it should be auto
>         cap to
>         >         200Hz in this
>         >         >         case.
>         >         >         >         >
>         >         >         >         >         -DK
>         >         >         >         >         >
>         >         >         >         >         > Signed-off-by:
>         Puthikorn
>         >         Voravootivat
>         >         >         >         <puthik@chromium.org>
>         >         >         >         >         > ---
>         >         >         >         >         >
>         >         >
>          drivers/gpu/drm/i915/intel_dp_aux_backlight.c |
>         >         >         >         81
>         >         >         >         >
>          +++++++++++++++++++++++++++
>         >         >         >         >         >  1 file changed, 81
>         >         insertions(+)
>         >         >         >         >         >
>         >         >         >         >         > diff --git
>         >         >         >
>         >          a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
>         >         >         >         >
>         >         >
>         b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
>         >         >         >         >         > index
>         >         0b48851013cc..6f10a2f1ab76 100644
>         >         >         >         >         > ---
>         >         >         >
>         >          a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
>         >         >         >         >         > +++
>         >         >         >
>         >          b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
>         >         >         >         >         > @@ -113,12 +113,86
>         @@
>         >         >         >         >
>         >         >
>         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 void
>         >         >         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;
>         >         >         >         >         > +     }
>         >         >         >         >         > +
>         >         >         >         >         > +     fxp =
>         >         >         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
>         >         >         >         >         > +      * - Effective
>         frequency
>         >         is within
>         >         >         25% of
>         >         >         >         desired
>         >         >         >         >         frequency.
>         >         >         >         >         > +      */
>         >         >         >         >         > +     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;
>         >         >         >         >         > +     }
>         >         >         >         >         > +     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;
>         >         >         >         >         > +     }
>         >         >         >         >         > +     pn_min &=
>         >         >         DP_EDP_PWMGEN_BIT_COUNT_MASK;
>         >         >         >         >         > +     pn_max &=
>         >         >         DP_EDP_PWMGEN_BIT_COUNT_MASK;
>         >         >         >         >         > +
>         >         >         >         >         > +     fxp_min = fxp
>         * 3 / 4;
>         >         >         >         >         > +     fxp_max = fxp
>         * 5 / 4;
>         >         >         >         >
>         >         >         >         >
>         >         >         >         >         You are allowing fxp
>         between +/-
>         >         25% of
>         >         >         the actual.
>         >         >         >         This isn't
>         >         >         >         >         same as
>         >         >         >         >         the "Effective
>         frequency is
>         >         within 25% of
>         >         >         desired
>         >         >         >         frequency."
>         >         >         >         >         right? The
>         >         >         >         >         frequency can vary
>         between -20%
>         >         and +33%.
>         >         >         >         >
>         >         >         >         >
>         >         >         >         > You are right.
>         >         >         >         > You want me to change commit
>         message to
>         >         reflect
>         >         >         this or
>         >         >         >         change the
>         >         >         >         > code to
>         >         >         >         > match the commit message?
>         >         >         >
>         >         >         >
>         >         >         >         I am okay with fixing the
>         comment and
>         >         commit
>         >         >         message. Is the
>         >         >         >         25%
>         >         >         >         arbitrary or based on the
>         distances
>         >         between the
>         >         >         possible
>         >         >         >         values? Please
>         >         >         >         make a note in the comment if
>         it's the
>         >         former.
>         >         >         >
>         >         >         >
>         >         >         >         >         > +     if (fxp_min <
>         (1 <<
>         >         pn_min) ||
>         >         >         (255 <<
>         >         >         >         pn_max) <
>         >         >         >         >         fxp_max) {
>         >         >         >
>         >         >         >
>         >         >         >
>         >         >         >         >         > +
>         >          DRM_DEBUG_KMS("VBT defined
>         >         >         backlight
>         >         >         >         frequency
>         >         >         >         >         out of range\n");
>         >         >         >         >         > +
>          return;
>         >         >         >         >         > +     }
>         >         >         >         >         > +
>         >         >         >         >         > +     for (pn =
>         pn_max; pn >
>         >         pn_min;
>         >         >         pn--) {
>         >         >         >
>         >         >         >         Is there a reason this is not pn
>         >=
>         >         pn_min?
>         >         >         > This is bug because f value will be
>         incorrect in
>         >         the case
>         >         >         that pn ==
>         >         >         > pn_min.
>         >         >         > Thanks for catching this.
>         >         >         >
>         >         >
>         >         >
>         >         >         Isn't that a side-effect using the right
>         shift
>         >         operation for
>         >         >         division?
>         >         > The bug is just for loop that exit too soon.
>         >         >
>         >
>         >         Not sure I got that, what's the invalid "f" case
>         that you see
>         >         with
>         >         pn==pn_min?
>         >
>         >
>         >
>         > if the for loop has pn > pn_min, when pn == pn_min we will
>         exit the
>         > loop first
>         > without running the  f = clamp(fxp >> pn, 1, 255); in the
>         next line.
>         >
>         >
>         > This would fix with pn >= pn_min in for loop.
>         >
>         
>         
>         Ah! I thought you were giving me a reason to not change it to
>         pn >=
>         pn_min .
>         
>         
>         >
>         > We guarantee that the break statement will be called and pn
>         won't be
>         > pn_min - 1
>         > because we check (fxp_min < (1 << pn_min) || (255 << pn_max)
>         <
>         > fxp_max) before
>         > entering this for loop.
>         >
>         >
>         >
>         >         >         Would DIV_ROUND_CLOSEST allow you to use
>         pn_min?
>         >         > It does not related to the point above, but
>         >         DIV_ROUND_CLOSEST still
>         >         > better than just using right shift. I will change
>         to that in
>         >         next
>         >         > version.
>         >         >
>         >         >
>         >         >         >
>         >         >         >         >         > +             f =
>         clamp(fxp >>
>         >         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");
>         >         >         >
>         >         >         >
>         >         >         >         If the number of brightness
>         control bits
>         >         are
>         >         >         changing, the
>         >         >         >         max.
>         >         >         >         brightness value changes too.
>         >         >         >
>         >         >         >         Please see
>         intel_dp_aux_setup_backlight().
>         >         >         >
>         >         >         >
>         >         >         > I think this is fine because
>         >         >         > - intel_dp_aux_setup_backlight() will
>         >         >         > called intel_dp_aux_enable_backlight()
>         which
>         >         >         > called intel_dp_aux_set_pwm_freq() first
>         before
>         >         determine
>         >         >         the max
>         >         >         > brightness value.
>         >         >         > - Also, the panel I tested does not
>         change
>         >         >         > BACKLIGHT_BRIGHTNESS_BYTE_COUNT
>         >         >         >
>         >         >         > when changing the frequency.
>         >         >
>         >         >
>         >         >         The only values I see being set for max
>         brightness
>         >         are 0xFFFF
>         >         >         and 0xFF
>         >         >
>         >         >         if (intel_dp->edp_dpcd[2] &
>         >         >         DP_EDP_BACKLIGHT_BRIGHTNESS_BYTE_COUNT)
>         >         >                 panel->backlight.max = 0xFFFF;
>         >         >         else
>         >         >                 panel->backlight.max = 0xFF;
>         >         >
>         >         >         I can't see where you are setting this
>         based on Pn.
>         >         Can you
>         >         >         please point
>         >         >         out? From what I understand, max should be
>         2^Pn.
>         >         >
>         >         >
>         >         > It is suppose to be 0xFF or 0xFFFF only.
>         >         >
>         >         >
>         >         > From eDP spec:
>         >         > 0x702 bit 2 BACKLIGHT_BRIGHTNESS_BYTE_COUNT
>         >         > 0 = Indicates that the Sink device supports a
>         1-byte setting
>         >         for
>         >         > backlight brightness
>         >         > 1 = Indicates that the Sink device supports a
>         2-byte setting
>         >         for the
>         >         > backlight brightness
>         >         >
>         >         >
>         >         > 0x722 EDP_BACKLIGHT_BRIGHTNESS_MSB
>         >         > The actual number of assigned bits for the
>         backlight
>         >         brightness PWM
>         >         > generator is set by
>         >         > field 4:0 of the EDP_PWMGEN_BIT_COUNT register
>         (DPCD Address
>         >         00724h).
>         >         > Assigned bits are allocated to the MSB of the
>         enabled
>         >         register
>         >         > combination
>         >         >
>         >
>         >         ^This makes it somewhat clear but gets confusing
>         again.
>         >         >
>         >         > This means that if PWM_GEN_BIT_COUNT is less than
>         16 then
>         >         the panel
>         >         > will ignore
>         >         > the least significant bit in
>         EDP_BACKLIGHT_BRIGHTNESS
>         >         register.
>         >         >
>         >         >
>         >         > For example,
>         >         > if  BACKLIGHT_BRIGHTNESS_BYTE_COUNT == 1 and
>         >         PWM_GEN_BIT_COUNT = 16
>         >         > and EDP_BACKLIGHT_BRIGHTNESS == 0xabcd
>         >         > In this case, all bits in EDP_BACKLIGHT_BRIGHTNESS
>         will be
>         >         used.
>         >         >
>         >         > 0xabcd means 0xabcd / 0xffff or (43981 / 65535) or
>         67.111%
>         >         >
>         >         >
>         >         > if  BACKLIGHT_BRIGHTNESS_BYTE_COUNT == 1 and
>         >         PWM_GEN_BIT_COUNT = 12
>         >         > and EDP_BACKLIGHT_BRIGHTNESS == 0xabcd
>         >         >
>         >         > In this case, the last 4 bits will be discarded.
>         >         > 0xabcd means 0xabc / 0xfff or 2748 / 4095 or
>         67.106%
>         >         >
>         >         >
>         >         > I think it should be fine to just have 8 or 16
>         bits of the
>         >         max
>         >         > brightness and let panel drop
>         >         > the unneed bit to simplify the driver code.
>         >         >
>         >
>         >         Note:
>         >         The number of assigned or controllable bits will
>         have the
>         >         ability to
>         >         provide full brightness control. Examples:
>         >         •
>         >          For 10-bit operation, 000h = 0% brightness, and
>         3FFh = 100%
>         >         brightness.
>         >
>         >         Going by the logic you stated, for a 10-bit control,
>         the MSB
>         >         and LSB
>         >         registers have to be written FFFFh to set 100%
>         brightness with
>         >         the last
>         >         6 bits dropped off by the hardware.
>         >
>         >         I don't like the way the spec leaves this to
>         interpretation.
>         >         Have you
>         >         experimented with both 0x0ABC and 0xABCD for the
>         12-bit,
>         >         BYTE_COUNT==1
>         >         example you gave?
>         >
>         > Yes, already test with the real panel.
>         >
>         >
>         > Here is better test case than 0xABCD
>         > 0xFFFF with 6, 12, 16 shows very bright screen with same
>         brightness in
>         > all case. (All 100%)
>         > 0x0FFF with 6, 12, 16 bits shows dim screen. 6 bit case is
>         slightly
>         > dimmer than other case.
>         > Note that the brightness are 4.76% / 6.23% / 6.25% in 6 /
>         12 / 16 bits
>         > case.
>         >
>         
>         
>         Great! Thanks for confirming.
>         
>         > You can apply this patch for easy testing
>         > https://patchwork.kernel.org/patch/6241481/
>         >
>         >
>         >
>         >         >
>         >         >
>         >         >         Also, won't this function be called every
>         time
>         >         >         _enable_backlight() is
>         >         >         called? Isn't doing this from setup
>         sufficient? I
>         >         guess,
>         
> I looked at this a bit and look like we need to call this every time
> _enable() is called.
> One panel that I have reset the DP_EDP_PWMGEN_BIT_COUNT (pn) to the
> lowest value
> every time panel backlight is turned off. So doing this from setup is
> insufficient.


Thanks for checking. I think, we can still avoid doing these every time 
a) DPCD reads for PWM_BIT_COUNT_CAP_{MAX,MIN} 
b) recomputing pn and f 

-DK

>  
>         >         >         fixing it is an
>         >         >         optimization that'd be nice to have but
>         not
>         >         necessary.
>         >         >
>         >         > Lets get this patch set done first. I can send in
>         another
>         >         patch after
>         >         > this is go in to optimize this.
>         >
>         >         Sure. This patch should be good to go with answers
>         to my
>         >         questions. The
>         >         only thing remaining would be getting an ACK from
>         Jani for
>         >         adding a new
>         >         parameter for dynamic backlight control.
>         >
>         >
>         >         -DK
>         >
>         >         > Probably need to add new member in struct
>         drm_i915_private
>         >         >
>         >         >         >
>         >         >         >
>         >         >         >         >         > +
>          return;
>         >         >         >         >         > +     }
>         >         >         >         >         > +     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;
>         >         >         >         >         > +     }
>         >         >         >         >         > +}
>         >         >         >         >         > +
>         >         >         >         >         >  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
>         new_dpcd_buf =
>         >         0;
>         >         >         >         >         >       uint8_t
>         >         edp_backlight_mode = 0;
>         >         >         >         >         > +     bool freq_cap;
>         >         >         >         >         >
>         >         >         >         >         >       if
>         >         >         (drm_dp_dpcd_readb(&intel_dp->aux,
>         >         >         >         >         >
>         >         >         >
>         DP_EDP_BACKLIGHT_MODE_SET_REGISTER,
>         >         >         >         >         &dpcd_buf) != 1) {
>         >         >         >         >         > @@ -151,6 +225,10 @@
>         static
>         >         void
>         >         >         >         >
>         >          intel_dp_aux_enable_backlight(struct
>         >         >         intel_connector
>         >         >         >         >         *connector)
>         >         >         >         >         >
>         >          DRM_DEBUG_KMS("Enable
>         >         >         dynamic
>         >         >         >         brightness.\n");
>         >         >         >         >         >       }
>         >         >         >         >         >
>         >         >         >         >         > +     freq_cap =
>         >         intel_dp->edp_dpcd[2] &
>         >         >         >         >
>         >          DP_EDP_BACKLIGHT_FREQ_AUX_SET_CAP;
>         >         >         >         >         > +     if (freq_cap)
>         >         >         >         >         > +
>          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) {
>         >         >         >         >         > @@ -158,6 +236,9 @@
>         static
>         >         void
>         >         >         >         >
>         >          intel_dp_aux_enable_backlight(struct
>         >         >         intel_connector
>         >         >         >         >         *connector)
>         >         >         >         >         >               }
>         >         >         >         >         >       }
>         >         >         >         >         >
>         >         >         >         >         > +     if (freq_cap)
>         >         >         >         >         > +
>         >         >         >
>         intel_dp_aux_set_pwm_freq(connector);
>         >         >         >         >         > +
>         >         >         >         >         >
>         >          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
>         >         >
>         >         >
>         >         >
>         >         >
>         >         > _______________________________________________
>         >         > 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
>         
>         
> 
> 
> _______________________________________________
> 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

✓ Fi.CI.BAT: success for Enhancement to intel_dp_aux_backlight driver (rev6)

[Patchwork]

== Series Details ==

Series: Enhancement to intel_dp_aux_backlight driver (rev6)
URL   : https://patchwork.freedesktop.org/series/21086/
State : success

== Summary ==

Series 21086v6 Enhancement to intel_dp_aux_backlight driver
https://patchwork.freedesktop.org/api/1.0/series/21086/revisions/6/mbox/

Test gem_exec_suspend:
        Subgroup basic-s4-devices:
                pass       -> DMESG-WARN (fi-kbl-7560u) fdo#100125

fdo#100125 https://bugs.freedesktop.org/show_bug.cgi?id=100125

fi-bdw-5557u     total:278  pass:267  dwarn:0   dfail:0   fail:0   skip:11  time:439s
fi-bdw-gvtdvm    total:278  pass:256  dwarn:8   dfail:0   fail:0   skip:14  time:430s
fi-bsw-n3050     total:278  pass:242  dwarn:0   dfail:0   fail:0   skip:36  time:589s
fi-bxt-j4205     total:278  pass:259  dwarn:0   dfail:0   fail:0   skip:19  time:515s
fi-bxt-t5700     total:278  pass:258  dwarn:0   dfail:0   fail:0   skip:20  time:558s
fi-byt-j1900     total:278  pass:254  dwarn:0   dfail:0   fail:0   skip:24  time:500s
fi-byt-n2820     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time:493s
fi-hsw-4770      total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time:415s
fi-hsw-4770r     total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time:410s
fi-ilk-650       total:278  pass:228  dwarn:0   dfail:0   fail:0   skip:50  time:423s
fi-ivb-3520m     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time:496s
fi-ivb-3770      total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time:479s
fi-kbl-7500u     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time:465s
fi-kbl-7560u     total:278  pass:267  dwarn:1   dfail:0   fail:0   skip:10  time:632s
fi-skl-6260u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time:461s
fi-skl-6700hq    total:278  pass:261  dwarn:0   dfail:0   fail:0   skip:17  time:578s
fi-skl-6700k     total:278  pass:256  dwarn:4   dfail:0   fail:0   skip:18  time:470s
fi-skl-6770hq    total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time:497s
fi-skl-gvtdvm    total:278  pass:265  dwarn:0   dfail:0   fail:0   skip:13  time:441s
fi-snb-2520m     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time:538s
fi-snb-2600      total:278  pass:249  dwarn:0   dfail:0   fail:0   skip:29  time:413s

51350ea90d1a5c8bb7e6532eb61c64a57e83ab9b drm-tip: 2017y-05m-11d-13h-05m-06s UTC integration manifest
7b72965 drm/i915: Set PWM divider to match desired frequency in vbt
1020108 drm: Add definition for eDP backlight frequency
b52777c drm/i915: Restore brightness level in aux backlight driver
8409812 drm/i915: Add option to support dynamic backlight via DPCD
1ede9a2 drm/i915: Set backlight mode before enable backlight
f596728 drm/i915: Allow choosing how to adjust brightness if both supported
41a4958 drm/i915: Drop AUX backlight enable check for backlight control
d34cb86 drm/i915: Correctly enable backlight brightness adjustment via DPCD
1e5dac5 drm/i915: Fix cap check for intel_dp_aux_backlight driver

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4678/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx