[Intel-gfx] [PATCH 00/35] drm/i915: ILK+ watermark rewrite
Paulo Zanoni
przanoni at gmail.com
Fri Jul 5 19:41:02 CEST 2013
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
More information about the Intel-gfx
mailing list