Re: [PATCH v12] drm/i915: Extend LRC pinning to cover GPU context writeback

From: Daniel Vetter <daniel@ffwll.ch>
To: Nick Hoath <nicholas.hoath@intel.com>
Cc: "intel-gfx@lists.freedesktop.org" <intel-gfx@lists.freedesktop.org>
Subject: Re: [PATCH v12] drm/i915: Extend LRC pinning to cover GPU context writeback
Date: Tue, 26 Jan 2016 11:08:18 +0100	[thread overview]
Message-ID: <20160126100818.GZ11240@phenom.ffwll.local> (raw)
In-Reply-To: <56A73FCE.8050008@intel.com>

On Tue, Jan 26, 2016 at 09:43:42AM +0000, Nick Hoath wrote:
> On 25/01/2016 18:19, Daniel Vetter wrote:
> >On Fri, Jan 22, 2016 at 02:25:27PM +0000, Nick Hoath wrote:
> >>Use the first retired request on a new context to unpin
> >>the old context. This ensures that the hw context remains
> >>bound until it has been written back to by the GPU.
> >>Now that the context is pinned until later in the request/context
> >>lifecycle, it no longer needs to be pinned from context_queue to
> >>retire_requests.
> >>This fixes an issue with GuC submission where the GPU might not
> >>have finished writing back the context before it is unpinned. This
> >>results in a GPU hang.
> >>
> >>v2: Moved the new pin to cover GuC submission (Alex Dai)
> >>     Moved the new unpin to request_retire to fix coverage leak
> >>v3: Added switch to default context if freeing a still pinned
> >>     context just in case the hw was actually still using it
> >>v4: Unwrapped context unpin to allow calling without a request
> >>v5: Only create a switch to idle context if the ring doesn't
> >>     already have a request pending on it (Alex Dai)
> >>     Rename unsaved to dirty to avoid double negatives (Dave Gordon)
> >>     Changed _no_req postfix to __ prefix for consistency (Dave Gordon)
> >>     Split out per engine cleanup from context_free as it
> >>     was getting unwieldy
> >>     Corrected locking (Dave Gordon)
> >>v6: Removed some bikeshedding (Mika Kuoppala)
> >>     Added explanation of the GuC hang that this fixes (Daniel Vetter)
> >>v7: Removed extra per request pinning from ring reset code (Alex Dai)
> >>     Added forced ring unpin/clean in error case in context free (Alex Dai)
> >>v8: Renamed lrc specific last_context to lrc_last_context as there
> >>     were some reset cases where the codepaths leaked (Mika Kuoppala)
> >>     NULL'd last_context in reset case - there was a pointer leak
> >>     if someone did reset->close context.
> >>v9: Rebase over "Fix context/engine cleanup order"
> >>v10: Rebase over nightly, remove WARN_ON which caused the
> >>     dependency on dev.
> >>v11: Kick BAT rerun
> >>v12: Rebase
> >>
> >>Signed-off-by: Nick Hoath <nicholas.hoath@intel.com>
> >>Issue: VIZ-4277
> >
> >When resending patches, please include everyone who ever commented on this
> >in Cc: lines here. It's for the record and helps in assigning blame when
> >things inevitably blow up again ;-)
> 
> Even when it's just a resend to cause a BAT run for coverage?

Yes, it's to have a record of folks who participated in a patch's
discussion in git. Since when it blows up you really want to know whom are
the folks you should Cc:, and for that it's best to have a Cc: list
ready-made in the offending patch.

I even go as far as adding Cc: lines for testers/reviwers on top of the
r-b/t-b tags I'm adding when resending, for extra laziness when the
inevitable revert comes around. But that's a bit over the top ;-)

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx