Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] Persistent accumulator state causes spurious validation failures for subsequent operations. - [Medium] Unconditional accumulator format validation rejects valid command streams on U85. --
commit 1aba4b3d50bf7ffb97c4f8a708b27672c76cbb73 Author: Rob Herring (Arm) <[email protected]> accel: ethosu: Validate accumulator input The U85 ACC_FORMAT command can select IFM2 as the accumulator input. This is used by null-pool operations and can also be used by convolution. Track this selection and validate the IFM2 feature map against the OFM extent before submitting the operation. > diff --git a/drivers/accel/ethosu/ethosu_gem.c > b/drivers/accel/ethosu/ethosu_gem.c > index f4bd31018e564..632a2352491a0 100644 > --- a/drivers/accel/ethosu/ethosu_gem.c > +++ b/drivers/accel/ethosu/ethosu_gem.c [ ... ] > @@ -522,6 +523,32 @@ static int feat_matrix_size(struct ethosu_device *edev, > max_len); > } > > +static int > +calc_acc_input_size(struct drm_device *ddev, > + struct ethosu_validated_cmdstream_info *info, > + struct cmd_state *st) > +{ > + struct ethosu_device *edev = to_ethosu_device(ddev); > + u64 len; > + int ret; > + > + if (!ethosu_is_u65(edev) && > + !cmd_state_reg_is_set(st, NPU_SET_ACC_FORMAT)) > + return -EINVAL; [Severity: Medium] Does this unconditional check reject valid command streams on U85? If a user submits a U85 command stream containing only operations that do not use the accumulator (such as pure elementwise or resize operations), the compiler may optimize away the unnecessary NPU_SET_ACC_FORMAT register. By requiring it unconditionally here, the driver will reject these valid command streams with -EINVAL, preventing userspace from executing certain models. > + > + if (!st->acc_input_ifm2) > + return 0; > + > + /* The accumulator has one input value for each OFM element. */ > + ret = feat_matrix_size(edev, info, st, &st->ifm2, > + FEAT_MATRIX_IFM2, st->ofm.width, > + st->ofm.height[2], st->ofm.depth, false, &len); [Severity: High] Can this persistent state cause spurious validation failures for subsequent operations? When a user submits a command stream where an operation (like POOL) sets NPU_SET_ACC_FORMAT to use IFM2, st->acc_input_ifm2 becomes true. If a subsequent operation (like ELEMENTWISE or RESIZE) ignores the accumulator, the compiler does not emit a new format register, leaving st->acc_input_ifm2 as true. This will validate the previous operation's IFM2 state against the new operation's OFM dimensions in calc_acc_input_size(), leading to a dimension mismatch and artificially inflated memory requirements or an -EINVAL rejection. > + dev_dbg(ddev->dev, "ACC IFM2:%d:0x%llx-0x%llx\n", > + st->ifm2.region, st->ifm2.base[0], len); > + > + return ret; > +} > + [ ... ] > @@ -692,6 +722,9 @@ static int calc_sizes_elemwise(struct drm_device *ddev, > true, &len); > dev_dbg(ddev->dev, "op %d: OFM:%d:0x%llx-0x%llx\n", > op, st->ofm.region, st->ofm.base[0], len); > + if (ret) > + return ret; > + ret = calc_acc_input_size(ddev, info, st); [Severity: High] Since elementwise operations do not use the accumulator, does calling calc_acc_input_size() here inadvertently apply stale accumulator state from previous operations? > if (ret) > return ret; > if (!feat_matrix_chained(edev, &st->ofm)) -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=18
