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

[Dave Airlie]

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.