From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 9C7DDC61DF4 for ; Fri, 24 Nov 2023 09:17:39 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232840AbjKXJRb (ORCPT ); Fri, 24 Nov 2023 04:17:31 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:48804 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229705AbjKXJR2 (ORCPT ); Fri, 24 Nov 2023 04:17:28 -0500 Received: from madras.collabora.co.uk (madras.collabora.co.uk [46.235.227.172]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id DC38E12B for ; Fri, 24 Nov 2023 01:17:33 -0800 (PST) Received: from [100.107.97.3] (cola.collaboradmins.com [195.201.22.229]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) (Authenticated sender: kholk11) by madras.collabora.co.uk (Postfix) with ESMTPSA id ADB076607341; Fri, 24 Nov 2023 09:17:31 +0000 (GMT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=collabora.com; s=mail; t=1700817452; bh=jfNGllP778vYujaJKDb/B9ipiM15hlfBX7FsAcZ8aJU=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=XHhMQg1jVFz37OKm/l5D/BhYa0YgRFKjGtcfhOU51GrE3si7b27fXKNAdJm51njjO G/OQFBnMiGDfvYDHHtqdVRA4/xHB0DqM75ZWypZHj+oKeFYUMv9g10mvHnlY7DAOQU 8Fe8zrP8ALDNYp9AgX7YYkzVlXOJYm685RAbsVpDHv/Cqm43yHaUdzOHdkGGsI9Ap/ YE2KxmPlSkkIwCcc7/6PmhYTbIR6seuwkMRslZosZUJMEd/pXQ9ojltw9Tn6zZfuBV fO4X6cwGDJiDTlGv33SG8d4VmoYbQzXK3wieJzyIfqlk6dDa8fVHYr0AfjIRyjCh+1 ac0PFAXPYLhXw== Message-ID: <5e60f1d1-8e3a-42ca-af56-126faa67ea86@collabora.com> Date: Fri, 24 Nov 2023 10:17:29 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH] drm/panfrost: Ignore core_mask for poweroff and sync interrupts To: Boris Brezillon Cc: steven.price@arm.com, robh@kernel.org, maarten.lankhorst@linux.intel.com, mripard@kernel.org, tzimmermann@suse.de, airlied@gmail.com, daniel@ffwll.ch, dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org, krzysztof.kozlowski@linaro.org, kernel@collabora.com References: <20231123095320.41433-1-angelogioacchino.delregno@collabora.com> <20231123113530.46191ded@collabora.com> <1740797f-f3ae-4868-924a-08d6d731e506@collabora.com> <20231123135933.34d643f7@collabora.com> <5019af46-f5ae-4db5-979e-802b61025ba4@collabora.com> <20231123145103.23b6eac9@collabora.com> <43cc8641-6a60-41d9-b8f2-32227235702a@collabora.com> <20231123164019.629c91f9@collabora.com> From: AngeloGioacchino Del Regno Content-Language: en-US In-Reply-To: <20231123164019.629c91f9@collabora.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Il 23/11/23 16:40, Boris Brezillon ha scritto: > On Thu, 23 Nov 2023 16:14:12 +0100 > AngeloGioacchino Del Regno > wrote: > >> Il 23/11/23 14:51, Boris Brezillon ha scritto: >>> On Thu, 23 Nov 2023 14:24:57 +0100 >>> AngeloGioacchino Del Regno >>> wrote: >>> >>>>>> >>>>>> So, while I agree that it'd be slightly more readable as a diff if those >>>>>> were two different commits I do have reasons against splitting..... >>>>> >>>>> If we just need a quick fix to avoid PWRTRANS interrupts from kicking >>>>> in when we power-off the cores, I think we'd be better off dropping >>>>> GPU_IRQ_POWER_CHANGED[_ALL] from the value we write to GPU_INT_MASK >>>>> at [re]initialization time, and then have a separate series that fixes >>>>> the problem more generically. >>>>> >>>> >>>> But that didn't work: >>>> https://lore.kernel.org/all/d95259b8-10cf-4ded-866c-47cbd2a44f84@linaro.org/ >>> >>> I meant, your 'ignore-core_mask' fix + the >>> 'drop GPU_IRQ_POWER_CHANGED[_ALL] in GPU_INT_MASK' one. >>> >>> So, >>> >>> https://lore.kernel.org/all/4c73f67e-174c-497e-85a5-cb053ce657cb@collabora.com/ >>> + >>> https://lore.kernel.org/all/d95259b8-10cf-4ded-866c-47cbd2a44f84@linaro.org/ >>> >>>> >>>> >>>> ...while this "full" solution worked: >>>> https://lore.kernel.org/all/39e9514b-087c-42eb-8d0e-f75dc620e954@linaro.org/ >>>> >>>> https://lore.kernel.org/all/5b24cc73-23aa-4837-abb9-b6d138b46426@linaro.org/ >>>> >>>> >>>> ...so this *is* a "quick fix" already... :-) >>> >>> It's a half-baked solution for the missing irq-synchronization-on-suspend >>> issue IMHO. I understand why you want it all in one patch that can serve >>> as a fix for 123b431f8a5c ("drm/panfrost: Really power off GPU cores in >>> panfrost_gpu_power_off()"), which is why I'm suggesting to go for an >>> even simpler diff (see below), and then fully address the >>> irq-synhronization-on-suspend issue in a follow-up patchset. >>> >>> --->8--- >>> diff --git a/drivers/gpu/drm/panfrost/panfrost_gpu.c b/drivers/gpu/drm/panfrost/panfrost_gpu.c >>> index 09f5e1563ebd..6e2d7650cc2b 100644 >>> --- a/drivers/gpu/drm/panfrost/panfrost_gpu.c >>> +++ b/drivers/gpu/drm/panfrost/panfrost_gpu.c >>> @@ -78,7 +78,10 @@ int panfrost_gpu_soft_reset(struct panfrost_device *pfdev) >>> } >>> >>> gpu_write(pfdev, GPU_INT_CLEAR, GPU_IRQ_MASK_ALL); >>> - gpu_write(pfdev, GPU_INT_MASK, GPU_IRQ_MASK_ALL); > > We probably want a comment here: > > /* Only enable the interrupts we care about. */ > >>> + gpu_write(pfdev, GPU_INT_MASK, >>> + GPU_IRQ_MASK_ERROR | >>> + GPU_IRQ_PERFCNT_SAMPLE_COMPLETED | >>> + GPU_IRQ_CLEAN_CACHES_COMPLETED); >>> >> >> ...but if we do that, the next patch(es) will contain a partial revert of this >> commit, putting back this to gpu_write(pfdev, GPU_INT_MASK, GPU_IRQ_MASK_ALL)... > > Why should we revert it? We're not processing the PWRTRANS interrupts > in the interrupt handler, those should never have been enabled in the > first place. The only reason we'd want to revert that change is if we > decide to do have interrupt-based waits in the poweron/off > implementation, which, as far as I'm aware, is not something we intend > to do any time soon. > You're right, yes. Okay, I'll push the new code soon. Cheers! >> >> I'm not sure that it's worth changing this like that, then changing it back right >> after :-\ >> >> Anyway, if anyone else agrees with doing it and then partially revert, I have no >> issues going with this one instead; what I care about ultimately is resolving the >> regression ASAP :-) >> >> Cheers, >> Angelo >> >>> /* >>> * All in-flight jobs should have released their cycle >>> @@ -425,11 +428,10 @@ void panfrost_gpu_power_on(struct panfrost_device *pfdev) >>> >>> void panfrost_gpu_power_off(struct panfrost_device *pfdev) >>> { >>> - u64 core_mask = panfrost_get_core_mask(pfdev); >>> int ret; >>> u32 val; >>> >>> - gpu_write(pfdev, SHADER_PWROFF_LO, pfdev->features.shader_present & core_mask); >>> + gpu_write(pfdev, SHADER_PWROFF_LO, pfdev->features.shader_present); >>> ret = readl_relaxed_poll_timeout(pfdev->iomem + SHADER_PWRTRANS_LO, >>> val, !val, 1, 1000); >>> if (ret) >>> @@ -441,7 +443,7 @@ void panfrost_gpu_power_off(struct panfrost_device *pfdev) >>> if (ret) >>> dev_err(pfdev->dev, "tiler power transition timeout"); >>> >>> - gpu_write(pfdev, L2_PWROFF_LO, pfdev->features.l2_present & core_mask); >>> + gpu_write(pfdev, L2_PWROFF_LO, pfdev->features.l2_present); >>> ret = readl_poll_timeout(pfdev->iomem + L2_PWRTRANS_LO, >>> val, !val, 0, 1000); >>> if (ret) >>> >>> >> >