[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 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 think I would prefer NONE to INVALID because INVALID makes it sound
like it represents some sort of error condition.  But yeah that sounds
reasonable to me.

>> 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/20151124/74c4f871/attachment.sig>