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