[PATCH] Remove spurious use of MI_SET_CONTEXT.

Mon Dec 3 12:16:22 PST 2007

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?

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

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

> In other words, I don't think this is a good idea...

But I think it's an excellent idea :)

Kristian

> Keith
>
>
> Kristian Høgsberg wrote:
> > From: Kristian Høgsberg <krh at temari.boston.redhat.com>
> >
> > The driver uses MI_SET_CONTEXT to save the current state before
> > emitting the invariant 3d state.  Nothing ever restores this state though,
> > so this patch removes that.  This was also the only remaining use of
> > the ring buffer in TTM mode, so we're one step closer to a ring-buffer
> > free DDX driver.
> > ---
> >  src/common.h      |    2 +-
> >  src/i830.h        |    2 --
> >  src/i830_driver.c |   11 -----------
> >  src/i830_memory.c |   10 ----------
> >  4 files changed, 1 insertions(+), 24 deletions(-)
> >
> > diff --git a/src/common.h b/src/common.h
> > index f558e8f..46bfa8e 100644
> > --- a/src/common.h
> > +++ b/src/common.h
> > @@ -232,7 +232,7 @@ union intfloat {
> >
> >  #define BEGIN_LP_RING(n)                                             \
> >       RING_LOCALS                                                     \
> > -     DO_LP_RING(n)                                                   \
> > +     DO_LP_RING(n)
> >
> >  /* Memory mapped register access macros */
> >  #define INREG8(addr)        *(volatile CARD8  *)(RecPtr->MMIOBase + (addr))
> > diff --git a/src/i830.h b/src/i830.h
> > index 9387651..cbc1b93 100644
> > --- a/src/i830.h
> > +++ b/src/i830.h
> > @@ -404,8 +404,6 @@ typedef struct _I830Rec {
> >     void (*PointerMoved)(int, int, int);
> >     CreateScreenResourcesProcPtr    CreateScreenResources;
> >
> > -   i830_memory *logical_context;
> > -
> >  #ifdef XF86DRI
> >     i830_memory *back_buffer;
> >     i830_memory *third_buffer;
> > diff --git a/src/i830_driver.c b/src/i830_driver.c
> > index 6393a61..27df05b 100644
> > --- a/src/i830_driver.c
> > +++ b/src/i830_driver.c
> > @@ -2254,17 +2254,6 @@ IntelEmitInvarientState(ScrnInfoPtr pScrn)
> >     if (*pI830->last_3d != LAST_3D_OTHER)
> >        return;
> >
> > -   ctx_addr = pI830->logical_context->offset;
> > -   assert((pI830->logical_context->offset & 2047) == 0);
> > -   {
> > -      BEGIN_LP_RING(2);
> > -      OUT_RING(MI_SET_CONTEXT);
> > -      OUT_RING(pI830->logical_context->offset |
> > -            CTXT_NO_RESTORE |
> > -            CTXT_PALETTE_SAVE_DISABLE | CTXT_PALETTE_RESTORE_DISABLE);
> > -      ADVANCE_LP_RING();
> > -   }
> > -
> >     if (!IS_I965G(pI830))
> >     {
> >        if (IS_I9XX(pI830))
> > diff --git a/src/i830_memory.c b/src/i830_memory.c
> > index 85b6528..6287bab 100644
> > --- a/src/i830_memory.c
> > +++ b/src/i830_memory.c
> > @@ -347,7 +347,6 @@ i830_reset_allocations(ScrnInfoPtr pScrn)
> >      pI830->exa_offscreen = NULL;
> >      pI830->exa_965_state = NULL;
> >      pI830->overlay_regs = NULL;
> > -    pI830->logical_context = NULL;
> >  #ifdef XF86DRI
> >      pI830->back_buffer = NULL;
> >      pI830->third_buffer = NULL;
> > @@ -1356,15 +1355,6 @@ i830_allocate_2d_memory(ScrnInfoPtr pScrn)
> >       pI830->SWCursor = TRUE;
> >      }
> >
> > -    /* Space for the X Server's 3D context.  32k is fine for right now. */
> > -    pI830->logical_context = i830_allocate_memory(pScrn, "logical 3D context",
> > -                                               KB(32), GTT_PAGE_SIZE, 0);
> > -    if (pI830->logical_context == NULL) {
> > -     xf86DrvMsg(pScrn->scrnIndex, X_WARNING,
> > -                "Failed to allocate logical context space.\n");
> > -     return FALSE;
> > -    }
> > -
> >      /* even in XAA, 965G needs state mem buffer for rendering */
> >      if (IS_I965G(pI830) && !pI830->noAccel && pI830->exa_965_state == NULL) {
> >       pI830->exa_965_state =
>
>
>