[PATCH vkd3d 3/5] tests: Add a shader test for texture sampling.

[Zebediah Figura (she/her)]

On 11/2/21 08:46, Giovanni Mascellani wrote:
> Hi,
> 
> On 02/11/21 01:43, Zebediah Figura wrote:
>> +static D3D12_TEXTURE_ADDRESS_MODE parse_sampler_address_mode(const 
>> char *line, const char **rest)
>> +{
>> +    if (match_string(line, "border", rest))
>> +        return D3D12_TEXTURE_ADDRESS_MODE_BORDER;
>> +    if (match_string(line, "clamp", rest))
>> +        return D3D12_TEXTURE_ADDRESS_MODE_CLAMP;
>> +    if (match_string(line, "mirror once", rest))
>> +        return D3D12_TEXTURE_ADDRESS_MODE_MIRROR_ONCE;
>> +    if (match_string(line, "mirror", rest))
>> +        return D3D12_TEXTURE_ADDRESS_MODE_MIRROR;
>> +    if (match_string(line, "wrap", rest))
>> +        return D3D12_TEXTURE_ADDRESS_MODE_WRAP;
>> +    fprintf(stderr, "Malformed address mode '%s'.\n", line);
>> +    return D3D12_TEXTURE_ADDRESS_MODE_WRAP;
>> +}
> 
> I don't like very much the "mirror once" thing: the structure of the 
> file hints that spaces are used for token separation, and here we would 
> be using two tokens for an option that seems to be completely similar to 
> the other options using just one token. Also, the hypothetical line 
> "mirror_once mirror mirror_once" would be easier to mentally scan than 
> "mirror once mirror mirror once". Can we change to "mirror_once"?

Sure, seems reasonable to me.

> Also, the validation fundamentalist that is in me would happily swap the 
> last return statement with a call to abort(), but I guess I can silence 
> him for a test program.

It probably would be a good idea to fail more noisily in general, 
honestly. I'll send a follow-up patch.

>> +static void parse_sampler_directive(struct sampler *sampler, const 
>> char *line)
>> +{
>> +    const char *const orig_line = line;
>> +
>> +    if (match_string(line, "address", &line))
>> +    {
>> +        sampler->u_address = parse_sampler_address_mode(line, &line);
>> +        sampler->v_address = parse_sampler_address_mode(line, &line);
>> +        sampler->w_address = parse_sampler_address_mode(line, &line);
>> +    }
>> +    else if (match_string(line, "filter", &line))
>> +    {
>> +        static const struct
>> +        {
>> +            const char *string;
>> +            D3D12_FILTER filter;
>> +        }
>> +        filters[] =
>> +        {
>> +            {"point point point",       D3D12_FILTER_MIN_MAG_MIP_POINT},
>> +            {"point point linear",      
>> D3D12_FILTER_MIN_MAG_POINT_MIP_LINEAR},
>> +            {"point linear point",      
>> D3D12_FILTER_MIN_POINT_MAG_LINEAR_MIP_POINT},
>> +            {"point linear linear",     
>> D3D12_FILTER_MIN_POINT_MAG_MIP_LINEAR},
>> +            {"linear point point",      
>> D3D12_FILTER_MIN_LINEAR_MAG_MIP_POINT},
>> +            {"linear point linear",     
>> D3D12_FILTER_MIN_LINEAR_MAG_POINT_MIP_LINEAR},
>> +            {"linear linear point",     
>> D3D12_FILTER_MIN_MAG_LINEAR_MIP_POINT},
>> +            {"linear linear linear",    
>> D3D12_FILTER_MIN_MAG_MIP_LINEAR},
>> +        };
>> +        unsigned int i;
>> +
>> +        for (i = 0; i < ARRAY_SIZE(filters); ++i)
>> +        {
>> +            if (match_string(line, filters[i].string, &line))
>> +                sampler->filter = filters[i].filter;
>> +        }
>> +    }
>> +    else
>> +    {
>> +        goto err;
>> +    }
>> +
>> +    return;
>> +
>> +err:
>> +    fprintf(stderr, "Ignoring malformed line '%s'.\n", orig_line);
>> +}
> 
> I don't find the goto statement very useful: you could just print the 
> error in the "else" branch. Unless you have plans to extend the function 
> in ways I don't see now, I think this is just complicating the control 
> flow.

