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

-- 
Ville Syrjälä
Intel OTC