On Fri, Apr 6, 2018 at 3:04 PM, Christian König
<ckoenig.leichtzumerken at gmail.com> wrote:
> Am 06.04.2018 um 19:24 schrieb Alex Deucher:
>>
>> Steal 9 MB for vga emulation and fb if vga is enabled, otherwise,
>> steal enough to cover the current display size as set by the vbios.
>>
>> If no memory is used (e.g., secondary or headless card), skip
>> stolen memory reserve.
>>
>> Signed-off-by: Alex Deucher <alexander.deucher at amd.com>
>> ---
>> drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 17 +++++++------
>> drivers/gpu/drm/amd/amdgpu/gmc_v6_0.c | 17 ++++++++++++-
>> drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c | 17 ++++++++++++-
>> drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c | 17 ++++++++++++-
>> drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c | 42
>> ++++++++++++++++++++++++++++++++-
>> 5 files changed, 99 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>> index 205da3ff9cd0..2de2f5e5a0f9 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>> @@ -1454,12 +1454,14 @@ int amdgpu_ttm_init(struct amdgpu_device *adev)
>> return r;
>> }
>> - r = amdgpu_bo_create_kernel(adev, adev->gmc.stolen_size,
>> PAGE_SIZE,
>> - AMDGPU_GEM_DOMAIN_VRAM,
>> - &adev->stolen_vga_memory,
>> - NULL, NULL);
>> - if (r)
>> - return r;
>> + if (adev->gmc.stolen_size) {
>> + r = amdgpu_bo_create_kernel(adev, adev->gmc.stolen_size,
>> PAGE_SIZE,
>> + AMDGPU_GEM_DOMAIN_VRAM,
>> + &adev->stolen_vga_memory,
>> + NULL, NULL);
>> + if (r)
>> + return r;
>> + }
>
>
> We should probably rather handle zero size in amdgpu_bo_create_kernel() as
> just setting the pointer to NULL instead of trying to allocate a zero sized
> BO.
yeah, I guess that would work too.
>
>> DRM_INFO("amdgpu: %uM of VRAM memory ready\n",
>> (unsigned) (adev->gmc.real_vram_size / (1024 * 1024)));
>> @@ -1534,7 +1536,8 @@ void amdgpu_ttm_fini(struct amdgpu_device *adev)
>> return;
>> amdgpu_ttm_debugfs_fini(adev);
>> - amdgpu_bo_free_kernel(&adev->stolen_vga_memory, NULL, NULL);
>> + if (adev->gmc.stolen_size)
>> + amdgpu_bo_free_kernel(&adev->stolen_vga_memory, NULL,
>> NULL);
>
>
> The if can be skipped here, freeing a NULL pointer is harmless.
Ok.
>
>
>> amdgpu_ttm_fw_reserve_vram_fini(adev);
>> if (adev->mman.aper_base_kaddr)
>> iounmap(adev->mman.aper_base_kaddr);
>> diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v6_0.c
>> b/drivers/gpu/drm/amd/amdgpu/gmc_v6_0.c
>> index 5617cf62c566..63f0b65854a3 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/gmc_v6_0.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v6_0.c
>> @@ -825,6 +825,21 @@ static int gmc_v6_0_late_init(void *handle)
>> return 0;
>> }
>> +static unsigned gmc_v6_0_get_vbios_fb_size(struct amdgpu_device *adev)
>> +{
>> + u32 d1vga_control = RREG32(mmD1VGA_CONTROL);
>> +
>> + if (REG_GET_FIELD(d1vga_control, D1VGA_CONTROL,
>> D1VGA_MODE_ENABLE)) {
>> + return 9 * 1024 * 1024; /* reserve 8MB for vga emulator
>> and 1 MB for FB */
>> + } else {
>> + u32 viewport = RREG32(mmVIEWPORT_SIZE);
>> + unsigned size = (REG_GET_FIELD(viewport, VIEWPORT_SIZE,
>> VIEWPORT_HEIGHT) *
>> + REG_GET_FIELD(viewport, VIEWPORT_SIZE,
>> VIEWPORT_WIDTH) *
>> + 4);
>> + return size;
>> + }
>> +}
>> +
>> static int gmc_v6_0_sw_init(void *handle)
>> {
>> int r;
>> @@ -851,7 +866,7 @@ static int gmc_v6_0_sw_init(void *handle)
>> adev->gmc.mc_mask = 0xffffffffffULL;
>> - adev->gmc.stolen_size = 256 * 1024;
>> + adev->gmc.stolen_size = gmc_v6_0_get_vbios_fb_size(adev);
>> adev->need_dma32 = false;
>> dma_bits = adev->need_dma32 ? 32 : 40;
>> diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c
>> b/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c
>> index 80054f36e487..2deb5c93ef28 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c
>> @@ -964,6 +964,21 @@ static int gmc_v7_0_late_init(void *handle)
>> return 0;
>> }
>> +static unsigned gmc_v7_0_get_vbios_fb_size(struct amdgpu_device *adev)
>> +{
>> + u32 d1vga_control = RREG32(mmD1VGA_CONTROL);
>> +
>> + if (REG_GET_FIELD(d1vga_control, D1VGA_CONTROL,
>> D1VGA_MODE_ENABLE)) {
>> + return 9 * 1024 * 1024; /* reserve 8MB for vga emulator
>> and 1 MB for FB */
>> + } else {
>> + u32 viewport = RREG32(mmVIEWPORT_SIZE);
>> + unsigned size = (REG_GET_FIELD(viewport, VIEWPORT_SIZE,
>> VIEWPORT_HEIGHT) *
>> + REG_GET_FIELD(viewport, VIEWPORT_SIZE,
>> VIEWPORT_WIDTH) *
>> + 4);
>> + return size;
>> + }
>> +}
>> +
>> static int gmc_v7_0_sw_init(void *handle)
>> {
>> int r;
>> @@ -998,7 +1013,7 @@ static int gmc_v7_0_sw_init(void *handle)
>> */
>> adev->gmc.mc_mask = 0xffffffffffULL; /* 40 bit MC */
>> - adev->gmc.stolen_size = 256 * 1024;
>> + adev->gmc.stolen_size = gmc_v7_0_get_vbios_fb_size(adev);
>> /* set DMA mask + need_dma32 flags.
>> * PCIE - can handle 40-bits.
>> diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c
>> b/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c
>> index d71d4cb68f9c..04b00df3c6e6 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c
>> @@ -1055,6 +1055,21 @@ static int gmc_v8_0_late_init(void *handle)
>> return 0;
>> }
>> +static unsigned gmc_v8_0_get_vbios_fb_size(struct amdgpu_device *adev)
>> +{
>> + u32 d1vga_control = RREG32(mmD1VGA_CONTROL);
>> +
>> + if (REG_GET_FIELD(d1vga_control, D1VGA_CONTROL,
>> D1VGA_MODE_ENABLE)) {
>> + return 9 * 1024 * 1024; /* reserve 8MB for vga emulator
>> and 1 MB for FB */
>> + } else {
>> + u32 viewport = RREG32(mmVIEWPORT_SIZE);
>> + unsigned size = (REG_GET_FIELD(viewport, VIEWPORT_SIZE,
>> VIEWPORT_HEIGHT) *
>> + REG_GET_FIELD(viewport, VIEWPORT_SIZE,
>> VIEWPORT_WIDTH) *
>> + 4);
>> + return size;
>> + }
>> +}
>> +
>> #define mmMC_SEQ_MISC0_FIJI 0xA71
>> static int gmc_v8_0_sw_init(void *handle)
>> @@ -1096,7 +1111,7 @@ static int gmc_v8_0_sw_init(void *handle)
>> */
>> adev->gmc.mc_mask = 0xffffffffffULL; /* 40 bit MC */
>> - adev->gmc.stolen_size = 256 * 1024;
>> + adev->gmc.stolen_size = gmc_v8_0_get_vbios_fb_size(adev);
>> /* set DMA mask + need_dma32 flags.
>> * PCIE - can handle 40-bits.
>> diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
>> b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
>> index 070946e1e4a7..252a6c6998b7 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
>> @@ -793,6 +793,46 @@ static int gmc_v9_0_gart_init(struct amdgpu_device
>> *adev)
>> return amdgpu_gart_table_vram_alloc(adev);
>> }
>> +#define mmHUBP0_DCSURF_PRI_VIEWPORT_DIMENSION
>> 0x055d
>> +#define mmHUBP0_DCSURF_PRI_VIEWPORT_DIMENSION_BASE_IDX
>> 2
>> +#define HUBP0_DCSURF_PRI_VIEWPORT_DIMENSION__PRI_VIEWPORT_WIDTH__SHIFT
>> 0x0
>> +#define HUBP0_DCSURF_PRI_VIEWPORT_DIMENSION__PRI_VIEWPORT_HEIGHT__SHIFT
>> 0x10
>> +#define HUBP0_DCSURF_PRI_VIEWPORT_DIMENSION__PRI_VIEWPORT_WIDTH_MASK
>> 0x00003FFFL
>> +#define HUBP0_DCSURF_PRI_VIEWPORT_DIMENSION__PRI_VIEWPORT_HEIGHT_MASK
>> 0x3FFF0000L
>
>
> Mhm, hub defines in the middle of gmc_v9_0.c? At least put them at the start
> of the file.
OK.
>
> But I would rather see that delegated to DAL or something like this, cause
> that doesn't really looks GMC related.
We already include the DCE headers in all the gmc files because we
already have to snoop some of the vga registers for other things. The
only reason I added the DCN register defines was because the viewport
register is different between DCE and DCN. It seemed like overkill to
add an api to DC just to fetch one register.
The other issue is what to do about boards like APUs with limited
vram? E.g., what if the board only has 32MB of vram and the vbios is
using 32MB of vram for the pre-OS display. Seems like in that case,
just setting the stolen size to 0 and dealing with potential garbage
on the display during the transition from vbios to driver is an ok
trade off. I'm not sure how often this will come up in practice.
I'll respin the patches.
Alex
>
> Christian.
>
>
>> +
>> +static unsigned gmc_v9_0_get_vbios_fb_size(struct amdgpu_device *adev)
>> +{
>> + u32 d1vga_control = RREG32_SOC15(DCE, 0, mmD1VGA_CONTROL);
>> +
>> + if (REG_GET_FIELD(d1vga_control, D1VGA_CONTROL,
>> D1VGA_MODE_ENABLE)) {
>> + return 9 * 1024 * 1024; /* reserve 8MB for vga emulator
>> and 1 MB for FB */
>> + } else {
>> + u32 viewport;
>> + unsigned size;
>> +
>> + switch (adev->asic_type) {
>> + case CHIP_RAVEN:
>> + viewport = RREG32_SOC15(DCE, 0,
>> mmHUBP0_DCSURF_PRI_VIEWPORT_DIMENSION);
>> + size = (REG_GET_FIELD(viewport,
>> +
>> HUBP0_DCSURF_PRI_VIEWPORT_DIMENSION, PRI_VIEWPORT_HEIGHT) *
>> + REG_GET_FIELD(viewport,
>> +
>> HUBP0_DCSURF_PRI_VIEWPORT_DIMENSION, PRI_VIEWPORT_WIDTH) *
>> + 4);
>> + break;
>> + case CHIP_VEGA10:
>> + case CHIP_VEGA12:
>> + default:
>> + viewport = RREG32_SOC15(DCE, 0,
>> mmSCL0_VIEWPORT_SIZE);
>> + size = (REG_GET_FIELD(viewport,
>> SCL0_VIEWPORT_SIZE, VIEWPORT_HEIGHT) *
>> + REG_GET_FIELD(viewport,
>> SCL0_VIEWPORT_SIZE, VIEWPORT_WIDTH) *
>> + 4);
>> + break;
>> + }
>> +
>> + return size;
>> + }
>> +}
>> +
>> static int gmc_v9_0_sw_init(void *handle)
>> {
>> int r;
>> @@ -848,7 +888,7 @@ static int gmc_v9_0_sw_init(void *handle)
>> * It needs to reserve 8M stolen memory for vega10
>> * TODO: Figure out how to avoid that...
>> */
>> - adev->gmc.stolen_size = 8 * 1024 * 1024;
>> + adev->gmc.stolen_size = gmc_v9_0_get_vbios_fb_size(adev);
>> /* set DMA mask + need_dma32 flags.
>> * PCIE - can handle 44-bits.
>
>
