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

2015-07-08 Thread 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 ch...@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
___
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

2015-07-06 Thread Martin Peres 



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 ch...@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;
  
+   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

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

2015-07-06 Thread Chris Wilson 
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 ch...@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;
 +   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

2015-07-06 Thread Kenneth Graunke 
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 ch...@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.

  
 +   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