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 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 _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev