On Mon, Mar 2, 2020 at 3:25 PM Luben Tuikov <luben.tuikov@xxxxxxx> 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@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().
>
> 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@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
_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
