[PATCH] Remove spurious use of MI_SET_CONTEXT.

[Kristian Høgsberg]

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?

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.  Which leaves us with 2) which isn't all that
complicated, in fact I think I have something running here (patches
forthcoming).

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.

cheers,
Kristian