[Intel-gfx] [RFC 6/6] drm/i915/vlv: Add intermediate field in intel_crtc_wm_state and handlers for two-level watermark

[Ville Syrjälä]

On Wed, Jun 15, 2016 at 12:49:49PM +0300, Ville Syrjälä wrote:
> On Tue, Jun 14, 2016 at 03:57:28PM -0700, Matt Roper wrote:
> > On Tue, Jun 14, 2016 at 07:40:54AM -0700, Ding, ChiX wrote:
> > > Hi Maarten
> > > Thanks for the reply. Please correct me if I'm wrong : 
> > > If the intermediate watermark is bigger than any of the old and the new, then it's not really safe.
> > > It could be bigger than the FIFO size. 
> > > 
> > > I did some tests using average value and it ran into FIFO underrun, e.g.
> > > [100424.405624] [drm:vlv_invert_wms] Pipe A fifo for primary, sprite0, sprite 1 : 511 / 0 / 0 
> > > [100424.413231] [drm:vlv_compute_fifo] Pipe A fifo for primary, sprite0, sprite 1 : 256 / 255 / 0
> > > [100424.413242] [drm:vlv_compute_pipe_wm] Pipe A
> > > [100424.413245] [drm:vlv_compute_pipe_wm] level : 0
> > > [100424.413249] [drm:vlv_compute_pipe_wm] Watermark values for primary, sprite0, sprite1 : 136, 135, 0
> > > [100424.413253] [drm:vlv_compute_intermediate_wm] level : 0
> > > [100424.413257] [drm:vlv_compute_intermediate_wm] A watermark values for primary, sprite0, sprite1 : 136, 135, 0
> > > [100424.413261] [drm:vlv_compute_intermediate_wm] B watermark values for primary, sprite0, sprite1 : 391, 0, 0
> > > [100424.429823] [drm:vlv_initial_watermarks] vlv_initial_watermarks
> > > 
> > > At first, the current active
> > > fifo sizes 511 / 0 / 0 for primary, sprite0, sprite 1
> > > watermark 391, 0, 0
> > > 
> > > Then the optimal values are calculated : 
> > > fifo sizes 256 / 255 / 0
> > > watermarks 136, 135, 0
> > > 
> > > The average watermark for primary plane is 391+136/2 = 263, bigger
> > > than the primary plane fifo size for optimal : 256. It causes underrun
> > > later on when writing intermediate watermark and the fifo size is
> > > changed to 256 for primary plane by vlv_pipe_set_fifo_size() function.
> > > 
> > > If I use min() for intermediate watermark, I don't run into underrun
> > > issue in the tests.  My understanding is that with min(), in some
> > > cases we get 0,0 for watermark , but it's still a legitimate value.
> > > Please let me know if I'm mistaken.
> > > 
> > > 
> > > Another question: the sum of fifo sizes for primary plane, sprite 0,
> > > sprite 1 should always be 512-1 = 511.  But the watermarks of the
> > > planes don't need to add up to 511. They need to stay under or at most
> > > equal to the respective fifo size, right?
> > 
> > I may be overlooking something since I haven't studied VLV/CHV in as
> > much detail as the other platforms, but there are basically two parts to
> > the per-crtc watermark calculations we do during the atomic 'check'
> > phase.
> > 
> > The first part is slicing up the fifo/DDB between the various planes
> > that are in use.  The fifo buffer is a fixed size resource made up of
> > blocks, but it's programmable how much of that buffer is devoted to
> > driving a specific plane.  The formula we use for slicing up the fifo is
> > to just give each plane a number of blocks proportional to the fraction
> > of the total data rate that the plane contributes (with some extra logic
> > at the end to deal with leftover blocks due to integer division).
> > 
> > The second part is calculating the watermark values for each plane.  The
> > fifo holds data that's been fetched from RAM for the display controller
> > to consume; the watermark values (measured in fifo blocks) are basically
> > telling the hardware how/when it needs to wake up and fetch more data
> > from RAM into the fifo to ensure the fifo doesn't reach empty.  If you
> > set the watermark value for a plane too low, then the hardware won't
> > fetch more data from RAM early enough and the fifo will run dry before
> > new data from RAM shows up; when this happens, the display controller
> > doesn't have any data to tell it how to keep driving the display so you
> > can get various kinds of display corruption (and dmesg warnings about a
> > FIFO underrun).  Programming watermarks too high is always "fine" from a
> > correctness point of view --- you're just causing the hardware to wake
> > up and fetch more data from RAM earlier than it really had to.  You may
> > be impacting power usage, but you're safe from underruns/corruption.
> > 
> > For intermediate watermarks, we need "worst case" values that satisfy
> > both the old and new configuration (i.e., won't cause underruns for
> > either config).  So I think you want to be using max() instead of min()
> > to calculate your intermediate values.  Once you've calculated the
> > worst-case (max) values for each plane, I figure you'd want to work
> > backwards from there and slice up your intermediate per-place fifo
> > allocation second...I guess you'd probably just set each plane's fifo
> > size equal to its watermark value to start with, then distribute the
> > remaining blocks equally between all of the planes that are active in
> > either new or old configuration (and if you don't have any remaining
> > blocks, then there's no valid intermediate configuration so we should
> > probably fail the request).
> > 
> > Again, this is just completely off the top of my head and I haven't
> > actually looked at the VLV/CHV bspecs lately, so I might be completely
> > overlooking some important detail about these platforms.
> 
> There won't be any intermediate FIFO sizes. The FIFO repartitioning
> happens atomically with plane updates, or rather should, but the exact
> nature of the register double buffering is still a bit of a mystery.
> How exactly the double buffering works may dictate how we have to do
> things, so until someone figures it out, I don't think there's any
> point in trying to fix the watermark updates.
> 
> step 1. figure out the DSBARB double buffering
> step 2. ?
> step 3. profit

Bah, since no one else seems to have bothered to do this, I went ahead
and wrote a small test to exercise the DSPARB double buffering. The
results surprised me in a positive way; The hardware does seem to work
in a sane way, that is, only the DSPARB bits for pipe A appear to be
latched by pipe A vblank, pipe B bits by pipe B vblank etc. I've run
through all combinations of two pipes and didn't see any cross-pipe
latching happening. However I only tested with primary planes thus
far. I think I'll still give it another go with some sprites enabled,
just to be extra sure it's working correctly...

-- 
Ville Syrjälä
Intel OTC