Re: [PATCH v2 2/3] drm/amdkfd: abstract the iommu device checking with ignore_crat

[Huang Rui <ray.huang@amd.com>]

On Tue, Aug 18, 2020 at 11:11:48PM +0800, Kuehling, Felix wrote:
> I'd recommend making this the first change in the series. Make
> 'drm/amdkfd: force raven as "dgpu" path' the second patch. That way it
> only needs to change one place.
> 

Yes, right.

> A few more comments inline.
> 
> 
> Am 2020-08-18 um 9:09 a.m. schrieb Huang Rui:
> > It's better to use inline function to wrap the iommu checking.
> >
> > Signed-off-by: Huang Rui <ray.huang@amd.com>
> > ---
> >  drivers/gpu/drm/amd/amdkfd/kfd_chardev.c               |  2 +-
> >  drivers/gpu/drm/amd/amdkfd/kfd_dbgdev.c                |  4 ++--
> >  .../gpu/drm/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                  |  8 ++++++++
> >  drivers/gpu/drm/amd/amdkfd/kfd_topology.c              |  6 +++---
> >  7 files changed, 22 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> > index 20ef754dc62e..0598a1682854 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 && !ignore_crat)
> > +	if (kfd_go_iommu_v2(dev->device_info))
> >  		return false;
> >  
> >  	amdgpu_amdkfd_get_local_mem_info(dev->kgd, &mem_info);
> > diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_dbgdev.c b/drivers/gpu/drm/amd/amdkfd/kfd_dbgdev.c
> > index 0c4161ac4102..04103273f52b 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 && !ignore_crat);
> > +				kfd_go_iommu_v2(dbgdev->dev->device_info));
> >  
> >  		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 && !ignore_crat);
> > +				kfd_go_iommu_v2(dbgdev->dev->device_info));
> >  
> >  		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_queue_manager_v9.c b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager_v9.c
> > index 48b86bdbb680..70cede08e555 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 && !ignore_crat))
> > +		    !(kfd_go_iommu_v2(dqm->dev->device_info)))
> >  			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 6044d36bceb6..fe7e31014289 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 && !ignore_crat)) {
> > +	if (!kfd_go_iommu_v2(pdd->dev->device_info)) {
> >  		/* 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 && !ignore_crat)) {
> > +			if (!kfd_go_iommu_v2(dev->device_info)) {
> >  				/* 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 c767b524bea4..ea05446572e1 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 && !ignore_crat))
> > +	if (!kfd_go_iommu_v2(kfd->device_info))
> >  		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 && !ignore_crat))
> > +	if (!kfd_go_iommu_v2(kfd->device_info))
> >  		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 && !ignore_crat) || pdd->bound == PDD_BOUND)
> > +	if (!kfd_go_iommu_v2(dev->device_info) || 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 && !ignore_crat))
> > +	if (!kfd_go_iommu_v2(kfd->device_info))
> >  		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 && !ignore_crat))
> > +	if (!kfd_go_iommu_v2(kfd->device_info))
> >  		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..5087b23ccdb2 100644
> > --- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> > @@ -1232,6 +1232,14 @@ static inline int kfd_devcgroup_check_permission(struct kfd_dev *kfd)
> >  #endif
> >  }
> >  
> > +static inline bool kfd_go_iommu_v2(const struct kfd_device_info *info)
> 
> I'd prefer a different function name: kfd_device_use_iommu_v2.
> 
> Then make the parameter const struct kfd_device *dev.
> 
> > +{
> > +	if (!info)
> > +		return false;
> > +
> > +	return info->needs_iommu_device && !ignore_crat;
> > +}
> 
> If you need a NULL check, you could still write it in a more compact way
> (I like inline functions to be one-liners):
> 
>     return !ignore_crat && dev && dev->device_info->needs_iommu_device;
> 
> If you make this the first patch in the series, remove the !ignore_crat
> part. That would be added in the second change.

That's fine. It looks better, thanks. I will update it in V2.


Thanks,
Ray

> 
> Regards,
>   Felix
> 
> 
> > +
> >  /* Debugfs */
> >  #if defined(CONFIG_DEBUG_FS)
> >  
> > diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_topology.c b/drivers/gpu/drm/amd/amdkfd/kfd_topology.c
> > index ad8139594b6f..e6f03867e0ed 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 && !ignore_crat)
> > +		if (!kfd_go_iommu_v2(dev->gpu->device_info)
> >  			|| dev->gpu->device_info->asic_family == CHIP_KAVERI) {
> >  			amdgpu_amdkfd_get_local_mem_info(dev->gpu->kgd,
> >  				&local_mem_info);
> > @@ -1198,7 +1198,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 && !ignore_crat) &&
> > +		if (!kfd_go_iommu_v2(gpu->device_info) &&
> >  		    dev->node_props.cpu_cores_count)
> >  			continue;
> >  
> > @@ -1453,7 +1453,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 && !ignore_crat)
> > +	if (kfd_go_iommu_v2(dev->gpu->device_info))
> >  		dev->node_props.capability |= HSA_CAP_ATS_PRESENT;
> >  	else
> >  		dev->node_props.capability &= ~HSA_CAP_ATS_PRESENT;
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx