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

[Ville Syrjälä]

On Tue, Oct 15, 2013 at 02:01:39PM -0300, Paulo Zanoni wrote:
> 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 current code is still unsafe wrt. underruns, even on HSW. So if we
extend underrun reporting at this point, we're sure to get some extra
noise. I'm working on the safe update mechnism, and it's starting to
look fairly decent, although it does make the whole thing a notch more
complex again.

I still had some sprite underrun issues on ILK, but I think I just
discovered the magic sequence to fix that. Now I have the watermarks
for all planes set up dynamically depending on the current need. Eg.
if you disable the cursor even the cursor watermarks get zeroed out.
Also it seems I've accidentally fixed the 5/6 DDB split on IVB.
Previously I had issues with it, but that may have been a simple bug
in my earlier code since I'm not seeing it currently. But I do need to
run some more tests on all affected machines to make sure I haven't
missed something.

I could post the basic ILK/SNB/IVB enabling patches already. I don't think
they can severly regress the current situation, so in that sense they
should be fairly OK to push in. I just figured I'd give you a few days to
catch your breath :)

> >
> > - 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.

We'd need to add a debugfs file to override the latency values since I
changed the code to use the copies stored in dev_priv. But that could be
a generally useful underrun debug mechanism anyway. I suppose the file
contents could just reflect the MLTR/SSKPD register value, or we could
try to come up with something a bit more user friendly.

> 
> >
> > - 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?

It all depends whether someone can still call the watermark code without
clock or fb. I don't think I can fix that part by frobbing the watermark
code.

-- 
Ville Syrjälä
Intel OTC