Re: [PATCH 1/3] drm/amdgpu: reserve 64M stolen memory for Apple 5K iMac

[Mario Limonciello <mario.limonciello@amd.com>]

On 10/27/26 18:59, Richard Lane wrote:
> On 27/10/2026 13:37, Alex Deucher wrote:
>> On Tue, Oct 27, 2026 at 9:16 AM Mario Limonciello
>> <mario.limonciello@amd.com> wrote:
>>>
>>>
>>>
>>> On 10/27/26 07:57, Alex Deucher wrote:
>>>> On Tue, Oct 27, 2026 at 8:21 AM Christian König
>>>> <christian.koenig@amd.com> wrote:
>>>>>
>>>>> On 10/26/26 23:46, Richard Lane wrote:
>>>>>> Apple 27" 5K iMacs report a single 2560x2880 tile (28.125 MiB) in
>>>>>> VIEWPORT_SIZE while scanning out up to 64 MiB across the relocated EFI
>>>>>> framebuffer.
>>>>>>
>>>>>> Override the stolen VGA memory allocation size to 64 MiB when matching
>>>>>> PCI_VENDOR_ID_APPLE with a 2560x2880x4 viewport size to prevent
>>>>>> framebuffer corruption during boot handover.
>>>>>
>>>>> Hacks like those are usually a big no-go.
>>>>>
>>>>> Why is that framebuffer handling not working with DC out of the box?
>>>>
>>>> The GMC IP code reserves the memory used by the display hardware from
>>>> the pre-OS console.  The gmc get_vbios_fb_size() callback looks to see
>>>> what display viewport was set by the pre-OS environment.  If the Macs
>>>> need a workaround, that's where it should go.  Since these displays
>>>> require multiple display pipes, my guess is you need to double the
>>>> allocation in that case since the current code just checks the first
>>>> pipe.
>>>
>>>
>>> Could this also be a logic error in that callback?  Would it be worth
>>> double checking all the register values in that callback for this system?
>>>
>>> I know for example there are a few different IP specific implementations
>>> in gmc_v9_0_get_vbios_fb_size().
>>
>> Not likely.  I suspect the issue is that these systems use two display
>> pipes so we only reserve enough memory to cover half of the display.
>> On PCs the vbios never sets up MST.
>>
>> Alex
>>
> 
> I tested doubling the allocation in gmc_v8_0_get_vbios_fb_size():
> 
> 	if (adev->pdev->subsystem_vendor == PCI_VENDOR_ID_APPLE &&
> 	    size == (2560 * 2880 * 4))
> 		size *= 2;
> 
> That eliminated the corruption across the middle (45-60%) of the screen,
> but still left a band of corruption along the very bottom (95-100%) of
> the screen.
> 
> Dumping the DCE registers inside gmc_v8_0_get_vbios_fb_size() on the
> iMac17,1 showed:
> 
>    vram_start = 0xf400000000, d1vga = 0x0
>    CRTC0: grph_en=0x1 surf_hi=0xf4 surf_lo=0x300000 pitch=5120 vp_size=2560x2880
>    CRTC1: grph_en=0x80000000 surf_hi=0x0 surf_lo=0x0 pitch=0 vp_size=0x0
> 
> Even though only CRTC0 is active at EFI boot with a 2560x2880 viewport,
> Apple's EFI allocates a full 5120-pixel stride surface (mmGRPH_PITCH) and places
> mmGRPH_PRIMARY_SURFACE_ADDRESS at a 3 MiB offset (0x300000) from
> vram_start.
> 
> Because CRTC0 scans out 2880 lines at a 5120-pixel pitch starting at
> offset 3 MiB, the active pre-OS scanout spans [3 MiB, 59.25 MiB) in VRAM:
> - Reserving 2560 * 2880 * 4 (28.125 MiB) places permanent driver BOs at
>    ~28.125..32 MiB, corrupting the middle (40-60%) of the screen.
> - Reserving 2 * 2560 * 2880 * 4 (56.25 MiB) places those BOs at
>    ~56.25..59.25 MiB, leaving the final 3 MiB exposed due to the 0x300000
>    surface offset and corrupting the bottom (95-100%) of the screen.
> - Accounting for the 5120 pitch and the 0x300000 surface offset reserves
>    the full 59.25 MiB span and eliminates the corruption completely.
> 
> For v2 of the patch, which of the following two approaches in both
> gmc_v7_0/v8_0_get_vbios_fb_size() would you prefer? Both tested working.

