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

[Rodrigo Vivi]

On Mon, Sep 25, 2023 at 01:43:07PM +0530, Sundaresan, Sujaritha wrote:
> 
> 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 at 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 ?

I'm really sorry for having missed this here.
I still believe that there are ways of combining them, even if we have
to change the internal rw function. But if that gets too complicated or too
ugly fine, let's have something alternative, but with a better naming

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