Re: [RFC PATCH 1/4] drm/amdgpu: set compute queue priority at mqd_init

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



[AMD Official Use Only - Internal Distribution Only]


Yes I missed gfx_v10_0.c. I will resend a updated one tomorrow. 

Regards, 
Nirmoy 
Get Outlook for Android

From: Alex Deucher <alexdeucher@xxxxxxxxx>
Sent: Thursday, February 27, 2020 10:50:35 PM
To: Nirmoy Das <nirmoy.aiemd@xxxxxxxxx>
Cc: amd-gfx list <amd-gfx@xxxxxxxxxxxxxxxxxxxxx>; Deucher, Alexander <Alexander.Deucher@xxxxxxx>; Huang, Ray <Ray.Huang@xxxxxxx>; Das, Nirmoy <Nirmoy.Das@xxxxxxx>; Koenig, Christian <Christian.Koenig@xxxxxxx>
Subject: Re: [RFC PATCH 1/4] drm/amdgpu: set compute queue priority at mqd_init
 
On Thu, Feb 27, 2020 at 4:48 PM Alex Deucher <alexdeucher@xxxxxxxxx> wrote:
>
> On Thu, Feb 27, 2020 at 4:37 PM Nirmoy Das <nirmoy.aiemd@xxxxxxxxx> wrote:
> >
> > We were changing compute ring priority while rings were being used
> > before every job submission which is not recommended. This patch
> > sets compute queue priority at mqd initialization for gfx8 and gfx9.
> >
> > Policy: Enable high priority compute queues only if gpu has >1 MEC, if
> > so PIPE0 and PIPE1 will be in high priority.
>
> I think we still want high priority queues on even when there is only
> one MEC.  It might work better on multi-MEC chips, but it should still
> work on single MEC chips.
>
> >
> > high/normal priority compute sched list are generated from set of high/normal
> > priority compute queues. At context creation, entity of compute queue
> > get a sched list from high or normal priority depending on ctx->priority
> >
> > Signed-off-by: Nirmoy Das <nirmoy.das@xxxxxxx>
> > ---
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c   |  4 ----
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c  | 19 +++++++++++++++----
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c  | 14 ++++++++++++++
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h  | 12 ++++++++++++
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_job.c  |  6 ------
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h |  1 +
> >  drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c    | 23 ++++++++++++++++++++---
> >  drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c    | 20 ++++++++++++++++++++

Also, can you add a patch for gfx_v10_0.c?

Alex

> >  8 files changed, 82 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> > index f397ff97b4e4..8304d0c87899 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> > @@ -1205,7 +1205,6 @@ static int amdgpu_cs_submit(struct amdgpu_cs_parser *p,
> >         struct amdgpu_fpriv *fpriv = p->filp->driver_priv;
> >         struct drm_sched_entity *entity = p->entity;
> >         enum drm_sched_priority priority;
> > -       struct amdgpu_ring *ring;
> >         struct amdgpu_bo_list_entry *e;
> >         struct amdgpu_job *job;
> >         uint64_t seq;
> > @@ -1258,9 +1257,6 @@ static int amdgpu_cs_submit(struct amdgpu_cs_parser *p,
> >         priority = job->base.s_priority;
> >         drm_sched_entity_push_job(&job->base, entity);
> >
> > -       ring = to_amdgpu_ring(entity->rq->sched);
> > -       amdgpu_ring_priority_get(ring, priority);
> > -
> >         amdgpu_vm_move_to_lru_tail(p->adev, &fpriv->vm);
> >
> >         ttm_eu_fence_buffer_objects(&p->ticket, &p->validated, p->fence);
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
> > index 94a6c42f29ea..a1742b1d4f9c 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
> > @@ -85,8 +85,14 @@ static int amdgpu_ctx_init_entity(struct amdgpu_ctx *ctx, const u32 hw_ip, const
> >                         num_scheds = 1;
> >                         break;
> >                 case AMDGPU_HW_IP_COMPUTE:
> > -                       scheds = adev->gfx.compute_sched;
> > -                       num_scheds = adev->gfx.num_compute_sched;
> > +                       if (priority > DRM_SCHED_PRIORITY_NORMAL &&
> > +                           adev->gfx.num_compute_sched_high) {
> > +                               scheds = adev->gfx.compute_sched_high;
> > +                               num_scheds = adev->gfx.num_compute_sched_high;
> > +                       } else {
> > +                               scheds = adev->gfx.compute_sched;
> > +                               num_scheds = adev->gfx.num_compute_sched;
> > +                       }
> >                         break;
> >                 case AMDGPU_HW_IP_DMA:
> >                         scheds = adev->sdma.sdma_sched;
> > @@ -638,8 +644,13 @@ void amdgpu_ctx_init_sched(struct amdgpu_device *adev)
> >         }
> >
> >         for (i = 0; i < adev->gfx.num_compute_rings; i++) {
> > -               adev->gfx.compute_sched[i] = &adev->gfx.compute_ring[i].sched;
> > -               adev->gfx.num_compute_sched++;
> > +               if (!adev->gfx.compute_ring[i].high_priority) {
> > +                       adev->gfx.compute_sched[adev->gfx.num_compute_sched++] =
> > +                               &adev->gfx.compute_ring[i].sched;
> > +               } else {
> > +                       adev->gfx.compute_sched_high[adev->gfx.num_compute_sched_high++] =
> > +                               &adev->gfx.compute_ring[i].sched;
> > +               }
> >         }
> >
> >         for (i = 0; i < adev->sdma.num_instances; i++) {
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
> > index 7403588684b3..bdea5d44edf4 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
> > @@ -192,6 +192,20 @@ static bool amdgpu_gfx_is_multipipe_capable(struct amdgpu_device *adev)
> >         return adev->gfx.mec.num_mec > 1;
> >  }
> >
> > +bool amdgpu_gfx_is_high_priority_compute_queue(struct amdgpu_device *adev,
> > +                                              int queue)
> > +{
> > +       bool multipipe_policy = amdgpu_gfx_is_multipipe_capable(adev);
> > +
> > +       /* only enable high priority queue if more than 1 MEC.
> > +        * When multipipe_policy is true amdgpu gets queue 0,1 from each pipe of
> > +        * 1st MEC. Policy: make queue 0 of each pipe as high priority compute queue */
> > +       if (multipipe_policy && queue == 0)
> > +               return true;
> > +
> > +       return false;
> > +}
> > +
> >  void amdgpu_gfx_compute_queue_acquire(struct amdgpu_device *adev)
> >  {
> >         int i, queue, pipe, mec;
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h
> > index 37ba05b63b2a..13ee0ea66c6f 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h
> > @@ -41,6 +41,14 @@
> >  #define AMDGPU_MAX_GFX_QUEUES KGD_MAX_QUEUES
> >  #define AMDGPU_MAX_COMPUTE_QUEUES KGD_MAX_QUEUES
> >
> > +#define AMDGPU_GFX_PIPE_PRIO_LOW    0
> > +#define AMDGPU_GFX_PIPE_PRIO_NORMAL 1
> > +#define AMDGPU_GFX_PIPE_PRIO_HIGH   2
> > +
> > +#define AMDGPU_GFX_QUEUE_PRIORITY_MINIMUM  0
> > +#define AMDGPU_GFX_QUEUE_PRIORITY_MAXIMUM  15
> > +
> > +
> >  struct amdgpu_mec {
> >         struct amdgpu_bo        *hpd_eop_obj;
> >         u64                     hpd_eop_gpu_addr;
> > @@ -281,7 +289,9 @@ struct amdgpu_gfx {
> >         unsigned                        num_gfx_rings;
> >         struct amdgpu_ring              compute_ring[AMDGPU_MAX_COMPUTE_RINGS];
> >         struct drm_gpu_scheduler        *compute_sched[AMDGPU_MAX_COMPUTE_RINGS];
> > +       struct drm_gpu_scheduler        *compute_sched_high[AMDGPU_MAX_COMPUTE_RINGS];
> >         uint32_t                        num_compute_sched;
> > +       uint32_t                        num_compute_sched_high;
> >         unsigned                        num_compute_rings;
> >         struct amdgpu_irq_src           eop_irq;
> >         struct amdgpu_irq_src           priv_reg_irq;
> > @@ -363,6 +373,8 @@ void amdgpu_gfx_bit_to_mec_queue(struct amdgpu_device *adev, int bit,
> >                                  int *mec, int *pipe, int *queue);
> >  bool amdgpu_gfx_is_mec_queue_enabled(struct amdgpu_device *adev, int mec,
> >                                      int pipe, int queue);
> > +bool amdgpu_gfx_is_high_priority_compute_queue(struct amdgpu_device *adev,
> > +                                              int queue);
> >  int amdgpu_gfx_me_queue_to_bit(struct amdgpu_device *adev, int me,
> >                                int pipe, int queue);
> >  void amdgpu_gfx_bit_to_me_queue(struct amdgpu_device *adev, int bit,
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> > index d42be880a236..4981e443a884 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> > @@ -117,12 +117,10 @@ void amdgpu_job_free_resources(struct amdgpu_job *job)
> >
> >  static void amdgpu_job_free_cb(struct drm_sched_job *s_job)
> >  {
> > -       struct amdgpu_ring *ring = to_amdgpu_ring(s_job->sched);
> >         struct amdgpu_job *job = to_amdgpu_job(s_job);
> >
> >         drm_sched_job_cleanup(s_job);
> >
> > -       amdgpu_ring_priority_put(ring, s_job->s_priority);
> >         dma_fence_put(job->fence);
> >         amdgpu_sync_free(&job->sync);
> >         amdgpu_sync_free(&job->sched_sync);
> > @@ -143,7 +141,6 @@ int amdgpu_job_submit(struct amdgpu_job *job, struct drm_sched_entity *entity,
> >                       void *owner, struct dma_fence **f)
> >  {
> >         enum drm_sched_priority priority;
> > -       struct amdgpu_ring *ring;
> >         int r;
> >
> >         if (!f)
> > @@ -158,9 +155,6 @@ int amdgpu_job_submit(struct amdgpu_job *job, struct drm_sched_entity *entity,
> >         priority = job->base.s_priority;
> >         drm_sched_entity_push_job(&job->base, entity);
> >
> > -       ring = to_amdgpu_ring(entity->rq->sched);
> > -       amdgpu_ring_priority_get(ring, priority);
> > -
> >         return 0;
> >  }
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
> > index 24caff085d00..34fcd467f18d 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
> > @@ -222,6 +222,7 @@ struct amdgpu_ring {
> >         struct mutex            priority_mutex;
> >         /* protected by priority_mutex */
> >         int                     priority;
> > +       bool                    high_priority;
> >
> >  #if defined(CONFIG_DEBUG_FS)
> >         struct dentry *ent;
> > diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
> > index e63f98b2d389..4260becd569b 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
> > @@ -4426,6 +4426,23 @@ static int gfx_v8_0_deactivate_hqd(struct amdgpu_device *adev, u32 req)
> >         return r;
> >  }
> >
> > +static void gfx_v8_0_mqd_set_priority(struct amdgpu_ring *ring, struct vi_mqd *mqd)
> > +{
> > +       struct amdgpu_device *adev = ring->adev;
> > +
> > +       if (ring->funcs->type == AMDGPU_RING_TYPE_COMPUTE) {
> > +               if (amdgpu_gfx_is_high_priority_compute_queue(adev, ring->queue)) {
> > +                       mqd->cp_hqd_pipe_priority = AMDGPU_GFX_PIPE_PRIO_HIGH;
> > +                       ring->high_priority = true;
> > +                       mqd->cp_hqd_queue_priority =
> > +                               AMDGPU_GFX_QUEUE_PRIORITY_MAXIMUM;
> > +                       mqd->cp_hqd_quantum = RREG32(mmCP_HQD_QUANTUM);
>
> I think we can move this back to mqd_init().  It needs to be set up
> for both low and high priority queues.
>
> > +               }
> > +               else
> > +                       ring->high_priority = false;
> > +       }
> > +}
> > +
> >  static int gfx_v8_0_mqd_init(struct amdgpu_ring *ring)
> >  {
> >         struct amdgpu_device *adev = ring->adev;
> > @@ -4549,9 +4566,6 @@ static int gfx_v8_0_mqd_init(struct amdgpu_ring *ring)
> >         /* defaults */
> >         mqd->cp_hqd_eop_rptr = RREG32(mmCP_HQD_EOP_RPTR);
> >         mqd->cp_hqd_eop_wptr = RREG32(mmCP_HQD_EOP_WPTR);
> > -       mqd->cp_hqd_pipe_priority = RREG32(mmCP_HQD_PIPE_PRIORITY);
> > -       mqd->cp_hqd_queue_priority = RREG32(mmCP_HQD_QUEUE_PRIORITY);
> > -       mqd->cp_hqd_quantum = RREG32(mmCP_HQD_QUANTUM);
> >         mqd->cp_hqd_ctx_save_base_addr_lo = RREG32(mmCP_HQD_CTX_SAVE_BASE_ADDR_LO);
> >         mqd->cp_hqd_ctx_save_base_addr_hi = RREG32(mmCP_HQD_CTX_SAVE_BASE_ADDR_HI);
> >         mqd->cp_hqd_cntl_stack_offset = RREG32(mmCP_HQD_CNTL_STACK_OFFSET);
> > @@ -4563,6 +4577,9 @@ static int gfx_v8_0_mqd_init(struct amdgpu_ring *ring)
> >         mqd->cp_hqd_eop_wptr_mem = RREG32(mmCP_HQD_EOP_WPTR_MEM);
> >         mqd->cp_hqd_eop_dones = RREG32(mmCP_HQD_EOP_DONES);
> >
> > +       /* set static priority for a queue/ring */
> > +       gfx_v8_0_mqd_set_priority(ring, mqd);
> > +
> >         /* map_queues packet doesn't need activate the queue,
> >          * so only kiq need set this field.
> >          */
> > diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
> > index 4135e4126e82..85a54849abd1 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
> > @@ -3310,6 +3310,23 @@ static void gfx_v9_0_kiq_setting(struct amdgpu_ring *ring)
> >         WREG32_SOC15_RLC(GC, 0, mmRLC_CP_SCHEDULERS, tmp);
> >  }
> >
> > +static void gfx_v9_0_mqd_set_priority(struct amdgpu_ring *ring, struct v9_mqd *mqd)
> > +{
> > +       struct amdgpu_device *adev = ring->adev;
> > +
> > +       if (ring->funcs->type == AMDGPU_RING_TYPE_COMPUTE) {
> > +               if (amdgpu_gfx_is_high_priority_compute_queue(adev, ring->queue)) {
> > +                       mqd->cp_hqd_pipe_priority = AMDGPU_GFX_PIPE_PRIO_HIGH;
> > +                       ring->high_priority = true;
> > +                       mqd->cp_hqd_queue_priority =
> > +                               AMDGPU_GFX_QUEUE_PRIORITY_MAXIMUM;
> > +                       mqd->cp_hqd_quantum = RREG32(mmCP_HQD_QUANTUM);
>
> Same comment here.
>
> Alex
>
> > +               }
> > +               else
> > +                       ring->high_priority = false;
> > +       }
> > +}
> > +
> >  static int gfx_v9_0_mqd_init(struct amdgpu_ring *ring)
> >  {
> >         struct amdgpu_device *adev = ring->adev;
> > @@ -3446,6 +3463,9 @@ static int gfx_v9_0_mqd_init(struct amdgpu_ring *ring)
> >         tmp = REG_SET_FIELD(tmp, CP_HQD_IB_CONTROL, MIN_IB_AVAIL_SIZE, 3);
> >         mqd->cp_hqd_ib_control = tmp;
> >
> > +       /* set static priority for a queue/ring */
> > +       gfx_v9_0_mqd_set_priority(ring, mqd);
> > +
> >         /* map_queues packet doesn't need activate the queue,
> >          * so only kiq need set this field.
> >          */
> > --
> > 2.25.0
> >
> > _______________________________________________
> > amd-gfx mailing list
> > amd-gfx@xxxxxxxxxxxxxxxxxxxxx
> > https://nam11.safelinks.protection.outlook.com/?url="">
_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux