Re: [Mesa-dev] [PATCH 4/4] i965: Use intel_bufferobj_buffer() wrapper in image surface state setup.
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.
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
[Mesa-dev] [PATCH 4/4] i965: Use intel_bufferobj_buffer() wrapper in image surface state setup.
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 Reported-by: Nanley CheryCc: 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 : +intel_bufferobj_buffer(brw, intel_buffer_object(obj->BufferObject), + obj->BufferOffset, buffer_size, + access != GL_READ_ONLY); 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