Re: [PATCH] drm/amdgpu/vcn:enable priority queues for encoder

["Sahu, Satyajit" <satyajit.sahu@xxxxxxx>]

On 8/10/2021 6:12 PM, Das, Nirmoy wrote:
> On 8/10/2021 11:38 AM, Sahu, Satyajit wrote:
> > On 8/10/2021 2:01 PM, Das, Nirmoy wrote:
> > > On 8/10/2021 9:09 AM, Satyajit Sahu wrote:
> > > > VCN and VCE support multiple queues with different priority.
> > > > Use differnt encoder queue based on the priority set by UMD.
> > > >
> > > > Signed-off-by: Satyajit Sahu <satyajit.sahu@xxxxxxx>
> > > > ---
> > > >   drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c   | 35 
> > > > +++++++++++++++++++++--
> > > >   drivers/gpu/drm/amd/amdgpu/amdgpu_sched.c |  2 +-
> > > >   drivers/gpu/drm/amd/amdgpu/amdgpu_vce.c   | 14 +++++++++
> > > >   drivers/gpu/drm/amd/amdgpu/amdgpu_vce.h   |  9 ++++++
> > > >   drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c   | 14 +++++++++
> > > >   drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h   |  8 ++++++
> > > >   drivers/gpu/drm/amd/amdgpu/vce_v2_0.c     |  4 ++-
> > > >   drivers/gpu/drm/amd/amdgpu/vce_v3_0.c     |  4 ++-
> > > >   drivers/gpu/drm/amd/amdgpu/vce_v4_0.c     |  4 ++-
> > > >   drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c     |  4 ++-
> > > >   drivers/gpu/drm/amd/amdgpu/vcn_v2_0.c     |  4 ++-
> > > >   drivers/gpu/drm/amd/amdgpu/vcn_v2_5.c     |  4 ++-
> > > >   drivers/gpu/drm/amd/amdgpu/vcn_v3_0.c     |  5 ++--
> > > >   13 files changed, 99 insertions(+), 12 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c 
> > > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
> > > > index e7a010b7ca1f..b0ae2b45c7c3 100644
> > > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
> > > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
> > > > @@ -73,15 +73,44 @@ static enum gfx_pipe_priority 
> > > > amdgpu_ctx_sched_prio_to_compute_prio(enum drm_sch
> > > >       }
> > > >   }
> > > >   +static enum gfx_pipe_priority 
> > > > amdgpu_ctx_sched_prio_to_vcn_prio(enum drm_sched_priority prio)
> > > > +{
> > > > +    switch (prio) {
> > > > +    case DRM_SCHED_PRIORITY_HIGH:
> > > > +        return AMDGPU_VCN_ENC_PRIO_HIGH;
> > > > +    case DRM_SCHED_PRIORITY_KERNEL:
> > > > +        return AMDGPU_VCN_ENC_PRIO_VERY_HIGH;
> > > > +    default:
> > > > +        return AMDGPU_VCN_ENC_PRIO_NORMAL;
> > > > +    }
> > > > +}
> > > > +
> > > > +static enum gfx_pipe_priority 
> > > > amdgpu_ctx_sched_prio_to_vce_prio(enum drm_sched_priority prio)
> > > > +{
> > > > +    switch (prio) {
> > > > +    case DRM_SCHED_PRIORITY_HIGH:
> > > > +        return AMDGPU_VCE_ENC_PRIO_HIGH;
> > > > +    case DRM_SCHED_PRIORITY_KERNEL:
> > > > +        return AMDGPU_VCE_ENC_PRIO_VERY_HIGH;
> > > > +    default:
> > > > +        return AMDGPU_VCE_ENC_PRIO_NORMAL;
> > > > +    }
> > > > +}
> > > > +
> > > >   static unsigned int amdgpu_ctx_prio_sched_to_hw(struct 
> > > > amdgpu_device *adev,
> > > >                            enum drm_sched_priority prio,
> > > >                            u32 hw_ip)
> > > >   {
> > > >       unsigned int hw_prio;
> > > >   -    hw_prio = (hw_ip == AMDGPU_HW_IP_COMPUTE) ?
> > > > -            amdgpu_ctx_sched_prio_to_compute_prio(prio) :
> > > > -            AMDGPU_RING_PRIO_DEFAULT;
> > > > +    if (hw_ip == AMDGPU_HW_IP_COMPUTE)
> > > > +        hw_prio = amdgpu_ctx_sched_prio_to_compute_prio(prio);
> > > > +    else if (hw_ip == AMDGPU_HW_IP_VCN_ENC)
> > > > +        hw_prio = amdgpu_ctx_sched_prio_to_vcn_prio(prio);
> > > > +    else if (hw_ip == AMDGPU_HW_IP_VCE)
> > > > +        hw_prio = amdgpu_ctx_sched_prio_to_vce_prio(prio);
> > > > +    else
> > > > +        hw_prio = AMDGPU_RING_PRIO_DEFAULT;
> > > >       hw_ip = array_index_nospec(hw_ip, AMDGPU_HW_IP_NUM);
> > > >       if (adev->gpu_sched[hw_ip][hw_prio].num_scheds == 0)
> > > >           hw_prio = AMDGPU_RING_PRIO_DEFAULT;
> > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sched.c 
> > > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_sched.c
> > > > index b7d861ed5284..4087acb6b8bb 100644
> > > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_sched.c
> > > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sched.c
> > > > @@ -37,7 +37,7 @@ int amdgpu_to_sched_priority(int amdgpu_priority,
> > > >   {
> > > >       switch (amdgpu_priority) {
> > > >       case AMDGPU_CTX_PRIORITY_VERY_HIGH:
> > > > -        *prio = DRM_SCHED_PRIORITY_HIGH;
> > > > +        *prio = DRM_SCHED_PRIORITY_KERNEL;
> > >
> > >
> > > This change looks unrelated to the patch. AFAIK KERNEL priority is 
> > > reserved for kernel tasks, userspace shouldn't be able to set this 
> > > privilege priority.
> > >
> > >
> > > Regards,
> > >
> > > Nirmoy
> >
> > VCE has 3 rings (General purpose, Low latency and Real time). From 
> > the user side when priority VERY_HIGH(1023) is set, real time should 
> > be chosen, if available. That is the reason i have mapped 
> > AMDGPU_CTX_PRIORITY_VERY_HIGH to DRM_SCHED_PRIORITY_KERNEL.
>
>
> How are low latency and real-time ring deffer ? Do we have usecase for 
> all three types of ring ?
>
>
> Apart from the priority name there is another issue with using 
> DRM_SCHED_PRIORITY_KERNEL, jobs with kernel priority will not be 
> punished[1]
>
> [1] 
> https://elixir.bootlin.com/linux/v5.13.9/source/drivers/gpu/drm/scheduler/sched_main.c#L939 
>
>
>
> May be we should have DRM_SCHED_PRIORITY_VERY_HIGH ?
>
>
> Regards,
>
> Nirmoy

Low latency and real time rings are different rings with different 
priorities. Adding DRM_SCHED_PRIORITY_VERY_HIGH sounds good. I will send 
the patch again with that modification.


regards,

Satyajit

> > regards,
> >
> > Satyajit
> >
> > > >           break;
> > > >       case AMDGPU_CTX_PRIORITY_HIGH:
> > > >           *prio = DRM_SCHED_PRIORITY_HIGH;
> > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vce.c 
> > > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_vce.c
> > > > index 1ae7f824adc7..9d59ca31bc22 100644
> > > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vce.c
> > > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vce.c
> > > > @@ -1168,3 +1168,17 @@ int amdgpu_vce_ring_test_ib(struct 
> > > > amdgpu_ring *ring, long timeout)
> > > >       amdgpu_bo_free_kernel(&bo, NULL, NULL);
> > > >       return r;
> > > >   }
> > > > +
> > > > +enum vce_enc_ring_priority get_vce_ring_prio(int index)
> > > > +{
> > > > +    switch(index) {
> > > > +    case 0:
> > > > +        return AMDGPU_VCE_ENC_PRIO_NORMAL;
> > > > +    case 1:
> > > > +        return AMDGPU_VCE_ENC_PRIO_HIGH;
> > > > +    case 2:
> > > > +        return AMDGPU_VCE_ENC_PRIO_VERY_HIGH;
> > > > +    default:
> > > > +        return AMDGPU_VCE_ENC_PRIO_NORMAL;
> > > > +    }
> > > > +}
> > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vce.h 
> > > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_vce.h
> > > > index d6d83a3ec803..f5265f385890 100644
> > > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vce.h
> > > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vce.h
> > > > @@ -32,6 +32,13 @@
> > > >     #define AMDGPU_VCE_FW_53_45    ((53 << 24) | (45 << 16))
> > > >   +enum vce_enc_ring_priority {
> > > > +    AMDGPU_VCE_ENC_PRIO_NORMAL = 1,
> > > > +    AMDGPU_VCE_ENC_PRIO_HIGH,
> > > > +    AMDGPU_VCE_ENC_PRIO_VERY_HIGH,
> > > > +    AMDGPU_VCE_ENC_PRIO_MAX
> > > > +};
> > > > +
> > > >   struct amdgpu_vce {
> > > >       struct amdgpu_bo    *vcpu_bo;
> > > >       uint64_t        gpu_addr;
> > > > @@ -72,4 +79,6 @@ void amdgpu_vce_ring_end_use(struct amdgpu_ring 
> > > > *ring);
> > > >   unsigned amdgpu_vce_ring_get_emit_ib_size(struct amdgpu_ring *ring);
> > > >   unsigned amdgpu_vce_ring_get_dma_frame_size(struct amdgpu_ring 
> > > > *ring);
> > > >   +enum vce_enc_ring_priority get_vce_ring_prio(int index);
> > > > +
> > > >   #endif
> > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c 
> > > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c
> > > > index 6780df0fb265..ca6cc07a726b 100644
> > > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c
> > > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c
> > > > @@ -951,3 +951,17 @@ int amdgpu_vcn_enc_ring_test_ib(struct 
> > > > amdgpu_ring *ring, long timeout)
> > > >         return r;
> > > >   }
> > > > +
> > > > +enum vcn_enc_ring_priority get_vcn_enc_ring_prio(int index)
> > > > +{
> > > > +    switch(index) {
> > > > +    case 0:
> > > > +        return AMDGPU_VCN_ENC_PRIO_NORMAL;
> > > > +    case 1:
> > > > +        return AMDGPU_VCN_ENC_PRIO_HIGH;
> > > > +    case 2:
> > > > +        return AMDGPU_VCN_ENC_PRIO_VERY_HIGH;
> > > > +    default:
> > > > +        return AMDGPU_VCN_ENC_PRIO_NORMAL;
> > > > +    }
> > > > +}
> > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h 
> > > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h
> > > > index d74c62b49795..bf14ee54f774 100644
> > > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h
> > > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.h
> > > > @@ -290,6 +290,13 @@ enum vcn_ring_type {
> > > >       VCN_UNIFIED_RING,
> > > >   };
> > > >   +enum vcn_enc_ring_priority {
> > > > +    AMDGPU_VCN_ENC_PRIO_NORMAL = 1,
> > > > +    AMDGPU_VCN_ENC_PRIO_HIGH,
> > > > +    AMDGPU_VCN_ENC_PRIO_VERY_HIGH,
> > > > +    AMDGPU_VCN_ENC_PRIO_MAX
> > > > +};
> > > > +
> > > >   int amdgpu_vcn_sw_init(struct amdgpu_device *adev);
> > > >   int amdgpu_vcn_sw_fini(struct amdgpu_device *adev);
> > > >   int amdgpu_vcn_suspend(struct amdgpu_device *adev);
> > > > @@ -307,5 +314,6 @@ int amdgpu_vcn_dec_sw_ring_test_ib(struct 
> > > > amdgpu_ring *ring, long timeout);
> > > >     int amdgpu_vcn_enc_ring_test_ring(struct amdgpu_ring *ring);
> > > >   int amdgpu_vcn_enc_ring_test_ib(struct amdgpu_ring *ring, long 
> > > > timeout);
> > > > +enum vcn_enc_ring_priority get_vcn_enc_ring_prio(int index);
> > > >     #endif
> > > > diff --git a/drivers/gpu/drm/amd/amdgpu/vce_v2_0.c 
> > > > b/drivers/gpu/drm/amd/amdgpu/vce_v2_0.c
> > > > index c7d28c169be5..2b6b7f1a77b9 100644
> > > > --- a/drivers/gpu/drm/amd/amdgpu/vce_v2_0.c
> > > > +++ b/drivers/gpu/drm/amd/amdgpu/vce_v2_0.c
> > > > @@ -431,10 +431,12 @@ static int vce_v2_0_sw_init(void *handle)
> > > >           return r;
> > > >         for (i = 0; i < adev->vce.num_rings; i++) {
> > > > +        unsigned int hw_prio = get_vce_ring_prio(i);
> > > > +
> > > >           ring = &adev->vce.ring[i];
> > > >           sprintf(ring->name, "vce%d", i);
> > > >           r = amdgpu_ring_init(adev, ring, 512, &adev->vce.irq, 0,
> > > > -                     AMDGPU_RING_PRIO_DEFAULT, NULL);
> > > > +                     hw_prio, NULL);
> > > >           if (r)
> > > >               return r;
> > > >       }
> > > > diff --git a/drivers/gpu/drm/amd/amdgpu/vce_v3_0.c 
> > > > b/drivers/gpu/drm/amd/amdgpu/vce_v3_0.c
> > > > index 3b82fb289ef6..5ce182a837f3 100644
> > > > --- a/drivers/gpu/drm/amd/amdgpu/vce_v3_0.c
> > > > +++ b/drivers/gpu/drm/amd/amdgpu/vce_v3_0.c
> > > > @@ -440,10 +440,12 @@ static int vce_v3_0_sw_init(void *handle)
> > > >           return r;
> > > >         for (i = 0; i < adev->vce.num_rings; i++) {
> > > > +        unsigned int hw_prio = get_vce_ring_prio(i);
> > > > +
> > > >           ring = &adev->vce.ring[i];
> > > >           sprintf(ring->name, "vce%d", i);
> > > >           r = amdgpu_ring_init(adev, ring, 512, &adev->vce.irq, 0,
> > > > -                     AMDGPU_RING_PRIO_DEFAULT, NULL);
> > > > +                     hw_prio, NULL);
> > > >           if (r)
> > > >               return r;
> > > >       }
> > > > diff --git a/drivers/gpu/drm/amd/amdgpu/vce_v4_0.c 
> > > > b/drivers/gpu/drm/amd/amdgpu/vce_v4_0.c
> > > > index 90910d19db12..c085defaabfe 100644
> > > > --- a/drivers/gpu/drm/amd/amdgpu/vce_v4_0.c
> > > > +++ b/drivers/gpu/drm/amd/amdgpu/vce_v4_0.c
> > > > @@ -463,6 +463,8 @@ static int vce_v4_0_sw_init(void *handle)
> > > >       }
> > > >         for (i = 0; i < adev->vce.num_rings; i++) {
> > > > +        unsigned int hw_prio = get_vce_ring_prio(i);
> > > > +
> > > >           ring = &adev->vce.ring[i];
> > > >           sprintf(ring->name, "vce%d", i);
> > > >           if (amdgpu_sriov_vf(adev)) {
> > > > @@ -478,7 +480,7 @@ static int vce_v4_0_sw_init(void *handle)
> > > >                   ring->doorbell_index = 
> > > > adev->doorbell_index.uvd_vce.vce_ring2_3 * 2 + 1;
> > > >           }
> > > >           r = amdgpu_ring_init(adev, ring, 512, &adev->vce.irq, 0,
> > > > -                     AMDGPU_RING_PRIO_DEFAULT, NULL);
> > > > +                     hw_prio, NULL);
> > > >           if (r)
> > > >               return r;
> > > >       }
> > > > diff --git a/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c 
> > > > b/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c
> > > > index 121ee9f2b8d1..f471c65d78bc 100644
> > > > --- a/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c
> > > > +++ b/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c
> > > > @@ -145,10 +145,12 @@ static int vcn_v1_0_sw_init(void *handle)
> > > >           SOC15_REG_OFFSET(UVD, 0, mmUVD_NO_OP);
> > > >         for (i = 0; i < adev->vcn.num_enc_rings; ++i) {
> > > > +        unsigned int hw_prio = get_vcn_enc_ring_prio(i);
> > > > +
> > > >           ring = &adev->vcn.inst->ring_enc[i];
> > > >           sprintf(ring->name, "vcn_enc%d", i);
> > > >           r = amdgpu_ring_init(adev, ring, 512, 
> > > > &adev->vcn.inst->irq, 0,
> > > > -                     AMDGPU_RING_PRIO_DEFAULT, NULL);
> > > > +                     hw_prio, NULL);
> > > >           if (r)
> > > >               return r;
> > > >       }
> > > > diff --git a/drivers/gpu/drm/amd/amdgpu/vcn_v2_0.c 
> > > > b/drivers/gpu/drm/amd/amdgpu/vcn_v2_0.c
> > > > index f4686e918e0d..06978d5a93c7 100644
> > > > --- a/drivers/gpu/drm/amd/amdgpu/vcn_v2_0.c
> > > > +++ b/drivers/gpu/drm/amd/amdgpu/vcn_v2_0.c
> > > > @@ -159,6 +159,8 @@ static int vcn_v2_0_sw_init(void *handle)
> > > >       adev->vcn.inst->external.nop = SOC15_REG_OFFSET(UVD, 0, 
> > > > mmUVD_NO_OP);
> > > >         for (i = 0; i < adev->vcn.num_enc_rings; ++i) {
> > > > +        unsigned int hw_prio = get_vcn_enc_ring_prio(i);
> > > > +
> > > >           ring = &adev->vcn.inst->ring_enc[i];
> > > >           ring->use_doorbell = true;
> > > >           if (!amdgpu_sriov_vf(adev))
> > > > @@ -167,7 +169,7 @@ static int vcn_v2_0_sw_init(void *handle)
> > > >               ring->doorbell_index = 
> > > > (adev->doorbell_index.vcn.vcn_ring0_1 << 1) + 1 + i;
> > > >           sprintf(ring->name, "vcn_enc%d", i);
> > > >           r = amdgpu_ring_init(adev, ring, 512, 
> > > > &adev->vcn.inst->irq, 0,
> > > > -                     AMDGPU_RING_PRIO_DEFAULT, NULL);
> > > > +                     hw_prio, NULL);
> > > >           if (r)
> > > >               return r;
> > > >       }
> > > > diff --git a/drivers/gpu/drm/amd/amdgpu/vcn_v2_5.c 
> > > > b/drivers/gpu/drm/amd/amdgpu/vcn_v2_5.c
> > > > index e0c0c3734432..fba453ca74fa 100644
> > > > --- a/drivers/gpu/drm/amd/amdgpu/vcn_v2_5.c
> > > > +++ b/drivers/gpu/drm/amd/amdgpu/vcn_v2_5.c
> > > > @@ -194,6 +194,8 @@ static int vcn_v2_5_sw_init(void *handle)
> > > >               return r;
> > > >             for (i = 0; i < adev->vcn.num_enc_rings; ++i) {
> > > > +            unsigned int hw_prio = get_vcn_enc_ring_prio(i);
> > > > +
> > > >               ring = &adev->vcn.inst[j].ring_enc[i];
> > > >               ring->use_doorbell = true;
> > > >   @@ -203,7 +205,7 @@ static int vcn_v2_5_sw_init(void *handle)
> > > >               sprintf(ring->name, "vcn_enc_%d.%d", j, i);
> > > >               r = amdgpu_ring_init(adev, ring, 512,
> > > >                            &adev->vcn.inst[j].irq, 0,
> > > > -                         AMDGPU_RING_PRIO_DEFAULT, NULL);
> > > > +                         hw_prio, NULL);
> > > >               if (r)
> > > >                   return r;
> > > >           }
> > > > diff --git a/drivers/gpu/drm/amd/amdgpu/vcn_v3_0.c 
> > > > b/drivers/gpu/drm/amd/amdgpu/vcn_v3_0.c
> > > > index 3d18aab88b4e..f5baf7321b0d 100644
> > > > --- a/drivers/gpu/drm/amd/amdgpu/vcn_v3_0.c
> > > > +++ b/drivers/gpu/drm/amd/amdgpu/vcn_v3_0.c
> > > > @@ -224,6 +224,8 @@ static int vcn_v3_0_sw_init(void *handle)
> > > >               return r;
> > > >             for (j = 0; j < adev->vcn.num_enc_rings; ++j) {
> > > > +            unsigned int hw_prio = get_vcn_enc_ring_prio(j);
> > > > +
> > > >               /* VCN ENC TRAP */
> > > >               r = amdgpu_irq_add_id(adev, amdgpu_ih_clientid_vcns[i],
> > > >                   j + VCN_2_0__SRCID__UVD_ENC_GENERAL_PURPOSE, 
> > > > &adev->vcn.inst[i].irq);
> > > > @@ -239,8 +241,7 @@ static int vcn_v3_0_sw_init(void *handle)
> > > >               }
> > > >               sprintf(ring->name, "vcn_enc_%d.%d", i, j);
> > > >               r = amdgpu_ring_init(adev, ring, 512, 
> > > > &adev->vcn.inst[i].irq, 0,
> > > > -                         AMDGPU_RING_PRIO_DEFAULT,
> > > > - &adev->vcn.inst[i].sched_score);
> > > > +                         hw_prio, &adev->vcn.inst[i].sched_score);
> > > >               if (r)
> > > >                   return r;
> > > >           }