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@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"?
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@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
