I'm OK with this patch.
Marek
On Wed, Mar 29, 2017 at 12:57 PM, Nicolai Hähnle <nhaeh...@gmail.com> wrote:
> Hi Michal,
>
> thanks for the patch. That piglit test actually fails on radeonsi as well.
>
>
> On 28.03.2017 22:39, Michal Srb wrote:
>>
>> st_finalize_texture always accesses image at face 0, but it may not be set
>> if we are working with cubemap that had other face set.
>>
>> This fixes crash in piglit
>> same-attachment-glFramebufferTexture2D-GL_DEPTH_STENCIL_ATTACHMENT.
>
>
> Please make sure commit messages are wrapped to <75 characters.
>
> Also:
>
> Cc: mesa-sta...@lists.freedesktop.org
>
>
>> ---
>> Hi, this is my attempt to fix crash in piglit test
>> same-attachment-glFramebufferTexture2D-GL_DEPTH_STENCIL_ATTACHMENT ran with
>> LIBGL_ALWAYS_INDIRECT=1.
>> I am not sure if it is the right approach. From what I found online
>> rendering into a face of a cube texture that doesn't have all faces set
>> would be invalid, but the test passes with other drivers, so maybe it's ok.
>> This makes it pass with software rendering as well.
>
>
> I actually don't see anything in the spec that would require texture
> completeness. That makes sense, since rendering into one image of a texture
> doesn't imply using sampler state. So allowing the test to pass is good.
>
> The flip-side is that this means calling st_finalize_texture at all may not
> be the right thing to do in the FBO code (except perhaps as an opportunistic
> optimization). After all, we could have a messed up situation where there
> are incompatible mip level in a texture, and we render to one of them
> anyway.
>
> Cleaning that up would be quite involved. I think this fix is fine for now,
> since it does improve the situation:
>
> Reviewed-by: Nicolai Hähnle <nicolai.haeh...@amd.com>
>
> Let's see if there are any other opinions...
>
> Cheers,
> Nicolai
>
>
>
>>
>> src/gallium/state_trackers/dri/dri2.c | 2 +-
>> src/mesa/state_tracker/st_atom_image.c | 2 +-
>> src/mesa/state_tracker/st_atom_texture.c | 2 +-
>> src/mesa/state_tracker/st_cb_fbo.c | 2 +-
>> src/mesa/state_tracker/st_cb_texture.c | 5 +++--
>> src/mesa/state_tracker/st_cb_texture.h | 3 ++-
>> src/mesa/state_tracker/st_gen_mipmap.c | 2 +-
>> 7 files changed, 10 insertions(+), 8 deletions(-)
>>
>> diff --git a/src/gallium/state_trackers/dri/dri2.c
>> b/src/gallium/state_trackers/dri/dri2.c
>> index b50e096..ed6004f 100644
>> --- a/src/gallium/state_trackers/dri/dri2.c
>> +++ b/src/gallium/state_trackers/dri/dri2.c
>> @@ -1808,7 +1808,7 @@ dri2_interop_export_object(__DRIcontext *_ctx,
>> return MESA_GLINTEROP_INVALID_MIP_LEVEL;
>> }
>>
>> - if (!st_finalize_texture(ctx, st->pipe, obj)) {
>> + if (!st_finalize_texture(ctx, st->pipe, obj, 0)) {
>> mtx_unlock(&ctx->Shared->Mutex);
>> return MESA_GLINTEROP_OUT_OF_RESOURCES;
>> }
>> diff --git a/src/mesa/state_tracker/st_atom_image.c
>> b/src/mesa/state_tracker/st_atom_image.c
>> index 5dd2cd6..4101552 100644
>> --- a/src/mesa/state_tracker/st_atom_image.c
>> +++ b/src/mesa/state_tracker/st_atom_image.c
>> @@ -64,7 +64,7 @@ st_bind_images(struct st_context *st, struct gl_program
>> *prog,
>> struct pipe_image_view *img = &images[i];
>>
>> if (!_mesa_is_image_unit_valid(st->ctx, u) ||
>> - !st_finalize_texture(st->ctx, st->pipe, u->TexObj) ||
>> + !st_finalize_texture(st->ctx, st->pipe, u->TexObj, 0) ||
>> !stObj->pt) {
>> memset(img, 0, sizeof(*img));
>> continue;
>> diff --git a/src/mesa/state_tracker/st_atom_texture.c
>> b/src/mesa/state_tracker/st_atom_texture.c
>> index 92023e0..5b481ec 100644
>> --- a/src/mesa/state_tracker/st_atom_texture.c
>> +++ b/src/mesa/state_tracker/st_atom_texture.c
>> @@ -73,7 +73,7 @@ update_single_texture(struct st_context *st,
>> }
>> stObj = st_texture_object(texObj);
>>
>> - retval = st_finalize_texture(ctx, st->pipe, texObj);
>> + retval = st_finalize_texture(ctx, st->pipe, texObj, 0);
>> if (!retval) {
>> /* out of mem */
>> return GL_FALSE;
>> diff --git a/src/mesa/state_tracker/st_cb_fbo.c
>> b/src/mesa/state_tracker/st_cb_fbo.c
>> index 78433bf..dce4239 100644
>> --- a/src/mesa/state_tracker/st_cb_fbo.c
>> +++ b/src/mesa/state_tracker/st_cb_fbo.c
>> @@ -488,7 +488,7 @@ st_render_texture(struct gl_context *ctx,
>> struct st_renderbuffer *strb = st_renderbuffer(rb);
>> struct pipe_resource *pt;
>>
>> - if (!st_finalize_texture(ctx, pipe, att->Texture))
>> + if (!st_finalize_texture(ctx, pipe, att->Texture, att->CubeMapFace))
>> return;
>>
>> pt = st_get_texobj_resource(att->Texture);
>> diff --git a/src/mesa/state_tracker/st_cb_texture.c
>> b/src/mesa/state_tracker/st_cb_texture.c
>> index bc6f108..1b486d7 100644
>> --- a/src/mesa/state_tracker/st_cb_texture.c
>> +++ b/src/mesa/state_tracker/st_cb_texture.c
>> @@ -2434,7 +2434,8 @@ copy_image_data_to_texture(struct st_context *st,
>> GLboolean
>> st_finalize_texture(struct gl_context *ctx,
>> struct pipe_context *pipe,
>> - struct gl_texture_object *tObj)
>> + struct gl_texture_object *tObj,
>> + GLuint cubeMapFace)
>> {
>> struct st_context *st = st_context(ctx);
>> struct st_texture_object *stObj = st_texture_object(tObj);
>> @@ -2478,7 +2479,7 @@ st_finalize_texture(struct gl_context *ctx,
>>
>> }
>>
>> - firstImage =
>> st_texture_image_const(_mesa_base_tex_image(&stObj->base));
>> + firstImage =
>> st_texture_image_const(stObj->base.Image[cubeMapFace][stObj->base.BaseLevel]);
>> assert(firstImage);
>>
>> /* If both firstImage and stObj point to a texture which can contain
>> diff --git a/src/mesa/state_tracker/st_cb_texture.h
>> b/src/mesa/state_tracker/st_cb_texture.h
>> index 415d59f..f647b16 100644
>> --- a/src/mesa/state_tracker/st_cb_texture.h
>> +++ b/src/mesa/state_tracker/st_cb_texture.h
>> @@ -47,7 +47,8 @@ st_get_blit_mask(GLenum srcFormat, GLenum dstFormat);
>> extern GLboolean
>> st_finalize_texture(struct gl_context *ctx,
>> struct pipe_context *pipe,
>> - struct gl_texture_object *tObj);
>> + struct gl_texture_object *tObj,
>> + GLuint cubeMapFace);
>>
>>
>> extern void
>> diff --git a/src/mesa/state_tracker/st_gen_mipmap.c
>> b/src/mesa/state_tracker/st_gen_mipmap.c
>> index 10af11e..16b914a 100644
>> --- a/src/mesa/state_tracker/st_gen_mipmap.c
>> +++ b/src/mesa/state_tracker/st_gen_mipmap.c
>> @@ -125,7 +125,7 @@ st_generate_mipmap(struct gl_context *ctx, GLenum
>> target,
>> *
>> * After this, we'll have all mipmap levels in one resource.
>> */
>> - st_finalize_texture(ctx, st->pipe, texObj);
>> + st_finalize_texture(ctx, st->pipe, texObj, 0);
>> }
>>
>> pt = stObj->pt;
>>
>
>
> --
> Lerne, wie die Welt wirklich ist,
> Aber vergiss niemals, wie sie sein sollte.
>
> _______________________________________________
> 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
