Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.35 as permitted sender) client-ip=23.128.96.35;
Message-ID: <e33eb526-c0b7-c404-72d2-390ce7fb0108@amd.com>
Date:   Fri, 29 Sep 2023 10:39:28 +0200
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.6.1
Subject: Re: [PATCH v2 1/1] drm/amdkfd: Fix unaligned doorbell absolute offset
 for gfx8
Content-Language: en-US
To:     Felix Kuehling <felix.kuehling@amd.com>,
        "Joshi, Mukul" <Mukul.Joshi@amd.com>,
        "Yadav, Arvind" <Arvind.Yadav@amd.com>,
        "Koenig, Christian" <Christian.Koenig@amd.com>,
        "Deucher, Alexander" <Alexander.Deucher@amd.com>,
        "Pan, Xinhui" <Xinhui.Pan@amd.com>,
        "airlied@gmail.com" <airlied@gmail.com>,
        "daniel@ffwll.ch" <daniel@ffwll.ch>
Cc:     "dri-devel@lists.freedesktop.org" <dri-devel@lists.freedesktop.org>,
        "amd-gfx@lists.freedesktop.org" <amd-gfx@lists.freedesktop.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
References: <20230928095427.4337-1-Arvind.Yadav@amd.com>
 <20230928095427.4337-2-Arvind.Yadav@amd.com>
 <DM4PR12MB6445548E05C8E3B29AAEAFCAEEC1A@DM4PR12MB6445.namprd12.prod.outlook.com>
 <8cd0539d-5d5e-e670-9577-7db72393b784@amd.com>
 <8c9c3b5b-ba1e-ba07-3001-242eab9ca1a6@amd.com>
 <2f56333d-4549-3118-5cd0-3ad2be025b66@amd.com>
