Re: [Intel-gfx] [PATCH 25/25] i965: hw context support

[Ben Widawsky <ben@bwidawsk.net>]

On Mon, 4 Jun 2012 16:01:54 -0700
Paul Berry <stereotype441@gmail.com> wrote:

> On 4 June 2012 14:43, Ben Widawsky <ben@bwidawsk.net> wrote:
> 
> > Based off of a patch from Ken Graunke. I just modified it for a more
> > modern mesa (also don't allow contexts on blit ring).
> >
> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> > ---
> >  src/mesa/drivers/dri/i965/brw_context.c        |    1 +
> >  src/mesa/drivers/dri/i965/brw_vtbl.c           |    5 ++++-
> >  src/mesa/drivers/dri/intel/intel_batchbuffer.c |    9 +++++++--
> >  src/mesa/drivers/dri/intel/intel_context.c     |    2 ++
> >  src/mesa/drivers/dri/intel/intel_context.h     |    2 +-
> >  5 files changed, 15 insertions(+), 4 deletions(-)
> >
> > diff --git a/src/mesa/drivers/dri/i965/brw_context.c
> > b/src/mesa/drivers/dri/i965/brw_context.c
> > index f7073cd..d4159c7 100644
> > --- a/src/mesa/drivers/dri/i965/brw_context.c
> > +++ b/src/mesa/drivers/dri/i965/brw_context.c
> > @@ -298,6 +298,7 @@ brwCreateContext(int api,
> >
> >    brw->prim_restart.in_progress = false;
> >    brw->prim_restart.enable_cut_index = false;
> > +   intel->hw_ctx = drm_intel_gem_context_create(intel->bufmgr);
> >
> >    brw_init_state( brw );
> >
> > diff --git a/src/mesa/drivers/dri/i965/brw_vtbl.c
> > b/src/mesa/drivers/dri/i965/brw_vtbl.c
> > index 5699749..d9fd2cb 100644
> > --- a/src/mesa/drivers/dri/i965/brw_vtbl.c
> > +++ b/src/mesa/drivers/dri/i965/brw_vtbl.c
> > @@ -170,7 +170,10 @@ static void brw_new_batch( struct intel_context
> > *intel )
> >     * This is probably not as severe as on 915, since almost all of our
> > state
> >     * is just in referenced buffers.
> >     */
> > -   brw->state.dirty.brw |= BRW_NEW_CONTEXT | BRW_NEW_BATCH;
> > +   if (intel->hw_ctx == NULL)
> > +      brw->state.dirty.brw |= BRW_NEW_CONTEXT;
> > +
> > +   brw->state.dirty.brw |= BRW_NEW_BATCH;
> >
> 
> The comment above this change ("Mark all context state as needing to be
> re-emitted.") is no longer accurate.  Perhaps change it to something like
> this?
> 
> "If the kernel supports hardware contexts, then most hardware state is
> preserved between batches; we only need to re-emit state that is required
> to be in every batch.  Otherwise we need to re-emit all the state that
> would otherwise be stored in the context (which for all intents and
> purposes means everything)."
> 
> Also, I think it would be ok to delete the comment "This is probably not as
> severe as on 915 ... referenced buffers"--that comment is mostly just a
> rationalization for not having implemented hardware context support
> earlier, and not a very convincing one at that :)
> 
> 
> >
> >    /* Assume that the last command before the start of our batch was a
> >     * primitive, for safety.
> > diff --git a/src/mesa/drivers/dri/intel/intel_batchbuffer.c
> > b/src/mesa/drivers/dri/intel/intel_batchbuffer.c
> > index 76a69f7..7ba141d 100644
> > --- a/src/mesa/drivers/dri/intel/intel_batchbuffer.c
> > +++ b/src/mesa/drivers/dri/intel/intel_batchbuffer.c
> > @@ -188,8 +188,13 @@ do_flush_locked(struct intel_context *intel)
> >       if (ret == 0) {
> >          if (unlikely(INTEL_DEBUG & DEBUG_AUB) && intel->vtbl.annotate_aub)
> >             intel->vtbl.annotate_aub(intel);
> > -        ret = drm_intel_bo_mrb_exec(batch->bo, 4*batch->used, NULL, 0, 0,
> > -                                    flags);
> > +        if (intel->hw_ctx == NULL || batch->is_blit) {
> > +           ret = drm_intel_bo_mrb_exec(batch->bo, 4*batch->used, NULL, 0,
> > 0,
> > +                                       flags);
> > +        } else {
> > +           ret = drm_intel_gem_bo_context_exec(batch->bo, intel->hw_ctx,
> > +                                           4 * batch->used, flags);
> > +        }
> >       }
> >    }
> >
> > diff --git a/src/mesa/drivers/dri/intel/intel_context.c
> > b/src/mesa/drivers/dri/intel/intel_context.c
> > index 9deb4ca..46c2492 100644
> > --- a/src/mesa/drivers/dri/intel/intel_context.c
> > +++ b/src/mesa/drivers/dri/intel/intel_context.c
> > @@ -593,6 +593,8 @@ intelInitContext(struct intel_context *intel,
> >    if (intelScreen->bufmgr == NULL)
> >       return false;
> >
> > +   intel->hw_ctx = NULL;
> > +
> >    /* Can't rely on invalidate events, fall back to glViewport hack */
> >    if (!driContextPriv->driScreenPriv->dri2.useInvalidate) {
> >       intel->saved_viewport = functions->Viewport;
> > diff --git a/src/mesa/drivers/dri/intel/intel_context.h
> > b/src/mesa/drivers/dri/intel/intel_context.h
> > index cc3ee0d..c026fea 100644
> > --- a/src/mesa/drivers/dri/intel/intel_context.h
> > +++ b/src/mesa/drivers/dri/intel/intel_context.h
> > @@ -226,7 +226,7 @@ struct intel_context
> >    int urb_size;
> >
> >    struct intel_batchbuffer batch;
> > -
> > +   drm_intel_context *hw_ctx;
> >    drm_intel_bo *first_post_swapbuffers_batch;
> >    bool need_throttle;
> >    bool no_batch_wrap;
> > --
> > 1.7.10.2
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
> 
> I'm kind of surprised to see a call to drm_intel_gem_context_create(), but
> no call anywhere to a clean-up function that destroys the context.  Was
> that an oversight, or is there a reason why it's unnecessary?  If it's the
> latter, a comment in brw_destroy_context() would be helpful.

Destroying a context is not required, though it is desirable. The
context will be destroyed when the DRM file handle is closed. I think I
had that info in a comment somewhere, but I forget exactly where it is.
I'd propose mesa people find a good place for destroy as the best
solution :-)


-- 
Ben Widawsky, Intel Open Source Technology Center