Jason Ekstrand <ja...@jlekstrand.net> writes:
> On Mon, Nov 23, 2015 at 6:15 AM, Francisco Jerez <curroje...@riseup.net>
> wrote:
>> Jason Ekstrand <ja...@jlekstrand.net> writes:
>>
>>> On Fri, Nov 20, 2015 at 5:49 AM, Francisco Jerez <curroje...@riseup.net>
>>> wrote:
>>>> Chad Versace <chad.vers...@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.vers...@intel.com> >>>>> _______________________________________________ >>>>> mesa-dev mailing list >>>>> mesa-dev@lists.freedesktop.org >>>>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
signature.asc
Description: PGP signature
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
