Re: [PATCH 16/16] drm/i915: Rename ilk_check_wm to ilk_validate_wm_level

[Daniel Vetter <daniel@ffwll.ch>]

On Fri, Oct 11, 2013 at 02:08:07PM -0300, Paulo Zanoni wrote:
> 2013/10/9  <ville.syrjala@linux.intel.com>:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Makes the behaviour of the function more clear.
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Thanks :)
> 
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@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:

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

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

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

- 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