Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <2404b7f3-a465-a706-8af3-459105f0dc54@amd.com>
Date:   Wed, 31 May 2023 15:33:16 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.11.0
Subject: Re: [PATCH] drm/amdgpu: Rearrange WREG32 operations in gfxhub_v2_1.c
Content-Language: en-US
To:     ghostfly233 <ghostfly23333@gmail.com>, alexander.deucher@amd.com,
        Xinhui.Pan@amd.com
Cc:     amd-gfx@lists.freedesktop.org, linux-kernel@vger.kernel.org,
        airlied@gmail.com, daniel@ffwll.ch
References: <64690ac5.170a0220.92bb3.2e7a@mx.google.com>
From:   =?UTF-8?Q?Christian_K=c3=b6nig?= <christian.koenig@amd.com>
In-Reply-To: <64690ac5.170a0220.92bb3.2e7a@mx.google.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?VHhjOGRiTllGdlF4NThwNU8xWGt4dk84YVhzY0ZXajJIUVJGaDF3amFPVU9w?=
 =?utf-8?B?dlc0bzBHMzFKbUQ5TG8yOUdGN1ZYdE1XQUY2blJXTUVDaEhlK1FvNndCRnVO?=
 =?utf-8?B?TmF4Nk9XU3JRV1ROTjJpdXFMNDhzQ0dYMExQWnpma1J2QmJFcEpYdjdtTHN3?=
 =?utf-8?B?QW5pd29nb2pjcU9PMTZud1hhUnh3VGJiUUQyRjlNeDhCNkFSdlowV003azk1?=
 =?utf-8?B?Y1dmczlvOG92UHlmc1F5b2Z2Vmc3T3dCMktoVWNjcS91QUlUWmNFRUhQaFRo?=
 =?utf-8?B?cFhCMEx5eFZIaUpMdkZieFJDd2RURFRuTm1HWGYxYmNGTlBtWXQzQ092UHBm?=
 =?utf-8?B?MndFMklpSnpLRk5Zb1EvMjNHNVNTNlAxYy9rVFp3dU16TjRRMHNNc3VtazFn?=
 =?utf-8?B?MitFTTRqUStmV2tQakM1eWYvSWM3QkNDWGtLak5PK05DSzdMTWZmL0FsWFRv?=
 =?utf-8?B?TUFFV1BmUXZvZ01ocERDUG16RXk5T2hLbVp2VXE5cVQ2eERHQkk1WXhqRVdH?=
 =?utf-8?B?SVVSeDZUNjdGb2FXbWQ5cU0vQm9LekRnWU8rdEw2dExuYnFkc0JRVkY5N3lz?=
 =?utf-8?B?WFdvSkh5QzdnM2VuQ3c4Vmw3RlZsK1ZWUWJWanpuZHFmUm5qTkNlRmlHc1dP?=
 =?utf-8?B?Rk1uTHdkZUt4V1lSbG1EVk9EUUdocWo2U25uREowdzhtb1RvOUpWdFZNWmli?=
 =?utf-8?B?dHh0alZLeVJsaEZKdWYvZHl0RVVid09VdklSbFkyV01LWlc5bFpibUxpeGZ5?=
 =?utf-8?B?L2hzdEUyL1pUeEtyRUprekJyMkRTWWtWMDJib3hYREJ0Y1ZjL2dacFVldHgv?=
 =?utf-8?B?WHVvai82Q1JSNjE4eStjYU9KSnYvd0xLN28wUDRjZkNmTFAzSTRhQ1graEhK?=
 =?utf-8?B?dXNINHVNcE1IMDdtSEhMRDd1R2NwYzV6WTVLUkw1WGZqQUpBdkdIOFY5QWJ5?=
 =?utf-8?B?VGd6RFpZQm5abDA1Yk1reElnZ2lDVys5NGdjNlJHa1dnYTFqeWYzdXJjU0tG?=
 =?utf-8?B?VEtYZlVnY3FJcWVoTkdWaGhiRTVlQnFKMC9nWFI1bi9NdzJpc0JzT3ZWUUhE?=
 =?utf-8?B?SDkxWVQyc2ZvWHE0Z0VFcU1lSzhFVWdqVkswQ1BqZ2huVXlMVHlYZW9WcG9F?=
 =?utf-8?B?ZHk0N3RLSjRiZlVkREJvUnhpL3JxaEovWFVCOWY5VnYzQ3NDRURaekFUZjJw?=
 =?utf-8?B?S2dpdTZOamdsVmxRdk9qY2EwaGtCMFVOeUpZL21wSlFKaituLy9qWU94Ympa?=
 =?utf-8?B?TThsRU9WMjRaN0NGdVk1NytreGVGQXlac3dTREd4SDcvUzYrbGZYelNuREpM?=
 =?utf-8?B?YTNqS2lFeng2bTVuL21yMFMvRlYrU1RleDVvL1cyVWthZnhrL2VrdDliRFV0?=
 =?utf-8?B?WG80YjhiWXhHN2hOTGxWa2VBa0E0emJBdlN4UjJ4N3pSYlBrVUFDVnpSRVE4?=
 =?utf-8?B?VEl6V1VpWStJUmx5N1NzS0hLWmpIWGx1bCt1aTZ0NFBCemdKZ3BQK2I5ZDFG?=
 =?utf-8?B?ckpGT2RxT1dVU2xtd3BFZGRQa0I2YlFPN1Z0OU00UlltUzhrTERZdlhRRmoy?=
 =?utf-8?B?U2xxZGxISmNOKzFsaVg4REJ2VHd4WklUcDd0WWw4SlRpTVJuc1pCbHAxZlYy?=
 =?utf-8?B?ZnJNK3lldGxEZlkvZTZ1MG8vOWZCRzc2Q2tJK0xNRW9TcEEzNjdLcHVzZTRJ?=
 =?utf-8?B?NnlLM1piOWxHTExzWHo0TkdLNDcvZXhoSXBVYjV0cThhTVBtQ0J5RVg0YThw?=
 =?utf-8?B?MVZXSmJhUFZPVHZvdElocHFvU2xaRGRYaUVsNzgvSEFlR1FlbDhGeDNvem5X?=
 =?utf-8?B?Nkc5RzVxZ2pubm1RdHl0cjFURmRpUnh5Z2RYb01wVDdObFJ4Ni9YbGIxNFdZ?=
 =?utf-8?B?Rkw3M1RzQmYzTEpVY1NvdXJOSkI1a05JV3BvY1Q2bWlJZTZ2ZDFyYXJOZlVG?=
 =?utf-8?B?VmJESG1iMTZjZmpUbXBzOU5sUGJKbllXbmE4eXdUUHBiNys0VDJoUW9YQnZi?=
 =?utf-8?B?bmVKSGNQUmhiZkJDUGhKVzl0SE1QQ0lMRXkvQjk0K0p4ZTdvbG9Wd0xpTXh5?=
 =?utf-8?B?a2Izd0xIVkRMVFJxRlZDczdveHB4cFRValY2ZTJnaks3bkFQK3UxRFMxUDh0?=
 =?utf-8?B?WGJLK3Y5eUpyYmtvdmp1aC92L2RxU2hsNmlLRnJ1aE5CV09WVHJjZDFiR1NG?=
 =?utf-8?Q?YXffw0ci6vUNXQfr0NDbDvV+gYmBw1+XFbjojgm/i3yz?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 9ca13133-8996-409e-fb2d-08db61db99e0
