On Thu, 5 Nov 2020 13:27:04 +0000 Steven Price steven.price@arm.com wrote:
- old_status = atomic_xchg(&queue->status,
PANFROST_QUEUE_STATUS_STOPPED);- WARN_ON(old_status != PANFROST_QUEUE_STATUS_ACTIVE &&
old_status != PANFROST_QUEUE_STATUS_STOPPED);- if (old_status == PANFROST_QUEUE_STATUS_STOPPED)
goto out;NIT: It's slightly cleaner if you swap the above lines, i.e.:
if (old_status == PANFROST_QUEUE_STATUS_STOPPED) goto out; WARN_ON(old_status != PANFROST_QUEUE_STATUS_ACTIVE);
I agree.
- drm_sched_stop(&queue->sched, bad);
- if (bad)
drm_sched_increase_karma(bad);- stopped = true;
- /*
* Set the timeout to max so the timer doesn't get started* when we return from the timeout handler (restored in* panfrost_scheduler_start()).*/- queue->sched.timeout = MAX_SCHEDULE_TIMEOUT;
+out: mutex_unlock(&queue->lock); return stopped; } +static void panfrost_scheduler_start(struct panfrost_queue_state *queue) +{
- enum panfrost_queue_status old_status;
- mutex_lock(&queue->lock);
- old_status = atomic_xchg(&queue->status,
PANFROST_QUEUE_STATUS_STARTING);- if (WARN_ON(old_status != PANFROST_QUEUE_STATUS_STOPPED))
goto out;The error handling isn't great here - in this case the queue status is left in _STATUS_STARTING, which at best would lead to another WARN_ON being hit, but also has the effect of ignoring job faults. Probably the timeout would eventually get things back to normal.
Obviously this situation will never occur™, but we can do better either by continuing with the normal logic below, or even better replacing atomic_xchg() with an atomic_cmpxchg() (so leave the status alone if not _STOPPED). Both seem like better error recovery options to me. But keep the WARN_ON because something has clearly gone wrong if this happens.
The second approach doesn't unblock things if we end up with old_status != STOPPED and the queue is really stopped (which shouldn't happen, unless we have a problem in our state machine). I think I'll go for the first option and restart the queue unconditionally (I'm keeping the WARN_ON(), of course).