[Mesa-dev] [PATCH] st/mesa: fix default interpolation for colors. (v2)

[Brian Paul]

On 01/06/2012 08:26 AM, Dave Airlie wrote:
> On Fri, Jan 6, 2012 at 3:13 PM, Brian Paul<brianp at vmware.com>  wrote:
>> On 01/06/2012 06:57 AM, Dave Airlie wrote:
>>>
>>> From: Dave Airlie<airlied at redhat.com>
>>>
>>> Brian mentioned that mesa-demos/reflect was broken on softpipe,
>>> by my previous commit. The problem was were blindly translating none
>>> to perspective, when color/pntc at least need it linear.
>>>
>>> v2: no regressions version.
>>> use shademodel to pick what none means.
>>>
>>> Signed-off-by: Dave Airlie<airlied at redhat.com>
>>> ---
>>>   src/mesa/state_tracker/st_program.c |   19 ++++++++++++-------
>>>   1 files changed, 12 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/src/mesa/state_tracker/st_program.c
>>> b/src/mesa/state_tracker/st_program.c
>>> index 146a9f3..1f6094e 100644
>>> --- a/src/mesa/state_tracker/st_program.c
>>> +++ b/src/mesa/state_tracker/st_program.c
>>> @@ -436,10 +436,14 @@ st_get_vp_variant(struct st_context *st,
>>>
>>>
>>>   static unsigned
>>> -st_translate_interp(enum glsl_interp_qualifier glsl_qual)
>>> +st_translate_interp(enum glsl_interp_qualifier glsl_qual, bool is_color,
>>> GLenum shade_model)
>>>   {
>>>      switch (glsl_qual) {
>>>      case INTERP_QUALIFIER_NONE:
>>> +      if (is_color)
>>> +         if (shade_model == GL_FLAT)
>>> +            return TGSI_INTERPOLATE_LINEAR;
>>> +      return TGSI_INTERPOLATE_PERSPECTIVE;
>>
>>
>> This doesn't look right.  If shade_mode == GL_FLAT, shouldn't we return
>> TGSI_INTERPOLATE_CONSTANT?
>
> Yeah the code is very wrong, I was confused by the fact that softpipe
> perspective interp is broken and some piglit results.
>
>>>      case INTERP_QUALIFIER_SMOOTH:
>>>         return TGSI_INTERPOLATE_PERSPECTIVE;
>>>      case INTERP_QUALIFIER_FLAT:
>>> @@ -542,12 +546,14 @@ st_translate_fragment_program(struct st_context *st,
>>>               case FRAG_ATTRIB_COL0:
>>>                  input_semantic_name[slot] = TGSI_SEMANTIC_COLOR;
>>>                  input_semantic_index[slot] = 0;
>>> -               interpMode[slot] =
>>> st_translate_interp(stfp->Base.InterpQualifier[attr]);
>>> +               interpMode[slot] =
>>> st_translate_interp(stfp->Base.InterpQualifier[attr],
>>> +                                                      TRUE,
>>> st->ctx->Light.ShadeModel);
>>>                  break;
>>>               case FRAG_ATTRIB_COL1:
>>>                  input_semantic_name[slot] = TGSI_SEMANTIC_COLOR;
>>>                  input_semantic_index[slot] = 1;
>>> -               interpMode[slot] =
>>> st_translate_interp(stfp->Base.InterpQualifier[attr]);
>>> +               interpMode[slot] =
>>> st_translate_interp(stfp->Base.InterpQualifier[attr],
>>> +                                                      TRUE,
>>> st->ctx->Light.ShadeModel);
>>>                  break;
>>>               case FRAG_ATTRIB_FOGC:
>>>                  input_semantic_name[slot] = TGSI_SEMANTIC_FOG;
>>> @@ -601,10 +607,9 @@ st_translate_fragment_program(struct st_context *st,
>>>                  assert(attr>= FRAG_ATTRIB_TEX0);
>>>                  input_semantic_index[slot] = (attr - FRAG_ATTRIB_TEX0);
>>>                  input_semantic_name[slot] = TGSI_SEMANTIC_GENERIC;
>>> -               if (attr == FRAG_ATTRIB_PNTC)
>>> -                  interpMode[slot] = TGSI_INTERPOLATE_LINEAR;
>>> -               else
>>> -                  interpMode[slot] =
>>> st_translate_interp(stfp->Base.InterpQualifier[attr]);
>>> +               interpMode[slot] =
>>> st_translate_interp(stfp->Base.InterpQualifier[attr],
>>> +                                                      (attr ==
>>> FRAG_ATTRIB_PNTC),
>>> +
>>>   st->ctx->Light.ShadeModel);
>>
>>
>> The ShadeModel value should only apply to color attibutes so it shouldn't
>> appear here in the texcoords/generic/point-coord case.
>>
>> I think the code should read:
>>
>>    if (attr == FRAG_ATTRIB_PNTC)
>>       interpMode[slot] = TGSI_INTERPOLATE_LINEAR;
>>    else
>>       interpMode[slot] =
>> st_translate_interp((stfp->Base.InterpQualifier[attr], false, 0);
>
> Yeah I'll probably just commit v1 + that change.
>
> then I'll try and figure why softpipe gives different answer for
> perspective than everyone else.
>
> Dave.

Looking forward, I think we'll eventually want to remove the 
pipe_rasterizer_state::flatshade field and always use the fragment 
shader interpolation qualifiers.

This would mean that if a shader was used both with 
glShadeModel(GL_FLAT) and GL_SMOOTH we'd wind up with two variants of 
the shader, but that should be rare.

-Brian