[Mesa-dev] [PATCH] mesa: fix texture updates for ATI_fragment_shader

[Miklós Máté]

On 16/10/17 01:59, Timothy Arceri wrote:
> On 16/10/17 04:10, Miklós Máté wrote:
>> Hi,
>>
>> I'd like to ask you to revert this change.
>>
>> As Ian Romanick pointed out this makes ATI_fs behave like ARB_fp, 
>> however there is a major difference between the two: with ATI_fs 
>> there is no way of knowing the texture targets until the draw call. 
>> When an ATI_fs is created, st_init_atifs_prog() sets every texture 
>> target to TEXTURE_2D_BIT, and st_fixup_atifs() sets the correct one, 
>> but unfortunately _mesa_update_texture_state() is called between 
>> them. After this patch update_program_texture_state() validates the 
>> texture targets to match the bound texture units, and thus rejects 
>> sampling from cube maps. This results in broken rendering in Knights 
>> of the Old Republic.
>
> Before reverting anything are you able to create a piglit test that 
> reproduces the issue, so that this doesn't get broken again in future?
I've been meaning to create piglit tests for ATI_fs ever since I 
implemented it for gallium, I just couldn't find the time to do so. This 
regression a great incentive to start working on it.

MM

>
> Ideally we would have one for the crash Marek is seeing also.
>
>
>>
>> MM
>>
>> On 27/09/17 17:39, Marek Olšák wrote:
>>> From: Marek Olšák <marek.olsak at amd.com>
>>>
>>> ---
>>>   src/mesa/main/texstate.c | 8 +++++---
>>>   1 file changed, 5 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/src/mesa/main/texstate.c b/src/mesa/main/texstate.c
>>> index 269e291..edd2253 100644
>>> --- a/src/mesa/main/texstate.c
>>> +++ b/src/mesa/main/texstate.c
>>> @@ -833,23 +833,25 @@ update_ff_texture_state(struct gl_context *ctx,
>>>   void
>>>   _mesa_update_texture_state(struct gl_context *ctx)
>>>   {
>>>      struct gl_program *prog[MESA_SHADER_STAGES];
>>>      int i;
>>>      int old_max_unit = ctx->Texture._MaxEnabledTexImageUnit;
>>>      BITSET_DECLARE(enabled_texture_units, 
>>> MAX_COMBINED_TEXTURE_IMAGE_UNITS);
>>>      memcpy(prog, ctx->_Shader->CurrentProgram, sizeof(prog));
>>> -   if (prog[MESA_SHADER_FRAGMENT] == NULL &&
>>> -       _mesa_arb_fragment_program_enabled(ctx)) {
>>> -      prog[MESA_SHADER_FRAGMENT] = ctx->FragmentProgram.Current;
>>> +   if (prog[MESA_SHADER_FRAGMENT] == NULL) {
>>> +      if (_mesa_arb_fragment_program_enabled(ctx))
>>> +         prog[MESA_SHADER_FRAGMENT] = ctx->FragmentProgram.Current;
>>> +      else if (_mesa_ati_fragment_shader_enabled(ctx))
>>> +         prog[MESA_SHADER_FRAGMENT] = 
>>> ctx->ATIFragmentShader.Current->Program;
>>>      }
>>>      /* TODO: only set this if there are actual changes */
>>>      ctx->NewState |= _NEW_TEXTURE_OBJECT | _NEW_TEXTURE_STATE;
>>>      ctx->Texture._GenFlags = 0x0;
>>>      ctx->Texture._TexMatEnabled = 0x0;
>>>      ctx->Texture._TexGenEnabled = 0x0;
>>>      ctx->Texture._MaxEnabledTexImageUnit = -1;
>>>      ctx->Texture._EnabledCoordUnits = 0x0;
>>
>>
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev