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

[Boris Brezillon]

On Thu, 8 Feb 2024 16:18:48 +0000
Liviu Dudau <Liviu.Dudau at arm.com> wrote:

> On Thu, Feb 08, 2024 at 05:00:23PM +0100, Boris Brezillon wrote:
> > On Thu, 8 Feb 2024 15:55:36 +0000
> > Liviu Dudau <Liviu.Dudau at arm.com> wrote:
> >   
> > > On Thu, Feb 08, 2024 at 04:14:59PM +0100, Boris Brezillon wrote:  
> > > > On Thu, 8 Feb 2024 14:30:02 +0000
> > > > Liviu Dudau <Liviu.Dudau at arm.com> wrote:
> > > >     
> > > > > > +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;
> > > > > > +
> > > > > > +	init_completion(&ptdev->unplug.done);
> > > > > > +	ret = drmm_mutex_init(&ptdev->base, &ptdev->unplug.lock);
> > > > > > +	if (ret)
> > > > > > +		return ret;
> > > > > > +
> > > > > > +	ret = drmm_mutex_init(&ptdev->base, &ptdev->pm.mmio_lock);
> > > > > > +	if (ret)
> > > > > > +		return ret;
> > > > > > +
> > > > > > +	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 holding the latest flush to 1. This will cause the
> > > > > > +	 * flush to avoided as we know it isn't necessary if the submission
> > > > > > +	 * happens while the dummy page is mapped. Zero cannot be used because
> > > > > > +	 * that means 'always flush'.
> > > > > > +	 */
> > > > > > +	*ptdev->pm.dummy_latest_flush = 1;
> > > > > > +
> > > > > > +	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_unplug_gpu;
> > > > > > +
> > > > > > +	ret = panthor_fw_init(ptdev);
> > > > > > +	if (ret)
> > > > > > +		goto err_unplug_mmu;
> > > > > > +
> > > > > > +	ret = panthor_sched_init(ptdev);
> > > > > > +	if (ret)
> > > > > > +		goto err_unplug_fw;
> > > > > > +
> > > > > > +	/* ~3 frames */
> > > > > > +	pm_runtime_set_autosuspend_delay(ptdev->base.dev, 50);
> > > > > > +
> > > > > > +	ret = drm_dev_register(&ptdev->base, 0);
> > > > > > +	if (ret)
> > > > > > +		goto err_unplug_sched;      
> > > > > 
> > > > > For sake of replicating the panthor_device_unplus() calls, should we do
> > > > > here:
> > > > > 
> > > > > 	if (ret) {
> > > > > 		pm_runtime_dont_use_autosuspend(ptdev->base.dev);    
> > > > 
> > > > But pm_runtime_use_autosuspend() is called after that, why do we need
> > > > to call pm_runtime_dont_use_autosuspend() here?    
> > > 
> > > This is in the case where ret != 0, so we're going to skip over
> > > pm_runtime_use_autosuspend(). We've just called
> > > pm_runtime_set_autosuspend_delay() which by my reading also enables
> > > runtime PM when it calls update_autosuspend(), so this is needed.  
> > 
> > That's not how I understand it. To me,
> > pm_runtime_set_autosuspend_delay() just updates the delay, but doesn't
> > change the autosuspend status, and update_autosuspend() doesn't seem to
> > change it either.  
> 
> That depends on how you interpret the vague: "If it changes the other way
> [read: not negative delay and (or?) power.use_autosuspend set], allow runtime
> suspends".
> 
> The "else" branch in update_autosuspend() is taken when either delay >= 0 *or*
> dev->power.use_autosuspend is zero. rpm_idle() will then be called regardless
> of the values for old_delay and old_use.
> 
> Maybe we have uncovered a bug?

Nah, you're right, but that's confusing. I thought by moving
pm_runtime_set_autosuspend_delay() before the drm_dev_register() we
wouldn't change the autosuspend state, and at the same time preset a
default delay that can be changed by userland as soon as the device is
exposed (when drm_dev_register() is called). If we can't do that,
there's no point splitting the set_autosuspend_delay() and
use_autosuspend() calls, we can just move them before
drm_dev_register() and call dont_use_autosuspend() is something fails
(as you suggested).