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