[Mesa-dev] [PATCH 1/3] i965: solve cubemap negative x/y/z faces buffer offset issue in dEQP.
Jason Ekstrand
jason at jlekstrand.net
Thu Oct 6 16:37:50 UTC 2016
On Wed, Oct 5, 2016 at 9:11 PM, Jason Ekstrand <jason at jlekstrand.net> wrote:
> On Wed, Oct 5, 2016 at 7:05 PM, Xu, Randy <randy.xu at 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.
>
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:jason at jlekstrand.net]
>> *Sent:* Thursday, October 6, 2016 1:58 AM
>> *To:* Xu, Randy <randy.xu at intel.com>
>> *Cc:* Palli, Tapani <tapani.palli at intel.com>;
>> mesa-dev at 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.xu at 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:jason at jlekstrand.net]
>> *Sent:* Tuesday, October 4, 2016 11:59 PM
>> *To:* Palli, Tapani <tapani.palli at intel.com>
>> *Cc:* Xu, Randy <randy.xu at intel.com>; mesa-dev at lists.freedesktop.org;
>> Xu at 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.palli at 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.xu at 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.xu at 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.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
>> mesa-dev at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>>
>>
>>
>>
>>
>> _______________________________________________
>>
>> mesa-dev mailing list
>>
>> mesa-dev at lists.freedesktop.org
>>
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>>
>>
>>
>>
>>
>>
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20161006/a222135c/attachment-0001.html>
More information about the mesa-dev
mailing list