That makes sense, Felix.
My big issue is that if I am looking for Hawaii, I go to gfx7, I don’t look in gfx8. And then I have to search the git history to see if it was a mistake, or intentional. As it stands, the consolidation of
code is just going to make things more confusing in the end when it comes to functions like this where GFX versions are not respected, which I think is what Felix is trying to get at. If the GFX name is in the function, it should apply to all ASICs in that
GFX family.
I understand that it can help to avoid issues where a bugfix goes into one gfx* file and is missed in the other (which might be the impetus for your cleanup efforts), but that’s mostly on the dev to do properly.
Otherwise anyone who is even reasonably new to the code is going to be completely confused, which is something we want to avoid. Even those who are working on the code daily will be chasing ghosts looking in the “wrong” gfx* files/functions trying to figure
out what’s happening.
All I want is clarity and consistency. If we’re using the GFX versions for naming, then let’s keep the functions and definitions correct, at the cost of duplicated code, because accuracy is key. Might mean
we have to undo some of Yong’s consolidation, but if the end result is correct and easily-understood code, then it’s worth it.
Kent
From: Kuehling, Felix <Felix.Kuehling@amd.com>
Sent: Thursday, November 7, 2019 12:47 PM
To: Zhao, Yong <Yong.Zhao@amd.com>; Russell, Kent <Kent.Russell@amd.com>; amd-gfx@lists.freedesktop.org
Subject: Re: [PATCH 2/3] drm/amdkfd: only keep release_mem function for Hawaii
No, please lets not add a new nomenclature for PM4 packet versions. GFX versions are agreed on between hardware, firmware, and software and it's generally understood what they mean. If we add a new PM4 packet versioning scheme on our own, then this will
add a lot of confusion when talking to firmware teams.
You know, this would all be more straight forward if we accepted a little bit of code duplication and had packet writing functions per GFX version. You'll see this pattern a lot in the amdgpu driver where each IP version duplicates a bunch of code. In many
cases you may be able to save a few lines of code by sharing functions between IP versions. But you'll add some confusion and burden on future maintenance.
This is the price we pay for micro-optimizing minor code duplication.
Regards,
Felix
On 2019-11-07 12:39, Zhao, Yong wrote:
Hi Kent,
I can't agree more on this. Also, the same applies to the file names. Definitely we need to agree on the naming scheme before making it happen.
Yong
On 2019-11-07 12:33 p.m., Russell, Kent wrote:
I think that the versioning is getting a little confusing since we’re using the old GFX versions, but not really sticking to it due to the shareability of certain managers and shaders. Could we look into doing something like gen1 or gen2, or some other more ambiguous non-GFX-related versioning? Otherwise we’ll keep having these questions of “why is Hawaii GFX8”, “why is Arcturus GFX9”, etc. Then if things change, we just up the value concretely, instead of maybe doing a v11 if GFX11 changes things, and only GFX11 ASICs use those functions/variables.
Obviously not high-priority, but maybe something to consider as you continue to consolidate and remove duplicate code.
Kent
From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of Zhao, Yong
Sent: Thursday, November 7, 2019 11:57 AM
To: Kuehling, Felix <Felix.Kuehling@amd.com>; amd-gfx@lists.freedesktop.org
Subject: Re: [PATCH 2/3] drm/amdkfd: only keep release_mem function for Hawaii
Hi Felix,
That's because v8 and v7 share the same packet_manager_funcs. In this case, it is better to keep it as it is.
Regards,
Yong
From: Kuehling, Felix <Felix.Kuehling@amd.com>
Sent: Wednesday, November 6, 2019 11:45 PM
To: Zhao, Yong <Yong.Zhao@amd.com>; amd-gfx@lists.freedesktop.org <amd-gfx@lists.freedesktop.org>
Subject: Re: [PATCH 2/3] drm/amdkfd: only keep release_mem function for Hawaii
On 2019-10-30 20:17, Zhao, Yong wrote:
> release_mem won't be used at all on GFX9 and GFX10, so delete it.
Hawaii was GFXv7. So we're not using the release_mem packet on GFXv8
either. Why arbitrarily limit this change to GFXv9 and 10?
Regards,
Felix
>
> Change-Id: I13787a8a29b83e7516c582a7401f2e14721edf5f
> Signed-off-by: Yong Zhao <Yong.Zhao@amd.com>
> ---
> .../gpu/drm/amd/amdkfd/kfd_kernel_queue_v10.c | 35 ++-----------------
> .../gpu/drm/amd/amdkfd/kfd_kernel_queue_v9.c | 33 ++---------------
> 2 files changed, 4 insertions(+), 64 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_kernel_queue_v10.c b/drivers/gpu/drm/amd/amdkfd/kfd_kernel_queue_v10.c
> index aed32ab7102e..bfd6221acae9 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_kernel_queue_v10.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_kernel_queue_v10.c
> @@ -298,37 +298,6 @@ static int pm_query_status_v10(struct packet_manager *pm, uint32_t *buffer,
> return 0;
> }
>
> -
> -static int pm_release_mem_v10(uint64_t gpu_addr, uint32_t *buffer)
> -{
> - struct pm4_mec_release_mem *packet;
> -
> - WARN_ON(!buffer);
> -
> - packet = (struct pm4_mec_release_mem *)buffer;
> - memset(buffer, 0, sizeof(struct pm4_mec_release_mem));
> -
> - packet->header.u32All = pm_build_pm4_header(IT_RELEASE_MEM,
> - sizeof(struct pm4_mec_release_mem));
> -
> - packet->bitfields2.event_type = CACHE_FLUSH_AND_INV_TS_EVENT;
> - packet->bitfields2.event_index = event_index__mec_release_mem__end_of_pipe;
> - packet->bitfields2.tcl1_action_ena = 1;
> - packet->bitfields2.tc_action_ena = 1;
> - packet->bitfields2.cache_policy = cache_policy__mec_release_mem__lru;
> -
> - packet->bitfields3.data_sel = data_sel__mec_release_mem__send_32_bit_low;
> - packet->bitfields3.int_sel =
> - int_sel__mec_release_mem__send_interrupt_after_write_confirm;
> -
> - packet->bitfields4.address_lo_32b = (gpu_addr & 0xffffffff) >> 2;
> - packet->address_hi = upper_32_bits(gpu_addr);
> -
> - packet->data_lo = 0;
> -
> - return sizeof(struct pm4_mec_release_mem) / sizeof(unsigned int);
> -}
> -
> const struct packet_manager_funcs kfd_v10_pm_funcs = {
> .map_process = pm_map_process_v10,
> .runlist = pm_runlist_v10,
> @@ -336,13 +305,13 @@ const struct packet_manager_funcs kfd_v10_pm_funcs = {
> .map_queues = pm_map_queues_v10,
> .unmap_queues = pm_unmap_queues_v10,
> .query_status = pm_query_status_v10,
> - .release_mem = pm_release_mem_v10,
> + .release_mem = NULL,
> .map_process_size = sizeof(struct pm4_mes_map_process),
> .runlist_size = sizeof(struct pm4_mes_runlist),
> .set_resources_size = sizeof(struct pm4_mes_set_resources),
> .map_queues_size = sizeof(struct pm4_mes_map_queues),
> .unmap_queues_size = sizeof(struct pm4_mes_unmap_queues),
> .query_status_size = sizeof(struct pm4_mes_query_status),
> - .release_mem_size = sizeof(struct pm4_mec_release_mem)
> + .release_mem_size = 0,
> };
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_kernel_queue_v9.c b/drivers/gpu/drm/amd/amdkfd/kfd_kernel_queue_v9.c
> index 3b5ca2b1d7a6..f0e4910a8865 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_kernel_queue_v9.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_kernel_queue_v9.c
> @@ -336,35 +336,6 @@ static int pm_query_status_v9(struct packet_manager *pm, uint32_t *buffer,
> return 0;
> }
>
> -
> -static int pm_release_mem_v9(uint64_t gpu_addr, uint32_t *buffer)
> -{
> - struct pm4_mec_release_mem *packet;
> -
> - packet = (struct pm4_mec_release_mem *)buffer;
> - memset(buffer, 0, sizeof(struct pm4_mec_release_mem));
> -
> - packet->header.u32All = pm_build_pm4_header(IT_RELEASE_MEM,
> - sizeof(struct pm4_mec_release_mem));
> -
> - packet->bitfields2.event_type = CACHE_FLUSH_AND_INV_TS_EVENT;
> - packet->bitfields2.event_index = event_index__mec_release_mem__end_of_pipe;
> - packet->bitfields2.tcl1_action_ena = 1;
> - packet->bitfields2.tc_action_ena = 1;
> - packet->bitfields2.cache_policy = cache_policy__mec_release_mem__lru;
> -
> - packet->bitfields3.data_sel = data_sel__mec_release_mem__send_32_bit_low;
> - packet->bitfields3.int_sel =
> - int_sel__mec_release_mem__send_interrupt_after_write_confirm;
> -
> - packet->bitfields4.address_lo_32b = (gpu_addr & 0xffffffff) >> 2;
> - packet->address_hi = upper_32_bits(gpu_addr);
> -
> - packet->data_lo = 0;
> -
> - return 0;
> -}
> -
> const struct packet_manager_funcs kfd_v9_pm_funcs = {
> .map_process = pm_map_process_v9,
> .runlist = pm_runlist_v9,
> @@ -372,12 +343,12 @@ const struct packet_manager_funcs kfd_v9_pm_funcs = {
> .map_queues = pm_map_queues_v9,
> .unmap_queues = pm_unmap_queues_v9,
> .query_status = pm_query_status_v9,
> - .release_mem = pm_release_mem_v9,
> + .release_mem = NULL,
> .map_process_size = sizeof(struct pm4_mes_map_process),
> .runlist_size = sizeof(struct pm4_mes_runlist),
> .set_resources_size = sizeof(struct pm4_mes_set_resources),
> .map_queues_size = sizeof(struct pm4_mes_map_queues),
> .unmap_queues_size = sizeof(struct pm4_mes_unmap_queues),
> .query_status_size = sizeof(struct pm4_mes_query_status),
> - .release_mem_size = sizeof(struct pm4_mec_release_mem)
> + .release_mem_size = 0,
> };