From mboxrd@z Thu Jan 1 00:00:00 1970 From: Clint Taylor Subject: Re: [PATCH 02/14] drm/i915: Reorganize vlv eDP reboot notifier Date: Thu, 04 Sep 2014 10:47:41 -0700 Message-ID: <5408A5BD.8060309@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> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1"; Format="flowed" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by gabe.freedesktop.org (Postfix) with ESMTP id 9F33B6E5F4 for ; Thu, 4 Sep 2014 10:49:03 -0700 (PDT) In-Reply-To: <20140826140644.GM4193@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?= , Daniel Vetter Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org 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 a= dds >>>>> unnecessary churn. >>>> >>>> I'd just add another reboot notifier then. >>> >>> Fair enough; it should be vlv_edp_notify_handler then. (No, don't send 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 docs? >>> >>> *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 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 tab= le 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 people >> where unhappy with that enforced delay and it was ditched on vlv, but th= en >> 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 the > 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 that > 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? 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. "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. 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. Clint