Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
From:   "Wang, Zhi A" <zhi.a.wang@intel.com>
To:     Christoph Hellwig <hch@lst.de>, Zhi Wang <zhi.wang.linux@gmail.com>
CC:     "intel-gfx@lists.freedesktop.org" <intel-gfx@lists.freedesktop.org>,
        Joonas Lahtinen <joonas.lahtinen@linux.intel.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "jani.nikula@linux.intel.com" <jani.nikula@linux.intel.com>,
        Zhenyu Wang <zhenyuw@linux.intel.com>,
        "dri-devel@lists.freedesktop.org" <dri-devel@lists.freedesktop.org>,
        "jgg@nvidia.com" <jgg@nvidia.com>,
        "Vivi, Rodrigo" <rodrigo.vivi@intel.com>,
        "intel-gvt-dev@lists.freedesktop.org" 
        <intel-gvt-dev@lists.freedesktop.org>
Subject: Re: [PATCH v6 2/3] i915/gvt: Save the initial HW state snapshot in
 i915
Thread-Topic: [PATCH v6 2/3] i915/gvt: Save the initial HW state snapshot in
 i915
Thread-Index: AQHYHNy6NmrKnennPUe03qa1ybalWKyK1IeAgADOO4A=
Date:   Wed, 9 Feb 2022 19:50:09 +0000
Message-ID: <8b430166-7603-fb72-f9b0-257008750538@intel.com>
References: <20220208111151.13115-1-zhi.a.wang@intel.com>
 <20220208111151.13115-2-zhi.a.wang@intel.com> <20220209073201.GB9050@lst.de>
In-Reply-To: <20220209073201.GB9050@lst.de>
Accept-Language: en-FI, en-US
Content-Language: aa
user-agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.14.0
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?Windows-1252?Q?9TQbi60h/1Gr5Oinosv/ne1PaIM1QvIY6kvDKw/yKA3vJhg0J6dqwyda?=
 =?Windows-1252?Q?nHTO6zI3udBNgdXERgmh788gmumLwqHFozCUFizPaEe3yc3nai95Izmu?=
 =?Windows-1252?Q?MU/lehItNx0kLvETWyFND73KPiKeQ89aBr2hPKXSCqMFMPR9kDAmVzzW?=
 =?Windows-1252?Q?jGz9Sq7BEIcFqltqjp0C5WG8L4GIhraIxImwnHHTlAJDujDf/Pd8v7Ro?=
 =?Windows-1252?Q?pFuBHM0uLvSmitY/2RbrQoAnAeDwQxPGeivXw++sX3cMvjqyhct369ZX?=
 =?Windows-1252?Q?YcuL5z6ewVGnGRnorUeVQydH8/DtZbMyZjpM9aRkG5jhTqvCh7ek9Sr3?=
 =?Windows-1252?Q?X4TVw3jRiX+mpWalkd6E7cvkmYAWRtrPbhnmVY8wvqbxBY+O4UqXu3Mk?=
 =?Windows-1252?Q?vyPfpW2GR4FCI4OaIEafLPDMr3ZpDmhcxyNhUdZk29CrT6G5KY5n5iyI?=
 =?Windows-1252?Q?WLZP6j2IXe4XWog0wsjoJWc8PkUhVQ8u1ISxDSQbXMCn0jSPpvEWWyKm?=
 =?Windows-1252?Q?DIuIw/nPs6NWmCBADIFwpI48TbAFQ+4rRDh0EoofLFwaVehWXz+2YvaQ?=
 =?Windows-1252?Q?vKuV1kvDLzR9tJOrHOTKw1RxS4jt/LkdgVToUu+rRMNSrvWlTD+kBmXj?=
 =?Windows-1252?Q?qPwlFjwkzdeFl0cWbQCBZcyXXYeVqIQsAs46h2gsl7WCDndyr6n0n5TI?=
 =?Windows-1252?Q?btNpTbqvDzgXPg9a+WHQlRB9RnTOhl1wq6bR1xLBtzgrYFHxAL2PYi7h?=
 =?Windows-1252?Q?2SUCvlPl6tXD5RuSYt5p+bEOKQHKsLN7L7Zq5vxp5OX5UkG/PgbEeFDk?=
 =?Windows-1252?Q?T8dOhLzif6ArkS7Vp78mWMXEQYbiniWjLZLVllCyqL/1VyYCN59yuf2c?=
 =?Windows-1252?Q?oG2T6sxKaFu35bu9A7ZS4OezCorE37FDUVL6IwcMuNADKUjC3DcZ770i?=
 =?Windows-1252?Q?3wSBUvLjGXMcgZs7D9hPt/a0iGQkdw7TtDUkihCh8eBjb6PQgpS6jmyE?=
 =?Windows-1252?Q?zIky1aGzThggl/2ztoBuXgUsEkdk+A/o3t94IiBQCJuVQe1EaPt94rn5?=
 =?Windows-1252?Q?4RTU34pQ6wDRgAcu4/WBm27h6JYwMkGEdKqQecP/4NVfuZ48zZ1+8udC?=
 =?Windows-1252?Q?2yA+XGnN9eHRV9D1QLrk6u53N/rU52hIGkFDAZut96uSZWakH9e/tgWC?=
 =?Windows-1252?Q?rIqhUg/kp6Uity8/8M8vVX+bGrBCKNlIrJ+35pE36gd5Jo7ub1/D7L8q?=
 =?Windows-1252?Q?I/94UM31mrhwNDLj3bIKP+Ch7IJ2q9wvi3rHm0UUVag5e1cFcKZx7IPa?=
 =?Windows-1252?Q?tqf25t4ueCCbWc4gixo3YjAIcsYYesduZMCkyWk3vm5fZnsxmz5Vdx4U?=
 =?Windows-1252?Q?WxwO+dAlJAD5TACKvsBRIyaM+UXloODG2JG5pk6XHva2o+TyfH1DzghX?=
 =?Windows-1252?Q?hU2Umpr8rJ4qZXgw0bmTflaCyRwBeyUpyyFwyWIbAHE7LoHIVSfRS6Sa?=
 =?Windows-1252?Q?82rfK2Bt/JUoHOuu0mBzukeyfXOxl5sdPV2RR4N5115vH5SICfNn4dVh?=
 =?Windows-1252?Q?Vz+bPQTR8zzxQKsrHiFo0Ff88t46Dq07WGbM6MD9z2Y9CcQyb4xn5x8T?=
 =?Windows-1252?Q?PKcLRgUKrQQ2CIjasgzgQM67Bq5xnOf3y7S7p7NsF0HdI2qWqNMeqxM2?=
 =?Windows-1252?Q?eFAsPYPOirFQd+mH6XLEOqpuhN4tA4CAWvH5Sadiy4zOHfZssTp1kQ?=
 =?Windows-1252?Q?=3D=3D?=
