Re: [Mesa-dev] [PATCH 4/4] i965: Use intel_bufferobj_buffer() wrapper in image surface state setup.

[Nanley Chery]

On Fri, May 11, 2018 at 04:55:27PM -0700, Nanley Chery wrote:
> On Mon, Mar 19, 2018 at 11:26:59AM -0700, Francisco Jerez wrote:
> > Instead of directly using intel_obj->buffer.  Among other things
> > intel_bufferobj_buffer() will update intel_buffer_object::
> > gpu_active_start/end, which are used by glBufferSubData() to decide
> > which path to take.  Fixes a failure in the Piglit
> > ARB_shader_image_load_store-host-mem-barrier Buffer Update/WaW tests,
> > which could be reproduced with a non-standard glGetTexSubImage
> > implementation (see bug report).
> > 
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105351
> 
> What do you think about leaving the bug open until we create a piglit
> test to reproduce it? We could create a new bug otherwise.
> 

Nevermind about leaving this open and creating another report. I'll
close the bug. Thanks!

-Nanley

> > Reported-by: Nanley Chery 
> > Cc: mesa-sta...@lists.freedesktop.org
> > ---
> >  src/mesa/drivers/dri/i965/brw_wm_surface_state.c | 8 +---
> >  1 file changed, 5 insertions(+), 3 deletions(-)
> > 
> > diff --git a/src/mesa/drivers/dri/i965/brw_wm_surface_state.c 
> > b/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
> > index 2ab15af793a..5d4c84bb55a 100644
> > --- a/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
> > +++ b/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
> > @@ -1510,14 +1510,16 @@ update_image_surface(struct brw_context *brw,
> >const unsigned format = get_image_format(brw, u->_ActualFormat, 
> > access);
> >  
> >if (obj->Target == GL_TEXTURE_BUFFER) {
> > - struct intel_buffer_object *intel_obj =
> > -intel_buffer_object(obj->BufferObject);
> >   const unsigned texel_size = (format == ISL_FORMAT_RAW ? 1 :
> >
> > _mesa_get_format_bytes(u->_ActualFormat));
> >   const unsigned buffer_size = buffer_texture_range_size(brw, obj);
> > + struct brw_bo *const bo = !obj->BufferObject ? NULL :
> 
> Interesting. Did the old code wrongly assume that
> obj->BufferObject != NULL? Maybe we need a test that tries to do image
> load/store on a texture buffer without a backing buffer object?
> 
> > +intel_bufferobj_buffer(brw, 
> > intel_buffer_object(obj->BufferObject),
> > +   obj->BufferOffset, buffer_size,
> > +   access != GL_READ_ONLY);
> >  
> 
> Looks right to me. The comment above the helper function says:
>[...]
>* Anywhere that uses buffer objects in the pipeline should be using this to
>* mark the range of the buffer that is being accessed by the pipeline.
>*/
>   
> ...and that is what we're doing here. This patch is
> Reviewed-by: Nanley Chery 
> 
> >   brw_emit_buffer_surface_state(
> > -brw, surf_offset, intel_obj->buffer, obj->BufferOffset,
> > +brw, surf_offset, bo, obj->BufferOffset,
> >  format, buffer_size, texel_size,
> >  access != GL_READ_ONLY ? RELOC_WRITE : 0);
> >  
> > -- 
> > 2.16.1
> > 
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 4/4] i965: Use intel_bufferobj_buffer() wrapper in image surface state setup.

[Nanley Chery]

On Mon, Mar 19, 2018 at 11:26:59AM -0700, Francisco Jerez wrote:
> Instead of directly using intel_obj->buffer.  Among other things
> intel_bufferobj_buffer() will update intel_buffer_object::
> gpu_active_start/end, which are used by glBufferSubData() to decide
> which path to take.  Fixes a failure in the Piglit
> ARB_shader_image_load_store-host-mem-barrier Buffer Update/WaW tests,
> which could be reproduced with a non-standard glGetTexSubImage
> implementation (see bug report).
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105351

What do you think about leaving the bug open until we create a piglit
test to reproduce it? We could create a new bug otherwise.

> Reported-by: Nanley Chery 
> Cc: mesa-sta...@lists.freedesktop.org
> ---
>  src/mesa/drivers/dri/i965/brw_wm_surface_state.c | 8 +---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/src/mesa/drivers/dri/i965/brw_wm_surface_state.c 
> b/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
> index 2ab15af793a..5d4c84bb55a 100644
> --- a/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
> +++ b/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
> @@ -1510,14 +1510,16 @@ update_image_surface(struct brw_context *brw,
>const unsigned format = get_image_format(brw, u->_ActualFormat, 
> access);
>  
>if (obj->Target == GL_TEXTURE_BUFFER) {
> - struct intel_buffer_object *intel_obj =
> -intel_buffer_object(obj->BufferObject);
>   const unsigned texel_size = (format == ISL_FORMAT_RAW ? 1 :
>
> _mesa_get_format_bytes(u->_ActualFormat));
>   const unsigned buffer_size = buffer_texture_range_size(brw, obj);
> + struct brw_bo *const bo = !obj->BufferObject ? NULL :

Interesting. Did the old code wrongly assume that
obj->BufferObject != NULL? Maybe we need a test that tries to do image
load/store on a texture buffer without a backing buffer object?

> +intel_bufferobj_buffer(brw, 
> intel_buffer_object(obj->BufferObject),
> +   obj->BufferOffset, buffer_size,
> +   access != GL_READ_ONLY);
>  

Looks right to me. The comment above the helper function says:
   [...]
   * Anywhere that uses buffer objects in the pipeline should be using this to
   * mark the range of the buffer that is being accessed by the pipeline.
   */
  
...and that is what we're doing here. This patch is
Reviewed-by: Nanley Chery 

>   brw_emit_buffer_surface_state(
> -brw, surf_offset, intel_obj->buffer, obj->BufferOffset,
> +brw, surf_offset, bo, obj->BufferOffset,
>  format, buffer_size, texel_size,
>  access != GL_READ_ONLY ? RELOC_WRITE : 0);
>  
> -- 
> 2.16.1
> 
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev