[PATCH v2] drm/amd: Fail initialization earlier when DC is disabled
Mario Limonciello
mario.limonciello at amd.com
Thu Mar 6 20:13:30 UTC 2025
On 3/6/2025 14:11, Alex Deucher wrote:
> On Thu, Mar 6, 2025 at 2:31 PM Mario Limonciello
> <mario.limonciello at amd.com> wrote:
>>
>> On 3/6/2025 13:19, Alex Deucher wrote:
>>> On Thu, Mar 6, 2025 at 1:58 PM Mario Limonciello
>>> <mario.limonciello at amd.com> 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 the screen will visibly freeze when the
>>>> driver finishes early init.
>>>>
>>>> As it's known before early init is done whether DC support is required
>>>> detect this during discovery and bail if DC support was disabled
>>>> for any reason. This will ensure that the existing framebuffer
>>>> provided by efifb or simpledrm keeps working.
>>>
>>> I think there are a couple of corner cases we need to handle:
>>> 1. if adev->enable_virtual_display is set. The user has configured
>>> virtual displays and hence they want to use them rather than the
>>> actual physical displays. This is useful with GPUs in servers or for
>>> early bring up.
>> > 2. If the board supports DCN IP, but all it's been fused off due to>
>> silicon flaws (e.g., adev->harvest_ip_mask & AMD_HARVEST_IP_DMU_MASK).
>>> In that case, we don't want to fail.
>>
>> In that case I wonder if it's better to use
>> amdgpu_device_asic_has_dc_support() instead of
>> amdgpu_device_has_dc_support() which should cover both of those concerns.
>
> That should work, or maybe just warn once in
> amdgpu_device_asic_has_dc_support(). E.g., something like:
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index 1b9b4f8daf531..c986e619dbe99 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -3988,6 +3988,8 @@ bool amdgpu_device_asic_has_dc_support(enum
> amd_asic_type asic_type)
> */
> return amdgpu_dc > 0;
> default:
> + if (amdgpu_dc == 0)
> + DRM_INFO_ONCE("Display Core has been disable
> via kernel parameter, No display!\n");
> return amdgpu_dc != 0;
> #else
> default:
>
The problem is without a display that message will probably not be seen
unless someone knows to look for journalctl -k -b-1 or similar.
So my main concern is that people who shoot themselves in the foot at
least have a display to see the hole in their foot.
I'll have a try with my other idea and follow up with a v3 if I'm happy
with that.
>
>>
>>>
>>> Alex
>>>
>>>>
>>>> Signed-off-by: Mario Limonciello <mario.limonciello at amd.com>
>>>> ---
>>>> v2:
>>>> * Update commit message justification
>>>> * Add correct "default" handling
>>>> ---
>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c | 46 +++++++++++++------
>>>> 1 file changed, 33 insertions(+), 13 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
>>>> index a4258127083d..24f532de6322 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,39 +2162,63 @@ 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:
>>>> - dev_err(adev->dev,
>>>> - "Failed to add dm ip block(DCE_HWIP:0x%x)\n",
>>>> - amdgpu_ip_version(adev, DCE_HWIP, 0));
>>>> - return -EINVAL;
>>>> + if (amdgpu_device_has_dc_support(adev)) {
>>>> + dev_err(adev->dev,
>>>> + "Failed to add dm ip block(DCE_HWIP:0x%x)\n",
>>>> + amdgpu_ip_version(adev, DCE_HWIP, 0));
>>>> + return -EINVAL;
>>>> + }
>>>> + return 0;
>>>> }
>>>> } else if (amdgpu_ip_version(adev, DCI_HWIP, 0)) {
>>>> switch (amdgpu_ip_version(adev, DCI_HWIP, 0)) {
>>>> 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",
>>>> - amdgpu_ip_version(adev, DCI_HWIP, 0));
>>>> - return -EINVAL;
>>>> + if (amdgpu_device_has_dc_support(adev)) {
>>>> + dev_err(adev->dev,
>>>> + "Failed to add dm ip block(DCI_HWIP:0x%x)\n",
>>>> + amdgpu_ip_version(adev, DCI_HWIP, 0));
>>>> + return -EINVAL;
>>>> + }
>>>> + return 0;
>>>> }
>>>> }
>>>> -#endif
>>>> return 0;
>>>> }
>>>>
>>>> --
>>>> 2.48.1
>>>>
>>
More information about the amd-gfx
mailing list