Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.32 as permitted sender) client-ip=23.128.96.32;
Message-ID: <0d546c7c-2302-8d51-e37d-da47d9f5a39f@amd.com>
Date:   Wed, 4 Oct 2023 22:47:10 +0530
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.12.0
Subject: Re: [PATCH v3 2/2] drm/amdkfd: get doorbell's absolute offset based
 on the db size
To:     Felix Kuehling <felix.kuehling@amd.com>,
        Arvind Yadav <Arvind.Yadav@amd.com>, Christian.Koenig@amd.com,
        alexander.deucher@amd.com, shashank.sharma@amd.com,
        Mukul.Joshi@amd.com, Xinhui.Pan@amd.com, airlied@gmail.com,
        daniel@ffwll.ch
Cc:     amd-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org,
        linux-kernel@vger.kernel.org
References: <20231004161652.3082-1-Arvind.Yadav@amd.com>
 <20231004161652.3082-3-Arvind.Yadav@amd.com>
 <c0d6ea10-b648-4f44-8ce6-156aefe556f9@amd.com>
Content-Language: en-US
From:   "Yadav, Arvind" <arvyadav@amd.com>
In-Reply-To: <c0d6ea10-b648-4f44-8ce6-156aefe556f9@amd.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?Y3E0emhKb2pNM0J4U2JkUFBzdGYyOHJDQWo3c204ZE9hUCtMd24yb2VWZE43?=
 =?utf-8?B?bVpBQi9nbUNSZUM1TC9Ma2F2bkFxMXpqb1g0TGd4d0ovenJjUFc0dmw3NnhF?=
 =?utf-8?B?TDVLb0NrNS9JVTExZmRsOE8zWWdyZmZvTW5ieG04TGlqSUNXUGlMVDM2REFP?=
 =?utf-8?B?OEdIcHg4bDJabmlQbEhhajBjNmFnTEN0STlEREdrUTBEU0FNQzFjNDA3Mndk?=
 =?utf-8?B?NWsrdVYwSk1rNTVnd1h5LzYzek45Qm1ra0hWSDUwaTgrSFFhME5ObHVONGZE?=
 =?utf-8?B?Z2hWTzgzNW5iQmhoVXFSYXM4ajZ5VmV2Y3VOT0dKSkdMaU5VZllwaWdMZThY?=
 =?utf-8?B?ZzBvU1ZGWjJRRU1tKzdMQTNNZUxoUDE5eWk0UmVkU1Y4b2JBME5CTXJTNGFM?=
 =?utf-8?B?anU2aUhXMG1Bcmd1U3kvVzF3c1c3OWJHZExRRkc1TmtkcHU5cEpFSDBjZTcv?=
 =?utf-8?B?bGQ1ak1sTVB5WnYveFc0T25Xc2VBTThOQTl3bStmOGVaN3lRYjhENXptRnU3?=
 =?utf-8?B?UUdYdHM5Y2xTT0hQYTd1SFRPVkEwYXV3dUlCTVNCRjg3ekNHY2FON1VnTVNN?=
 =?utf-8?B?TVNRdkp0S0NFWFZnOUJGeXRQU0k5SDVETXc3NHNpU2QzQ0swcVVQMnpHVzJX?=
 =?utf-8?B?S00xZnMvZmgyZVpCdTN0WkpvaUx5bnBaVDcwTDBFUFUvQy9kRWg0ZUpOejB6?=
 =?utf-8?B?U1hlR1JnMkxRL1ZqS1Rtd1hDMXpwQXZMOVZxOS9KSTIweTVHN3hRY1dJWVdT?=
 =?utf-8?B?UzRJMzY5QnZTL2tIdTk4ckVvS2V5dUhFTitJQ2UrcWVMOG5VSlFhaVhNbVcy?=
 =?utf-8?B?NngzbkZWMnhlZDZGQng5NFgyRk12ZGhwdjFYYVNwOGlSbHdFMjBZSDNxaVZH?=
 =?utf-8?B?c1h5d1doQ0IxRWZNeUM3Z0Q5Ui9WWkJkZmhNWHozME91aDk1NzhzanVnTFBB?=
 =?utf-8?B?eU9NOWIxWHdaNHhMN0xiTnFkbTE2d25MU2NBY2VsdlpxdHd6a2RLZ0JBQUJK?=
 =?utf-8?B?dXlUblJJN2FMNitmTkhuQktHbThzeGxGVFlyeGpiSVk3czNFNldsTWpkQ1FK?=
 =?utf-8?B?bzcxYnBnRUh5Qjg5K1BVQ2M2TStURDh0VE9UdTBHZVVvUmx6aFR6ZEFucmpp?=
 =?utf-8?B?akhoUjVrbFAzamNtZmJNekhVaDFaeW1rUnJRWlR1RnFVOVlGT05HTEZaM25U?=
 =?utf-8?B?RTIxTEVlMlAvNW9lRVFnSFE4ZkVNZDl6UklhMFNrVU00MzJUUkJRTVA5UDNy?=
 =?utf-8?B?b0J0eWU1b3k5RDhuOUlSVnk5bGF3dlZySjAzQ0hYQURXaGVHcUora2dnbzU5?=
 =?utf-8?B?RXdsR3pQanBld2ovdVlDUWtidjVmVE90eDA2N3RsNHBZem8zUysyTXhlUWtH?=
 =?utf-8?B?ZHZtK1VLVnZYaVFpSENBYXBHSU1XTlhkU2F4UWJJeVBVYVhrNFVJZmw4SXhM?=
 =?utf-8?B?TU9ZQnBCbWJ1cktkRCs0NHVZN3RtMVU4SjBvOVVzZ0NGODM1NmE2Rm5mQkVT?=
 =?utf-8?B?R2tyMnM5My9OSTBUUG1BSjl3YzU4UmhJVnFSQ2lXNGVtVElyT05ZNnQzWlkx?=
 =?utf-8?B?a3NDa1Q3ZWVDa09NN2VoSGplS2FxL2pwR0s1OE5tSVljSWVxMExnUU8rNVp0?=
 =?utf-8?B?ZXJPa0ZEamJSU2xYeFd2UFAraVZoeU1JRXBjRVlXbWlkczR5dW9MZ0I4WWRO?=
 =?utf-8?B?bUtGeDFkRk1aeWx5UlpKeFpzNnh4T08vcDVuSjZJeUNIRm9yUFVsb2ZBODdH?=
 =?utf-8?B?b2ZmQjRqdVN6SlRtUkt6NnhhQklIdEx3d0Z2T01UTFdwb3FkaEdtcGlXNUFJ?=
 =?utf-8?B?Y2pLZ2UrT0JNVGlkclhTZ2tVVDV6am9PM2xUaXNNQ2RVa2w0Nnp2c0lYR3pu?=
 =?utf-8?B?UjJIRHgxcU12TWZDQ280T1pWc3FHSENlZ2ErS01hME53VktVbE1KMU51a3Qz?=
 =?utf-8?B?YVJhRDNPcjJRM2ZWb0txZmZtenpSWWoyc1VNZFFLbG5lUGNtLzhhdjlXWmRM?=
 =?utf-8?B?K01aZmE3aG5EcmxVT3hoS01VM0cyekFJQmhVVmdKODZXRnBBSDR3ZUVRdjZS?=
 =?utf-8?B?ckpialJkY3dFalFIT2doYTNmV0Y0WmNPSGdxU2F2RHBhZlArS3grUFRnWGZ3?=
 =?utf-8?Q?fybOPxh8n+tcqWam2641jnphj?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 15f8f0ff-6354-482b-8871-08dbc4fdc5a9
