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

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

On 9/13/2023 9:33 AM, Sundaresan, Sujaritha wrote:
>
> 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
>
Hi Rodrigo,

As I am making the final fixes, looks like it would be better to have 
separate new read/write functions.

With changing the existing read/write functions, we are having to change 
the pcode_mailbox_rw function.

That is causing issues with other existing functions. Shall I keep the 
new functions defined separately ?

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
>>>>>>