From:   Shashank Sharma <shashank.sharma@amd.com>
In-Reply-To: <2f56333d-4549-3118-5cd0-3ad2be025b66@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?MkM4aFkya050UzkxQk1Xa2FMTXZVN0F5SHViZThEdm12L2RGTmNkNlVHcTgw?=
 =?utf-8?B?VUVEWDlzMkwxVmFoVzdhamtnVlZET0lNeWF3M2F4UDZZMG1wZDdHU2EweXh0?=
 =?utf-8?B?V243SUVqNGFMWk5Fb1IrSmtwQVpRSTVGbXd1UUxsUVZMS2EweGNBSTV5VU5a?=
 =?utf-8?B?TVh1RkVEdXVJUUQrR05LWE9id1dqaG5qY1pIRks5Nk1qTGM0TFdhcHorOEJW?=
 =?utf-8?B?Z1l6VjhldGlaYmtmUjJoTVVoNjh3VnlwNFlSb3hkNEZQV01ZY1pNWjBPRmpu?=
 =?utf-8?B?NXlCWUZ6aXlNS3lOUXY0VXVWZnMrR1prWjlWUHhxQy9MYzNpeHhiYTlWUjlO?=
 =?utf-8?B?clA1dnNGOGhyTFQxcnpXQzFMb0N1NEF1R2hGZCtzeWF2UGdtL2FlRisyQVMx?=
 =?utf-8?B?dGxXVDYwQmtPYUZoNU9hNE1KYUs2enFITzc4KzJYYVZwaDJwYXcvbnZUNU40?=
 =?utf-8?B?d1F5L0dsUGsybEhsMkxVQSt4QVAxVUp0MzNNZmRibXkwcVNkN09rd3JYeEtl?=
 =?utf-8?B?THQ5alJ3cGtaTkwxek5ZMzJrZ3d4UFhQWWFqZHVKbVRaaDE4cE83RHNyeXlv?=
 =?utf-8?B?Mjk1dEJOVDlHbldER3JmajBIU083YUpLNUFPaElhOUNEdWV6NFFtSmtjdjVI?=
 =?utf-8?B?WDBSV1FJQVZiNHhTeU8vZVU0alQ3N0FJMm0rTzVCbkgvZGJWckY0d216M256?=
 =?utf-8?B?dSsrOWxTd0VEa3JPZkhrdFNOSUEwQTJHWld1OFdzeU80Y2hXZlBUamhSYWRz?=
 =?utf-8?B?cDVhd1RGajVVOURCMmk1S3lVTlFVNUk0ckpibWtQeHI5VkJVbHFzVjBIUDBm?=
 =?utf-8?B?TUorc2w5N1FqZGdLdldSZE5pdGpyTGM1NDJITTBPVUdEc211S2pELzBaRjBx?=
 =?utf-8?B?YlVYejVCeW9qeERnS0hPNVJLNlhjeGJsZjdRYTdMNy9Cdkw1ZDdzSTZUb0VE?=
 =?utf-8?B?VHZQODNXdGF4bURqYjhxOUVMaW5YVkV2b3RVZ3E1YmRCWCtoQUJHc09pdjd1?=
 =?utf-8?B?M2F4SWVyLytDQWJvL2pXM2c0MU0vRFFSNWtvSnVqMlR3K0ZGSkN1V0svQmZI?=
 =?utf-8?B?cnVGZ2xqMEtDWlRKdlc3Nnl6NUJjRHpQRlhMeWhuK1prb1BqMnV4VTdPRGpL?=
 =?utf-8?B?TzlZTFlGKzg1TTJnMld3MDNwYkJGL0cwRXNrYUxDOVVQQkpjU1h1dXAzZ3hw?=
 =?utf-8?B?ZzJZdmVpaEFicitJWXBDMWtYY09XVG5CVkdyaUFwUi90RUs4bjk4MlByVkRO?=
 =?utf-8?B?K0VjYVh1eU50Q0wza3QrblMzTi9LMFIrS256L3pVS3J0WkRNNytoZ2Roclh5?=
 =?utf-8?B?ZlU5NTRwbFJlakFiZVdmS3NZNlFpUjlTbXF5RW5YUzlpTDh2QjNMVDg4SEJK?=
 =?utf-8?B?ejJRL0JBUWpXN0RGOHZuNHlVNEZyaU42ZEV3WitlQmpIMXZuT21nc1lqMThY?=
 =?utf-8?B?VkNHWUVHdVpDZEg4YVRCSWE0dlgwR2JpWEZGdkEwOVljdjAyWXZ0UTNwUlJ4?=
 =?utf-8?B?SUR3dEdNUWZ6dmFJY3c2ZDdoeWw4YTBqRENRMHNRN2hJbVdOaldDb0ZNM3Mz?=
 =?utf-8?B?ajJLZ1hqMVl2SVV1NldlWHEyWWZDaHNjQzJLRS9hOWtOTCtVUGRHSGRMR0Ns?=
 =?utf-8?B?eGkwY1RsVDRLQkR2YzBkU1dkTW1tQUlEQVJLQ2FRSFg2YndzYTVWOHlYMTIx?=
 =?utf-8?B?aHllM1IvNiszc2dRZ09kS3JVcVE4bDMrT1Z3ZHF0T3JucUVIaGt1OFNaWHdN?=
 =?utf-8?B?RlMvci9sVGtna09xOUVTZHRzTFhoUXhpNEZJeHgrQ0V3QXRQaHRVUldYMjk4?=
 =?utf-8?B?KzN4eU9mMGl5bVNaZ3BZMlZwWVJETGhHY0ZBajhTSHB2Qm5peGM4QkZvc0Jh?=
 =?utf-8?B?QWsrdEljWnc4dmhBazVjR3BVWUMwZUZqV0VTODRWQVB4N3I0Qkw5dmdqd3lz?=
 =?utf-8?B?czIyMFhQamNVQlNsV1RtMzlJTFc4T3YxRllGdlVsRDRuK0REZXBlc2c2WlM1?=
 =?utf-8?B?eE9DeHgrbWVuVmxlYUVudTBadjF4MnROMnQxNjlRSGU0enNLUWpOQ2Jlbm9a?=
 =?utf-8?B?K3dpakdvY0tzMFVvbmFNcyt4WmhJZ1RJdUYyblBsdU1tVU5GQnMrclNnTWRE?=
 =?utf-8?Q?WlZoJt4HXkcOnCiEVZFIgNHBm?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: ec6c4ddd-b5a6-4791-62aa-08dbc0c79d06
