Re: [PATCH 1/8] drm/i915: Get rid of the synchronous flag in switch_mm

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, Jun 19, 2014 at 02:00:24PM +0000, Mateo Lozano, Oscar wrote:
> > -----Original Message-----
> > From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel
> > Vetter
> > Sent: Wednesday, June 18, 2014 8:51 PM
> > To: Mateo Lozano, Oscar
> > Cc: intel-gfx@lists.freedesktop.org
> > Subject: Re: [Intel-gfx] [PATCH 1/8] drm/i915: Get rid of the synchronous flag
> > in switch_mm
> > 
> > On Wed, Jun 18, 2014 at 05:15:35PM +0100, oscar.mateo@intel.com wrote:
> > > From: Oscar Mateo <oscar.mateo@intel.com>
> > >
> > > As a first step, split switch_mm into two different functions:
> > > sync_switch_mm (currently used by ppgtt_enable) and switch_mm (used by
> > > the outside world).
> > >
> > > Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
> > 
> > I guess that jira task description was a bit unclear - removing the bool
> > synchronous argument isn't the goal itself. The idea was to rework the gpu
> > reset sequence so that it matches the same sequence we use at driver load
> > and so can use the same code. This patch removes the bool synchronous, but
> > the logical split with two different ways to load ppgtt is still there.
> 
> No, I realized I didn´t really do anything, but I saw the possibility of saving one special casing in Execlists and I jumped for it, forgetting about the JIRA task (psychological fixation, I guess)
> Anyway, I was going to ask for more details on the JIRA...
> 
> > The reason for aiming as hard as possible to use the exact same code for
> > driver load, gpu reset and runtime pm/system resume is that we've simply
> > seen too many bugs due to slight variations and unintended omissions.
> > 
> > If for some really odd reason this is impossible (e.g. when the gpu needs a
> > special dance when coming out of reset) then we need a giant comment
> > explaining the magic. But without such a clear reason the goal should always
> > be to have just 1 single autorative piece of code for a given state transition.
> 
> Ok. How about this: on ring init, I manually set the Aliasing PPGTT and
> the default context object synchronously via MMIO writes (the Aliasing
> PPGTT via the ppgtt->sync_switch_mm() and the default context directly
> using the CCID). I assume that, whenever we are in ring init, the GPU is
> sufficiently idle to manually frob all the required registers. That way,
> I get rid of i915_gem_context_enable() completely and I don´t have to do
> any special casing at all.  Ring commands (PPGTT switch and
> MI_SET_CONTEXT) are then left completely for the normal do_switch().
> What do you think?

Can't we do the ring_init loading the same way as a normal context switch?
That's kinda my dream land thing. Of course if the initial loading is
sufficiently differnt then we have a problem. So essentially do_switch
would check whether the ppgtt changes and load that as part of the
context.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch