On Fri, Jul 05, 2013 at 02:41:02PM -0300, Paulo Zanoni wrote: > 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. 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?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 -- Ville Syrj?l? Intel OTC
