[Mesa-dev] [PATCH 1/7] i965/fs_surface_builder: Explicitly handle FORMAT_NONE in num_image_coordinates

[Francisco Jerez]

Jason Ekstrand <jason at jlekstrand.net> writes:

> On Fri, Nov 20, 2015 at 5:49 AM, Francisco Jerez <currojerez at riseup.net> wrote:
>> Chad Versace <chad.versace at intel.com> writes:
>>
>>> On Wed 04 Nov 2015, Jason Ekstrand wrote:
>>>> Previously, we were relying on has_matching_typed_format returning true for
>>>> MESA_FORMAT_NONE which, in turn, relied on _mesa_get_format_bytes returning
>>>> 1 for MESA_FORMAT_NONE.  All of this is extremely non-obvious.  Instead,
>>>> this commit makes us handle it explicitly.
>>>> ---
>>>>  src/mesa/drivers/dri/i965/brw_fs_surface_builder.cpp | 1 +
>>>>  1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/src/mesa/drivers/dri/i965/brw_fs_surface_builder.cpp b/src/mesa/drivers/dri/i965/brw_fs_surface_builder.cpp
>>>> index 534d849..31ecb5b 100644
>>>> --- a/src/mesa/drivers/dri/i965/brw_fs_surface_builder.cpp
>>>> +++ b/src/mesa/drivers/dri/i965/brw_fs_surface_builder.cpp
>>>> @@ -409,6 +409,7 @@ namespace {
>>>>            * reads want the array index to be at the Z component.
>>>>            */
>>>>           const bool array_index_at_z =
>>>> +            format != MESA_FORMAT_NONE &&
>>>>              !image_format_info::has_matching_typed_format(
>>>>                 bld.shader->devinfo, format);
>>>>           const unsigned zero_dims =
>>>
>>>
>>> Knowing nothing about the implicit assumptions you discovered that
>>> relied on _mesa_get_format_bytes(MESA_FORMAT_NONE) => 1, the patch is
>>> still looks like an improvement to me.
>>>
>> It didn't.  It relied on _mesa_get_format_bytes(MESA_FORMAT_NONE) not
>> being greater than 4, which seems sensible anyway.
>
> I can change the commit message to say
>
> Previously, we were relying on has_matching_typed_format returning true for
> MESA_FORMAT_NONE which, in turn, relied on _mesa_get_format_bytes returning
> a value <= 4 for MESA_FORMAT_NONE.  While reliable, this is extremely
> non-obvious.  Instead,
> this commit makes us handle it explicitly.

has_matching_typed_format(MESA_FORMAT_NONE) returned true by design,
GL_NONE/MESA_FORMAT_NONE represents a shader-unspecified format
throughout the image load/store code, and in Gen hardware that
necessarily implies typed (since otherwise we would need to know the
format for the compiler to be able to generate appropriate format
conversion code, but we don't).  Its semantics are blurred quite a bit
in this series though: All instances of MESA_FORMAT_NONE are replaced
with BRW_SURFACEFORMAT_RAW, which is already used in the image surface
state setup code to represent an *untyped* format (that's what a RAW
surface format means on Gen hardware), i.e. a set of formats fully
disjoint from what MESA_FORMAT_NONE used to represent.

For that reason 'has_matching_typed_format(MESA_FORMAT_NONE) = true'
makes sense to me, but 'has_matching_typed_format(BRW_SURFACEFORMAT_RAW)
= true' and the identification of MESA_FORMAT_NONE with
BRW_SURFACEFORMAT_RAW does not.

I believe this confusion may not have led to any actual bugs though,
because BRW_SURFACEFORMAT_RAW was only used in the state upload code and
was never actually visible to the compiler.  Likewise MESA_FORMAT_NONE
was never visible to the state upload code because an image unit with
invalid format would have been caught by the _mesa_is_image_unit_valid()
check before the translation to native formats.  Seems rather
disquieting still...

>
>>> Acked-by: Chad Versace <chad.versace at intel.com>
>>> _______________________________________________
>>> mesa-dev mailing list
>>> mesa-dev at lists.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 212 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20151123/293c13d9/attachment-0001.sig>