Re: 830GM still woes

[Daniel Vetter <daniel@ffwll.ch>]

On Fri, May 16, 2014 at 07:04:54PM +0300, Ville Syrjälä wrote:
> On Fri, May 16, 2014 at 05:09:53PM +0200, Daniel Vetter wrote:
> > On Fri, May 16, 2014 at 03:41:05PM +0100, Chris Wilson wrote:
> > > On Fri, May 16, 2014 at 04:02:48PM +0200, Thomas Richter wrote:
> > > > It's not that I haven't had a patch for it. Really trivial. I wonder
> > > > what keeps you from adding this to the kernel and just make things
> > > > working?
> > > 
> > > You mean this patch?
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > > index f671aca..3981898 100644
> > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > @@ -944,7 +944,7 @@ static const struct intel_watermark_params i915_wm_info = {
> > >  static const struct intel_watermark_params i830_wm_info = {
> > >         I855GM_FIFO_SIZE,
> > >         I915_MAX_WM,
> > > -       1,
> > > +       8,
> > >         2,
> > >         I830_FIFO_LINE_SIZE
> > >  };
> > > @@ -1001,7 +1001,7 @@ static unsigned long intel_calculate_wm(unsigned long clock_in_khz,
> > >         /* Don't promote wm_size to unsigned... */
> > >         if (wm_size > (long)wm->max_wm)
> > >                 wm_size = wm->max_wm;
> > > -       if (wm_size <= 0)
> > > +       if (wm_size < (long)wm->default_wm)
> > >                 wm_size = wm->default_wm;
> > >         return wm_size;
> > >  }
> > > 
> > > I haven't spotted any explanation as to why that is, but a rough guess
> > > would be that we program it to read in blocks of 8 superwords and that
> > > it tries and fails to read from memory when the fifo only has room for 1
> > > superword.
> > 
> > I have it - we need to proper align watermark limits and fifo sizes and
> > round them apparently. Bspec at least strongly suggests that, and it would
> > perfectly fit Thomas' symptoms.
> 
> Where have you seen that? And how should they be aligned? I've never
> seen anything like that in the spec. Also based on tests on my 830
> it doesn't need special alignment, it just needs some kind of minumum
> value that's always somewhere around 6-8 (IIRC).
> 
> I do see this note "Up to FIFO Size minus burst length + 32 bytes"
> in one of the tables in the display doc. I can't tell if that means
> 'fifo_size - (burst_size + 32B)' or 'fifo_size - burst_size + 32B'.
> But in any case would actually make the minimum allowed value 7 or 9
> since we always configure the burst size to 8.
> 
> On Gen3 the units change to 64B but it still has the same note with
> the +32B, so I'm not sure what should be done there. I guess it's
> just a copy paste fumble and maybe the same minimum value should
> still apply.

Yeah the burst size stuff - afaiu we should select the biggest one
possible and if that's not working out round the watermark up to match the
burst size. I didn't spot the +32/-32bytes anywhere though ... I guess
going with burst_size + 1 should be safest, especially if we make the code
more flexible to also allow a burst size of 4 for the really high-res
stuff.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch