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

[Christoph Bumiller]

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 ...

> -Brian
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev