Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <59ab4749-8fb9-c0d5-1f7e-d5fceda85063@amd.com>
Date:   Wed, 4 Jan 2023 10:24:07 +0530
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.13.1
Subject: Re: [PATCH v4 07/27] drm/amd: Convert SDMA to use
 `amdgpu_ucode_ip_version_decode`
Content-Language: en-US
To:     Mario Limonciello <mario.limonciello@amd.com>,
        Alex Deucher <alexander.deucher@amd.com>,
        linux-kernel@vger.kernel.org
Cc:     Javier Martinez Canillas <javierm@redhat.com>,
        Carlos Soriano Sanchez <csoriano@redhat.com>,
        amd-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org,
        David Airlie <airlied@gmail.com>,
        Daniel Vetter <daniel@ffwll.ch>, christian.koenig@amd.com,
        "Pan, Xinhui" <Xinhui.Pan@amd.com>
References: <20230103221852.22813-1-mario.limonciello@amd.com>
 <20230103221852.22813-8-mario.limonciello@amd.com>
From:   "Lazar, Lijo" <lijo.lazar@amd.com>
In-Reply-To: <20230103221852.22813-8-mario.limonciello@amd.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?WlowdXFqbVVCMFU1a3hjdm43Nkxqcm5zY2dDSVdXZE0vNzFwZ2JKUUdIMi9u?=
 =?utf-8?B?WGdPU2g0c2U2S3FhZXZVTGVqUjY1UDZxamVIRmNhRWZpcTdZVlVXMEs3Q2Zt?=
 =?utf-8?B?NkJIaFcxRHFhekgwZmFMcUNDVFR6ODZvRXNjNFdvckNYWVZxTDdlS2wzZG1O?=
 =?utf-8?B?RkRIemxOVzE3YXZiYzRVRG93TlFWeWJNclN1Ymo4NWlodmlKRVRsaUlrNlVW?=
 =?utf-8?B?cnBtMTMvWnlWVkdRMjdZa1JZV1pTeW83U2lqVUlteVRRMmM3QjU1aTAzaXFi?=
 =?utf-8?B?NVRSWHJSTlBlZFhtQ01SL2RjNERzK0NwMXVIVVYvSUt3RHRTRnk3VnBTQmUx?=
 =?utf-8?B?U3UrTGJ5T0k1S3BtRzZkbGExMk9MOThsUFNHZ3JIZHhTMjMrS3RabWtiWitD?=
 =?utf-8?B?WmFzdWt1Qnlvd3A5cXhvRE5ndFh0a1FPVXgzY0NuRnZpeTQ1dWJqb2JwMzhE?=
 =?utf-8?B?MGFBaDU5Q0hFd2hobVVNRXZTanpBQ1FYcHFxZ0ovalNhQVFER0p2UjQybnNO?=
 =?utf-8?B?MTZKOCtSU3d0cHZ5OHVtT0NvSUVxOHNEeUNhSEhFY2ZYQzNqMS8yOFdQNzRk?=
 =?utf-8?B?cmdWRWt6SmZUMkxkck5acHpOUk1PZ2haU0t6TC9MelJWaUhsN1B4Nkd6dkg3?=
 =?utf-8?B?UE5JMlUwUzZDemFXWG9WZFdYV2xwNnlhbWoxd2UzNlFvL2NaalcxQUNiRjdO?=
 =?utf-8?B?Zm5ocXgwbGpWVVlXcU1aQlh0cFdkejRqMGhXNThZdmtMSWVuWjRBMzcvWEFF?=
 =?utf-8?B?TEd5N1hUdzBSNkVVbUZDNkViOEZxUEFqL1o1Q3pCOG5RY2tIWHlaTEYzQ3Zh?=
 =?utf-8?B?M2s5dVE0aVlQekllR09udUFBK3pIRHNneFVPU3NPaHNMTkNvRnpVZHhpTkF1?=
 =?utf-8?B?SDZnTXVJYkdwOTNzT2Z2K3RYc0NnbExrUXYzR0luT2dZanl1Y2FyMFhhOE0w?=
 =?utf-8?B?dGsvUFpNekJaV2ZsdjlmYmZCZzdOOHZaT2REMVZMY1ZqbkRQQjRSSGt3UFhW?=
 =?utf-8?B?VzZ5VzQ3Q3dDbVdrV1lXbEt5RmpybE40UmF6aDd3ckM5anN4NndlMi9mbTNn?=
 =?utf-8?B?Zm5NSloxV0xiK2ppY1pSVXZyMktIQ3RPWk43UjNGZ1E2NkpWSzlKYzlBT04z?=
 =?utf-8?B?WWR3cGJkR2krMmxuLzFSZkd2TzlDbVZnWEJ4YjZSWnBJbzRVQVRudXVDQ3E4?=
 =?utf-8?B?Q08veXRURlZGMkc3MkxxVHpYeVNCYWRHTnZZZEVwemViSEZLYjFBa0NQMGpY?=
 =?utf-8?B?ejBzdGg5M3lIR215MFhxSUVvSG1IemIzVlBQZ29aZXc0dHNyTVFObVlidGNa?=
 =?utf-8?B?R3VxRjNGdjZWd3RyQUVuaWJxaTlpeXk4aGtoMzFSQkVxanZOdHVTUVFpUHpF?=
 =?utf-8?B?ZWx0WE4vUzh2WmlXVDhaNTVwUTFDNmJSUnhVTTYzc0lhQVUwSGNTM0JmdnV6?=
 =?utf-8?B?ZUhkL3pLV25Jc2NQRW15d0kwU0ZUaXFhcmVqc0pnOVl6ZGFUR3N2SVY5dEZq?=
 =?utf-8?B?WWxKU1lzLzR3VGZ2WERvUEh1MnF5ZnFVdld5SjBRcTE4Y0MrU0duMUt1VXBV?=
 =?utf-8?B?eW5UM3FXYmlqeXpzcDNXdjJ4eGVocHdYNVd5eGxLbUI0TlVwL0VHbHpJTE9Y?=
 =?utf-8?B?SU14MGhzcDZxdHYzREU0eWNvQlFqWi85b1VrcGRYNFRpRE5YekpxSlBpb2Nh?=
 =?utf-8?B?R3cxY3J3MFhEVm5Scks0MUg3TDZVV0x6TDdYWUNCYXpnd2JCL1AwUTZJd1c1?=
 =?utf-8?B?WUF6cTBGUHRPUnJuVThPc2RIYTR1UzhvVFowZGtKU0ZnTFJBOUVhOVdxWDBK?=
 =?utf-8?B?aDAyem81RFd3Mk1Oa0lhQTZVckdZeHpGbVVuT2VaZ3hxZkQ4clV6a2xDdXUy?=
 =?utf-8?B?d24xczlNUjFTa05MbkJHckZCa1JmbEtJb3hMTWg4Wm5JNG1acW9mZlU0OTht?=
 =?utf-8?B?eEpqTjI1N2hiZCt1QlNHWWFaaU1nRTRkbTBPTkh4UDJ5K01XNHo1d1I4Ty9X?=
 =?utf-8?B?Qjd5MFc5TXErL3g0THVMZnM5Z3pIL3FlQ3B0MnE5ZFhaalhwQ1I1VVJvQXVP?=
 =?utf-8?B?eFBwUS9qV3U3c1hwTHFXdFNndC9jMnNUaWx2ek1qMlhOTlQvOTRCVVFsSzJR?=
 =?utf-8?Q?So42JlONN/1ezzQGV5pGSJFXa?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 01364e6d-8c8b-446b-8b74-08daee0fbd7c
