Re: [Intel-xe] [PATCH 1/3] drm/xe: Add a couple of pcode helpers

["Sundaresan, Sujaritha" <sujaritha.sundaresan@intel.com>]

On 9/12/2023 8:03 PM, Rodrigo Vivi wrote:
> On Tue, Sep 12, 2023 at 10:38:50AM +0530, Sundaresan, Sujaritha wrote:
>> On 9/3/2023 7:00 PM, Sundaresan, Sujaritha wrote:
>>> On 9/2/2023 2:04 AM, Rodrigo Vivi wrote:
>>>> On Fri, Sep 01, 2023 at 05:45:43PM +0530, Sujaritha Sundaresan wrote:
>>>>> Some pcode commands take additional sub-commands and parameters. Add a
>>>>> couple of helpers to help formatting these commands to improve code
>>>>> readability.
>>>>>
>>>>> Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
>>>>> ---
>>>>>    drivers/gpu/drm/xe/xe_pcode.c | 28 ++++++++++++++++++++++++++++
>>>>>    drivers/gpu/drm/xe/xe_pcode.h |  3 +++
>>>>>    2 files changed, 31 insertions(+)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/xe/xe_pcode.c
>>>>> b/drivers/gpu/drm/xe/xe_pcode.c
>>>>> index 7f1bf2297f51..e45169f47500 100644
>>>>> --- a/drivers/gpu/drm/xe/xe_pcode.c
>>>>> +++ b/drivers/gpu/drm/xe/xe_pcode.c
>>>>> @@ -104,6 +104,34 @@ int xe_pcode_read(struct xe_gt *gt, u32
>>>>> mbox, u32 *val, u32 *val1)
>>>>>        return err;
>>>>>    }
>>>> a doc would be required...
>>>>
>>>>> +int xe_pcode_read_p(struct xe_gt *gt, u32 mbcmd, u32 p1, u32
>>>>> p2, u32 *val)
>>>> a better name would be nice....
>>>>
>>>>> +{
>>>>> +    u32 mbox;
>>>>> +    int err;
>>>>> +
>>>>> +    mbox = REG_FIELD_PREP(PCODE_MB_COMMAND, mbcmd)
>>>>> +        | REG_FIELD_PREP(PCODE_MB_PARAM1, p1)
>>>>> +        | REG_FIELD_PREP(PCODE_MB_PARAM2, p2);
>>>>> +
>>>>> +    err = xe_pcode_read(gt, mbox, val, NULL);
>>>> but why not simply modifying the existent one to accept 2 params?
>>>>
>>>> int xe_pcode_read(struct xe_gt *gt, u32 mbox_param1, u32 mbox_param2,
>>>>                 u32 *val, u32 *val1)
>>>>
>>>> and the equivalent write...
>>>>
>>>> oh, and while doing that, could you please add the missing documentation
>>>> to these 2 functions?
>>>>
>>>> Thanks,
>>>> Rodrigo.
>>> Sure that would work. Will add the docs as well.
>>>
>>> Thanks,
>>>
>>> Suja
>> Hi Rodrigo,
>>
>> Another question,
>>
>> I can change the existing pcode_read function, but would it be better to
>> have a separate new write equivalent ?
> I wonder if we should do s/xe_pcode_write_timeout(/xe_pcode_write(
>
> where timeout is still an argument but it can be null.
> And then we merge with your options here and make a single write fn.

I'll see if this works out.

Thanks,

Suja

>
>>>>> +
>>>>> +    return err;
>>>>> +}
>>>>> +
>>>>> +int xe_pcode_write_p(struct xe_gt *gt, u32 mbcmd, u32 p1, u32
>>>>> p2, u32 val)
>>>>> +{
>>>>> +    u32 mbox;
>>>>> +    int err;
>>>>> +
>>>>> +    mbox = REG_FIELD_PREP(PCODE_MB_COMMAND, mbcmd)
>>>>> +        | REG_FIELD_PREP(PCODE_MB_PARAM1, p1)
>>>>> +        | REG_FIELD_PREP(PCODE_MB_PARAM2, p2);
>>>>> +
>>>>> +    err = xe_pcode_write(gt, mbox, val);
>>>>> +
>>>>> +    return err;
>>>>> +}
>>>>> +
>>>>>    static int xe_pcode_try_request(struct xe_gt *gt, u32 mbox,
>>>>>                    u32 request, u32 reply_mask, u32 reply,
>>>>>                    u32 *status, bool atomic, int timeout_us)
>>>>> diff --git a/drivers/gpu/drm/xe/xe_pcode.h
>>>>> b/drivers/gpu/drm/xe/xe_pcode.h
>>>>> index 3b4aa8c1a3ba..8d4103afd7e0 100644
>>>>> --- a/drivers/gpu/drm/xe/xe_pcode.h
>>>>> +++ b/drivers/gpu/drm/xe/xe_pcode.h
>>>>> @@ -19,6 +19,9 @@ int xe_pcode_write_timeout(struct xe_gt *gt,
>>>>> u32 mbox, u32 val,
>>>>>    #define xe_pcode_write(gt, mbox, val) \
>>>>>        xe_pcode_write_timeout(gt, mbox, val, 1)
>>>>>    +int xe_pcode_read_p(struct xe_gt *gt, u32 mbcmd, u32 p1, u32
>>>>> p2, u32 *val);
>>>>> +int xe_pcode_write_p(struct xe_gt *gt, u32 mbcmd, u32 p1, u32
>>>>> p2, u32 val);
>>>>> +
>>>>>    int xe_pcode_request(struct xe_gt *gt, u32 mbox, u32 request,
>>>>>                 u32 reply_mask, u32 reply, int timeout_ms);
>>>>>    --
>>>>> 2.25.1
>>>>>