From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH 02/14] drm/i915: Reorganize vlv eDP reboot notifier Date: Fri, 5 Sep 2014 11:23:19 +0300 Message-ID: <20140905082319.GQ4193@intel.com> References: <1408389369-22898-1-git-send-email-ville.syrjala@linux.intel.com> <1408389369-22898-3-git-send-email-ville.syrjala@linux.intel.com> <87egwdhsk8.fsf@intel.com> <20140826125851.GK4193@intel.com> <8761hfl743.fsf@intel.com> <20140826133607.GY15520@phenom.ffwll.local> <20140826140644.GM4193@intel.com> <5408A5BD.8060309@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by gabe.freedesktop.org (Postfix) with ESMTP id 2ED3F6E7E6 for ; Fri, 5 Sep 2014 01:23:23 -0700 (PDT) Content-Disposition: inline In-Reply-To: <5408A5BD.8060309@intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Clint Taylor Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Thu, Sep 04, 2014 at 10:47:41AM -0700, Clint Taylor wrote: > On 08/26/2014 07:06 AM, Ville Syrj=E4l=E4 wrote: > > On Tue, Aug 26, 2014 at 03:36:07PM +0200, Daniel Vetter wrote: > >> On Tue, Aug 26, 2014 at 04:21:00PM +0300, Jani Nikula wrote: > >>> On Tue, 26 Aug 2014, Ville Syrj=E4l=E4 wrote: > >>>> On Tue, Aug 19, 2014 at 10:00:55AM +0300, Jani Nikula wrote: > >>>>> On Mon, 18 Aug 2014, ville.syrjala@linux.intel.com wrote: > >>>>>> From: Ville Syrj=E4l=E4 > >>>>>> > >>>>>> Move the vlv_power_sequencer_pipe() after the IS_VALLEYVIEW() check > >>>>>> and flatten the rest of the function. > >>>>> > >>>>> Please imagine adding another platform there, and realize this just= adds > >>>>> unnecessary churn. > >>>> > >>>> I'd just add another reboot notifier then. > >>> > >>> Fair enough; it should be vlv_edp_notify_handler then. (No, don't sen= d a > >>> patch to change that! ;) > >>> > >>>> Frankly I don't understand the current one either. Why does it need = to > >>>> set the delay to max for instance? And does this mean that the > >>>> PANEL_POWER_RESET bit doesn't actually work as advertised in the doc= s? > >>> > >>> *shrug* experimental evidence? > >>> > >>> commit 01527b3127997ef6370d5ad4fa25d96847fbf12a > >>> Author: Clint Taylor > >>> Date: Mon Jul 7 13:01:46 2014 -0700 > >>> > >>> drm/i915/vlv: T12 eDP panel timing enforcement during reboot > >>> > >>> The panel power sequencer on vlv doesn't appear to accept change= s 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 t= able for > >>> the connected panel. > >> > >> So if I remember this piece of lore correctly in the past the pp was > >> pessimistic, and enforced this delay on resume/boot-up, assuming you've > >> shut down _right_ before the machine was lit up again. Apparently peop= le > >> where unhappy with that enforced delay and it was ditched on vlv, but = then > >> it broke panels if you actually managed to reboot quickly enough. > > > > IIRC the way the reset bit is documented the hardware itself is supposed > > to initiate the power off cycle when it gets some reset notification and > > it should enforce the timing before allowing the panel power to be > > re-enabled. Although it does seem that it would also reset the > > "power cycle delay" so it would maybe only enforce some default delay in > > that case (300ms based on the documented default value of 0x4). So if t= he > > panel requires more than the 300ms then I understand the msleep() here. > > I guess use of the VDD force bit just after reset might also require th= at > > we do the power down + wait before reset. So that part does make sense > > to me, but I still don't understand the "power cycle delay"=3D0x1f part. > > > All eDP panels require at least 500ms according to the eDP = > specifications T12 minimum of 500ms. The panel manufacturer's = > specifications I have seen also have a 500ms minimum. Is the default = > register value of 4 relevant for LVDS? IIRC I saw 400ms mentioned for LVDS somewhere, so I have no idea why someone made the default 300ms. Not that I actaully verified that the reset valus is indeed 4 on actual hardware, I just read it from the spec. > From our testing on VLV the PPS starts immediately upon ungate of the = > display block. There is no way to stop or alter this sequence that = > starts with the default value (4) in the register. The panel power will = > assert at the end of the sequence. Without the msleep() adding time a = > quick rebooting platform will not meet the 500ms minimum. Ok so I guess in theory we could do msleep(whatever-300) and we should still meet the panel timings, assuming we care to optimize a few hunder ms here. > = > "power cycle delay" of 0x1f was added to prevent LCD_VDD from asserting = > before the reboot actually completes which includes the msleep() time. = > The "power cycle delay" value could be computed based on the T12 time in = > the VBT. Just setting the highest value made sense for simplicity and = > the fact the value is reset to default during the reboot. At that point we should have the correct t11_t12 delay programmed into the register already. Also it shouldn't really matter what we have in there because of the msleep(). By the time the msleep() is done and we reboot, the register gets reset to defaul anyway, and it should be OK to enable VDD again, even immediately rather than after the default 300ms since we already slept for the entire power cycle delay before rebooting. > The "PANEL_POWER_RESET bit doesn't actually work as advertised" = > statement was probably in error based on my understanding of the PPS at = > the time the patch was made. OK, so I guess it sort of works then, except since it resets the power cycle delay to 300ms it waits for less than it should. So yeah seems we do need the manual sleep to make sure we don't violate the panel timings. I guess we should then add the reboot notifier for all platforms since this power sequencer reset behaviour isn't specific to VLV. -- = Ville Syrj=E4l=E4 Intel OTC