diff mbox series

drm/i915: Refine eDP aux backlight enable/disable sequence

Message ID 1561087486-19339-1-git-send-email-shawn.c.lee@intel.com (mailing list archive)
State New, archived
Headers show
Series drm/i915: Refine eDP aux backlight enable/disable sequence | expand

Commit Message

Lee, Shawn C June 21, 2019, 3:24 a.m. UTC
Modify aux backlight enable/disable sequence just like what we
did for genernal eDP panel.
1. Setup PWM freq and brightness level before enable display backlight.
2. Set PWM to 0 after backlight enable was off.

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Jani Nikula <jani.nikula@intel.com>
Cc: Jose Roberto de Souza <jose.souza@intel.com>
Cc: Cooper Chiou <cooper.chiou@intel.com>

Signed-off-by: Lee Shawn C <shawn.c.lee@intel.com>
---
 drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Jani Nikula June 25, 2019, 2:11 p.m. UTC | #1
On Thu, 20 Jun 2019, Lee Shawn C <shawn.c.lee@intel.com> wrote:
> Modify aux backlight enable/disable sequence just like what we
> did for genernal eDP panel.
> 1. Setup PWM freq and brightness level before enable display backlight.
> 2. Set PWM to 0 after backlight enable was off.

It took me a while to realize why we do this in this order.

eDP v1.4b says about DPCD addresses 00722h-00723h
i.e. EDP_BACKLIGHT_BRIGHTNESS MSB and LSB:

"""
Writes are ignored if:

[...]

- BACKLIGHT_ENABLE bit in the EDP_DISPLAY_CONTROL register (DPCD Address
  00720h, bit 0) is cleared to 0"
"""

i.e. per the spec this change makes the brightness updates no-ops.

Do you have a panel where this patch makes a difference?

BR,
Jani.