Content-Type: text/plain; charset="Windows-1252"
Content-ID: <185100D7601EF74D876647A194A5E4E6@namprd11.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: DM4PR11MB5549.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: fb9870d4-049a-424e-2a8c-08d9ec056105
X-MS-Exchange-CrossTenant-originalarrivaltime: 09 Feb 2022 19:50:09.5574
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: MuM3HccZiETXvfkmMBbBRugBL1KkoekFqgLI38q/IjL3waAWsMfjhrTFqOROr5xjUsA4FyP/pw97rKy631F+UA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA2PR11MB4812
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-2.0 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
	DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,NICE_REPLY_A,RDNS_NONE,SPF_HELO_NONE,
	T_SCC_BODY_TEXT_LINE autolearn=no autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
	lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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 =3D iter->i915;
>> +	u32 *mmio, i;
>> +
>> +	for (i =3D offset; i < offset + size; i +=3D 4) {
>> +		mmio =3D iter->data + i;
>> +		*mmio =3D intel_uncore_read_notrace(to_gt(dev_priv)->uncore,
>> +						  _MMIO(i));
>=20
> This reads much stranger than:
>=20
> 	u32 *mmio =3D iter->data;
>=20
> 	for (i =3D offset; i < offset + size; i +=3D 4) {
> 		mmio[i] =3D intel_uncore_read_notrace(to_gt(dev_priv)->uncore,
> 						    _MMIO(i));
> 	}
>=20
I am not sure the suggestion is correct. That's the reason why I didn't tak=
e the comments in the previous review.

if mmio is u32 *, the step of mmio[0] -> mmio[1] will be 4, and i will be i=
ncreased by 4 in each loop.
I guess the correct one would be mmio[i/4] =3D xxxxx; would that looks bett=
er? 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;
>=20
> Shouldn't this be in the caller of the method?
>=20
>> +	save_mmio(iter, offset, size);
>> +	return 0;
>=20
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.
>=20
>> +	mem =3D vzalloc(2 * SZ_1M);
>=20
> Don't we want a driver-wide constant for this instead of a magic number?
>=20

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

Thanks,
Zhi.