Re: [PATCH v2 19/28] drm/panthor: Add AW assignment tracking

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [Medium] drm/panthor: race replaying pending AW requests in
  arbitration_resource_init()
- [Low] drm/panthor: remove extraneous semicolon after
  to_assigned_sched_idx()
- [Low] drm/panthor: initialize __free() variable in
  arbitration_dt_parse_assigned_aw()
--

commit e073b116d1f4f953a3ab51f1f483486ff4942f9f
Author: Karunika Choo <karunika.choo@arm.com>

drm/panthor: Add AW assignment tracking

This commit tracks the scheduler assigned to each access window, initializes
assignments from device tree partition masks, and replays requests that
arrive before access windows are assigned.

> diff --git a/drivers/gpu/drm/panthor/arbitration/panthor_arbitration_drv.c b/drivers/gpu/drm/panthor/arbitration/panthor_arbitration_drv.c
> index 48016d0d88557..6479a7602868c 100644
> --- a/drivers/gpu/drm/panthor/arbitration/panthor_arbitration_drv.c
> +++ b/drivers/gpu/drm/panthor/arbitration/panthor_arbitration_drv.c

[ ... ]

> @@ -17,6 +19,28 @@
>  
>  #define PANTHOR_PM_AUTOSUSPEND_DELAY_MS 100
>  
> +#define DT_ASSIGNED_AW				"arm,access-window-mask"
> +
> +static int to_assigned_sched_idx(struct panthor_arbitration *adev, u8 aw_id)
> +{
> +	int idx = -1;
> +
> +	lockdep_assert_held(&adev->assignment_lock);
> +
> +	if (aw_id >= AM_ARB_MAX_AW_COUNT) {
> +		dev_err(adev->dev, "Invalid AW ID (%u)", aw_id);
> +		return -EINVAL;
> +	}
> +
> +	idx = adev->aw_sched_map[aw_id];
> +	if (idx < 0 || idx >= AM_ARB_MAX_PC_COUNT || !adev->sched[idx]) {
> +		dev_warn_ratelimited(adev->dev, "AW%u not assigned to any sched", aw_id);
> +		return -ENOENT;
> +	}
> +
> +	return idx;
> +};

[Severity: Low]
This isn't a bug, but there is an extra semicolon after the closing brace of
to_assigned_sched_idx().

