[Intel-gfx] [PATCH 1/2] drm/i915: Do not rely on wm preservation for ILK watermarks

[Maarten Lankhorst]

Op 27-10-17 om 03:09 schreef Matt Roper:
> On Wed, Oct 25, 2017 at 08:03:47AM +0200, Maarten Lankhorst wrote:
>> Op 25-10-17 om 01:01 schreef Matt Roper:
>>> On Thu, Oct 19, 2017 at 05:13:40PM +0200, Maarten Lankhorst wrote:
>>>> The original intent was to preserve watermarks as much as possible
>>>> in intel_pipe_wm.raw_wm, and put the validated ones in intel_pipe_wm.wm.
>>>>
>>>> It seems this approach is insufficient and we don't always preserve
>>>> the raw watermarks, so just use the atomic iterator we're already using
>>>> to get a const pointer to all bound planes on the crtc.
>>>>
>>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=102373
>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
>>>> Cc: stable at vger.kernel.org #v4.8+
>>> It's been a while since I looked at this code, so I'm not sure I'm
>>> following all the context/history here.  Before this patch, we had
>>> calculated watermarks for all levels (even those above the maximum
>>> usable watermark level as determined by sprite usage) into raw_wm.  But
>>> as far as I can tell, we never actually used those values for anything
>>> so that was no different than just throwing the values away?
>> No, the trick introduced there was that we preserved the raw watermarks for future
>> calculations to not include all planes in a commit. This is now solved by using the
>> read-only drm_atomic_crtc_state_for_each_plane_state iterator, to get all
>> const plane states without having to include them.
>>
>>> Is my understanding correct that this is mostly a revert of 71f0a62614
>>> ("drm/i915: Only use sanitized values for ILK watermarks) except that it
>>> also modifies the level validation loop so that we're only calling
>>> ilk_compute_wm_level() on the levels up to usable_level whereas before
>>> that patch we were calling it on all levels, but then
>>> setting wm->enable = false for the unusable levels?
>>>
>>> By my understanding, this looks like a safe simplification of the
>>> current logic, but I don't see where the functional change is here.
>>> Was the Buzilla: reference supposed to be tied to patch #2 of this
>>> series or am I missing something important?
>> We now throw away all watermarks, and use drm_atomic_crtc_state_for_each_plane_state
>> to get the plane states. As a result we can throw away the raw watermarks, which have
>> proven to be insufficiently preserved. :)
> Okay, makes more sense now.  Thanks for clarifying.
>
> Reviewed-by: Matt Roper <matthew.d.roper at intel.com>
Thanks, pushed both patches. Hopefully issues are gone now. :)