From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH 00/35] drm/i915: ILK+ watermark rewrite Date: Fri, 5 Jul 2013 20:54:46 +0300 Message-ID: <20130705175446.GI5004@intel.com> References: <1373014667-19484-1-git-send-email-ville.syrjala@linux.intel.com> <20130705172200.GF5004@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by gabe.freedesktop.org (Postfix) with ESMTP id AE38CE5C36 for ; Fri, 5 Jul 2013 10:54:49 -0700 (PDT) Content-Disposition: inline In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org Errors-To: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org To: Paulo Zanoni Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Fri, Jul 05, 2013 at 02:41:02PM -0300, Paulo Zanoni wrote: > 2013/7/5 Ville Syrj=E4l=E4 : > > On Fri, Jul 05, 2013 at 01:54:02PM -0300, Paulo Zanoni wrote: > >> 2013/7/5 : > >> > Here's my big ILK+ watermark rewrite. The main idea of the series is= to > >> > write the watermark registers at vblank to make the changes (almost)= in > >> > sync with the plane changes that caused the change in watermarks. > >> > > >> > I sent a massive RFC patch a while back, and this is now the somewhat > >> > split up version. > >> > > >> > It's still not quite where we need to get wrt. pre-computing and > >> > properly checking the LP0 watermarks, but at least it gets us a bit > >> > closer to that goal. > >> > > >> > There's quite a bit of refactoring, small fixes, renaming, and what = have you > >> > at the beginning of the series, so a lot of it should be mergeable w= /o too much > >> > risk. Many of the patches only touch codepaths that are used by HSW = currently, > >> > but by the end of the series, ILK,SNB,IVB,HSW are all using the exac= t same > >> > code. > >> > > >> > So far I've run this somewhat succesfully on ILK and IVB. > >> > >> I booted your series on Haswell, on top of drm-intel-nightly. > >> > >> I booted with only an eDP output (1920x1080). After booting, I checked > >> dmesg and there's this message: > >> [ 15.754282] [drm:ilk_update_pipe_wm] *ERROR* pipe A watermark > >> programming took 94278 ns > > > > Just diagnostics to measture the overhead. Do you have some drm.debugs > > enabled, or why does it take that long? It was ~5 us on my machines w/o > > debugs, although it could naturally take a bit longer on HSW due to the > > multi-pipe LP1+ watermarks. But I wasn't expecting 20x increase. > = > I constantly have drm.debug=3D0xe enabled. I imagine QA also uses it, > and will probably open bug reports about it since the message is > DRM_ERROR. Obviosuly we wouldn't merge that thing upstream. It's just a development aid. > >> Also, I ran intel_reg_dumper and now WM_PIPE_B and WM_PIPE_C are > >> enabled with the maximum values, but the watermarks calculator tells > >> me to use 0 on them. The other values are correct. > > > > Not sure where they got the maximum. I was first thinking that it's > > caused by "return UINT_MAX" change + the clamping LP0 to max instead > > of failing, but the UINT_MAX thing should only happen when there's no > > latency value for the level. > > > >> Then I plugged a DP monitor (2560x1440), and right after enabling it I > >> see a new message: > >> [ 323.354641] [drm:ilk_update_pipe_wm] *ERROR* pipe B watermark > >> programming took 96015 ns > >> > >> Then I ran intel_reg_dumper and WM_PIPE_C is still configured with the > >> maximum values. Also, WM_LP2 and WM_LP3 are enabled, but they > >> shouldn't be enabled. WM_LP1 has the correct values. Also, when I run > >> intel_reg_dumper I see screen corruption, but I guess this may be just > >> a consequence of WM_LP2 and WM_LP3 being enabled. > >> > >> Then I disabled the eDP output, leaving only DP. The desktop > >> environment automagically moved the DP output to pipe A. The WM_PIPE_B > >> and PIPE_WM_LINETIME_B registers did not get zeroed. > > > > Leaving the linetime WM not zeroed was expected. I didn't bother with it > > since the pipe is off anyway. It should be easy to zero it out if we > > want to. > > > > The pipe watermarks probably didn't get cleared since we update them > > after turning the pipe off in the disable path. So there won't be any > > vblank irqs from which to perform the update. We could fix that if we > > set up the new watermarks just before turning the pipe off. Then we > > should still get on more vblank irq. But that would need some change > > to the way we determine that the pipe will be off (crtc->active is > > updated too late). The other option would be to simply call > > ilk_update_pipe_wm() from the crtc disable path directly. > = > Don't forget that we didn't even zero the LP levels we needed to > disable when switching from eDP to eDP+DP. I prefer always zeroing > everything we're not using, since that's what the spec says we need to > do. Actually, zeroing the LP watermark registers mid screen causes ILK to underrun immediately. There's a comment in the code about it. So instead of zeroing I just opted to flip the enable bit, which works fine on ILK. > >> The WM_LP1 values > >> are correct, but WM_LP2 and WM_LP3 do not match the spec: the WM > >> calculator disabled the LP4 level, but your code enabled it, so the > >> value that should be written in WM_LP3 is now in WM_LP2, and WM_LP3 is > >> wrong. > > > > IIRC the wm calculator doesn't take into account the number of available > > bits in the registeris, so it might give you a larger value than what > > you can fit in the register. But that doesn't seem to your issue. I > > would problably want to see the parameters that were used to figure out > > where things go wrong. > = > I already mentioned them: > Only pipe A enabled, 2560 hsrc, 2720 htotal, 241.50 pixel rate, not > interlaced, no downscaling, cursor enabled with 4bpp 64hsrc, primary > enabled with 4bpp, sprite disabled. SSKPD 140000A005A2404F. CDCLK > 450MHz. Ah OK. Well, further analysis will have to wait until I get back from my summer vacation. > = > > > >> So three small bugs: (i) stuff not being correctly zeroed and (ii) > >> using LP4 as the max level instead of LP3 when using a mode with > >> 2560hsrc 2720htotal 241.50MHz and (iii) that dmesg error. > >> > >> > >> > > >> > ---------------------------------------------------------------- > >> > Ville Syrj=E4l=E4 (35): > >> > drm/i915: Add scaled paramater to update_sprite_watermarks() > >> > drm/i915: Pass the actual sprite width to watermarks functions > >> > drm/i915: Calculate the sprite WM based on the source width in= stead of the destination width > >> > drm/i915: Rename hsw_wm_get_pixel_rate to ilk_pipe_pixel_rate > >> > drm/i915: Rename most wm compute functions to ilk_ prefix > >> > drm/i915: Pass the watermark level to primary WM compute funct= ions > >> > drm/i915: Don't pass "mem_value" to ilk_compute_fbc_wm > >> > drm/i915: Change the watermark latency type to uint16_t > >> > drm/i915: Split out reading of HSW watermark latency values > >> > drm/i915: Don't multiply the watermark latency values too early > >> > drm/i915: Add SNB/IVB support to intel_read_wm_latency > >> > drm/i915: Add ILK support to intel_read_wm_latency > >> > drm/i915: Store the watermark latency values in dev_priv > >> > drm/i915: Use the stored cursor and plane latencies properly > >> > drm/i915: Print the watermark latencies during init > >> > drm/i915: Disable specific watermark levels when latency is ze= ro > >> > drm/i915: Pull watermark level validity check out > >> > drm/i915: Split watermark level computation from the code > >> > drm/i915: Kill fbc_enable from hsw_lp_wm_results > >> > drm/i915: Rename hsw_data_buf_partitioning to intel_ddb_partit= ioning > >> > drm/i915: Rename hsw_lp_wm_result to intel_wm_level > >> > drm/i915: Calculate max watermark levels for ILK+ > >> > drm/i915; Pull some watermarks state into a separate structure > >> > drm/i915: Split plane watermark parameters into a separate str= uct > >> > drm/i915: Pass crtc to our update/disable_plane hooks > >> > drm/i915: Don't try to disable plane if it's already disabled > >> > drm/i915: Pass plane and crtc to intel_update_sprite_watermarks > >> > drm/i915: Always call intel_update_sprite_watermarks() when di= sabling a plane > >> > drm/i915: Pass crtc to intel_update_watermarks() and call it i= n one place during modeset > >> > drm/i915: Replace the ILK/SNB/IVB/HSW watermark code > >> > drm/i915: Move HSW linetime watermark handling to modeset code > >> > hack: Add debug prints to watermark compute funcs > >> > hack: Don't disable underrun reporting on the first error on I= LK/SNB/IVB > >> > hack: Make fifo underruns DRM_ERROR > >> > hack: Print watermark programming duration > >> > > >> > drivers/gpu/drm/i915/i915_drv.h | 41 +- > >> > drivers/gpu/drm/i915/i915_irq.c | 32 +- > >> > drivers/gpu/drm/i915/i915_reg.h | 2 + > >> > drivers/gpu/drm/i915/intel_display.c | 45 +- > >> > drivers/gpu/drm/i915/intel_drv.h | 47 +- > >> > drivers/gpu/drm/i915/intel_pm.c | 1796 +++++++++++++++++------= ----------- > >> > drivers/gpu/drm/i915/intel_sprite.c | 54 +- > >> > 7 files changed, 1039 insertions(+), 978 deletions(-) > >> > _______________________________________________ > >> > Intel-gfx mailing list > >> > Intel-gfx@lists.freedesktop.org > >> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > >> > >> > >> > >> -- > >> Paulo Zanoni > > > > -- > > Ville Syrj=E4l=E4 > > Intel OTC > = > = > = > -- = > Paulo Zanoni -- = Ville Syrj=E4l=E4 Intel OTC