Received-SPF: pass (google.com: domain of linux-kernel+bounces-19113-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <c82fbab0-1ba1-43cf-9f31-57d365b2baa2@amd.com>
Date: Mon, 8 Jan 2024 07:45:09 +0100
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 1/1] Refactor radeon driver to use
 drm_gem_create_map_offset() instead of its custom implementation for
 associating GEM object with a fake offset. Since, we already have a generic
 implementation, we don't need the custom function and it is better to
 standardize the code.
Content-Language: en-US
To: Dipam Turkar <dipamt1729@gmail.com>, alexander.deucher@amd.com
Cc: Xinhui.Pan@amd.com, airlied@gmail.com, daniel@ffwll.ch,
 amd-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org,
 linux-kernel@vger.kernel.org
References: <20240106141422.10734-1-dipamt1729@gmail.com>
From: =?UTF-8?Q?Christian_K=C3=B6nig?= <christian.koenig@amd.com>
In-Reply-To: <20240106141422.10734-1-dipamt1729@gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?utf-8?B?RXIvMm9nS05CdjRZNW9XRFRYR2crOVphaTl4Q3ZWbWdmNkJySVdFWEhTQ2J1?=
 =?utf-8?B?WlpEY1ljbkU2UmdJMDhPc1c0VkZCTFB2clhKRXlETklpZ1YzZEpQSGFvd1Jm?=
 =?utf-8?B?eld3TXpFRWVyb1dvSDhqQXBRUkM2cjFLbzYyMDVpTStYQTQ3VmV1enJPcDlo?=
 =?utf-8?B?YTVwUGZQQjdWaFVVWitGYzJsdkNNdE1oZGVnWFJnV3pSREpCYnFYYis4dzZi?=
 =?utf-8?B?SXYxU0VDd1BVTlpLTGd4Z1dxcEd6UjJNalVmUEFxLzNia0Zaa1MyTncvTzhz?=
 =?utf-8?B?OEpmNzcwM041RFo0cDN6OFA4WklDeXNTcCtRWWpwdm9VNHlhVW9TQXNRYzJu?=
 =?utf-8?B?YzlEVzJIYkkwcU1LYmh1UnY5YzM1cHdiUlZGa1pPU0s0d3VmQlpKejYwSGVh?=
 =?utf-8?B?b21Mb0dTcDZDakVPOElkSWF5MmNsSnIzVzNNdkU2RVM3UEFlTHBWcTZwcVJQ?=
 =?utf-8?B?bm9DeTBuWHVSdnNDaWpVSDR4a0NLVGhCOS9lc1BlUUg0M011MG9FaEdlNnFO?=
 =?utf-8?B?RFZ0ZUlBZEFQYWRPWEo4ZHBtZEE1WTBuME5USmhXQmFDS2tZa29aektGaW5u?=
 =?utf-8?B?MkdXUVVFWXdqTzlQYkdhOVRPOEN3WXlnTEtRMEozUTVTaDFaTkFWM3MvK1V0?=
 =?utf-8?B?R3o1ZlNiajgvcDlMK28rL1FzcGllWHNuQ2pFZW5pelhtTENBY1RSamhxTzJi?=
 =?utf-8?B?SE50eHo0TXYyWVdXcUtoa0w3NmNrTWxuSUFiSTZRS3Q4TWt0Y2JMdTUvclpw?=
 =?utf-8?B?emJkb29Fa1NNZGhzZmxBL0U5eUd6SCtlU2YweVlNZk0yVW0rMllvZ2hYL1Y0?=
 =?utf-8?B?WkhMYXVNSzZwNmdHNEdWMlliYVEvUnI5VEQwelVnZW9xaGx2cHozZG5QZENk?=
 =?utf-8?B?eTBWL0RXbTB1THd6QnlseWM1R1ZMNkdSWlRnMFl6SlU0RExhU1V5bGhMRzAr?=
 =?utf-8?B?NDBtcUI5cTRYaEJwNzNTSmU4Zko4cE9ZQXZCNnF0Um9BVWFXU3UwZm1FZ1hH?=
 =?utf-8?B?dUx6QmxJZFhKT1V4QUJBczcxckpid21od1ExVVYxNnlEWlNyK1UxTDhtekUw?=
 =?utf-8?B?Z1dTYVl6M0E1di9paDM5S2x5TDY4MHF5YlZ0UEFiSENFblg2Uk5jMHowb0JZ?=
 =?utf-8?B?alJzMkkvcDg0amoreEMxWjNVcjhYb053UElFLzFMTUllc3daSlQyVnN6ZUcy?=
 =?utf-8?B?cWtNRDlmR3czcDY1TkZpUktEcmhJZi9UN09tZUNVTVhDVWlod2NFclQvOThr?=
 =?utf-8?B?a0x2ZnlpSEt2TStlZjZQNGEway9Vc0VhcHBSM2RON0NYSG0zVnFHbnpxYkRz?=
 =?utf-8?B?c21uTk1Ob2lVUVo0YUtCYU1wVkl6dGFQQUhSY0VJajUzenIwajN5N1JQTklJ?=
 =?utf-8?B?RGZXRmtvUnBEOU1DQm8ya2pmNisvclJCRXUvNlRvaGM2dFE4S1BmL2NHMzds?=
 =?utf-8?B?QmZ5YVYvMkU2K3ZwZWF6UlptaTFCNnpvaER4WWN4d1F6Q00zOER4YUpoU25T?=
 =?utf-8?B?cFA0b0U1YlpiS3BDYzZXUGYwQXdGWVg4d3cycU9tZlBGTEViUHVPWTQxZTdP?=
 =?utf-8?B?dWdLSEdCR1FodjRmcS9vSG5xcXV0NkhiNEJCd0Ria2JxLzUyeU1qWWoxNmY2?=
 =?utf-8?B?UlN2NEErSTByVEJVM2lGQWdlTGxHRlM4d3FheHYrYzF4d2pvT2Fuc0JlcnN4?=
 =?utf-8?B?Vmw4YndrblVON1UrMTZpQlRxSEJzUi9DOVdvbTNaczNqT1cyZHRybk1hVGhr?=
 =?utf-8?B?RGQ1OWhnK3UzbnV4azZWTEN2U2dxbElFL1FRK2ozcmkvU05nUDNKNUlnV2Q2?=
 =?utf-8?B?dVpMdXd5TVJMNTJ2aXBrSVgvU2ZZTThOUXU1Ui9CRXlWL0t2dTRPK2xUejBD?=
 =?utf-8?B?OVN0Y3ppdFkvbmxHSWsxZVpzUityTjNyTnFVUzB3UG90bjczTWQrOXdTbThw?=
 =?utf-8?B?dUxuakN1R3dZeVdCNWE5SE9oMStySnBPbW9IdW1wTmZwWjB3VWRlbHV3em9V?=
 =?utf-8?B?QmJGNHg5NllxM3ZxTjJwRGQrTmg0WUwwUWxyQWI2c3krSE8rVmRPeDlOYmpx?=
 =?utf-8?B?VTN4VEtJTWJNMkxHVFJGTFdZK3RDRlo5TzVGMTk1a1hwUEV4dnNjYmx0a3Rz?=
 =?utf-8?Q?MpmwwN4ZMzs/kYUSpUYjx0/bw?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: ad30f7b0-5128-44ab-ebeb-08dc10155d94
