[Mesa-dev] [PATCH] r600g: fix color exports when we have no CBs
Roland Scheidegger
sroland at vmware.com
Tue Aug 27 15:28:59 PDT 2013
Am 27.08.2013 23:52, schrieb Vadim Girlin:
> On 08/28/2013 12:43 AM, Marek Olšák wrote:
>> Shader variants are BAD, BAD, BAD. Have you ever played an AAA game
>> with a Mesa driver that likes to compile shader variants on first use?
>> It's HORRIBLE.
>
> I don't think that shader variants are bad, but it's definitely bad when
> we are compiling variants that are never used. Currently glxgears
> compiles 18 ps/vs shaders. In my branch with initial GS support [1] I
> switched handling of the shaders to deferred compilation, that is,
> shaders are compiled only before the actual draw. I found later that
> it's not really required for GS, but IIRC this change results in only 5
> shaders being compiled for glxgears instead of 18. It seems most of the
> useless variants are results of state changes between creation of the
> shader state (initial compilation) and actual draw call.
>
> I had some concerns about increased overhead with those changes, and
> it's actually noticeable with drawoverhead demo, but I didn't see any
> regressions with a few real apps that I tested, e.g. glxgears even
> showed slightly better performance with these changes. Probably I also
> implemented it in a not very optimal way (I was mostly concentrated on
> GS support) and the overhead can be reduced.
>
> One more thing is duplicate shaders, I've analyzed shader dumps from
> Unigine Heaven 3.0 some time ago and found that from about 320 compiled
> shaders, only about 180 (50%) were unique, others were duplicates
> (detected by comparing the bytecode dumps for them in an automated way),
> maybe they had different shader keys (which still resulted in the same
> bytecode), but I suspect duplicate pipe shaders were also involved.
> Unfortunately I didn't have a time to investigate it more thoroughly
> since then.
>
> So my point is that we don't really need to eliminate shader variants,
> first we need to eliminate compilation of unused variants and duplicate
> shaders. Also we might want to consider offloading of the compilation to
> separate thread(s) and caching of shader binaries between runs.
Hmm ok that seems a way more complicated problem than I thought :-).
Compile early and you might compile variants you will never use, compile
late and the delay might be noticeable.
I just thought it might be unlikely you'd actually need two variants -
e.g. some depth exporting shader is probably unlikely to use alpha test.
But ok I guess it shouldn't write color in this case, so even then it
might never be worth bothering. Was just a random idea ;-).
Roland
>
> Vadim
>
> [1] http://cgit.freedesktop.org/~vadimg/mesa/log/?h=r600-geom-shaders
>
>>
>> What the patch does is probably the right solution. At least
>> alpha-test state changes don't cause shader recompilation and
>> re-binding, which also negatively affects performance. Ideally we
>> shouldn't depend on the framebuffer state at all, but we need to
>> emulate the TGSI property FS_COLOR0_WRITES_ALL_CBUFS. I think we
>> should always be fine with key.nr_cbufs forced to 8 for any shader
>> without that property. I expect app developers to do the right thing
>> and not write outputs they don't need.
>>
>> Marek
>>
>> On Tue, Aug 27, 2013 at 9:00 PM, Roland Scheidegger
>> <sroland at vmware.com> wrote:
>>> Not that I'm qualified to review r600 code, but couldn't you create
>>> different shader variants depending on whether you need alpha test? At
>>> least I would assume shader exports aren't free.
>>>
>>> Roland
>>>
>>> Am 27.08.2013 19:56, schrieb Vadim Girlin:
>>>> We need to export at least one color if the shader writes it,
>>>> even when nr_cbufs==0.
>>>>
>>>> Signed-off-by: Vadim Girlin <vadimgirlin at gmail.com>
>>>> ---
>>>>
>>>> Tested on evergreen with multiple combinations of backends - no
>>>> regressions,
>>>> fixes some tests:
>>>>
>>>> default - fixes fb-alphatest-nocolor and fb_alphatest-nocolor-ff
>>>> default+sb - fixes fb-alphatest-nocolor and fb_alphatest-nocolor-ff
>>>> llvm - fixes about 25 tests related to depth/stencil
>>>> llvm+sb - fixes about 300 tests (llvm's depth/stencil issues and
>>>> regressions cased by reordering of exports in sb)
>>>>
>>>> With this patch, there are no regressions with default+sb vs default.
>>>> There is one regression with llvm+sb vs llvm -
>>>> fs-texturegrad-miplevels,
>>>> AFAICS it's a problem with llvm backend uncovered by sb -
>>>> SET_GRADIENTS_V/H
>>>> instructions are not placed in the same TEX clause with
>>>> corresponding SAMPLE_G.
>>>>
>>>> src/gallium/drivers/r600/r600_shader.c | 7 ++++---
>>>> 1 file changed, 4 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/src/gallium/drivers/r600/r600_shader.c
>>>> b/src/gallium/drivers/r600/r600_shader.c
>>>> index 300b5c4..f7eab76 100644
>>>> --- a/src/gallium/drivers/r600/r600_shader.c
>>>> +++ b/src/gallium/drivers/r600/r600_shader.c
>>>> @@ -918,6 +918,7 @@ static int r600_shader_from_tgsi(struct
>>>> r600_screen *rscreen,
>>>> unsigned opcode;
>>>> int i, j, k, r = 0;
>>>> int next_pos_base = 60, next_param_base = 0;
>>>> + int max_color_exports = MAX2(key.nr_cbufs, 1);
>>>> /* Declarations used by llvm code */
>>>> bool use_llvm = false;
>>>> bool indirect_gprs;
>>>> @@ -1130,7 +1131,7 @@ static int r600_shader_from_tgsi(struct
>>>> r600_screen *rscreen,
>>>> radeon_llvm_ctx.face_gpr = ctx.face_gpr;
>>>> radeon_llvm_ctx.r600_inputs = ctx.shader->input;
>>>> radeon_llvm_ctx.r600_outputs = ctx.shader->output;
>>>> - radeon_llvm_ctx.color_buffer_count = MAX2(key.nr_cbufs
>>>> , 1);
>>>> + radeon_llvm_ctx.color_buffer_count = max_color_exports;
>>>> radeon_llvm_ctx.chip_class = ctx.bc->chip_class;
>>>> radeon_llvm_ctx.fs_color_all = shader->fs_write_all
>>>> && (rscreen->chip_class >= EVERGREEN);
>>>> radeon_llvm_ctx.stream_outputs = &so;
>>>> @@ -1440,7 +1441,7 @@ static int r600_shader_from_tgsi(struct
>>>> r600_screen *rscreen,
>>>> case TGSI_PROCESSOR_FRAGMENT:
>>>> if (shader->output[i].name ==
>>>> TGSI_SEMANTIC_COLOR) {
>>>> /* never export more colors than the
>>>> number of CBs */
>>>> - if (shader->output[i].sid >=
>>>> key.nr_cbufs) {
>>>> + if (shader->output[i].sid >=
>>>> max_color_exports) {
>>>> /* skip export */
>>>> j--;
>>>> continue;
>>>> @@ -1450,7 +1451,7 @@ static int r600_shader_from_tgsi(struct
>>>> r600_screen *rscreen,
>>>> output[j].type =
>>>> V_SQ_CF_ALLOC_EXPORT_WORD0_SQ_EXPORT_PIXEL;
>>>> shader->nr_ps_color_exports++;
>>>> if (shader->fs_write_all &&
>>>> (rscreen->chip_class >= EVERGREEN)) {
>>>> - for (k = 1; k < key.nr_cbufs;
>>>> k++) {
>>>> + for (k = 1; k <
>>>> max_color_exports; k++) {
>>>> j++;
>>>> memset(&output[j], 0,
>>>> sizeof(struct r600_bytecode_output));
>>>> output[j].gpr =
>>>> shader->output[i].gpr;
>>>>
>>> _______________________________________________
>>> mesa-dev mailing list
>>> mesa-dev at lists.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>>
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
More information about the mesa-dev
mailing list