On Tue, Feb 08, 2022 at 06:11:50AM -0500, Zhi Wang wrote: > + struct drm_i915_private *dev_priv = iter->i915; > + u32 *mmio, i; > + > + for (i = offset; i < offset + size; i += 4) { > + mmio = iter->data + i; > + *mmio = intel_uncore_read_notrace(to_gt(dev_priv)->uncore, > + _MMIO(i)); This reads much stranger than: u32 *mmio = iter->data; for (i = offset; i < offset + size; i += 4) { mmio[i] = intel_uncore_read_notrace(to_gt(dev_priv)->uncore, _MMIO(i)); } > +static int handle_mmio(struct intel_gvt_mmio_table_iter *iter, > + u32 offset, u32 device, u32 size) > +{ > + if (WARN_ON(!IS_ALIGNED(offset, 4))) > + return -EINVAL; Shouldn't this be in the caller of the method? > + save_mmio(iter, offset, size); > + return 0; Now that the block callback is gone save_mmio and handle_mmio can be merged. > + mem = vzalloc(2 * SZ_1M); Don't we want a driver-wide constant for this instead of a magic number?