On Wed, Mar 5, 2025 at 4:53 PM Mario Limonciello
<mario.limonciello@xxxxxxx> wrote:
>
> On 3/5/2025 15:37, Mario Limonciello wrote:
> > Modern APU and dGPU require DC support to be able to light up the
> > display. If DC support has been disabled either by kernel config
> > or by kernel command line fail init early so that the system won't
> > freeze with a lack of display.
> >
> > Signed-off-by: Mario Limonciello <mario.limonciello@xxxxxxx>
> > ---
> > drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c | 24 +++++++++++++++----
> > 1 file changed, 19 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
> > index a4258127083d..c4e1505dcaac 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
> > @@ -2139,10 +2139,6 @@ static int amdgpu_discovery_set_display_ip_blocks(struct amdgpu_device *adev)
> > return 0;
> > }
> >
> > - if (!amdgpu_device_has_dc_support(adev))
> > - return 0;
> > -
> > -#if defined(CONFIG_DRM_AMD_DC)
> > if (amdgpu_ip_version(adev, DCE_HWIP, 0)) {
> > switch (amdgpu_ip_version(adev, DCE_HWIP, 0)) {
> > case IP_VERSION(1, 0, 0):
> > @@ -2166,15 +2162,24 @@ static int amdgpu_discovery_set_display_ip_blocks(struct amdgpu_device *adev)
> > case IP_VERSION(3, 5, 1):
> > case IP_VERSION(3, 6, 0):
> > case IP_VERSION(4, 1, 0):
> > + if (!amdgpu_device_has_dc_support(adev)) {
> > + dev_err(adev->dev,
> > + "DC support is required for dm ip block(DCE_HWIP:0x%x)\n",
> > + amdgpu_ip_version(adev, DCE_HWIP, 0));
> > + return -EINVAL;
> > + }
> > +
> > /* TODO: Fix IP version. DC code expects version 4.0.1 */
> > if (adev->ip_versions[DCE_HWIP][0] == IP_VERSION(4, 1, 0))
> > adev->ip_versions[DCE_HWIP][0] = IP_VERSION(4, 0, 1);
> >
> > +#if defined(CONFIG_DRM_AMD_DC)
> > if (amdgpu_sriov_vf(adev))
> > amdgpu_discovery_set_sriov_display(adev);
> > else
> > amdgpu_device_ip_block_add(adev, &dm_ip_block);
> > break;
> > +#endif
> > default:
>
> Looking closer at this failure path I *think* this patch will cause
> issues on GPU without DC support.
>
> In the 'default' case now I think it should do something like this:
>
> if (amdgpu_device_has_dc_support(adev))
> //existing error flow
> else
> return 0;
>
> Agree?
I think it would be better to just dump a warning in the log if the
board supports displays but CONFIG_DRM_AMD_DC=n rather than failing to
initialize the driver. The end result is pretty much the same and it
wouldn't break users that want this scenario for some reason.
Alex
>
> > dev_err(adev->dev,
> > "Failed to add dm ip block(DCE_HWIP:0x%x)\n",
> > @@ -2186,11 +2191,21 @@ static int amdgpu_discovery_set_display_ip_blocks(struct amdgpu_device *adev)
> > case IP_VERSION(12, 0, 0):
> > case IP_VERSION(12, 0, 1):
> > case IP_VERSION(12, 1, 0):
> > +
> > + if (!amdgpu_device_has_dc_support(adev)) {
> > + dev_err(adev->dev,
> > + "DC support is required for dm ip block(DCI_HWIP:0x%x)\n",
> > + amdgpu_ip_version(adev, DCI_HWIP, 0));
> > + return -EINVAL;
> > + }
> > +
> > +#if defined(CONFIG_DRM_AMD_DC)
> > if (amdgpu_sriov_vf(adev))
> > amdgpu_discovery_set_sriov_display(adev);
> > else
> > amdgpu_device_ip_block_add(adev, &dm_ip_block);
> > break;
> > +#endif
> > default:
> > dev_err(adev->dev,
> > "Failed to add dm ip block(DCI_HWIP:0x%x)\n",
> > @@ -2198,7 +2213,6 @@ static int amdgpu_discovery_set_display_ip_blocks(struct amdgpu_device *adev)
> > return -EINVAL;
> > }
> > }
> > -#endif
> > return 0;
> > }
> >
>
