[Mesa-dev] [PATCH] st: Add cubeMapFace parameter to st_finalize_texture.
Nicolai Hähnle
nhaehnle at gmail.com
Sat Apr 1 07:05:38 UTC 2017
Pushed.
On 29.03.2017 21:58, Marek Olšák wrote:
> I'm OK with this patch.
>
> Marek
>
> On Wed, Mar 29, 2017 at 12:57 PM, Nicolai Hähnle <nhaehnle at 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-stable at 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.haehnle at 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 at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
--
Lerne, wie die Welt wirklich ist,
Aber vergiss niemals, wie sie sein sollte.
More information about the mesa-dev
mailing list