[PATCH] Remove spurious use of MI_SET_CONTEXT.

[Keith Whitwell]

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.

> 
> 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.

>> 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.

Keith