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

Tue Jun 14 22:57:28 UTC 2016

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.

Matt

> 
> Thanks a lot,
> Chi
> 
> 
> 
> 
> 
> 
> 
> -----Original Message-----
> From: Maarten Lankhorst [mailto:maarten.lankhorst at linux.intel.com] 
> Sent: Monday, June 13, 2016 10:22 AM
> To: Ding, ChiX; ville.syrjala at linux.intel.com; Roper, Matthew D
> Cc: isg-gms at eclists.intel.com; Adebisi, YetundeX
> Subject: Re: [RFC 6/6] drm/i915/vlv: Add intermediate field in intel_crtc_wm_state and handlers for two-level watermark
> 
> Op 10-06-16 om 18:48 schreef Ding, ChiX:
> > Hi Maarten
> > I've changed the code to use the macro 
> > drm_atomic_crtc_state_for_each_plane_state and the other place to check !new_state->active || modeset in computing intermediate watermark. Test result is good.
> >
> > I have some questions however. 
> > Regarding the logic of computing intermediate watermark, you suggested to use average "
> > wm old:
> > { 0, 511 }
> >
> > wm new:
> > { 511, 0 }
> >
> > min(a, b):
> > { 0, 0 }
> >
> > Averaged:
> > { 255, 255 }; --> only 510 summed, should probably add 1 to the first non-null value since it must add up to with 512 - 1."
> >
> > 1) It looks good to me to use the average when one of the old and new values is 0 as listed in your example.
> > But when none of the old and new are 0 and they're not equal, average 
> > value will be bigger than the smaller value of the two. I thought that the idea of comuting intermediate for VLV is to pick a safe value, i.e. a value <= either of the old and the new?
> No, the average will always be rounded down due to integer math. Not sure how you would end up with higher values if you take the average and divide by 2?
> >
> >
> > 2) About the wm values of the planes add up to 511, I don't have the 
> > spec of watermark for VLV but I noticed that it doesn't seem like the 
> > case from dmesg output without the two-level watermark patches Example 1 :
> >  [13634.545172] [drm:vlv_pipe_set_fifo_size] Pipe A FIFO split 511 / 
> > 511 / 511  [13634.545180] [drm:vlv_update_wm] Setting FIFO watermarks 
> > - A: plane=0, cursor=0, sprite0=0, sprite1=0, SR: plane=0, cursor=0 
> > level=0 cxsr=0
> >
> > Example 2:
> >  [13635.127046] [drm:vlv_pipe_set_fifo_size] Pipe A FIFO split 511 / 
> > 511 / 511  [13635.127053] [drm:vlv_update_wm] Setting FIFO watermarks 
> > - A: plane=421, cursor=63, sprite0=0, sprite1=0, SR: plane=0, cursor=0 
> > level=0 cxsr=0
> Look at vlv_compute_fifo, the last loop adds the remainder to the first active plane, or when none are active puts it all on the primary plane.
> >
> > 3) I see that in vlv_wm_get_hw_state(), it assigns the current 
> > vlv_wm_state to the optimal, for_each_intel_crtc(dev, crtc) {
> > 	pipe = crtc->pipe;
> > 	to_intel_crtc_state(crtc->base.state)->wm.vlv.optimal
> > 		= crtc->wm_state;
> >
> > I thought that the optimal is computed in intel_crtc_atomic_check() by 
> > .compute_pipe_wm() handler Do we need to assign the current value to it in  vlv_wm_get_hw_state(), which is called by intel_modeset_setup_hw_state()?
> This is a get_hw_state function. It's only used by hw readout during driver init or resume.

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795