On Wed, Oct 5, 2016 at 7:05 PM, Xu, Randy <randy...@intel.com> wrote:
> Hi, Jason > > > > Do you want to add this assert in the patch? I did some test, no issue > found, but I don’t see the case that we need override the texture target in > brw_emit_surface_state, i.e. surf.dim_layout != dim_layout > > How can we create this case? And we may need another patch to solve the > issue as it’s a new corner case. > I believe we can only hit that case if we render to it and use a render target read. You probably can hit that case but it'll be a bit tricky to trigger. On second thought, I don't think an assert is right. Instead, I think we probably need to get the tile_x/y from intel_miptree_get_tile_offsets and then add that to tile_x and tile_y. I don't think we can ever end up in the case where we have tile offsets coming in from EGL *and* we have a non-zero base_level or base-array_layer. In fact, we should probably assert as much. --Jason > Thanks, > > Randy > > > > 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 3a5c573..d727526 100644 > > --- a/src/mesa/drivers/dri/i965/brw_wm_surface_state.c > > +++ b/src/mesa/drivers/dri/i965/brw_wm_surface_state.c > > @@ -109,6 +109,7 @@ brw_emit_surface_state(struct brw_context *brw, > > */ > > assert(brw->has_surface_tile_offset); > > assert(view.levels == 1 && view.array_len == 1); > > + assert(tile_x == 0 && tile_y == 0); > > > > offset += intel_miptree_get_tile_offsets(mt, view.base_level, > > view.base_array_layer, > > > > > > > > *From:* Jason Ekstrand [mailto:ja...@jlekstrand.net] > *Sent:* Thursday, October 6, 2016 1:58 AM > *To:* Xu, Randy <randy...@intel.com> > *Cc:* Palli, Tapani <tapani.pa...@intel.com>; > mesa-dev@lists.freedesktop.org > > *Subject:* Re: [Mesa-dev] [PATCH 1/3] i965: solve cubemap negative x/y/z > faces buffer offset issue in dEQP. > > > > Randy, > > I hadn't realized that we could get images in from EGL where we have a > non-zero tile_x and tile_y offset for layer 0 mip 0. That explains > things. In that case, I believe this is the correct patch. That said, I > would like to see an "assert(tile_x == 0 && tile_y == 0)" right before we > do the intel_miptree_get_tile_offset() in the case below. I don't think > those can ever happen at the same time, but if they do, I want to know. > > --Jason > > > > On Tue, Oct 4, 2016 at 5:13 PM, Xu, Randy <randy...@intel.com> wrote: > > Hi, Jason & Tapani > > > > Thanks for your review, let me introduce the dEQP failure first. > > > > In dEQP-EGL.functional.image.create.gles2_cubemap_negative_*_texture, 2D > textures are generated from all 6 faces of a Cubemap texture (64x64), and > then rendered through glDrawXXX. > > In brw_miptree_get_vertical_slice_pitch, the mt->qpitch is counted as 144. > > return h0 + h1 + (brw->gen >= 7 ? 12 : 11) * mt->valign; > // 64+32+12*4 = 144 > > > > Take the face negative_x for example, the total offset in bo is > 144(y)*64(x)*4(bpp) = 36864. > > It’s TILING_Y buffer, as the y (144) is not 32 aligned (mask_y = 31 from > intel_region_get_tile_masks), the total bo offset is divided into two > parts: 36864 = 32768 (offset 128*64*4) + 16(tile_y)*64*4 > > case I915_TILING_Y: > > *mask_x = 128 / cpp - 1; > > *mask_y = 31; > > > > > > Both the tile_y and offset are passed to texture2D in > create_mt_for_dri_image, while the tile_y is not used to count the total > offset in rendering path, that’s why I add this patch. > > Please check and comment more. > > > > Thanks, > > Randy > > > > > > > > > > *From:* Jason Ekstrand [mailto:ja...@jlekstrand.net] > *Sent:* Tuesday, October 4, 2016 11:59 PM > *To:* Palli, Tapani <tapani.pa...@intel.com> > *Cc:* Xu, Randy <randy...@intel.com>; mesa-dev@lists.freedesktop.org; > x...@freedesktop.org > *Subject:* Re: [Mesa-dev] [PATCH 1/3] i965: solve cubemap negative x/y/z > faces buffer offset issue in dEQP. > > > > On Tue, Oct 4, 2016 at 8:55 AM, Tapani Pälli <tapani.pa...@intel.com> > wrote: > > On 10/04/2016 06:09 PM, Jason Ekstrand wrote: > > On Thu, Sep 29, 2016 at 11:27 PM, Xu,Randy <randy...@intel.com> wrote: > > Add the miptree level/slice x/y_offset when count the surface offset > in brw_emit_surface_state. The surface offset has two parts, one is > from mt->offset, which should be 32 aligned in width/height for tiled > buffer; another is from mt->level[current_level].slice[current_slice]. > x/y_offset. > > This fix will solve 12 deqp failure > dEQP-EGL.functional.image.create.gles2_cubemap_negative_*_texture > > Signed-off-by: Xu,Randy <randy...@intel.com> > --- > src/mesa/drivers/dri/i965/brw_wm_surface_state.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > 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 61a4b94..3a5c573 100644 > --- a/src/mesa/drivers/dri/i965/brw_wm_surface_state.c > +++ b/src/mesa/drivers/dri/i965/brw_wm_surface_state.c > @@ -85,7 +85,8 @@ brw_emit_surface_state(struct brw_context *brw, > unsigned read_domains, unsigned write_domains) > { > const struct surface_state_info ss_info = > surface_state_infos[brw->gen]; > - uint32_t tile_x = 0, tile_y = 0; > + uint32_t tile_x = mt->level[0].slice[0].x_offset; > + uint32_t tile_y = mt->level[0].slice[0].y_offset; > > > > This isn't correct. First off, there are some fairly strict restrictions > on what we can do with tile_x and tile_y and we can't just shove x/y_offset > in there. We need to use intel_miptree_get_tile_offsets to get both a byte > offset and an intratile offset. Second, we should already be taking slices > into account for cube maps via base_array_layer where needed. > > Unfortunately, I'm not 100% sure what the correct patch is without a bit > more information about what the test is doing that causes a problem. > > > > I did take a brief look and when running the set mentioned above (for > example with ./deqp-egl --deqp-case=*EGL.functional. > image.create.gles2_cubemap_negative_*_texture) what happens is that we > never end up to the part of code calling intel_miptree_get_tile_offsets in > that function (because surf.dim_layout != dim_layout condition does not > trigger). This is just what I observed, should we just call > intel_miptree_get_tile_offsets() unconditionally then? > > > > No. Very much no. The intel_miptree_get_tile_offsets() stuff is a hack > that lets us convert non-2D things to 2D things and it comes with piles of > restrictions. > > > > > > --Jason > > > > uint32_t offset = mt->offset; > > struct isl_surf surf; > -- > 2.7.4 > > _______________________________________________ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/mesa-dev > > > > > > _______________________________________________ > > mesa-dev mailing list > > mesa-dev@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev > > > > > > >
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev