From mboxrd@z Thu Jan 1 00:00:00 1970 From: Clint Taylor Subject: Re: [PATCH] drm/i915/dp: Backlight PWM enable before BL Enable assert Date: Thu, 21 Aug 2014 10:23:43 -0700 Message-ID: <53F62B1F.2060605@intel.com> References: <1408394915-21882-1-git-send-email-clinton.a.taylor@intel.com> <20140820112323.GF4193@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1"; Format="flowed" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mga03.intel.com (mga03.intel.com [143.182.124.21]) by gabe.freedesktop.org (Postfix) with ESMTP id 003046E226 for ; Thu, 21 Aug 2014 10:24:47 -0700 (PDT) In-Reply-To: <20140820112323.GF4193@intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: =?ISO-8859-1?Q?Ville_Syrj=E4l=E4?= Cc: Intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On 08/20/2014 04:23 AM, Ville Syrj=E4l=E4 wrote: > On Mon, Aug 18, 2014 at 01:48:35PM -0700, clinton.a.taylor@intel.com wrot= e: >> From: Clint Taylor >> >> Backlight on delay uses PWM enable time to seperate PWM to >> backlight enable assert. Previous time difference used timing >> from VDD enable which occur several seconds before resulting >> in PWM starting 5ms after backlight enable. Changes to backlight >> duty cycle take affect at the end of the current PWM cycle. >> Measured time for the PWM cycle is 5ms. 5 additional ms must be >> added to the backlight_on_delay to get correct VBT timing of >> PWM to backlight enable assert. >> >> V2: Rebase to Jani Nikula backlight power patch 1/4 >> >> Change-Id: I2982f9785d92e8d64c04ca25c8bd4c5d1dc8067c >> Signed-off-by: Clint Taylor >> --- >> drivers/gpu/drm/i915/intel_dp.c | 6 ++++-- >> drivers/gpu/drm/i915/intel_drv.h | 1 + >> 2 files changed, 5 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/inte= l_dp.c >> index d8baf60..aed923b 100644 >> --- a/drivers/gpu/drm/i915/intel_dp.c >> +++ b/drivers/gpu/drm/i915/intel_dp.c >> @@ -1141,7 +1141,7 @@ static void wait_panel_power_cycle(struct intel_dp= *intel_dp) >> >> static void wait_backlight_on(struct intel_dp *intel_dp) >> { >> - wait_remaining_ms_from_jiffies(intel_dp->last_power_on, >> + wait_remaining_ms_from_jiffies(intel_dp->last_backlight_on, >> intel_dp->backlight_on_delay); >> } >> >> @@ -1418,6 +1418,7 @@ void intel_edp_backlight_on(struct intel_dp *intel= _dp) >> DRM_DEBUG_KMS("\n"); >> >> intel_panel_enable_backlight(intel_dp->attached_connector); >> + intel_dp->last_backlight_on =3D jiffies; > > Seems to me we should just have a msleep(PWM_CYCLE_DELAY) here since > (assuming I understood correctly) only the PWM cycle time is important > between these two steps, and the "t8" (vbt spec says t7 actually) is > relevant only from end of link training to backlight on. > The driver has overloaded both T8 and T9 VBT timing entries. The eDP = specification really doesn't have timing data defined for this = requirement, but the panel manufacturers have it in their panel = specification and they use various names like (Te, t17, etc...) for this = delay. > Hmm, no actually your idea might be better since on VLV/CHV we turn the > pipe on after link training, and I assume t7/t8 should really start > counting from the pipe on on those platforms. So I guess the most > appropriate solution might be somehting like > > { > intel_dp->last_backlight_on =3D jiffies; > intel_panel_enable_backlight(intel_dp->attached_connector); > msleep(POWER_CYCLE_DELAY); > _intel_edp_backlight_on(intel_dp); > } > > But I'm not sure the distinction makes any difference since > intel_panel_enable_backlight() shouldn't take a significant amount of > time, and msleep() is itself rather inaccurate. > There is also a need to add this delay when turning off the PWM enable = bit through intel_panel_disable_backlight(). If the PWM enable bit is = turned off while the PWM signal is high then the signal remains high. If = the bit is turned off when the signal is low the PWM will remain low. = Since we don't know the current state of the PWM signal we must set the = duty_cycle to 0, wait PWM_CYCLE_DELAY, and then turn off the enable bit. Actually it might be better if we never turn off the PWM enable bit in = intel_panel_disable_backlight() and we just use the duty cycle register = to control the PWM. > So I guess your approach is the simplest option here. > >> _intel_edp_backlight_on(intel_dp); >> } >> >> @@ -4256,9 +4257,10 @@ intel_dp_init_panel_power_sequencer(struct drm_de= vice *dev, >> assign_final(t11_t12); >> #undef assign_final >> >> +#define PWM_CYCLE_DELAY 5 > > Shoduln't this be calculated from the PWM frequqncy? Not sure what a PWM > cycle here is exactly. Just one full period of the signal? > The PWM cycle in this case turns out to be 1 full cycle of the PWM = waveform though it depends on which display core clock (128 or = 25Mhz(S0ix) is being used. Since we only care about the minimum elapsed = time to meet the panel specification a value of 5ms will work even = though more or less PWM cycles would take place before the BL_ENABLE is = asserted. I would prefer not to add complexity to switch between panel = timings for normal and S0ix display clock modes. How often does the = backlight get enabled while in S0ix? > Also would be nice to have a comment in the code explaining what this is > and why we need to add it to the delay. Sure, As you may have noticed in other patches I really like comments. > >> #define get_delay(field) (DIV_ROUND_UP(final.field, 10)) >> intel_dp->panel_power_up_delay =3D get_delay(t1_t3); >> - intel_dp->backlight_on_delay =3D get_delay(t8); >> + intel_dp->backlight_on_delay =3D get_delay(t8) + PWM_CYCLE_DELAY; >> intel_dp->backlight_off_delay =3D get_delay(t9); >> intel_dp->panel_power_down_delay =3D get_delay(t10); >> intel_dp->panel_power_cycle_delay =3D get_delay(t11_t12); >> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/int= el_drv.h >> index 3abc915..ad6fcc1 100644 >> --- a/drivers/gpu/drm/i915/intel_drv.h >> +++ b/drivers/gpu/drm/i915/intel_drv.h >> @@ -556,6 +556,7 @@ struct intel_dp { >> bool want_panel_vdd; >> unsigned long last_power_cycle; >> unsigned long last_power_on; >> + unsigned long last_backlight_on; >> unsigned long last_backlight_off; >> >> struct notifier_block edp_notifier; >> -- >> 1.8.3.2 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx >