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: <ea5d5e75-9c63-b4c4-40f1-dea42b121223@amd.com>
Date:   Mon, 17 Oct 2022 11:57:43 -0400
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.3.3
Subject: Re: [PATCH v2] drm/amd/display: add an ASSERT() to irq service
 functions
To:     Hamza Mahfooz <hamza.mahfooz@amd.com>,
        amd-gfx@lists.freedesktop.org
Cc:     Roman Li <roman.li@amd.com>, Leo Li <sunpeng.li@amd.com>,
        Rodrigo Siqueira <Rodrigo.Siqueira@amd.com>,
        Alex Deucher <alexander.deucher@amd.com>,
        =?UTF-8?Q?Christian_K=c3=b6nig?= <christian.koenig@amd.com>,
        "Pan, Xinhui" <Xinhui.Pan@amd.com>,
        David Airlie <airlied@linux.ie>,
        Daniel Vetter <daniel@ffwll.ch>,
        Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>,
        Mario Limonciello <mario.limonciello@amd.com>,
        Alan Liu <HaoPing.Liu@amd.com>, Alex Hung <alex.hung@amd.com>,
        dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org
References: <20221017153836.65515-1-hamza.mahfooz@amd.com>
Content-Language: en-US
From:   Harry Wentland <harry.wentland@amd.com>
In-Reply-To: <20221017153836.65515-1-hamza.mahfooz@amd.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?a0wvdXRwQ21hbkNmS3NYN1Uxeno3alloYzJndDlidURGZi93UFh6cS9MdnM1?=
 =?utf-8?B?bXRkMVJHVitOWjBLMlNFMFplZ0l0VVRkSGpiUzhrdmlWTXNmMDZjNjBnZUVj?=
 =?utf-8?B?WXFBMVJHOVdlaXlmeDUwdGcxR211Qks2Q3pXQ25YVm90ZExTd01kUlB1OFA5?=
 =?utf-8?B?YkVjYXVGQXhtNE95VVhOZGFlaC9HWXYwUHZ0ckNlZ3BkY2RzcDh1S21wNnkx?=
 =?utf-8?B?SGdabVFNNk9WcG5tTnZRLzBpd1RCaG5vejVTUVNGL2pxcnVweVlPSWp5LzVa?=
 =?utf-8?B?L1RTZWVZSnhYN29adHIraXZuaGpUYk5pWlpEZkJWczVsM3FHU1lCWDQwUFdo?=
 =?utf-8?B?enZuWFNITGtLTU53NkE4WjErRFZGSTVBZlduM3Rwbk1yT3JlSXE2dklXbFRF?=
 =?utf-8?B?K21HNm5HS2hxYmxOSlhhbFU0Zkp4NWZxcUFZNCtkYWVJalVjLzRtSlYwZk85?=
 =?utf-8?B?STNDSG0xeTkxSnVOMjZHNVFiUGNudzUzRmxuN0s4OXlQa3M1QVBNZ3EybUw0?=
 =?utf-8?B?VFVCZGJJWUZZMm5xWmpwS3F1YVdmdVZNWTd6NHdSc0NUZWh4MWJoZDF1YkNy?=
 =?utf-8?B?aCsyNmpVT1EvaS9tZ29RNnBYU3NyM2d6YmtkWFFScU9PUEV1YkRqdVRqcjEv?=
 =?utf-8?B?TGVuLzdCWFhwMS9KcmFCbUNDODNRRmROSE1mSzZlYnpGTE9PdlExSTBXajdB?=
 =?utf-8?B?OHh5dTdBdTR1c09LNzVxdVhKb3oxbktBYTFPQ1hxY2tXRnJpQm4rMnpITkw5?=
 =?utf-8?B?VzdBM2o5QndXclhEeGdWempLV2xoUXdqelB5OS8yN0cvTmxtcG5HNFRCWU10?=
 =?utf-8?B?NXN4SC9DRUcwVHc3Zjk4a3U5MWIxWEFpeEpQOEVGRCtReTZtRkdUVGR4RGhB?=
 =?utf-8?B?NFBRY21DTXZiVzBzVUtobzI4amgwRERNUWFLZDk1dkpucWhxK2k0a0N5a3o4?=
 =?utf-8?B?cHBxdU5MSXoxRGhzdkY4Tnk4SjkvaEZ1S0tES2dKMzlSdkNZY1NTczRxaVNX?=
 =?utf-8?B?aEdKcG1Sc2MyeUhmUFg1cG5XNU5UcEJqTUk5L2crU1RRT0dxTHFZdCtKYjVE?=
 =?utf-8?B?S01VSzRWTEZtNDZXZkhmSVM1cStWV1hsVHF3bFJyY2lKUmVVNWc1SG9QQ0F1?=
 =?utf-8?B?OXdxYmNjZUVXb1N2OEg0aWpKZ0YxMXhWS0dBNDlMZHFBZU9vbml2T0JvZlpL?=
 =?utf-8?B?TmxEVktwQlZZcE55WUhFZGRnQ3cvcXFXa01UMWx5cEgvUXBBaFJkdU9hK3Jz?=
 =?utf-8?B?ZDBNUXlnUloxZ2gxMWNJaWNsdGZTekFVS0YxekpaWVNNcGZJZ29BU0lGbDRN?=
 =?utf-8?B?bk9nTkxiS0E0ZHZqVndQajZaamJEcUJkUEorODlWekJkQ0RTWEtpTG5QUHJG?=
 =?utf-8?B?VnN4RnZRM1lkUWtUWi9KRmRhaGwzdk1pNENBZE4zNTNIbnRzT1dPc041aDJv?=
 =?utf-8?B?M0xUNWJVRVJQRTlab3ZTaTcxaGRWTWsxNS9ELzI1UzFaU3FTZ2Rra3ZIVk5L?=
 =?utf-8?B?dHNqMkRhWC9qMUZnN1ZxMXExc1NxT0FGZ3VCT3YrNkFkYThweXlxcTBSaTF1?=
 =?utf-8?B?YUZVWXhBN3Y3dGplSjdJU3VFUS9qNWxNbms0bjlpeEo2R2E4QkE0U0dRZzMx?=
 =?utf-8?B?NXQ3bkVaVEd1WjgzbGpUM245Q0dKTGZXRm96UXRUd0RQM2IrSVRJVWJBL0Qv?=
 =?utf-8?B?dnE5UmVnWEtTR0MzQ1E3SmRXaGVLNE5hOHRtRmY2WFgzay9nTVc4Z0pMaERB?=
 =?utf-8?B?bTNSRU1zU3hiRG8rZHMyVVRHUDIrQ2JXZ3o5ZFNXQmVhSnlSWjBITG9QaFRQ?=
 =?utf-8?B?QzArbHdwZitqQ2hQOStTWW5VUnZIZkRNM3Q3N1grbC9vL0wrVGorK3lCeXJN?=
 =?utf-8?B?S0xNeHNvMlBoSm9PNU9qcUl2ZndIYWxtN29MWFc5YVBPZlE1VHdKWUVBZG0r?=
 =?utf-8?B?eDhRYWNLbTh6NFFlQ2UxUmZLOGpLbENxQmVKQ0FtTEpvNXloMWkzaGhuZG1u?=
 =?utf-8?B?MXhsbXhOYjlsdWdRa2RvQjRtemlIWC9ody9QMGl3MElkcVFCeld0OHdGK1Nr?=
 =?utf-8?B?ekdlcDR1QUJMYk5xb2lLVHR5ZDZ0cDVTVXlrYVY0ZDdnaU9mY1NoVTdNSisx?=
 =?utf-8?Q?bo5+8gnLMOacG2y4Mk7MOoGwv?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: ee035020-ad1a-4f33-8d55-08dab058564c
