Re: [PATCH v7 6/9] drm/xe/sysctrl: Replace FIELD_GET() with REG_FIELD_GET()

[Michal Wajdeczko <michal.wajdeczko@intel.com>]

On 9/7/2026 2:07 PM, Mallesh, Koujalagi wrote:
> 
> On 07-09-2026 05:17 pm, Michal Wajdeczko wrote:
>>
>> On 9/7/2026 12:53 PM, Mallesh Koujalagi wrote:
>>> Use REG_FIELD_GET() when extracting fields from SysCtrl and
>> nit:
>>
>> Use REG_FIELD_GET()and REG_FIELD_PREP() when working with fields
> Sure!
>>
>>> application headers. This better reflects that the masks
>>> represent register-style bitfields and improves consistency.
>>>
>>> Signed-off-by: Mallesh Koujalagi <mallesh.koujalagi@intel.com>
>>> ---
>>> v7:
>>> - Update FIELD_PREP to REG_FIELD_PREP. (Sashiko)
>>> - Update FIELD_GET to REG_FIELD_GET.
>>> ---
>>>   drivers/gpu/drm/xe/xe_sysctrl_mailbox.c | 20 ++++++++++----------
>>>   drivers/gpu/drm/xe/xe_sysctrl_mailbox.h |  6 +++---
>>>   2 files changed, 13 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/xe/xe_sysctrl_mailbox.c b/drivers/gpu/drm/xe/xe_sysctrl_mailbox.c
>>> index 50bbdab072de..cdd929ebed4a 100644
>>> --- a/drivers/gpu/drm/xe/xe_sysctrl_mailbox.c
>>> +++ b/drivers/gpu/drm/xe/xe_sysctrl_mailbox.c
>>> @@ -24,16 +24,16 @@ struct xe_sysctrl_mailbox_msg_hdr {
>>>   } __packed;
>>>     #define XE_SYSCTRL_HDR_GROUP_ID(hdr) \
>>> -    FIELD_GET(SYSCTRL_HDR_GROUP_ID_MASK, le32_to_cpu((hdr)->data))
>>> +    REG_FIELD_GET(SYSCTRL_HDR_GROUP_ID_MASK, le32_to_cpu((hdr)->data))
>>>     #define XE_SYSCTRL_HDR_COMMAND(hdr) \
>>> -    FIELD_GET(SYSCTRL_HDR_COMMAND_MASK, le32_to_cpu((hdr)->data))
>>> +    REG_FIELD_GET(SYSCTRL_HDR_COMMAND_MASK, le32_to_cpu((hdr)->data))
>>>     #define XE_SYSCTRL_HDR_IS_RESPONSE(hdr) \
>>> -    FIELD_GET(SYSCTRL_HDR_IS_RESPONSE, le32_to_cpu((hdr)->data))
>>> +    REG_FIELD_GET(SYSCTRL_HDR_IS_RESPONSE, le32_to_cpu((hdr)->data))
>>>     #define XE_SYSCTRL_HDR_RESULT(hdr) \
>>> -    FIELD_GET(SYSCTRL_HDR_RESULT_MASK, le32_to_cpu((hdr)->data))
>>> +    REG_FIELD_GET(SYSCTRL_HDR_RESULT_MASK, le32_to_cpu((hdr)->data))
>>>     static int sysctrl_wait_bit_clear(struct xe_sysctrl *sc, u32 bit_mask,
>>>                     unsigned int timeout_ms)
>>> @@ -112,8 +112,8 @@ static int sysctrl_prepare_command(struct xe_device *xe,
>>>           return -ENOMEM;
>>>         hdr = (struct xe_sysctrl_mailbox_msg_hdr *)buffer;
>>> -    hdr->data = cpu_to_le32(FIELD_PREP(SYSCTRL_HDR_GROUP_ID_MASK, group_id) |
>>> -                     FIELD_PREP(SYSCTRL_HDR_COMMAND_MASK, command));
>>> +    hdr->data = cpu_to_le32(REG_FIELD_PREP(SYSCTRL_HDR_GROUP_ID_MASK, group_id) |
>>> +                REG_FIELD_PREP(SYSCTRL_HDR_COMMAND_MASK, command));
>>>         if (data_in && data_in_len)
>>>           memcpy(buffer + sizeof(*hdr), data_in, data_in_len);
>>> @@ -186,8 +186,8 @@ static int sysctrl_process_frame(struct xe_sysctrl *sc, void *out,
>>>       }
>>>         ctrl_reg = xe_mmio_read32(sc->mmio, SYSCTRL_MB_CTRL);
>>> -    total_frames = FIELD_GET(SYSCTRL_FRAME_TOTAL_MASK, ctrl_reg);
>>> -    curr_frame = FIELD_GET(SYSCTRL_FRAME_CURRENT_MASK, ctrl_reg);
>>> +    total_frames = REG_FIELD_GET(SYSCTRL_FRAME_TOTAL_MASK, ctrl_reg);
>>> +    curr_frame = REG_FIELD_GET(SYSCTRL_FRAME_CURRENT_MASK, ctrl_reg);
>>>         sysctrl_read_frame(sc, out, frame_size);
>>>       sysctrl_clear_response(sc);
>>> @@ -295,8 +295,8 @@ void xe_sysctrl_create_command(struct xe_sysctrl_mailbox_command *command, u8 gr
>>>   {
>>>       struct xe_sysctrl_app_msg_hdr header = {0};
>>>   -    header.data = FIELD_PREP(APP_HDR_GROUP_ID_MASK, group_id) |
>>> -              FIELD_PREP(APP_HDR_COMMAND_MASK, cmd_id);
>>> +    header.data = REG_FIELD_PREP(APP_HDR_GROUP_ID_MASK, group_id) |
>>> +              REG_FIELD_PREP(APP_HDR_COMMAND_MASK, cmd_id);
>> hmm, shouldn't we use le32 conversion here?
>> other sysctrl cmd/hdr parsing/prep seems to use it
> 
> le32 conversion is not required.
> 
> le32 already removed in the code, find the change: https://patchwork.freedesktop.org/patch/717045/?series=164526&rev=1

hmm, but was this fix correct? maybe the problem is elsewhere?

today we have:

struct xe_sysctrl_mailbox_msg_hdr {
	__le32 data;
} __packed;

and:

struct xe_sysctrl_app_msg_hdr {
	u32 data;
} __packed;

both used during sysctrl communication but only one is using __le32

IMO either both or none should use __le32,
or I'm missing something?

> 
>>
>>>         command->header = header;
>>>       command->data_in = request;
>>> diff --git a/drivers/gpu/drm/xe/xe_sysctrl_mailbox.h b/drivers/gpu/drm/xe/xe_sysctrl_mailbox.h
>>> index f0d5e3d7f5e3..5a3dd14e3a11 100644
>>> --- a/drivers/gpu/drm/xe/xe_sysctrl_mailbox.h
>>> +++ b/drivers/gpu/drm/xe/xe_sysctrl_mailbox.h
>>> @@ -15,13 +15,13 @@ struct xe_sysctrl;
>>>   struct xe_sysctrl_mailbox_command;
>>>     #define XE_SYSCTRL_APP_HDR_GROUP_ID(hdr) \
>>> -    FIELD_GET(APP_HDR_GROUP_ID_MASK, (hdr)->data)
>>> +    REG_FIELD_GET(APP_HDR_GROUP_ID_MASK, (hdr)->data)
>>>     #define XE_SYSCTRL_APP_HDR_COMMAND(hdr) \
>>> -    FIELD_GET(APP_HDR_COMMAND_MASK, (hdr)->data)
>>> +    REG_FIELD_GET(APP_HDR_COMMAND_MASK, (hdr)->data)
>>>     #define XE_SYSCTRL_APP_HDR_VERSION(hdr) \
>>> -    FIELD_GET(APP_HDR_VERSION_MASK, (hdr)->data)
>>> +    REG_FIELD_GET(APP_HDR_VERSION_MASK, (hdr)->data)
>> ditto
> ditto
>>>     void xe_sysctrl_create_command(struct xe_sysctrl_mailbox_command *command, u8 group_id, u8 cmd_id,
>>>                      void *request, size_t request_len, void *response,