[Intel-gfx] [PATCH 16/16] drm/i915: Rename ilk_check_wm to ilk_validate_wm_level

[Paulo Zanoni]

2013/10/15 Daniel Vetter <daniel at ffwll.ch>:
> On Fri, Oct 11, 2013 at 02:08:07PM -0300, Paulo Zanoni wrote:
>> 2013/10/9  <ville.syrjala at linux.intel.com>:
>> > From: Ville Syrjälä <ville.syrjala at linux.intel.com>
>> >
>> > Makes the behaviour of the function more clear.
>> >
>> > Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
>>
>> Thanks :)
>>
>> Reviewed-by: Paulo Zanoni <paulo.r.zanoni at intel.com>
>
> With the exception of the tracepoint patch I've merged the entire series,
> thanks for patches&review.
>
> Now all these watermark changes start to freak me out since we seem to
> fully rely on Paulo's sharp eyes to check them. I really think it's time
> to blow through a few cycles to independently check all this stuff. Some
> ideas:

Before reviewing each of Ville's series I usually dump the current WM
configurations of eDP-only, eDP+DP, nothing, DP+something with
intel-reg-dumper and then apply his patches and compare the results.
So far we're good, the only change I have noticed was already
discussed here. Also, all this code only runs on Haswell right now
(even though the goal is to run it on ILK+), so checking regressions
is not really that hard today. Of course, I don't do full corner-case
checking.

I always thought about writing some IGT test to check watermarks, but
the problem is that we'd have to reimplement the WM code on user space
if we want to validate the Kernel code, so not really a feasible
solution.

>
> - Enable the fifo underrun stuff and make it really load. Maybe only on
>   haswell for a start. If this starts to hit issues in the wild we might
>   need some form of display error state which captures all the
>   sprites/cursor/planes/crtc/wm/... state. Maybe we could do this as part
>   of the error state stuff we already have, but with the GT side of things
>   not enabled (since presumably the GT is really busy and we shouldn't
>   unduly poke it).

That was already suggested, but since Ville seems to have the code to
properly set watermarks on ILK+ already written, I think we should
just wait for it.

>
> - The hw state readout needs cross-checking. We now rely on the read out
>   wm state (for the first modeset at least, but there's always fastboot).
>   Experienc says that without cross checks this will get broken eventually
>   and lead to fun-to-debug bugs.

The nice thing of this series is that it adds the infrastructure to do
the HW state readout + check. I even suggested this already. Maybe
it's already on Ville's TODO list :)

>
> - I'm not sure whether there's a sane way to dump out the wm settings and
>   check them in userspace. Duplicating the entire calculation is pointless
>   and we can't really integrate the excel spreadsheet from the hw guys
>   into igt. And using a set of interesting corner-cases to test all the
>   basic modes (one pipe, sprite splits, ...) is probably too inflexible.
>   But if we can get stable watermark settings by e.g. injecting an special
>   edid somewhere so that we know the exact dotclocks this might be
>   interesting.

Watermarks also depend on the machine memory configuration (SSKPD) so
that's not really easy... The intel-reg-dumper tools dumps all the
relevant registers and can be easily be used to compare against the
spreadsheed.

OTOH, we could "hardcode" the common SSKPD values (at least from QA's
machines) and the values for some common modes (1024x768, 1920x1080,
etc) and check the state set by the Kernel against our hardcoded
state... It's not the best solution, but at least it's something.

>
> - At least exercising some of the special cases (and then relying on the
>   state cross-checker and fifo underrun reporting to catch fallout) from
>   userspace would be good.
>
>
> I'm running a bit low on good stuff here, so better ideas highly welcome.
> It's not really an area I've wreak much havoc in at all ...
>
> One other thing I've noticed is that we still have calls to
> intel_crtc_active sprinkled throughout the hsw wm functions. Should we be
> able to ditch those and replace them with a plain crtc->active check, now
> that we have wm state readout?
>
> Cheers, Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch



-- 
Paulo Zanoni