Thanks for testing. I fixed the whitespace, and resent patch 4 due to a bug.
Roland
Am 03.01.2018 um 08:29 schrieb Konstantin Kharlamov:
> Sorry, I don't have an expertise to give a r-b, but here's my t-b :) I
> found no statistically significant changes at "the big keybench" of
> `vblank_mode=0 ./xonotic-linux64-glx`.
>
> But note, there's a trailing whitespace at patch 5 (first "+" after "@@
> -1267,6 +1268,20 @@"), and patch 6 (first "+" after "@@ -1723,6 +1723,21
> @@").
>
> Tested-by: Konstantin Kharlamov <hi-an...@yandex.ru>
>
> On 03.01.2018 05:25, srol...@vmware.com wrote:
>> From: Roland Scheidegger <srol...@vmware.com>
>>
>> Similar to const buffers. The driver must not emit any tes-related
>> state if tes
>> is disabled, since the hw slots are all shared by VS, therefore it would
>> overwrite them (the mesa state tracker might not do this, but it would be
>> perfectly legal to do so).
>> Nevertheless I think the dirty state tracking logic in the driver is
>> fundamentally flawed when tes is disabled/enabled, since it looks to
>> me like
>> the VS (and TES) state would not get reemitted to the correct slots
>> (if it's
>> not dirty anyway). Unless I'm missing something...
>> Theoretically, the overwrite problem could be solved by using
>> non-overlapping
>> resource slots for TES and VS (since we're not even close to using
>> half the
>> resource slots), but it wouldn't work for constant buffers nor
>> samplers, and
>> for VS would still need to propagate changes to both LS and VS, so
>> probably
>> not a useful idea.
>> Unfortunately there's zero coverage of this with piglit, since all
>> tessellation
>> shader tests are just shader_runner tests, which are unsuitable for
>> testing
>> any kind of state dependency tracking issues (so I can't even quickly
>> hack
>> something up to proove it and fix it...).
>> TCS otoh is just fine - like GS it has its own hw slots.
>> ---
>> src/gallium/drivers/r600/evergreen_state.c | 4 ++++
>> src/gallium/drivers/r600/r600_state_common.c | 15 +++++++++++++++
>> 2 files changed, 19 insertions(+)
>>
>> diff --git a/src/gallium/drivers/r600/evergreen_state.c
>> b/src/gallium/drivers/r600/evergreen_state.c
>> index 4cc48dfa11..fb1de9cbf4 100644
>> --- a/src/gallium/drivers/r600/evergreen_state.c
>> +++ b/src/gallium/drivers/r600/evergreen_state.c
>> @@ -2334,6 +2334,8 @@ static void
>> evergreen_emit_tcs_sampler_views(struct r600_context *rctx, struct r
>> static void evergreen_emit_tes_sampler_views(struct r600_context
>> *rctx, struct r600_atom *atom)
>> {
>> + if (!rctx->tes_shader)
>> + return;
>> evergreen_emit_sampler_views(rctx,
>> &rctx->samplers[PIPE_SHADER_TESS_EVAL].views,
>> EG_FETCH_CONSTANTS_OFFSET_VS +
>> R600_MAX_CONST_BUFFERS, 0);
>> }
>> @@ -2404,6 +2406,8 @@ static void
>> evergreen_emit_tcs_sampler_states(struct r600_context *rctx, struct
>> static void evergreen_emit_tes_sampler_states(struct r600_context
>> *rctx, struct r600_atom *atom)
>> {
>> + if (!rctx->tes_shader)
>> + return;
>> evergreen_emit_sampler_states(rctx,
>> &rctx->samplers[PIPE_SHADER_TESS_EVAL], 18,
>> R_00A414_TD_VS_SAMPLER0_BORDER_INDEX, 0);
>> }
>> diff --git a/src/gallium/drivers/r600/r600_state_common.c
>> b/src/gallium/drivers/r600/r600_state_common.c
>> index 4364350487..a434156c16 100644
>> --- a/src/gallium/drivers/r600/r600_state_common.c
>> +++ b/src/gallium/drivers/r600/r600_state_common.c
>> @@ -1723,6 +1723,21 @@ static bool r600_update_derived_state(struct
>> r600_context *rctx)
>> UPDATE_SHADER_CLIP(R600_HW_STAGE_VS, vs);
>> }
>> }
>> +
>> + /*
>> + * XXX: I believe there's some fatal flaw in the dirty state
>> logic when
>> + * enabling/disabling tes.
>> + * VS/ES share all buffer/resource/sampler slots. If TES is enabled,
>> + * it will therefore overwrite the VS slots. If it now gets
>> disabled,
>> + * the VS needs to rebind all buffer/resource/sampler slots - not
>> only
>> + * has TES overwritten the corresponding slots, but when the VS was
>> + * operating as LS the things with correpsonding dirty bits got
>> bound
>> + * to LS slots and won't reflect what is dirty as VS stage even
>> if the
>> + * TES didn't overwrite it. The story for re-enabled TES is similar.
>> + * In any case, we're not allowed to submit any TES state when
>> + * TES is disabled (the state tracker may not do this but this looks
>> + * like an optimization to me, not something which can be relied
>> on).
>> + */
>> /* Update clip misc state. */
>> if (clip_so_current) {
>>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
