On Fri, 14 Jun 2019, Jani Nikula <jani.nik...@intel.com> wrote:
>On Thu, 13 Jun 2019, "Lee, Shawn C" <shawn.c....@intel.com> wrote:
>> Follow generla eDP backlight enable control sequence. Add T8 (valid
>> video data to backlight enable) delay before turn backlight_enable on.
>> And T9 (backlight disable to end of valida video data) delay after
>> backlight_enable off.
>
>There are two things that are wrong here:
>
>First, you're adding *two* waits on the DP AUX backlight enable and disable
>paths. IIUC there is no reason to wait between setting the level and enabling
>here. The waits have already been done on intel_dp.c level.
>
>Second, the last_power_on, backlight_on_delay, last_backlight_off, and
>backlight_off_delay members of intel_dp should all be private to intel_dp.c.
>Indeed these waits even have wrappers *within* intel_dp.c so that only very
>specific functions in intel_dp.c ever look at these members. This is a huge
>red flag.
>
>What's the problem you're trying to solve? Does this fix something
>*actual* that you're seeing?
>
>BR,
>Jani.
This change will call on/off delay twice. It is a mistake.
I check the driver again. To use the original on/off delay in
_intel_edp_backlight_on() and _intel_edp_backlight_off() are enough.
And we should bypass backlight_enable on/off control by soc if aux backlight
control active. Right?
One more question about the off flow. In this case, backlight_enable will be
off at backlight.disable() function.
But wait delay was executed at _intel_edp_backlight_off() before
backlight.disable().
We are trying to enable a panel with DP aux backlight control. The modification
in patch #1 to correct the sequence and make it works.
But seems we should modify the delay time to fit panel's spec just like what we
did for general eDP panel.
Best regards,
Shawn
>
>>
>> Cc: Ville Syrjälä <ville.syrj...@linux.intel.com>
>> Cc: Jani Nikula <jani.nik...@intel.com>
>> Cc: Jose Roberto de Souza <jose.so...@intel.com>
>> Cc: Cooper Chiou <cooper.ch...@intel.com>
>>
>> Signed-off-by: Lee, Shawn C <shawn.c....@intel.com>
>> ---
>> drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 13 +++++++++++++
>> 1 file changed, 13 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
>> b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
>> index 98210ae17285..b008e887f4e9 100644
>> --- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
>> +++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
>> @@ -217,12 +217,25 @@ static void intel_dp_aux_enable_backlight(const struct
>> intel_crtc_state *crtc_st
>> }
>>
>> intel_dp_aux_set_backlight(conn_state,
>> connector->panel.backlight.level);
>> +
>> + wait_remaining_ms_from_jiffies(intel_dp->last_power_on,
>> + intel_dp->backlight_on_delay);
>> +
>> set_aux_backlight_enable(intel_dp, true); }
>>
>> static void intel_dp_aux_disable_backlight(const struct
>> drm_connector_state *old_conn_state) {
>> + struct intel_connector *connector =
>> to_intel_connector(old_conn_state->connector);
>> + struct intel_dp *intel_dp =
>> +enc_to_intel_dp(&connector->encoder->base);
>> +
>> + intel_dp->last_backlight_off = jiffies;
>> +
>>
>> set_aux_backlight_enable(enc_to_intel_dp(old_conn_state->best_encoder)
>> , false);
>> +
>> + wait_remaining_ms_from_jiffies(intel_dp->last_backlight_off,
>> + intel_dp->backlight_off_delay);
>> +
>> intel_dp_aux_set_backlight(old_conn_state, 0); }
>
>--
>Jani Nikula, Intel Open Source Graphics Center
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
