[Intel-gfx] [PATCH 3/4] drm/i915: Sanitize watermarks after hardware state readout (v2)

[Ville Syrjälä]

On Thu, Nov 19, 2015 at 08:01:41PM -0800, Matt Roper wrote:
> On Thu, Nov 05, 2015 at 02:55:20PM +0200, Jani Nikula wrote:
> > On Thu, 05 Nov 2015, Matt Roper <matthew.d.roper at intel.com> wrote:
> > > On Wed, Nov 04, 2015 at 04:12:33PM +0200, Jani Nikula wrote:
> > >> On Tue, 03 Nov 2015, Matt Roper <matthew.d.roper at intel.com> wrote:
> > >> > Although we can do a good job of reading out hardware state, the
> > >> > graphics firmware may have programmed the watermarks in a creative way
> > >> > that doesn't match how i915 would have chosen to program them.  We
> > >> > shouldn't trust the firmware's watermark programming, but should rather
> > >> > re-calculate how we think WM's should be programmed and then shove those
> > >> > values into the hardware.
> > >> >
> > >> > We can do this pretty easily by creating a dummy top-level state,
> > >> > running it through the check process to calculate all the values, and
> > >> > then just programming the watermarks for each CRTC.
> > >> >
> > >> > v2:  Move watermark sanitization after our BIOS fb reconstruction; the
> > >> >      watermark calculations that we do here need to look at pstate->fb,
> > >> >      which isn't setup yet in intel_modeset_setup_hw_state(), even
> > >> >      though we have an enabled & visible plane.
> > >> >
> > >> > Cc: Jani Nikula <jani.nikula at intel.com>
> > >> > Cc: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> > >> > Signed-off-by: Matt Roper <matthew.d.roper at intel.com>
> > >> > ---
> > >> > Jani, based on your logs it looks like the culprit is that we're calculating
> > >> > watermarks at startup time after we've read out preliminary hardware state (so
> > >> > we know the primary plane is enabled & visible), but before we reconstruct the
> > >> > BIOS fb (so pstate->fb is NULL).  I think moving the watermark sanitization
> > >> > later in the process so that we'll have a proper fb setup should hopefully
> > >> > solve the issue.  Can you test this version when you get a chance?  I'll also
> > >> > send a rebased patch #4 since the code movement here means that the previous
> > >> > version won't apply cleanly.
> > >> 
> > >> Sorry Matt, black screen with new versions of patches 3-4. Dmesg at
> > >> http://pastebin.com/3LXZwvWu
> > >
> > > Hmm, okay, looks like we're getting closer (successfully avoided the
> > > divide by zero problem), but I neglected to grab the necessary locks so
> > > the sanitization doesn't actually happen.  Does applying
> > > http://paste.debian.net/322932 on top of the series work any better for
> > > you?  I haven't had time to pull back out an ILK-style system, so that's
> > > only compile-tested at the moment.
> > 
> > Still warns http://pastebin.com/yGtde5X2
> > 
> > BR,
> > Jani.
> 
> Okay, thanks to Jani setting up his machine so that I can debug
> remotely, I think I understand where we're going wrong now.  The missing
> piece of the puzzle is that Jani's display is 3840x2160.  That means the
> BIOS FB we need to inherit is deemed too large by our initial hardware
> readout code (i.e., greater than half of
> dev_priv->gtt.stolen_usable_size),

I would just revert the patch that added that restriction. IMO it makes
no sense as long as FBC remains disabled by default. And even worse it
doesn't even check that FBC is even supported by the platform. We're
just wasting memory now.

> so we don't even bother inheriting
> the FB and pstate->fb stays NULL, even though pstate->visible = true and
> the plane is part of the CRTC's plane_mask.
> 
> Adding Maarten, Ville, and Ander to Cc since I think they may have some
> insights on how best to handle this.  Seems like our options are:
>  * If our watermark code comes up with a NULL FB while pstate->visible,
>    just pretend we're working on a 32-bit framebuffer that matches the
>    CRTC mode dimensions.  We'll then calculate appropriate watermark
>    values, even though we don't have any real information for the FB
>    that the hardware is scanning out of.
>  * Set pstate->visible = false and clear the bit from plane_mask.  I
>    think we'll run into state verification warnings then ("plane A
>    should be disabled but is not") since our HW state and SW state don't
>    match.
>  * Actually turn off the primary plane (hardware-wise) if we fail to
>    inherit the BIOS FB.

And yes, option 3 is what I also think we should do.

-- 
Ville Syrjälä
Intel OTC
---------------------------------------------------------------------
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki 
Business Identity Code: 0357606 - 4 
Domiciled in Helsinki 

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.