[Mesa-dev] [PATCH 03/18] i965: Share the workaround bo between all contexts

[Chris Wilson]

On Mon, Jul 06, 2015 at 09:34:10AM -0700, Kenneth Graunke wrote:
> On Monday, July 06, 2015 11:33:08 AM Chris Wilson wrote:
> > Since the workaround bo is used strictly as a write-only buffer, we need
> > only allocate one per screen and use the same one from all contexts.
> > 
> > (The caveat here is during extension initialisation, where we write into
> > and read back register values from the buffer, but that is performed only
> > once for the first context - and baring synchronisation issues should not
> > be a problem. Safer would be to move that also to the screen.)
> > 
> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > ---
> >  src/mesa/drivers/dri/i965/brw_pipe_control.c | 6 +++---
> >  src/mesa/drivers/dri/i965/intel_screen.c     | 4 ++++
> >  src/mesa/drivers/dri/i965/intel_screen.h     | 1 +
> >  3 files changed, 8 insertions(+), 3 deletions(-)
> > 
> > diff --git a/src/mesa/drivers/dri/i965/brw_pipe_control.c b/src/mesa/drivers/dri/i965/brw_pipe_control.c
> > index 7ee3cb6..05e14cd 100644
> > --- a/src/mesa/drivers/dri/i965/brw_pipe_control.c
> > +++ b/src/mesa/drivers/dri/i965/brw_pipe_control.c
> > @@ -341,12 +341,12 @@ brw_init_pipe_control(struct brw_context *brw,
> >      * the gen6 workaround because it involves actually writing to
> >      * the buffer, and the kernel doesn't let us write to the batch.
> >      */
> > -   brw->workaround_bo = drm_intel_bo_alloc(brw->bufmgr,
> > -                                           "pipe_control workaround",
> > -                                           4096, 4096);
> > +   brw->workaround_bo = brw->intelScreen->workaround_bo;
> >     if (brw->workaround_bo == NULL)
> >        return -ENOMEM;
> 
> Checking for out-of-memory conditions in code that doesn't actually
> allocate anything looks funky now.  I'd be inclined just to drop the
> -ENOMEM path and make this a void function.
> 
> Alternatively, you could just fold this into the brw_context setup and
> ditch the functions altogether.  Up to you.

Ok, will refactor this since the check should now be at the screen.
> >  
> > +   drm_intel_bo_reference(brw->workaround_bo);
> > +
> >     brw->pipe_controls_since_last_cs_stall = 0;
> >  
> >     return 0;
> > diff --git a/src/mesa/drivers/dri/i965/intel_screen.c b/src/mesa/drivers/dri/i965/intel_screen.c
> > index 839a984..cd8e6eb 100644
> > --- a/src/mesa/drivers/dri/i965/intel_screen.c
> > +++ b/src/mesa/drivers/dri/i965/intel_screen.c
> > @@ -961,6 +961,7 @@ intelDestroyScreen(__DRIscreen * sPriv)
> >  {
> >     struct intel_screen *intelScreen = sPriv->driverPrivate;
> >  
> > +   drm_intel_bo_unreference(intelScreen->workaround_bo);
> >     dri_bufmgr_destroy(intelScreen->bufmgr);
> >     driDestroyOptionInfo(&intelScreen->optionCache);
> >  
> > @@ -1096,6 +1097,9 @@ intel_init_bufmgr(struct intel_screen *intelScreen)
> >        return false;
> >     }
> >  
> > +   intelScreen->workaround_bo =
> > +      drm_intel_bo_alloc(intelScreen->bufmgr, "pipe_control w/a", 4096, 4096);
> > +
> 
> Seems a little funny to put this in intel_init_bufmgr, since it's not
> setting up libdrm - why not put it in the caller?

No reason. I was considering this to be global state associated with the
bufmgr so just liked to keep it together.

Thanks,
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre