On Thu, Jul 23, 2015 at 4:38 AM, Francisco Jerez <curroje...@riseup.net> wrote:
> Jason Ekstrand <ja...@jlekstrand.net> writes:
>
>> This all looks correct as far as I can tell. However, I'm very
>> concerned about the number of checks such as
>> has_matching_typed_format() that are built-in to the compiler (via
>> surface_builder) where we then go on to do something that is highly
>> dependant on state setup doing the exact same check (not via
>> surface_builder) and doing the right thing. Another example would be
>> the interplay with has_split_bit_layout. How do we know these won't
>> get out of sync?
>>
> They cannot get out of sync because the policy which surface format to
> use for a given GLSL format is shared between the compiler and the
> state-setup code, see brw_lower_mesa_image_format() in "i965: Implement
> surface state set-up for shader images.".
I came back to this today and looked at it again. I think my main
objection is that all of these metadata functions work in terms of a
devinfo and a format and then immediately call
brw_lower_mesa_image_format. Why don't they work instead in terms
comparing the lowered format to the nominal format? Then it would be
a lot more obvious that you're doing a transformation from one format
to another. As is, it's a bunch of magic "what do I have to do next"
queries which, while in the same namespace, are in a different file.
It would also be more efficient because you would only call
brw_lower_mesa_image_format once.
Does that make more sense?
--Jason
> Thanks.
>
>> On Tue, Jul 21, 2015 at 9:38 AM, Francisco Jerez <curroje...@riseup.net>
>> wrote:
>>> v2: Drop VEC4 suport.
>>> v3: Rebase.
>>> ---
>>> .../drivers/dri/i965/brw_fs_surface_builder.cpp | 216
>>> +++++++++++++++++++++
>>> src/mesa/drivers/dri/i965/brw_fs_surface_builder.h | 17 ++
>>> 2 files changed, 233 insertions(+)
>>>
>>> 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 ea1c4aa..46b449f 100644
>>> --- a/src/mesa/drivers/dri/i965/brw_fs_surface_builder.cpp
>>> +++ b/src/mesa/drivers/dri/i965/brw_fs_surface_builder.cpp
>>> @@ -587,3 +587,219 @@ namespace {
>>> }
>>> }
>>> }
>>> +
>>> +namespace brw {
>>> + namespace image_access {
>>> + /**
>>> + * Load a vector from a surface of the given format and
>>> dimensionality
>>> + * at the given coordinates.
>>> + */
>>> + fs_reg
>>> + emit_image_load(const fs_builder &bld,
>>> + const fs_reg &image, const fs_reg &addr,
>>> + mesa_format format, unsigned dims)
>>> + {
>>> + using namespace image_format_info;
>>> + using namespace image_format_conversion;
>>> + using namespace image_validity;
>>> + using namespace image_coordinates;
>>> + using namespace surface_access;
>>> + const brw_device_info *devinfo = bld.shader->devinfo;
>>> + const mesa_format lower_format =
>>> + brw_lower_mesa_image_format(devinfo, format);
>>> + fs_reg tmp;
>>> +
>>> + if (has_matching_typed_format(devinfo, format)) {
>>> + /* Hopefully we get here most of the time... */
>>> + tmp = emit_typed_read(bld, image, addr, dims,
>>> +
>>> _mesa_format_num_components(lower_format));
>>> + } else {
>>> + /* Untyped surface reads return 32 bits of the surface per
>>> + * component, without any sort of unpacking or type conversion,
>>> + */
>>> + const unsigned size = _mesa_get_format_bytes(format) / 4;
>>> +
>>> + /* they don't properly handle out of bounds access, so we have
>>> to
>>> + * check manually if the coordinates are valid and predicate
>>> the
>>> + * surface read on the result,
>>> + */
>>> + const brw_predicate pred =
>>> + emit_bounds_check(bld, image, addr, dims);
>>> +
>>> + /* and they don't know about surface coordinates, we need to
>>> + * convert them to a raw memory offset.
>>> + */
>>> + const fs_reg laddr = emit_address_calculation(bld, image,
>>> addr, dims);
>>> +
>>> + tmp = emit_untyped_read(bld, image, laddr, 1, size, pred);
>>> +
>>> + /* An out of bounds surface access should give zero as result.
>>> */
>>> + for (unsigned c = 0; c < 4; ++c)
>>> + set_predicate(pred, bld.SEL(offset(tmp, bld, c),
>>> + offset(tmp, bld, c),
>>> fs_reg(0)));
>>> + }
>>> +
>>> + /* Set the register type to D instead of UD if the data type is
>>> + * represented as a signed integer in memory so that sign
>>> extension
>>> + * is handled correctly by unpack.
>>> + */
>>> + if (needs_sign_extension(format))
>>> + tmp = retype(tmp, BRW_REGISTER_TYPE_D);
>>> +
>>> + if (!has_supported_bit_layout(devinfo, format)) {
>>> + /* Unpack individual vector components from the bitfield if the
>>> + * hardware is unable to do it for us.
>>> + */
>>> + if (has_split_bit_layout(devinfo, format))
>>> + tmp = emit_pack(bld, tmp, get_bit_shifts(lower_format),
>>> + get_bit_widths(lower_format));
>>> + else
>>> + tmp = emit_unpack(bld, tmp, get_bit_shifts(format),
>>> + get_bit_widths(format));
>>> +
>>> + } else if ((needs_sign_extension(format) &&
>>> + !is_conversion_trivial(devinfo, format)) ||
>>> + has_undefined_high_bits(devinfo, format)) {
>>> + /* Perform a trivial unpack even though the bit layout matches
>>> in
>>> + * order to get the most significant bits of each component
>>> + * initialized properly.
>>> + */
>>> + tmp = emit_unpack(bld, tmp, color_u(0, 32, 64, 96),
>>> + get_bit_widths(format));
>>> + }
>>> +
>>> + if (!_mesa_is_format_integer(format)) {
>>> + if (is_conversion_trivial(devinfo, format)) {
>>> + /* Just need to cast the vector to the target type. */
>>> + tmp = retype(tmp, BRW_REGISTER_TYPE_F);
>>> + } else {
>>> + /* Do the right sort of type conversion to float. */
>>> + if (_mesa_get_format_datatype(format) == GL_FLOAT)
>>> + tmp = emit_convert_from_float(
>>> + bld, tmp, get_bit_widths(format));
>>> + else
>>> + tmp = emit_convert_from_scaled(
>>> + bld, tmp, get_bit_widths(format),
>>> + _mesa_is_format_signed(format));
>>> + }
>>> + }
>>> +
>>> + /* Initialize missing components of the result. */
>>> + return emit_pad(bld, tmp, get_bit_widths(format));
>>> + }
>>> +
>>> + /**
>>> + * Store a vector in a surface of the given format and
>>> dimensionality at
>>> + * the given coordinates.
>>> + */
>>> + void
>>> + emit_image_store(const fs_builder &bld, const fs_reg &image,
>>> + const fs_reg &addr, const fs_reg &src,
>>> + mesa_format format, unsigned dims)
>>> + {
>>> + using namespace image_format_info;
>>> + using namespace image_format_conversion;
>>> + using namespace image_validity;
>>> + using namespace image_coordinates;
>>> + using namespace surface_access;
>>> + const brw_device_info *devinfo = bld.shader->devinfo;
>>> +
>>> + if (format == MESA_FORMAT_NONE) {
>>> + /* We don't know what the format is, but that's fine because it
>>> + * implies write-only access, and typed surface writes are
>>> always
>>> + * able to take care of type conversion and packing for us.
>>> + */
>>> + emit_typed_write(bld, image, addr, src, dims, 4);
>>> +
>>> + } else {
>>> + const mesa_format lower_format =
>>> + brw_lower_mesa_image_format(devinfo, format);
>>> + fs_reg tmp = src;
>>> +
>>> + if (!is_conversion_trivial(devinfo, format)) {
>>> + /* Do the right sort of type conversion. */
>>> + if (_mesa_get_format_datatype(format) == GL_FLOAT)
>>> + tmp = emit_convert_to_float(bld, tmp,
>>> get_bit_widths(format));
>>> +
>>> + else if (_mesa_is_format_integer(format))
>>> + tmp = emit_convert_to_integer(bld, tmp,
>>> get_bit_widths(format),
>>> +
>>> _mesa_is_format_signed(format));
>>> +
>>> + else
>>> + tmp = emit_convert_to_scaled(bld, tmp,
>>> get_bit_widths(format),
>>> +
>>> _mesa_is_format_signed(format));
>>> + }
>>> +
>>> + /* We're down to bit manipulation at this point. */
>>> + tmp = retype(tmp, BRW_REGISTER_TYPE_UD);
>>> +
>>> + if (!has_supported_bit_layout(devinfo, format)) {
>>> + /* Pack the vector components into a bitfield if the
>>> hardware
>>> + * is unable to do it for us.
>>> + */
>>> + if (has_split_bit_layout(devinfo, format))
>>> + tmp = emit_unpack(bld, tmp, get_bit_shifts(lower_format),
>>> + get_bit_widths(lower_format));
>>> +
>>> + else
>>> + tmp = emit_pack(bld, tmp, get_bit_shifts(format),
>>> + get_bit_widths(format));
>>> + }
>>> +
>>> + if (has_matching_typed_format(devinfo, format)) {
>>> + /* Hopefully we get here most of the time... */
>>> + emit_typed_write(bld, image, addr, tmp, dims,
>>> + _mesa_format_num_components(lower_format));
>>> +
>>> + } else {
>>> + /* Untyped surface writes store 32 bits of the surface per
>>> + * component, without any sort of packing or type
>>> conversion,
>>> + */
>>> + const unsigned size = _mesa_get_format_bytes(format) / 4;
>>> +
>>> + /* they don't properly handle out of bounds access, so we
>>> have
>>> + * to check manually if the coordinates are valid and
>>> predicate
>>> + * the surface write on the result,
>>> + */
>>> + const brw_predicate pred =
>>> + emit_bounds_check(bld, image, addr, dims);
>>> +
>>> + /* and, phew, they don't know about surface coordinates, we
>>> + * need to convert them to a raw memory offset.
>>> + */
>>> + const fs_reg laddr = emit_address_calculation(bld, image,
>>> addr, dims);
>>> +
>>> + emit_untyped_write(bld, image, laddr, tmp, 1, size, pred);
>>> + }
>>> + }
>>> + }
>>> +
>>> + /**
>>> + * Perform an atomic read-modify-write operation in a surface of the
>>> + * given dimensionality at the given coordinates. Main building
>>> block
>>> + * of the imageAtomic GLSL built-ins.
>>> + */
>>> + fs_reg
>>> + emit_image_atomic(const fs_builder &bld,
>>> + const fs_reg &image, const fs_reg &addr,
>>> + const fs_reg &src0, const fs_reg &src1,
>>> + unsigned dims, unsigned rsize, unsigned op)
>>> + {
>>> + using namespace image_validity;
>>> + using namespace image_coordinates;
>>> + using namespace surface_access;
>>> + /* Avoid performing an atomic operation on an unbound surface. */
>>> + const brw_predicate pred = emit_surface_check(bld, image);
>>> +
>>> + /* Thankfully we can do without untyped atomics here. */
>>> + const fs_reg tmp = emit_typed_atomic(bld, image, addr, src0, src1,
>>> + dims, rsize, op, pred);
>>> +
>>> + /* An unbound surface access should give zero as result. */
>>> + if (rsize)
>>> + set_predicate(pred, bld.SEL(tmp, tmp, fs_reg(0)));
>>> +
>>> + return tmp;
>>> + }
>>> + }
>>> +}
>>> diff --git a/src/mesa/drivers/dri/i965/brw_fs_surface_builder.h
>>> b/src/mesa/drivers/dri/i965/brw_fs_surface_builder.h
>>> index 5424be6..cad8c27 100644
>>> --- a/src/mesa/drivers/dri/i965/brw_fs_surface_builder.h
>>> +++ b/src/mesa/drivers/dri/i965/brw_fs_surface_builder.h
>>> @@ -213,5 +213,22 @@ namespace brw {
>>> _mesa_get_format_datatype(format) == GL_INT);
>>> }
>>> }
>>> +
>>> + namespace image_access {
>>> + fs_reg
>>> + emit_image_load(const fs_builder &bld,
>>> + const fs_reg &image, const fs_reg &addr,
>>> + mesa_format format, unsigned dims);
>>> +
>>> + void
>>> + emit_image_store(const fs_builder &bld, const fs_reg &image,
>>> + const fs_reg &addr, const fs_reg &src,
>>> + mesa_format format, unsigned dims);
>>> + fs_reg
>>> + emit_image_atomic(const fs_builder &bld,
>>> + const fs_reg &image, const fs_reg &addr,
>>> + const fs_reg &src0, const fs_reg &src1,
>>> + unsigned dims, unsigned rsize, unsigned op);
>>> + }
>>> }
>>> #endif
>>> --
>>> 2.4.3
>>>
>>> _______________________________________________
>>> 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