X-MS-Exchange-CrossTenant-AuthSource: BYAPR12MB4614.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 04 Jan 2023 04:54:20.0447
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: HTZi0O2YsH1ix6/SawAqcRUQcA+TqTw1iQNvSqbJxAO5S3WxOQI81JByHXkeLnof
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MN0PR12MB5858
X-Spam-Status: No, score=-5.2 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,
        RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


On 1/4/2023 3:48 AM, Mario Limonciello wrote:
> Simplifies the code so that all SDMA versions will get the firmware
> name from `amdgpu_ucode_ip_version_decode`.
> 
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
> v3->v4:
>   * Move out of IP discovery and instead simplify early_init
> v2->v3:
>   * Fix dGPU naming scheme
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.c |  7 ++-
>   drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.h |  4 +-
>   drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c   | 47 +-------------------
>   drivers/gpu/drm/amd/amdgpu/sdma_v5_0.c   | 30 +------------
>   drivers/gpu/drm/amd/amdgpu/sdma_v5_2.c   | 55 +-----------------------
>   drivers/gpu/drm/amd/amdgpu/sdma_v6_0.c   | 25 +----------
>   6 files changed, 13 insertions(+), 155 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.c
> index 9e85a078d918..83e8f0dae647 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.c
> @@ -200,15 +200,18 @@ void amdgpu_sdma_destroy_inst_ctx(struct amdgpu_device *adev,
>   }
>   
>   int amdgpu_sdma_init_microcode(struct amdgpu_device *adev,
> -			       char *fw_name, u32 instance,
> -			       bool duplicate)
> +			       u32 instance, bool duplicate)
>   {
>   	struct amdgpu_firmware_info *info = NULL;
>   	const struct common_firmware_header *header = NULL;
>   	int err = 0, i;
>   	const struct sdma_firmware_header_v2_0 *sdma_hdr;
>   	uint16_t version_major;
> +	char ucode_prefix[30];
> +	char fw_name[40];
>   
> +	amdgpu_ucode_ip_version_decode(adev, SDMA0_HWIP, ucode_prefix, sizeof(ucode_prefix));
> +	snprintf(fw_name, sizeof(fw_name), "amdgpu/%s%s.bin", ucode_prefix, !instance ? "" : "1");

It is safer to keep the original logic with instance number as suffix 
rather than hardcoding to 1.

Thanks,
Lijo

>   	err = amdgpu_ucode_load(adev, &adev->sdma.instance[instance].fw, fw_name);
>   	if (err)
>   		goto out;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.h
> index 7d99205c2e01..2d16e6d36728 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.h
> @@ -124,8 +124,8 @@ int amdgpu_sdma_process_ras_data_cb(struct amdgpu_device *adev,
>   int amdgpu_sdma_process_ecc_irq(struct amdgpu_device *adev,
>   				      struct amdgpu_irq_src *source,
>   				      struct amdgpu_iv_entry *entry);
> -int amdgpu_sdma_init_microcode(struct amdgpu_device *adev,
> -        char *fw_name, u32 instance, bool duplicate);
> +int amdgpu_sdma_init_microcode(struct amdgpu_device *adev, u32 instance,
> +			       bool duplicate);
>   void amdgpu_sdma_destroy_inst_ctx(struct amdgpu_device *adev,
>           bool duplicate);
>   void amdgpu_sdma_unset_buffer_funcs_helper(struct amdgpu_device *adev);
> diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c b/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
> index 4d780e4430e7..017ae298558e 100644
> --- a/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
> @@ -575,60 +575,17 @@ static void sdma_v4_0_setup_ulv(struct amdgpu_device *adev)
>   // vega10 real chip need to use PSP to load firmware
>   static int sdma_v4_0_init_microcode(struct amdgpu_device *adev)
>   {
> -	const char *chip_name;
> -	char fw_name[30];
>   	int ret, i;
>   
> -	DRM_DEBUG("\n");
> -
> -	switch (adev->ip_versions[SDMA0_HWIP][0]) {
> -	case IP_VERSION(4, 0, 0):
> -		chip_name = "vega10";
> -		break;
> -	case IP_VERSION(4, 0, 1):
> -		chip_name = "vega12";
> -		break;
> -	case IP_VERSION(4, 2, 0):
> -		chip_name = "vega20";
> -		break;
> -	case IP_VERSION(4, 1, 0):
> -	case IP_VERSION(4, 1, 1):
> -		if (adev->apu_flags & AMD_APU_IS_RAVEN2)
> -			chip_name = "raven2";
> -		else if (adev->apu_flags & AMD_APU_IS_PICASSO)
> -			chip_name = "picasso";
> -		else
> -			chip_name = "raven";
> -		break;
> -	case IP_VERSION(4, 2, 2):
> -		chip_name = "arcturus";
> -		break;
> -	case IP_VERSION(4, 1, 2):
> -		if (adev->apu_flags & AMD_APU_IS_RENOIR)
> -			chip_name = "renoir";
> -		else
> -			chip_name = "green_sardine";
> -		break;
> -	case IP_VERSION(4, 4, 0):
> -		chip_name = "aldebaran";
> -		break;
> -	default:
> -		BUG();
> -	}
> -
>   	for (i = 0; i < adev->sdma.num_instances; i++) {
> -		if (i == 0)
> -			snprintf(fw_name, sizeof(fw_name), "amdgpu/%s_sdma.bin", chip_name);
> -		else
> -			snprintf(fw_name, sizeof(fw_name), "amdgpu/%s_sdma%d.bin", chip_name, i);
>   		if (adev->ip_versions[SDMA0_HWIP][0] == IP_VERSION(4, 2, 2) ||
>                       adev->ip_versions[SDMA0_HWIP][0] == IP_VERSION(4, 4, 0)) {
>   			/* Acturus & Aldebaran will leverage the same FW memory
>   			   for every SDMA instance */
> -			ret = amdgpu_sdma_init_microcode(adev, fw_name, 0, true);
> +			ret = amdgpu_sdma_init_microcode(adev, 0, true);
>   			break;
>   		} else {
> -			ret = amdgpu_sdma_init_microcode(adev, fw_name, i, false);
> +			ret = amdgpu_sdma_init_microcode(adev, i, false);
>   			if (ret)
>   				return ret;
>   		}
> diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v5_0.c b/drivers/gpu/drm/amd/amdgpu/sdma_v5_0.c
> index d4d9f196db83..1941b3b7c5d9 100644
> --- a/drivers/gpu/drm/amd/amdgpu/sdma_v5_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/sdma_v5_0.c
> @@ -237,39 +237,13 @@ static void sdma_v5_0_init_golden_registers(struct amdgpu_device *adev)
>   // emulation only, won't work on real chip
>   // navi10 real chip need to use PSP to load firmware
>   static int sdma_v5_0_init_microcode(struct amdgpu_device *adev)
> -{
> -	const char *chip_name;
> -	char fw_name[40];
> -	int ret, i;
> +{	int ret, i;
>   
>   	if (amdgpu_sriov_vf(adev) && (adev->ip_versions[SDMA0_HWIP][0] == IP_VERSION(5, 0, 5)))
>   		return 0;
>   
> -	DRM_DEBUG("\n");
> -
> -	switch (adev->ip_versions[SDMA0_HWIP][0]) {
> -	case IP_VERSION(5, 0, 0):
> -		chip_name = "navi10";
> -		break;
> -	case IP_VERSION(5, 0, 2):
> -		chip_name = "navi14";
> -		break;
> -	case IP_VERSION(5, 0, 5):
> -		chip_name = "navi12";
> -		break;
> -	case IP_VERSION(5, 0, 1):
> -		chip_name = "cyan_skillfish2";
> -		break;
> -	default:
> -		BUG();
> -	}
> -
>   	for (i = 0; i < adev->sdma.num_instances; i++) {
> -		if (i == 0)
> -			snprintf(fw_name, sizeof(fw_name), "amdgpu/%s_sdma.bin", chip_name);
> -		else
> -			snprintf(fw_name, sizeof(fw_name), "amdgpu/%s_sdma1.bin", chip_name);
> -		ret = amdgpu_sdma_init_microcode(adev, fw_name, i, false);
> +		ret = amdgpu_sdma_init_microcode(adev, i, false);
>   		if (ret)
>   			return ret;
>   	}
> diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v5_2.c b/drivers/gpu/drm/amd/amdgpu/sdma_v5_2.c
> index 65e7a710298d..8e445eb9dd49 100644
> --- a/drivers/gpu/drm/amd/amdgpu/sdma_v5_2.c
> +++ b/drivers/gpu/drm/amd/amdgpu/sdma_v5_2.c
> @@ -89,59 +89,6 @@ static u32 sdma_v5_2_get_reg_offset(struct amdgpu_device *adev, u32 instance, u3
>   	return base + internal_offset;
>   }
>   
> -/**
> - * sdma_v5_2_init_microcode - load ucode images from disk
> - *
> - * @adev: amdgpu_device pointer
> - *
> - * Use the firmware interface to load the ucode images into
> - * the driver (not loaded into hw).
> - * Returns 0 on success, error on failure.
> - */
> -
> -// emulation only, won't work on real chip
> -// navi10 real chip need to use PSP to load firmware
> -static int sdma_v5_2_init_microcode(struct amdgpu_device *adev)
> -{
> -	const char *chip_name;
> -	char fw_name[40];
> -
> -	DRM_DEBUG("\n");
> -
> -	switch (adev->ip_versions[SDMA0_HWIP][0]) {
> -	case IP_VERSION(5, 2, 0):
> -		chip_name = "sienna_cichlid_sdma";
> -		break;
> -	case IP_VERSION(5, 2, 2):
> -		chip_name = "navy_flounder_sdma";
> -		break;
> -	case IP_VERSION(5, 2, 1):
> -		chip_name = "vangogh_sdma";
> -		break;
> -	case IP_VERSION(5, 2, 4):
> -		chip_name = "dimgrey_cavefish_sdma";
> -		break;
> -	case IP_VERSION(5, 2, 5):
> -		chip_name = "beige_goby_sdma";
> -		break;
> -	case IP_VERSION(5, 2, 3):
> -		chip_name = "yellow_carp_sdma";
> -		break;
> -	case IP_VERSION(5, 2, 6):
> -		chip_name = "sdma_5_2_6";
> -		break;
> -	case IP_VERSION(5, 2, 7):
> -		chip_name = "sdma_5_2_7";
> -		break;
> -	default:
> -		BUG();
> -	}
> -
> -	snprintf(fw_name, sizeof(fw_name), "amdgpu/%s.bin", chip_name);
> -
> -	return amdgpu_sdma_init_microcode(adev, fw_name, 0, true);
> -}
> -
>   static unsigned sdma_v5_2_ring_init_cond_exec(struct amdgpu_ring *ring)
>   {
>   	unsigned ret;
> @@ -1288,7 +1235,7 @@ static int sdma_v5_2_sw_init(void *handle)
>   			return r;
>   	}
>   
> -	r = sdma_v5_2_init_microcode(adev);
> +	r = amdgpu_sdma_init_microcode(adev, 0, true);
>   	if (r) {
>   		DRM_ERROR("Failed to load sdma firmware!\n");
>   		return r;
> diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v6_0.c b/drivers/gpu/drm/amd/amdgpu/sdma_v6_0.c
> index 049c26a45d85..bf1fa5e8d2f9 100644
> --- a/drivers/gpu/drm/amd/amdgpu/sdma_v6_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/sdma_v6_0.c
> @@ -78,29 +78,6 @@ static u32 sdma_v6_0_get_reg_offset(struct amdgpu_device *adev, u32 instance, u3
>   	return base + internal_offset;
>   }
>   
> -/**
> - * sdma_v6_0_init_microcode - load ucode images from disk
> - *
> - * @adev: amdgpu_device pointer
> - *
> - * Use the firmware interface to load the ucode images into
> - * the driver (not loaded into hw).
> - * Returns 0 on success, error on failure.
> - */
> -static int sdma_v6_0_init_microcode(struct amdgpu_device *adev)
> -{
> -	char fw_name[30];
> -	char ucode_prefix[30];
> -
> -	DRM_DEBUG("\n");
> -
> -	amdgpu_ucode_ip_version_decode(adev, SDMA0_HWIP, ucode_prefix, sizeof(ucode_prefix));
> -
> -	snprintf(fw_name, sizeof(fw_name), "amdgpu/%s.bin", ucode_prefix);
> -
> -	return amdgpu_sdma_init_microcode(adev, fw_name, 0, true);
> -}
> -
>   static unsigned sdma_v6_0_ring_init_cond_exec(struct amdgpu_ring *ring)
>   {
>   	unsigned ret;
> @@ -1260,7 +1237,7 @@ static int sdma_v6_0_sw_init(void *handle)
>   	if (r)
>   		return r;
>   
> -	r = sdma_v6_0_init_microcode(adev);
> +	r = amdgpu_sdma_init_microcode(adev, 0, true);
>   	if (r) {
>   		DRM_ERROR("Failed to load sdma firmware!\n");
>   		return r;