X-MS-Exchange-CrossTenant-AuthSource: PH7PR12MB5685.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 08 Jan 2024 06:45:13.3078
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: +jImhtLqLufzlmYIIlsXBo7QgJo+dk1FPVaGIyUkUQerKjDyFL8a/gyNC1OAHIui
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MN2PR12MB4389

Am 06.01.24 um 15:14 schrieb Dipam Turkar:
> Signed-off-by: Dipam Turkar <dipamt1729@gmail.com>
> ---
>   drivers/gpu/drm/radeon/radeon_drv.c |  2 +-
>   drivers/gpu/drm/radeon/radeon_gem.c | 24 ++----------------------
>   2 files changed, 3 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/gpu/drm/radeon/radeon_drv.c b/drivers/gpu/drm/radeon/radeon_drv.c
> index fa531493b111..f590ed65ffba 100644
> --- a/drivers/gpu/drm/radeon/radeon_drv.c
> +++ b/drivers/gpu/drm/radeon/radeon_drv.c
> @@ -578,7 +578,7 @@ static const struct drm_driver kms_driver = {
>   	.ioctls = radeon_ioctls_kms,
>   	.num_ioctls = ARRAY_SIZE(radeon_ioctls_kms),
>   	.dumb_create = radeon_mode_dumb_create,
> -	.dumb_map_offset = radeon_mode_dumb_mmap,
> +	.dumb_map_offset = drm_gem_dumb_map_offset,
>   	.fops = &radeon_driver_kms_fops,
>   
>   	.gem_prime_import_sg_table = radeon_gem_prime_import_sg_table,
> diff --git a/drivers/gpu/drm/radeon/radeon_gem.c b/drivers/gpu/drm/radeon/radeon_gem.c
> index 358d19242f4b..99794c550d2c 100644
> --- a/drivers/gpu/drm/radeon/radeon_gem.c
> +++ b/drivers/gpu/drm/radeon/radeon_gem.c
> @@ -31,6 +31,7 @@
>   
>   #include <drm/drm_device.h>
>   #include <drm/drm_file.h>
> +#include <drm/dem_gem.h>
>   #include <drm/drm_gem_ttm_helper.h>
>   #include <drm/radeon_drm.h>
>   
> @@ -480,33 +481,12 @@ int radeon_gem_set_domain_ioctl(struct drm_device *dev, void *data,
>   	return r;
>   }
>   
> -int radeon_mode_dumb_mmap(struct drm_file *filp,
> -			  struct drm_device *dev,
> -			  uint32_t handle, uint64_t *offset_p)
> -{
> -	struct drm_gem_object *gobj;
> -	struct radeon_bo *robj;
> -
> -	gobj = drm_gem_object_lookup(filp, handle);
> -	if (gobj == NULL) {
> -		return -ENOENT;
> -	}
> -	robj = gem_to_radeon_bo(gobj);
> -	if (radeon_ttm_tt_has_userptr(robj->rdev, robj->tbo.ttm)) {

Well apart from the obvious typos, the missing commit message and the 
mangled subject line this also removes this important check for userptrs 
here.

This in turn is a complete no-go since it can lead to kernel crashes.

Regards,
Christian.

> -		drm_gem_object_put(gobj);
> -		return -EPERM;
> -	}
> -	*offset_p = radeon_bo_mmap_offset(robj);
> -	drm_gem_object_put(gobj);
> -	return 0;
> -}
> -
>   int radeon_gem_mmap_ioctl(struct drm_device *dev, void *data,
>   			  struct drm_file *filp)
>   {
>   	struct drm_radeon_gem_mmap *args = data;
>   
> -	return radeon_mode_dumb_mmap(filp, dev, args->handle, &args->addr_ptr);
> +	return drm_gem_dumb_map_offset(filp, dev, args->handle, &args->addr_ptr);
>   }
>   
>   int radeon_gem_busy_ioctl(struct drm_device *dev, void *data,