[PATCH 1/2] drm/xe/guc: Use HXG definitions on HXG messages
Michal Wajdeczko
michal.wajdeczko at intel.com
Thu Jan 11 09:44:13 UTC 2024
On 11.01.2024 00:06, Matthew Brost wrote:
> On Wed, Jan 10, 2024 at 08:59:50PM +0100, Michal Wajdeczko wrote:
>> While parsing and processing CTB G2H messages we should extract
>> underlying HXG message and use HXG definitions on such message.
>> Using outer CTB layer message in HXG definitions require use of
>> shifted dword index, which might be confusing:
>>
>> FIELD_GET(GUC_HXG_MSG_0_xxx, msg[1])
>>
>> instead of:
>>
>> FIELD_GET(GUC_HXG_MSG_0_xxx, hxg[0])
>>
>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko at intel.com>
>> Cc: Matthew Brost <matthew.brost at intel.com>
>> ---
>> drivers/gpu/drm/xe/xe_guc_ct.c | 72 +++++++++++++++++++++-------------
>> 1 file changed, 44 insertions(+), 28 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_guc_ct.c b/drivers/gpu/drm/xe/xe_guc_ct.c
>> index c29f095aa1b9..d6b7020a2d2f 100644
>> --- a/drivers/gpu/drm/xe/xe_guc_ct.c
>> +++ b/drivers/gpu/drm/xe/xe_guc_ct.c
>> @@ -796,9 +796,20 @@ int xe_guc_ct_send_recv_no_fail(struct xe_guc_ct *ct, const u32 *action,
>> return guc_ct_send_recv(ct, action, len, response_buffer, true);
>> }
>>
>> +static u32 *msg_to_hxg(u32 *msg)
>> +{
>> + return msg + GUC_CTB_MSG_MIN_LEN;
>> +}
>> +
>> +static u32 msg_len_to_hxg(u32 len)
>
> msg_len_to_hxg_len?
ok
>
>> +{
>> + return len - GUC_CTB_MSG_MIN_LEN;
>> +}
>> +
>> static int parse_g2h_event(struct xe_guc_ct *ct, u32 *msg, u32 len)
>> {
>> - u32 action = FIELD_GET(GUC_HXG_EVENT_MSG_0_ACTION, msg[1]);
>> + u32 *hxg = msg_to_hxg(msg);
>> + u32 action = FIELD_GET(GUC_HXG_EVENT_MSG_0_ACTION, hxg[0]);
>>
>> lockdep_assert_held(&ct->lock);
>>
>> @@ -817,9 +828,10 @@ static int parse_g2h_response(struct xe_guc_ct *ct, u32 *msg, u32 len)
>> {
>> struct xe_gt *gt = ct_to_gt(ct);
>> struct xe_device *xe = gt_to_xe(gt);
>> - u32 response_len = len - GUC_CTB_MSG_MIN_LEN;
>> + const u32 *hxg = msg_to_hxg(msg);
>
> Some places do:
> const u32 *hxg = msg_to_hxg(msg);
>
> Others do:
> u32 *hxg = msg_to_hxg(msg);
>
> I do not see any reason why we have 2 different conventions.
I was trying to add constness concept of parsed/processed HXG message,
but since it wasn't done correctly for passed CTB message arguments I
gave up, missed to cleanup this one leftover
will drop const for now
>
> Everything else LGTM.
>
> Matt
>
>> + u32 hxg_len = msg_len_to_hxg(len);
>> u32 fence = FIELD_GET(GUC_CTB_MSG_0_FENCE, msg[0]);
>> - u32 type = FIELD_GET(GUC_HXG_MSG_0_TYPE, msg[1]);
>> + u32 type = FIELD_GET(GUC_HXG_MSG_0_TYPE, hxg[0]);
>> struct g2h_fence *g2h_fence;
>>
>> lockdep_assert_held(&ct->lock);
>> @@ -836,8 +848,8 @@ static int parse_g2h_response(struct xe_guc_ct *ct, u32 *msg, u32 len)
>> if (type == GUC_HXG_TYPE_RESPONSE_FAILURE)
>> xe_gt_err(gt, "FAST_REQ H2G fence 0x%x failed! e=0x%x, h=%u\n",
>> fence,
>> - FIELD_GET(GUC_HXG_FAILURE_MSG_0_ERROR, msg[1]),
>> - FIELD_GET(GUC_HXG_FAILURE_MSG_0_HINT, msg[1]));
>> + FIELD_GET(GUC_HXG_FAILURE_MSG_0_ERROR, hxg[0]),
>> + FIELD_GET(GUC_HXG_FAILURE_MSG_0_HINT, hxg[0]));
>> else
>> xe_gt_err(gt, "unexpected response %u for FAST_REQ H2G fence 0x%x!\n",
>> type, fence);
>> @@ -857,18 +869,14 @@ static int parse_g2h_response(struct xe_guc_ct *ct, u32 *msg, u32 len)
>>
>> if (type == GUC_HXG_TYPE_RESPONSE_FAILURE) {
>> g2h_fence->fail = true;
>> - g2h_fence->error =
>> - FIELD_GET(GUC_HXG_FAILURE_MSG_0_ERROR, msg[1]);
>> - g2h_fence->hint =
>> - FIELD_GET(GUC_HXG_FAILURE_MSG_0_HINT, msg[1]);
>> + g2h_fence->error = FIELD_GET(GUC_HXG_FAILURE_MSG_0_ERROR, hxg[0]);
>> + g2h_fence->hint = FIELD_GET(GUC_HXG_FAILURE_MSG_0_HINT, hxg[0]);
>> } else if (type == GUC_HXG_TYPE_NO_RESPONSE_RETRY) {
>> g2h_fence->retry = true;
>> - g2h_fence->reason =
>> - FIELD_GET(GUC_HXG_RETRY_MSG_0_REASON, msg[1]);
>> + g2h_fence->reason = FIELD_GET(GUC_HXG_RETRY_MSG_0_REASON, hxg[0]);
>> } else if (g2h_fence->response_buffer) {
>> - g2h_fence->response_len = response_len;
>> - memcpy(g2h_fence->response_buffer, msg + GUC_CTB_MSG_MIN_LEN,
>> - response_len * sizeof(u32));
>> + g2h_fence->response_len = hxg_len;
>> + memcpy(g2h_fence->response_buffer, hxg, hxg_len * sizeof(u32));
>> }
>>
>> g2h_release_space(ct, GUC_CTB_HXG_MSG_MAX_LEN);
>> @@ -884,14 +892,13 @@ static int parse_g2h_response(struct xe_guc_ct *ct, u32 *msg, u32 len)
>> static int parse_g2h_msg(struct xe_guc_ct *ct, u32 *msg, u32 len)
>> {
>> struct xe_device *xe = ct_to_xe(ct);
>> - u32 hxg, origin, type;
>> + const u32 *hxg = msg_to_hxg(msg);
>> + u32 origin, type;
>> int ret;
>>
>> lockdep_assert_held(&ct->lock);
>>
>> - hxg = msg[1];
>> -
>> - origin = FIELD_GET(GUC_HXG_MSG_0_ORIGIN, hxg);
>> + origin = FIELD_GET(GUC_HXG_MSG_0_ORIGIN, hxg[0]);
>> if (unlikely(origin != GUC_HXG_ORIGIN_GUC)) {
>> drm_err(&xe->drm,
>> "G2H channel broken on read, origin=%d, reset required\n",
>> @@ -901,7 +908,7 @@ static int parse_g2h_msg(struct xe_guc_ct *ct, u32 *msg, u32 len)
>> return -EPROTO;
>> }
>>
>> - type = FIELD_GET(GUC_HXG_MSG_0_TYPE, hxg);
>> + type = FIELD_GET(GUC_HXG_MSG_0_TYPE, hxg[0]);
>> switch (type) {
>> case GUC_HXG_TYPE_EVENT:
>> ret = parse_g2h_event(ct, msg, len);
>> @@ -927,14 +934,19 @@ static int process_g2h_msg(struct xe_guc_ct *ct, u32 *msg, u32 len)
>> {
>> struct xe_device *xe = ct_to_xe(ct);
>> struct xe_guc *guc = ct_to_guc(ct);
>> - u32 action = FIELD_GET(GUC_HXG_EVENT_MSG_0_ACTION, msg[1]);
>> - u32 *payload = msg + GUC_CTB_HXG_MSG_MIN_LEN;
>> - u32 adj_len = len - GUC_CTB_HXG_MSG_MIN_LEN;
>> + u32 hxg_len = msg_len_to_hxg(len);
>> + u32 *hxg = msg_to_hxg(msg);
>> + u32 action, adj_len;
>> + u32 *payload;
>> int ret = 0;
>>
>> - if (FIELD_GET(GUC_HXG_MSG_0_TYPE, msg[1]) != GUC_HXG_TYPE_EVENT)
>> + if (FIELD_GET(GUC_HXG_MSG_0_TYPE, hxg[0]) != GUC_HXG_TYPE_EVENT)
>> return 0;
>>
>> + action = FIELD_GET(GUC_HXG_EVENT_MSG_0_ACTION, hxg[0]);
>> + payload = hxg + GUC_HXG_EVENT_MSG_MIN_LEN;
>> + adj_len = hxg_len - GUC_HXG_EVENT_MSG_MIN_LEN;
>> +
>> switch (action) {
>> case XE_GUC_ACTION_SCHED_CONTEXT_MODE_DONE:
>> ret = xe_guc_sched_done_handler(guc, payload, adj_len);
>> @@ -995,6 +1007,7 @@ static int g2h_read(struct xe_guc_ct *ct, u32 *msg, bool fast_path)
>> u32 tail, head, len;
>> s32 avail;
>> u32 action;
>> + u32 *hxg;
>>
>> lockdep_assert_held(&ct->fast_lock);
>>
>> @@ -1045,10 +1058,11 @@ static int g2h_read(struct xe_guc_ct *ct, u32 *msg, bool fast_path)
>> avail * sizeof(u32));
>> }
>>
>> - action = FIELD_GET(GUC_HXG_EVENT_MSG_0_ACTION, msg[1]);
>> + hxg = msg_to_hxg(msg);
>> + action = FIELD_GET(GUC_HXG_EVENT_MSG_0_ACTION, hxg[0]);
>>
>> if (fast_path) {
>> - if (FIELD_GET(GUC_HXG_MSG_0_TYPE, msg[1]) != GUC_HXG_TYPE_EVENT)
>> + if (FIELD_GET(GUC_HXG_MSG_0_TYPE, hxg[0]) != GUC_HXG_TYPE_EVENT)
>> return 0;
>>
>> switch (action) {
>> @@ -1074,9 +1088,11 @@ static void g2h_fast_path(struct xe_guc_ct *ct, u32 *msg, u32 len)
>> {
>> struct xe_device *xe = ct_to_xe(ct);
>> struct xe_guc *guc = ct_to_guc(ct);
>> - u32 action = FIELD_GET(GUC_HXG_EVENT_MSG_0_ACTION, msg[1]);
>> - u32 *payload = msg + GUC_CTB_HXG_MSG_MIN_LEN;
>> - u32 adj_len = len - GUC_CTB_HXG_MSG_MIN_LEN;
>> + u32 hxg_len = msg_len_to_hxg(len);
>> + u32 *hxg = msg_to_hxg(msg);
>> + u32 action = FIELD_GET(GUC_HXG_EVENT_MSG_0_ACTION, hxg[0]);
>> + u32 *payload = hxg + GUC_HXG_MSG_MIN_LEN;
>> + u32 adj_len = hxg_len - GUC_HXG_MSG_MIN_LEN;
>> int ret = 0;
>>
>> switch (action) {
>>
>> base-commit: bb9e8031d2feb59becdea41e54e62f1bc47f3ef9
>> --
>> 2.25.1
>>
More information about the Intel-xe
mailing list