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: <4e30fde8-f54c-4e07-b52d-fe90e5d44332@collabora.com>
Date:   Mon, 4 Dec 2023 12:25:44 +0100
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v3 3/3] drm/panfrost: Synchronize and disable interrupts
 before powering off
Content-Language: en-US
To:     Steven Price <steven.price@arm.com>,
        Boris Brezillon <boris.brezillon@collabora.com>
Cc:     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, kernel@collabora.com,
        m.szyprowski@samsung.com, krzysztof.kozlowski@linaro.org
References: <20231201104027.35273-1-angelogioacchino.delregno@collabora.com>
 <20231201104027.35273-4-angelogioacchino.delregno@collabora.com>
 <20231201121437.7d5cdefb@collabora.com>
 <5cad3d21-5485-49aa-ae97-589dcb831925@arm.com>
From:   AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@collabora.com>
In-Reply-To: <5cad3d21-5485-49aa-ae97-589dcb831925@arm.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

Il 01/12/23 13:34, Steven Price ha scritto:
> On 01/12/2023 11:14, Boris Brezillon wrote:
>> On Fri,  1 Dec 2023 11:40:27 +0100
>> AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
>> wrote:
>>
>>> To make sure that we don't unintentionally perform any unclocked and/or
>>> unpowered R/W operation on GPU registers, before turning off clocks and
>>> regulators we must make sure that no GPU, JOB or MMU ISR execution is
>>> pending: doing that required to add a mechanism to synchronize the
>>
>>                        ^ requires the addition of a mechanism...
>>
>>> interrupts on suspend.
>>>
>>> Add functions panfrost_{gpu,job,mmu}_suspend_irq() which will perform
>>> interrupts masking and ISR execution synchronization, and then call
>>> those in the panfrost_device_runtime_suspend() handler in the exact
>>> sequence of job (may require mmu!) -> mmu -> gpu.
>>>
>>> As a side note, JOB and MMU suspend_irq functions needed some special
>>> treatment: as their interrupt handlers will unmask interrupts, it was
>>> necessary to add a bitmap for `is_suspended` which is used to address
>>
>>              to add an `is_suspended` bitmap which is used...
>>
>>> the possible corner case of unintentional IRQ unmasking because of ISR
>>> execution after a call to synchronize_irq().
>>
>> Also fixes the case where the interrupt handler is called when the
>> device is suspended because the IRQ line is shared with another device.
>> No need to update the commit message for that though.
>>
>>>
>>> At resume, clear each is_suspended bit in the reset path of JOB/MMU
>>> to allow unmasking the interrupts.
>>>
>>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
>>> ---
> 
> <snip>
> 
>>>   static void panfrost_job_handle_err(struct panfrost_device *pfdev,
>>>   				    struct panfrost_job *job,
>>>   				    unsigned int js)
>>> @@ -792,9 +802,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_suspended))
>>> +		job_write(pfdev, JOB_INT_MASK,
>>> +			  GENMASK(16 + NUM_JOB_SLOTS - 1, 16) |
>>> +			  GENMASK(NUM_JOB_SLOTS - 1, 0));
>>> +
>>
>> Missing if (test_bit(PANFROST_COMP_BIT_JOB, pfdev->is_suspended)) in
>> panfrost_job_irq_handler(), to make sure you don't access the registers
>> if the GPU is suspended.
> 
> I think generally these IRQ handler functions should simply check the
> is_suspended flag and early out if the flag is set. It's not the
> re-enabling of the interrupts specifically that we want to gate - it's
> any access to the hardware as in the shared-IRQ case the GPU might
> already have been powered down/unclocked.
> 

Yes, in the thread handler we're still powered, because we are synchronizing
irqs - adding the test_bit in the hardirq handler will prevent scheduling the
irq_handler_thread.

What the test_bit() here does is to allow us to handle the last interrupt(s)
(synchronize_irqs() in the suspend function) before cutting off power, without
unwillingly re-enabling the job (or mmu in panfrost_mmu.c) interrupts.

Cheers,
Angelo