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 06B73C4167B for ; Tue, 28 Nov 2023 15:54:16 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1346454AbjK1PyH (ORCPT ); Tue, 28 Nov 2023 10:54:07 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:34358 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1346497AbjK1Px4 (ORCPT ); Tue, 28 Nov 2023 10:53:56 -0500 Received: from madras.collabora.co.uk (madras.collabora.co.uk [IPv6:2a00:1098:0:82:1000:25:2eeb:e5ab]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id DE773BD for ; Tue, 28 Nov 2023 07:54:02 -0800 (PST) Received: from localhost (cola.collaboradmins.com [195.201.22.229]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) (Authenticated sender: bbrezillon) by madras.collabora.co.uk (Postfix) with ESMTPSA id BB97D66072A4; Tue, 28 Nov 2023 15:54:00 +0000 (GMT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=collabora.com; s=mail; t=1701186841; bh=kX8VFnjMmnUgzzjN92JSyU07DWZZRpGOPGYPIFQv0sI=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=hlDKx346ueHXtFqp4rG4thCHSpEwBhnnPyznIUhe0IsmQLLDEg6UMSVZfW0TI0F7a 67uk/P5Mzu4dCCr0hGtRvlwvXObfkHClrlrBQDcOSLenmc7EHauF48DDUOf50uTfwj i4K7CsQcUQ5deNunZ7R8h4YMRA86C7foHWkP/foEMPeCx73eba0ApCVo8HbxuC3GQB AFS3YeB/T73Kod+k0HdtCa+2ldqcMN4ybpwlGsJC93RvUo8iQ8mwZe/01egy2rOWGa DXCuQZ8JoYp/7+4VsEAvjOXm6b1snFUORsOJybic7rnugvRJE127SN6UC+0aa5YoYW 9CkRSBuHhILWQ== Date: Tue, 28 Nov 2023 16:53:57 +0100 From: Boris Brezillon To: AngeloGioacchino Del Regno Cc: robh@kernel.org, steven.price@arm.com, 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, kernel@collabora.com, m.szyprowski@samsung.com, krzysztof.kozlowski@linaro.org Subject: Re: [PATCH v2 3/3] drm/panfrost: Synchronize and disable interrupts before powering off Message-ID: <20231128165357.2c9bfdf1@collabora.com> In-Reply-To: References: <20231128124510.391007-1-angelogioacchino.delregno@collabora.com> <20231128124510.391007-4-angelogioacchino.delregno@collabora.com> <20231128145712.3f4d3f74@collabora.com> Organization: Collabora X-Mailer: Claws Mail 4.1.1 (GTK 3.24.38; x86_64-redhat-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, 28 Nov 2023 16:10:45 +0100 AngeloGioacchino Del Regno wrote: > >> static void panfrost_job_handle_err(struct panfrost_device *pfdev, > >> struct panfrost_job *job, > >> unsigned int js) > >> @@ -792,9 +800,13 @@ static irqreturn_t panfrost_job_irq_handler_thread(int irq, void *data) > >> struct panfrost_device *pfdev = data; > >> > >> panfrost_job_handle_irqs(pfdev); > >> - job_write(pfdev, JOB_INT_MASK, > >> - GENMASK(16 + NUM_JOB_SLOTS - 1, 16) | > >> - GENMASK(NUM_JOB_SLOTS - 1, 0)); > >> + > >> + /* Enable interrupts only if we're not about to get suspended */ > >> + if (!test_bit(PANFROST_COMP_BIT_JOB, pfdev->is_suspending)) > > > > The irq-line is requested with IRQF_SHARED, meaning the line might be > > shared between all three GPU IRQs, but also with other devices. I think > > if we want to be totally safe, we need to also check this is_suspending > > field in the hard irq handlers before accessing the xxx_INT_yyy > > registers. > > > > This would mean that we would have to force canceling jobs in the suspend > handler, but if the IRQ never fired, would we still be able to find the > right bits flipped in JOB_INT_RAWSTAT? There should be no jobs left if we enter suspend. If there is, that's a bug we should fix, but I'm digressing. > > From what I understand, are you suggesting to call, in job_suspend_irq() > something like > > void panfrost_job_suspend_irq(struct panfrost_device *pfdev) > { > u32 status; > > set_bit(PANFROST_COMP_BIT_JOB, pfdev->is_suspending); > > job_write(pfdev, JOB_INT_MASK, 0); > synchronize_irq(pfdev->js->irq); > > status = job_read(pfdev, JOB_INT_STAT); I guess you meant _RAWSTAT. _STAT should always be zero after we've written 0 to _INT_MASK. > if (status) > panfrost_job_irq_handler_thread(pfdev->js->irq, (void*)pfdev); Nope, we don't need to read the STAT reg and forcibly call the threaded handler if it's != 0. The synchronize_irq() call should do exactly that (make sure all pending interrupts are processed before returning), and our previous job_write(pfdev, JOB_INT_MASK, 0) guarantees that no new interrupts will kick in after that point. > } > > and then while still retaining the check in the IRQ thread handler, also > check it in the hardirq handler like > > static irqreturn_t panfrost_job_irq_handler(int irq, void *data) > { > struct panfrost_device *pfdev = data; > u32 status; > > if (!test_bit(PANFROST_COMP_BIT_JOB, pfdev->is_suspending)) > return IRQ_NONE; Yes, that's the extra check I was talking about, and that's also the very reason I'm suggesting to call this field suspended_irqs instead of is_suspending. Ultimately, each bit in this bitmap encodes the status of a specific IRQ, not the transition from active-to-suspended, otherwise we'd be clearing the bit at the end of panfrost_job_suspend_irq(), right after the synchronize_irq(). But if we were doing that, our hard IRQ handler could be called because other devices raised an interrupt on the very same IRQ line while we are suspended, and we'd be doing an invalid GPU reg read while the clks/power-domains are off. > > status = job_read(pfdev, JOB_INT_STAT); > if (!status) > return IRQ_NONE; > > job_write(pfdev, JOB_INT_MASK, 0); > return IRQ_WAKE_THREAD; > } > > (rinse and repeat for panfrost_mmu) > > ..or am I misunderstanding you? > > Cheers, > Angelo > >