Re: 830GM still woes

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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.

-- 
Ville Syrjälä
Intel OTC