Re: [PATCH v6 2/3] i915/gvt: Save the initial HW state snapshot in i915

[Wang, Zhi A]

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.