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

On Tue, Feb 08, 2022 at 05:15:00PM +0200, Jani Nikula wrote:

...

...

#ifdef CONFIG_DRM_I915_GVT

+#define D_BDW (1 << 0) +#define D_SKL (1 << 1) +#define D_KBL (1 << 2) +#define D_BXT (1 << 3) +#define D_CFL (1 << 4)

+#define D_GEN9PLUS (D_SKL | D_KBL | D_BXT | D_CFL) +#define D_GEN8PLUS (D_BDW | D_SKL | D_KBL | D_BXT | D_CFL)

+#define D_SKL_PLUS (D_SKL | D_KBL | D_BXT | D_CFL) +#define D_BDW_PLUS (D_BDW | D_SKL | D_KBL | D_BXT | D_CFL)

+#define D_PRE_SKL (D_BDW) +#define D_ALL (D_BDW | D_SKL | D_KBL | D_BXT | D_CFL)

If these really need to be in a header in i915/, I think they need to be longer with some namespacing or something. I do wish these could be hidden though.

I think we could actually kill them off entirely. They are used as arguments to the macros that setup the mmio table.

Thefunctions to build these tabls are already organized by families, so we'd need relatively few conditions to just build them the right way. There also are some runtime checks in the callbacks, but they seem entirely superflous as far as I can tell.

Only the cmd parser is a bit messy. So maybe we could keep these constants just for the cmd parser inside of gvt for now (and clean that up later) and remove them entirely from the mmio table.

I agree that's the correct way for not exporting this to i915 by just organizing them in the functions, like what you said. But I guess it's also matter of time and schedule as well. If we go that direction, it might take longer time for coding as this is a big re-factor. Also, we need our QA to do another full test run. That needs to be considered. (If we are ok with that)

Besides, we have to have a methodology to make sure everything is the same as before. Currently I am comparing the numbers of tracked mmio and the mmio snapshot. It would be nice to have more insight. :)

Thanks, Zhi.