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@xxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxxxxxxxx
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=02%7C01%7Cluben.tuikov%40amd.com%7Cfb0f769b48da4c3c390108d7bafb4e1b%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637183460845495056&sdata=qqzOg3W%2FvkOrG2eglBM7NmByS1ZreZAfigOJWFgA1Hw%3D&reserved=0
>
_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
