Re: [PATCH REBASED v10 6/6] drm/i915/skl: Update DDB values atomically with wms/plane attrs

[Maarten Lankhorst]

Hey,

Op 10-08-16 om 16:28 schreef Lyude:
> Now that we can hook into update_crtcs and control the order in which we
> update CRTCs at each modeset, we can finish the final step of fixing
> Skylake's watermark handling by performing DDB updates at the same time
> as plane updates and watermark updates.
>
> The first major change in this patch is skl_update_crtcs(), which
> handles ensuring that we order each CRTC update in our atomic commits
> properly so that they honor the DDB flush order.
>
> The second major change in this patch is the order in which we flush the
> pipes. While the previous order may have worked, it can't be used in
> this approach since it no longer will do the right thing. For example,
> using the old ddb flush order:
>
> We have pipes A, B, and C enabled, and we're disabling C. Initial ddb
> allocation looks like this:
>
> |   A   |   B   |xxxxxxx|
>
> Since we're performing the ddb updates after performing any CRTC
> disablements in intel_atomic_commit_tail(), the space to the right of
> pipe B is unallocated.
>
> 1. Flush pipes with new allocation contained into old space. None
>    apply, so we skip this
> 2. Flush pipes having their allocation reduced, but overlapping with a
>    previous allocation. None apply, so we also skip this
> 3. Flush pipes that got more space allocated. This applies to A and B,
>    giving us the following update order: A, B
>
> This is wrong, since updating pipe A first will cause it to overlap with
> B and potentially burst into flames. Our new order (see the code
> comments for details) would update the pipes in the proper order: B, A.
>
> As well, we calculate the order for each DDB update during the check
> phase, and reference it later in the commit phase when we hit
> skl_update_crtcs().
>
> This long overdue patch fixes the rest of the underruns on Skylake.
>
> Changes since v1:
>  - Add skl_ddb_entry_write() for cursor into skl_write_cursor_wm()
> Changes since v2:
>  - Use the method for updating CRTCs that Ville suggested
>  - In skl_update_wm(), only copy the watermarks for the crtc that was
>    passed to us
> Changes since v3:
>  - Small comment fix in skl_ddb_allocation_overlaps()
>
> Fixes: 0e8fb7ba7ca5 ("drm/i915/skl: Flush the WM configuration")
> Fixes: 8211bd5bdf5e ("drm/i915/skl: Program the DDB allocation")
> [omitting CC for stable, since this patch will need to be changed for
> such backports first]
This series breaks on kms_atomic_transition.plane-all-transition (just uploaded the changed tests to igt, please rebuild)

[ 5455.543871] [drm:verify_wm_state.isra.72 [i915]] *ERROR* mismatch in DDB state pipe A plane 1 (expected (0,0), found (0,860))

There's also a WARN_ON(... && total_data_rate == 0) which you need to comment out for the tests to pass cleanly.

~Maarten