On 2017-11-01 11:16 PM, Pixel Ding wrote:
> From: pding <Pixel.Ding at amd.com>
>
> Add amdgpu_device_alloc() which was part of previous
> amdgpu_device_init(). Then it's flexible to handle init
> sequence since kfd has dependency to amdgpu_device base
> fields.
>
> Signed-off-by: pding <Pixel.Ding at amd.com>
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu.h | 4 +--
> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 54 +++++++++++++++++++++++-------
> drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 18 +++-------
> 3 files changed, 49 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> index 2730a75..6f2dc2b 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> @@ -1655,11 +1655,11 @@ static inline struct amdgpu_device *amdgpu_ttm_adev(struct ttm_bo_device *bdev)
> {
> return container_of(bdev, struct amdgpu_device, mman.bdev);
> }
> -
> -int amdgpu_device_init(struct amdgpu_device *adev,
> +int amdgpu_device_alloc(struct amdgpu_device **padev,
> struct drm_device *ddev,
> struct pci_dev *pdev,
> uint32_t flags);
> +int amdgpu_device_init(struct amdgpu_device *adev);
> void amdgpu_device_fini(struct amdgpu_device *adev);
> int amdgpu_gpu_wait_for_idle(struct amdgpu_device *adev);
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index 068b56a..809e656 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -2092,25 +2092,33 @@ bool amdgpu_device_has_dc_support(struct amdgpu_device *adev)
> }
>
> /**
> - * amdgpu_device_init - initialize the driver
> + * amdgpu_device_alloc - allocate/init amdgpu_device structure
I'm not sure this is a good name for this function. It does a lot more
than just allocate. It also initializes a bunch of fields. Maybe call it
something like amdgpu_device_early_init.
Also, the allocation doesn't really have to be part of it. That could
just stay where it is in amdgpu_driver_load_kms. As I understand it, the
key is that you split amdgpu_device_init into two pieces, so that you
initialized just enough to start probing KFD before you get to the
actual device initialization.
That said, an easier and cleaner solution may be to move
amdgpu_amdkfd_device_probe into amdgpu_early_init and
amdgpu_amdkfd_device_init into amdgpu_init.
Regards,
 Felix
> *
> - * @adev: amdgpu_device pointer
> + * @padev: pointer to amdgpu_device pointer
> * @pdev: drm dev pointer
> * @pdev: pci dev pointer
> * @flags: driver flags
> *
> - * Initializes the driver info and hw (all asics).
> * Returns 0 for success or an error on failure.
> - * Called at driver startup.
> */
> -int amdgpu_device_init(struct amdgpu_device *adev,
> - struct drm_device *ddev,
> - struct pci_dev *pdev,
> - uint32_t flags)
> +int amdgpu_device_alloc(struct amdgpu_device **padev,
> + struct drm_device *ddev,
> + struct pci_dev *pdev,
> + uint32_t flags)
> {
> - int r, i;
> - bool runtime = false;
> - u32 max_MBps;
> + struct amdgpu_device *adev;
> +
> + adev = kzalloc(sizeof(struct amdgpu_device), GFP_KERNEL);
> + if (adev == NULL)
> + return -ENOMEM;
> +
> + if ((amdgpu_runtime_pm != 0) &&
> + amdgpu_has_atpx() &&
> + (amdgpu_is_atpx_hybrid() ||
> + amdgpu_has_atpx_dgpu_power_cntl()) &&
> + ((flags & AMD_IS_APU) == 0) &&
> + !pci_is_thunderbolt_attached(pdev))
> + flags |= AMD_IS_PX;
>
> adev->shutdown = false;
> adev->dev = &pdev->dev;
> @@ -2183,6 +2191,28 @@ int amdgpu_device_init(struct amdgpu_device *adev,
>
> INIT_DELAYED_WORK(&adev->late_init_work, amdgpu_late_init_func_handler);
>
> + *padev = adev;
> +
> + return 0;
> +}
> +/**
> + * amdgpu_device_init - initialize the driver
> + *
> + * @adev: amdgpu_device pointer
> + * @pdev: drm dev pointer
> + * @pdev: pci dev pointer
> + * @flags: driver flags
> + *
> + * Initializes the driver info and hw (all asics).
> + * Returns 0 for success or an error on failure.
> + * Called at driver startup.
> + */
> +int amdgpu_device_init(struct amdgpu_device *adev)
> +{
> + int r, i;
> + bool runtime = false;
> + u32 max_MBps;
> +
> /* Registers mapping */
> /* TODO: block userspace mapping of io register */
> if (adev->asic_type >= CHIP_BONAIRE) {
> @@ -2226,7 +2256,7 @@ int amdgpu_device_init(struct amdgpu_device *adev,
>
> if (amdgpu_runtime_pm == 1)
> runtime = true;
> - if (amdgpu_device_is_px(ddev))
> + if (amdgpu_device_is_px(adev->ddev))
> runtime = true;
> if (!pci_is_thunderbolt_attached(adev->pdev))
> vga_switcheroo_register_client(adev->pdev,
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> index 3e9760d..acdb010 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> @@ -124,19 +124,11 @@ int amdgpu_driver_load_kms(struct drm_device *dev, unsigned long flags)
> #endif
> retry_init:
>
> - adev = kzalloc(sizeof(struct amdgpu_device), GFP_KERNEL);
> - if (adev == NULL) {
> - return -ENOMEM;
> - }
> - dev->dev_private = (void *)adev;
> + r = amdgpu_device_alloc(&adev, dev, dev->pdev, flags);
> + if (r)
> + return r;
>
> - if ((amdgpu_runtime_pm != 0) &&
> - amdgpu_has_atpx() &&
> - (amdgpu_is_atpx_hybrid() ||
> - amdgpu_has_atpx_dgpu_power_cntl()) &&
> - ((flags & AMD_IS_APU) == 0) &&
> - !pci_is_thunderbolt_attached(dev->pdev))
> - flags |= AMD_IS_PX;
> + dev->dev_private = (void *)adev;
>
> /* amdgpu_device_init should report only fatal error
> * like memory allocation failure or iomapping failure,
> @@ -144,7 +136,7 @@ int amdgpu_driver_load_kms(struct drm_device *dev, unsigned long flags)
> * properly initialize the GPU MC controller and permit
> * VRAM allocation
> */
> - r = amdgpu_device_init(adev, dev, dev->pdev, flags);
> + r = amdgpu_device_init(adev);
> if (r == -EAGAIN && ++retry <= 3) {
> adev->virt.caps &= ~AMDGPU_SRIOV_CAPS_RUNTIME;
> adev->virt.ops = NULL;
