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

Tue Apr 12 18:22:50 UTC 2016

Am 12.04.2016 um 18:45 schrieb 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.
> 

Oh fwiw the change looks good, so
Reviewed-by: Roland Scheidegger <sroland at vmware.com>