| Message ID | 1560492565-30405-2-git-send-email-shawn.c.lee@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] drm/i915: Refine eDP aux backlight enable/disable sequence | expand |
On Thu, 13 Jun 2019, "Lee, Shawn C" <shawn.c.lee@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. > > 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/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); > }
On Fri, 14 Jun 2019, Jani Nikula <jani.nikula@intel.com> wrote: >On Thu, 13 Jun 2019, "Lee, Shawn C" <shawn.c.lee@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.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/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 >
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); }
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. 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/intel_dp_aux_backlight.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)