[RFC PATCH 0/3] cleanup compute queue priority setting

[RFC PATCH 0/3] cleanup compute queue priority setting

[Nirmoy Das]

This is my initial idea which sets half compute queues to normal priority and
other half to high priority. I am reusing existing set_priority codes
here.

My understanding of gfx_v*_0_pipe_reserve_resources is very low so I am
probably doing some mistake at init_priority()

Nirmoy Das (3):
  drm/amdgpu: implement ring init_priority for compute ring
  drm/amdgpu: change hw sched list on ctx priority override
  drm/amdgpu: remove unused functions

 drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c   |  4 --
 drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c  | 57 ++++++++++++++++--
 drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h  |  2 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_job.c  |  6 --
 drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c | 74 ++----------------------
 drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h |  5 +-
 drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c    | 14 +++++
 drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c    | 13 +++++
 8 files changed, 86 insertions(+), 89 deletions(-)

--
2.25.0

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[RFC PATCH 1/3] drm/amdgpu: implement ring init_priority for compute ring

[Nirmoy Das]

init_priority will set second compute queue(gfx8 and gfx9) of a pipe to high priority
and 1st queue to normal priority.

Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h |  1 +
 drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c    | 14 ++++++++++++++
 drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c    | 13 +++++++++++++
 3 files changed, 28 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
