2013/7/5 Ville Syrj?l? <ville.syrjala at linux.intel.com>: > On Fri, Jul 05, 2013 at 01:54:02PM -0300, Paulo Zanoni wrote: >> 2013/7/5 <ville.syrjala at linux.intel.com>: >> > 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 exact 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=0xe enabled. I imagine QA also uses it, and will probably open bug reports about it since the message is DRM_ERROR. > >> 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. > >> 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. > >> 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?l? (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 instead 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 functions >> > 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 zero >> > 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_partitioning >> > 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 struct >> > 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 disabling a plane >> > drm/i915: Pass crtc to intel_update_watermarks() and call it in 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 ILK/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 at lists.freedesktop.org >> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx >> >> >> >> -- >> Paulo Zanoni > > -- > Ville Syrj?l? > Intel OTC -- Paulo Zanoni
