On 2025-06-06 7:28 am, Tomeu Vizoso wrote: [...]

diff --git a/drivers/accel/rocket/rocket_device.h b/drivers/accel/rocket/rocket_device.h index 10acfe8534f00a7985d40a93f4b2f7f69d43caee..50e46f0516bd1615b5f826c5002a6c0ecbf9aed4 100644 --- a/drivers/accel/rocket/rocket_device.h +++ b/drivers/accel/rocket/rocket_device.h @@ -13,6 +13,8 @@ struct rocket_device { struct drm_device ddev;

• struct mutex sched_lock;
• struct mutex iommu_lock;

Just realised I missed this in the last patch, but iommu_lock appears to be completely unnecessary now.

struct rocket_core *cores;

[...]

+static void rocket_job_hw_submit(struct rocket_core *core, struct rocket_job *job) +{

• struct rocket_task *task;
• bool task_pp_en = 1;
• bool task_count = 1;
• /* GO ! */
• /* Don't queue the job if a reset is in progress */
• if (atomic_read(&core->reset.pending))

• return;
  

• task = &job->tasks[job->next_task_idx];
• job->next_task_idx++;
• rocket_pc_writel(core, BASE_ADDRESS, 0x1);
• rocket_cna_writel(core, S_POINTER, 0xe + 0x10000000 * core->index);
• rocket_core_writel(core, S_POINTER, 0xe + 0x10000000 * core->index);

Those really look like bitfield operations rather than actual arithmetic to me.

• rocket_pc_writel(core, BASE_ADDRESS, task->regcmd);

I don't see how regcmd is created (I guess that's in userspace?), but given that it's explicitly u64 all the way through - and especially since you claim to support 40-bit DMA addresses - it definitely seems suspicious that the upper 32 bits never seem to be consumed anywhere :/

• rocket_pc_writel(core, REGISTER_AMOUNTS, (task->regcmd_count + 1) / 2 - 1);
• rocket_pc_writel(core, INTERRUPT_MASK, PC_INTERRUPT_MASK_DPU_0 | PC_INTERRUPT_MASK_DPU_1);
• rocket_pc_writel(core, INTERRUPT_CLEAR, PC_INTERRUPT_CLEAR_DPU_0 | PC_INTERRUPT_CLEAR_DPU_1);
• rocket_pc_writel(core, TASK_CON, ((0x6 | task_pp_en) << 12) | task_count);
• rocket_pc_writel(core, TASK_DMA_BASE_ADDR, 0x0);
• rocket_pc_writel(core, OPERATION_ENABLE, 0x1);
• dev_dbg(core->dev, "Submitted regcmd at 0x%llx to core %d", task->regcmd, core->index);

+}

[...]

+static struct dma_fence *rocket_job_run(struct drm_sched_job *sched_job) +{

• struct rocket_job *job = to_rocket_job(sched_job);
• struct rocket_device *rdev = job->rdev;
• struct rocket_core *core = sched_to_core(rdev, sched_job->sched);
• struct dma_fence *fence = NULL;
• int ret;
• if (unlikely(job->base.s_fence->finished.error))

• return NULL;
  

• /*

• * Nothing to execute: can happen if the job has finished while
  

• * we were resetting the GPU.
  

GPU? (Similarly in various other comments/prints)

• */
  

• if (job->next_task_idx == job->task_count)

• return NULL;
  

• fence = rocket_fence_create(core);
• if (IS_ERR(fence))

• return fence;
  

• if (job->done_fence)

• dma_fence_put(job->done_fence);
  

• job->done_fence = dma_fence_get(fence);
• ret = pm_runtime_get_sync(core->dev);
• if (ret < 0)

• return fence;
  

• ret = iommu_attach_group(job->domain, iommu_group_get(core->dev));

I don't see iommu_group_put() anywhere, so you're leaking refcounts all over.

• if (ret < 0)

• return fence;
  

• scoped_guard(spinlock, &core->job_lock) {

• core->in_flight_job = job;
  

• rocket_job_hw_submit(core, job);
  

• }
• return fence;

+}

[...]

+static void rocket_job_handle_irq(struct rocket_core *core) +{

• u32 status, raw_status;
• pm_runtime_mark_last_busy(core->dev);
• status = rocket_pc_readl(core, INTERRUPT_STATUS);
• raw_status = rocket_pc_readl(core, INTERRUPT_RAW_STATUS);
• rocket_pc_writel(core, OPERATION_ENABLE, 0x0);
• rocket_pc_writel(core, INTERRUPT_CLEAR, 0x1ffff);

What was the point of reading the status registers if we're just going to blindly clear every possible condition anyway?

• scoped_guard(spinlock, &core->job_lock)

• if (core->in_flight_job)
  

• 	rocket_job_handle_done(core, core->in_flight_job);
  

But then is it really OK to just start the next task regardless of whether the current task was reporting successful completion or an error?

+}

+static void +rocket_reset(struct rocket_core *core, struct drm_sched_job *bad) +{

• bool cookie;
• if (!atomic_read(&core->reset.pending))

• return;
  

• /*

• * Stop the scheduler.
  

• *
  

• * FIXME: We temporarily get out of the dma_fence_signalling section
  

• * because the cleanup path generate lockdep splats when taking locks
  

• * to release job resources. We should rework the code to follow this
  

• * pattern:
  

• *
  

• *	try_lock
  

• *	if (locked)
  

• *		release
  

• *	else
  

• *		schedule_work_to_release_later
  

• */
  

• drm_sched_stop(&core->sched, bad);
• cookie = dma_fence_begin_signalling();
• if (bad)

• drm_sched_increase_karma(bad);
  

• /*

• * Mask job interrupts and synchronize to make sure we won't be
  

• * interrupted during our reset.
  

• */
  

• rocket_pc_writel(core, INTERRUPT_MASK, 0x0);
• synchronize_irq(core->irq);

...except it's a shared IRQ, so it can still merrily fire at any time.

• /* Handle the remaining interrupts before we reset. */
• rocket_job_handle_irq(core);
• /*

• * Remaining interrupts have been handled, but we might still have
  

• * stuck jobs. Let's make sure the PM counters stay balanced by
  

• * manually calling pm_runtime_put_noidle() and
  

• * rocket_devfreq_record_idle() for each stuck job.
  

• * Let's also make sure the cycle counting register's refcnt is
  

• * kept balanced to prevent it from running forever
  

Comments that don't match the code are more confusing than helpful :/

• */
  

• scoped_guard(spinlock, &core->job_lock) {

• if (core->in_flight_job)
  

• 	pm_runtime_put_noidle(core->dev);
  

• core->in_flight_job = NULL;
  

• }
• /* Proceed with reset now. */
• pm_runtime_force_suspend(core->dev);
• pm_runtime_force_resume(core->dev);

Can you guarantee that actually resets the hardware if something else is holding the power domain open or RPM is disabled? I'm not familiar with the details of drm_sched, but if there are other jobs queued behind the stuck one would it even pass the rocket_job_is_idle() check for suspend to succeed anyway?

Not to mention that you have an actual reset control in the DT binding, which isn't even optional... :/

• /* GPU has been reset, we can clear the reset pending bit. */
• atomic_set(&core->reset.pending, 0);
• /*

• * Now resubmit jobs that were previously queued but didn't have a
  

• * chance to finish.
  

• * FIXME: We temporarily get out of the DMA fence signalling section
  

• * while resubmitting jobs because the job submission logic will
  

• * allocate memory with the GFP_KERNEL flag which can trigger memory
  

• * reclaim and exposes a lock ordering issue.
  

• */
  

• dma_fence_end_signalling(cookie);
• drm_sched_resubmit_jobs(&core->sched);

Since I happened to look, this says it's deprecated?

• cookie = dma_fence_begin_signalling();
• /* Restart the scheduler */
• drm_sched_start(&core->sched, 0);
• dma_fence_end_signalling(cookie);

+}

