[Intel-gfx] [PATCH 2/7] drm/i915/gen11: Link nv12 Y and UV planes in the atomic state, v3.

[Ville Syrjälä]

On Tue, Sep 25, 2018 at 01:18:43PM -0700, Matt Roper wrote:
> On Tue, Sep 25, 2018 at 09:34:29PM +0300, Ville Syrjälä wrote:
> > On Tue, Sep 25, 2018 at 11:01:32AM -0700, Matt Roper wrote:
> > > On Mon, Sep 24, 2018 at 04:18:10PM +0300, Ville Syrjälä wrote:
<snip>
> > > > It pretty much has to. The design error we have at the moment is not
> > > > programming the watermarks from the update_plane()/disable_plane().
> > > > That one I've attempted to fix in:
> > > > git://github.com/vsyrjala/linux.git skl_plane_update_ddb_sequence
> > > > 
> > > > And supposedly that one does fix bugs related to watermarks vs.
> > > > plane updates.
> > > 
> > > Where did the bugs arise?  Were we unsuccessful at actually evading the
> > > vblank (leading to the planes and watermarks taking effect on different
> > > vblanks) or is it something else?
> > 
> > There are a few issues here:
> > - write to any plane registers apart from SURF will cancel an already
> >   pending plane update. Well, it's not 100% cancelled as some registers
> >   aren't part of the SURF based arming mechanism but IIRC they still
> >   cause a cancellation. This means doing a watermark update before a
> >   pending plane update was latched cancels most of the plane update.
> >   This at least caused the cursor to remain on in when it should have
> >   been turned off.
> 
> Wow, I think I've run into this problem before, but never figured out
> what the exact root cause was.  I tried to write an IGT to capture it a
> while back, but the behavior went away with my simpler tests, probably
> because I wasn't changing enough settings to trigger the necessary
> watermark updates.
> 
> The behavior was puzzling since some of the plane updates actually would
> take effect (e.g., PLANE_POS being zeroed would move the plane back to
> 0,0), but others wouldn't (most notable the enable/disable bit in
> PLANE_CTL).  So the result might be a garbage rectangle in the upper
> left corner of the screen or a storm of GTT page faults that would bring
> make the system unusable.
> 
> Is this actually expected behavior that's documented somewhere in the
> bspec, or is it just something you've discovered through
> experimentation?  I couldn't find any explanation for updates getting
> partially unarmed when I went through the bspec a while back, but I may
> have overlooked something.

I don't see the cancellation behaviour mentioned in the current spec.
It was actually documented for gen3 cursors (see eg. [1]) but apart
from that there is no mention of it in any spec AFAICS. I believe
only the cursors had this behaviour on gen3, and I think even that
was changed again to not cancel around the gen4-5 timeframe. SKL
seems to have re-introduced it for all planes for whatever reason.
But I've never performed an exhaustive test on all the
platforms/planes to confirm which way each one works.

[1] commit d34cfebbf9cc ("drm/i915: Fix cursor updates on some platforms")

> 
> > - overlapping DDB allocations can hard hang the box. So any vblank that
> >   sneaks in while we've partiially reprogrammed the ddb could be a
> >   death sentence. A suggested solution was to turn off the planes
> >   before ddb reprogramming but that didn't work out so well when I
> >   tried it due to the whole cancellation thing.
> > 
> > So I pulled in the ddb/wm programming into the normal plane update.
> > That means no more accidental cancellations from elsewhere.
> > 
> > And to avoid any ddb overlaps we simply sequence the plane updates
> > carefully. It's pretty much the same algorithm that we use to avoid
> > ddb overlaps between pipes, with the exception that we don't need the
> > vblank waits. So if a vblank does sneak at any point during the
> > sequence we can be assured that the partially latched state does not
> > have any overlapping ddb allocations.
> 
> Sounds reasonable.  Do you think we should try to land your work before
> Maarten's gen11 NV12 patches?

I wouldn't expect significant rebase hurdles from the nv12 work.
So I'm ok with landing the nv12 stuff first.

I'm not 100% sure I managed to handle the cursor ddb correctly in
that branch. We always allocate a ddb chunk for the cursor even
if it's disabled so it behaves slightly differently compared to
the other planes when it comes to updating the ddb from
.disable_plane(). In fact I don't even remember anymore how I
handled that case. So there might still be some work left to
polish that branch. I'm a bit busy with other things atm, so
it'll probably be a while. Unless someone else wants to pick
up where I left off, which is totally fine by me.

-- 
Ville Syrjälä
Intel