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.
