[Intel-gfx] [PATCH 00/35] drm/i915: ILK+ watermark rewrite
Ville Syrjälä
ville.syrjala at linux.intel.com
Fri Jul 5 19:54:46 CEST 2013
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
More information about the Intel-gfx
mailing list