Re: [PATCH] drm/i915: Refine eDP aux backlight enable/disable sequence

Jani Nikula
2019-06-25T15:10:31Z

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
>>

--
Jani Nikula, Intel Open Source Graphics Center