Not for parse_sampler_directive() specifically, but in general yes.

But something like a fatal_error(...) function would also obviate the 
need for a goto.

> 
>> @@ -305,21 +383,40 @@ static void parse_test_directive(struct 
>> shader_context *context, const char *lin
>>               range->RegisterSpace = 0;
>>               range->OffsetInDescriptorsFromTableStart = 0;
>> -            texture->heap = 
>> create_gpu_descriptor_heap(context->c.device, 
>> D3D12_DESCRIPTOR_HEAP_TYPE_CBV_SRV_UAV, 1);
>> -            texture->resource = 
>> create_default_texture(context->c.device, texture->width, 
>> texture->height,
>> -                    texture->format, 0, D3D12_RESOURCE_STATE_COPY_DEST);
>> -            resource_data.pData = texture->data;
>> -            resource_data.SlicePitch = resource_data.RowPitch = 
>> texture->width * texture->texel_size;
>> -            upload_texture_data(texture->resource, &resource_data, 1, 
>> context->c.queue, command_list);
>> -            reset_command_list(command_list, context->c.allocator);
>> -            transition_resource_state(command_list, 
>> texture->resource, D3D12_RESOURCE_STATE_COPY_DEST,
>> -                    D3D12_RESOURCE_STATE_NON_PIXEL_SHADER_RESOURCE | 
>> D3D12_RESOURCE_STATE_PIXEL_SHADER_RESOURCE);
>> -            ID3D12Device_CreateShaderResourceView(context->c.device, 
>> texture->resource,
>> -                    NULL, get_cpu_descriptor_handle(&context->c, 
>> texture->heap, 0));
>> +            if (!texture->resource)
>> +            {
>> +                texture->heap = 
>> create_gpu_descriptor_heap(context->c.device,
>> +                        D3D12_DESCRIPTOR_HEAP_TYPE_CBV_SRV_UAV, 1);
>> +                texture->resource = 
>> create_default_texture(context->c.device, texture->width, 
>> texture->height,
>> +                        texture->format, 0, 
>> D3D12_RESOURCE_STATE_COPY_DEST);
>> +                resource_data.pData = texture->data;
>> +                resource_data.SlicePitch = resource_data.RowPitch = 
>> texture->width * texture->texel_size;
>> +                upload_texture_data(texture->resource, 
>> &resource_data, 1, context->c.queue, command_list);
>> +                reset_command_list(command_list, context->c.allocator);
>> +                transition_resource_state(command_list, 
>> texture->resource, D3D12_RESOURCE_STATE_COPY_DEST,
>> +                        
>> D3D12_RESOURCE_STATE_NON_PIXEL_SHADER_RESOURCE | 
>> D3D12_RESOURCE_STATE_PIXEL_SHADER_RESOURCE);
>> +                
>> ID3D12Device_CreateShaderResourceView(context->c.device, 
>> texture->resource,
>> +                        NULL, get_cpu_descriptor_handle(&context->c, 
>> texture->heap, 0));
>> +            }
> 
> Is this hunk related to the patch? AFAIU, it just prevents recreating 
> the texture resource if it's already there, which is a good thing, but 
> probably worthy of another patch.

Mostly because this is the first shader_test file that includes more 
than one valid [test] directive. But yes, it could use a separate patch.

Same for the hunk quoted below.

> 
>> @@ -650,11 +765,36 @@ START_TEST(shader_runner_d3d12)
>>               if (!strcmp(line, "[pixel shader]\n"))
>>               {
>>                   state = STATE_SHADER_PIXEL;
>> +
>> +                if (context.ps_code)
>> +                    ID3D10Blob_Release(context.ps_code);
>> +                context.ps_code = NULL;
>>               }
> 
> This hunk too seems unrelated to this patch.
> 
> Thanks, Giovanni.
>