Re: [PATCH 00/35] drm/i915: ILK+ watermark rewrite

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Fri, Jul 05, 2013 at 01:54:02PM -0300, Paulo Zanoni wrote:
> 2013/7/5  <ville.syrjala@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.

> 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.

> 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.

> 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@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
> -- 
> Paulo Zanoni

-- 
Ville Syrjälä
Intel OTC