X-MS-Exchange-CrossTenant-AuthSource: PH7PR12MB6000.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 04 Oct 2023 17:17:22.8636
 (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: of09ZgVkw6+1WQgAm++mebaljxYMMfQE483+i92/XYKXirVfjXXEXsssxJIvCZWkMf8GJIh5A6T0bDukewW7RQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH0PR12MB7079
X-Spam-Status: No, score=-2.4 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED,
	DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED
	autolearn=unavailable autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on agentk.vger.email
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (agentk.vger.email [0.0.0.0]); Wed, 04 Oct 2023 10:18:01 -0700 (PDT)


On 10/4/2023 10:29 PM, Felix Kuehling wrote:
>
> On 2023-10-04 12:16, Arvind Yadav wrote:
>> This patch is to align the absolute doorbell offset
>> based on the doorbell's size. So that doorbell offset
>> will be aligned for both 32 bit and 64 bit.
>>
>> v2:
>> - Addressed the review comment from Felix.
>> v3:
>> - Adding doorbell_size as parameter to get db absolute offset.
>>
>> Cc: Christian Koenig <christian.koenig@amd.com>
>> Cc: Alex Deucher <alexander.deucher@amd.com>
>> Signed-off-by: Shashank Sharma <shashank.sharma@amd.com>
>> Signed-off-by: Arvind Yadav <Arvind.Yadav@amd.com>
>
> The final result looks good to me. But please squash the two patches 
> into one. The first patch on its own breaks the build, and that's 
> something we don't want to commit to the branch history as it makes 
> tracking regressions (e.g. with git bisect) very hard or impossible.
>
> More nit-picks inline.
Sure, we can have one patch.
>
>
>> ---
>>   .../gpu/drm/amd/amdkfd/kfd_device_queue_manager.c   |  6 +++++-
>>   drivers/gpu/drm/amd/amdkfd/kfd_doorbell.c           | 13 +++++++++++--
>>   .../gpu/drm/amd/amdkfd/kfd_process_queue_manager.c  |  4 +++-
>>   3 files changed, 19 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c 
>> b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
>> index 0d3d538b64eb..690ff131fe4b 100644
>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
>> @@ -346,6 +346,7 @@ static int allocate_doorbell(struct 
>> qcm_process_device *qpd,
>>                    uint32_t const *restore_id)
>>   {
>>       struct kfd_node *dev = qpd->dqm->dev;
>> +    uint32_t doorbell_size;
>>         if (!KFD_IS_SOC15(dev)) {
>>           /* On pre-SOC15 chips we need to use the queue ID to
>> @@ -405,9 +406,12 @@ static int allocate_doorbell(struct 
>> qcm_process_device *qpd,
>>           }
>>       }
>>   +    doorbell_size = dev->kfd->device_info.doorbell_size;
>> +
>>       q->properties.doorbell_off = 
>> amdgpu_doorbell_index_on_bar(dev->adev,
>>                                     qpd->proc_doorbells,
>> -                                  q->doorbell_id);
>> +                                  q->doorbell_id,
>> +                                  doorbell_size);
>
> You don't need a local variable for doorbell size that's only used 
> once. Just pass dev->kfd->device_info.doorbell_size directly.
>
I have used local variable to make the code cleaner but I will remove 
local variable.
>
>>       return 0;
>>   }
>>   diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_doorbell.c 
>> b/drivers/gpu/drm/amd/amdkfd/kfd_doorbell.c
>> index 7b38537c7c99..59dd76c4b138 100644
>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_doorbell.c
>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_doorbell.c
>> @@ -161,7 +161,10 @@ void __iomem *kfd_get_kernel_doorbell(struct 
>> kfd_dev *kfd,
>>       if (inx >= KFD_MAX_NUM_OF_QUEUES_PER_PROCESS)
>>           return NULL;
>>   -    *doorbell_off = amdgpu_doorbell_index_on_bar(kfd->adev, 
>> kfd->doorbells, inx);
>> +    *doorbell_off = amdgpu_doorbell_index_on_bar(kfd->adev,
>> +                             kfd->doorbells,
>> +                             inx,
>> + kfd->device_info.doorbell_size);
>>       inx *= 2;
>>         pr_debug("Get kernel queue doorbell\n"
>> @@ -233,6 +236,7 @@ phys_addr_t kfd_get_process_doorbells(struct 
>> kfd_process_device *pdd)
>>   {
>>       struct amdgpu_device *adev = pdd->dev->adev;
>>       uint32_t first_db_index;
>> +    uint32_t doorbell_size;
>>         if (!pdd->qpd.proc_doorbells) {
>>           if (kfd_alloc_process_doorbells(pdd->dev->kfd, pdd))
>> @@ -240,7 +244,12 @@ phys_addr_t kfd_get_process_doorbells(struct 
>> kfd_process_device *pdd)
>>               return 0;
>>       }
>>   -    first_db_index = amdgpu_doorbell_index_on_bar(adev, 
>> pdd->qpd.proc_doorbells, 0);
>> +    doorbell_size = pdd->dev->kfd->device_info.doorbell_size;
>> +
>> +    first_db_index = amdgpu_doorbell_index_on_bar(adev,
>> +                              pdd->qpd.proc_doorbells,
>> +                              0,
>> +                              doorbell_size);
>
> Same as above, no local variable needed.
Noted,
>
>
>>       return adev->doorbell.base + first_db_index * sizeof(uint32_t);
>>   }
>>   diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c 
>> b/drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c
>> index adb5e4bdc0b2..010cd8e8e6a1 100644
>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c
>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c
>> @@ -375,9 +375,11 @@ int pqm_create_queue(struct 
>> process_queue_manager *pqm,
>>            * relative doorbell index = Absolute doorbell index -
>>            * absolute index of first doorbell in the page.
>>            */
>> +        uint32_t doorbell_size = 
>> pdd->dev->kfd->device_info.doorbell_size;
>>           uint32_t first_db_index = 
>> amdgpu_doorbell_index_on_bar(pdd->dev->adev,
>> pdd->qpd.proc_doorbells,
>> -                                       0);
>> +                                       0,
>> +                                       doorbell_size);
>
> No local variable needed.
>
Noted,

Thanks
~Arvind

> Regards,
>   Felix
>
>
>>             *p_doorbell_offset_in_process = (q->properties.doorbell_off
>>                           - first_db_index) * sizeof(uint32_t);