X-MS-Exchange-CrossTenant-AuthSource: MW4PR12MB5667.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 29 Sep 2023 08:39:37.2624
 (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: zpI//NYFSx2C6k8aeH20V3IzpYeRNCK7KeXNwHLFlJgurpb/UbTcAj+LBytKX6zPMO3m6a4vtStIXVKr001udQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH3PR12MB8306
X-Spam-Status: No, score=-2.3 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
	autolearn=unavailable autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on groat.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 (groat.vger.email [0.0.0.0]); Fri, 29 Sep 2023 01:40:21 -0700 (PDT)


On 28/09/2023 20:53, Felix Kuehling wrote:
> On 2023-09-28 11:38, Shashank Sharma wrote:
>> Hello Felix, Mukul,
>>
>> On 28/09/2023 17:30, Felix Kuehling wrote:
>>> On 2023-09-28 10:30, Joshi, Mukul wrote:
>>>> [AMD Official Use Only - General]
>>>>
>>>>> -----Original Message-----
>>>>> From: Yadav, Arvind <Arvind.Yadav@amd.com>
>>>>> Sent: Thursday, September 28, 2023 5:54 AM
>>>>> To: Koenig, Christian <Christian.Koenig@amd.com>; Deucher, Alexander
>>>>> <Alexander.Deucher@amd.com>; Sharma, Shashank
>>>>> <Shashank.Sharma@amd.com>; Kuehling, Felix <Felix.Kuehling@amd.com>;
>>>>> Joshi, Mukul <Mukul.Joshi@amd.com>; Pan, Xinhui <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; Yadav, Arvind <Arvind.Yadav@amd.com>; Koenig,
>>>>> Christian <Christian.Koenig@amd.com>
>>>>> Subject: [PATCH v2 1/1] drm/amdkfd: Fix unaligned doorbell 
>>>>> absolute offset
>>>>> for gfx8
>>>>>
>>>>> This patch is to adjust the absolute doorbell offset against the 
>>>>> doorbell id
>>>>> considering the doorbell size of 32/64 bit.
>>>>>
>>>>> v2:
>>>>> - Addressed the review comment from Felix.
>>>>>
>>>>> 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>
>>>>> ---
>>>>>   drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c | 9 ++++++++-
>>>>>   1 file changed, 8 insertions(+), 1 deletion(-)
>>>>>
>>>>> 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..c54c4392d26e 100644
>>>>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
>>>>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
>>>>> @@ -407,7 +407,14 @@ static int allocate_doorbell(struct
>>>>> qcm_process_device *qpd,
>>>>>
>>>>>        q->properties.doorbell_off = amdgpu_doorbell_index_on_bar(dev-
>>>>>> adev,
>>>>>                                                                  qpd-
>>>>>> proc_doorbells,
>>>>> -                                                               q-
>>>>>> doorbell_id);
>>>>> +                                                               0);
>>>>> +
>>>> It looks like amdgpu_doorbell_index_on_bar() works only for 64-bit 
>>>> doorbells.
>>>> Shouldn't it work for both 32-bit and 64-bit doorbells considering 
>>>> this is common
>>>> doorbell manager code?
>>
>>
>> Yes, You are right that the calculations to find a particular 
>> doorbell in the doorbell page considers a doorbell width of 64-bit.
>>
>>>
>>> I could see this argument going either way. KFD is the only one that 
>>> cares about managing doorbells for user mode queues on GFXv8 GPUs. 
>>> This is not a use case that amdgpu cares about. So I'm OK with KFD 
>>> doing its own address calculations to make sure doorbells continue 
>>> to work on GFXv8.
>>>
>>> It may not be worth adding complexity to the common doorbell manager 
>>> code to support legacy GPUs with 32-bit doorbells.
>>
>>
>> I was thinking about adding an additional input parameter which will 
>> indicate if the doorbell width is 32-bit vs 64-bit (like 
>> is_doorbell_64_bit), and doorbell manager can alter the multiplier 
>> while calculating the final offset. Please let me know if that will 
>> work for both the cases.
>
> Yes, that would work for KFD because we already have the doorbell size 
> in our device-info structure. Instead of making it a boolean flag, you 
> could make it a doorbell_size parameter, in byte or dword units to 
> simplify the pointer math.


Sounds good, will do that and send an update.

- Shashank

>
> Regards,
>   Felix
>
>
>>
>> - Shashank
>>
>>>
>>>
>>> Regards,
>>>   Felix
>>>
>>>
>>>>
>>>> Thanks,
>>>> Mukul
>>>>
>>>>> +     /* Adjust the absolute doorbell offset against the doorbell id
>>>>> considering
>>>>> +      * the doorbell size of 32/64 bit.
>>>>> +      */
>>>>> +     q->properties.doorbell_off += q->doorbell_id *
>>>>> + dev->kfd->device_info.doorbell_size / 4;
>>>>> +
>>>>>        return 0;
>>>>>   }
>>>>>
>>>>> -- 
>>>>> 2.34.1