Kenneth Graunke <kenn...@whitecape.org> writes:
> On Monday, June 11, 2018 7:25:55 PM PDT Francisco Jerez wrote:
>> This replaces brw_set_message_descriptor() with the composition of
>> brw_set_desc() and a new inline helper function that packs the common
>> message descriptor controls into an integer. The goal is to represent
>> all message descriptors as a 32-bit integer which is written at once
>> into the instruction, which is more flexible (SENDS anyone?), robust
>> (see d2eecf0b0b24d203d0f171807681dffd830d54de fixing an issue
>> ultimately caused by some bits of the extended message descriptor
>> being left undefined) and future-proof than the current approach of
>> specifying the individual descriptor fields directly into the
>> instruction.
>>
>> This approach also seems more self-documenting, since it will allow
>> removing calls to functions with way too many arguments like
>> brw_set_*_message() and brw_send_indirect_message(), and instead
>> provide a single descriptor argument constructed from an appropriate
>> combination of brw_*_desc() helpers.
>>
>> Note that because brw_set_message_descriptor() was (conditionally?)
>> overriding fields of the instruction which strictly speaking weren't
>> part of the message descriptor, this involves calling
>> brw_inst_set_sfid() and brw_inst_set_eot() in some cases in addition
>> to brw_set_desc().
>> ---
>> src/intel/compiler/brw_eu.h | 29 +++++---
>> src/intel/compiler/brw_eu_emit.c | 108
>> +++++++++++-------------------
>> src/intel/compiler/brw_vec4_generator.cpp | 17 +++--
>> 3 files changed, 68 insertions(+), 86 deletions(-)
>>
>> diff --git a/src/intel/compiler/brw_eu.h b/src/intel/compiler/brw_eu.h
>> index 5a396339fde..b2b20713e45 100644
>> --- a/src/intel/compiler/brw_eu.h
>> +++ b/src/intel/compiler/brw_eu.h
>> @@ -256,14 +256,6 @@ void brw_set_sampler_message(struct brw_codegen *p,
>> unsigned simd_mode,
>> unsigned return_format);
>>
>> -void brw_set_message_descriptor(struct brw_codegen *p,
>> - brw_inst *inst,
>> - enum brw_message_target sfid,
>> - unsigned msg_length,
>> - unsigned response_length,
>> - bool header_present,
>> - bool end_of_thread);
>> -
>> void brw_set_dp_read_message(struct brw_codegen *p,
>> brw_inst *insn,
>> unsigned binding_table_index,
>> @@ -287,6 +279,27 @@ void brw_set_dp_write_message(struct brw_codegen *p,
>> unsigned end_of_thread,
>> unsigned send_commit_msg);
>>
>> +/**
>> + * Construct a message descriptor immediate with the specified common
>> + * descriptor controls.
>> + */
>> +static inline uint32_t
>> +brw_message_desc(const struct gen_device_info *devinfo,
>> + unsigned msg_length,
>> + unsigned response_length,
>> + bool header_present)
>> +{
>
> Perhaps it would be good to add
>
> assert(msg_length >= 1 && msg_length <= 15);
>
>> + if (devinfo->gen >= 5) {
>
> assert(response_length <= 16);
>
>
>> + return (msg_length << 25 |
>> + response_length << 20 |
>> + header_present << 19);
>> + } else {
>
> assert(response_length <= 8);
>
> I'm not so concerned with validating the values here, just thinking it
> might make sense to verify that mlen fits in a U4, for example, so we
> don't accidentally bleed over into other fields when encoding it.
>It's kind of a PITA to assert that each field is in range manually for each one of these helpers (the following patches introduce a pile of functions very much like this one), and verifying that the assertions are complete and match the definition of the hardware fields is not straightforward to review. I would have used the SET_FIELD() macro if it wasn't because it relies on macros being defined with specific suffixes for each field. If you believe it's going to be valuable I think I'm going to introduce a new helper more easily reusable than SET_FIELD() checking for overflow based on a bitfield specification, and use it instead of the plain left-shift operators. > I suppose the validator already catches this, though... > I don't think the validator will be able to catch such cases in general. >> + return (msg_length << 20 | >> + response_length << 16); >> + } >> +}
signature.asc
Description: PGP signature
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
