On 25/06/2021 14:33, Boris Brezillon wrote:
This is not yet needed because we let active jobs be killed during by the reset and we don't really bother making sure they can be restarted. But once we start adding soft-stop support, controlling when we deal with the remaining interrrupts and making sure those are handled before the reset is issued gets tricky if we keep job interrupts active.
Let's prepare for that and mask+flush job IRQs before issuing a reset.
Signed-off-by: Boris Brezillon boris.brezillon@collabora.com
drivers/gpu/drm/panfrost/panfrost_job.c | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c index 88d34fd781e8..0566e2f7e84a 100644 --- a/drivers/gpu/drm/panfrost/panfrost_job.c +++ b/drivers/gpu/drm/panfrost/panfrost_job.c @@ -34,6 +34,7 @@ struct panfrost_queue_state { struct panfrost_job_slot { struct panfrost_queue_state queue[NUM_JOB_SLOTS]; spinlock_t job_lock;
- int irq;
};
static struct panfrost_job * @@ -400,7 +401,15 @@ static void panfrost_reset(struct panfrost_device *pfdev, if (bad) drm_sched_increase_karma(bad);
- spin_lock(&pfdev->js->job_lock);
I'm not sure it's safe to remove this lock as this protects the pfdev->jobs array: I can't see what would prevent panfrost_job_close() running at the same time without the lock. Am I missing something?
- /* Mask job interrupts and synchronize to make sure we won't be
* interrupted during our reset.*/- job_write(pfdev, JOB_INT_MASK, 0);
- synchronize_irq(pfdev->js->irq);
- /* Schedulers are stopped and interrupts are masked+flushed, we don't
* need to protect the 'evict unfinished jobs' lock with the job_lock.*/@@ -408,7 +417,6 @@ static void panfrost_reset(struct panfrost_device *pfdev, pfdev->jobs[i] = NULL; } }
spin_unlock(&pfdev->js->job_lock);
panfrost_device_reset(pfdev);
@@ -504,6 +512,7 @@ static void panfrost_job_handle_irq(struct panfrost_device *pfdev, u32 status)
job = pfdev->jobs[j]; /* Only NULL if job timeout occurred */
WARN_ON(!job);
Was this WARN_ON intentional?
Steve
if (job) { pfdev->jobs[j] = NULL;@@ -563,7 +572,7 @@ static void panfrost_reset_work(struct work_struct *work) int panfrost_job_init(struct panfrost_device *pfdev) { struct panfrost_job_slot *js;
- int ret, j, irq;
int ret, j;
INIT_WORK(&pfdev->reset.work, panfrost_reset_work);
@@ -573,11 +582,11 @@ int panfrost_job_init(struct panfrost_device *pfdev)
spin_lock_init(&js->job_lock);
- irq = platform_get_irq_byname(to_platform_device(pfdev->dev), "job");
- if (irq <= 0)
- js->irq = platform_get_irq_byname(to_platform_device(pfdev->dev), "job");
- if (js->irq <= 0) return -ENODEV;
- ret = devm_request_threaded_irq(pfdev->dev, irq,
- ret = devm_request_threaded_irq(pfdev->dev, js->irq, panfrost_job_irq_handler, panfrost_job_irq_handler_thread, IRQF_SHARED, KBUILD_MODNAME "-job",