+static enum drm_gpu_sched_stat rocket_job_timedout(struct drm_sched_job *sched_job) +{

• struct rocket_job *job = to_rocket_job(sched_job);
• struct rocket_device *rdev = job->rdev;
• struct rocket_core *core = sched_to_core(rdev, sched_job->sched);
• /*

• * If the GPU managed to complete this jobs fence, the timeout is
  

• * spurious. Bail out.
  

• */
  

• if (dma_fence_is_signaled(job->done_fence))

• return DRM_GPU_SCHED_STAT_NOMINAL;
  

Do we really need the same return condition twice? What if the IRQ fires immediately after we've made this check, and is handled without delay such that sychronize_irq() effectively still does nothing? Either way we've taken longer than the timeout value to observe the job completing successfully, and either that's significant and worth warning about or it's not - I don't see any point in trying to (inaccurately) nitpick *why* it might have happened.

• /*

• * Rocket IRQ handler may take a long time to process an interrupt
  

• * if there is another IRQ handler hogging the processing.
  

• * For example, the HDMI encoder driver might be stuck in the IRQ
  

• * handler for a significant time in a case of bad cable connection.
  

What have HDMI cables got to do with anything here? Yes, in general IRQ latency can be high, since CPUs can have IRQs masked and/or be taking higher-priority interrupts for any number of reasons. I don't see how an oddly-specific example (of apparently poor driver design, to boot) is useful.

• * In order to catch such cases and not report spurious rocket
  

• * job timeouts, synchronize the IRQ handler and re-check the fence
  

• * status.
  

• */
  

• synchronize_irq(core->irq);
• if (dma_fence_is_signaled(job->done_fence)) {

• dev_warn(core->dev, "unexpectedly high interrupt latency\n");
  

• return DRM_GPU_SCHED_STAT_NOMINAL;
  

• }
• dev_err(core->dev, "gpu sched timeout");
• atomic_set(&core->reset.pending, 1);
• rocket_reset(core, sched_job);
• iommu_detach_group(NULL, iommu_group_get(core->dev));
• return DRM_GPU_SCHED_STAT_NOMINAL;

+}

+static void rocket_reset_work(struct work_struct *work) +{

• struct rocket_core *core;
• core = container_of(work, struct rocket_core, reset.work);
• rocket_reset(core, NULL);

+}

+static const struct drm_sched_backend_ops rocket_sched_ops = {

• .run_job = rocket_job_run,
• .timedout_job = rocket_job_timedout,
• .free_job = rocket_job_free

+};

+static irqreturn_t rocket_job_irq_handler_thread(int irq, void *data) +{

• struct rocket_core *core = data;
• rocket_job_handle_irq(core);
• return IRQ_HANDLED;

+}

+static irqreturn_t rocket_job_irq_handler(int irq, void *data) +{

• struct rocket_core *core = data;
• u32 raw_status = rocket_pc_readl(core, INTERRUPT_RAW_STATUS);

Given that this can be a shared IRQ as above, it would be a good idea to take care to avoid register accesses while suspended. Especially if you're trying to utilise suspend to reset a failing job that may well be throwing IOMMU faults.

• WARN_ON(raw_status & PC_INTERRUPT_RAW_STATUS_DMA_READ_ERROR);
• WARN_ON(raw_status & PC_INTERRUPT_RAW_STATUS_DMA_READ_ERROR);
• if (!(raw_status & PC_INTERRUPT_RAW_STATUS_DPU_0 ||

•      raw_status & PC_INTERRUPT_RAW_STATUS_DPU_1))
  

• return IRQ_NONE;
  

• rocket_pc_writel(core, INTERRUPT_MASK, 0x0);
• return IRQ_WAKE_THREAD;

+}

+int rocket_job_init(struct rocket_core *core) +{

• struct drm_sched_init_args args = {

• .ops = &rocket_sched_ops,
  

• .num_rqs = DRM_SCHED_PRIORITY_COUNT,
  

• .credit_limit = 1,
  

Ah, does this mean that all the stuff about queued jobs was in fact all nonsense anyway?

• .timeout = msecs_to_jiffies(JOB_TIMEOUT_MS),
  

• .name = dev_name(core->dev),
  

• .dev = core->dev,
  

• };
• int ret;
• INIT_WORK(&core->reset.work, rocket_reset_work);
• spin_lock_init(&core->job_lock);
• core->irq = platform_get_irq(to_platform_device(core->dev), 0);
• if (core->irq < 0)

• return core->irq;
  

• ret = devm_request_threaded_irq(core->dev, core->irq,

• 			rocket_job_irq_handler,
  

• 			rocket_job_irq_handler_thread,
  

• 			IRQF_SHARED, KBUILD_MODNAME "-job",
  

Is it really a "job" interrupt though? The binding and the register definitions suggest it's just a general status interrupt for the core. Furthermore since we expect to have multiple cores, being able to more easily identify and attribute per-core IRQ activity seems more useful for debugging than copy-pasting from something really rather different which also expects to be the only one of its kind on the system.

Thanks, Robin.

• 			core);
  

• if (ret) {

• dev_err(core->dev, "failed to request job irq");
  

• return ret;
  

• }