X-MS-Exchange-CrossTenant-AuthSource: BN8PR12MB3587.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 31 May 2023 13:33:21.3468
 (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: 5T4DA4XmYt6YVedL4a3o6Aa7DSqXj65rG70WRh8NP8BRUDSCjz6n3MbewhomM7cL
X-MS-Exchange-Transport-CrossTenantHeadersStamped: LV8PR12MB9206
X-Spam-Status: No, score=-1.2 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FORGED_SPF_HELO,NICE_REPLY_A,
        RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_NONE,
        T_SCC_BODY_TEXT_LINE,URIBL_BLOCKED autolearn=no autolearn_force=no
        version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Am 20.05.23 um 13:59 schrieb ghostfly233:
> In gfxhub_v2_1_setup_vmid_config(), the GCVM_CONTEXT1_CNTL reg is
> written before related GCVM_CONTEXT1_PAGE_TABLE_START_ADDR and
> GCVM_CONTEXT1_PAGE_TABLE_END_ADDR regs are written, which may
> cause undefined behavior.

In which hw documentation did you found that?

>
> This patch rearranges WREG32 operations in gfxhub_v2_1_setup_vmid_config(),
> so that it can ensure the addresses are initialized before CNTL is enabled
> and reduce the risk of encountering undefined behavior.

I have absolutely no idea how you came to this conclusion, but as far as 
I know this is just bluntly incorrect.

The control register must be written before the other parameters are set 
or otherwise the hw wouldn't know what to do with those values and might 
even block writes to the registers or hang.

Regards,
Christian.

>
> Signed-off-by: Zibin Liu <ghostfly23333@gmail.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/gfxhub_v2_1.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/gfxhub_v2_1.c b/drivers/gpu/drm/amd/amdgpu/gfxhub_v2_1.c
> index 4aacbbec31e2..6d094e7315eb 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gfxhub_v2_1.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gfxhub_v2_1.c
> @@ -321,8 +321,6 @@ static void gfxhub_v2_1_setup_vmid_config(struct amdgpu_device *adev)
>   		tmp = REG_SET_FIELD(tmp, GCVM_CONTEXT1_CNTL,
>   				    RETRY_PERMISSION_OR_INVALID_PAGE_FAULT,
>   				    !adev->gmc.noretry);
> -		WREG32_SOC15_OFFSET(GC, 0, mmGCVM_CONTEXT1_CNTL,
> -				    i * hub->ctx_distance, tmp);
>   		WREG32_SOC15_OFFSET(GC, 0, mmGCVM_CONTEXT1_PAGE_TABLE_START_ADDR_LO32,
>   				    i * hub->ctx_addr_distance, 0);
>   		WREG32_SOC15_OFFSET(GC, 0, mmGCVM_CONTEXT1_PAGE_TABLE_START_ADDR_HI32,
> @@ -333,6 +331,8 @@ static void gfxhub_v2_1_setup_vmid_config(struct amdgpu_device *adev)
>   		WREG32_SOC15_OFFSET(GC, 0, mmGCVM_CONTEXT1_PAGE_TABLE_END_ADDR_HI32,
>   				    i * hub->ctx_addr_distance,
>   				    upper_32_bits(adev->vm_manager.max_pfn - 1));
> +		WREG32_SOC15_OFFSET(GC, 0, mmGCVM_CONTEXT1_CNTL,
> +				    i * hub->ctx_distance, tmp);
>   	}
>   
>   	hub->vm_cntx_cntl = tmp;