index 24caff085d00..a109373b9fe8 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
@@ -170,6 +170,7 @@ struct amdgpu_ring_funcs {
 	/* priority functions */
 	void (*set_priority) (struct amdgpu_ring *ring,
 			      enum drm_sched_priority priority);
+	void (*init_priority) (struct amdgpu_ring *ring);
 	/* Try to soft recover the ring to make the fence signal */
 	void (*soft_recovery)(struct amdgpu_ring *ring, unsigned vmid);
 	int (*preempt_ib)(struct amdgpu_ring *ring);
diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
index fa245973de12..14bab6e08bd6 100644
--- a/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
@@ -6334,6 +6334,19 @@ static void gfx_v8_0_ring_set_priority_compute(struct amdgpu_ring *ring,
 	gfx_v8_0_pipe_reserve_resources(adev, ring, acquire);
 }

+static void gfx_v8_0_ring_init_priority_compute(struct amdgpu_ring *ring)
+{
+	/* set pipe 0 to normal priority and pipe 1 to high priority*/
+	if (ring->queue == 1) {
+		gfx_v8_0_hqd_set_priority(ring->adev, ring, true);
+		gfx_v8_0_ring_set_pipe_percent(ring, true);
+	} else {
+		gfx_v8_0_hqd_set_priority(ring->adev, ring, false);
+		gfx_v8_0_ring_set_pipe_percent(ring, false);
+	}
+
+}
+
 static void gfx_v8_0_ring_emit_fence_compute(struct amdgpu_ring *ring,
 					     u64 addr, u64 seq,
 					     unsigned flags)
@@ -6967,6 +6980,7 @@ static const struct amdgpu_ring_funcs gfx_v8_0_ring_funcs_compute = {
 	.insert_nop = amdgpu_ring_insert_nop,
 	.pad_ib = amdgpu_ring_generic_pad_ib,
 	.set_priority = gfx_v8_0_ring_set_priority_compute,
+	.init_priority = gfx_v8_0_ring_init_priority_compute,
 	.emit_wreg = gfx_v8_0_ring_emit_wreg,
 };

diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
index 1c7a16b91686..0c66743fb6f5 100644
--- a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
@@ -5143,6 +5143,18 @@ static void gfx_v9_0_ring_set_priority_compute(struct amdgpu_ring *ring,
 	gfx_v9_0_pipe_reserve_resources(adev, ring, acquire);
 }

+static void gfx_v9_0_ring_init_priority_compute(struct amdgpu_ring *ring)
+{
+	/* set pipe 0 to normal priority and pipe 1 to high priority*/
+	if (ring->queue == 1) {
+		gfx_v9_0_hqd_set_priority(ring->adev, ring, true);
+		gfx_v9_0_ring_set_pipe_percent(ring, true);
+	} else {
+		gfx_v9_0_hqd_set_priority(ring->adev, ring, false);
+		gfx_v9_0_ring_set_pipe_percent(ring, true);
+	}
+}
+
 static void gfx_v9_0_ring_set_wptr_compute(struct amdgpu_ring *ring)
 {
 	struct amdgpu_device *adev = ring->adev;
@@ -6514,6 +6526,7 @@ static const struct amdgpu_ring_funcs gfx_v9_0_ring_funcs_compute = {
 	.insert_nop = amdgpu_ring_insert_nop,
 	.pad_ib = amdgpu_ring_generic_pad_ib,
 	.set_priority = gfx_v9_0_ring_set_priority_compute,
+	.init_priority = gfx_v9_0_ring_init_priority_compute,
 	.emit_wreg = gfx_v9_0_ring_emit_wreg,
 	.emit_reg_wait = gfx_v9_0_ring_emit_reg_wait,
 	.emit_reg_write_reg_wait = gfx_v9_0_ring_emit_reg_write_reg_wait,
--
2.25.0

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [RFC PATCH 1/3] drm/amdgpu: implement ring init_priority for compute ring

[Alex Deucher]

On Wed, Feb 26, 2020 at 3:34 PM Nirmoy Das <nirmoy.aiemd@gmail.com> wrote:
>
> init_priority will set second compute queue(gfx8 and gfx9) of a pipe to high priority
> and 1st queue to normal priority.
>
> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h |  1 +
>  drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c    | 14 ++++++++++++++
>  drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c    | 13 +++++++++++++
>  3 files changed, 28 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
> index 24caff085d00..a109373b9fe8 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
> @@ -170,6 +170,7 @@ struct amdgpu_ring_funcs {
>         /* priority functions */
>         void (*set_priority) (struct amdgpu_ring *ring,
>                               enum drm_sched_priority priority);
> +       void (*init_priority) (struct amdgpu_ring *ring);
>         /* Try to soft recover the ring to make the fence signal */
>         void (*soft_recovery)(struct amdgpu_ring *ring, unsigned vmid);
>         int (*preempt_ib)(struct amdgpu_ring *ring);
> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
> index fa245973de12..14bab6e08bd6 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
> @@ -6334,6 +6334,19 @@ static void gfx_v8_0_ring_set_priority_compute(struct amdgpu_ring *ring,
>         gfx_v8_0_pipe_reserve_resources(adev, ring, acquire);
>  }
>
> +static void gfx_v8_0_ring_init_priority_compute(struct amdgpu_ring *ring)
> +{
> +       /* set pipe 0 to normal priority and pipe 1 to high priority*/
> +       if (ring->queue == 1) {
> +               gfx_v8_0_hqd_set_priority(ring->adev, ring, true);
> +               gfx_v8_0_ring_set_pipe_percent(ring, true);
> +       } else {
> +               gfx_v8_0_hqd_set_priority(ring->adev, ring, false);
> +               gfx_v8_0_ring_set_pipe_percent(ring, false);
> +       }
> +
> +}

We should drop gfx_v8_0_hqd_set_priority() and set the priorities in
the mqd instead.  In gfx_v8_0_mqd_init(), set
mqd->cp_hqd_pipe_priority and mqd->cp_hqd_queue_priority as
appropriate for each queue.  I'm not sure we want to mess with
gfx_v8_0_ring_set_pipe_percent ultimately at all once this lands.
That stuff statically adjusts the priorities between gfx and compute.

> +
>  static void gfx_v8_0_ring_emit_fence_compute(struct amdgpu_ring *ring,
>                                              u64 addr, u64 seq,
>                                              unsigned flags)
> @@ -6967,6 +6980,7 @@ static const struct amdgpu_ring_funcs gfx_v8_0_ring_funcs_compute = {
>         .insert_nop = amdgpu_ring_insert_nop,
>         .pad_ib = amdgpu_ring_generic_pad_ib,
>         .set_priority = gfx_v8_0_ring_set_priority_compute,
> +       .init_priority = gfx_v8_0_ring_init_priority_compute,
>         .emit_wreg = gfx_v8_0_ring_emit_wreg,
>  };
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
> index 1c7a16b91686..0c66743fb6f5 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
> @@ -5143,6 +5143,18 @@ static void gfx_v9_0_ring_set_priority_compute(struct amdgpu_ring *ring,
>         gfx_v9_0_pipe_reserve_resources(adev, ring, acquire);
>  }
>
> +static void gfx_v9_0_ring_init_priority_compute(struct amdgpu_ring *ring)
> +{
> +       /* set pipe 0 to normal priority and pipe 1 to high priority*/
> +       if (ring->queue == 1) {
> +               gfx_v9_0_hqd_set_priority(ring->adev, ring, true);
> +               gfx_v9_0_ring_set_pipe_percent(ring, true);
> +       } else {
> +               gfx_v9_0_hqd_set_priority(ring->adev, ring, false);
> +               gfx_v9_0_ring_set_pipe_percent(ring, true);
> +       }
> +}

Same comment as above.

Alex

> +
>  static void gfx_v9_0_ring_set_wptr_compute(struct amdgpu_ring *ring)
>  {
>         struct amdgpu_device *adev = ring->adev;
> @@ -6514,6 +6526,7 @@ static const struct amdgpu_ring_funcs gfx_v9_0_ring_funcs_compute = {
>         .insert_nop = amdgpu_ring_insert_nop,
>         .pad_ib = amdgpu_ring_generic_pad_ib,
>         .set_priority = gfx_v9_0_ring_set_priority_compute,
> +       .init_priority = gfx_v9_0_ring_init_priority_compute,
>         .emit_wreg = gfx_v9_0_ring_emit_wreg,
>         .emit_reg_wait = gfx_v9_0_ring_emit_reg_wait,
>         .emit_reg_write_reg_wait = gfx_v9_0_ring_emit_reg_write_reg_wait,
> --
> 2.25.0
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [RFC PATCH 1/3] drm/amdgpu: implement ring init_priority for compute ring

[Nirmoy]

On 2/27/20 5:44 AM, Alex Deucher wrote:
> On Wed, Feb 26, 2020 at 3:34 PM Nirmoy Das <nirmoy.aiemd@gmail.com> wrote:
>> init_priority will set second compute queue(gfx8 and gfx9) of a pipe to high priority
>> and 1st queue to normal priority.
>>
>> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h |  1 +
>>   drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c    | 14 ++++++++++++++
>>   drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c    | 13 +++++++++++++
>>   3 files changed, 28 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
>> index 24caff085d00..a109373b9fe8 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
>> @@ -170,6 +170,7 @@ struct amdgpu_ring_funcs {
>>          /* priority functions */
>>          void (*set_priority) (struct amdgpu_ring *ring,
>>                                enum drm_sched_priority priority);
>> +       void (*init_priority) (struct amdgpu_ring *ring);
>>          /* Try to soft recover the ring to make the fence signal */
>>          void (*soft_recovery)(struct amdgpu_ring *ring, unsigned vmid);
>>          int (*preempt_ib)(struct amdgpu_ring *ring);
>> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
>> index fa245973de12..14bab6e08bd6 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
>> @@ -6334,6 +6334,19 @@ static void gfx_v8_0_ring_set_priority_compute(struct amdgpu_ring *ring,
>>          gfx_v8_0_pipe_reserve_resources(adev, ring, acquire);
>>   }
>>
>> +static void gfx_v8_0_ring_init_priority_compute(struct amdgpu_ring *ring)
>> +{
>> +       /* set pipe 0 to normal priority and pipe 1 to high priority*/
>> +       if (ring->queue == 1) {
>> +               gfx_v8_0_hqd_set_priority(ring->adev, ring, true);
>> +               gfx_v8_0_ring_set_pipe_percent(ring, true);
>> +       } else {
>> +               gfx_v8_0_hqd_set_priority(ring->adev, ring, false);
>> +               gfx_v8_0_ring_set_pipe_percent(ring, false);
>> +       }
>> +
>> +}
> We should drop gfx_v8_0_hqd_set_priority() and set the priorities in
> the mqd instead.  In gfx_v8_0_mqd_init(), set
> mqd->cp_hqd_pipe_priority and mqd->cp_hqd_queue_priority as
> appropriate for each queue.  I'm not sure we want to mess with
> gfx_v8_0_ring_set_pipe_percent ultimately at all once this lands.
> That stuff statically adjusts the priorities between gfx and compute.
Thanks Alex. I will send a updated patch for this.
>
>> +
>>   static void gfx_v8_0_ring_emit_fence_compute(struct amdgpu_ring *ring,
>>                                               u64 addr, u64 seq,
>>                                               unsigned flags)
>> @@ -6967,6 +6980,7 @@ static const struct amdgpu_ring_funcs gfx_v8_0_ring_funcs_compute = {
>>          .insert_nop = amdgpu_ring_insert_nop,
>>          .pad_ib = amdgpu_ring_generic_pad_ib,
>>          .set_priority = gfx_v8_0_ring_set_priority_compute,
>> +       .init_priority = gfx_v8_0_ring_init_priority_compute,
>>          .emit_wreg = gfx_v8_0_ring_emit_wreg,
>>   };
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
>> index 1c7a16b91686..0c66743fb6f5 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
>> @@ -5143,6 +5143,18 @@ static void gfx_v9_0_ring_set_priority_compute(struct amdgpu_ring *ring,
>>          gfx_v9_0_pipe_reserve_resources(adev, ring, acquire);
>>   }
>>
>> +static void gfx_v9_0_ring_init_priority_compute(struct amdgpu_ring *ring)
>> +{
>> +       /* set pipe 0 to normal priority and pipe 1 to high priority*/
>> +       if (ring->queue == 1) {
>> +               gfx_v9_0_hqd_set_priority(ring->adev, ring, true);
>> +               gfx_v9_0_ring_set_pipe_percent(ring, true);
>> +       } else {
>> +               gfx_v9_0_hqd_set_priority(ring->adev, ring, false);
>> +               gfx_v9_0_ring_set_pipe_percent(ring, true);
>> +       }
>> +}
> Same comment as above.
>
> Alex
>
>> +
>>   static void gfx_v9_0_ring_set_wptr_compute(struct amdgpu_ring *ring)
>>   {
>>          struct amdgpu_device *adev = ring->adev;
>> @@ -6514,6 +6526,7 @@ static const struct amdgpu_ring_funcs gfx_v9_0_ring_funcs_compute = {
>>          .insert_nop = amdgpu_ring_insert_nop,
>>          .pad_ib = amdgpu_ring_generic_pad_ib,
>>          .set_priority = gfx_v9_0_ring_set_priority_compute,
>> +       .init_priority = gfx_v9_0_ring_init_priority_compute,
>>          .emit_wreg = gfx_v9_0_ring_emit_wreg,
>>          .emit_reg_wait = gfx_v9_0_ring_emit_reg_wait,
>>          .emit_reg_write_reg_wait = gfx_v9_0_ring_emit_reg_write_reg_wait,
>> --
>> 2.25.0
>>
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx@lists.freedesktop.org
>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&amp;data=02%7C01%7Cnirmoy.das%40amd.com%7Cdfeeaa22459548bb7b9808d7bb3fcb60%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637183754987155471&amp;sdata=CUR%2BxoLBO%2FPSG7B1lyKAgQVcMLr%2BqQRYlHHs3OGFZPA%3D&amp;reserved=0
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [RFC PATCH 1/3] drm/amdgpu: implement ring init_priority for compute ring

[Christian König]

Am 27.02.20 um 10:57 schrieb Nirmoy:
>
> On 2/27/20 5:44 AM, Alex Deucher wrote:
>> On Wed, Feb 26, 2020 at 3:34 PM Nirmoy Das <nirmoy.aiemd@gmail.com> 
>> wrote:
>>> init_priority will set second compute queue(gfx8 and gfx9) of a pipe 
>>> to high priority
>>> and 1st queue to normal priority.
>>>
>>> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
>>> ---
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h |  1 +
>>>   drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c    | 14 ++++++++++++++
>>>   drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c    | 13 +++++++++++++
>>>   3 files changed, 28 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h 
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
>>> index 24caff085d00..a109373b9fe8 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
>>> @@ -170,6 +170,7 @@ struct amdgpu_ring_funcs {
>>>          /* priority functions */
>>>          void (*set_priority) (struct amdgpu_ring *ring,
>>>                                enum drm_sched_priority priority);
>>> +       void (*init_priority) (struct amdgpu_ring *ring);
>>>          /* Try to soft recover the ring to make the fence signal */
>>>          void (*soft_recovery)(struct amdgpu_ring *ring, unsigned 
>>> vmid);
>>>          int (*preempt_ib)(struct amdgpu_ring *ring);
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c 
>>> b/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
>>> index fa245973de12..14bab6e08bd6 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
>>> @@ -6334,6 +6334,19 @@ static void 
>>> gfx_v8_0_ring_set_priority_compute(struct amdgpu_ring *ring,
>>>          gfx_v8_0_pipe_reserve_resources(adev, ring, acquire);
>>>   }
>>>
>>> +static void gfx_v8_0_ring_init_priority_compute(struct amdgpu_ring 
>>> *ring)
>>> +{
>>> +       /* set pipe 0 to normal priority and pipe 1 to high priority*/
>>> +       if (ring->queue == 1) {
>>> +               gfx_v8_0_hqd_set_priority(ring->adev, ring, true);
>>> +               gfx_v8_0_ring_set_pipe_percent(ring, true);
>>> +       } else {
>>> +               gfx_v8_0_hqd_set_priority(ring->adev, ring, false);
>>> +               gfx_v8_0_ring_set_pipe_percent(ring, false);
>>> +       }
>>> +
>>> +}
>> We should drop gfx_v8_0_hqd_set_priority() and set the priorities in
>> the mqd instead.  In gfx_v8_0_mqd_init(), set
>> mqd->cp_hqd_pipe_priority and mqd->cp_hqd_queue_priority as
>> appropriate for each queue.  I'm not sure we want to mess with
>> gfx_v8_0_ring_set_pipe_percent ultimately at all once this lands.
>> That stuff statically adjusts the priorities between gfx and compute.
> Thanks Alex. I will send a updated patch for this.

Maybe wait a second with that I'm just going over patch #2.

Christian.

>>
>>> +
>>>   static void gfx_v8_0_ring_emit_fence_compute(struct amdgpu_ring 
>>> *ring,
>>>                                               u64 addr, u64 seq,
>>>                                               unsigned flags)
>>> @@ -6967,6 +6980,7 @@ static const struct amdgpu_ring_funcs 
>>> gfx_v8_0_ring_funcs_compute = {
>>>          .insert_nop = amdgpu_ring_insert_nop,
>>>          .pad_ib = amdgpu_ring_generic_pad_ib,
>>>          .set_priority = gfx_v8_0_ring_set_priority_compute,
>>> +       .init_priority = gfx_v8_0_ring_init_priority_compute,
>>>          .emit_wreg = gfx_v8_0_ring_emit_wreg,
>>>   };
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c 
>>> b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
>>> index 1c7a16b91686..0c66743fb6f5 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
>>> @@ -5143,6 +5143,18 @@ static void 
>>> gfx_v9_0_ring_set_priority_compute(struct amdgpu_ring *ring,
>>>          gfx_v9_0_pipe_reserve_resources(adev, ring, acquire);
>>>   }
>>>
>>> +static void gfx_v9_0_ring_init_priority_compute(struct amdgpu_ring 
>>> *ring)
>>> +{
>>> +       /* set pipe 0 to normal priority and pipe 1 to high priority*/
>>> +       if (ring->queue == 1) {
>>> +               gfx_v9_0_hqd_set_priority(ring->adev, ring, true);
>>> +               gfx_v9_0_ring_set_pipe_percent(ring, true);
>>> +       } else {
>>> +               gfx_v9_0_hqd_set_priority(ring->adev, ring, false);
>>> +               gfx_v9_0_ring_set_pipe_percent(ring, true);
>>> +       }
>>> +}
>> Same comment as above.
>>
>> Alex
>>
>>> +
>>>   static void gfx_v9_0_ring_set_wptr_compute(struct amdgpu_ring *ring)
>>>   {
>>>          struct amdgpu_device *adev = ring->adev;
>>> @@ -6514,6 +6526,7 @@ static const struct amdgpu_ring_funcs 
>>> gfx_v9_0_ring_funcs_compute = {
>>>          .insert_nop = amdgpu_ring_insert_nop,
>>>          .pad_ib = amdgpu_ring_generic_pad_ib,
>>>          .set_priority = gfx_v9_0_ring_set_priority_compute,
>>> +       .init_priority = gfx_v9_0_ring_init_priority_compute,
>>>          .emit_wreg = gfx_v9_0_ring_emit_wreg,
>>>          .emit_reg_wait = gfx_v9_0_ring_emit_reg_wait,
>>>          .emit_reg_write_reg_wait = 
>>> gfx_v9_0_ring_emit_reg_write_reg_wait,
>>> -- 
>>> 2.25.0
>>>
>>> _______________________________________________
>>> amd-gfx mailing list
>>> amd-gfx@lists.freedesktop.org
>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&amp;data=02%7C01%7Cnirmoy.das%40amd.com%7Cdfeeaa22459548bb7b9808d7bb3fcb60%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637183754987155471&amp;sdata=CUR%2BxoLBO%2FPSG7B1lyKAgQVcMLr%2BqQRYlHHs3OGFZPA%3D&amp;reserved=0 
>>>

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [RFC PATCH 1/3] drm/amdgpu: implement ring init_priority for compute ring

[Luben Tuikov]

On 2020-02-26 3:37 p.m., Nirmoy Das wrote:
> init_priority will set second compute queue(gfx8 and gfx9) of a pipe to high priority
> and 1st queue to normal priority.
> 
> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h |  1 +
>  drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c    | 14 ++++++++++++++
>  drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c    | 13 +++++++++++++
>  3 files changed, 28 insertions(+)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
> index 24caff085d00..a109373b9fe8 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
> @@ -170,6 +170,7 @@ struct amdgpu_ring_funcs {
>  	/* priority functions */
>  	void (*set_priority) (struct amdgpu_ring *ring,
>  			      enum drm_sched_priority priority);
> +	void (*init_priority) (struct amdgpu_ring *ring);
>  	/* Try to soft recover the ring to make the fence signal */
>  	void (*soft_recovery)(struct amdgpu_ring *ring, unsigned vmid);
>  	int (*preempt_ib)(struct amdgpu_ring *ring);
> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
> index fa245973de12..14bab6e08bd6 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
> @@ -6334,6 +6334,19 @@ static void gfx_v8_0_ring_set_priority_compute(struct amdgpu_ring *ring,
>  	gfx_v8_0_pipe_reserve_resources(adev, ring, acquire);
>  }
> 
> +static void gfx_v8_0_ring_init_priority_compute(struct amdgpu_ring *ring)

Why two verbs in this function: "init" and "compute"?
Certainly there is no need for "compute".
Just call it

gfx_blah_ring_priority_init()

Putting the object first and the verb last, as is commonly done.

> +{
> +	/* set pipe 0 to normal priority and pipe 1 to high priority*/
> +	if (ring->queue == 1) {
> +		gfx_v8_0_hqd_set_priority(ring->adev, ring, true);
> +		gfx_v8_0_ring_set_pipe_percent(ring, true);
> +	} else {
> +		gfx_v8_0_hqd_set_priority(ring->adev, ring, false);
> +		gfx_v8_0_ring_set_pipe_percent(ring, false);
> +	}
> +
> +}

Again. Notice that the only difference between the two outcomes
of the conditional is the last parameter to both.

So you could write your code like this:

gfx_v8_0_hqd_set_priority(ring->adev, ring, ring->queue == 1);
gfx_v8_0_ring_set_pipe_percent(ring, ring->queue == 1);

Further more if "priority" had to be variable value,
I'd parameterize it in a map (i.e. array) and use
a computed index to index the map in order to retrieve
the variabled "priority". This eliminates if-conditionals.

Note in general that we want less if-conditionals,
in the execution path and more streamline execution.

> +
>  static void gfx_v8_0_ring_emit_fence_compute(struct amdgpu_ring *ring,
>  					     u64 addr, u64 seq,
>  					     unsigned flags)
> @@ -6967,6 +6980,7 @@ static const struct amdgpu_ring_funcs gfx_v8_0_ring_funcs_compute = {
>  	.insert_nop = amdgpu_ring_insert_nop,
>  	.pad_ib = amdgpu_ring_generic_pad_ib,
>  	.set_priority = gfx_v8_0_ring_set_priority_compute,
> +	.init_priority = gfx_v8_0_ring_init_priority_compute,
>  	.emit_wreg = gfx_v8_0_ring_emit_wreg,
>  };
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
> index 1c7a16b91686..0c66743fb6f5 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
> @@ -5143,6 +5143,18 @@ static void gfx_v9_0_ring_set_priority_compute(struct amdgpu_ring *ring,
>  	gfx_v9_0_pipe_reserve_resources(adev, ring, acquire);
>  }
> 
> +static void gfx_v9_0_ring_init_priority_compute(struct amdgpu_ring *ring)

Ditto for this name as per above.

> +{
> +	/* set pipe 0 to normal priority and pipe 1 to high priority*/
> +	if (ring->queue == 1) {
> +		gfx_v9_0_hqd_set_priority(ring->adev, ring, true);
> +		gfx_v9_0_ring_set_pipe_percent(ring, true);
> +	} else {
> +		gfx_v9_0_hqd_set_priority(ring->adev, ring, false);
> +		gfx_v9_0_ring_set_pipe_percent(ring, true);
> +	}
> +}

Note how similar this is to the v8 above?
Those could be made the same and he vX parameterized to
the correct implementation.

For instance, if you parameterize the "gfx_vX_0_hqd_set_priority()"
and "gfx_vX_0_ring_set_pipe_percent()". Then your code becomes,
like this pseudo-code:

ring_init_set_priority(ring)
{
    map = ring->property[ring->version];

    map->hqd_priority_set(ring->adev, ring, ring->queue == 1);
    map->ring_pipe_percent_set(ring, ring->queue == 1);
}

Regards,
Luben


> +
>  static void gfx_v9_0_ring_set_wptr_compute(struct amdgpu_ring *ring)
>  {
>  	struct amdgpu_device *adev = ring->adev;
> @@ -6514,6 +6526,7 @@ static const struct amdgpu_ring_funcs gfx_v9_0_ring_funcs_compute = {
>  	.insert_nop = amdgpu_ring_insert_nop,
>  	.pad_ib = amdgpu_ring_generic_pad_ib,
>  	.set_priority = gfx_v9_0_ring_set_priority_compute,
> +	.init_priority = gfx_v9_0_ring_init_priority_compute,
>  	.emit_wreg = gfx_v9_0_ring_emit_wreg,
>  	.emit_reg_wait = gfx_v9_0_ring_emit_reg_wait,
>  	.emit_reg_write_reg_wait = gfx_v9_0_ring_emit_reg_write_reg_wait,
> --
> 2.25.0
> 
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&amp;data=02%7C01%7Cluben.tuikov%40amd.com%7Cfb0f769b48da4c3c390108d7bafb4e1b%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637183460845495056&amp;sdata=qqzOg3W%2FvkOrG2eglBM7NmByS1ZreZAfigOJWFgA1Hw%3D&amp;reserved=0
> 

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [RFC PATCH 1/3] drm/amdgpu: implement ring init_priority for compute ring

[Christian König]

Am 28.02.20 um 04:29 schrieb Luben Tuikov:
> On 2020-02-26 3:37 p.m., Nirmoy Das wrote:
>> init_priority will set second compute queue(gfx8 and gfx9) of a pipe to high priority
>> and 1st queue to normal priority.
>>
>> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h |  1 +
>>   drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c    | 14 ++++++++++++++
>>   drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c    | 13 +++++++++++++
>>   3 files changed, 28 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
>> index 24caff085d00..a109373b9fe8 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
>> @@ -170,6 +170,7 @@ struct amdgpu_ring_funcs {
>>   	/* priority functions */
>>   	void (*set_priority) (struct amdgpu_ring *ring,
>>   			      enum drm_sched_priority priority);
>> +	void (*init_priority) (struct amdgpu_ring *ring);
>>   	/* Try to soft recover the ring to make the fence signal */
>>   	void (*soft_recovery)(struct amdgpu_ring *ring, unsigned vmid);
>>   	int (*preempt_ib)(struct amdgpu_ring *ring);
>> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
>> index fa245973de12..14bab6e08bd6 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
>> @@ -6334,6 +6334,19 @@ static void gfx_v8_0_ring_set_priority_compute(struct amdgpu_ring *ring,
>>   	gfx_v8_0_pipe_reserve_resources(adev, ring, acquire);
>>   }
>>
>> +static void gfx_v8_0_ring_init_priority_compute(struct amdgpu_ring *ring)
> Why two verbs in this function: "init" and "compute"?

Compute is not a verb here but rather the description of the ring type.

> Certainly there is no need for "compute".
> Just call it
>
> gfx_blah_ring_priority_init()

I would call it gfx_*_init_compute_ring_priority().

>
> Putting the object first and the verb last, as is commonly done.

We need to make sure that we note that this is for the compute rings.

Regards,
Christian.

>
>> +{
>> +	/* set pipe 0 to normal priority and pipe 1 to high priority*/
>> +	if (ring->queue == 1) {
>> +		gfx_v8_0_hqd_set_priority(ring->adev, ring, true);
>> +		gfx_v8_0_ring_set_pipe_percent(ring, true);
>> +	} else {
>> +		gfx_v8_0_hqd_set_priority(ring->adev, ring, false);
>> +		gfx_v8_0_ring_set_pipe_percent(ring, false);
>> +	}
>> +
>> +}
> Again. Notice that the only difference between the two outcomes
> of the conditional is the last parameter to both.
>
> So you could write your code like this:
>
> gfx_v8_0_hqd_set_priority(ring->adev, ring, ring->queue == 1);
> gfx_v8_0_ring_set_pipe_percent(ring, ring->queue == 1);
>
> Further more if "priority" had to be variable value,
> I'd parameterize it in a map (i.e. array) and use
> a computed index to index the map in order to retrieve
> the variabled "priority". This eliminates if-conditionals.
>
> Note in general that we want less if-conditionals,
> in the execution path and more streamline execution.
>
>> +
>>   static void gfx_v8_0_ring_emit_fence_compute(struct amdgpu_ring *ring,
>>   					     u64 addr, u64 seq,
>>   					     unsigned flags)
>> @@ -6967,6 +6980,7 @@ static const struct amdgpu_ring_funcs gfx_v8_0_ring_funcs_compute = {
>>   	.insert_nop = amdgpu_ring_insert_nop,
>>   	.pad_ib = amdgpu_ring_generic_pad_ib,
>>   	.set_priority = gfx_v8_0_ring_set_priority_compute,
>> +	.init_priority = gfx_v8_0_ring_init_priority_compute,
>>   	.emit_wreg = gfx_v8_0_ring_emit_wreg,
>>   };
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
>> index 1c7a16b91686..0c66743fb6f5 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
>> @@ -5143,6 +5143,18 @@ static void gfx_v9_0_ring_set_priority_compute(struct amdgpu_ring *ring,
>>   	gfx_v9_0_pipe_reserve_resources(adev, ring, acquire);
>>   }
>>
>> +static void gfx_v9_0_ring_init_priority_compute(struct amdgpu_ring *ring)
> Ditto for this name as per above.
>
>> +{
>> +	/* set pipe 0 to normal priority and pipe 1 to high priority*/
>> +	if (ring->queue == 1) {
>> +		gfx_v9_0_hqd_set_priority(ring->adev, ring, true);
>> +		gfx_v9_0_ring_set_pipe_percent(ring, true);
>> +	} else {
>> +		gfx_v9_0_hqd_set_priority(ring->adev, ring, false);
>> +		gfx_v9_0_ring_set_pipe_percent(ring, true);
>> +	}
>> +}
> Note how similar this is to the v8 above?
> Those could be made the same and he vX parameterized to
> the correct implementation.
>
> For instance, if you parameterize the "gfx_vX_0_hqd_set_priority()"
> and "gfx_vX_0_ring_set_pipe_percent()". Then your code becomes,
> like this pseudo-code:
>
> ring_init_set_priority(ring)
> {
>      map = ring->property[ring->version];
>
>      map->hqd_priority_set(ring->adev, ring, ring->queue == 1);
>      map->ring_pipe_percent_set(ring, ring->queue == 1);
> }
>
> Regards,
> Luben
>
>
>> +
>>   static void gfx_v9_0_ring_set_wptr_compute(struct amdgpu_ring *ring)
>>   {
>>   	struct amdgpu_device *adev = ring->adev;
>> @@ -6514,6 +6526,7 @@ static const struct amdgpu_ring_funcs gfx_v9_0_ring_funcs_compute = {
>>   	.insert_nop = amdgpu_ring_insert_nop,
>>   	.pad_ib = amdgpu_ring_generic_pad_ib,
>>   	.set_priority = gfx_v9_0_ring_set_priority_compute,
>> +	.init_priority = gfx_v9_0_ring_init_priority_compute,
>>   	.emit_wreg = gfx_v9_0_ring_emit_wreg,
>>   	.emit_reg_wait = gfx_v9_0_ring_emit_reg_wait,
>>   	.emit_reg_write_reg_wait = gfx_v9_0_ring_emit_reg_write_reg_wait,
>> --
>> 2.25.0
>>
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx@lists.freedesktop.org
>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&amp;data=02%7C01%7Cluben.tuikov%40amd.com%7Cfb0f769b48da4c3c390108d7bafb4e1b%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637183460845495056&amp;sdata=qqzOg3W%2FvkOrG2eglBM7NmByS1ZreZAfigOJWFgA1Hw%3D&amp;reserved=0
>>

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [RFC PATCH 1/3] drm/amdgpu: implement ring init_priority for compute ring

[Luben Tuikov]

On 2020-02-28 2:38 a.m., Christian König wrote:
> Am 28.02.20 um 04:29 schrieb Luben Tuikov:
>> On 2020-02-26 3:37 p.m., Nirmoy Das wrote:
>>> init_priority will set second compute queue(gfx8 and gfx9) of a pipe to high priority
>>> and 1st queue to normal priority.
>>>
>>> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
>>> ---
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h |  1 +
>>>   drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c    | 14 ++++++++++++++
>>>   drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c    | 13 +++++++++++++
>>>   3 files changed, 28 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
>>> index 24caff085d00..a109373b9fe8 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
>>> @@ -170,6 +170,7 @@ struct amdgpu_ring_funcs {
>>>   	/* priority functions */
>>>   	void (*set_priority) (struct amdgpu_ring *ring,
>>>   			      enum drm_sched_priority priority);
>>> +	void (*init_priority) (struct amdgpu_ring *ring);
>>>   	/* Try to soft recover the ring to make the fence signal */
>>>   	void (*soft_recovery)(struct amdgpu_ring *ring, unsigned vmid);
>>>   	int (*preempt_ib)(struct amdgpu_ring *ring);
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
>>> index fa245973de12..14bab6e08bd6 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
>>> @@ -6334,6 +6334,19 @@ static void gfx_v8_0_ring_set_priority_compute(struct amdgpu_ring *ring,
>>>   	gfx_v8_0_pipe_reserve_resources(adev, ring, acquire);
>>>   }
>>>
>>> +static void gfx_v8_0_ring_init_priority_compute(struct amdgpu_ring *ring)
>> Why two verbs in this function: "init" and "compute"?
> 
> Compute is not a verb here but rather the description of the ring type.
> 
>> Certainly there is no need for "compute".
>> Just call it
>>
>> gfx_blah_ring_priority_init()
> 
> I would call it gfx_*_init_compute_ring_priority().

That's better. Can we abbreviate it so that the name
isn't that long? We do have abbreviations all over the code,
so "cr" for "compute_ring" would probably be okay.
Then we can use "CR" in comments and "cr" in code to mean
"compute_ring".

gfx_*_init_cr_prio()

Regards,
Luben

> 
>>
>> Putting the object first and the verb last, as is commonly done.
> 
> We need to make sure that we note that this is for the compute rings.
> 
> Regards,
> Christian.
> 
>>
>>> +{
>>> +	/* set pipe 0 to normal priority and pipe 1 to high priority*/
>>> +	if (ring->queue == 1) {
>>> +		gfx_v8_0_hqd_set_priority(ring->adev, ring, true);
>>> +		gfx_v8_0_ring_set_pipe_percent(ring, true);
>>> +	} else {
>>> +		gfx_v8_0_hqd_set_priority(ring->adev, ring, false);
>>> +		gfx_v8_0_ring_set_pipe_percent(ring, false);
>>> +	}
>>> +
>>> +}
>> Again. Notice that the only difference between the two outcomes
>> of the conditional is the last parameter to both.
>>
>> So you could write your code like this:
>>
>> gfx_v8_0_hqd_set_priority(ring->adev, ring, ring->queue == 1);
>> gfx_v8_0_ring_set_pipe_percent(ring, ring->queue == 1);
>>
>> Further more if "priority" had to be variable value,
>> I'd parameterize it in a map (i.e. array) and use
>> a computed index to index the map in order to retrieve
>> the variabled "priority". This eliminates if-conditionals.
>>
>> Note in general that we want less if-conditionals,
>> in the execution path and more streamline execution.
>>
>>> +
>>>   static void gfx_v8_0_ring_emit_fence_compute(struct amdgpu_ring *ring,
>>>   					     u64 addr, u64 seq,
>>>   					     unsigned flags)
>>> @@ -6967,6 +6980,7 @@ static const struct amdgpu_ring_funcs gfx_v8_0_ring_funcs_compute = {
>>>   	.insert_nop = amdgpu_ring_insert_nop,
>>>   	.pad_ib = amdgpu_ring_generic_pad_ib,
>>>   	.set_priority = gfx_v8_0_ring_set_priority_compute,
>>> +	.init_priority = gfx_v8_0_ring_init_priority_compute,
>>>   	.emit_wreg = gfx_v8_0_ring_emit_wreg,
>>>   };
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
>>> index 1c7a16b91686..0c66743fb6f5 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
>>> @@ -5143,6 +5143,18 @@ static void gfx_v9_0_ring_set_priority_compute(struct amdgpu_ring *ring,
>>>   	gfx_v9_0_pipe_reserve_resources(adev, ring, acquire);
>>>   }
>>>
>>> +static void gfx_v9_0_ring_init_priority_compute(struct amdgpu_ring *ring)
>> Ditto for this name as per above.
>>
>>> +{
>>> +	/* set pipe 0 to normal priority and pipe 1 to high priority*/
>>> +	if (ring->queue == 1) {
>>> +		gfx_v9_0_hqd_set_priority(ring->adev, ring, true);
>>> +		gfx_v9_0_ring_set_pipe_percent(ring, true);
>>> +	} else {
>>> +		gfx_v9_0_hqd_set_priority(ring->adev, ring, false);
>>> +		gfx_v9_0_ring_set_pipe_percent(ring, true);
>>> +	}
>>> +}
>> Note how similar this is to the v8 above?
>> Those could be made the same and he vX parameterized to
>> the correct implementation.
>>
>> For instance, if you parameterize the "gfx_vX_0_hqd_set_priority()"
>> and "gfx_vX_0_ring_set_pipe_percent()". Then your code becomes,
>> like this pseudo-code:
>>
>> ring_init_set_priority(ring)
>> {
>>      map = ring->property[ring->version];
>>
>>      map->hqd_priority_set(ring->adev, ring, ring->queue == 1);
>>      map->ring_pipe_percent_set(ring, ring->queue == 1);
>> }
>>
>> Regards,
>> Luben
>>
>>
>>> +
>>>   static void gfx_v9_0_ring_set_wptr_compute(struct amdgpu_ring *ring)
>>>   {
>>>   	struct amdgpu_device *adev = ring->adev;
>>> @@ -6514,6 +6526,7 @@ static const struct amdgpu_ring_funcs gfx_v9_0_ring_funcs_compute = {
>>>   	.insert_nop = amdgpu_ring_insert_nop,
>>>   	.pad_ib = amdgpu_ring_generic_pad_ib,
>>>   	.set_priority = gfx_v9_0_ring_set_priority_compute,
>>> +	.init_priority = gfx_v9_0_ring_init_priority_compute,
>>>   	.emit_wreg = gfx_v9_0_ring_emit_wreg,
>>>   	.emit_reg_wait = gfx_v9_0_ring_emit_reg_wait,
>>>   	.emit_reg_write_reg_wait = gfx_v9_0_ring_emit_reg_write_reg_wait,
>>> --
>>> 2.25.0
>>>
>>> _______________________________________________
>>> amd-gfx mailing list
>>> amd-gfx@lists.freedesktop.org
>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&amp;data=02%7C01%7Cluben.tuikov%40amd.com%7Cfb0f769b48da4c3c390108d7bafb4e1b%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637183460845495056&amp;sdata=qqzOg3W%2FvkOrG2eglBM7NmByS1ZreZAfigOJWFgA1Hw%3D&amp;reserved=0
>>>
> 

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [RFC PATCH 1/3] drm/amdgpu: implement ring init_priority for compute ring

[Alex Deucher]

On Mon, Mar 2, 2020 at 3:25 PM Luben Tuikov <luben.tuikov@amd.com> wrote:
>
> On 2020-02-28 2:38 a.m., Christian König wrote:
> > Am 28.02.20 um 04:29 schrieb Luben Tuikov:
> >> On 2020-02-26 3:37 p.m., Nirmoy Das wrote:
> >>> init_priority will set second compute queue(gfx8 and gfx9) of a pipe to high priority
> >>> and 1st queue to normal priority.
> >>>
> >>> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
> >>> ---
> >>>   drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h |  1 +
> >>>   drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c    | 14 ++++++++++++++
> >>>   drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c    | 13 +++++++++++++
> >>>   3 files changed, 28 insertions(+)
> >>>
> >>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
> >>> index 24caff085d00..a109373b9fe8 100644
> >>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
> >>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
> >>> @@ -170,6 +170,7 @@ struct amdgpu_ring_funcs {
> >>>     /* priority functions */
> >>>     void (*set_priority) (struct amdgpu_ring *ring,
> >>>                           enum drm_sched_priority priority);
> >>> +   void (*init_priority) (struct amdgpu_ring *ring);
> >>>     /* Try to soft recover the ring to make the fence signal */
> >>>     void (*soft_recovery)(struct amdgpu_ring *ring, unsigned vmid);
> >>>     int (*preempt_ib)(struct amdgpu_ring *ring);
> >>> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
> >>> index fa245973de12..14bab6e08bd6 100644
> >>> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
> >>> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
> >>> @@ -6334,6 +6334,19 @@ static void gfx_v8_0_ring_set_priority_compute(struct amdgpu_ring *ring,
> >>>     gfx_v8_0_pipe_reserve_resources(adev, ring, acquire);
> >>>   }
> >>>
> >>> +static void gfx_v8_0_ring_init_priority_compute(struct amdgpu_ring *ring)
> >> Why two verbs in this function: "init" and "compute"?
> >
> > Compute is not a verb here but rather the description of the ring type.
> >
> >> Certainly there is no need for "compute".
> >> Just call it
> >>
> >> gfx_blah_ring_priority_init()
> >
> > I would call it gfx_*_init_compute_ring_priority().
>
> That's better. Can we abbreviate it so that the name
> isn't that long? We do have abbreviations all over the code,
> so "cr" for "compute_ring" would probably be okay.
> Then we can use "CR" in comments and "cr" in code to mean
> "compute_ring".
>
> gfx_*_init_cr_prio()

It's not that much more typing and then it makes things very clear
what we are talking about.  cr seems too confusing for a minimal gain
in reduced typing.  Plus, it would be inconsistent with the naming
across the other gfx functions.

Alex

>
> Regards,
> Luben
>
> >
> >>
> >> Putting the object first and the verb last, as is commonly done.
> >
> > We need to make sure that we note that this is for the compute rings.
> >
> > Regards,
> > Christian.
> >
> >>
> >>> +{
> >>> +   /* set pipe 0 to normal priority and pipe 1 to high priority*/
> >>> +   if (ring->queue == 1) {
> >>> +           gfx_v8_0_hqd_set_priority(ring->adev, ring, true);
> >>> +           gfx_v8_0_ring_set_pipe_percent(ring, true);
> >>> +   } else {
> >>> +           gfx_v8_0_hqd_set_priority(ring->adev, ring, false);
> >>> +           gfx_v8_0_ring_set_pipe_percent(ring, false);
> >>> +   }
> >>> +
> >>> +}
> >> Again. Notice that the only difference between the two outcomes
> >> of the conditional is the last parameter to both.
> >>
> >> So you could write your code like this:
> >>
> >> gfx_v8_0_hqd_set_priority(ring->adev, ring, ring->queue == 1);
> >> gfx_v8_0_ring_set_pipe_percent(ring, ring->queue == 1);
> >>
> >> Further more if "priority" had to be variable value,
> >> I'd parameterize it in a map (i.e. array) and use
> >> a computed index to index the map in order to retrieve
> >> the variabled "priority". This eliminates if-conditionals.
> >>
> >> Note in general that we want less if-conditionals,
> >> in the execution path and more streamline execution.
> >>
> >>> +
> >>>   static void gfx_v8_0_ring_emit_fence_compute(struct amdgpu_ring *ring,
> >>>                                          u64 addr, u64 seq,
> >>>                                          unsigned flags)
> >>> @@ -6967,6 +6980,7 @@ static const struct amdgpu_ring_funcs gfx_v8_0_ring_funcs_compute = {
> >>>     .insert_nop = amdgpu_ring_insert_nop,
> >>>     .pad_ib = amdgpu_ring_generic_pad_ib,
> >>>     .set_priority = gfx_v8_0_ring_set_priority_compute,
> >>> +   .init_priority = gfx_v8_0_ring_init_priority_compute,
> >>>     .emit_wreg = gfx_v8_0_ring_emit_wreg,
> >>>   };
> >>>
> >>> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
> >>> index 1c7a16b91686..0c66743fb6f5 100644
> >>> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
> >>> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
> >>> @@ -5143,6 +5143,18 @@ static void gfx_v9_0_ring_set_priority_compute(struct amdgpu_ring *ring,
> >>>     gfx_v9_0_pipe_reserve_resources(adev, ring, acquire);
> >>>   }
> >>>
> >>> +static void gfx_v9_0_ring_init_priority_compute(struct amdgpu_ring *ring)
> >> Ditto for this name as per above.
> >>
> >>> +{
> >>> +   /* set pipe 0 to normal priority and pipe 1 to high priority*/
> >>> +   if (ring->queue == 1) {
> >>> +           gfx_v9_0_hqd_set_priority(ring->adev, ring, true);
> >>> +           gfx_v9_0_ring_set_pipe_percent(ring, true);
> >>> +   } else {
> >>> +           gfx_v9_0_hqd_set_priority(ring->adev, ring, false);
> >>> +           gfx_v9_0_ring_set_pipe_percent(ring, true);
> >>> +   }
> >>> +}
> >> Note how similar this is to the v8 above?
> >> Those could be made the same and he vX parameterized to
> >> the correct implementation.
> >>
> >> For instance, if you parameterize the "gfx_vX_0_hqd_set_priority()"
> >> and "gfx_vX_0_ring_set_pipe_percent()". Then your code becomes,
> >> like this pseudo-code:
> >>
> >> ring_init_set_priority(ring)
> >> {
> >>      map = ring->property[ring->version];
> >>
> >>      map->hqd_priority_set(ring->adev, ring, ring->queue == 1);
> >>      map->ring_pipe_percent_set(ring, ring->queue == 1);
> >> }
> >>
> >> Regards,
> >> Luben
> >>
> >>
> >>> +
> >>>   static void gfx_v9_0_ring_set_wptr_compute(struct amdgpu_ring *ring)
> >>>   {
> >>>     struct amdgpu_device *adev = ring->adev;
> >>> @@ -6514,6 +6526,7 @@ static const struct amdgpu_ring_funcs gfx_v9_0_ring_funcs_compute = {
> >>>     .insert_nop = amdgpu_ring_insert_nop,
> >>>     .pad_ib = amdgpu_ring_generic_pad_ib,
> >>>     .set_priority = gfx_v9_0_ring_set_priority_compute,
> >>> +   .init_priority = gfx_v9_0_ring_init_priority_compute,
> >>>     .emit_wreg = gfx_v9_0_ring_emit_wreg,
> >>>     .emit_reg_wait = gfx_v9_0_ring_emit_reg_wait,
> >>>     .emit_reg_write_reg_wait = gfx_v9_0_ring_emit_reg_write_reg_wait,
> >>> --
> >>> 2.25.0
> >>>
> >>> _______________________________________________
> >>> amd-gfx mailing list
> >>> amd-gfx@lists.freedesktop.org
> >>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&amp;data=02%7C01%7Cluben.tuikov%40amd.com%7Cfb0f769b48da4c3c390108d7bafb4e1b%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637183460845495056&amp;sdata=qqzOg3W%2FvkOrG2eglBM7NmByS1ZreZAfigOJWFgA1Hw%3D&amp;reserved=0
> >>>
> >
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[RFC PATCH 2/3] drm/amdgpu: change hw sched list on ctx priority override

[Nirmoy Das]

We were changing compute ring priority while rings were being used
before every job submission which is not recommended. This patch
recreates entity with higher/normal priority sched list when user
changes ctx's priority.

high/normal priority sched list are generated from set of high/normal
priority compute queues. When there are no high priority hw queues then
it fall backs to software priority.

Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c   |  4 --
 drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c  | 58 ++++++++++++++++++++++--
 drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h  |  2 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_job.c  |  6 ---
 drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c |  4 ++
 5 files changed, 59 insertions(+), 15 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..ea4dc57d2237 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
@@ -85,8 +85,13 @@ 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) {
+				scheds = adev->gfx.compute_sched;
+				num_scheds = adev->gfx.num_compute_sched;
+			} else {
+				scheds = adev->gfx.compute_sched_high;
+				num_scheds = adev->gfx.num_compute_sched_high;
+			}
 			break;
 		case AMDGPU_HW_IP_DMA:
 			scheds = adev->sdma.sdma_sched;
@@ -502,6 +507,24 @@ struct dma_fence *amdgpu_ctx_get_fence(struct amdgpu_ctx *ctx,
 	return fence;
 }
 
+static void amdgpu_ctx_hw_priority_override(struct amdgpu_ctx *ctx,
+					    const u32 hw_ip,
+					    enum drm_sched_priority priority)
+{
+	int i;
+
+	for (i = 0; i < amdgpu_ctx_num_entities[hw_ip]; ++i) {
+		if (!ctx->entities[hw_ip][i])
+			continue;
+
+		/* TODO what happens with prev scheduled jobs */
+		drm_sched_entity_destroy(&ctx->entities[hw_ip][i]->entity);
+		amdgpu_ctx_fini_entity(ctx->entities[hw_ip][i]);
+
+		amdgpu_ctx_init_entity(ctx, AMDGPU_HW_IP_COMPUTE, i);
+
+	}
+}
 void amdgpu_ctx_priority_override(struct amdgpu_ctx *ctx,
 				  enum drm_sched_priority priority)
 {
@@ -515,12 +538,18 @@ void amdgpu_ctx_priority_override(struct amdgpu_ctx *ctx,
 	for (i = 0; i < AMDGPU_HW_IP_NUM; ++i) {
 		for (j = 0; j < amdgpu_ctx_num_entities[i]; ++j) {
 			struct drm_sched_entity *entity;
+			struct amdgpu_ring *ring;
 
 			if (!ctx->entities[i][j])
 				continue;
 
 			entity = &ctx->entities[i][j]->entity;
-			drm_sched_entity_set_priority(entity, ctx_prio);
+			ring = to_amdgpu_ring(entity->rq->sched);
+
+			if (ring->funcs->init_priority)
+				amdgpu_ctx_hw_priority_override(ctx, i, priority);
+			else
+				drm_sched_entity_set_priority(entity, ctx_prio);
 		}
 	}
 }
@@ -630,6 +659,7 @@ void amdgpu_ctx_mgr_fini(struct amdgpu_ctx_mgr *mgr)
 
 void amdgpu_ctx_init_sched(struct amdgpu_device *adev)
 {
+	enum drm_sched_priority priority;
 	int i, j;
 
 	for (i = 0; i < adev->gfx.num_gfx_rings; i++) {
@@ -638,8 +668,26 @@ 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++;
+		priority = adev->gfx.compute_ring[i].priority;
+		if (priority <= DRM_SCHED_PRIORITY_NORMAL) {
+			adev->gfx.compute_sched[i] =
+				&adev->gfx.compute_ring[i].sched;
+			adev->gfx.num_compute_sched++;
+		} else {
+			adev->gfx.compute_sched_high[i] =
+				&adev->gfx.compute_ring[i].sched;
+			adev->gfx.num_compute_sched_high++;
+		}
+	}
+
+	/* if there are no high prio compute queue then mirror with normal
+	 * priority so amdgpu_ctx_init_entity() works as expected */
+	if (!adev->gfx.num_compute_sched_high) {
+		for (i = 0; i < adev->gfx.num_compute_sched; i++) {
+			adev->gfx.compute_sched_high[i] =
+			       adev->gfx.compute_sched[i];
+		}
+		adev->gfx.num_compute_sched_high = adev->gfx.num_compute_sched;
 	}
 
 	for (i = 0; i < adev->sdma.num_instances; i++) {
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h
index ca17ffb01301..d58d748e3a56 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h
@@ -279,7 +279,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;
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.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
index 18e11b0fdc3e..4501ae7afb2e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
@@ -326,6 +326,10 @@ int amdgpu_ring_init(struct amdgpu_device *adev, struct amdgpu_ring *ring,
 
 	ring->max_dw = max_dw;
 	ring->priority = DRM_SCHED_PRIORITY_NORMAL;
+	if (ring->funcs->type == AMDGPU_RING_TYPE_COMPUTE &&
+	    ring->funcs->init_priority)
+		ring->funcs->init_priority(ring);
+
 	mutex_init(&ring->priority_mutex);
 
 	for (i = 0; i < DRM_SCHED_PRIORITY_MAX; ++i)
-- 
2.25.0

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [RFC PATCH 2/3] drm/amdgpu: change hw sched list on ctx priority override

[Christian König]

Am 26.02.20 um 21:37 schrieb Nirmoy Das:
> We were changing compute ring priority while rings were being used
> before every job submission which is not recommended. This patch
> recreates entity with higher/normal priority sched list when user
> changes ctx's priority.
>
> high/normal priority sched list are generated from set of high/normal
> priority compute queues. When there are no high priority hw queues then
> it fall backs to software priority.
>
> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c   |  4 --
>   drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c  | 58 ++++++++++++++++++++++--
>   drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h  |  2 +
>   drivers/gpu/drm/amd/amdgpu/amdgpu_job.c  |  6 ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c |  4 ++
>   5 files changed, 59 insertions(+), 15 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..ea4dc57d2237 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
> @@ -85,8 +85,13 @@ 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) {
> +				scheds = adev->gfx.compute_sched;
> +				num_scheds = adev->gfx.num_compute_sched;
> +			} else {
> +				scheds = adev->gfx.compute_sched_high;
> +				num_scheds = adev->gfx.num_compute_sched_high;
> +			}
>   			break;
>   		case AMDGPU_HW_IP_DMA:
>   			scheds = adev->sdma.sdma_sched;
> @@ -502,6 +507,24 @@ struct dma_fence *amdgpu_ctx_get_fence(struct amdgpu_ctx *ctx,
>   	return fence;
>   }
>   
> +static void amdgpu_ctx_hw_priority_override(struct amdgpu_ctx *ctx,
> +					    const u32 hw_ip,
> +					    enum drm_sched_priority priority)
> +{
> +	int i;
> +
> +	for (i = 0; i < amdgpu_ctx_num_entities[hw_ip]; ++i) {
> +		if (!ctx->entities[hw_ip][i])
> +			continue;
> +
> +		/* TODO what happens with prev scheduled jobs */

If we do it right, that should be unproblematic.

The entity changes the rq/scheduler it submits stuff to only when it is 
idle, e.g. no jobs on the hardware nor software queue.

So changing the priority when there is still work should be ok because 
it won't take effect until the entity is idle.

Can of course be that userspace then wonders why the new priority 
doesn't take effect. But when you shoot yourself into the foot it is 
supposed to hurt, doesn't it?

> +		drm_sched_entity_destroy(&ctx->entities[hw_ip][i]->entity);
> +		amdgpu_ctx_fini_entity(ctx->entities[hw_ip][i]);
> +
> +		amdgpu_ctx_init_entity(ctx, AMDGPU_HW_IP_COMPUTE, i);

Well, that is most likely NOT the right way of doing it :) Destroying 
the entity with fini and reinit might cause quite a bunch of problems.

Could be that this works as well, but I would rather just assign 
sched_list and num_sched_list.

> +
> +	}
> +}
>   void amdgpu_ctx_priority_override(struct amdgpu_ctx *ctx,
>   				  enum drm_sched_priority priority)
>   {
> @@ -515,12 +538,18 @@ void amdgpu_ctx_priority_override(struct amdgpu_ctx *ctx,
>   	for (i = 0; i < AMDGPU_HW_IP_NUM; ++i) {
>   		for (j = 0; j < amdgpu_ctx_num_entities[i]; ++j) {
>   			struct drm_sched_entity *entity;
> +			struct amdgpu_ring *ring;
>   
>   			if (!ctx->entities[i][j])
>   				continue;
>   
>   			entity = &ctx->entities[i][j]->entity;
> -			drm_sched_entity_set_priority(entity, ctx_prio);
> +			ring = to_amdgpu_ring(entity->rq->sched);
> +
> +			if (ring->funcs->init_priority)

As Alex noted in patch #1 we need to do a bit different, but I'm also 
not 100% sure how.

Maybe ping Alex on this once more or just add a bool to ring->funcs 
indicating that we can do this.

> +				amdgpu_ctx_hw_priority_override(ctx, i, priority);
> +			else
> +				drm_sched_entity_set_priority(entity, ctx_prio);

It might be a good idea to do this anyway, even with the different 
hardware priorities around.

>   		}
>   	}
>   }
> @@ -630,6 +659,7 @@ void amdgpu_ctx_mgr_fini(struct amdgpu_ctx_mgr *mgr)
>   
>   void amdgpu_ctx_init_sched(struct amdgpu_device *adev)
>   {
> +	enum drm_sched_priority priority;
>   	int i, j;
>   
>   	for (i = 0; i < adev->gfx.num_gfx_rings; i++) {
> @@ -638,8 +668,26 @@ 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++;
> +		priority = adev->gfx.compute_ring[i].priority;
> +		if (priority <= DRM_SCHED_PRIORITY_NORMAL) {
> +			adev->gfx.compute_sched[i] =
> +				&adev->gfx.compute_ring[i].sched;
> +			adev->gfx.num_compute_sched++;
> +		} else {
> +			adev->gfx.compute_sched_high[i] =
> +				&adev->gfx.compute_ring[i].sched;
> +			adev->gfx.num_compute_sched_high++;
> +		}
> +	}
> +
> +	/* if there are no high prio compute queue then mirror with normal
> +	 * priority so amdgpu_ctx_init_entity() works as expected */
> +	if (!adev->gfx.num_compute_sched_high) {
> +		for (i = 0; i < adev->gfx.num_compute_sched; i++) {
> +			adev->gfx.compute_sched_high[i] =
> +			       adev->gfx.compute_sched[i];
> +		}
> +		adev->gfx.num_compute_sched_high = adev->gfx.num_compute_sched;

It might be a good idea to have this chunk in patch #1 instead.

Regards,
Christian.

>   	}
>   
>   	for (i = 0; i < adev->sdma.num_instances; i++) {
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h
> index ca17ffb01301..d58d748e3a56 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h
> @@ -279,7 +279,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;
> 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.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
> index 18e11b0fdc3e..4501ae7afb2e 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
> @@ -326,6 +326,10 @@ int amdgpu_ring_init(struct amdgpu_device *adev, struct amdgpu_ring *ring,
>   
>   	ring->max_dw = max_dw;
>   	ring->priority = DRM_SCHED_PRIORITY_NORMAL;
> +	if (ring->funcs->type == AMDGPU_RING_TYPE_COMPUTE &&
> +	    ring->funcs->init_priority)
> +		ring->funcs->init_priority(ring);
> +
>   	mutex_init(&ring->priority_mutex);
>   
>   	for (i = 0; i < DRM_SCHED_PRIORITY_MAX; ++i)

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [RFC PATCH 2/3] drm/amdgpu: change hw sched list on ctx priority override

[Nirmoy]

On 2/27/20 11:08 AM, Christian König wrote:
>
>>               scheds = adev->sdma.sdma_sched;
>> @@ -502,6 +507,24 @@ struct dma_fence *amdgpu_ctx_get_fence(struct 
>> amdgpu_ctx *ctx,
>>       return fence;
>>   }
>>   +static void amdgpu_ctx_hw_priority_override(struct amdgpu_ctx *ctx,
>> +                        const u32 hw_ip,
>> +                        enum drm_sched_priority priority)
>> +{
>> +    int i;
>> +
>> +    for (i = 0; i < amdgpu_ctx_num_entities[hw_ip]; ++i) {
>> +        if (!ctx->entities[hw_ip][i])
>> +            continue;
>> +
>> +        /* TODO what happens with prev scheduled jobs */
>
> If we do it right, that should be unproblematic.
>
> The entity changes the rq/scheduler it submits stuff to only when it 
> is idle, e.g. no jobs on the hardware nor software queue.
>
> So changing the priority when there is still work should be ok because 
> it won't take effect until the entity is idle.
Thanks clarifying that.
>
> Can of course be that userspace then wonders why the new priority 
> doesn't take effect. But when you shoot yourself into the foot it is 
> supposed to hurt, doesn't it?
  :D
>
>> + drm_sched_entity_destroy(&ctx->entities[hw_ip][i]->entity);
>> +        amdgpu_ctx_fini_entity(ctx->entities[hw_ip][i]);
>> +
>> +        amdgpu_ctx_init_entity(ctx, AMDGPU_HW_IP_COMPUTE, i);
>
> Well, that is most likely NOT the right way of doing it :) Destroying 
> the entity with fini and reinit might cause quite a bunch of problems.
>
> Could be that this works as well, but I would rather just assign 
> sched_list and num_sched_list.

How about doing that with a new function like 
drm_sched_entity_modify_sched() ?


_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [RFC PATCH 2/3] drm/amdgpu: change hw sched list on ctx priority override

[Christian König]

Am 27.02.20 um 11:26 schrieb Nirmoy:
>
> On 2/27/20 11:08 AM, Christian König wrote:
>>
>>>               scheds = adev->sdma.sdma_sched;
>>> @@ -502,6 +507,24 @@ struct dma_fence *amdgpu_ctx_get_fence(struct 
>>> amdgpu_ctx *ctx,
>>>       return fence;
>>>   }
>>>   +static void amdgpu_ctx_hw_priority_override(struct amdgpu_ctx *ctx,
>>> +                        const u32 hw_ip,
>>> +                        enum drm_sched_priority priority)
>>> +{
>>> +    int i;
>>> +
>>> +    for (i = 0; i < amdgpu_ctx_num_entities[hw_ip]; ++i) {
>>> +        if (!ctx->entities[hw_ip][i])
>>> +            continue;
>>> +
>>> +        /* TODO what happens with prev scheduled jobs */
>>
>> If we do it right, that should be unproblematic.
>>
>> The entity changes the rq/scheduler it submits stuff to only when it 
>> is idle, e.g. no jobs on the hardware nor software queue.
>>
>> So changing the priority when there is still work should be ok 
>> because it won't take effect until the entity is idle.
> Thanks clarifying that.
>>
>> Can of course be that userspace then wonders why the new priority 
>> doesn't take effect. But when you shoot yourself into the foot it is 
>> supposed to hurt, doesn't it?
>  :D
>>
>>> + drm_sched_entity_destroy(&ctx->entities[hw_ip][i]->entity);
>>> +        amdgpu_ctx_fini_entity(ctx->entities[hw_ip][i]);
>>> +
>>> +        amdgpu_ctx_init_entity(ctx, AMDGPU_HW_IP_COMPUTE, i);
>>
>> Well, that is most likely NOT the right way of doing it :) Destroying 
>> the entity with fini and reinit might cause quite a bunch of problems.
>>
>> Could be that this works as well, but I would rather just assign 
>> sched_list and num_sched_list.
>
> How about doing that with a new function like 
> drm_sched_entity_modify_sched() ?

Yes, sounds like the sanest thing to as well.

Christian.
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [RFC PATCH 2/3] drm/amdgpu: change hw sched list on ctx priority override

[Alex Deucher]

On Thu, Feb 27, 2020 at 5:08 AM Christian König
<christian.koenig@amd.com> wrote:
>
> Am 26.02.20 um 21:37 schrieb Nirmoy Das:
> > We were changing compute ring priority while rings were being used
> > before every job submission which is not recommended. This patch
> > recreates entity with higher/normal priority sched list when user
> > changes ctx's priority.
> >
> > high/normal priority sched list are generated from set of high/normal
> > priority compute queues. When there are no high priority hw queues then
> > it fall backs to software priority.
> >
> > Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
> > ---
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c   |  4 --
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c  | 58 ++++++++++++++++++++++--
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h  |  2 +
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_job.c  |  6 ---
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c |  4 ++
> >   5 files changed, 59 insertions(+), 15 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..ea4dc57d2237 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
> > @@ -85,8 +85,13 @@ 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) {
> > +                             scheds = adev->gfx.compute_sched;
> > +                             num_scheds = adev->gfx.num_compute_sched;
> > +                     } else {
> > +                             scheds = adev->gfx.compute_sched_high;
> > +                             num_scheds = adev->gfx.num_compute_sched_high;
> > +                     }
> >                       break;
> >               case AMDGPU_HW_IP_DMA:
> >                       scheds = adev->sdma.sdma_sched;
> > @@ -502,6 +507,24 @@ struct dma_fence *amdgpu_ctx_get_fence(struct amdgpu_ctx *ctx,
> >       return fence;
> >   }
> >
> > +static void amdgpu_ctx_hw_priority_override(struct amdgpu_ctx *ctx,
> > +                                         const u32 hw_ip,
> > +                                         enum drm_sched_priority priority)
> > +{
> > +     int i;
> > +
> > +     for (i = 0; i < amdgpu_ctx_num_entities[hw_ip]; ++i) {
> > +             if (!ctx->entities[hw_ip][i])
> > +                     continue;
> > +
> > +             /* TODO what happens with prev scheduled jobs */
>
> If we do it right, that should be unproblematic.
>
> The entity changes the rq/scheduler it submits stuff to only when it is
> idle, e.g. no jobs on the hardware nor software queue.
>
> So changing the priority when there is still work should be ok because
> it won't take effect until the entity is idle.
>
> Can of course be that userspace then wonders why the new priority
> doesn't take effect. But when you shoot yourself into the foot it is
> supposed to hurt, doesn't it?
>
> > +             drm_sched_entity_destroy(&ctx->entities[hw_ip][i]->entity);
> > +             amdgpu_ctx_fini_entity(ctx->entities[hw_ip][i]);
> > +
> > +             amdgpu_ctx_init_entity(ctx, AMDGPU_HW_IP_COMPUTE, i);
>
> Well, that is most likely NOT the right way of doing it :) Destroying
> the entity with fini and reinit might cause quite a bunch of problems.
>
> Could be that this works as well, but I would rather just assign
> sched_list and num_sched_list.
>
> > +
> > +     }
> > +}
> >   void amdgpu_ctx_priority_override(struct amdgpu_ctx *ctx,
> >                                 enum drm_sched_priority priority)
> >   {
> > @@ -515,12 +538,18 @@ void amdgpu_ctx_priority_override(struct amdgpu_ctx *ctx,
> >       for (i = 0; i < AMDGPU_HW_IP_NUM; ++i) {
> >               for (j = 0; j < amdgpu_ctx_num_entities[i]; ++j) {
> >                       struct drm_sched_entity *entity;
> > +                     struct amdgpu_ring *ring;
> >
> >                       if (!ctx->entities[i][j])
> >                               continue;
> >
> >                       entity = &ctx->entities[i][j]->entity;
> > -                     drm_sched_entity_set_priority(entity, ctx_prio);
> > +                     ring = to_amdgpu_ring(entity->rq->sched);
> > +
> > +                     if (ring->funcs->init_priority)
>
> As Alex noted in patch #1 we need to do a bit different, but I'm also
> not 100% sure how.
>

We shouldn't be changing this at runtime.  We need to set up the queue
priority at init time and then schedule to the appropriate quueue at
runtime.  We set the pipe/queue priority in the mqd (memory queue
descriptor).  When we init the rings we configure the mqds in memory
and then tell the CP to configure the rings.  The CP then fetches the
config from memory (the mqd) and pushes the configuration to the hqd
(hardware queue descriptor).  Currrently we just statically set up the
queues at driver init time, but the hw has the capability to schedule
queues dynamically at runtime.  E.g., we could have a per process mqd
for each queue and then tell the CP to schedule the mqd on the
hardware at runtime.  For now, I think we should just set up some
static pools of rings (e.g., normal and high priority or low, normal,
and high priorities).  Note that you probably want to keep the high
priority queues on a different pipe from the low/normal priority
queues.  Depending on the asic there are 1 or 2 MECs (compute micro
engines) and each MEC supports 4 pipes.  Each pipe can handle up to 8
queues.

Alex

> Maybe ping Alex on this once more or just add a bool to ring->funcs
> indicating that we can do this.
>
> > +                             amdgpu_ctx_hw_priority_override(ctx, i, priority);
> > +                     else
> > +                             drm_sched_entity_set_priority(entity, ctx_prio);
>
> It might be a good idea to do this anyway, even with the different
> hardware priorities around.
>
> >               }
> >       }
> >   }
> > @@ -630,6 +659,7 @@ void amdgpu_ctx_mgr_fini(struct amdgpu_ctx_mgr *mgr)
> >
> >   void amdgpu_ctx_init_sched(struct amdgpu_device *adev)
> >   {
> > +     enum drm_sched_priority priority;
> >       int i, j;
> >
> >       for (i = 0; i < adev->gfx.num_gfx_rings; i++) {
> > @@ -638,8 +668,26 @@ 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++;
> > +             priority = adev->gfx.compute_ring[i].priority;
> > +             if (priority <= DRM_SCHED_PRIORITY_NORMAL) {
> > +                     adev->gfx.compute_sched[i] =
> > +                             &adev->gfx.compute_ring[i].sched;
> > +                     adev->gfx.num_compute_sched++;
> > +             } else {
> > +                     adev->gfx.compute_sched_high[i] =
> > +                             &adev->gfx.compute_ring[i].sched;
> > +                     adev->gfx.num_compute_sched_high++;
> > +             }
> > +     }
> > +
> > +     /* if there are no high prio compute queue then mirror with normal
> > +      * priority so amdgpu_ctx_init_entity() works as expected */
> > +     if (!adev->gfx.num_compute_sched_high) {
> > +             for (i = 0; i < adev->gfx.num_compute_sched; i++) {
> > +                     adev->gfx.compute_sched_high[i] =
> > +                            adev->gfx.compute_sched[i];
> > +             }
> > +             adev->gfx.num_compute_sched_high = adev->gfx.num_compute_sched;
>
> It might be a good idea to have this chunk in patch #1 instead.
>
> Regards,
> Christian.
>
> >       }
> >
> >       for (i = 0; i < adev->sdma.num_instances; i++) {
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h
> > index ca17ffb01301..d58d748e3a56 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h
> > @@ -279,7 +279,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;
> > 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.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
> > index 18e11b0fdc3e..4501ae7afb2e 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
> > @@ -326,6 +326,10 @@ int amdgpu_ring_init(struct amdgpu_device *adev, struct amdgpu_ring *ring,
> >
> >       ring->max_dw = max_dw;
> >       ring->priority = DRM_SCHED_PRIORITY_NORMAL;
> > +     if (ring->funcs->type == AMDGPU_RING_TYPE_COMPUTE &&
> > +         ring->funcs->init_priority)
> > +             ring->funcs->init_priority(ring);
> > +
> >       mutex_init(&ring->priority_mutex);
> >
> >       for (i = 0; i < DRM_SCHED_PRIORITY_MAX; ++i)
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [RFC PATCH 2/3] drm/amdgpu: change hw sched list on ctx priority override

[Nirmoy]

On 2/27/20 3:35 PM, Alex Deucher wrote:
> We shouldn't be changing this at runtime.  We need to set up the queue
> priority at init time and then schedule to the appropriate quueue at
> runtime.  We set the pipe/queue priority in the mqd (memory queue
> descriptor).  When we init the rings we configure the mqds in memory
> and then tell the CP to configure the rings.  The CP then fetches the
> config from memory (the mqd) and pushes the configuration to the hqd
> (hardware queue descriptor).  Currrently we just statically set up the
> queues at driver init time, but the hw has the capability to schedule
> queues dynamically at runtime.  E.g., we could have a per process mqd
> for each queue and then tell the CP to schedule the mqd on the
> hardware at runtime.  For now, I think we should just set up some
> static pools of rings (e.g., normal and high priority or low, normal,
> and high priorities).  Note that you probably want to keep the high
> priority queues on a different pipe from the low/normal priority
> queues.  Depending on the asic there are 1 or 2 MECs (compute micro
> engines) and each MEC supports 4 pipes.  Each pipe can handle up to 8
> queues.

After some debugging I realized we have amdgpu_gfx_compute_queue_acquire()

which forces amdgpu to only use queue 0,1 of every pipe form MEC 0 even 
if we

have more than 1 MEC.


Does it make sense to have two high priority queue on the same pipe ?

Regards,

Nirmoy


> Alex
>
>>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [RFC PATCH 2/3] drm/amdgpu: change hw sched list on ctx priority override

[Alex Deucher]

On Thu, Feb 27, 2020 at 3:28 PM Nirmoy <nirmodas@amd.com> wrote:
>
>
> On 2/27/20 3:35 PM, Alex Deucher wrote:
> > We shouldn't be changing this at runtime.  We need to set up the queue
> > priority at init time and then schedule to the appropriate quueue at
> > runtime.  We set the pipe/queue priority in the mqd (memory queue
> > descriptor).  When we init the rings we configure the mqds in memory
> > and then tell the CP to configure the rings.  The CP then fetches the
> > config from memory (the mqd) and pushes the configuration to the hqd
> > (hardware queue descriptor).  Currrently we just statically set up the
> > queues at driver init time, but the hw has the capability to schedule
> > queues dynamically at runtime.  E.g., we could have a per process mqd
> > for each queue and then tell the CP to schedule the mqd on the
> > hardware at runtime.  For now, I think we should just set up some
> > static pools of rings (e.g., normal and high priority or low, normal,
> > and high priorities).  Note that you probably want to keep the high
> > priority queues on a different pipe from the low/normal priority
> > queues.  Depending on the asic there are 1 or 2 MECs (compute micro
> > engines) and each MEC supports 4 pipes.  Each pipe can handle up to 8
> > queues.
>
> After some debugging I realized we have amdgpu_gfx_compute_queue_acquire()
>
> which forces amdgpu to only use queue 0,1 of every pipe form MEC 0 even
> if we
>
> have more than 1 MEC.
>

IIRC, that is to spread the queues across as many pipes as possible.

>
> Does it make sense to have two high priority queue on the same pipe ?

Good question.  Not sure what the best option is for splitting up the
queues.  Maybe one set of queues (low and high) per pipe?

Alex

>
> Regards,
>
> Nirmoy
>
>
> > Alex
> >
> >>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [RFC PATCH 2/3] drm/amdgpu: change hw sched list on ctx priority override

[Nirmoy]

On 2/27/20 10:02 PM, Alex Deucher wrote:
> On Thu, Feb 27, 2020 at 3:28 PM Nirmoy <nirmodas@amd.com> wrote:
>>
>> On 2/27/20 3:35 PM, Alex Deucher wrote:
>>> We shouldn't be changing this at runtime.  We need to set up the queue
>>> priority at init time and then schedule to the appropriate quueue at
>>> runtime.  We set the pipe/queue priority in the mqd (memory queue
>>> descriptor).  When we init the rings we configure the mqds in memory
>>> and then tell the CP to configure the rings.  The CP then fetches the
>>> config from memory (the mqd) and pushes the configuration to the hqd
>>> (hardware queue descriptor).  Currrently we just statically set up the
>>> queues at driver init time, but the hw has the capability to schedule
>>> queues dynamically at runtime.  E.g., we could have a per process mqd
>>> for each queue and then tell the CP to schedule the mqd on the
>>> hardware at runtime.  For now, I think we should just set up some
>>> static pools of rings (e.g., normal and high priority or low, normal,
>>> and high priorities).  Note that you probably want to keep the high
>>> priority queues on a different pipe from the low/normal priority
>>> queues.  Depending on the asic there are 1 or 2 MECs (compute micro
>>> engines) and each MEC supports 4 pipes.  Each pipe can handle up to 8
>>> queues.
>> After some debugging I realized we have amdgpu_gfx_compute_queue_acquire()
>>
>> which forces amdgpu to only use queue 0,1 of every pipe form MEC 0 even
>> if we
>>
>> have more than 1 MEC.
>>
> IIRC, that is to spread the queues across as many pipes as possible.
okay
>
>> Does it make sense to have two high priority queue on the same pipe ?
> Good question.  Not sure what the best option is for splitting up the
> queues.  Maybe one set of queues (low and high) per pipe?

I think a low and high priority queue per pipe should work well AFAIU.


Nirmoy

>
> Alex
>
>> Regards,
>>
>> Nirmoy
>>
>>
>>> Alex
>>>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [RFC PATCH 2/3] drm/amdgpu: change hw sched list on ctx priority override

[Nirmoy]

Found two issues :

On 2/26/20 9:37 PM, Nirmoy Das wrote:
> +static void amdgpu_ctx_hw_priority_override(struct amdgpu_ctx *ctx,
> +					    const u32 hw_ip,
> +					    enum drm_sched_priority priority)
> +{
> +	int i;
> +
> +	for (i = 0; i < amdgpu_ctx_num_entities[hw_ip]; ++i) {
> +		if (!ctx->entities[hw_ip][i])
> +			continue;
> +
> +		/* TODO what happens with prev scheduled jobs */
> +		drm_sched_entity_destroy(&ctx->entities[hw_ip][i]->entity);
> +		amdgpu_ctx_fini_entity(ctx->entities[hw_ip][i]);
> +
> +		amdgpu_ctx_init_entity(ctx, AMDGPU_HW_IP_COMPUTE, i);
s/AMDGPU_HW_IP_COMPUTE/hw_ip
>   
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
> index 18e11b0fdc3e..4501ae7afb2e 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
> @@ -326,6 +326,10 @@ int amdgpu_ring_init(struct amdgpu_device *adev, struct amdgpu_ring *ring,
>   
>   	ring->max_dw = max_dw;
>   	ring->priority = DRM_SCHED_PRIORITY_NORMAL;
> +	if (ring->funcs->type == AMDGPU_RING_TYPE_COMPUTE &&
> +	    ring->funcs->init_priority)
I didn't set ring->priority to DRM_SCHED_PRIORITY_HIGH in the previous 
patch
> +		ring->funcs->init_priority(ring);
> +
>   	mutex_init(&ring->priority_mutex);
>   
>   	for (i = 0; i < DRM_SCHED_PRIORITY_MAX; ++i)
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [RFC PATCH 2/3] drm/amdgpu: change hw sched list on ctx priority override

[Luben Tuikov]

On 2020-02-26 3:37 p.m., Nirmoy Das wrote:
> We were changing compute ring priority while rings were being used
> before every job submission which is not recommended. This patch
> recreates entity with higher/normal priority sched list when user
> changes ctx's priority.
> 
> high/normal priority sched list are generated from set of high/normal
> priority compute queues. When there are no high priority hw queues then
> it fall backs to software priority.
> 
> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c   |  4 --
>  drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c  | 58 ++++++++++++++++++++++--
>  drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h  |  2 +
>  drivers/gpu/drm/amd/amdgpu/amdgpu_job.c  |  6 ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c |  4 ++
>  5 files changed, 59 insertions(+), 15 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..ea4dc57d2237 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
> @@ -85,8 +85,13 @@ 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) {
> +				scheds = adev->gfx.compute_sched;
> +				num_scheds = adev->gfx.num_compute_sched;
> +			} else {
> +				scheds = adev->gfx.compute_sched_high;
> +				num_scheds = adev->gfx.num_compute_sched_high;
> +			}

Why more if-conditionals?

If you initialize a map of DRM_SCHED_PRIORITY_MAX of type then you can simply do:

scheds = adev->gfx.map[priority];

So, part of your array would contain normal and the rest high.

Also, I don't think you should introduce yet another
compute_sched[] array. All this should be folded into
a single array containing both normal and high.

Also consider changing to this:

enum drm_sched_priority {
	DRM_SCHED_PRIORITY_UNSET,
--------DRM_SCHED_PRIORITY_INVALID,--------<--- remove
	DRM_SCHED_PRIORITY_MIN,
	DRM_SCHED_PRIORITY_LOW = DRM_SCHED_PRIORITY_MIN,
	DRM_SCHED_PRIORITY_NORMAL,
	DRM_SCHED_PRIORITY_HIGH_SW,
	DRM_SCHED_PRIORITY_HIGH_HW,
	DRM_SCHED_PRIORITY_KERNEL,
	DRM_SCHED_PRIORITY_MAX,
};

We should never have an "invalid priority", just ignored priority. :-)
Second, having 0 as UNSET gives you easy priority when you set
map[0] = normal_priority, as memory usually comes memset to 0.
IOW, you'd not need to check against UNSET, and simply use
the default [0] which you'd set to normal priority.

>  			break;
>  		case AMDGPU_HW_IP_DMA:
>  			scheds = adev->sdma.sdma_sched;
> @@ -502,6 +507,24 @@ struct dma_fence *amdgpu_ctx_get_fence(struct amdgpu_ctx *ctx,
>  	return fence;
>  }
>  
> +static void amdgpu_ctx_hw_priority_override(struct amdgpu_ctx *ctx,
> +					    const u32 hw_ip,
> +					    enum drm_sched_priority priority)
> +{
> +	int i;
> +
> +	for (i = 0; i < amdgpu_ctx_num_entities[hw_ip]; ++i) {
> +		if (!ctx->entities[hw_ip][i])
> +			continue;
> +
> +		/* TODO what happens with prev scheduled jobs */
> +		drm_sched_entity_destroy(&ctx->entities[hw_ip][i]->entity);
> +		amdgpu_ctx_fini_entity(ctx->entities[hw_ip][i]);
> +
> +		amdgpu_ctx_init_entity(ctx, AMDGPU_HW_IP_COMPUTE, i);
> +
> +	}
> +}
>  void amdgpu_ctx_priority_override(struct amdgpu_ctx *ctx,
>  				  enum drm_sched_priority priority)
>  {
> @@ -515,12 +538,18 @@ void amdgpu_ctx_priority_override(struct amdgpu_ctx *ctx,
>  	for (i = 0; i < AMDGPU_HW_IP_NUM; ++i) {
>  		for (j = 0; j < amdgpu_ctx_num_entities[i]; ++j) {
>  			struct drm_sched_entity *entity;
> +			struct amdgpu_ring *ring;
>  
>  			if (!ctx->entities[i][j])
>  				continue;
>  
>  			entity = &ctx->entities[i][j]->entity;
> -			drm_sched_entity_set_priority(entity, ctx_prio);
> +			ring = to_amdgpu_ring(entity->rq->sched);
> +
> +			if (ring->funcs->init_priority)
> +				amdgpu_ctx_hw_priority_override(ctx, i, priority);
> +			else
> +				drm_sched_entity_set_priority(entity, ctx_prio);

Why more if-conditionals?
Could we not have a map?

>  		}
>  	}
>  }
> @@ -630,6 +659,7 @@ void amdgpu_ctx_mgr_fini(struct amdgpu_ctx_mgr *mgr)
>  
>  void amdgpu_ctx_init_sched(struct amdgpu_device *adev)
>  {
> +	enum drm_sched_priority priority;
>  	int i, j;
>  
>  	for (i = 0; i < adev->gfx.num_gfx_rings; i++) {
> @@ -638,8 +668,26 @@ 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++;
> +		priority = adev->gfx.compute_ring[i].priority;
> +		if (priority <= DRM_SCHED_PRIORITY_NORMAL) {
> +			adev->gfx.compute_sched[i] =
> +				&adev->gfx.compute_ring[i].sched;
> +			adev->gfx.num_compute_sched++;
> +		} else {
> +			adev->gfx.compute_sched_high[i] =
> +				&adev->gfx.compute_ring[i].sched;
> +			adev->gfx.num_compute_sched_high++;
> +		}
> +	}

I think it would be better to use a map for this
as opposed to if-conditional.

> +
> +	/* if there are no high prio compute queue then mirror with normal
> +	 * priority so amdgpu_ctx_init_entity() works as expected */
> +	if (!adev->gfx.num_compute_sched_high) {
> +		for (i = 0; i < adev->gfx.num_compute_sched; i++) {
> +			adev->gfx.compute_sched_high[i] =
> +			       adev->gfx.compute_sched[i];
> +		}
> +		adev->gfx.num_compute_sched_high = adev->gfx.num_compute_sched;
>  	}
>  
>  	for (i = 0; i < adev->sdma.num_instances; i++) {
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h
> index ca17ffb01301..d58d748e3a56 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h
> @@ -279,7 +279,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;
> 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.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
> index 18e11b0fdc3e..4501ae7afb2e 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
> @@ -326,6 +326,10 @@ int amdgpu_ring_init(struct amdgpu_device *adev, struct amdgpu_ring *ring,
>  
>  	ring->max_dw = max_dw;
>  	ring->priority = DRM_SCHED_PRIORITY_NORMAL;
> +	if (ring->funcs->type == AMDGPU_RING_TYPE_COMPUTE &&
> +	    ring->funcs->init_priority)
> +		ring->funcs->init_priority(ring);
> +

Why not add "init_priority" to all and just call it here unconditionally?

Regards,
Luben

>  	mutex_init(&ring->priority_mutex);
>  
>  	for (i = 0; i < DRM_SCHED_PRIORITY_MAX; ++i)
> 

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[RFC PATCH 3/3] drm/amdgpu: remove unused functions

[Nirmoy Das]

Remove unused amdgpu_ring_priority_put() and amdgpu_ring_priority_get()

Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c | 70 ------------------------
 drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h |  4 --
 2 files changed, 74 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
index 4501ae7afb2e..8ac4b569c036 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
@@ -150,76 +150,6 @@ void amdgpu_ring_undo(struct amdgpu_ring *ring)
 		ring->funcs->end_use(ring);
 }
 
-/**
- * amdgpu_ring_priority_put - restore a ring's priority
- *
- * @ring: amdgpu_ring structure holding the information
- * @priority: target priority
- *
- * Release a request for executing at @priority
- */
-void amdgpu_ring_priority_put(struct amdgpu_ring *ring,
-			      enum drm_sched_priority priority)
-{
-	int i;
-
-	if (!ring->funcs->set_priority)
-		return;
-
-	if (atomic_dec_return(&ring->num_jobs[priority]) > 0)
-		return;
-
-	/* no need to restore if the job is already at the lowest priority */
-	if (priority == DRM_SCHED_PRIORITY_NORMAL)
-		return;
-
-	mutex_lock(&ring->priority_mutex);
-	/* something higher prio is executing, no need to decay */
-	if (ring->priority > priority)
-		goto out_unlock;
-
-	/* decay priority to the next level with a job available */
-	for (i = priority; i >= DRM_SCHED_PRIORITY_MIN; i--) {
-		if (i == DRM_SCHED_PRIORITY_NORMAL
-				|| atomic_read(&ring->num_jobs[i])) {
-			ring->priority = i;
-			ring->funcs->set_priority(ring, i);
-			break;
-		}
-	}
-
-out_unlock:
-	mutex_unlock(&ring->priority_mutex);
-}
-
-/**
- * amdgpu_ring_priority_get - change the ring's priority
- *
- * @ring: amdgpu_ring structure holding the information
- * @priority: target priority
- *
- * Request a ring's priority to be raised to @priority (refcounted).
- */
-void amdgpu_ring_priority_get(struct amdgpu_ring *ring,
-			      enum drm_sched_priority priority)
-{
-	if (!ring->funcs->set_priority)
-		return;
-
-	if (atomic_inc_return(&ring->num_jobs[priority]) <= 0)
-		return;
-
-	mutex_lock(&ring->priority_mutex);
-	if (priority <= ring->priority)
-		goto out_unlock;
-
-	ring->priority = priority;
-	ring->funcs->set_priority(ring, priority);
-
-out_unlock:
-	mutex_unlock(&ring->priority_mutex);
-}
-
 /**
  * amdgpu_ring_init - init driver ring struct.
  *
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
index a109373b9fe8..e6c3bcb990fd 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
@@ -259,10 +259,6 @@ void amdgpu_ring_insert_nop(struct amdgpu_ring *ring, uint32_t count);
 void amdgpu_ring_generic_pad_ib(struct amdgpu_ring *ring, struct amdgpu_ib *ib);
 void amdgpu_ring_commit(struct amdgpu_ring *ring);
 void amdgpu_ring_undo(struct amdgpu_ring *ring);
-void amdgpu_ring_priority_get(struct amdgpu_ring *ring,
-			      enum drm_sched_priority priority);
-void amdgpu_ring_priority_put(struct amdgpu_ring *ring,
-			      enum drm_sched_priority priority);
 int amdgpu_ring_init(struct amdgpu_device *adev, struct amdgpu_ring *ring,
 		     unsigned ring_size, struct amdgpu_irq_src *irq_src,
 		     unsigned irq_type);
-- 
2.25.0

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx