Re: [PATCH v5] drm/amdkfd: implement the dGPU fallback path for apu (v5)

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Am 2020-08-21 um 9:42 p.m. schrieb Huang Rui:
> On Sat, Aug 22, 2020 at 12:48:00AM +0800, Kuehling, Felix wrote:
>> Am 2020-08-21 um 8:50 a.m. schrieb Huang Rui:
>>> We still have a few iommu issues which need to address, so force raven
>>> as "dgpu" path for the moment.
>>>
>>> This is to add the fallback path to bypass IOMMU if IOMMU v2 is disabled
>>> or ACPI CRAT table not correct.
>>>
>>> v2: Use ignore_crat parameter to decide whether it will go with IOMMUv2.
>>> v3: Align with existed thunk, don't change the way of raven, only renoir
>>>     will use "dgpu" path by default.
>>> v4: don't update global ignore_crat in the driver, and revise fallback
>>>     function if CRAT is broken.
>>> v5: refine acpi crat good but no iommu support case, and rename the
>>>     title.
>>>
>>> Signed-off-by: Huang Rui <ray.huang@xxxxxxx>
>>> ---
>>>  drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c       |  5 ++-
>>>  drivers/gpu/drm/amd/amdkfd/kfd_chardev.c      |  2 +-
>>>  drivers/gpu/drm/amd/amdkfd/kfd_crat.c         | 22 ++++++++++--
>>>  drivers/gpu/drm/amd/amdkfd/kfd_dbgdev.c       |  4 +--
>>>  drivers/gpu/drm/amd/amdkfd/kfd_device.c       |  5 ++-
>>>  .../amd/amdkfd/kfd_device_queue_manager_v9.c  |  2 +-
>>>  drivers/gpu/drm/amd/amdkfd/kfd_flat_memory.c  |  4 +--
>>>  drivers/gpu/drm/amd/amdkfd/kfd_iommu.c        | 10 +++---
>>>  drivers/gpu/drm/amd/amdkfd/kfd_priv.h         |  5 ++-
>>>  drivers/gpu/drm/amd/amdkfd/kfd_topology.c     | 36 +++++++++++++++++--
>>>  10 files changed, 76 insertions(+), 19 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>> index a9a4319c24ae..189f9d7e190d 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>> @@ -684,11 +684,14 @@ MODULE_PARM_DESC(debug_largebar,
>>>   * Ignore CRAT table during KFD initialization. By default, KFD uses the ACPI CRAT
>>>   * table to get information about AMD APUs. This option can serve as a workaround on
>>>   * systems with a broken CRAT table.
>>> + *
>>> + * Default is auto (according to asic type, iommu_v2, and crat table, to decide
>>> + * whehter use CRAT)
>>>   */
>>>  int ignore_crat;
>>>  module_param(ignore_crat, int, 0444);
>>>  MODULE_PARM_DESC(ignore_crat,
>>> -	"Ignore CRAT table during KFD initialization (0 = use CRAT (default), 1 = ignore CRAT)");
>>> +	"Ignore CRAT table during KFD initialization (0 = auto (default), 1 = ignore CRAT)");
>>>  
>>>  /**
>>>   * DOC: halt_if_hws_hang (int)
>>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
>>> index 1b60e0ed6b5c..3ec5322ba88d 100644
>>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
>>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
>>> @@ -1258,7 +1258,7 @@ bool kfd_dev_is_large_bar(struct kfd_dev *dev)
>>>  		return true;
>>>  	}
>>>  
>>> -	if (dev->device_info->needs_iommu_device)
>>> +	if (dev->use_iommu_v2)
>>>  		return false;
>>>  
>>>  	amdgpu_amdkfd_get_local_mem_info(dev->kgd, &mem_info);
>>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_crat.c b/drivers/gpu/drm/amd/amdkfd/kfd_crat.c
>>> index 59557e3e206a..aba40df189d9 100644
>>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_crat.c
>>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_crat.c
>>> @@ -740,6 +740,25 @@ static int kfd_fill_gpu_cache_info(struct kfd_dev *kdev,
>>>  	return 0;
>>>  }
>>>  
>>> +
>>> +#ifdef CONFIG_ACPI
>>> +
>>> +static bool kfd_ignore_crat(void)
>>> +{
>>> +	bool ret;
>>> +
>>> +	if (ignore_crat)
>>> +		return true;
>>> +
>>> +#ifndef KFD_SUPPORT_IOMMU_V2
>>> +	ret = true;
>>> +#else
>>> +	ret = false;
>>> +#endif
>>> +
>>> +	return ret;
>>> +}
>>> +
>>>  /*
>>>   * kfd_create_crat_image_acpi - Allocates memory for CRAT image and
>>>   * copies CRAT from ACPI (if available).
>>> @@ -751,7 +770,6 @@ static int kfd_fill_gpu_cache_info(struct kfd_dev *kdev,
>>>   *
>>>   *	Return 0 if successful else return error code
>>>   */
>>> -#ifdef CONFIG_ACPI
>>>  int kfd_create_crat_image_acpi(void **crat_image, size_t *size)
>>>  {
>>>  	struct acpi_table_header *crat_table;
>>> @@ -775,7 +793,7 @@ int kfd_create_crat_image_acpi(void **crat_image, size_t *size)
>>>  		return -EINVAL;
>>>  	}
>>>  
>>> -	if (ignore_crat) {
>>> +	if (kfd_ignore_crat()) {
>>>  		pr_info("CRAT table disabled by module option\n");
>>>  		return -ENODATA;
>>>  	}
>>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_dbgdev.c b/drivers/gpu/drm/amd/amdkfd/kfd_dbgdev.c
>>> index 3e5904f8876a..e4159b5f83d5 100644
>>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_dbgdev.c
>>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_dbgdev.c
>>> @@ -309,7 +309,7 @@ static int dbgdev_address_watch_nodiq(struct kfd_dbgdev *dbgdev,
>>>  	for (i = 0; i < adw_info->num_watch_points; i++) {
>>>  		dbgdev_address_watch_set_registers(adw_info, &addrHi, &addrLo,
>>>  				&cntl, i, pdd->qpd.vmid,
>>> -				dbgdev->dev->device_info->needs_iommu_device);
>>> +				dbgdev->dev->use_iommu_v2);
>>>  
>>>  		pr_debug("\t\t%30s\n", "* * * * * * * * * * * * * * * * * *");
>>>  		pr_debug("\t\t%20s %08x\n", "register index :", i);
>>> @@ -399,7 +399,7 @@ static int dbgdev_address_watch_diq(struct kfd_dbgdev *dbgdev,
>>>  	for (i = 0; i < adw_info->num_watch_points; i++) {
>>>  		dbgdev_address_watch_set_registers(adw_info, &addrHi, &addrLo,
>>>  				&cntl, i, vmid,
>>> -				dbgdev->dev->device_info->needs_iommu_device);
>>> +				dbgdev->dev->use_iommu_v2);
>>>  
>>>  		pr_debug("\t\t%30s\n", "* * * * * * * * * * * * * * * * * *");
>>>  		pr_debug("\t\t%20s %08x\n", "register index :", i);
>>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_device.c b/drivers/gpu/drm/amd/amdkfd/kfd_device.c
>>> index 2c030c2b5b8d..fdf64d361be3 100644
>>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_device.c
>>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_device.c
>>> @@ -112,6 +112,7 @@ static const struct kfd_device_info carrizo_device_info = {
>>>  	.num_xgmi_sdma_engines = 0,
>>>  	.num_sdma_queues_per_engine = 2,
>>>  };
>>> +#endif
>>>  
>>>  static const struct kfd_device_info raven_device_info = {
>>>  	.asic_family = CHIP_RAVEN,
>>> @@ -130,7 +131,6 @@ static const struct kfd_device_info raven_device_info = {
>>>  	.num_xgmi_sdma_engines = 0,
>>>  	.num_sdma_queues_per_engine = 2,
>>>  };
>>> -#endif
>>>  
>>>  static const struct kfd_device_info hawaii_device_info = {
>>>  	.asic_family = CHIP_HAWAII,
>>> @@ -688,6 +688,9 @@ bool kgd2kfd_device_init(struct kfd_dev *kfd,
>>>  		goto gws_error;
>>>  	}
>>>  
>>> +	/* If CRAT is broken, won't set iommu enabled */
>>> +	kfd_double_confirm_iommu_support(kfd);
>>> +
>>>  	if (kfd_iommu_device_init(kfd)) {
>>>  		dev_err(kfd_device, "Error initializing iommuv2\n");
>>>  		goto device_iommu_error;
>>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager_v9.c b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager_v9.c
>>> index 95a82ac455f2..309f63a0b34a 100644
>>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager_v9.c
>>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager_v9.c
>>> @@ -62,7 +62,7 @@ static int update_qpd_v9(struct device_queue_manager *dqm,
>>>  				SH_MEM_ALIGNMENT_MODE_UNALIGNED <<
>>>  					SH_MEM_CONFIG__ALIGNMENT_MODE__SHIFT;
>>>  		if (amdgpu_noretry &&
>>> -		    !dqm->dev->device_info->needs_iommu_device)
>>> +		    !dqm->dev->use_iommu_v2)
>>>  			qpd->sh_mem_config |=
>>>  				1 << SH_MEM_CONFIG__RETRY_DISABLE__SHIFT;
>>>  
>>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_flat_memory.c b/drivers/gpu/drm/amd/amdkfd/kfd_flat_memory.c
>>> index 78714f9a8b11..798303c3c7ac 100644
>>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_flat_memory.c
>>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_flat_memory.c
>>> @@ -321,7 +321,7 @@ static void kfd_init_apertures_vi(struct kfd_process_device *pdd, uint8_t id)
>>>  	pdd->lds_base = MAKE_LDS_APP_BASE_VI();
>>>  	pdd->lds_limit = MAKE_LDS_APP_LIMIT(pdd->lds_base);
>>>  
>>> -	if (!pdd->dev->device_info->needs_iommu_device) {
>>> +	if (!pdd->dev->use_iommu_v2) {
>>>  		/* dGPUs: SVM aperture starting at 0
>>>  		 * with small reserved space for kernel.
>>>  		 * Set them to CANONICAL addresses.
>>> @@ -423,7 +423,7 @@ int kfd_init_apertures(struct kfd_process *process)
>>>  				return -EINVAL;
>>>  			}
>>>  
>>> -			if (!dev->device_info->needs_iommu_device) {
>>> +			if (!dev->use_iommu_v2) {
>>>  				/* dGPUs: the reserved space for kernel
>>>  				 * before SVM
>>>  				 */
>>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_iommu.c b/drivers/gpu/drm/amd/amdkfd/kfd_iommu.c
>>> index 4d3b4188b9a1..0c0269497e51 100644
>>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_iommu.c
>>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_iommu.c
>>> @@ -41,7 +41,7 @@ int kfd_iommu_check_device(struct kfd_dev *kfd)
>>>  	struct amd_iommu_device_info iommu_info;
>>>  	int err;
>>>  
>>> -	if (!kfd->device_info->needs_iommu_device)
>>> +	if (!kfd->use_iommu_v2)
>>>  		return -ENODEV;
>>>  
>>>  	iommu_info.flags = 0;
>>> @@ -63,7 +63,7 @@ int kfd_iommu_device_init(struct kfd_dev *kfd)
>>>  	unsigned int pasid_limit;
>>>  	int err;
>>>  
>>> -	if (!kfd->device_info->needs_iommu_device)
>>> +	if (!kfd->use_iommu_v2)
>>>  		return 0;
>>>  
>>>  	iommu_info.flags = 0;
>>> @@ -109,7 +109,7 @@ int kfd_iommu_bind_process_to_device(struct kfd_process_device *pdd)
>>>  	struct kfd_process *p = pdd->process;
>>>  	int err;
>>>  
>>> -	if (!dev->device_info->needs_iommu_device || pdd->bound == PDD_BOUND)
>>> +	if (!dev->use_iommu_v2 || pdd->bound == PDD_BOUND)
>>>  		return 0;
>>>  
>>>  	if (unlikely(pdd->bound == PDD_BOUND_SUSPENDED)) {
>>> @@ -284,7 +284,7 @@ static void kfd_unbind_processes_from_device(struct kfd_dev *kfd)
>>>   */
>>>  void kfd_iommu_suspend(struct kfd_dev *kfd)
>>>  {
>>> -	if (!kfd->device_info->needs_iommu_device)
>>> +	if (!kfd->use_iommu_v2)
>>>  		return;
>>>  
>>>  	kfd_unbind_processes_from_device(kfd);
>>> @@ -304,7 +304,7 @@ int kfd_iommu_resume(struct kfd_dev *kfd)
>>>  	unsigned int pasid_limit;
>>>  	int err;
>>>  
>>> -	if (!kfd->device_info->needs_iommu_device)
>>> +	if (!kfd->use_iommu_v2)
>>>  		return 0;
>>>  
>>>  	pasid_limit = kfd_get_pasid_limit();
>>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
>>> index 60243798cce2..641b964387ec 100644
>>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
>>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
>>> @@ -308,12 +308,14 @@ struct kfd_dev {
>>>  
>>>  	/* xGMI */
>>>  	uint64_t hive_id;
>>> -    
>>>  	/* UUID */
>>>  	uint64_t unique_id;
>>>  
>>>  	bool pci_atomic_requested;
>>>  
>>> +	/* Use IOMMU v2 flag */
>>> +	bool use_iommu_v2;
>>> +
>>>  	/* SRAM ECC flag */
>>>  	atomic_t sram_ecc_flag;
>>>  
>>> @@ -1009,6 +1011,7 @@ struct kfd_dev *kfd_device_by_pci_dev(const struct pci_dev *pdev);
>>>  struct kfd_dev *kfd_device_by_kgd(const struct kgd_dev *kgd);
>>>  int kfd_topology_enum_kfd_devices(uint8_t idx, struct kfd_dev **kdev);
>>>  int kfd_numa_node_to_apic_id(int numa_node_id);
>>> +void kfd_double_confirm_iommu_support(struct kfd_dev *gpu);
>>>  
>>>  /* Interrupts */
>>>  int kfd_interrupt_init(struct kfd_dev *dev);
>>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_topology.c b/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
>>> index cbb8535abf0c..d141f726d037 100644
>>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
>>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
>>> @@ -545,7 +545,7 @@ static ssize_t node_show(struct kobject *kobj, struct attribute *attr,
>>>  		 * If the ASIC is APU except Kaveri, set local memory size
>>>  		 * to 0 to disable local memory support
>>>  		 */
>>> -		if (!dev->gpu->device_info->needs_iommu_device
>>> +		if (!dev->gpu->use_iommu_v2
>>>  			|| dev->gpu->device_info->asic_family == CHIP_KAVERI) {
>>>  			amdgpu_amdkfd_get_local_mem_info(dev->gpu->kgd,
>>>  				&local_mem_info);
>>> @@ -1197,7 +1197,7 @@ static struct kfd_topology_device *kfd_assign_gpu(struct kfd_dev *gpu)
>>>  		/* Discrete GPUs need their own topology device list
>>>  		 * entries. Don't assign them to CPU/APU nodes.
>>>  		 */
>>> -		if (!gpu->device_info->needs_iommu_device &&
>>> +		if (!gpu->use_iommu_v2 &&
>>>  		    dev->node_props.cpu_cores_count)
>>>  			continue;
>>>  
>>> @@ -1452,7 +1452,7 @@ int kfd_topology_add_device(struct kfd_dev *gpu)
>>>  	* Overwrite ATS capability according to needs_iommu_device to fix
>>>  	* potential missing corresponding bit in CRAT of BIOS.
>>>  	*/
>>> -	if (dev->gpu->device_info->needs_iommu_device)
>>> +	if (dev->gpu->use_iommu_v2)
>>>  		dev->node_props.capability |= HSA_CAP_ATS_PRESENT;
>>>  	else
>>>  		dev->node_props.capability &= ~HSA_CAP_ATS_PRESENT;
>>> @@ -1579,6 +1579,36 @@ int kfd_numa_node_to_apic_id(int numa_node_id)
>>>  	return kfd_cpumask_to_apic_id(cpumask_of_node(numa_node_id));
>>>  }
>>>  
>>> +void kfd_double_confirm_iommu_support(struct kfd_dev *gpu)
>>> +{
>>> +	struct kfd_topology_device *dev;
>>> +
>>> +	gpu->use_iommu_v2 = false;
>>> +
>>> +	down_read(&topology_lock);
>>> +
>>> +	/* Only use IOMMUv2 if there is an APU topology node with no GPU
>>> +	 * assigned yet. This GPU will be assigned to it.
>>> +	 */
>>> +	list_for_each_entry(dev, &topology_device_list, list) {
>>> +		if (dev->node_props.cpu_cores_count &&
>>> +		    dev->node_props.simd_count &&
>>> +		    !dev->gpu) {
>>> +			/* If device_info indicates no iommu support, force
>>> +			 * simd_count as 0 to tell user mode no GPU in the APU
>>> +			 * node.
>>> +			 */
>>> +			if (!gpu->device_info->needs_iommu_device)
>>> +				dev->node_props.simd_count = 0;
>> This assumes that the APU's built-in GPU is the first to be initialized.
>> If there is a dGPU and that gets initialized first, you will set
>> dev->node_props.simd_count = 0 here, which will force the APU to run in
>> dGPU mode.
>>
>> Maybe that's intentional, but it seems weird that this depends on the
>> order in which GPUs are initialized. That order may not be stable
>> between reboots, so relying on that can lead to unexpected behaviour and
>> confusing bug reports.
>>
> This is to fix the case if ACPI CRAT is good on Renoir. If ACPI CRAT good
> by default on Renoir, we will initialize the APU node at that time. If it
> checked needs_iommu_device here, then we can report the simd_count as 0 to
> user mode to treat this node as pure CPU. This is in spired by below
> comments:
>
>>> I'd suggest trying a more general solution in node_show in
>>> kfd_topology.c. If we see an APU node (that has CPU and GPU cores) with
>>> no associated GPU (dev->gpu_id is 0 or dev->gpu is NULL), we can report
>>> it as a pure CPU node to user mode by just reporting the simd_count as 0.

The best place to do that without depending on the order in which GPUs
are initialized is in node_show:

            sysfs_show_32bit_prop(buffer, offs, "simd_count",
                                  dev->gpu ? dev->node_props.simd_count : 0);

Regards,
  Felix


> Thanks,
> Ray
>
>> Regards,
>>   Felix
>>
>>
>>> +			else
>>> +				gpu->use_iommu_v2 = true;
>>> +			break;
>>> +		}
>>> +	}
>>> +
>>> +	up_read(&topology_lock);
>>> +}
>>> +
>>>  #if defined(CONFIG_DEBUG_FS)
>>>  
>>>  int kfd_debugfs_hqds_by_device(struct seq_file *m, void *data)
_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux