[PATCH] Remove spurious use of MI_SET_CONTEXT.

[Kristian Høgsberg]

On Dec 3, 2007 3:37 PM, Keith Whitwell <keith at tungstengraphics.com> wrote:
> Kristian Høgsberg wrote:
> > On Dec 3, 2007 2:22 PM, Keith Whitwell <keith at tungstengraphics.com> wrote:
> >
> > Hi Keith,
> >
> > First off: are you talking about MI_SET_CONTEXT as specified in the
> > docs, or are you referring to bugs in the hardware you've come across?
>
> This is just by memory -- which is pretty fallible.
>
> But -- it's always been the case that leaving any bit of hardware state
> uninitialized is asking for trouble, and I'm sure that one of the cases
> where I've tracked down hangs on startup was attributed to MI_SET_CONTEXT.
>
> Things differ from machine to machine, and in this case I believe the
> driver worked fine with development hardware, but the release version
> set things up differently somehow, and required this state to be
> initialized by hand.
>
> I guess I'm saying that you should make sure at least that this gets set
> once to NULL at system startup, no matter what else you do.
>
>
> >> It is necessary to do at least on MI_SET_CONTEXT on driver
> >> initialization, otherwise hardware will hang.  Possibly you've only
> >> tested on hardware that has run an older version of the driver once
> >> already, or possibly there is now another component (eg the EXA driver)
> >> emitting these.
> >
> > The patch is against the DDX driver to remove the use of
> > MI_SET_CONTEXT there.  As far as I can tell, there is no use of
> > MI_SET_CONTEXT in the DRI driver (except the instruction decoder).
>
> Yes, but this may change.  Keith's indicated he's interested in it, and
> now that we have ttm, the possibility is there for this instruction to
> be of some use.
>
> > So
> > the DRI driver does not emit any, and with the patch above, no part of
> > the system emits MI_SET_CONTEXT.  I've tested it on a G33 directly
> > from power-up, and I don't see anything in the docs that imply this is
> > necessary.  Also the use of MI_SET_CONTEXT here has the
> > CTXT_NO_RESTORE bit set, which means it doesn't restore anything, it
> > only save the context into a DDX driver private area and it's the only
> > part of the driver that touches the logical_context buffer.  Not mesa,
> > nor the drm has access to this buffer and doesn't use MI_SET_CONTEXT
> > anyway.  All this code does is to save the state off into a buffer and
> > nothing ever looks at it again.
> >
> >> It is also necessary to do an MI_SET_CONTEXT in case there is a 3d
> >> client out there *really using* MI_SET_CONTEXT.
> >
> > Yes, but for that to work there has to be a MI_SET_CONTEXT somewhere
> > in the system that actually restores the saved state.  The patch
> > removes the only MI_SET_CONTEXT use in the entires stack and all it
> > ever did was to save the state.
>
> OK.
>
>
> >> IE. if I modify the EXA driver to use MI_SET_CONTEXT, and you remove
> >> this code from the 3d driver, you will get unexpected results.  This
> >> MI_SET_CONTEXT would cause the other context to be saved -- ie the
> >> hardware only does something on transitions between different hardware
> >> contexts, and it is only with this packet that it knows there is a
> >> context switch...
> >
> > If we want to use MI_SET_CONTEXT from userspace to swap contexts, both
> > the dri and ddx drivers have to be careful to swap in their state
> > (while saving the old state) before changing the state, and they have
> > to do it under the lock to make sure that they don't get scheduled out
> > between restoring their state and manipulating it.  Of course, right
> > now, nothing relies on MI_SET_CONTEXT, but both the ddx and dri
> > drivers do full state emission when they detect that they lost the
> > context.
>
> I think there's a misunderstanding here -- why would they need the lock?
>
> Just emit MI_SET_CONTEXT in place of the current batchbuffer preamble.
> Nothing can interrupt a batchbuffer half-way through in our system.

MI_SET_CONTEXT has to go in a ring buffer, it can't go in a batch
buffer. I've only just started to read the docs, but there seem to be
a distinction in the instruction set design that priviledged
instructions has to go in a ring buffer, but regular rendering can go
either place.  Thus, if you restrict access to the ring buffer and
require clients to submit batch buffers, you have a fair shot at
implementing a sandboxed environment where clients can't mess up other
clients state (ie with instructions such as MI_SET_CONTEXT).

So the lock is needed to access the ring buffer in this case, even if
you just submit a MI_SET_CONTEXT followed by a MI_START_BATCH_BUFFER.

> > I don't think using it from userspace is that interesting though.  I'd
> > like to move the MI_SET_CONTEXT use to the drm, so that each batch
> > buffer command is prefixed by a MI_SET_CONTEXT command.  The batch
> > buffer is stored as part of the kernel side drm_context_t and
> > userspace needs to specify the context in the super-ioctl.  This way,
> > nothing in userspace will need to touch the ring and having the DRM
> > enforce context switches like this prevents state changes ending up in
> > the wrong context.  And doing an MI_SET_CONTEXT is free if it's not
> > actually switching to a new context.
>
> Why not just have the userspace do this?  IE, userspace allocates a
> page-sized buffer to hold the context and always emits MI_SET_CONTEXT
> and a relocation itself.
>
> I don't see why the kernel needs to have anything to do with it -
> there's nothing special about this operation that I can see requiring
> kernel supervision, and no need to use the lock.

As discussed above, to avoid user space ring buffer access and with
it, user space locking.  I agree that if we could use MI_SET_CONTEXT
from a batch buffer, there would be no need for this, when you think
about it, it makes sense that it's restricted to ring buffer usage,
and it won't incur a lot of overhead/complexity in the kernel.

> >> In other words, I don't think this is a good idea...
> >
> > But I think it's an excellent idea :)
>
> As long as you run the packet once, somewhere, I don't mind.  But humor
> me and at least do it one time with NULL parameters at startup.

Yup, definitely.  The docs detail the startup sequence, and explicity
states that the first time a new context is used, MI_SET_CONTEXT has
to be invoked with the NO_RESTORE flag to avoid restoring from
uninitialized memory.  When the context is later swapped out, its
current state is written to the memory allocated to back it, at which
point is it fully initialized and can be restored from.

Kristian