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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <4a0e7ae6-8adc-24ba-dd19-8c04cfadb803@linux.intel.com>
Date:	Thu, 11 Aug 2016 16:10:30 +0200
From:	Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>
To:	Lyude <cpaul@...hat.com>, intel-gfx@...ts.freedesktop.org,
	Ville Syrjälä <ville.syrjala@...ux.intel.com>,
	Matt Roper <matthew.d.roper@...el.com>
Cc:	Daniel Vetter <daniel.vetter@...el.com>,
	Radhakrishna Sripada <radhakrishna.sripada@...el.com>,
	Hans de Goede <hdegoede@...hat.com>,
	Jani Nikula <jani.nikula@...ux.intel.com>,
	David Airlie <airlied@...ux.ie>,
	dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH REBASED v10 6/6] drm/i915/skl: Update DDB values
 atomically with wms/plane attrs

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