Re: [PATCH] amdgpu: add context creation flags in CS IOCTL

["Sharma, Shashank" <shashank.sharma@xxxxxxx>]

On 8/8/2022 12:59 PM, Christian König wrote:
> Am 02.08.22 um 15:55 schrieb Shashank Sharma:
> > This patch adds:
> > - A new input parameter "flags" in the amdgpu_ctx_create2 call.
> > - Some new flags defining workload type hints.
> > - Some change in the caller function of amdgpu_ctx_create2, to
> >    accomodate this new parameter.
> >
> > The idea is to pass the workload hints while context creation,
>
> Bad idea.
>
> Please take AMDGPU_CTX_OP_SET_STABLE_PSTATE and 
> AMDGPU_CTX_OP_GET_STABLE_PSTATE as blueprint for this and don't add 
> extra flags to the context creation.
>
> Regards,
> Christian.

Hey Christian,
Noted, let me have a look at AMDGPU_CTX_OP_GET/SET_STABLE_PSTATE 
implementation.

- Shashank

> >   so
> > that kernel GPU scheduler can pass this information to GPU FW, which in
> > turn can adjust the GPU characterstics as per the workload type.
> >
> > Signed-off-by: Shashank Sharma <shashank.sharma@xxxxxxx>
> > Cc: Alex Deucher <alexander.deucher@xxxxxxx>
> > Cc: Marek Olsak <marek.olsak@xxxxxxx>
> > Cc: Christian Koenig <christian.koenig@xxxxxxx>
> > Cc: Amarnath Somalapuram <amaranath.somalapuram@xxxxxxx>
> > ---
> >   amdgpu/amdgpu.h          |  2 ++
> >   amdgpu/amdgpu_cs.c       |  5 ++++-
> >   include/drm/amdgpu_drm.h | 10 +++++++++-
> >   3 files changed, 15 insertions(+), 2 deletions(-)
> >
> > diff --git a/amdgpu/amdgpu.h b/amdgpu/amdgpu.h
> > index b118dd48..1ebb46e6 100644
> > --- a/amdgpu/amdgpu.h
> > +++ b/amdgpu/amdgpu.h
> > @@ -874,6 +874,7 @@ int amdgpu_bo_list_update(amdgpu_bo_list_handle 
> > handle,
> >    *
> >    * \param   dev      - \c [in] Device handle. See 
> > #amdgpu_device_initialize()
> >    * \param   priority - \c [in] Context creation flags. See 
> > AMDGPU_CTX_PRIORITY_*
> > + * \param   flags    - \c [in] Context flags. See AMDGPU_CTX_FLAGS_*
> >    * \param   context  - \c [out] GPU Context handle
> >    *
> >    * \return   0 on success\n
> > @@ -884,6 +885,7 @@ int amdgpu_bo_list_update(amdgpu_bo_list_handle 
> > handle,
> >   */
> >   int amdgpu_cs_ctx_create2(amdgpu_device_handle dev,
> >                uint32_t priority,
> > +             uint32_t flags,
> >                amdgpu_context_handle *context);
> >   /**
> >    * Create GPU execution Context
> > diff --git a/amdgpu/amdgpu_cs.c b/amdgpu/amdgpu_cs.c
> > index fad484bf..d4723ea5 100644
> > --- a/amdgpu/amdgpu_cs.c
> > +++ b/amdgpu/amdgpu_cs.c
> > @@ -44,12 +44,14 @@ static int 
> > amdgpu_cs_reset_sem(amdgpu_semaphore_handle sem);
> >    *
> >    * \param   dev      - \c [in] Device handle. See 
> > #amdgpu_device_initialize()
> >    * \param   priority - \c [in] Context creation flags. See 
> > AMDGPU_CTX_PRIORITY_*
> > + * \param   flags    - \c [in] Context flags. See AMDGPU_CTX_FLAGS_*
> >    * \param   context  - \c [out] GPU Context handle
> >    *
> >    * \return  0 on success otherwise POSIX Error code
> >   */
> >   drm_public int amdgpu_cs_ctx_create2(amdgpu_device_handle dev,
> >                        uint32_t priority,
> > +                     uint32_t flags,
> >                        amdgpu_context_handle *context)
> >   {
> >       struct amdgpu_context *gpu_context;
> > @@ -74,6 +76,7 @@ drm_public int 
> > amdgpu_cs_ctx_create2(amdgpu_device_handle dev,
> >       memset(&args, 0, sizeof(args));
> >       args.in.op = AMDGPU_CTX_OP_ALLOC_CTX;
> >       args.in.priority = priority;
> > +    args.in.flags = flags;
> >       r = drmCommandWriteRead(dev->fd, DRM_AMDGPU_CTX, &args, 
> > sizeof(args));
> >       if (r)
> > @@ -97,7 +100,7 @@ error:
> >   drm_public int amdgpu_cs_ctx_create(amdgpu_device_handle dev,
> >                       amdgpu_context_handle *context)
> >   {
> > -    return amdgpu_cs_ctx_create2(dev, AMDGPU_CTX_PRIORITY_NORMAL, 
> > context);
> > +    return amdgpu_cs_ctx_create2(dev, AMDGPU_CTX_PRIORITY_NORMAL, 0, 
> > context);
> >   }
> >   /**
> > diff --git a/include/drm/amdgpu_drm.h b/include/drm/amdgpu_drm.h
> > index 0cbd1540..d9fb1f20 100644
> > --- a/include/drm/amdgpu_drm.h
> > +++ b/include/drm/amdgpu_drm.h
> > @@ -238,10 +238,18 @@ union drm_amdgpu_bo_list {
> >   #define AMDGPU_CTX_PRIORITY_HIGH        512
> >   #define AMDGPU_CTX_PRIORITY_VERY_HIGH   1023
> > +/* GPU context workload hint bitmask */
> > +#define AMDGPU_CTX_FLAGS_WORKLOAD_HINT_MASK    0xFF
> > +#define AMDGPU_CTX_FLAGS_WORKLOAD_HINT_NONE    0
> > +#define AMDGPU_CTX_FLAGS_WORKLOAD_HINT_3D      (1 << 1)
> > +#define AMDGPU_CTX_FLAGS_WORKLOAD_HINT_VIDEO   (1 << 2)
> > +#define AMDGPU_CTX_FLAGS_WORKLOAD_HINT_VR      (1 << 3)
> > +#define AMDGPU_CTX_FLAGS_WORKLOAD_HINT_COMPUTE (1 << 4)
> > +
> >   struct drm_amdgpu_ctx_in {
> >       /** AMDGPU_CTX_OP_* */
> >       __u32    op;
> > -    /** For future use, no flags defined so far */
> > +    /** AMDGPU_CTX_FLAGS_* */
> >       __u32    flags;
> >       __u32    ctx_id;
> >       /** AMDGPU_CTX_PRIORITY_* */