[Mesa-dev] [PATCH 3/3] r600g: implement fragment and vertex color clamp

[Roland Scheidegger]

Am 27.06.2011 16:04, schrieb Keith Whitwell:
> On Mon, 2011-06-27 at 15:32 +0200, Marek Olšák wrote:
>> On Mon, Jun 27, 2011 at 2:38 PM, Roland Scheidegger
>> <sroland at vmware.com> wrote:
>>> Am 25.06.2011 00:22, schrieb Vadim Girlin:
>>>> On 06/24/2011 11:38 PM, Jerome Glisse wrote:
>>>>> On Fri, Jun 24, 2011 at 12:29 PM, Vadim
>> Girlin<vadimgirlin at gmail.com>
>>>>> wrote:
>>>>>> Fixes https://bugs.freedesktop.org/show_bug.cgi?id=38440
>>>>>>
>>>>>> Signed-off-by: Vadim Girlin<vadimgirlin at gmail.com>
>>>>>
>>>>> As discussed previously, there is better to handle this. I think
>> best
>>>>> solution is to always add the instruction and to conditionally
>> execute
>>>>> them thanks to the boolean constant. If this reveal to have a too
>> big
>>>>> impact on shader, other solution i see is adding a cf block with
>> those
>>>>> instructions and to enable or disable that block (cf_nop) and
>> reupload
>>>>> shader that would avoid a rebuild.
>>>>
>>>> I know its not optimal to do a full rebuild, but rebuild is needed
>> only
>>>> when the application will use the same shader in different clamping
>>>> states. It won't be a problem if the application doesn't change
>> clamping
>>>> state or if it changes the state but uses each shader in one state
>> only.
>>>> So assuming that typical app will not use one shader in both
>> states, it
>>>> shouldn't be a problem. Is this assumption wrong? I'm not really
>> sure
>>>> because I have no much experience in this. But if it's wrong then
>> it's
>>>> probably better for performance to build and cache both versions.
>>> I tend to think you're right apps probably don't want to use the
>> same
>>> shader both with and without clamping.
>>
>> It still can be changed by st/mesa or by u_blitter and u_blit for
>> various reasons. IIRC, the OpenGL default is TRUE if the current
>> framebuffer is fixed-point including texture_snorm and FALSE
>> otherwise, so changing the framebuffer may change the clamp color
>> state. Besides that, the u_blitter and u_blit operations always
>> disable the clamping, so if a framebuffer is fixed-point and thus
>> clamp color state is TRUE (if not changed by an app), the driver may
>> receive state changes that turn the clamping on, off, on, off,... with
>> the blit operations turning it off and everything else turning it on.
>> The state might be changing pretty much all the time and doing full
>> shader rebuilds repeatedly may turn some apps into a slideshow.
> 
> I haven't looked at the code, maybe this is irrelevant for some reason,
> but the alternative to doing rebuilds when this type of state changes is
> to permit >1 compiled version of the shader to exist, parameterized in
> different ways.  That way the ping-pong scenario you describe results in
> swapping between shaders (which should be cheap), rather than
> rebuilding.
> 
>> Therefore we must ensure that a fragment shader is set/built as late
>> as possible, i.e. in draw_vbo. Each shader variant should be compiled
>> once at most and stored for later use. create_fs_state and
>> bind_fs_state should not do anything except copying the parameters. 
> 
> Actually it sounds like you're describing the same idea here...
> 
> Keith

I wasn't aware that the boolean block might be free in terms of
performance. If so that might indeed be a good solution without needing
to recompile. But there might be other cases which could benefit from
cached shaders, so it looks both options have merit.

Roland