[v3 2/6] drm/amdxcp: introduce new API amdgpu_xcp_drm_dev_free()

[Gerry Liu]

> 2025年1月8日 17:31，Lazar, Lijo <lijo.lazar at amd.com> 写道：
> 
> 
> 
> On 1/8/2025 2:26 PM, Jiang Liu wrote:
>> Introduce new interface amdgpu_xcp_drm_dev_free() to free a specific
>> drm_device crreated by amdgpu_xcp_drm_dev_alloc(), which will be used
>> to do error recovery.
>> 
>> Signed-off-by: Jiang Liu <gerry at linux.alibaba.com>
>> ---
>> drivers/gpu/drm/amd/amdxcp/amdgpu_xcp_drv.c | 76 +++++++++++++++++----
>> drivers/gpu/drm/amd/amdxcp/amdgpu_xcp_drv.h |  1 +
>> 2 files changed, 63 insertions(+), 14 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/amd/amdxcp/amdgpu_xcp_drv.c b/drivers/gpu/drm/amd/amdxcp/amdgpu_xcp_drv.c
>> index faed84172dd4..fc92b5fe1040 100644
>> --- a/drivers/gpu/drm/amd/amdxcp/amdgpu_xcp_drv.c
>> +++ b/drivers/gpu/drm/amd/amdxcp/amdgpu_xcp_drv.c
>> @@ -45,21 +45,32 @@ static const struct drm_driver amdgpu_xcp_driver = {
>> 
>> static int8_t pdev_num;
>> static struct xcp_device *xcp_dev[MAX_XCP_PLATFORM_DEVICE];
>> +static DEFINE_MUTEX(xcp_mutex);
>> 
>> int amdgpu_xcp_drm_dev_alloc(struct drm_device **ddev)
>> {
>> 	struct platform_device *pdev;
>> 	struct xcp_device *pxcp_dev;
>> 	char dev_name[20];
>> -	int ret;
>> +	int ret, index;
>> 
>> -	if (pdev_num >= MAX_XCP_PLATFORM_DEVICE)
>> -		return -ENODEV;
>> +	mutex_lock(&xcp_mutex);
>> +	if (pdev_num >= MAX_XCP_PLATFORM_DEVICE) {
>> +		ret = -ENODEV;
>> +		goto out_unlock;
>> +	}
>> +
>> +	for (index = 0; index < MAX_XCP_PLATFORM_DEVICE; index++) {
>> +		if (!xcp_dev[index])
>> +			break;
>> +	}
>> 
>> -	snprintf(dev_name, sizeof(dev_name), "amdgpu_xcp_%d", pdev_num);
>> +	snprintf(dev_name, sizeof(dev_name), "amdgpu_xcp_%d", index);
>> 	pdev = platform_device_register_simple(dev_name, -1, NULL, 0);
>> -	if (IS_ERR(pdev))
>> -		return PTR_ERR(pdev);
>> +	if (IS_ERR(pdev)) {
>> +		ret = PTR_ERR(pdev);
>> +		goto out_unregister;
>> +	}
>> 
>> 	if (!devres_open_group(&pdev->dev, NULL, GFP_KERNEL)) {
>> 		ret = -ENOMEM;
>> @@ -72,10 +83,11 @@ int amdgpu_xcp_drm_dev_alloc(struct drm_device **ddev)
>> 		goto out_devres;
>> 	}
>> 
>> -	xcp_dev[pdev_num] = pxcp_dev;
>> -	xcp_dev[pdev_num]->pdev = pdev;
>> +	xcp_dev[index] = pxcp_dev;
>> +	xcp_dev[index]->pdev = pdev;
>> 	*ddev = &pxcp_dev->drm;
>> 	pdev_num++;
>> +	mutex_unlock(&xcp_mutex);
>> 
>> 	return 0;
>> 
>> @@ -83,21 +95,57 @@ int amdgpu_xcp_drm_dev_alloc(struct drm_device **ddev)
>> 	devres_release_group(&pdev->dev, NULL);
>> out_unregister:
>> 	platform_device_unregister(pdev);
>> +out_unlock:
>> +	mutex_unlock(&xcp_mutex);
>> 
>> 	return ret;
>> }
>> EXPORT_SYMBOL(amdgpu_xcp_drm_dev_alloc);
>> 
>> +static void amdgpu_xcp_drm_dev_destroy(int index)
> 
> <Nit> Use something like __amdgpu_xcp_drm_dev_free(int index) to keep
> the 'free' suffix.
> 
>> +{
>> +	struct platform_device *pdev;
>> +
>> +	pdev = xcp_dev[index]->pdev;
>> +	devres_release_group(&pdev->dev, NULL);
>> +	platform_device_unregister(pdev);
>> +	xcp_dev[index] = NULL;
>> +	pdev_num--;
>> +}
>> +
>> +void amdgpu_xcp_drm_dev_free(struct drm_device *ddev)
>> +{
>> +	int index;
>> +	struct xcp_device *pxcp_dev;
>> +
>> +	if (ddev == NULL)
> 
> Does it make sense to add !pdev_num check or a WARN_ON(!pdev_num) below?
> 
>> +		return;
>> +
>> +	pxcp_dev = container_of(ddev, struct xcp_device, drm);
>> +
>> +	mutex_lock(&xcp_mutex);
>> +	for (index = 0; index < MAX_XCP_PLATFORM_DEVICE; index++) {
>> +		if (xcp_dev[index] == pxcp_dev) {
>> +			amdgpu_xcp_drm_dev_destroy(index);
>> +			break;
>> +		}
>> +	}
>> +	mutex_unlock(&xcp_mutex);
>> +}
>> +EXPORT_SYMBOL(amdgpu_xcp_drm_dev_free);
>> +
>> void amdgpu_xcp_drv_release(void)
>> {
>> -	for (--pdev_num; pdev_num >= 0; --pdev_num) {
>> -		struct platform_device *pdev = xcp_dev[pdev_num]->pdev;
>> +	int index;
>> 
>> -		devres_release_group(&pdev->dev, NULL);
>> -		platform_device_unregister(pdev);
>> -		xcp_dev[pdev_num] = NULL;
> 
> To better optimize, add one check like below.
> 	if (!pdev_num)
> 		return;
Thanks for review and suggestions, all comments will be addressed in next version.

> 
> Thanks,
> Lijo
> 
>> +	mutex_lock(&xcp_mutex);
>> +	for (index = 0; index < MAX_XCP_PLATFORM_DEVICE; index++) {
>> +		if (xcp_dev[index]) {
>> +			amdgpu_xcp_drm_dev_destroy(index);
>> +		}
>> 	}
>> -	pdev_num = 0;
>> +	WARN_ON(pdev_num != 0);
>> +	mutex_unlock(&xcp_mutex);
>> }
>> EXPORT_SYMBOL(amdgpu_xcp_drv_release);
>> 
>> diff --git a/drivers/gpu/drm/amd/amdxcp/amdgpu_xcp_drv.h b/drivers/gpu/drm/amd/amdxcp/amdgpu_xcp_drv.h
>> index c1c4b679bf95..580a1602c8e3 100644
>> --- a/drivers/gpu/drm/amd/amdxcp/amdgpu_xcp_drv.h
>> +++ b/drivers/gpu/drm/amd/amdxcp/amdgpu_xcp_drv.h
>> @@ -25,5 +25,6 @@
>> #define _AMDGPU_XCP_DRV_H_
>> 
>> int amdgpu_xcp_drm_dev_alloc(struct drm_device **ddev);
>> +void amdgpu_xcp_drm_dev_free(struct drm_device *ddev);
>> void amdgpu_xcp_drv_release(void);
>> #endif /* _AMDGPU_XCP_DRV_H_ */

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/amd-gfx/attachments/20250109/631e088b/attachment-0001.htm>