On Wed, Oct 5, 2016 at 9:11 PM, Jason Ekstrand <[email protected]> wrote:
> On Wed, Oct 5, 2016 at 7:05 PM, Xu, Randy <[email protected]> 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. > Never mind... Now that I think about it, I don't think that case is possible. I think the only time we'll have a tile offset coming in from outside via an EGL image is if the texture is 2D. In that case, we won't hit the surf.dim_layout != dim_layout case and we should be fine. I think just the assert that you have below will do. > --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:[email protected]] >> *Sent:* Thursday, October 6, 2016 1:58 AM >> *To:* Xu, Randy <[email protected]> >> *Cc:* Palli, Tapani <[email protected]>; >> [email protected] >> >> *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 <[email protected]> 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:[email protected]] >> *Sent:* Tuesday, October 4, 2016 11:59 PM >> *To:* Palli, Tapani <[email protected]> >> *Cc:* Xu, Randy <[email protected]>; [email protected]; >> [email protected] >> *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 <[email protected]> >> wrote: >> >> On 10/04/2016 06:09 PM, Jason Ekstrand wrote: >> >> On Thu, Sep 29, 2016 at 11:27 PM, Xu,Randy <[email protected]> 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 <[email protected]> >> --- >> 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.im >> age.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 >> [email protected] >> https://lists.freedesktop.org/mailman/listinfo/mesa-dev >> >> >> >> >> >> _______________________________________________ >> >> mesa-dev mailing list >> >> [email protected] >> >> https://lists.freedesktop.org/mailman/listinfo/mesa-dev >> >> >> >> >> >> >> > >
_______________________________________________ mesa-dev mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/mesa-dev
