| Message ID | 1560404842-13583-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 sequence. | expand |
On Wed, Jun 12, 2019 at 10:47:22PM -0700, Lee, Shawn C wrote: > Modify aux backlight enable sequence just like what we > did for genernal eDP panel. > 1. Setup PWM freq and brightness level before enable display backlight. > 2. Add T8 (valid data to backlight enable) delay. If we respect the on_delay shouldn't we then respect the off_delay too? In which case I suggest two patches: 1. swaap the enable vs. set_brightness order 2. add the on/off delays > > 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 | 6 +++++- > 1 file changed, 5 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 7ded95a334db..c112e0b930de 100644 > --- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c > +++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c > @@ -216,8 +216,12 @@ 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); > + > + 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) > -- > 2.7.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Thu, 13 Jun 2019, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > On Wed, Jun 12, 2019 at 10:47:22PM -0700, Lee, Shawn C wrote: >> Modify aux backlight enable sequence just like what we >> did for genernal eDP panel. >> 1. Setup PWM freq and brightness level before enable display backlight. >> 2. Add T8 (valid data to backlight enable) delay. > > If we respect the on_delay shouldn't we then respect the off_delay too? > > In which case I suggest two patches: > 1. swaap the enable vs. set_brightness order > 2. add the on/off delays The aux backlight functions are called via the same hooks as everything in intel_panel.c, which do not have delays (they're all on the calling side in intel_dp.c) so I'm confused why these would require the delays. BR, Jani. > >> >> 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 | 6 +++++- >> 1 file changed, 5 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 7ded95a334db..c112e0b930de 100644 >> --- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c >> +++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c >> @@ -216,8 +216,12 @@ 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); >> + >> + 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) >> -- >> 2.7.4 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Thu, 13 Jun 2019, Jani Nikula <jani.nikula@intel.com> wrote: >On Thu, 13 Jun 2019, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: >> On Wed, Jun 12, 2019 at 10:47:22PM -0700, Lee, Shawn C wrote: >>> Modify aux backlight enable sequence just like what we did for >>> genernal eDP panel. >>> 1. Setup PWM freq and brightness level before enable display backlight. >>> 2. Add T8 (valid data to backlight enable) delay. >> >> If we respect the on_delay shouldn't we then respect the off_delay too? >> >> In which case I suggest two patches: >> 1. swaap the enable vs. set_brightness order 2. add the on/off delays > >The aux backlight functions are called via the same hooks as everything in intel_panel.c, which do not have delays (they're all on the calling side in intel_dp.c) so I'm confused why these would require the delays. > >BR, >Jani. Refer to intel_edp_backlight_on(). PWM was restored at the end of intel_panel_enable_backlight(). After that, driver would wait for some delay wait_backlight_on() before enable backlight. Thanks for Ville's recommendation. I will send separate for this change. Best regards, Shawn >>> >>> 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 | 6 +++++- >>> 1 file changed, 5 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 7ded95a334db..c112e0b930de 100644 >>> --- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c >>> +++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c >>> @@ -216,8 +216,12 @@ 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); >>> + >>> + 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) >>> -- >>> 2.7.4 >>> >>> _______________________________________________ >>> Intel-gfx mailing list >>> Intel-gfx@lists.freedesktop.org >>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx > >-- >Jani Nikula, Intel Open Source Graphics Center
On Fri, 14 Jun 2019, "Lee, Shawn C" <shawn.c.lee@intel.com> wrote: > On Thu, 13 Jun 2019, Jani Nikula <jani.nikula@intel.com> wrote: >>On Thu, 13 Jun 2019, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: >>> On Wed, Jun 12, 2019 at 10:47:22PM -0700, Lee, Shawn C wrote: >>>> Modify aux backlight enable sequence just like what we did for >>>> genernal eDP panel. >>>> 1. Setup PWM freq and brightness level before enable display backlight. >>>> 2. Add T8 (valid data to backlight enable) delay. >>> >>> If we respect the on_delay shouldn't we then respect the off_delay too? >>> >>> In which case I suggest two patches: >>> 1. swaap the enable vs. set_brightness order 2. add the on/off delays >> >>The aux backlight functions are called via the same hooks as everything in intel_panel.c, which do not have delays (they're all on the calling side in intel_dp.c) so I'm confused why these would require the delays. >> >>BR, >>Jani. > > Refer to intel_edp_backlight_on(). PWM was restored at the end of > intel_panel_enable_backlight(). After that, driver would wait for > some delay wait_backlight_on() before enable backlight. You need to be more specific. I repeat: When you have DPCD AUX backlight in use, it gets enabled via intel_panel_enable_backlight(). All the delays for backlight (regardless of whether it's DDI PWM or DPCD AUX) are already handled in intel_dp.c. What is the exact problem you're trying to solve? BR, Jani. > > Thanks for Ville's recommendation. I will send separate for this change. > > Best regards, > Shawn > >>>> >>>> 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 | 6 +++++- >>>> 1 file changed, 5 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 7ded95a334db..c112e0b930de 100644 >>>> --- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c >>>> +++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c >>>> @@ -216,8 +216,12 @@ 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); >>>> + >>>> + 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) >>>> -- >>>> 2.7.4 >>>> >>>> _______________________________________________ >>>> Intel-gfx mailing list >>>> Intel-gfx@lists.freedesktop.org >>>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx >> >>-- >>Jani Nikula, Intel Open Source Graphics Center > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c index 7ded95a334db..c112e0b930de 100644 --- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c +++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c @@ -216,8 +216,12 @@ 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); + + 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)
Modify aux backlight enable sequence just like what we did for genernal eDP panel. 1. Setup PWM freq and brightness level before enable display backlight. 2. Add T8 (valid data to backlight enable) delay. 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 | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)