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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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()

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&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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux