Re: [PATCH v2 04/15] drm/panthor: Add the device logical block

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Aug 30, 2023 at 02:17:57PM +0100, Steven Price wrote:
> On 29/08/2023 15:00, Boris Brezillon wrote:
> > On Fri, 11 Aug 2023 16:47:56 +0100
> > Steven Price <steven.price@xxxxxxx> wrote:
> > 
> >> On 09/08/2023 17:53, 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.
> >>>
> >>> v2:
> >>> - Rename the driver (pancsf -> panthor)
> >>> - Change the license (GPL2 -> MIT + GPL2)
> >>> - Split the driver addition commit
> >>> - Add devfreq/PM support
> >>> - Use drm_dev_{unplug,enter,exit}() to provide safe device removal
> >>>
> >>> Signed-off-by: Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx>
> >>> ---
> >>>  drivers/gpu/drm/panthor/panthor_device.c | 479 +++++++++++++++++++++++
> >>>  drivers/gpu/drm/panthor/panthor_device.h | 354 +++++++++++++++++
> >>>  2 files changed, 833 insertions(+)
> >>>  create mode 100644 drivers/gpu/drm/panthor/panthor_device.c
> >>>  create mode 100644 drivers/gpu/drm/panthor/panthor_device.h
> >>>
> >>> diff --git a/drivers/gpu/drm/panthor/panthor_device.c b/drivers/gpu/drm/panthor/panthor_device.c
> >>> new file mode 100644
> >>> index 000000000000..15f102116fa0
> >>> --- /dev/null
> >>> +++ b/drivers/gpu/drm/panthor/panthor_device.c
> >>> @@ -0,0 +1,479 @@
> >>> +// SPDX-License-Identifier: GPL-2.0 or MIT
> >>> +/* Copyright 2018 Marty E. Plummer <hanetzer@xxxxxxxxxxxxx> */
> >>> +/* Copyright 2019 Linaro, Ltd, Rob Herring <robh@xxxxxxxxxx> */
> >>> +/* Copyright 2023 Collabora ltd. */
> >>> +
> >>> +#include <linux/clk.h>
> >>> +#include <linux/reset.h>
> >>> +#include <linux/platform_device.h>
> >>> +#include <linux/pm_domain.h>
> >>> +#include <linux/pm_runtime.h>
> >>> +#include <linux/regulator/consumer.h>
> >>> +
> >>> +#include <drm/drm_drv.h>
> >>> +#include <drm/drm_managed.h>
> >>> +
> >>> +#include "panthor_sched.h"
> >>> +#include "panthor_device.h"
> >>> +#include "panthor_devfreq.h"
> >>> +#include "panthor_gpu.h"
> >>> +#include "panthor_fw.h"
> >>> +#include "panthor_mmu.h"
> >>> +#include "panthor_regs.h"
> >>> +
> >>> +static int panthor_clk_init(struct panthor_device *ptdev)
> >>> +{
> >>> +	ptdev->clks.core = devm_clk_get(ptdev->base.dev, NULL);
> >>> +	if (IS_ERR(ptdev->clks.core)) {
> >>> +		drm_err(&ptdev->base, "get 'core' clock failed %ld\n",
> >>> +			PTR_ERR(ptdev->clks.core));  
> >>
> >> I suspect it would be a good idea to use dev_err_probe() here (and
> >> below) as I believe devm_clk_get can return -EPROBE_DEFER.
> > 
> > Nice, didn't know there was a logging function that was silencing
> > probe-defer errors.
> > 
> >>
> >>> +		return PTR_ERR(ptdev->clks.core);
> >>> +	}
> >>> +
> >>> +	ptdev->clks.stacks = devm_clk_get_optional(ptdev->base.dev, "stacks");
> >>> +	if (IS_ERR(ptdev->clks.stacks)) {
> >>> +		drm_err(&ptdev->base, "get 'stacks' clock failed %ld\n",
> >>> +			PTR_ERR(ptdev->clks.stacks));
> >>> +		return PTR_ERR(ptdev->clks.stacks);
> >>> +	}
> >>> +
> >>> +	ptdev->clks.coregroup = devm_clk_get_optional(ptdev->base.dev, "coregroup");
> >>> +	if (IS_ERR(ptdev->clks.coregroup)) {
> >>> +		drm_err(&ptdev->base, "get 'coregroup' clock failed %ld\n",
> >>> +			PTR_ERR(ptdev->clks.coregroup));
> >>> +		return PTR_ERR(ptdev->clks.coregroup);
> >>> +	}
> >>> +
> >>> +	drm_info(&ptdev->base, "clock rate = %lu\n", clk_get_rate(ptdev->clks.core));
> >>> +	return 0;
> >>> +}
> >>> +
> >>> +void panthor_device_unplug(struct panthor_device *ptdev)
> >>> +{
> >>> +	/* FIXME: This is racy. */  
> >>
> >> Can we fix this? From a quick look it seems like a sequence like below
> >> should avoid the race.
> >>
> >> 	if (!drm_dev_enter())
> >> 		/* Already unplugged */
> >> 		return;
> >> 	ptdev->base.unplugged = true;
> >> 	drm_dev_exit();
> >>
> >> Although possibly that should be in the DRM core rather than open-coded
> >> here.
> > 
> > Are you sure that's protecting us against two concurrent calls to
> > drm_dev_unplug() (drm_dev_enter() is taking a read-lock)?
> 
> Well now I'm not sure ;) This was based on the implementations of
> drm_dev_is_unplugged() and drm_dev_unplug(). drm_dev_is_unplugged()
> simply tries to enter then exit. drm_dev_unplug() sets dev->unplugged
> (without first taking any locks). So my naïve combination resulted in
> the above.
> 
> The part I was missing is the synchronize_srcu() call in
> drm_dev_unplug() is what matches up with the read lock in drm_dev_enter().
> 
> > And that's not
> > the only thing I need actually. If there are 2 threads entering
> > panthor_device_unplug(), I need to make sure the one who losts (arrived
> > after unplugged was set to false) is waiting for all operations after
> > the drm_dev_unplug() call to be done, otherwise we might return from
> > platform_driver->remove() before the unplug cleanups are done, and
> > there might still be threads/workqueues accessing device resources
> > while/after they get released by the device-model.
> 
> I can't figure out how to do this other than adding a new atomic status
> bit into panthor. So something like:
> 
> 	if (!drm_dev_enter())
> 		/* Already unplugged */
> 		return;
> 
> 	if (atomic_cmpxchg(&unplugging, false, true)) {
> 		/* Racing with another thread */
> 		drm_dev_exit();
> 		/* Wait for other threads to exit */
> 		synchronize_srcu(&drm_unplug_srcu);
> 		return;
> 	}
> 
> 	panthor_xxx_unplug()
> 
> 	drm_dev_exit();
> 
> Or at least I think that might work. The need to synchronize with
> drm_unplug_srcu means this really needs a new helper in drm_drv.c.
> 
> >>
> >>> +	if (drm_dev_is_unplugged(&ptdev->base))
> >>> +		return;
> >>> +
> >>> +	drm_WARN_ON(&ptdev->base, pm_runtime_get_sync(ptdev->base.dev) < 0);
> >>> +
> >>> +	/* Call drm_dev_unplug() so any access to HW block happening after
> >>> +	 * that point get rejected.
> >>> +	 */
> >>> +	drm_dev_unplug(&ptdev->base);
> >>> +
> >>> +	/* Now, try to cleanly shutdown the GPU before the device resources
> >>> +	 * get reclaimed.
> >>> +	 */
> >>> +	panthor_sched_unplug(ptdev);
> >>> +	panthor_fw_unplug(ptdev);
> >>> +	panthor_mmu_unplug(ptdev);
> >>> +	panthor_gpu_unplug(ptdev);
> >>> +
> >>> +	pm_runtime_dont_use_autosuspend(ptdev->base.dev);
> >>> +	pm_runtime_put_sync_suspend(ptdev->base.dev);
> >>> +}
> >>> +
> >>> +static void panthor_device_reset_cleanup(struct drm_device *ddev, void *data)
> >>> +{
> >>> +	struct panthor_device *ptdev = container_of(ddev, struct panthor_device, base);
> >>> +
> >>> +	cancel_work_sync(&ptdev->reset.work);
> >>> +	destroy_workqueue(ptdev->reset.wq);
> >>> +}
> >>> +
> >>> +static void panthor_device_reset_work(struct work_struct *work)
> >>> +{
> >>> +	struct panthor_device *ptdev = container_of(work, struct panthor_device, reset.work);
> >>> +	int ret, cookie;
> >>> +
> >>> +	if (!drm_dev_enter(&ptdev->base, &cookie))
> >>> +		return;
> >>> +
> >>> +	panthor_sched_pre_reset(ptdev);
> >>> +	panthor_fw_pre_reset(ptdev, true);
> >>> +	panthor_mmu_pre_reset(ptdev);
> >>> +	panthor_gpu_soft_reset(ptdev);
> >>> +	panthor_gpu_l2_power_on(ptdev);
> >>> +	panthor_mmu_post_reset(ptdev);
> >>> +	ret = panthor_fw_post_reset(ptdev);
> >>> +	if (ret)
> >>> +		goto out;
> >>> +
> >>> +	atomic_set(&ptdev->reset.pending, 0);
> >>> +	panthor_sched_post_reset(ptdev);
> >>> +	drm_dev_exit(cookie);
> >>> +
> >>> +out:
> >>> +	if (ret) {  
> >>
> >> This looks like a race condition too - is there a need for a
> >> drm_dev_exit_and_unplug() function?
> > 
> > drm_dev_exit() is just releasing the read-lock. drm_dev_unplug()
> > waits for all readers to be done and sets the unplugged value to true.
> > So we only get readers/writer synchronization here, but nothing doing
> > writer/writer sync. I guess the drm core leaves that to drivers, given
> > drm_dev_unplug() is usually called from xxx_driver->remove() hook, on
> > which serialization is guaranteed by the device-model.
> > 
> > TLDR; yes, it's racy, but I don't think drm_dev_exit_and_unplug() would
> > help solve the existing race.
> 
> Yeah, I hadn't really thought through the reader/writer locks.
> 
> > It's worth noting that we currently have only 2 paths calling
> > panthor_device_unplug(): the platform_driver->remove() hook and the
> > reset worker. Calling drm_dev_unplug() might not be the right thing to
> > do, I just thought it was a good match to reflect the fact the device
> > becomes inaccessible, without adding yet another kind of device-lost
> > field.
> 
> I quite liked the unplugged approach, it hides the complexities of the
> GPU breaking nicely.
> 
> However I do think this path needs fixing in some way, because of the
> "goto out" we end up calling panthor_device_unplug() while in the
> drm_dev_enter() section. Which, unless I'm mistaken, means
> panthor_device_unplug() will call drm_dev_unplug() in that section -
> which should produce a lockdep warning at the very least, if not an
> actual deadlock.
> 
> Given it's only a read lock - I think simply moving drm_dev_exit() below
> the "out:" label fixes the deadlock without making any races worse.
> Whether the race here actually matters I'm not sure.
> 
> >>
> >>> +		panthor_device_unplug(ptdev);
> >>> +		drm_err(&ptdev->base, "Failed to boot MCU after reset, making device unusable.");
> >>> +	}
> >>> +}
> >>> +
> >>> +static bool panthor_device_is_initialized(struct panthor_device *ptdev)
> >>> +{
> >>> +	return !!ptdev->scheduler;
> >>> +}
> >>> +
> >>> +static void panthor_device_free_page(struct drm_device *ddev, void *data)
> >>> +{
> >>> +	free_page((unsigned long)data);
> >>> +}
> >>> +
> >>> +int panthor_device_init(struct panthor_device *ptdev)
> >>> +{
> >>> +	struct resource *res;
> >>> +	struct page *p;
> >>> +	int ret;
> >>> +
> >>> +	ptdev->coherent = device_get_dma_attr(ptdev->base.dev) == DEV_DMA_COHERENT;
> >>> +
> >>> +	drmm_mutex_init(&ptdev->base, &ptdev->pm.lock);
> >>> +	atomic_set(&ptdev->pm.state, PANTHOR_DEVICE_PM_STATE_SUSPENDED);
> >>> +	p = alloc_page(GFP_KERNEL | __GFP_ZERO);
> >>> +	if (!p)
> >>> +		return -ENOMEM;
> >>> +
> >>> +	ptdev->pm.dummy_latest_flush = page_address(p);
> >>> +	ret = drmm_add_action_or_reset(&ptdev->base, panthor_device_free_page,
> >>> +				       ptdev->pm.dummy_latest_flush);
> >>> +	if (ret)
> >>> +		return ret;
> >>> +
> >>> +	/* Set the dummy page to the default LATEST_FLUSH value. This
> >>> +	 * will be updated on the next suspend.
> >>> +	 */
> >>> +	*ptdev->pm.dummy_latest_flush = CSF_GPU_LATEST_FLUSH_ID_DEFAULT;  
> >>
> >> I see why this register default value was defined. Although I'm not sure
> >> it has any benefit over just using zero... If the GPU is off when user
> >> space reads the FLUSH_ID then the GPU's caches are definitely empty so
> >> any flush ID is valid.
> > 
> > Zero means we'll force a cache flush for all CS that were created while
> > the device was suspended, that's not ideal.
> > 
> >>
> >> Interestingly looking at kbase it seems to use an initial value of 1
> >> (POWER_DOWN_LATEST_FLUSH_VALUE). I guess zero is less ideal because
> >> FLUSH_CACHE2 would then unconditionally do a flush.
> > 
> > I guess a value of 1 would work. It just means we'll get a spurious
> > flush if the CS is submitted after 32 flushes happened, on the other
> > hand we also a spurious flush on the first submitted CS when we use
> > POWER_DOWN_LATEST_FLUSH_VALUE. I'll switch to 1, drop the default def,
> > and update the comment accordingly.
> 
> Yeah, matching kbase is almost certainly the safest approach ;) Sorry, I
> was reviewing the patches mostly in order and this looked really odd
> until I started digging into it. Zero is clearly not the ideal value,
> but the reset value is also just a weird value for hardware validation
> (it enables easier checking of the wrap condition). Since kbase picks 1,
> that must be a value which works well!
> 
> >>
> >>> +
> >>> +	INIT_WORK(&ptdev->reset.work, panthor_device_reset_work);
> >>> +	ptdev->reset.wq = alloc_ordered_workqueue("panthor-reset-wq", 0);
> >>> +	if (!ptdev->reset.wq)
> >>> +		return -ENOMEM;
> >>> +
> >>> +	ret = drmm_add_action_or_reset(&ptdev->base, panthor_device_reset_cleanup, NULL);
> >>> +	if (ret)
> >>> +		return ret;
> >>> +
> >>> +	ret = panthor_clk_init(ptdev);
> >>> +	if (ret)
> >>> +		return ret;
> >>> +
> >>> +	ret = panthor_devfreq_init(ptdev);
> >>> +	if (ret)
> >>> +		return ret;
> >>> +
> >>> +	ptdev->iomem = devm_platform_get_and_ioremap_resource(to_platform_device(ptdev->base.dev),
> >>> +							      0, &res);
> >>> +	if (IS_ERR(ptdev->iomem))
> >>> +		return PTR_ERR(ptdev->iomem);
> >>> +
> >>> +	ptdev->phys_addr = res->start;
> >>> +
> >>> +	ret = devm_pm_runtime_enable(ptdev->base.dev);
> >>> +	if (ret)
> >>> +		return ret;
> >>> +
> >>> +	ret = pm_runtime_resume_and_get(ptdev->base.dev);
> >>> +	if (ret)
> >>> +		return ret;
> >>> +
> >>> +	ret = panthor_gpu_init(ptdev);
> >>> +	if (ret)
> >>> +		goto err_rpm_put;
> >>> +
> >>> +	ret = panthor_mmu_init(ptdev);
> >>> +	if (ret)
> >>> +		goto err_rpm_put;
> >>> +
> >>> +	ret = panthor_fw_init(ptdev);
> >>> +	if (ret)
> >>> +		goto err_rpm_put;
> >>> +
> >>> +	ret = panthor_sched_init(ptdev);
> >>> +	if (ret)
> >>> +		goto err_rpm_put;
> >>> +
> >>> +	/* ~3 frames */
> >>> +	pm_runtime_set_autosuspend_delay(ptdev->base.dev, 50);
> >>> +	pm_runtime_use_autosuspend(ptdev->base.dev);
> >>> +	pm_runtime_put_autosuspend(ptdev->base.dev);
> >>> +	return 0;
> >>> +
> >>> +err_rpm_put:
> >>> +	pm_runtime_put_sync_suspend(ptdev->base.dev);
> >>> +	return ret;
> >>> +}
> >>> +
> >>> +#define PANTHOR_EXCEPTION(id) \
> >>> +	[DRM_PANTHOR_EXCEPTION_ ## id] = { \
> >>> +		.name = #id, \
> >>> +	}
> >>> +
> >>> +struct panthor_exception_info {
> >>> +	const char *name;
> >>> +};
> >>> +
> >>> +static const struct panthor_exception_info panthor_exception_infos[] = {
> >>> +	PANTHOR_EXCEPTION(OK),
> >>> +	PANTHOR_EXCEPTION(TERMINATED),
> >>> +	PANTHOR_EXCEPTION(KABOOM),
> >>> +	PANTHOR_EXCEPTION(EUREKA),
> >>> +	PANTHOR_EXCEPTION(ACTIVE),
> >>> +	PANTHOR_EXCEPTION(CS_RES_TERM),
> >>> +	PANTHOR_EXCEPTION(CS_CONFIG_FAULT),
> >>> +	PANTHOR_EXCEPTION(CS_ENDPOINT_FAULT),
> >>> +	PANTHOR_EXCEPTION(CS_BUS_FAULT),
> >>> +	PANTHOR_EXCEPTION(CS_INSTR_INVALID),
> >>> +	PANTHOR_EXCEPTION(CS_CALL_STACK_OVERFLOW),
> >>> +	PANTHOR_EXCEPTION(CS_INHERIT_FAULT),
> >>> +	PANTHOR_EXCEPTION(INSTR_INVALID_PC),
> >>> +	PANTHOR_EXCEPTION(INSTR_INVALID_ENC),
> >>> +	PANTHOR_EXCEPTION(INSTR_BARRIER_FAULT),
> >>> +	PANTHOR_EXCEPTION(DATA_INVALID_FAULT),
> >>> +	PANTHOR_EXCEPTION(TILE_RANGE_FAULT),
> >>> +	PANTHOR_EXCEPTION(ADDR_RANGE_FAULT),
> >>> +	PANTHOR_EXCEPTION(IMPRECISE_FAULT),
> >>> +	PANTHOR_EXCEPTION(OOM),
> >>> +	PANTHOR_EXCEPTION(CSF_FW_INTERNAL_ERROR),
> >>> +	PANTHOR_EXCEPTION(CSF_RES_EVICTION_TIMEOUT),
> >>> +	PANTHOR_EXCEPTION(GPU_BUS_FAULT),
> >>> +	PANTHOR_EXCEPTION(GPU_SHAREABILITY_FAULT),
> >>> +	PANTHOR_EXCEPTION(SYS_SHAREABILITY_FAULT),
> >>> +	PANTHOR_EXCEPTION(GPU_CACHEABILITY_FAULT),
> >>> +	PANTHOR_EXCEPTION(TRANSLATION_FAULT_0),
> >>> +	PANTHOR_EXCEPTION(TRANSLATION_FAULT_1),
> >>> +	PANTHOR_EXCEPTION(TRANSLATION_FAULT_2),
> >>> +	PANTHOR_EXCEPTION(TRANSLATION_FAULT_3),
> >>> +	PANTHOR_EXCEPTION(TRANSLATION_FAULT_4),
> >>> +	PANTHOR_EXCEPTION(PERM_FAULT_0),
> >>> +	PANTHOR_EXCEPTION(PERM_FAULT_1),
> >>> +	PANTHOR_EXCEPTION(PERM_FAULT_2),
> >>> +	PANTHOR_EXCEPTION(PERM_FAULT_3),
> >>> +	PANTHOR_EXCEPTION(ACCESS_FLAG_1),
> >>> +	PANTHOR_EXCEPTION(ACCESS_FLAG_2),
> >>> +	PANTHOR_EXCEPTION(ACCESS_FLAG_3),
> >>> +	PANTHOR_EXCEPTION(ADDR_SIZE_FAULT_IN),
> >>> +	PANTHOR_EXCEPTION(ADDR_SIZE_FAULT_OUT0),
> >>> +	PANTHOR_EXCEPTION(ADDR_SIZE_FAULT_OUT1),
> >>> +	PANTHOR_EXCEPTION(ADDR_SIZE_FAULT_OUT2),
> >>> +	PANTHOR_EXCEPTION(ADDR_SIZE_FAULT_OUT3),
> >>> +	PANTHOR_EXCEPTION(MEM_ATTR_FAULT_0),
> >>> +	PANTHOR_EXCEPTION(MEM_ATTR_FAULT_1),
> >>> +	PANTHOR_EXCEPTION(MEM_ATTR_FAULT_2),
> >>> +	PANTHOR_EXCEPTION(MEM_ATTR_FAULT_3),
> >>> +};
> >>> +
> >>> +const char *panthor_exception_name(struct panthor_device *ptdev, u32 exception_code)
> >>> +{
> >>> +	if (drm_WARN_ON(&ptdev->base,  
> >>
> >> I'm not convinced this should be a WARN_ON as I suspect it's probably
> >> possible to inject values from user space (although I'm not completely
> >> sure on that).
> > 
> > Normally no (it's something returned by the FW), unless userspace gets
> > access to the kernel <-> FW interface, which would be worrisome :-).
> 
> I've no idea if it's actually possible, but it feels like it should be
> possible to create a firmware synchronisation object with an error code
> chosen by the user and possibly then propagate that error code back to
> the kernel. It's certainly not trivial though. Either way the WARN is
> unnecessary.
> 
> >> It's certainly not a driver error as such if we can't
> >> decode the value.
> > 
> > Ack on dropping the WARN_ON().
> > 
> >>
> >>> +			exception_code >= ARRAY_SIZE(panthor_exception_infos) ||
> >>> +			!panthor_exception_infos[exception_code].name))
> >>> +		return "Unknown exception type";
> >>> +
> >>> +	return panthor_exception_infos[exception_code].name;
> >>> +}
> >>> +
> >>> +static vm_fault_t panthor_mmio_vm_fault(struct vm_fault *vmf)
> >>> +{
> >>> +	struct vm_area_struct *vma = vmf->vma;
> >>> +	struct panthor_device *ptdev = vma->vm_private_data;
> >>> +	u64 id = vma->vm_pgoff << PAGE_SHIFT;
> >>> +	unsigned long pfn;
> >>> +	pgprot_t pgprot;
> >>> +	vm_fault_t ret;
> >>> +	bool active;
> >>> +	int cookie;
> >>> +
> >>> +	if (!drm_dev_enter(&ptdev->base, &cookie))
> >>> +		return VM_FAULT_SIGBUS;
> >>> +
> >>> +	mutex_lock(&ptdev->pm.lock);
> >>> +	active = atomic_read(&ptdev->pm.state) == PANTHOR_DEVICE_PM_STATE_ACTIVE;
> >>> +
> >>> +	switch (id) {
> >>> +	case DRM_PANTHOR_USER_FLUSH_ID_MMIO_OFFSET:
> >>> +		if (active)
> >>> +			pfn = __phys_to_pfn(ptdev->phys_addr + CSF_GPU_LATEST_FLUSH_ID);
> >>> +		else
> >>> +			pfn = virt_to_pfn(ptdev->pm.dummy_latest_flush);
> >>> +		break;
> >>> +
> >>> +	default:
> >>> +		ret = VM_FAULT_SIGBUS;
> >>> +		goto out_unlock;
> >>> +	}
> >>> +
> >>> +	pgprot = vma->vm_page_prot;
> >>> +	if (active)
> >>> +		pgprot = pgprot_noncached(pgprot);
> >>> +
> >>> +	ret = vmf_insert_pfn_prot(vma, vmf->address, pfn, pgprot);
> >>> +
> >>> +out_unlock:
> >>> +	mutex_unlock(&ptdev->pm.lock);
> >>> +	drm_dev_exit(cookie);
> >>> +	return ret;
> >>> +}
> >>> +
> >>> +static const struct vm_operations_struct panthor_mmio_vm_ops = {
> >>> +	.fault = panthor_mmio_vm_fault,
> >>> +};
> >>> +
> >>> +int panthor_device_mmap_io(struct panthor_device *ptdev, struct vm_area_struct *vma)
> >>> +{
> >>> +	u64 id = vma->vm_pgoff << PAGE_SHIFT;
> >>> +
> >>> +	switch (id) {
> >>> +	case DRM_PANTHOR_USER_FLUSH_ID_MMIO_OFFSET:
> >>> +		if (vma->vm_end - vma->vm_start != PAGE_SIZE ||
> >>> +		    (vma->vm_flags & (VM_WRITE | VM_EXEC)))
> >>> +			return -EINVAL;
> >>> +
> >>> +		break;
> >>> +
> >>> +	default:
> >>> +		return -EINVAL;
> >>> +	}
> >>> +
> >>> +	/* Defer actual mapping to the fault handler. */
> >>> +	vma->vm_private_data = ptdev;
> >>> +	vma->vm_ops = &panthor_mmio_vm_ops;
> >>> +	vm_flags_set(vma,
> >>> +		     VM_IO | VM_DONTCOPY | VM_DONTEXPAND |
> >>> +		     VM_NORESERVE | VM_DONTDUMP | VM_PFNMAP);
> >>> +	return 0;
> >>> +}
> >>> +
> >>> +#ifdef CONFIG_PM
> >>> +int panthor_device_resume(struct device *dev)
> >>> +{
> >>> +	struct panthor_device *ptdev = dev_get_drvdata(dev);
> >>> +	int ret, cookie;
> >>> +
> >>> +	mutex_lock(&ptdev->pm.lock);
> >>> +	atomic_set(&ptdev->pm.state, PANTHOR_DEVICE_PM_STATE_RESUMING);
> >>> +
> >>> +	ret = clk_prepare_enable(ptdev->clks.core);
> >>> +	if (ret)
> >>> +		goto err_unlock;
> >>> +
> >>> +	ret = clk_prepare_enable(ptdev->clks.stacks);
> >>> +	if (ret)
> >>> +		goto err_disable_core_clk;
> >>> +
> >>> +	ret = clk_prepare_enable(ptdev->clks.coregroup);
> >>> +	if (ret)
> >>> +		goto err_disable_stacks_clk;
> >>> +
> >>> +	ret = panthor_devfreq_resume(ptdev);
> >>> +	if (ret)
> >>> +		goto err_disable_coregroup_clk;
> >>> +
> >>> +	if (panthor_device_is_initialized(ptdev) &&
> >>> +	    drm_dev_enter(&ptdev->base, &cookie)) {
> >>> +		panthor_gpu_resume(ptdev);
> >>> +		panthor_mmu_resume(ptdev);
> >>> +		ret = drm_WARN_ON(&ptdev->base, panthor_fw_resume(ptdev));
> >>> +		if (!ret)
> >>> +			panthor_sched_resume(ptdev);
> >>> +
> >>> +		drm_dev_exit(cookie);
> >>> +
> >>> +		if (ret)
> >>> +			goto err_devfreq_suspend;
> >>> +	}
> >>> +
> >>> +	/* Clear all IOMEM mappings pointing to this device after we've
> >>> +	 * resumed. This way the fake mappings pointing to the dummy pages
> >>> +	 * are removed and the real iomem mapping will be restored on next
> >>> +	 * access.
> >>> +	 */
> >>> +	unmap_mapping_range(ptdev->base.anon_inode->i_mapping,
> >>> +			    DRM_PANTHOR_USER_MMIO_OFFSET, 0, 1);
> >>> +	atomic_set(&ptdev->pm.state, PANTHOR_DEVICE_PM_STATE_ACTIVE);  
> >>
> >> Is the ordering here correct? I think we need to set ACTIVE before the
> >> unmap_mapping_range otherwise there is a (very small) race where user
> >> space could fault the page (and get the dummy mapping) before the
> >> atomic_set.
> > 
> > We take the pm.lock in panthor_mmio_vm_fault().
> > 
> >>
> >> Hmm, actually we have the pm.lock, so no this isn't racy. In which case
> >> is there a good reason that you're using atomics? I can see two accesses
> >> which aren't protected by pm.lock:
> >>
> >>   * the early out in panthor_device_suspend() - which could easily be
> >> moved inside the lock.
> > 
> > When we're in suspend() we are the one in control of the pm.state, so
> > no race expected here.
> > 
> >>
> >>   * panthor_device_schedule_reset() - this looks racy (the power down
> >> could happen immediately after the atomic_read()), so I suspect it would
> >> be better moving the check into panthor_device_reset_work() and
> >> performing it with the pm.lock held.
> > 
> > I think the main reason for it being an atomic is because I didn't
> > have PM locking in the initial implementation, but I ended adding
> > locking at some point because I didn't really have choice. I thought
> > the race didn't exist because of the workqueue synchronization/work
> > cancellation that happens in panthor_sched_suspend(), but I see now
> > that it's not protecting us (thread queuing the job could be paused
> > just after checking the PM state and resumed after the suspend
> > happened). This being said, we might have a lock ordering issue if we
> > take the PM lock in that path (I need to check that).
> 
> Yeah I didn't bother to check whether it would create ordering issues...
> ;) I'll leave you to figure out the fix - the whole atomic + mutex was
> confusing and doesn't seem to have quite worked.
> 
> [...]
> 
> >>> +
> >>> +/**
> >>> + * PANTHOR_IRQ_HANDLER() - Define interrupt handlers and the interrupt
> >>> + * registration function.
> >>> + *
> >>> + * The boiler-plate to gracefully deal with shared interrupts is
> >>> + * auto-generated. All you have to do is call PANTHOR_IRQ_HANDLER()
> >>> + * just after you actual handler. The handler prototype is:  
> >> s/you/your/ or probably s/you/the/ since we don't expect people to be
> >> adding more ;)
> >>
> >>> + *
> >>> + * void (*handler)(struct panthor_device *, u32 status);
> >>> + */
> >>> +#define PANTHOR_IRQ_HANDLER(__name, __reg_prefix, __handler)					\
> >>> +static irqreturn_t panthor_ ## __name ## _irq_raw_handler(int irq, void *data)			\
> >>> +{												\
> >>> +	struct panthor_irq *pirq = data;							\
> >>> +	struct panthor_device *ptdev = pirq->ptdev;						\  
> >>
> >> Maybe I'm missing something, but I was expecting a check here for if the
> >> irq has been suspended and to avoid the register reads if it was.
> > 
> > Thought the INT_MASK=0 + synchronize_irq() in panthor_xxx_irq_suspend()
> > would guarantee that the handler can't be called after
> > panthor_xxx_irq_suspend() was called.
> 
> If the IRQ is shared then Linux doesn't know which device caused the
> interrupt, so another device's (shared) interrupt could cause our
> handler to be run.
> 
> >> Otherwise I'm not entirely sure I follow what all this code is for.
> > 
> > Not entirely sure which code we're talking about. The reason we
> > don't use the default raw IRQ handler is because it doesn't work if the
> > irq line is shared. In that case, we need to mask all interrupts to
> > make sure other handlers on the same irq line don't get spammed with
> > our IRQs.
> 
> What I'm not following is why we need all this extra infrastructure for
> IRQs. The 'setting the mask to 0' during suspend is simple enough and
> could be included in code which now calls panthor_xxx_irq_suspend()
> (equally for restoring the mask on resume). But there's a loads more
> code here.
> 
> My initial thought when I looked at this was that you were trying to
> solve the issue of a shared IRQ where Mali might get powered off, but
> the IRQ is then triggered by another device. In that case touching the
> Mali registers would be problematic, so I was expecting some code in
> _irq_raw_handler() to check whether the IRQ couldn't possibly be for us
> (i.e. mask==0) and early out with IRQ_NONE. kbase has a concept like
> this "gpu_powered" for exactly this reason.

There is also support for Juno setups where all the GPU IRQs are muxed into
a single interrupt out of the FPGAs, so you need to share the line between
blocks. I have initially suggested the generic approach to Boris over a
freedesktop MR, but even that one did not handle the case where you would
share the interrupt with another device (who would want to do that anyway,
right? :) )

Best regards,
Liviu

> 
> But I can't see anything in the code to handle that case. And the
> "spamming" of other drivers during suspend shouldn't really happen
> (there's something odd going on if the hardware is generating interrupts
> when it's meant to be suspended).
> 
> But maybe I'm just missing something - it's a while since I've dealt
> with interrupt code in Linux.
> 
> Steve
> 
> >>
> >> Steve
> >>
> >>> +												\
> >>> +	if (!gpu_read(ptdev, __reg_prefix ## _INT_STAT))					\
> >>> +		return IRQ_NONE;								\
> >>> +												\
> >>> +	gpu_write(ptdev, __reg_prefix ## _INT_MASK, 0);						\
> >>> +	return IRQ_WAKE_THREAD;									\
> >>> +}												\
> >>> +												\
> >>> +static irqreturn_t panthor_ ## __name ## _irq_threaded_handler(int irq, void *data)		\
> >>> +{												\
> >>> +	struct panthor_irq *pirq = data;							\
> >>> +	struct panthor_device *ptdev = pirq->ptdev;						\
> >>> +	irqreturn_t ret = IRQ_NONE;								\
> >>> +												\
> >>> +	while (true) {										\
> >>> +		u32 status = gpu_read(ptdev, __reg_prefix ## _INT_RAWSTAT) & pirq->mask;	\
> >>> +												\
> >>> +		if (!status)									\
> >>> +			break;									\
> >>> +												\
> >>> +		gpu_write(ptdev, __reg_prefix ## _INT_CLEAR, status);				\
> >>> +												\
> >>> +		__handler(ptdev, status);							\
> >>> +		ret = IRQ_HANDLED;								\
> >>> +	}											\
> >>> +												\
> >>> +	if (!atomic_read(&pirq->suspended))							\
> >>> +		gpu_write(ptdev, __reg_prefix ## _INT_MASK, pirq->mask);			\
> >>> +												\
> >>> +	return ret;										\
> >>> +}												\
> >>> +												\
> >>> +static inline void panthor_ ## __name ## _irq_suspend(struct panthor_irq *pirq)			\
> >>> +{												\
> >>> +	int cookie;										\
> >>> +												\
> >>> +	atomic_set(&pirq->suspended, true);							\
> >>> +												\
> >>> +	if (drm_dev_enter(&pirq->ptdev->base, &cookie)) {					\
> >>> +		gpu_write(pirq->ptdev, __reg_prefix ## _INT_MASK, 0);				\
> >>> +		synchronize_irq(pirq->irq);							\
> >>> +		drm_dev_exit(cookie);								\
> >>> +	}											\
> >>> +												\
> >>> +	pirq->mask = 0;										\
> >>> +}												\
> >>> +												\
> >>> +static inline void panthor_ ## __name ## _irq_resume(struct panthor_irq *pirq, u32 mask)	\
> >>> +{												\
> >>> +	int cookie;										\
> >>> +												\
> >>> +	atomic_set(&pirq->suspended, false);							\
> >>> +	pirq->mask = mask;									\
> >>> +												\
> >>> +	if (drm_dev_enter(&pirq->ptdev->base, &cookie)) {					\
> >>> +		gpu_write(pirq->ptdev, __reg_prefix ## _INT_CLEAR, mask);			\
> >>> +		gpu_write(pirq->ptdev, __reg_prefix ## _INT_MASK, mask);			\
> >>> +		drm_dev_exit(cookie);								\
> >>> +	}											\
> >>> +}												\
> >>> +												\
> >>> +static int panthor_request_ ## __name ## _irq(struct panthor_device *ptdev,			\
> >>> +					      struct panthor_irq *pirq,				\
> >>> +					      int irq, u32 mask)				\
> >>> +{												\
> >>> +	pirq->ptdev = ptdev;									\
> >>> +	pirq->irq = irq;									\
> >>> +	panthor_ ## __name ## _irq_resume(pirq, mask);						\
> >>> +												\
> >>> +	return devm_request_threaded_irq(ptdev->base.dev, irq,					\
> >>> +					 panthor_ ## __name ## _irq_raw_handler,		\
> >>> +					 panthor_ ## __name ## _irq_threaded_handler,		\
> >>> +					 IRQF_SHARED, KBUILD_MODNAME "-" # __name,		\
> >>> +					 pirq);							\
> >>> +}
> >>> +
> >>> +extern struct workqueue_struct *panthor_cleanup_wq;
> >>> +
> >>> +#endif  
> >>
> > 
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯



[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux