[Mesa-dev] [PATCH v2 2/3] gallium: Add capability for ARB_robust_buffer_access_behavior

[Roland Scheidegger]

Am 12.04.2016 um 18:26 schrieb Bas Nieuwenhuizen:
> On Tue, Apr 12, 2016 at 6:09 PM, Roland Scheidegger <sroland at vmware.com> wrote:
>> Am 12.04.2016 um 16:23 schrieb Bas Nieuwenhuizen:
>>> On Tue, Apr 12, 2016 at 3:56 PM, Roland Scheidegger <sroland at vmware.com> wrote:
>>>> Am 12.04.2016 um 15:12 schrieb Bas Nieuwenhuizen:
>>>>> Signed-off-by: Bas Nieuwenhuizen <bas at basnieuwenhuizen.nl>
>>>>> ---
>>>>>  src/gallium/docs/source/screen.rst               | 5 +++++
>>>>>  src/gallium/drivers/freedreno/freedreno_screen.c | 1 +
>>>>>  src/gallium/drivers/i915/i915_screen.c           | 1 +
>>>>>  src/gallium/drivers/ilo/ilo_screen.c             | 1 +
>>>>>  src/gallium/drivers/llvmpipe/lp_screen.c         | 1 +
>>>>>  src/gallium/drivers/nouveau/nv30/nv30_screen.c   | 1 +
>>>>>  src/gallium/drivers/nouveau/nv50/nv50_screen.c   | 1 +
>>>>>  src/gallium/drivers/nouveau/nvc0/nvc0_screen.c   | 1 +
>>>>>  src/gallium/drivers/r300/r300_screen.c           | 1 +
>>>>>  src/gallium/drivers/r600/r600_pipe.c             | 1 +
>>>>>  src/gallium/drivers/radeonsi/si_pipe.c           | 1 +
>>>>>  src/gallium/drivers/softpipe/sp_screen.c         | 1 +
>>>>>  src/gallium/drivers/svga/svga_screen.c           | 1 +
>>>>>  src/gallium/drivers/swr/swr_screen.cpp           | 1 +
>>>>>  src/gallium/drivers/vc4/vc4_screen.c             | 1 +
>>>>>  src/gallium/drivers/virgl/virgl_screen.c         | 1 +
>>>>>  src/gallium/include/pipe/p_defines.h             | 1 +
>>>>>  src/mesa/state_tracker/st_extensions.c           | 1 +
>>>>>  18 files changed, 22 insertions(+)
>>>>>
>>>>> diff --git a/src/gallium/docs/source/screen.rst b/src/gallium/docs/source/screen.rst
>>>>> index 824f580..b281029 100644
>>>>> --- a/src/gallium/docs/source/screen.rst
>>>>> +++ b/src/gallium/docs/source/screen.rst
>>>>> @@ -331,6 +331,11 @@ The integer capabilities:
>>>>>    primitive on a layer is obtained from ``PIPE_CAP_MAX_TEXTURE_ARRAY_LAYERS``
>>>>>    even though it can be larger than the number of layers supported by either
>>>>>    rendering or textures.
>>>>> +* ``PIPE_CAP_ROBUST_BUFFER_ACCESS``: Implementation uses bounds checking on
>>>>> +  resource accesses by shader if the context is created with
>>>>> +  PIPE_CONTEXT_ROBUST_BUFFER_ACCESS. See the ARB_robust_buffer_access_behavior
>>>>> +  extension for information on the required behavior for out of bounds accesses
>>>>> +  and accesses to unbound resources.
>>>> I think this is a bit of a misnomer. "Robust buffer access" is already
>>>> provided by just ARB_robustness in gl terms (meaning it just isn't
>>>> allowed to crash for out-of-bounds access generally).
>>>> Although admitedly PIPE_CAP_ROBUST_BUFFER_ACCESS_BEHAVIOR would be quite
>>>> a long name...
>>>>
>>>
>>> There is already a PIPE_CONTEXT_ROBUST_BUFFER_ACCESS flag in gallium
>>> with the comment
>>>
>>> /**
>>>  * Whether out-of-bounds shader loads must return zero and out-of-bounds
>>>  * shader stores must be dropped.
>>>  */
>>>
>>> Which makes me suspect it is really targeted at
>>> ARB_robust_buffer_access_behavior, not ARB_robustness. That said,
>>> either name would be fine with me.
>>
>> You are right, but that flag is really getting passed through context
>> creation (that is, cannot really be targeted at the _behavior
>> extension). So the comment might not be quite right.
>> IIRC it really works like this:
> 
> ok, I'll rename the cap to PIPE_CAP_ROBUST_BUFFER_ACCESS_BEHAVIOR.
> 
>> no ARB_robustness, or context not created with robust flag:
>> implementation is allowed to crash (I believe though all drivers
>> actually won't crash but return undefined values already anyway)
>>
>> ARB_robustness, robust flag, no ARB_rbab extension:
>> implementation may not crash, undefined values
>>
>> ARB_robustness, robust flag, ARB_rbab extension:
>> sort of defined behavior (still not quite as strict as d3d10 IIRC)
>>
>> I thought there wouldn't be a cap bit for ARB_robustness, but that's not
>> quite true. If I see that right drivers need to support
>> PIPE_CAP_DEVICE_RESET_STATUS_QUERY otherwise ARB_robustness won't be
>> exposed (and neither should ARB_rbab be in this case as this is a
>> requirement).
>>
> 
> ARB_robustness is always exposed by mesa: there is no extension enable bit.

Yes, my bad. I was really talking about the corresponding glx (or egl)
bit, GLX_ARB_create_context_robustness. ARB_robustness will only provide
some new entry functions on its own (that is, guard against some cpu
memory access overflow), but otherwise not really do anything against
gpu buffer out-of-bound access, if the context wasn't created with the
robust bit from the related create_context_robustness extensions. And
this is what's not exposed otherwise.

So, it might be ok to expose ARB_rbab without
PIPE_CAP_DEVICE_RESET_STATUS_QUERY (so, not exposing robust contexts).
Clearly the spec doesn't list anything else as a requirement. But it
will be completely meaningless, as the additional restrictions only
apply to contexts created with the robustness flag, which you can't
create if those glx/egl extensions aren't supported.

Roland