On 2/9/22 7:32 AM, Christoph Hellwig wrote:

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)); }

I am not sure the suggestion is correct. That's the reason why I didn't take the comments in the previous review.

if mmio is u32 *, the step of mmio[0] -> mmio[1] will be 4, and i will be increased by 4 in each loop. I guess the correct one would be mmio[i/4] = xxxxx; would that looks better? if yes, I will do that in the next version.

...

+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;

Yes. You are right. It's because I get rid of the mmio_block in intel_gvt.c

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?

We actually have one in i915, but it's not exported. Should we export that one?

Thanks, Zhi.