* Re: [PATCH] drm/amdgpu/discovery: fix fw based ip discovery
2025-07-30 15:59 [PATCH] drm/amdgpu/discovery: fix fw based ip discovery Alex Deucher
@ 2025-08-04 14:16 ` Alex Deucher
2025-08-06 14:17 ` Alex Deucher
2025-08-06 14:27 ` Mario Limonciello
2025-08-06 15:17 ` Lazar, Lijo
2 siblings, 1 reply; 5+ messages in thread
From: Alex Deucher @ 2025-08-04 14:16 UTC (permalink / raw)
To: Alex Deucher; +Cc: amd-gfx, stable
Ping?
Alex
On Wed, Jul 30, 2025 at 12:18 PM Alex Deucher <alexander.deucher@amd.com> wrote:
>
> We only need the fw based discovery table for sysfs. No
> need to parse it. Additionally parsing some of the board
> specific tables may result in incorrect data on some boards.
> just load the binary and don't parse it on those boards.
>
> Closes: https://gitlab.freedesktop.org/drm/amd/-/issues/4441
> Fixes: 80a0e8282933 ("drm/amdgpu/discovery: optionally use fw based ip discovery")
> Cc: stable@vger.kernel.org
> Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 5 +-
> drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c | 72 ++++++++++---------
> 2 files changed, 41 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index efe98ffb679a4..b2538cff222ce 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -2570,9 +2570,6 @@ static int amdgpu_device_parse_gpu_info_fw(struct amdgpu_device *adev)
>
> adev->firmware.gpu_info_fw = NULL;
>
> - if (adev->mman.discovery_bin)
> - return 0;
> -
> switch (adev->asic_type) {
> default:
> return 0;
> @@ -2594,6 +2591,8 @@ static int amdgpu_device_parse_gpu_info_fw(struct amdgpu_device *adev)
> chip_name = "arcturus";
> break;
> case CHIP_NAVI12:
> + if (adev->mman.discovery_bin)
> + return 0;
> chip_name = "navi12";
> break;
> }
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
> index 81b3443c8d7f4..27bd7659961e8 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
> @@ -2555,40 +2555,11 @@ int amdgpu_discovery_set_ip_blocks(struct amdgpu_device *adev)
>
> switch (adev->asic_type) {
> case CHIP_VEGA10:
> - case CHIP_VEGA12:
> - case CHIP_RAVEN:
> - case CHIP_VEGA20:
> - case CHIP_ARCTURUS:
> - case CHIP_ALDEBARAN:
> - /* this is not fatal. We have a fallback below
> - * if the new firmwares are not present. some of
> - * this will be overridden below to keep things
> - * consistent with the current behavior.
> + /* This is not fatal. We only need the discovery
> + * binary for sysfs. We don't need it for a
> + * functional system.
> */
> - r = amdgpu_discovery_reg_base_init(adev);
> - if (!r) {
> - amdgpu_discovery_harvest_ip(adev);
> - amdgpu_discovery_get_gfx_info(adev);
> - amdgpu_discovery_get_mall_info(adev);
> - amdgpu_discovery_get_vcn_info(adev);
> - }
> - break;
> - default:
> - r = amdgpu_discovery_reg_base_init(adev);
> - if (r) {
> - drm_err(&adev->ddev, "discovery failed: %d\n", r);
> - return r;
> - }
> -
> - amdgpu_discovery_harvest_ip(adev);
> - amdgpu_discovery_get_gfx_info(adev);
> - amdgpu_discovery_get_mall_info(adev);
> - amdgpu_discovery_get_vcn_info(adev);
> - break;
> - }
> -
> - switch (adev->asic_type) {
> - case CHIP_VEGA10:
> + amdgpu_discovery_init(adev);
> vega10_reg_base_init(adev);
> adev->sdma.num_instances = 2;
> adev->gmc.num_umc = 4;
> @@ -2611,6 +2582,11 @@ int amdgpu_discovery_set_ip_blocks(struct amdgpu_device *adev)
> adev->ip_versions[DCI_HWIP][0] = IP_VERSION(12, 0, 0);
> break;
> case CHIP_VEGA12:
> + /* This is not fatal. We only need the discovery
> + * binary for sysfs. We don't need it for a
> + * functional system.
> + */
> + amdgpu_discovery_init(adev);
> vega10_reg_base_init(adev);
> adev->sdma.num_instances = 2;
> adev->gmc.num_umc = 4;
> @@ -2633,6 +2609,11 @@ int amdgpu_discovery_set_ip_blocks(struct amdgpu_device *adev)
> adev->ip_versions[DCI_HWIP][0] = IP_VERSION(12, 0, 1);
> break;
> case CHIP_RAVEN:
> + /* This is not fatal. We only need the discovery
> + * binary for sysfs. We don't need it for a
> + * functional system.
> + */
> + amdgpu_discovery_init(adev);
> vega10_reg_base_init(adev);
> adev->sdma.num_instances = 1;
> adev->vcn.num_vcn_inst = 1;
> @@ -2674,6 +2655,11 @@ int amdgpu_discovery_set_ip_blocks(struct amdgpu_device *adev)
> }
> break;
> case CHIP_VEGA20:
> + /* This is not fatal. We only need the discovery
> + * binary for sysfs. We don't need it for a
> + * functional system.
> + */
> + amdgpu_discovery_init(adev);
> vega20_reg_base_init(adev);
> adev->sdma.num_instances = 2;
> adev->gmc.num_umc = 8;
> @@ -2697,6 +2683,11 @@ int amdgpu_discovery_set_ip_blocks(struct amdgpu_device *adev)
> adev->ip_versions[DCI_HWIP][0] = IP_VERSION(12, 1, 0);
> break;
> case CHIP_ARCTURUS:
> + /* This is not fatal. We only need the discovery
> + * binary for sysfs. We don't need it for a
> + * functional system.
> + */
> + amdgpu_discovery_init(adev);
> arct_reg_base_init(adev);
> adev->sdma.num_instances = 8;
> adev->vcn.num_vcn_inst = 2;
> @@ -2725,6 +2716,11 @@ int amdgpu_discovery_set_ip_blocks(struct amdgpu_device *adev)
> adev->ip_versions[UVD_HWIP][1] = IP_VERSION(2, 5, 0);
> break;
> case CHIP_ALDEBARAN:
> + /* This is not fatal. We only need the discovery
> + * binary for sysfs. We don't need it for a
> + * functional system.
> + */
> + amdgpu_discovery_init(adev);
> aldebaran_reg_base_init(adev);
> adev->sdma.num_instances = 5;
> adev->vcn.num_vcn_inst = 2;
> @@ -2751,6 +2747,16 @@ int amdgpu_discovery_set_ip_blocks(struct amdgpu_device *adev)
> adev->ip_versions[XGMI_HWIP][0] = IP_VERSION(6, 1, 0);
> break;
> default:
> + r = amdgpu_discovery_reg_base_init(adev);
> + if (r) {
> + drm_err(&adev->ddev, "discovery failed: %d\n", r);
> + return r;
> + }
> +
> + amdgpu_discovery_harvest_ip(adev);
> + amdgpu_discovery_get_gfx_info(adev);
> + amdgpu_discovery_get_mall_info(adev);
> + amdgpu_discovery_get_vcn_info(adev);
> break;
> }
>
> --
> 2.50.1
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] drm/amdgpu/discovery: fix fw based ip discovery
2025-08-04 14:16 ` Alex Deucher
@ 2025-08-06 14:17 ` Alex Deucher
0 siblings, 0 replies; 5+ messages in thread
From: Alex Deucher @ 2025-08-06 14:17 UTC (permalink / raw)
To: Alex Deucher; +Cc: amd-gfx, stable
Ping again? This fixes a regression.
Alex
On Mon, Aug 4, 2025 at 10:16 AM Alex Deucher <alexdeucher@gmail.com> wrote:
>
> Ping?
>
> Alex
>
> On Wed, Jul 30, 2025 at 12:18 PM Alex Deucher <alexander.deucher@amd.com> wrote:
> >
> > We only need the fw based discovery table for sysfs. No
> > need to parse it. Additionally parsing some of the board
> > specific tables may result in incorrect data on some boards.
> > just load the binary and don't parse it on those boards.
> >
> > Closes: https://gitlab.freedesktop.org/drm/amd/-/issues/4441
> > Fixes: 80a0e8282933 ("drm/amdgpu/discovery: optionally use fw based ip discovery")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
> > ---
> > drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 5 +-
> > drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c | 72 ++++++++++---------
> > 2 files changed, 41 insertions(+), 36 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > index efe98ffb679a4..b2538cff222ce 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > @@ -2570,9 +2570,6 @@ static int amdgpu_device_parse_gpu_info_fw(struct amdgpu_device *adev)
> >
> > adev->firmware.gpu_info_fw = NULL;
> >
> > - if (adev->mman.discovery_bin)
> > - return 0;
> > -
> > switch (adev->asic_type) {
> > default:
> > return 0;
> > @@ -2594,6 +2591,8 @@ static int amdgpu_device_parse_gpu_info_fw(struct amdgpu_device *adev)
> > chip_name = "arcturus";
> > break;
> > case CHIP_NAVI12:
> > + if (adev->mman.discovery_bin)
> > + return 0;
> > chip_name = "navi12";
> > break;
> > }
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
> > index 81b3443c8d7f4..27bd7659961e8 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
> > @@ -2555,40 +2555,11 @@ int amdgpu_discovery_set_ip_blocks(struct amdgpu_device *adev)
> >
> > switch (adev->asic_type) {
> > case CHIP_VEGA10:
> > - case CHIP_VEGA12:
> > - case CHIP_RAVEN:
> > - case CHIP_VEGA20:
> > - case CHIP_ARCTURUS:
> > - case CHIP_ALDEBARAN:
> > - /* this is not fatal. We have a fallback below
> > - * if the new firmwares are not present. some of
> > - * this will be overridden below to keep things
> > - * consistent with the current behavior.
> > + /* This is not fatal. We only need the discovery
> > + * binary for sysfs. We don't need it for a
> > + * functional system.
> > */
> > - r = amdgpu_discovery_reg_base_init(adev);
> > - if (!r) {
> > - amdgpu_discovery_harvest_ip(adev);
> > - amdgpu_discovery_get_gfx_info(adev);
> > - amdgpu_discovery_get_mall_info(adev);
> > - amdgpu_discovery_get_vcn_info(adev);
> > - }
> > - break;
> > - default:
> > - r = amdgpu_discovery_reg_base_init(adev);
> > - if (r) {
> > - drm_err(&adev->ddev, "discovery failed: %d\n", r);
> > - return r;
> > - }
> > -
> > - amdgpu_discovery_harvest_ip(adev);
> > - amdgpu_discovery_get_gfx_info(adev);
> > - amdgpu_discovery_get_mall_info(adev);
> > - amdgpu_discovery_get_vcn_info(adev);
> > - break;
> > - }
> > -
> > - switch (adev->asic_type) {
> > - case CHIP_VEGA10:
> > + amdgpu_discovery_init(adev);
> > vega10_reg_base_init(adev);
> > adev->sdma.num_instances = 2;
> > adev->gmc.num_umc = 4;
> > @@ -2611,6 +2582,11 @@ int amdgpu_discovery_set_ip_blocks(struct amdgpu_device *adev)
> > adev->ip_versions[DCI_HWIP][0] = IP_VERSION(12, 0, 0);
> > break;
> > case CHIP_VEGA12:
> > + /* This is not fatal. We only need the discovery
> > + * binary for sysfs. We don't need it for a
> > + * functional system.
> > + */
> > + amdgpu_discovery_init(adev);
> > vega10_reg_base_init(adev);
> > adev->sdma.num_instances = 2;
> > adev->gmc.num_umc = 4;
> > @@ -2633,6 +2609,11 @@ int amdgpu_discovery_set_ip_blocks(struct amdgpu_device *adev)
> > adev->ip_versions[DCI_HWIP][0] = IP_VERSION(12, 0, 1);
> > break;
> > case CHIP_RAVEN:
> > + /* This is not fatal. We only need the discovery
> > + * binary for sysfs. We don't need it for a
> > + * functional system.
> > + */
> > + amdgpu_discovery_init(adev);
> > vega10_reg_base_init(adev);
> > adev->sdma.num_instances = 1;
> > adev->vcn.num_vcn_inst = 1;
> > @@ -2674,6 +2655,11 @@ int amdgpu_discovery_set_ip_blocks(struct amdgpu_device *adev)
> > }
> > break;
> > case CHIP_VEGA20:
> > + /* This is not fatal. We only need the discovery
> > + * binary for sysfs. We don't need it for a
> > + * functional system.
> > + */
> > + amdgpu_discovery_init(adev);
> > vega20_reg_base_init(adev);
> > adev->sdma.num_instances = 2;
> > adev->gmc.num_umc = 8;
> > @@ -2697,6 +2683,11 @@ int amdgpu_discovery_set_ip_blocks(struct amdgpu_device *adev)
> > adev->ip_versions[DCI_HWIP][0] = IP_VERSION(12, 1, 0);
> > break;
> > case CHIP_ARCTURUS:
> > + /* This is not fatal. We only need the discovery
> > + * binary for sysfs. We don't need it for a
> > + * functional system.
> > + */
> > + amdgpu_discovery_init(adev);
> > arct_reg_base_init(adev);
> > adev->sdma.num_instances = 8;
> > adev->vcn.num_vcn_inst = 2;
> > @@ -2725,6 +2716,11 @@ int amdgpu_discovery_set_ip_blocks(struct amdgpu_device *adev)
> > adev->ip_versions[UVD_HWIP][1] = IP_VERSION(2, 5, 0);
> > break;
> > case CHIP_ALDEBARAN:
> > + /* This is not fatal. We only need the discovery
> > + * binary for sysfs. We don't need it for a
> > + * functional system.
> > + */
> > + amdgpu_discovery_init(adev);
> > aldebaran_reg_base_init(adev);
> > adev->sdma.num_instances = 5;
> > adev->vcn.num_vcn_inst = 2;
> > @@ -2751,6 +2747,16 @@ int amdgpu_discovery_set_ip_blocks(struct amdgpu_device *adev)
> > adev->ip_versions[XGMI_HWIP][0] = IP_VERSION(6, 1, 0);
> > break;
> > default:
> > + r = amdgpu_discovery_reg_base_init(adev);
> > + if (r) {
> > + drm_err(&adev->ddev, "discovery failed: %d\n", r);
> > + return r;
> > + }
> > +
> > + amdgpu_discovery_harvest_ip(adev);
> > + amdgpu_discovery_get_gfx_info(adev);
> > + amdgpu_discovery_get_mall_info(adev);
> > + amdgpu_discovery_get_vcn_info(adev);
> > break;
> > }
> >
> > --
> > 2.50.1
> >
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] drm/amdgpu/discovery: fix fw based ip discovery
2025-07-30 15:59 [PATCH] drm/amdgpu/discovery: fix fw based ip discovery Alex Deucher
2025-08-04 14:16 ` Alex Deucher
@ 2025-08-06 14:27 ` Mario Limonciello
2025-08-06 15:17 ` Lazar, Lijo
2 siblings, 0 replies; 5+ messages in thread
From: Mario Limonciello @ 2025-08-06 14:27 UTC (permalink / raw)
To: Alex Deucher, amd-gfx; +Cc: stable
On 7/30/2025 10:59 AM, Alex Deucher wrote:
> We only need the fw based discovery table for sysfs. No
> need to parse it. Additionally parsing some of the board
> specific tables may result in incorrect data on some boards.
> just load the binary and don't parse it on those boards.
>
> Closes: https://gitlab.freedesktop.org/drm/amd/-/issues/4441
> Fixes: 80a0e8282933 ("drm/amdgpu/discovery: optionally use fw based ip discovery")
> Cc: stable@vger.kernel.org
> Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
Reviewed-by: Mario Limonciello (AMD) <superm1@kernel.org>
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 5 +-
> drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c | 72 ++++++++++---------
> 2 files changed, 41 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index efe98ffb679a4..b2538cff222ce 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -2570,9 +2570,6 @@ static int amdgpu_device_parse_gpu_info_fw(struct amdgpu_device *adev)
>
> adev->firmware.gpu_info_fw = NULL;
>
> - if (adev->mman.discovery_bin)
> - return 0;
> -
> switch (adev->asic_type) {
> default:
> return 0;
> @@ -2594,6 +2591,8 @@ static int amdgpu_device_parse_gpu_info_fw(struct amdgpu_device *adev)
> chip_name = "arcturus";
> break;
> case CHIP_NAVI12:
> + if (adev->mman.discovery_bin)
> + return 0;
> chip_name = "navi12";
> break;
> }
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
> index 81b3443c8d7f4..27bd7659961e8 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
> @@ -2555,40 +2555,11 @@ int amdgpu_discovery_set_ip_blocks(struct amdgpu_device *adev)
>
> switch (adev->asic_type) {
> case CHIP_VEGA10:
> - case CHIP_VEGA12:
> - case CHIP_RAVEN:
> - case CHIP_VEGA20:
> - case CHIP_ARCTURUS:
> - case CHIP_ALDEBARAN:
> - /* this is not fatal. We have a fallback below
> - * if the new firmwares are not present. some of
> - * this will be overridden below to keep things
> - * consistent with the current behavior.
> + /* This is not fatal. We only need the discovery
> + * binary for sysfs. We don't need it for a
> + * functional system.
> */
> - r = amdgpu_discovery_reg_base_init(adev);
> - if (!r) {
> - amdgpu_discovery_harvest_ip(adev);
> - amdgpu_discovery_get_gfx_info(adev);
> - amdgpu_discovery_get_mall_info(adev);
> - amdgpu_discovery_get_vcn_info(adev);
> - }
> - break;
> - default:
> - r = amdgpu_discovery_reg_base_init(adev);
> - if (r) {
> - drm_err(&adev->ddev, "discovery failed: %d\n", r);
> - return r;
> - }
> -
> - amdgpu_discovery_harvest_ip(adev);
> - amdgpu_discovery_get_gfx_info(adev);
> - amdgpu_discovery_get_mall_info(adev);
> - amdgpu_discovery_get_vcn_info(adev);
> - break;
> - }
> -
> - switch (adev->asic_type) {
> - case CHIP_VEGA10:
> + amdgpu_discovery_init(adev);
> vega10_reg_base_init(adev);
> adev->sdma.num_instances = 2;
> adev->gmc.num_umc = 4;
> @@ -2611,6 +2582,11 @@ int amdgpu_discovery_set_ip_blocks(struct amdgpu_device *adev)
> adev->ip_versions[DCI_HWIP][0] = IP_VERSION(12, 0, 0);
> break;
> case CHIP_VEGA12:
> + /* This is not fatal. We only need the discovery
> + * binary for sysfs. We don't need it for a
> + * functional system.
> + */
> + amdgpu_discovery_init(adev);
> vega10_reg_base_init(adev);
> adev->sdma.num_instances = 2;
> adev->gmc.num_umc = 4;
> @@ -2633,6 +2609,11 @@ int amdgpu_discovery_set_ip_blocks(struct amdgpu_device *adev)
> adev->ip_versions[DCI_HWIP][0] = IP_VERSION(12, 0, 1);
> break;
> case CHIP_RAVEN:
> + /* This is not fatal. We only need the discovery
> + * binary for sysfs. We don't need it for a
> + * functional system.
> + */
> + amdgpu_discovery_init(adev);
> vega10_reg_base_init(adev);
> adev->sdma.num_instances = 1;
> adev->vcn.num_vcn_inst = 1;
> @@ -2674,6 +2655,11 @@ int amdgpu_discovery_set_ip_blocks(struct amdgpu_device *adev)
> }
> break;
> case CHIP_VEGA20:
> + /* This is not fatal. We only need the discovery
> + * binary for sysfs. We don't need it for a
> + * functional system.
> + */
> + amdgpu_discovery_init(adev);
> vega20_reg_base_init(adev);
> adev->sdma.num_instances = 2;
> adev->gmc.num_umc = 8;
> @@ -2697,6 +2683,11 @@ int amdgpu_discovery_set_ip_blocks(struct amdgpu_device *adev)
> adev->ip_versions[DCI_HWIP][0] = IP_VERSION(12, 1, 0);
> break;
> case CHIP_ARCTURUS:
> + /* This is not fatal. We only need the discovery
> + * binary for sysfs. We don't need it for a
> + * functional system.
> + */
> + amdgpu_discovery_init(adev);
> arct_reg_base_init(adev);
> adev->sdma.num_instances = 8;
> adev->vcn.num_vcn_inst = 2;
> @@ -2725,6 +2716,11 @@ int amdgpu_discovery_set_ip_blocks(struct amdgpu_device *adev)
> adev->ip_versions[UVD_HWIP][1] = IP_VERSION(2, 5, 0);
> break;
> case CHIP_ALDEBARAN:
> + /* This is not fatal. We only need the discovery
> + * binary for sysfs. We don't need it for a
> + * functional system.
> + */
> + amdgpu_discovery_init(adev);
> aldebaran_reg_base_init(adev);
> adev->sdma.num_instances = 5;
> adev->vcn.num_vcn_inst = 2;
> @@ -2751,6 +2747,16 @@ int amdgpu_discovery_set_ip_blocks(struct amdgpu_device *adev)
> adev->ip_versions[XGMI_HWIP][0] = IP_VERSION(6, 1, 0);
> break;
> default:
> + r = amdgpu_discovery_reg_base_init(adev);
> + if (r) {
> + drm_err(&adev->ddev, "discovery failed: %d\n", r);
> + return r;
> + }
> +
> + amdgpu_discovery_harvest_ip(adev);
> + amdgpu_discovery_get_gfx_info(adev);
> + amdgpu_discovery_get_mall_info(adev);
> + amdgpu_discovery_get_vcn_info(adev);
> break;
> }
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] drm/amdgpu/discovery: fix fw based ip discovery
2025-07-30 15:59 [PATCH] drm/amdgpu/discovery: fix fw based ip discovery Alex Deucher
2025-08-04 14:16 ` Alex Deucher
2025-08-06 14:27 ` Mario Limonciello
@ 2025-08-06 15:17 ` Lazar, Lijo
2 siblings, 0 replies; 5+ messages in thread
From: Lazar, Lijo @ 2025-08-06 15:17 UTC (permalink / raw)
To: Alex Deucher, amd-gfx; +Cc: stable
On 7/30/2025 9:29 PM, Alex Deucher wrote:
> We only need the fw based discovery table for sysfs. No
> need to parse it. Additionally parsing some of the board
> specific tables may result in incorrect data on some boards.
> just load the binary and don't parse it on those boards.
>
> Closes: https://gitlab.freedesktop.org/drm/amd/-/issues/4441
> Fixes: 80a0e8282933 ("drm/amdgpu/discovery: optionally use fw based ip discovery")
> Cc: stable@vger.kernel.org
> Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
One generic question - if discovery content is completely ignored by
driver, how external tool using sysfs could consume the data? Wouldn't
there be a mismatch in the config?
Thanks,
Lijo
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 5 +-
> drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c | 72 ++++++++++---------
> 2 files changed, 41 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index efe98ffb679a4..b2538cff222ce 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -2570,9 +2570,6 @@ static int amdgpu_device_parse_gpu_info_fw(struct amdgpu_device *adev)
>
> adev->firmware.gpu_info_fw = NULL;
>
> - if (adev->mman.discovery_bin)
> - return 0;
> -
> switch (adev->asic_type) {
> default:
> return 0;
> @@ -2594,6 +2591,8 @@ static int amdgpu_device_parse_gpu_info_fw(struct amdgpu_device *adev)
> chip_name = "arcturus";
> break;
> case CHIP_NAVI12:
> + if (adev->mman.discovery_bin)
> + return 0;
> chip_name = "navi12";
> break;
> }
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
> index 81b3443c8d7f4..27bd7659961e8 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
> @@ -2555,40 +2555,11 @@ int amdgpu_discovery_set_ip_blocks(struct amdgpu_device *adev)
>
> switch (adev->asic_type) {
> case CHIP_VEGA10:
> - case CHIP_VEGA12:
> - case CHIP_RAVEN:
> - case CHIP_VEGA20:
> - case CHIP_ARCTURUS:
> - case CHIP_ALDEBARAN:
> - /* this is not fatal. We have a fallback below
> - * if the new firmwares are not present. some of
> - * this will be overridden below to keep things
> - * consistent with the current behavior.
> + /* This is not fatal. We only need the discovery
> + * binary for sysfs. We don't need it for a
> + * functional system.
> */
> - r = amdgpu_discovery_reg_base_init(adev);
> - if (!r) {
> - amdgpu_discovery_harvest_ip(adev);
> - amdgpu_discovery_get_gfx_info(adev);
> - amdgpu_discovery_get_mall_info(adev);
> - amdgpu_discovery_get_vcn_info(adev);
> - }
> - break;
> - default:
> - r = amdgpu_discovery_reg_base_init(adev);
> - if (r) {
> - drm_err(&adev->ddev, "discovery failed: %d\n", r);
> - return r;
> - }
> -
> - amdgpu_discovery_harvest_ip(adev);
> - amdgpu_discovery_get_gfx_info(adev);
> - amdgpu_discovery_get_mall_info(adev);
> - amdgpu_discovery_get_vcn_info(adev);
> - break;
> - }
> -
> - switch (adev->asic_type) {
> - case CHIP_VEGA10:
> + amdgpu_discovery_init(adev);
> vega10_reg_base_init(adev);
> adev->sdma.num_instances = 2;
> adev->gmc.num_umc = 4;
> @@ -2611,6 +2582,11 @@ int amdgpu_discovery_set_ip_blocks(struct amdgpu_device *adev)
> adev->ip_versions[DCI_HWIP][0] = IP_VERSION(12, 0, 0);
> break;
> case CHIP_VEGA12:
> + /* This is not fatal. We only need the discovery
> + * binary for sysfs. We don't need it for a
> + * functional system.
> + */
> + amdgpu_discovery_init(adev);
> vega10_reg_base_init(adev);
> adev->sdma.num_instances = 2;
> adev->gmc.num_umc = 4;
> @@ -2633,6 +2609,11 @@ int amdgpu_discovery_set_ip_blocks(struct amdgpu_device *adev)
> adev->ip_versions[DCI_HWIP][0] = IP_VERSION(12, 0, 1);
> break;
> case CHIP_RAVEN:
> + /* This is not fatal. We only need the discovery
> + * binary for sysfs. We don't need it for a
> + * functional system.
> + */
> + amdgpu_discovery_init(adev);
> vega10_reg_base_init(adev);
> adev->sdma.num_instances = 1;
> adev->vcn.num_vcn_inst = 1;
> @@ -2674,6 +2655,11 @@ int amdgpu_discovery_set_ip_blocks(struct amdgpu_device *adev)
> }
> break;
> case CHIP_VEGA20:
> + /* This is not fatal. We only need the discovery
> + * binary for sysfs. We don't need it for a
> + * functional system.
> + */
> + amdgpu_discovery_init(adev);
> vega20_reg_base_init(adev);
> adev->sdma.num_instances = 2;
> adev->gmc.num_umc = 8;
> @@ -2697,6 +2683,11 @@ int amdgpu_discovery_set_ip_blocks(struct amdgpu_device *adev)
> adev->ip_versions[DCI_HWIP][0] = IP_VERSION(12, 1, 0);
> break;
> case CHIP_ARCTURUS:
> + /* This is not fatal. We only need the discovery
> + * binary for sysfs. We don't need it for a
> + * functional system.
> + */
> + amdgpu_discovery_init(adev);
> arct_reg_base_init(adev);
> adev->sdma.num_instances = 8;
> adev->vcn.num_vcn_inst = 2;
> @@ -2725,6 +2716,11 @@ int amdgpu_discovery_set_ip_blocks(struct amdgpu_device *adev)
> adev->ip_versions[UVD_HWIP][1] = IP_VERSION(2, 5, 0);
> break;
> case CHIP_ALDEBARAN:
> + /* This is not fatal. We only need the discovery
> + * binary for sysfs. We don't need it for a
> + * functional system.
> + */
> + amdgpu_discovery_init(adev);
> aldebaran_reg_base_init(adev);
> adev->sdma.num_instances = 5;
> adev->vcn.num_vcn_inst = 2;
> @@ -2751,6 +2747,16 @@ int amdgpu_discovery_set_ip_blocks(struct amdgpu_device *adev)
> adev->ip_versions[XGMI_HWIP][0] = IP_VERSION(6, 1, 0);
> break;
> default:
> + r = amdgpu_discovery_reg_base_init(adev);
> + if (r) {
> + drm_err(&adev->ddev, "discovery failed: %d\n", r);
> + return r;
> + }
> +
> + amdgpu_discovery_harvest_ip(adev);
> + amdgpu_discovery_get_gfx_info(adev);
> + amdgpu_discovery_get_mall_info(adev);
> + amdgpu_discovery_get_vcn_info(adev);
> break;
> }
>
^ permalink raw reply [flat|nested] 5+ messages in thread