[Mesa-dev] [PATCH] radeonsi: pass alpha_ref value to PS in the user sgpr

[Christian König]

Am 11.10.2013 01:15, schrieb Marek Olšák:
> On Thu, Oct 10, 2013 at 6:38 PM, Vadim Girlin <vadimgirlin at gmail.com> wrote:
>> On 10/10/2013 08:10 PM, Christian König wrote:
>>> Am 10.10.2013 18:02, schrieb Vadim Girlin:
>>>> On 10/10/2013 02:11 PM, Michel Dänzer wrote:
>>>>> On Don, 2013-10-10 at 12:49 +0400, Vadim Girlin wrote:
>>>>>> Currently it's hardcoded in the shader, so every change requires
>>>>>> compilation of the shader variant, killing the performance
>>>>>> in Serious Sam 3 and probably other apps.
>>>>>>
>>>>>> This patch passes alpha_ref in the user sgpr and removes it from
>>>>>> the shader key.
>>>>>>
>>>>>> Signed-off-by: Vadim Girlin <vadimgirlin at gmail.com>
>>>>>
>>>>> Reviewed-by: Michel Dänzer <michel.daenzer at amd.com>
>>>>>
>>>>> I presume this causes no regressions with piglit quick.tests.
>>>>
>>>> Yes, there are no regressions with piglit. Thanks for reviewing.
>>>>
>>>> By the way, I'm also not sure if this is the right way of doing it,
>>>> especially if we'll need to pass more parameters for any new features.
>>>>
>>>> Possibly some other ways could be more preferable, e.g. to put it with
>>>> any other data that we may need in the future into internal const
>>>> buffer (like we do in r600g for clip planes etc), or maybe there are
>>>> other ways on SI that I'm not aware of yet?
>>>
>>> That strongly depends on how often we use a parameter. The docs speak of
>>> a penalty associated with loading each SGPR so we should try to use as
>>> less as possible, but loading something from constant space is also
>>> costly without proper support for the constant IB.
>>
>> By the way, AFAICS some SGPR inputs are often not used at all in the
>> shaders, I guess we might want to use per-shader mapping of parameters to
>> SGPRs so that we'll only load actually used values for each shader. Compiler
>> will need to pack required input SGPRs to lowest indices and provide the
>> parameter->SGPR map to the driver. OTOH this would slightly increase the
>> amount of driver's work, so I'm not really sure yet if it's worth it, looks
>> like we already have a pretty significant overhead.
> I think this an interesting optimization. Most of our overhead is
> elsewhere anyway. The only overhead added here would only be in the
> bind_xx_shader function, because all the shader user-data registers
> would have to be updated. Note that some of the input SGPRs are
> already declared conditionally, e.g. the SGPRs containing streamout
> state.

Indeed, but the having the streamout optionally currently only works 
because it is the last entry in the list.

I think the first step should be to have only one pointer for samplers 
and resources or maybe even for samplers, resources and constants. We 
should take care that the offset still fits into the SMRD instruction, 
but apart from that I don't see why we need more than one pointer for them.

Basically we just need to allocate SGPRs for everything the shaders use 
(samplers, resources, constants, vertex buffers, stream out buffers 
etc...) and fill each SGPR just before issuing the draw call. A bitmask 
of whats used and a single command for filling all SGPRs in order should 
be pretty much sufficient.

Christian.