X-MS-Exchange-CrossTenant-AuthSource: CO6PR12MB5427.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 17 Oct 2022 15:57:47.9220
 (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: fs79cwl3DoZMOmzQ1aI9YFSUBlmo3guLlsCLEZZlOeN7Dcya3uR3RUE311XDP7n/Udlzh6ljGk+Qs16FJ42LLg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH7PR12MB6563
X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,
        RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS autolearn=ham
        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


On 2022-10-17 11:38, Hamza Mahfooz wrote:
> Currently, if we encounter unimplemented functions, it is difficult to
> tell what caused them just by looking at dmesg and that is compounded by
> the fact that it is often hard to reproduce said issues, for instance we
> have had reports of this condition being triggered when removing a
> secondary display that is setup in mirror mode and is connected using
> usb-c. So, to have access to more detailed debugging information, add an
> ASSERT() to dal_irq_service_ack() and dal_irq_service_set() that only
> triggers when we encounter an unimplemented function.
> 
> Signed-off-by: Hamza Mahfooz <hamza.mahfooz@amd.com>
> ---
> v2: detail specific instance that I'm interested in and use ASSERT()
>     instead of WARN().
> ---
>  .../gpu/drm/amd/display/dc/irq/irq_service.c    | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/display/dc/irq/irq_service.c b/drivers/gpu/drm/amd/display/dc/irq/irq_service.c
> index 7bad39bba86b..3d6ab4fd25cb 100644
> --- a/drivers/gpu/drm/amd/display/dc/irq/irq_service.c
> +++ b/drivers/gpu/drm/amd/display/dc/irq/irq_service.c
> @@ -112,8 +112,15 @@ bool dal_irq_service_set(
>  
>  	dal_irq_service_ack(irq_service, source);
>  
> -	if (info->funcs && info->funcs->set)
> +	if (info->funcs && info->funcs->set) {
> +		if (info->funcs->set == dal_irq_service_dummy_set)
> +			DC_LOG_WARNING("%s: src: %d, st: %d\n", __func__,
> +				       source, enable);
> +
> +		ASSERT(info->funcs->set != dal_irq_service_dummy_set);

I think you'll want the ASSERT inside the if block.

Harry

> +
>  		return info->funcs->set(irq_service, info, enable);
> +	}
>  
>  	dal_irq_service_set_generic(irq_service, info, enable);
>  
> @@ -146,8 +153,14 @@ bool dal_irq_service_ack(
>  		return false;
>  	}
>  
> -	if (info->funcs && info->funcs->ack)
> +	if (info->funcs && info->funcs->ack) {
> +		if (info->funcs->ack == dal_irq_service_dummy_ack)
> +			DC_LOG_WARNING("%s: src: %d\n", __func__, source);
> +
> +		ASSERT(info->funcs->ack != dal_irq_service_dummy_ack);
> +
>  		return info->funcs->ack(irq_service, info);
> +	}
>  
>  	dal_irq_service_ack_generic(irq_service, info);
>