Skip to content
Snippets Groups Projects

tests/shader_runner_metal: Implement texture support (alternative proposal).

Open Giovanni Mascellani requested to merge giomasce/vkd3d:eyjafjallajokull into master
2 unresolved threads

After drafting a few different experiments, I came up with this alternative proposal for !1316. Ideally it goes in the direction we want descriptor mapping to work on Metal in the long run, with a design similar to what is used by the Metal shader converter (though not directly compatible, even in the long run).

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
    • @@ -1281,6 +1281,12 @@ static int msl_generator_init(struct msl_generator *gen, struct vsir_program *pr
       
           memset(gen, 0, sizeof(*gen));
           gen->program = program;
      +    if (!(gen->prefix = msl_get_prefix(type)))
      +    {
      +        msl_compiler_error(gen, VKD3D_SHADER_ERROR_MSL_INTERNAL,
      +                "Internal compiler error: Unhandled shader type %#x.", type);
      +        return VKD3D_ERROR_INVALID_SHADER;
      +    }
           vkd3d_string_buffer_cache_init(&gen->string_buffers);
           if (!(gen->buffer = vkd3d_string_buffer_get(&gen->string_buffers)))
           {

      I think we could just do what GLSL does (i.e., set "prefix" to "unknown") here; we don't really need to abort compilation. Probably didn't really need to be part of this MR either.

      Subject: [PATCH 2/6] tests/shader_runner_metal: Handle multisampled 2D texture
       arrays properly.
      
      They're not supported by the shader runner anyway, but there's no
      reason to make the code subtly wrong.

      That's fine, but I imagine the runners are going to need to be touched one way or another anyway when support for multisampled array textures is added. /shrug

      +static id<MTLBuffer> encode_stage_descriptors(struct metal_runner *runner,
      +        id<MTLResource> *resident_resource, unsigned int* resident_resource_count)
      +{
      +    NSMutableArray<MTLArgumentDescriptor *> *argument_descriptors = [[[NSMutableArray alloc] init] autorelease];
      +    unsigned int resident_resource_index = 0;
      +    id<MTLDevice> device = runner->device;
      +    MTLArgumentDescriptor *arg_desc;
      +    id<MTLArgumentEncoder> encoder;
      +    id<MTLBuffer> argument_buffer;
      +    id<MTLBuffer> cb;
      +
      +    if (runner->r.uniform_count)
      +    {
      +        arg_desc = [MTLArgumentDescriptor argumentDescriptor];
      +        arg_desc.dataType = MTLDataTypePointer;
      +        arg_desc.index = 0;
      +        arg_desc.access = MTLBindingAccessReadOnly;
      +        [argument_descriptors addObject:arg_desc];
      +    }
      +
      +    if (![argument_descriptors count])
      +        return nil;
      +
      +    encoder = [[device newArgumentEncoderWithArguments:argument_descriptors] autorelease];
      +    argument_buffer = [[device newBufferWithLength:encoder.encodedLength
      +            options:DEFAULT_BUFFER_RESOURCE_OPTIONS] autorelease];
      +    [encoder setArgumentBuffer:argument_buffer offset:0];
      +
      +    if (runner->r.uniform_count)
      +    {
      +        cb = [[device newBufferWithBytes:runner->r.uniforms
      +                length:runner->r.uniform_count * sizeof(*runner->r.uniforms)
      +                options:DEFAULT_BUFFER_RESOURCE_OPTIONS] autorelease];
      +        [encoder setBuffer:cb offset:0 atIndex:0];
      +        resident_resource[resident_resource_index++] = cb;
      +    }
      +
      +    *resident_resource_count = resident_resource_index;
      +    return argument_buffer;
      +}

      It doesn't seem terribly necessary to introduce this function before we have compute shaders, but fine. If we're going to have this function though:

      • encode_stage_descriptors() is a poor name; there's nothing specific to a particular pipeline stage in here.
      • The useResources() call seems awkward. It adds MTLResourceUsageWrite for resources that in principle are read-only, and there doesn't seem much of a point in handling that outside of this function anyway.
      @@ -36,6 +36,8 @@ struct metal_resource
       
           id<MTLBuffer> buffer;
           id<MTLTexture> texture;
      +
      +    unsigned int binding;
       };

      We don't really need the "binding" field, do we? We're just binding things in the order they appear in the resources[] array. Getting rid of "binding" would allow us to get rid of init_descriptor_binding() as well.

      +    if (params->desc.type == RESOURCE_TYPE_TEXTURE)
      +    {
      +        MTLPixelFormat format = get_metal_pixel_format(params->desc.format);
      +        unsigned int texel_size = params->desc.texel_size;
      +        MTLTextureDescriptor *desc;
      +
      +        /* create a R32Uint view for structured buffer */
      +        if (params->stride && format == MTLPixelFormatInvalid)
      +        {
      +            format = MTLPixelFormatR32Uint;
      +            texel_size = 4;
      +        }
      +
      +        desc = [[MTLTextureDescriptor alloc] init];
      +        desc.textureType = MTLTextureTypeTextureBuffer;
      +        desc.pixelFormat = format;
      +        desc.width = params->data_size / texel_size;
      +
      +        resource->texture = [resource->buffer newTextureWithDescriptor:desc offset:0 bytesPerRow:params->data_size];
      +
      +        [desc release];
      +    }

      Does MTLTextureDescriptor creation need to be significantly different between texture and buffer resources? Do we even need to worry about buffer textures and structured buffers yet in the first place?

      +    switch (params->desc.type) {
      +        case RESOURCE_TYPE_RENDER_TARGET:
      +        case RESOURCE_TYPE_DEPTH_STENCIL:
      +            desc.storageMode = MTLStorageModePrivate;
      +            desc.usage = MTLTextureUsageRenderTarget;
      +            break;
      +
      +        case RESOURCE_TYPE_TEXTURE:
      +            desc.storageMode = MTLStorageModeManaged;
      +            desc.usage = MTLTextureUsageShaderRead;
      +            break;
      +
      +        default:
      +            break;
      +    }

      Formatting. Why MTLStorageModeManaged for SRVs?

      +    if (params->data)
      +    {
      +        unsigned int buffer_offset = 0;
      +        unsigned int level_height;
      +        unsigned int level_width;
      +        unsigned int level;
      +
      +        if (params->desc.sample_count > 1)
      +            fatal_error("Cannot upload data to a multisampled texture.\n");
      +
      +        for (level = 0; level < params->desc.level_count; ++level)
      +        {
      +            level_width  = get_level_dimension(params->desc.width, level);
      +            level_height = get_level_dimension(params->desc.height, level);
      +            [resource->texture replaceRegion:MTLRegionMake2D(0, 0, level_width, level_height)
      +                    mipmapLevel:level
      +                    slice:0
      +                    withBytes:&params->data[buffer_offset]
      +                    bytesPerRow:level_width * params->desc.texel_size
      +                    bytesPerImage:level_height * level_width * params->desc.texel_size];
      +            buffer_offset += level_height * level_width * params->desc.texel_size;
      +        }
      +    }

      "slice:0" doesn't seem right for array textures. I don't think any tests set initial data for array textures, but we could could at least error out like we do for multisample textures.

      +    for (i = 0; i < runner->r.resource_count; ++i)
      +    {
      +        const struct metal_resource *resource = metal_resource(runner->r.resources[i]);
      +
      +        switch (resource->r.desc.type)
      +        {
      +            case RESOURCE_TYPE_RENDER_TARGET:
      +            case RESOURCE_TYPE_DEPTH_STENCIL:
      +            case RESOURCE_TYPE_VERTEX_BUFFER:
      +                break;
      +
      +            case RESOURCE_TYPE_TEXTURE:
      +            case RESOURCE_TYPE_UAV:
      +                binding = &bindings[interface_info.binding_count++];
      +                if (resource->r.desc.type == RESOURCE_TYPE_UAV)
      +                    binding->type = VKD3D_SHADER_DESCRIPTOR_TYPE_UAV;
      +                else
      +                    binding->type = VKD3D_SHADER_DESCRIPTOR_TYPE_SRV;
      +                binding->register_space = 0;
      +                binding->register_index = resource->r.desc.slot;
      +                binding->shader_visibility = VKD3D_SHADER_VISIBILITY_ALL;
      +                binding->flags = VKD3D_SHADER_BINDING_FLAG_IMAGE;
      +                binding->binding.set = 0;
      +                binding->binding.binding = resource->binding;
      +                binding->binding.count = 1;
      +                break;
      +        }
      +    }

      I don't think we need to care about anything other than RESOURCE_TYPE_TEXTURE here for the moment. VKD3D_SHADER_BINDING_FLAG_IMAGE would be wrong for buffer UAVs/SRVs.

      +    for (i = 0; i < runner->r.resource_count; ++i)
      +    {
      +        struct metal_resource *resource = metal_resource(runner->r.resources[i]);
      +
      +        switch (resource->r.desc.type)
      +        {
      +            case RESOURCE_TYPE_TEXTURE:
      +            case RESOURCE_TYPE_UAV:
      +                arg_desc = [MTLArgumentDescriptor argumentDescriptor];
      +                arg_desc.dataType = MTLDataTypeTexture;
      +                arg_desc.index = resource->binding;
      +                assert(arg_desc.index == [argument_descriptors count]);
      +                arg_desc.access = MTLBindingAccessReadOnly;
      +                arg_desc.textureType = [resource->texture textureType];
      +                [argument_descriptors addObject:arg_desc];
      +                break;
      +
      +            default:
      +                break;
      +        }
      +    }

      MTLBindingAccessReadOnly would be wrong for UAVs.

      +struct msl_resource_type_info
      +{
      +    /* The number of coordinates needed to sample the resource type. */
      +    size_t sample_coord_size;
      +    /* The number of coordinates needed to read the resource type. */
      +    size_t read_coord_size;
      +    /* Whether the resource type is an array type. */
      +    bool array;
      +    /* Whether the resource type is a cube type. */
      +    bool cube;
      +    /* Whether the resource type supports lod. */
      +    bool lod;
      +    /* The type suffix for resource type. I.e., the "2d" part of "texture2d" */
      +    const char *type_suffix;
      +};

      "sample_coord_size" and "cube" are unused. sample_coord_size = read_coord_size + cube, so one of those three is also just redundant.

      +static const struct vkd3d_shader_descriptor_info1 *msl_get_descriptor_by_id(
      +        struct msl_generator *gen, enum vkd3d_shader_descriptor_type type, unsigned int id)
      +{
      +    const struct vkd3d_shader_scan_descriptor_info1 *info = &gen->program->descriptors;
      +
      +    for (unsigned int i = 0; i < info->descriptor_count; ++i)
      +    {
      +        const struct vkd3d_shader_descriptor_info1 *d = &info->descriptors[i];
      +
      +        if (d->type == type && d->register_id == id)
      +            return d;
      +    }
      +
      +    return NULL;
      +}

      That's vkd3d_shader_find_descriptor() isn't it? Applies to shader_glsl_get_descriptor_by_id() as well.

      +static void msl_print_resource_datatype(struct msl_generator *gen,
      +        struct vkd3d_string_buffer *buffer, enum vkd3d_shader_resource_data_type data_type)
      +{
      +    switch (data_type)
      +    {
      +        case VKD3D_SHADER_RESOURCE_DATA_FLOAT:
      +        case VKD3D_SHADER_RESOURCE_DATA_UNORM:
      +        case VKD3D_SHADER_RESOURCE_DATA_SNORM:
      +            vkd3d_string_buffer_printf(buffer, "float");
      +            break;
      +        case VKD3D_SHADER_RESOURCE_DATA_INT:
      +            vkd3d_string_buffer_printf(buffer, "int");
      +            break;
      +        case VKD3D_SHADER_RESOURCE_DATA_UINT:
      +            vkd3d_string_buffer_printf(buffer, "uint");
      +            break;
      +        default:
      +            msl_compiler_error(gen, VKD3D_SHADER_ERROR_MSL_INTERNAL,
      +                    "Internal compiler error: Unhandled resource datatype %#x.", data_type);
      +            vkd3d_string_buffer_printf(buffer, "<unrecognised resource datatype %#x>", data_type);
      +            break;
      +    }
      +}

      It's not necessarily wrong, but I'd suggest sticking with enum vkd3d_data_type here. (Particularly if we're going to use that in struct vkd3d_shader_descriptor_info1 at some point.)

      +static const struct vkd3d_shader_descriptor_binding *msl_get_srv_binding(const struct msl_generator *gen,
      +        unsigned int register_space, unsigned int register_idx)
      +{
      +    const struct vkd3d_shader_interface_info *interface_info = gen->interface_info;
      +    unsigned int i;
      +
      +    if (!interface_info)
      +        return NULL;
      +
      +    for (i = 0; i < interface_info->binding_count; ++i)
      +    {
      +        const struct vkd3d_shader_resource_binding *binding = &interface_info->bindings[i];
      +
      +        if (binding->type != VKD3D_SHADER_DESCRIPTOR_TYPE_SRV)
      +            continue;
      +        if (binding->register_space != register_space)
      +            continue;
      +        if (binding->register_index != register_idx)
      +            continue;
      +        if (!msl_check_shader_visibility(gen, binding->shader_visibility))
      +            continue;
      +        if (!(binding->flags & VKD3D_SHADER_BINDING_FLAG_IMAGE))
      +            continue;
      +
      +        return &binding->binding;
      +    }
      +
      +    return NULL;
      +}

      Checking for VKD3D_SHADER_BINDING_FLAG_IMAGE is wrong for buffer SRVs.

      +    if (!(binding = msl_get_srv_binding(gen, resource_space, resource_idx)))
      +    {
      +        msl_compiler_error(gen, VKD3D_SHADER_ERROR_MSL_BINDING_NOT_FOUND,
      +                "Cannot finding binding for SRV register %u.", resource_id);
      +        vkd3d_string_buffer_printf(gen->buffer, "<unhandled register %#x>", ins->src[1].reg.type);
      +        return;
      +    }
      +
      +    msl_dst_init(&dst, gen, ins, &ins->dst[0]);
      +    msl_src_init(&coord, gen, &ins->src[0], coord_mask);
      +    /* `coord_mask + 1` gives exactly the array index component mask if it is an array resource */
      +    /* Or it's simply unused, saving some branches */
      +    msl_src_init(&array_index, gen, &ins->src[0], coord_mask + 1);
      +    msl_src_init(&lod, gen, &ins->src[0], VKD3DSP_WRITEMASK_3);
      +    fetch = vkd3d_string_buffer_get(&gen->string_buffers);
      +
      +    vkd3d_string_buffer_printf(fetch, "as_type<uint4>(descriptors[%u].tex<texture%s<",
      +            binding->binding, resource_type_info->type_suffix);
      +    msl_print_resource_datatype(gen, fetch, d->resource_data_type);
      +    vkd3d_string_buffer_printf(fetch, ">>()");
      +    if (resource_space)
      +        vkd3d_string_buffer_printf(fetch, "_%u", resource_space);
      +    vkd3d_string_buffer_printf(fetch, ".read(");
      +    if (resource_type_info->read_coord_size > 1)
      +        vkd3d_string_buffer_printf(fetch, "as_type<uint%zu>(%s)", resource_type_info->read_coord_size, coord.str->buffer);
      +    else
      +        vkd3d_string_buffer_printf(fetch, "as_type<uint>(%s)", coord.str->buffer);
      +    if (resource_type_info->array)
      +        vkd3d_string_buffer_printf(fetch, ", as_type<uint>(%s)", array_index.str->buffer);
      +    if (resource_type_info->lod)
      +        vkd3d_string_buffer_printf(fetch, ", as_type<uint>(%s)", lod.str->buffer);
      +    vkd3d_string_buffer_printf(fetch, "))");
      +    msl_print_swizzle(fetch, ins->src[1].swizzle, ins->dst[0].write_mask);
      +
      +    msl_print_assignment(gen, &dst, "%s", fetch->buffer);
      • The error handling when the binding can't be found isn't as helpful as it could be. It should be easy to include some information about the resource ID/index/space, which would be helpful trying to figure out which binding we're looking for.
      • It seems helpful to introduce a helper function to print the descriptor variable/expression, somewhat similar to shader_glsl_print_combined_sampler_name() and shader_glsl_print_image_name() for GLSL, at least conceptually.
      • The "d->resource_data_type" access is unsafe; "d" could be NULL at that point.
      • The "if (resource_space)" bit just seems wrong.
      • The "as_type<>" bits for the read() parameters don't seem that helpful. The vkd3d_vec4 type is a union, and we should just use its .u field. We could use something like shader_glsl_print_src() to handle to different data type.
    • I implemented some suggestions, mostly those belonging to the shader runner. I still have to go through the suggestions for the MSL generator. I removed support for buffer textures and the bits dealing with UAVs.

      Why MTLStorageModeManaged for SRVs?

      It's needed to use replaceRegion. I suppose one might use Private and then copy through a staging buffer, but is there a particular reason for avoiding Managed?

      BTW, sorry for the many bloopers in this MR. It was rather tricky to get the agument buffer bits right, and I forgot to care about other stuff.

    • I pushed a number of changes. It still probably requires a whole re-read on my part, but not now, I need a break. A couple of comments, though.

      It's not necessarily wrong, but I'd suggest sticking with enum vkd3d_data_type here. (Particularly if we're going to use that in struct vkd3d_shader_descriptor_info1 at some point.)

      Sure, I'll do that as a rebase after !1446 is merged.

      It seems helpful to introduce a helper function to print the descriptor variable/expression, somewhat similar to shader_glsl_print_combined_sampler_name() and shader_glsl_print_image_name() for GLSL, at least conceptually.

      Maybe, but honestly it's not really clear to me what's the right abstraction level yet. Can this be postponed?

      The "as_type<>" bits for the read() parameters don't seem that helpful. The vkd3d_vec4 type is a union, and we should just use its .u field. We could use something like shader_glsl_print_src() to handle to different data type.

      Agreed, but I began doing that and it's a bit of slippery slope, it quickly needs quite a lot of changes (it's more work than shader_glsl_print_src() precisely because vkd3d_vec4 exists, but not all registers emit something of that type, so the bitcasting code must handle two different situations, which require integrating with all the other places where sources are written). Since this MR is already quite beefy, can this be postponed too?

    • Please register or sign in to reply
  • Giovanni Mascellani mentioned in merge request !1454 (merged)

    mentioned in merge request !1454 (merged)

  • added 5 commits

    • d070856d - tests/shader_runner_metal: Handle multisampled 2D texture arrays properly.
    • a459f2d9 - tests/shader_runner_metal: Introduce a helper to encode the argument buffer.
    • 09bcaf8a - vkd3d-shader/msl: Access descriptors with a type-erased array.
    • ef65a7ee - tests/shader_runner_metal: Implement texture support.
    • 0c3bc1c5 - vkd3d-shader/msl: Implement VKD3DSIH_LD.

    Compare with previous version

  • added 2 commits

    • 35850464 - tests/shader_runner_metal: Add texture support.
    • 3f710fff - vkd3d-shader/msl: Implement VKD3DSIH_LD.

    Compare with previous version

  • added 40 commits

    • 3f710fff...70655012 - 35 commits from branch wine:master
    • 769d2dff - tests/shader_runner_metal: Handle multisampled 2D texture arrays properly.
    • b82e01ee - tests/shader_runner_metal: Introduce a helper to encode the argument buffer.
    • 835f1c6d - vkd3d-shader/msl: Access descriptors with a type-erased array.
    • ebc9545f - tests/shader_runner_metal: Add texture support.
    • 146dd871 - vkd3d-shader/msl: Implement VKD3DSIH_LD.

    Compare with previous version

  • added 1 commit

    • 7069eea2 - vkd3d-shader/msl: Implement VKD3DSIH_LD.

    Compare with previous version

  • Giovanni Mascellani left review comments

    left review comments

    • Why MTLStorageModeManaged for SRVs?

      It's needed to use replaceRegion. I suppose one might use Private and then copy through a staging buffer, but is there a particular reason for avoiding Managed?

      No strong ones; it mostly just stood out to me because it's different from the RTV/DSV case. It does seem a little wasteful to use Managed for textures that we're going to initialise once and then not touch though, and we do use the "Private + staging buffer" approach in the other direction for RTVs. (I.e., see metal_runner_get_resource_readback().)

      Looking a bit closer, I also see we're using MTLResourceStorageModeShared in DEFAULT_BUFFER_RESOURCE_OPTIONS, which I think is not guaranteed to be available on all hardware?

      It seems helpful to introduce a helper function to print the descriptor variable/expression, somewhat similar to shader_glsl_print_combined_sampler_name() and shader_glsl_print_image_name() for GLSL, at least conceptually.

      Maybe, but honestly it's not really clear to me what's the right abstraction level yet. Can this be postponed?

      Sure; the reason to bring it up now is mostly that it might be a bit harder to track down the individual cases later though.

      The "as_type<>" bits for the read() parameters don't seem that helpful. The vkd3d_vec4 type is a union, and we should just use its .u field. We could use something like shader_glsl_print_src() to handle to different data type.

      Agreed, but I began doing that and it's a bit of slippery slope, it quickly needs quite a lot of changes (it's more work than shader_glsl_print_src() precisely because vkd3d_vec4 exists, but not all registers emit something of that type, so the bitcasting code must handle two different situations, which require integrating with all the other places where sources are written). Since this MR is already quite beefy, can this be postponed too?

      I think it should mostly be a matter of explicitly passing the data type to msl_print_register_name(), instead of using "reg->data_type" for that. The exception is "o_depth", but I think that could just be a vkd3d_vec4 as well?

      I'm not going to insist on putting that in this MR either, but like above, it seems easier to change now than later to me.

    • No strong ones; it mostly just stood out to me because it's different from the RTV/DSV case. It does seem a little wasteful to use Managed for textures that we're going to initialise once and then not touch though, and we do use the "Private + staging buffer" approach in the other direction for RTVs. (I.e., see metal_runner_get_resource_readback().)

      I had assumed that DSV and RTV must be private, but that doesn't seem to be true. Personally I don't care too much if the shader runner is a little bit wasteful, but it shouldn't be hard to use a staging buffer here too, I guess.

      Looking a bit closer, I also see we're using MTLResourceStorageModeShared in DEFAULT_BUFFER_RESOURCE_OPTIONS, which I think is not guaranteed to be available on all hardware?

      Ah, right. I had understood from https://developer.apple.com/documentation/metal/mtlresourceoptions/storagemodeshared?language=objc that it's always available for buffers, but macOS systems with discrete GPUs indeed exist. I'll have to think about it.

      Sure; the reason to bring it up now is mostly that it might be a bit harder to track down the individual cases later though.

      Well, my problem with that is not that I don't want to do it, but, as I said, that I'm not sure of what should go inside of it. If you have clear what's a good interface for it, I have no problems using yours.

      I'm not going to insist on putting that in this MR either, but like above, it seems easier to change now than later to me.

      My intention is to do that right after this MR gets merged. I just need to not further increase the cognitive cost of this MR, because it's already quite high for me.

    • Please register or sign in to reply
  • Sure; the reason to bring it up now is mostly that it might be a bit harder to track down the individual cases later though.

    Well, my problem with that is not that I don't want to do it, but, as I said, that I'm not sure of what should go inside of it. If you have clear what's a good interface for it, I have no problems using yours.

    I was thinking simply something like "msl_print_srv_name(buffer, msl, binding, resource_type, resource_data_type);", which would print something like "descriptors[%u].tex<texture%s<%s>>()", and then an equivalent msl_print_cbv_name().

    I don't know whether that's necessarily the optimal interface, but it should at least get us to the point where we can just search for msl_print_srv_name() or msl_print_cbv_name() when we need to touch the code generated for descriptors, instead of having to find that (potentially) spread over multiple vkd3d_string_buffer_printf() calls.

    I'm not going to insist on putting that in this MR either, but like above, it seems easier to change now than later to me.

    My intention is to do that right after this MR gets merged. I just need to not further increase the cognitive cost of this MR, because it's already quite high for me.

    Sure, that's fine.

  • added 4 commits

    • c88c072a - tests/shader_runner_metal: Do not use shared buffers.
    • 6976075a - vkd3d-shader/msl: Access descriptors with a type-erased array.
    • e2c84089 - tests/shader_runner_metal: Add texture support.
    • ee265f3d - vkd3d-shader/msl: Implement VKD3DSIH_LD.

    Compare with previous version

Please register or sign in to reply
Loading