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

[Jason Ekstrand]

On Mon, Nov 23, 2015 at 6:15 AM, Francisco Jerez <currojerez at riseup.net> wrote:
> 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.

I wish that had been better documented...  However, with the
experience I have gained hooking up image_load_store in other
places,that makes sense.

> 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.

Yeah, that's a distinction I would like to keep.  Perhaps a
BRW_SURFACEFORMAT_INVALID?  That's what we're doing in libisl right
now.  Does that sound reasonable?

> 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