> @@ -57,42 +81,226 @@ static int panthor_arbitration_runtime_resume(struct device *dev)
>  	return 0;
>  }
>  
> +static u32 arbitration_get_sched_aw_mask(struct panthor_arbitration *adev,
> +					 u8 sched_id)
> +{
> +	u32 aw_mask = 0;
> +
> +	lockdep_assert_held(&adev->assignment_lock);
> +
> +	for (int i = 0; i < AM_ARB_MAX_AW_COUNT; i++) {
> +		if (adev->aw_sched_map[i] == sched_id)
> +			aw_mask |= BIT(i);
> +	}
> +
> +	return aw_mask;
> +}
> +
> +static int arbitration_dt_parse_assigned_aw(struct panthor_arbitration *adev)
> +{
> +	struct panthor_arbitration_dt_info *dt_info = &adev->dt_info;
> +	struct device *dev = adev->dev;
> +	struct fwnode_handle *partitions __free(fwnode_handle) = NULL;
> +	u32 seen_mask = 0;
> +	u8 seen_partitions = 0;
> +	int ret;
> +
> +	partitions = device_get_named_child_node(dev, "partitions");

[Severity: Low]
This isn't a bug, but per the Cleanup API conventions, partitions can be
defined and initialized in a single statement rather than initialized to
NULL and assigned separately.

[ ... ]

> +static void arbitration_retry_pending_requests(struct panthor_arbitration *adev, u32 mask)
> +{
> +	unsigned long aw_mask = mask;
> +	u8 aw_id;
> +
> +	for_each_set_bit(aw_id, &aw_mask, AM_ARB_MAX_AW_COUNT) {
> +		int ret;
> +
> +		if (!test_and_clear_bit(aw_id, adev->pending_aw_request_mask))
> +			continue;
> +
> +		ret = panthor_arbitration_on_request(adev, aw_id);
> +		if (ret && ret != -EEXIST)
> +			set_bit(aw_id, adev->pending_aw_request_mask);
> +	}
> +}
> +
> +static int arbitration_resource_init(struct panthor_arbitration *adev)
> +{
> +	struct panthor_arbitration_dt_info *dt_info = &adev->dt_info;
> +
> +	/*
> +	 * If arm,access-window-mask is not defined or set to 0, assign AWs to
> +	 * PC0 by default.
> +	 */
> +	if (!dt_info->default_aw_masks[0])
> +		dt_info->default_aw_masks[0] = AM_ARB_MAX_AW_MASK;
> +
> +	for (int i = 0; i < AM_ARB_MAX_PC_COUNT; i++) {
> +		unsigned long pc_aw_mask = dt_info->default_aw_masks[i];
> +		u8 aw_id;
> +		int ret;
> +
> +		if (!adev->pc[i] || !adev->sched[i])
> +			continue;
> +
> +		scoped_guard(spinlock_irqsave, &adev->assignment_lock) {
> +			for_each_set_bit(aw_id, &pc_aw_mask, AM_ARB_MAX_AW_COUNT)
> +				adev->aw_sched_map[aw_id] = i;
> +		}
> +
> +		ret = arbitration_sched_try_start(adev, i);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	arbitration_retry_pending_requests(adev, AM_ARB_MAX_AW_MASK);
> +
> +	return 0;
> +}
> +
>  /* AW to Arbiter events */
>  int panthor_arbitration_on_request(struct panthor_arbitration *adev, u8 aw_id)
>  {
> -	/* TODO: AW to PC assignment */
> -	return panthor_arbitration_sched_on_request(adev->sched[0], aw_id);
> +	int idx;
> +
> +	guard(spinlock_irqsave)(&adev->assignment_lock);
> +
> +	idx = to_assigned_sched_idx(adev, aw_id);
> +
> +	if (idx == -ENOENT) {
> +		set_bit(aw_id, adev->pending_aw_request_mask);
> +
> +		return 0;
> +	} else if (idx < 0) {
> +		return idx;
> +	}
> +
> +	return panthor_arbitration_sched_on_request(adev->sched[idx], aw_id);
>  }

[Severity: Medium]
Can a request interrupt arriving between publishing adev->aw_sched_map[] and
arbitration_retry_pending_requests() reorder or duplicate AW requests?

panthor_resource_group_init() enables Resource Group interrupts and starts
the AW handshake before arbitration_resource_init() runs.  If an AW sends
VM_ARB_GPU_REQUEST before adev->aw_sched_map[] is populated,
panthor_arbitration_on_request() sets its bit in pending_aw_request_mask.
arbitration_resource_init() then populates adev->aw_sched_map[], drops
assignment_lock, and starts the scheduler before draining
pending_aw_request_mask:

CPU0                                  <hardirq>
arbitration_resource_init()
                                      rg_irq_handler()
                                        ...
                                          panthor_arbitration_on_request(aw0)
                                            set_bit(0, pending_aw_request_mask)
  lock(&adev->assignment_lock)
  adev->aw_sched_map[aw_id] = i
  unlock(&adev->assignment_lock)
  arbitration_sched_try_start()
                                      rg_irq_handler()
                                        ...
                                          panthor_arbitration_on_request(aw0)
                                            panthor_arbitration_sched_on_request()
  arbitration_retry_pending_requests()
    test_and_clear_bit(0, ...)
    panthor_arbitration_on_request(aw0)
      panthor_arbitration_sched_on_request()  <-- duplicate replay

Because panthor_arbitration_on_request() does not clear aw_id in
pending_aw_request_mask when idx >= 0, a resent request (such as from
panthor_aw_handshake_handle() on ARB_VM_INIT) is queued once by the
interrupt handler and replayed a second time by
arbitration_retry_pending_requests(), triggering a spurious warning in
panthor_arbitration_sched_on_request() or an extra GPU grant if the AW has
already entered ARB_SCHED_PHASE_STOPPING or ARB_SCHED_PHASE_IDLE.  A request
from a different AW in that window also bypasses the earlier pending request
and is granted the GPU first.

Replaying pending requests under assignment_lock when populating
adev->aw_sched_map[] (before starting the scheduler), or clearing
pending_aw_request_mask in panthor_arbitration_on_request(), would avoid the
race.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110153907.3241978-1-karunika.choo@arm.com?part=19