Re: [PATCH v3 03/14] drm/panthor: Add the device logical block

From: Steven Price <steven.price@arm.com>
To: Boris Brezillon <boris.brezillon@collabora.com>
Cc: "Nicolas Boichat" <drinkcat@chromium.org>,
	kernel@collabora.com, "Daniel Stone" <daniels@collabora.com>,
	"Neil Armstrong" <neil.armstrong@linaro.org>,
	"Liviu Dudau" <Liviu.Dudau@arm.com>,
	dri-devel@lists.freedesktop.org,
	"Matt Coster" <matt.coster@imgtec.com>,
	"Clément Péron" <peron.clem@gmail.com>,
	"Donald Robson" <donald.robson@imgtec.com>,
	"Grant Likely" <grant.likely@linaro.org>,
	"Marty E . Plummer" <hanetzer@startmail.com>,
	"Robin Murphy" <robin.murphy@arm.com>,
	"Faith Ekstrand" <faith.ekstrand@collabora.com>
Subject: Re: [PATCH v3 03/14] drm/panthor: Add the device logical block
Date: Thu, 7 Dec 2023 10:23:55 +0000	[thread overview]
Message-ID: <1b957ca4-71cf-42fd-ac81-1920592b952d@arm.com> (raw)
In-Reply-To: <20231207095635.435d3e77@collabora.com>

On 07/12/2023 08:56, Boris Brezillon wrote:
> On Thu, 7 Dec 2023 09:12:43 +0100
> Boris Brezillon <boris.brezillon@collabora.com> wrote:
> 
>> On Wed, 6 Dec 2023 16:55:42 +0000
>> Steven Price <steven.price@arm.com> wrote:
>>
>>> On 04/12/2023 17:32, Boris Brezillon wrote:  
>>>> The panthor driver is designed in a modular way, where each logical
>>>> block is dealing with a specific HW-block or software feature. In order
>>>> for those blocks to communicate with each other, we need a central
>>>> panthor_device collecting all the blocks, and exposing some common
>>>> features, like interrupt handling, power management, reset, ...
>>>>
>>>> This what this panthor_device logical block is about.
>>>>
>>>> v3:
>>>> - Add acks for the MIT+GPL2 relicensing
>>>> - Fix 32-bit support
>>>> - Shorten the sections protected by panthor_device::pm::mmio_lock to fix
>>>>   lock ordering issues.
>>>> - Rename panthor_device::pm::lock into panthor_device::pm::mmio_lock to
>>>>   better reflect what this lock is protecting
>>>> - Use dev_err_probe()
>>>> - Make sure we call drm_dev_exit() when something fails half-way in
>>>>   panthor_device_reset_work()
>>>> - Replace CSF_GPU_LATEST_FLUSH_ID_DEFAULT with a constant '1' and a
>>>>   comment to explain. Also remove setting the dummy flush ID on suspend.
>>>> - Remove drm_WARN_ON() in panthor_exception_name()
>>>> - Check pirq->suspended in panthor_xxx_irq_raw_handler()
>>>>
>>>> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
>>>> Signed-off-by: Steven Price <steven.price@arm.com>
>>>> Acked-by: Steven Price <steven.price@arm.com> # MIT+GPL2 relicensing,Arm
>>>> Acked-by: Grant Likely <grant.likely@linaro.org> # MIT+GPL2 relicensing,Linaro
>>>> Acked-by: Boris Brezillon <boris.brezillon@collabora.com> # MIT+GPL2 relicensing,Collabora    
>>>
>>> We still have the "FIXME: this is racy"  
>>
>> Yeah, I still didn't find a proper solution for that.
> 
> This [1] should fix the race condition in the unplug path.
> 
> [1]https://gitlab.freedesktop.org/panfrost/linux/-/commit/b79b28069e524ae7fea22a9a158b870eab2d5f9a

Looks like it should do the job. I'm surprised that we're the only ones
to face this though.

Looking at the new imagination driver it appears there's a similar problem:

pvr_device_lost() uses a boolean 'lost' to track multiple calls but that
boolean isn't protected by any specific lock (AFAICT). Indeed
pvr_device_lost() calls drm_dev_unplug() while in a drm_dev_enter()
critical section (see pvr_mmu_flush_exec()). If I'm not mistaken that's
the same problem we discussed and isn't allowed? drm_dev_unplug() will
synchronise on the SRCU that drm_dev_enter() is holding.

+CC: Frank, Donald, Matt from Imagination.

Steve