>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Cc: Jose Roberto de Souza <jose.souza@intel.com>
> Cc: Cooper Chiou <cooper.chiou@intel.com>
>
> Signed-off-by: Lee Shawn C <shawn.c.lee@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
> index 6b0b73479fb8..bbc579734238 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
> @@ -216,13 +216,14 @@ static void intel_dp_aux_enable_backlight(const struct intel_crtc_state *crtc_st
>  		}
>  	}
>  
> -	set_aux_backlight_enable(intel_dp, true);
>  	intel_dp_aux_set_backlight(conn_state, connector->panel.backlight.level);
> +	set_aux_backlight_enable(intel_dp, true);
>  }
>  
>  static void intel_dp_aux_disable_backlight(const struct drm_connector_state *old_conn_state)
>  {
>  	set_aux_backlight_enable(enc_to_intel_dp(old_conn_state->best_encoder), false);
> +	intel_dp_aux_set_backlight(old_conn_state, 0);
>  }
>  
>  static int intel_dp_aux_setup_backlight(struct intel_connector *connector,
Lee, Shawn C June 25, 2019, 2:49 p.m. UTC | #2
On Tue, 25 Jun 2019, Jani Nikula <jani.nikula@intel.com> wrote:
>On Thu, 20 Jun 2019, Lee Shawn C <shawn.c.lee@intel.com> wrote:
>> Modify aux backlight enable/disable sequence just like what we did for 
>> genernal eDP panel.
>> 1. Setup PWM freq and brightness level before enable display backlight.
>> 2. Set PWM to 0 after backlight enable was off.
>
>It took me a while to realize why we do this in this order.
>
>eDP v1.4b says about DPCD addresses 00722h-00723h i.e. EDP_BACKLIGHT_BRIGHTNESS MSB and LSB:
>
>"""
>Writes are ignored if:
>
>[...]
>
>- BACKLIGHT_ENABLE bit in the EDP_DISPLAY_CONTROL register (DPCD Address
>  00720h, bit 0) is cleared to 0"
>"""
>
>i.e. per the spec this change makes the brightness updates no-ops.
>
>Do you have a panel where this patch makes a difference?
>
>BR,
>Jani.
>

What you say is correct. According to spec, write EDP_BACKLIGHT_BRIGHTNESS does not affect
if BACKLIGHT_ENABLE already be off.

Backlight enable always turn off before stop PWM output when used general eDP backlight (from SOC) control.
But AUX backlight control is a little different. I will update this patch and remove "Set PWM to 0".

>>
>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Cc: Jani Nikula <jani.nikula@intel.com>
>> Cc: Jose Roberto de Souza <jose.souza@intel.com>
>> Cc: Cooper Chiou <cooper.chiou@intel.com>
>>
>> Signed-off-by: Lee Shawn C <shawn.c.lee@intel.com>
>> ---
>>  drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c 
>> b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
>> index 6b0b73479fb8..bbc579734238 100644
>> --- a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
>> +++ b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
>> @@ -216,13 +216,14 @@ static void intel_dp_aux_enable_backlight(const struct intel_crtc_state *crtc_st
>>  		}
>>  	}
>>  
>> -	set_aux_backlight_enable(intel_dp, true);
>>  	intel_dp_aux_set_backlight(conn_state, 
>> connector->panel.backlight.level);
>> +	set_aux_backlight_enable(intel_dp, true);
>>  }
>>  
>>  static void intel_dp_aux_disable_backlight(const struct 
>> drm_connector_state *old_conn_state)  {
>>  	
>> set_aux_backlight_enable(enc_to_intel_dp(old_conn_state->best_encoder)
>> , false);
>> +	intel_dp_aux_set_backlight(old_conn_state, 0);
>>  }
>>  
>>  static int intel_dp_aux_setup_backlight(struct intel_connector 
>> *connector,
>
>--
>Jani Nikula, Intel Open Source Graphics Center
>
Jani Nikula June 25, 2019, 3:12 p.m. UTC | #3
On Tue, 25 Jun 2019, "Lee, Shawn C" <shawn.c.lee@intel.com> wrote:
> On Tue, 25 Jun 2019, Jani Nikula <jani.nikula@intel.com> wrote:
>>On Thu, 20 Jun 2019, Lee Shawn C <shawn.c.lee@intel.com> wrote:
>>> Modify aux backlight enable/disable sequence just like what we did for 
>>> genernal eDP panel.
>>> 1. Setup PWM freq and brightness level before enable display backlight.
>>> 2. Set PWM to 0 after backlight enable was off.
>>
>>It took me a while to realize why we do this in this order.
>>
>>eDP v1.4b says about DPCD addresses 00722h-00723h i.e. EDP_BACKLIGHT_BRIGHTNESS MSB and LSB:
>>
>>"""
>>Writes are ignored if:
>>
>>[...]
>>
>>- BACKLIGHT_ENABLE bit in the EDP_DISPLAY_CONTROL register (DPCD Address
>>  00720h, bit 0) is cleared to 0"
>>"""
>>
>>i.e. per the spec this change makes the brightness updates no-ops.
>>
>>Do you have a panel where this patch makes a difference?
>>
>>BR,
>>Jani.
>>
>
> What you say is correct. According to spec, write EDP_BACKLIGHT_BRIGHTNESS does not affect
> if BACKLIGHT_ENABLE already be off.
>
> Backlight enable always turn off before stop PWM output when used general eDP backlight (from SOC) control.
> But AUX backlight control is a little different. I will update this patch and remove "Set PWM to 0".

AFAICT the only thing you can do, really, is calling
intel_dp_aux_set_backlight(old_conn_state, 0); *before*
set_aux_backlight_enable(enc_to_intel_dp(old_conn_state->best_encoder),
false); in intel_dp_aux_disable_backlight().

BR,
Jani.



>
>>>
>>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>> Cc: Jani Nikula <jani.nikula@intel.com>
>>> Cc: Jose Roberto de Souza <jose.souza@intel.com>
>>> Cc: Cooper Chiou <cooper.chiou@intel.com>
>>>
>>> Signed-off-by: Lee Shawn C <shawn.c.lee@intel.com>
>>> ---
>>>  drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c | 3 ++-
>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c 
>>> b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
>>> index 6b0b73479fb8..bbc579734238 100644
>>> --- a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
>>> +++ b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
>>> @@ -216,13 +216,14 @@ static void intel_dp_aux_enable_backlight(const struct intel_crtc_state *crtc_st
>>>  		}
>>>  	}
>>>  
>>> -	set_aux_backlight_enable(intel_dp, true);
>>>  	intel_dp_aux_set_backlight(conn_state, 
>>> connector->panel.backlight.level);
>>> +	set_aux_backlight_enable(intel_dp, true);
>>>  }
>>>  
>>>  static void intel_dp_aux_disable_backlight(const struct 
>>> drm_connector_state *old_conn_state)  {
>>>  	
>>> set_aux_backlight_enable(enc_to_intel_dp(old_conn_state->best_encoder)
>>> , false);
>>> +	intel_dp_aux_set_backlight(old_conn_state, 0);
>>>  }
>>>  
>>>  static int intel_dp_aux_setup_backlight(struct intel_connector 
>>> *connector,
>>
>>--
>>Jani Nikula, Intel Open Source Graphics Center
>>
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
index 6b0b73479fb8..bbc579734238 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
@@ -216,13 +216,14 @@  static void intel_dp_aux_enable_backlight(const struct intel_crtc_state *crtc_st
 		}
 	}
 
-	set_aux_backlight_enable(intel_dp, true);
 	intel_dp_aux_set_backlight(conn_state, connector->panel.backlight.level);
+	set_aux_backlight_enable(intel_dp, true);
 }
 
 static void intel_dp_aux_disable_backlight(const struct drm_connector_state *old_conn_state)
 {
 	set_aux_backlight_enable(enc_to_intel_dp(old_conn_state->best_encoder), false);
+	intel_dp_aux_set_backlight(old_conn_state, 0);
 }
 
 static int intel_dp_aux_setup_backlight(struct intel_connector *connector,