<html> <head> <meta http-equiv="Content-Type" content="text/html; charset=utf-8"> </head> <body text="#000000" bgcolor="#FFFFFF"> 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 <div class="moz-cite-prefix">On 2019-11-07 12:39, Zhao, Yong wrote: </div> <blockquote type="cite" cite="mid:41a6b535-ab7a-44f4-2899-f211f5ddc296@amd.com"> 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 <div class="moz-cite-prefix">On 2019-11-07 12:33 p.m., Russell, Kent wrote: </div> <blockquote type="cite" cite="mid:CY4PR1201MB2533087B346C4619F461FF3185780@CY4PR1201MB2533.namprd12.prod.outlook.com"> <meta name="Generator" content="Microsoft Word 15 (filtered medium)"> <style></style> <div class="WordSection1"> 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. <o:p></o:p> <o:p> </o:p> Obviously not high-priority, but maybe something to consider as you continue to consolidate and remove duplicate code.<o:p></o:p> <o:p> </o:p> Kent<o:p></o:p> <o:p> </o:p> <div> <div style="border:none;border-top:solid #E1E1E1 1.0pt;padding:3.0pt 0in 0in 0in"> From: amd-gfx <a class="moz-txt-link-rfc2396E" href="mailto:amd-gfx-bounces@lists.freedesktop.org" moz-do-not-send="true"> <amd-gfx-bounces@lists.freedesktop.org></a> On Behalf Of Zhao, Yong Sent: Thursday, November 7, 2019 11:57 AM To: Kuehling, Felix <a class="moz-txt-link-rfc2396E" href="mailto:Felix.Kuehling@amd.com" moz-do-not-send="true"> <Felix.Kuehling@amd.com></a>; <a class="moz-txt-link-abbreviated" href="mailto:amd-gfx@lists.freedesktop.org" moz-do-not-send="true"> amd-gfx@lists.freedesktop.org</a> Subject: Re: [PATCH 2/3] drm/amdkfd: only keep release_mem function for Hawaii<o:p></o:p> </div> </div> <o:p> </o:p> <div> Hi Felix,<o:p></o:p> </div> <div> <o:p> </o:p> </div> <div> That's because v8 and v7 share the same packet_manager_funcs. In this case, it is better to keep it as it is.<o:p></o:p> </div> <div> <o:p> </o:p> </div> <div> Regards,<o:p></o:p> </div> <div> Yong<o:p></o:p> </div> <div class="MsoNormal" style="text-align:center" align="center"> <hr width="98%" size="2" align="center"> </div> <div id="divRplyFwdMsg"> From: Kuehling, Felix <<a href="mailto:Felix.Kuehling@amd.com" moz-do-not-send="true">Felix.Kuehling@amd.com</a>> Sent: Wednesday, November 6, 2019 11:45 PM To: Zhao, Yong <<a href="mailto:Yong.Zhao@amd.com" moz-do-not-send="true">Yong.Zhao@amd.com</a>>; <a href="mailto:amd-gfx@lists.freedesktop.org" moz-do-not-send="true">amd-gfx@lists.freedesktop.org</a> <<a href="mailto:amd-gfx@lists.freedesktop.org" moz-do-not-send="true">amd-gfx@lists.freedesktop.org</a>> Subject: Re: [PATCH 2/3] drm/amdkfd: only keep release_mem function for Hawaii <o:p></o:p> <div> <o:p></o:p> </div> </div> <div> <div> 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 <<a href="mailto:Yong.Zhao@amd.com" moz-do-not-send="true">Yong.Zhao@amd.com</a>> > --- > .../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, > };<o:p></o:p> </div> </div> </div> </blockquote> </blockquote> </body> </html>