[PATCH] Remove spurious use of MI_SET_CONTEXT.

[Keith Whitwell]

Kristian Høgsberg wrote:
> On Dec 3, 2007 6:50 PM, Keith Whitwell <keith at tungstengraphics.com> wrote:
>> Kristian Høgsberg wrote:
>>> On Dec 3, 2007 4:20 PM, Keith Whitwell <keith at tungstengraphics.com> wrote:
>>> ...
>>>>>> 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 had trouble understanding this -- but indeed the address for the
>>>> context is a *physical* address in system memory...  Fabulous.
>>> No, it can be a GTT offset too, but it has to present when the context
>>> is swapped out.  That is not under our control, because it happens
>>> when the next context is swapped in.  So it has to be a no-evict
>>> buffer.  We don't want to fragment the GTT with no-evict buffers for
>>> this stuff, so the physical address option looks more interesting.
>> OK.
>>
>>>> That makes the whole scheme less interesting to me as it does rely on
>>>> the kernel doing the state management, and at this point I don't really
>>>> see this as being hugely worthwhile.
>>> What did you have in mind?  The context buffers are opaque, it's just
>>> a cookie you hold on to for the hardware.  You can't peek into them and
>>> manipulate the state anyway, so it doesn't seem to me that there's a
>>> big difference in whether the kernel or user space track the state
>>> buffers and issue the MI_SET_CONTEXT.
>> I don't want to peek into the contexts (though if you do, you'll find
>> they aren't all that opaque).
>>
>> I'm just saying I'd like to avoid adding complexity to the kernel module
>> unless there's real evidence that 1) there's a performance gain and 2)
>> that we can't achieve the same effect from userspace.
> 
> Right, we're on the same page here.  However, since MI_SET_CONTEXT can
> not be used from a batch buffer, we have the following options:
> 
>   1) Don't use MI_SET_CONTEXT, always emit full state
>   2) Use MI_SET_CONTEXT from the kernel, based on drm_context_t in super-ioctl
>   3) Emit MI_SET_CONTEXT from userspace
> 
> Maybe 1) is feasible, I don't know for sure.  I'm a little confused,
> to be honest, as I keep hearing that state emission is killing
> performance and Gallium is built around state caching and tries very
> hard to avoid superfluous state emission.  At the same time, I hear
> that emitting full state is negligible and not worth optimizing...
> where's the disconnect?

I'm not against an MI_SET_CONTEXT approach.

I'm a little disappointed that it requires kernel support to achieve it, 
  otherwise it would be a no-brainer.

Even requiring kernel changes, if there is a real benefit to 
performance, we probably want to do it -- but I'd like to figure out if 
that benefit is real or not first.

Anway, regarding (1), we're talking about one statechange per 
batchbuffer, possibly one per frame in a reasonably optimized driver -- 
that is going to be negligible no matter what.  I think the only time 
you'll start to notice it is if you get large numbers of tiny 
batchbuffers being emitted for some pathological reason.

> As for 3), this requires access to the ring buffer from userspace and
> thus locking, and I think we agree that that's not something we should
> design into the system. 

It's utterly broken.  It can't be made to work in fact as the 3d client 
cannot get to the ring even if it holds the lock.

> Which leaves us with 2) which isn't all that
> complicated, in fact I think I have something running here (patches
> forthcoming).

Do you see a performance gain for 3d?  For EXA?

> In the meantime, do you have any objections to the original patch
> (removing DDX driver use of MI_SET_CONTEXT)?  If nothing else this
> allows us to move forward on making the intel drivers lockless.

I've got no problem with removing that code.  I had some concerns about 
running it once for safety at init time, but probably even that I'm OK 
with now.

The fact that it has to be done from the ring means it's pretty unlikely 
that the bugs I saw related to this packet -- I think it's probably safe 
to remove it.

Keith