From mboxrd@z Thu Jan 1 00:00:00 1970 From: Clint Taylor Subject: Re: [PATCH] drm/i915/vlv: T12 eDP panel timing enforcement during reboot Date: Mon, 07 Jul 2014 10:19:27 -0700 Message-ID: <53BAD69F.2040800@intel.com> References: <87vbrgfcas.fsf@intel.com> <1404290153-26487-1-git-send-email-jani.nikula@intel.com> <53B5D422.7030608@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by gabe.freedesktop.org (Postfix) with ESMTP id E8E296E426 for ; Mon, 7 Jul 2014 10:20:21 -0700 (PDT) In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Paulo Zanoni Cc: Jani Nikula , Intel Graphics Development , "Barnes, Jesse" List-Id: intel-gfx@lists.freedesktop.org On 07/04/2014 05:26 AM, Paulo Zanoni wrote: > 2014-07-03 19:07 GMT-03:00 Clint Taylor : >> On 07/02/2014 07:40 AM, Paulo Zanoni wrote: >>> >>> 2014-07-02 5:35 GMT-03:00 Jani Nikula : >>>> >>>> From: Clint Taylor >>>> >>>> The panel power sequencer on vlv doesn't appear to accept changes to its >>>> T12 power down duration during warm reboots. This change forces a delay >>>> for warm reboots to the T12 panel timing as defined in the VBT table for >>>> the connected panel. >>>> >>>> Ver2: removed redundant pr_crit(), commented magic value for pp_div_reg >>>> >>>> Ver3: moved SYS_RESTART check earlier, new name for pp_div. >>>> >>>> Ver4: Minor issue changes >>>> >>>> Signed-off-by: Clint Taylor >>>> [Jani: rebased on current -nightly.] >>>> Signed-off-by: Jani Nikula >>>> >>>> --- >>>> >>>> I ended up doing the rebase myself, but I'd like to have a quick review >>>> before pushing. >>>> >>>> Thanks, >>>> Jani. >>>> --- >>>> drivers/gpu/drm/i915/intel_dp.c | 40 >>>> ++++++++++++++++++++++++++++++++++++++++ >>>> drivers/gpu/drm/i915/intel_drv.h | 2 ++ >>>> 2 files changed, 42 insertions(+) >>>> >>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c >>>> b/drivers/gpu/drm/i915/intel_dp.c >>>> index b5ec48913b47..f0d23c435cf6 100644 >>>> --- a/drivers/gpu/drm/i915/intel_dp.c >>>> +++ b/drivers/gpu/drm/i915/intel_dp.c >>>> @@ -28,6 +28,8 @@ >>>> #include >>>> #include >>>> #include >>>> +#include >>>> +#include >>>> #include >>>> #include >>>> #include >>>> @@ -336,6 +338,36 @@ static u32 _pp_stat_reg(struct intel_dp *intel_dp) >>>> return >>>> VLV_PIPE_PP_STATUS(vlv_power_sequencer_pipe(intel_dp)); >>>> } >>>> >>>> +/* Reboot notifier handler to shutdown panel power to guarantee T12 >>>> timing */ >>>> +static int edp_notify_handler(struct notifier_block *this, unsigned long >>>> code, >>>> + void *unused) >>>> +{ >>>> + struct intel_dp *intel_dp = container_of(this, typeof(* >>>> intel_dp), >>>> + edp_notifier); >>>> + struct drm_device *dev = intel_dp_to_dev(intel_dp); >>>> + struct drm_i915_private *dev_priv = dev->dev_private; >>>> + u32 pp_div; >>>> + u32 pp_ctrl_reg, pp_div_reg; >>>> + enum pipe pipe = vlv_power_sequencer_pipe(intel_dp); >>>> + >>>> + if (!is_edp(intel_dp) || code != SYS_RESTART) >>> >>> >>> What if someone does a power off and _very quickly_ starts the system >>> again? =P >>> >>> >> If someone removes and applies power within ~300ms this W/A will break down >> and the power sequence will not meet the eDP T12 timing. Since the PPS >> sequencer does not allow modifications to the default time intervals during >> the initial sequence the only way to guarantee we meet T12 time would be to >> delay display block power ungate for 300ms. Further delay of the boot time >> was not an acceptable solution for the customers. >> > > My suggestion here was just to not-return in more cases, instead of > only SYS_RESTART. #define SYS_DOWN 0x0001 /* Notify of system down */ #define SYS_RESTART SYS_DOWN #define SYS_HALT 0x0002 /* Notify of system halt */ #define SYS_POWER_OFF 0x0003 /* Notify of system power off */ The only real option would be just to ignore the code parameter as the only other values are full power offs. > > >> >>> Also, depending based on the suggestions below, you may not need the >>> is_edp() check (or you may want to convert it to a WARN). >>> >>>> + return 0; >>>> + >>>> + if (IS_VALLEYVIEW(dev)) { >>> >>> >>> This check is not really needed. It could also be an early return or a >>> WARN. >> >> >> Since we currently don't handle PCH offsets this was a safe way to allowing >> adding of the PCH functionality later if necessary. >> >>> >>> >>>> + pp_ctrl_reg = VLV_PIPE_PP_CONTROL(pipe); >>>> + pp_div_reg = VLV_PIPE_PP_DIVISOR(pipe); >>>> + pp_div = I915_READ(VLV_PIPE_PP_DIVISOR(pipe)); >>> >>> >>> Or "pp_div = I915_READ(pp_div_reg);", since we just defined it :) >> >> >> Agreed that's another way to do the same thing. >> >> >>> >>> >>>> + pp_div &= PP_REFERENCE_DIVIDER_MASK; >>>> + >>>> + /* 0x1F write to PP_DIV_REG sets max cycle delay */ >>>> + I915_WRITE(pp_div_reg, pp_div | 0x1F); >>>> + I915_WRITE(pp_ctrl_reg, PANEL_UNLOCK_REGS | >>>> PANEL_POWER_OFF); >>> >>> >>> So this is basically turning the panel off and turning the "force VDD" >>> bit off too, and we're not putting any power domain references back. >>> Even though this might not be a big problem since we're shutting down >>> the machine anyway, did we attach a serial cable and check if this >>> won't print any WARNs while shutting down? Shouldn't we try to make >>> this function call the other functions that shut down stuff instead of >>> forcing the panel down here? >> >> >> Development of this W/A was done with serial port attached. This function is >> the last method called in the I915 driver before power is removed. At reboot >> notifier time there is no error handling possible calling the normal >> shutdown functions does more housekeeping then we need for a system that >> will not have power in < 2 ms. > > For this code, even if we don't change it, I think we should at least > put a comment here describing this is an "acceptable" solution for a > machine shutdown, but that this code should not be reused in other > cases since we're forcing a panel shutdown without respecting the PM > references or using the standard ways of waiting for the timers. > Programmers from the future love code refactors, and I fear they may > start using this function for more cases than the current one, so the > comment may prevent future bugs. > > >> >> >>> >>> >>>> + msleep(intel_dp->panel_power_cycle_delay); >>>> + } >>>> + >>>> + return 0; >>>> +} >>>> + >>>> static bool edp_have_panel_power(struct intel_dp *intel_dp) >>>> { >>>> struct drm_device *dev = intel_dp_to_dev(intel_dp); >>>> @@ -3785,6 +3817,10 @@ void intel_dp_encoder_destroy(struct drm_encoder >>>> *encoder) >>>> drm_modeset_lock(&dev->mode_config.connection_mutex, >>>> NULL); >>>> edp_panel_vdd_off_sync(intel_dp); >>>> drm_modeset_unlock(&dev->mode_config.connection_mutex); >>>> + if (intel_dp->edp_notifier.notifier_call) { >>>> + >>>> unregister_reboot_notifier(&intel_dp->edp_notifier); >>>> + intel_dp->edp_notifier.notifier_call = NULL; >>>> + } >>>> } >>>> kfree(intel_dig_port); >>>> } >>>> @@ -4353,6 +4389,10 @@ intel_dp_init_connector(struct intel_digital_port >>>> *intel_dig_port, >>>> if (is_edp(intel_dp)) { >>>> intel_dp_init_panel_power_timestamps(intel_dp); >>>> intel_dp_init_panel_power_sequencer(dev, intel_dp, >>>> &power_seq); >>>> + if (IS_VALLEYVIEW(dev)) { >>>> + intel_dp->edp_notifier.notifier_call = >>>> edp_notify_handler; >>>> + >>>> register_reboot_notifier(&intel_dp->edp_notifier); >>> >>> >>> Why not put this inside intel_edp_init_connector? If you keep it here, >>> you also have to undo the notifier at the point >>> intel_dp_init_connector returns false (a few lines below). If you move >>> to the _edp version, then it depends on where inside >>> _edp_init_connector you put this.. >>> >> Agreed that if the device does not have DPCD and a ghost is created this >> notifier would need to be unregistered. >> >> >>> >>>> + } >>>> } >>>> >>>> intel_dp_aux_init(intel_dp, intel_connector); >>>> diff --git a/drivers/gpu/drm/i915/intel_drv.h >>>> b/drivers/gpu/drm/i915/intel_drv.h >>>> index 5f7c7bd94d90..87d1715db21d 100644 >>>> --- a/drivers/gpu/drm/i915/intel_drv.h >>>> +++ b/drivers/gpu/drm/i915/intel_drv.h >>>> @@ -541,6 +541,8 @@ struct intel_dp { >>>> unsigned long last_power_cycle; >>>> unsigned long last_power_on; >>>> unsigned long last_backlight_off; >>>> + struct notifier_block edp_notifier; >>>> + >>>> bool use_tps3; >>>> struct intel_connector *attached_connector; >>>> >>>> -- >>>> 2.0.0 >>>> >>>> _______________________________________________ >>>> Intel-gfx mailing list >>>> Intel-gfx@lists.freedesktop.org >>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx >>> >>> >>> >>> >> > > >