[Intel-gfx] [PATCH v4 0/6] Finally fix watermarks

[Matt Roper]

On Fri, Jul 29, 2016 at 12:39:05PM +0300, Ville Syrjälä wrote:
> On Thu, Jul 28, 2016 at 05:03:52PM -0700, Matt Roper wrote:
> > This is completely untested (and probably horribly broken/buggy), but
> > here's a quick mockup of the general approach I was thinking for
> > ensuring DDB & WM's can be updated together while ensuring the
> > three-step pipe flushing process is honored:
> > 
> >         https://github.com/mattrope/kernel/commits/experimental/lyude_ddb
> > 
> > Basically the idea is to take note of what's happening to the pipe's DDB
> > allocation (shrinking, growing, unchanged, etc.) during the atomic check
> > phase;
> 
> Didn't look too closely, but I think you can't actually do that unless
> you lock all the crtcs whenever the number of active pipes is goind to
> change. Meaning we'd essentially be back to the one-big-modeset-lock
> apporach, which will cause missed flips and whanot on the other pipes.
> 
> The alternative I think would consist of:
> - make sure level 0 watermark never exceeds total_ddb_size/max_pipes,
>   so that a modeset doesn't have to care about the wms for the other
>   pipes not fitting in

Unfortunately this part is the problem.  You might get away with doing
this on SKL/KBL which only have three planes max per pipe and a large
(896 block) DDB, but on BXT you have up to four planes (we don't
actually enable the topmost plane in a full-featured manner just yet,
but need to soon), yet the total DDB is only 512 blocks.  Sadly, the
platform with more planes was given a smaller DDB...   :-(

We're already in trouble because users that are running setups like 3x
4K with most/all planes in use at large sizes can't find level 0
watermarks that satisfy their needs and we have to reject the whole
configuration.  If we further limit each pipe's usage to total/maxpipes
(i.e., 170 blocks per pipe on BXT), then we're going to hit similar
issues when only driving one or two displays with with all of the planes
in use, even though we should have had more DDB space to work with.

I guess serious plane usage isn't too common in desktop setups today,
but it's a very critical feature in the embedded world; we can't really
afford to cripple plane usage further.  Unfortunately, as you point out
above, this means that we have to follow the bspec's DDB allocation
method, which in turn means we need to grab _all_ CRTC locks any time
_any_ CRTC is being turned on or turned off which is a BKL-style way of
doing things.


Matt

> - level 1+ watermarks would be checked against total_ddb_size
> - protect the plane/pipe commit with the wm mutex whenever the wms
>   need to be reprogrammed
> - keep the flush_wm thing around for the case when ddb size does get
>   changed, protect it with the wm lock
> - when programming wms, we will first filter out any level that
>   doesn't fit in with the current ddb size, and then program the
>   rest in
> - potentially introduce per-pipe wm locks if the one big lock looks
>   like an issue, which it might if the flush_wm holds it all the way
>   through
> 
> > then during the commit phase, we loop over the CRTC's three times
> > instead of just once, but only operate on a subset of the CRTC's in each
> > loop.  While operating on each CRTC, the plane, WM, and DDB all get
> > programmed together and have a single flush for all three.
> >
> > 
> > 
> > 
> > Matt
> > 
> > On Tue, Jul 26, 2016 at 01:34:36PM -0400, Lyude wrote:
> > > Latest version of https://lkml.org/lkml/2016/7/26/290 . Resending the whole
> > > thing to keep it in one place.
> > > 
> > > Lyude (5):
> > >   drm/i915/skl: Add support for the SAGV, fix underrun hangs
> > >   drm/i915/skl: Only flush pipes when we change the ddb allocation
> > >   drm/i915/skl: Fix extra whitespace in skl_flush_wm_values()
> > >   drm/i915/skl: Update plane watermarks atomically during plane updates
> > >   drm/i915/skl: Always wait for pipes to update after a flush
> > > 
> > > Matt Roper (1):
> > >   drm/i915/gen9: Only copy WM results for changed pipes to skl_hw
> > > 
> > >  drivers/gpu/drm/i915/i915_drv.h      |   3 +
> > >  drivers/gpu/drm/i915/i915_reg.h      |   5 +
> > >  drivers/gpu/drm/i915/intel_display.c |  24 ++++
> > >  drivers/gpu/drm/i915/intel_drv.h     |   4 +
> > >  drivers/gpu/drm/i915/intel_pm.c      | 240 +++++++++++++++++++++++++++++++----
> > >  drivers/gpu/drm/i915/intel_sprite.c  |   2 +
> > >  6 files changed, 255 insertions(+), 23 deletions(-)
> > > 
> > > -- 
> > > 2.7.4
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx at lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > -- 
> > Matt Roper
> > Graphics Software Engineer
> > IoTG Platform Enabling & Development
> > Intel Corporation
> > (916) 356-2795
> 
> -- 
> Ville Syrjälä
> Intel OTC

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