Re: [PATCH v4 1/2] iommu/arm-smmu-v3: Add a cmdq_entries module parameter

On Thu, Sep 03, 2026 at 08:09:55AM -0700, Nicolin Chen wrote:

On Thu, Sep 03, 2026 at 03:15:27PM +0100, Kiryl Shutsemau wrote:

quoted

On Wed, Sep 02, 2026 at 10:22:31AM -0700, Nicolin Chen wrote:

quoted

On Wed, Sep 02, 2026 at 01:17:23PM +0100, Kiryl Shutsemau (Meta) wrote:

quoted

+/**
+ * arm_smmu_queue_max_n_shift() - pick the log2 depth of a queue
+ * @hw_shift: log2 depth the hardware allows, capped for natural alignment
+ * @ent_sz_shift: log2 of the queue entry size in bytes
+ * @want: number of entries asked for, or zero to use @hw_shift
+ *
+ * @want is rounded down to a power of two. It never sizes a queue below one
+ * page, because coherent DMA is page granular: a shallower queue occupies the
+ * same memory as one that fills the page, and arm_smmu_init_one_queue() stops
+ * shrinking at a page too.

Well, since we want the cmdq depth, why not name the parameter
"max_cmdq_depth"? Then, no rounding and more straightforward.

The max_ prefix is better, thanks. cmdq_entries does not say it is a
ceiling.

On the units, the page floor has to stay whatever we call the parameter.

Ask for 8 entries and max_n_shift comes out as 3, which the
CMDQ_BATCH_ENTRIES check in arm_smmu_device_hw_probe() rejects with
-ENXIO. The floor gives you 256 entries on a 4K kernel instead of a failed
probe.

So log2 removes the rounding but not the flooring, and the number asked for
still is not always the number you get. I would rather keep entries than
make people convert.

The rounding is not silent, either: arm_smmu_init_one_queue() already
prints "allocated %u entries for cmdq", so the effective depth lands in
dmesg whichever unit the parameter takes.

On the name itself, "depth" reads to me as a number of entries rather than
its log2, so max_cmdq_depth=8 meaning 256 seems more surprising than the
rounding it replaces.

Well, maybe "cmdq_max_n_shift" is a better one over "depth".

Given CMDQ_BATCH_ENTRIES is used, I think cmdq_max_entries works.

cmdq_max_entries it is. Renamed for the next version.

-- 
  Kiryl Shutsemau / Kirill A. Shutemov