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

[Marek Olšák]

On Fri, Jan 6, 2012 at 4:50 PM, Christoph Bumiller
<e0425955 at student.tuwien.ac.at> wrote:
> On 01/06/2012 04:35 PM, Brian Paul wrote:
>> 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.
>>
>
> But all the radeon and NV GPUs have the shade model switch built-in,
> they don't need 2 shader variants ...

I agree with this point. r300g ignores the TGSI interpolate modes,
because such state doesn't exist in hardware. It's a GL3 feature. All
I can do is to follow pipe_rasterizer_state::flatshade.

Marek