My feeling is that Option A is less magical.  I'd actually wonder if we 
should be doing this more widely.  I'd *think* the same problem could 
happen on a non-Apple display that behaves similarly.

> 
> Option A (change whole function):
> 
>   static unsigned gmc_v8_0_get_vbios_fb_size(struct amdgpu_device *adev)
>   {
>   	u32 d1vga_control = RREG32(mmD1VGA_CONTROL);
>   	unsigned size;
>   
>   	if (REG_GET_FIELD(d1vga_control, D1VGA_CONTROL, D1VGA_MODE_ENABLE)) {
>   		size = AMDGPU_VBIOS_VGA_ALLOCATION;
>   	} else {
>   		u32 viewport = RREG32(mmVIEWPORT_SIZE);
> -		size = (REG_GET_FIELD(viewport, VIEWPORT_SIZE, VIEWPORT_HEIGHT) *
> -			REG_GET_FIELD(viewport, VIEWPORT_SIZE, VIEWPORT_WIDTH) * 4);
> +		size = REG_GET_FIELD(viewport, VIEWPORT_SIZE, VIEWPORT_HEIGHT) *
> +		       RREG32(mmGRPH_PITCH) * 4;
> +		if (size)
> +			size += RREG32(mmGRPH_PRIMARY_SURFACE_ADDRESS) -
> +				lower_32_bits(adev->gmc.vram_start);
>   	}
>   
>   	return size;
>   }
> 
> Option B (add Apple 5K iMacs quirk):
> 
>   static unsigned gmc_v8_0_get_vbios_fb_size(struct amdgpu_device *adev)
>   {
>   	u32 d1vga_control = RREG32(mmD1VGA_CONTROL);
>   	unsigned size;
>   
>   	if (REG_GET_FIELD(d1vga_control, D1VGA_CONTROL, D1VGA_MODE_ENABLE)) {
>   		size = AMDGPU_VBIOS_VGA_ALLOCATION;
>   	} else {
>   		u32 viewport = RREG32(mmVIEWPORT_SIZE);
>   		size = (REG_GET_FIELD(viewport, VIEWPORT_SIZE, VIEWPORT_HEIGHT) *
>   			REG_GET_FIELD(viewport, VIEWPORT_SIZE, VIEWPORT_WIDTH) * 4);
> +
> +		if (adev->pdev->subsystem_vendor == PCI_VENDOR_ID_APPLE &&
> +		    size == (2560 * 2880 * 4))
> +			size = size * 2 + RREG32(mmGRPH_PRIMARY_SURFACE_ADDRESS);
>   	}
>   
>   	return size;
>   }
> 
> 
> Richard
> 
>>>
>>>>
>>>> Alex
>>>>
>>>>>
>>>>> Regards,
>>>>> Christian.
>>>>>
>>>>>>
>>>>>> Signed-off-by: Richard Lane <richardlanedev@gmail.com>
>>>>>> ---
>>>>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c | 10 +++++++++-
>>>>>>    1 file changed, 9 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
>>>>>> index 4204c3c750a8..8b196fbfa724 100644
>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
>>>>>> @@ -1232,7 +1232,15 @@ void amdgpu_gmc_init_vga_resv_regions(struct amdgpu_device *adev)
>>>>>>           } else {
>>>>>>                   size = amdgpu_gmc_get_vbios_fb_size(adev);
>>>>>>
>>>>>> -               if (adev->mman.keep_stolen_vga_memory)
>>>>>> +               /*
>>>>>> +                * Apple 5K iMacs report one 2560x2880 tile in VIEWPORT_SIZE
>>>>>> +                * while scanning out up to 64M across the relocated EFI
>>>>>> +                * framebuffer.
>>>>>> +                */
>>>>>> +               if (adev->pdev->subsystem_vendor == PCI_VENDOR_ID_APPLE &&
>>>>>> +                   size == (2560 * 2880 * 4))
>>>>>> +                       size = 64U * 1024 * 1024;
>>>>>> +               else if (adev->mman.keep_stolen_vga_memory)
>>>>>>                           size = max(size, (unsigned)AMDGPU_VBIOS_VGA_ALLOCATION);
>>>>>>           }
>>>>>>
>>>>>> --
>>>>>> 2.55.0
>>>>>
>>>
>