Re: [PATCH v3 10/14] drm/panthor: Add the scheduler logical block

[Ketil Johnsen <ketil.johnsen@arm.com>]

On 04/12/2023 18:33, Boris Brezillon wrote:

<snip>

> +/**
> + * cs_slot_sync_queue_state_locked() - Synchronize the queue slot priority
> + * @ptdev: Device.
> + * @csg_id: Group slot.
> + * @cs_id: Queue slot.
> + *
> + * Queue state is updated on group suspend or STATUS_UPDATE event.
> + */
> +static void
> +cs_slot_sync_queue_state_locked(struct panthor_device *ptdev, u32 csg_id, u32 cs_id)
> +{
> +	struct panthor_group *group = ptdev->scheduler->csg_slots[csg_id].group;
> +	struct panthor_queue *queue = group->queues[cs_id];
> +	struct panthor_fw_cs_iface *cs_iface =
> +		panthor_fw_get_cs_iface(group->ptdev, csg_id, cs_id);
> +
> +	u32 status_wait_cond;
> +
> +	switch (cs_iface->output->status_blocked_reason) {
> +	case CS_STATUS_BLOCKED_REASON_UNBLOCKED:
> +		if (queue->iface.input->insert == queue->iface.output->extract &&
> +		    cs_iface->output->status_scoreboards == 0)
> +			group->idle_queues |= BIT(cs_id);
> +		break;
> +
> +	case CS_STATUS_BLOCKED_REASON_SYNC_WAIT:
> +		drm_WARN_ON(&ptdev->base, !list_empty(&group->wait_node));

I think we should remove this drm_WARN_ON(). With my user submission 
experiments, I keep hitting this warning because I'm a bit slow to 
signal a Mali sync object. In other words; I'm keeping a stream blocked 
for a while.

It is quite common to get two rapid job IRQs, e.g. one for a global 
event, and one for a particular CSG event. Depending on timing of the 
scheduled work to deal with the IRQs, I quite often end up with two 
tick_work() being scheduled and executed as a result of this. Both of 
these will see the same stream as CS_STATUS_BLOCKED_REASON_UNBLOCKED, 
and hence the second will trigger the drm_WARN_ON(), as the first run 
already added the group to the waiting list.

I'm pretty sure we can hit this drm_WARN_ON() when user space starts 
making use of multiple streams pr group as well, since two or more 
streams for the same group could both be 
CS_STATUS_BLOCKED_REASON_SYNC_WAIT, thus running into the same issue.

> +		list_move_tail(&group->wait_node, &group->ptdev->scheduler->groups.waiting);
> +		group->blocked_queues |= BIT(cs_id);
> +		queue->syncwait.gpu_va = cs_iface->output->status_wait_sync_ptr;
> +		queue->syncwait.ref = cs_iface->output->status_wait_sync_value;
> +		status_wait_cond = cs_iface->output->status_wait & CS_STATUS_WAIT_SYNC_COND_MASK;
> +		queue->syncwait.gt = status_wait_cond == CS_STATUS_WAIT_SYNC_COND_GT;
> +		if (cs_iface->output->status_wait & CS_STATUS_WAIT_SYNC_64B) {
> +			u64 sync_val_hi = cs_iface->output->status_wait_sync_value_hi;
> +
> +			queue->syncwait.sync64 = true;
> +			queue->syncwait.ref |= sync_val_hi << 32;
> +		} else {
> +			queue->syncwait.sync64 = false;
> +		}
> +		break;
> +
> +	default:
> +		/* Other reasons are not blocking. Consider the queue as runnable
> +		 * in those cases.
> +		 */
> +		break;
> +	}
> +}

<snip>

--
Thanks,
Ketil