Re: [Mesa-dev] [PATCH 03/18] i965: Share the workaround bo between all contexts
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 > > --- > > 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 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 03/18] i965: Share the workaround bo between all contexts
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 > --- > 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. > > + 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? > return true; > } > > diff --git a/src/mesa/drivers/dri/i965/intel_screen.h > b/src/mesa/drivers/dri/i965/intel_screen.h > index 941e0fc..e55fddb 100644 > --- a/src/mesa/drivers/dri/i965/intel_screen.h > +++ b/src/mesa/drivers/dri/i965/intel_screen.h > @@ -60,6 +60,7 @@ struct intel_screen > bool has_context_reset_notification; > > dri_bufmgr *bufmgr; > + drm_intel_bo *workaround_bo; > > /** > * A unique ID for shader programs. > signature.asc Description: This is a digitally signed message part. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 03/18] i965: Share the workaround bo between all contexts
On Mon, Jul 06, 2015 at 04:29:49PM +0300, Martin Peres wrote: > > > On 06/07/15 13:33, 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 > >--- > > 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; > >+ 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); > > I guess this is OK because after the last context got destroyed the > screen would get destroyed? Sure, but not taking a reference for each would get a little strange and you would still need a comment to explain why not following the standard idiom is safe. Then when we introduce context local brw_bo, the conversion from reference to local is much cleaner with the existing ref. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 03/18] i965: Share the workaround bo between all contexts
On 06/07/15 13:33, 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 --- 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; + 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); I guess this is OK because after the last context got destroyed the screen would get destroyed? In any case, I don't get the point of reference counting when we obviously only need to free the buffer when the screen gets freed. 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); + return true; } diff --git a/src/mesa/drivers/dri/i965/intel_screen.h b/src/mesa/drivers/dri/i965/intel_screen.h index 941e0fc..e55fddb 100644 --- a/src/mesa/drivers/dri/i965/intel_screen.h +++ b/src/mesa/drivers/dri/i965/intel_screen.h @@ -60,6 +60,7 @@ struct intel_screen bool has_context_reset_notification; dri_bufmgr *bufmgr; + drm_intel_bo *workaround_bo; /** * A unique ID for shader programs. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 03/18] i965: Share the workaround bo between all contexts
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 --- 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; + 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); + return true; } diff --git a/src/mesa/drivers/dri/i965/intel_screen.h b/src/mesa/drivers/dri/i965/intel_screen.h index 941e0fc..e55fddb 100644 --- a/src/mesa/drivers/dri/i965/intel_screen.h +++ b/src/mesa/drivers/dri/i965/intel_screen.h @@ -60,6 +60,7 @@ struct intel_screen bool has_context_reset_notification; dri_bufmgr *bufmgr; + drm_intel_bo *workaround_bo; /** * A unique ID for shader programs. -- 2.1.4 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev