| Message ID | 20241201160938.44355-5-tomitamoeko@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | vfio/igd: Enable legacy mode on more devices | expand |
On Mon, 2 Dec 2024 00:09:34 +0800 Tomita Moeko <tomitamoeko@gmail.com> wrote: > Define the igd device generations according to i915 kernel driver to > avoid confusion, and adjust comment placement to clearly reflect the > relationship between ids and devices. > > Signed-off-by: Tomita Moeko <tomitamoeko@gmail.com> > --- > hw/vfio/igd.c | 36 ++++++++++++++++-------------------- > 1 file changed, 16 insertions(+), 20 deletions(-) > > diff --git a/hw/vfio/igd.c b/hw/vfio/igd.c > index 8f300498e4..71342863d6 100644 > --- a/hw/vfio/igd.c > +++ b/hw/vfio/igd.c > @@ -59,33 +59,29 @@ > */ > static int igd_gen(VFIOPCIDevice *vdev) > { > - if ((vdev->device_id & 0xfff) == 0xa84) { > - return 8; /* Broxton */ > + if ((vdev->device_id & 0xffe) == 0xa84) { > + return 9; /* Broxton/Apollo Lake, Gemini Lake */ > } J4105 is a Gemini Lake, the IGD device ID is 0x3185 according to ark[1]?? I do find Apollo Lake examples (N3450) with ID 0x5a85, which I assume justifies masking out the zero bit of the ID. Since Broxton was apparently canceled, I think this unique device ID test was concocted from i915. Maybe Apollo Lake should just be added under gen9 below with 0x5a00? Then Gemini Lake should be added as 0x3100. Both should be noted as new IDs. Have either been tested? > > switch (vdev->device_id & 0xff00) { > - /* SandyBridge, IvyBridge, ValleyView, Haswell */ > - case 0x0100: > - case 0x0400: > - case 0x0a00: > - case 0x0c00: > - case 0x0d00: > - case 0x0f00: > + case 0x0100: /* SandyBridge, IvyBridge */ > return 6; > - /* BroadWell, CherryView, SkyLake, KabyLake */ > - case 0x1600: > - case 0x1900: > - case 0x2200: > - case 0x5900: > + case 0x0400: /* Haswell */ > + case 0x0a00: /* Haswell */ > + case 0x0c00: /* Haswell */ > + case 0x0d00: /* Haswell */ Crystal Well? > + case 0x0f00: /* Valleyview/Bay Trail */ > + return 7; > + case 0x1600: /* Broadwell */ > + case 0x2200: /* Cherryview */ These two jump from gen6/7 to gen8, that's certainly not just a cosmetic change. > return 8; > - /* CoffeeLake */ > - case 0x3e00: > + case 0x1900: /* Skylake */ > + case 0x5900: /* Kaby Lake */ These two jump from gen6/7 to gen9, so again, functional differences for these. Have these changes been tested? > + case 0x3e00: /* Coffee Lake */ > return 9; > - /* ElkhartLake */ > - case 0x4500: > + case 0x4500: /* Elkhart Lake */ > return 11; > - /* TigerLake */ > - case 0x9A00: > + case 0x9A00: /* Tiger Lake */ > return 12; > } > Actually, patch 1 doesn't even need to be addressed here because patch 2 already made the change to check (gen < 8) rather than (gen > 6), so we can just entirely drop patch 1 (ideally with an updated commit log in 2 to note the logic change for this upcoming split of gen6/7). Thanks, Alex [1]https://www.intel.com/content/www/us/en/products/sku/128989/intel-celeron-j4105-processor-4m-cache-up-to-2-50-ghz/specifications.html
On 12/2/24 14:04, Alex Williamson wrote: > On Mon, 2 Dec 2024 00:09:34 +0800 > Tomita Moeko <tomitamoeko@gmail.com> wrote: > >> Define the igd device generations according to i915 kernel driver to >> avoid confusion, and adjust comment placement to clearly reflect the >> relationship between ids and devices. >> >> Signed-off-by: Tomita Moeko <tomitamoeko@gmail.com> >> --- >> hw/vfio/igd.c | 36 ++++++++++++++++-------------------- >> 1 file changed, 16 insertions(+), 20 deletions(-) >> >> diff --git a/hw/vfio/igd.c b/hw/vfio/igd.c >> index 8f300498e4..71342863d6 100644 >> --- a/hw/vfio/igd.c >> +++ b/hw/vfio/igd.c >> @@ -59,33 +59,29 @@ >> */ >> static int igd_gen(VFIOPCIDevice *vdev) >> { >> - if ((vdev->device_id & 0xfff) == 0xa84) { >> - return 8; /* Broxton */ >> + if ((vdev->device_id & 0xffe) == 0xa84) { >> + return 9; /* Broxton/Apollo Lake, Gemini Lake */ >> } > > J4105 is a Gemini Lake, the IGD device ID is 0x3185 according to > ark[1]?? I made a mistake, Gemini Lake have device id 0x3184 and 0x3185, it won't match here. > I do find Apollo Lake examples (N3450) with ID 0x5a85, which I assume > justifies masking out the zero bit of the ID. > > Since Broxton was apparently canceled, I think this unique device ID > test was concocted from i915. Maybe Apollo Lake should just be added > under gen9 below with 0x5a00? Then Gemini Lake should be added as > 0x3100. Both should be noted as new IDs. Have either been tested? Broxton is the name for the GPU part in Apollo Lake https://www.intel.com/content/www/us/en/content-details/685496/2016-intel-atom-processors-celeron-processors-and-pentium-processors-based-on-the-apollo-lake-platform-broxton-graphics-programmer-s-reference-manual-preface.html Apollo lake have 5 ids, 0x0a84, 0x1a84, 0x1a85, 0x5a84, 0x5a85, the problem is 0x0a84, the prefix 0x0a00 is the same as Haswell, and their generation are different, the GGC register need to be emulated qemu are also different. Masking 0xffe here resolves it. Though I don't have a Gemini Lake device in hand, some proxmox users reported passthrough works. Giving that in old qemu releases, they were treated as gen 8 by default, and there is no difference in qemu for gen 8 and gen 9 device, I think it's okay to declare it as gen 9 here? https://forum.proxmox.com/threads/asrock-j4105-gpu-passthrough-vgabios-romfile.67507/ >> >> switch (vdev->device_id & 0xff00) { >> - /* SandyBridge, IvyBridge, ValleyView, Haswell */ >> - case 0x0100: >> - case 0x0400: >> - case 0x0a00: >> - case 0x0c00: >> - case 0x0d00: >> - case 0x0f00: >> + case 0x0100: /* SandyBridge, IvyBridge */ >> return 6; >> - /* BroadWell, CherryView, SkyLake, KabyLake */ >> - case 0x1600: >> - case 0x1900: >> - case 0x2200: >> - case 0x5900: >> + case 0x0400: /* Haswell */ >> + case 0x0a00: /* Haswell */ >> + case 0x0c00: /* Haswell */ >> + case 0x0d00: /* Haswell */ > > Crystal Well? All 4 are Haswell device id prefixes. https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/drm/intel/i915_pciids.h?h=v6.12#n187 >> + case 0x0f00: /* Valleyview/Bay Trail */ >> + return 7; >> + case 0x1600: /* Broadwell */ >> + case 0x2200: /* Cherryview */ > > These two jump from gen6/7 to gen8, that's certainly not just a cosmetic > change. Broadwell and Cherryview are gen 8 previously, no change here. https://gitlab.com/qemu-project/qemu/-/blob/v9.2.0-rc2/hw/vfio/igd.c#L86 >> return 8; >> - /* CoffeeLake */ >> - case 0x3e00: >> + case 0x1900: /* Skylake */ >> + case 0x5900: /* Kaby Lake */ > > These two jump from gen6/7 to gen9, so again, functional differences > for these. Have these changes been tested? > >> + case 0x3e00: /* Coffee Lake */ >> return 9; >> - /* ElkhartLake */ >> - case 0x4500: >> + case 0x4500: /* Elkhart Lake */ >> return 11; >> - /* TigerLake */ >> - case 0x9A00: >> + case 0x9A00: /* Tiger Lake */ >> return 12; >> } >> > > Actually, patch 1 doesn't even need to be addressed here because patch > 2 already made the change to check (gen < 8) rather than (gen > 6), so > we can just entirely drop patch 1 (ideally with an updated commit log > in 2 to note the logic change for this upcoming split of gen6/7). > Thanks, > > Alex Agreed, previous (gen > 6) actually means (gen >= 8), patch 1 is unnecessary. I am going to change the condition in this commit, and mention it in commit message. > [1]https://www.intel.com/content/www/us/en/products/sku/128989/intel-celeron-j4105-processor-4m-cache-up-to-2-50-ghz/specifications.html >
diff --git a/hw/vfio/igd.c b/hw/vfio/igd.c index 8f300498e4..71342863d6 100644 --- a/hw/vfio/igd.c +++ b/hw/vfio/igd.c @@ -59,33 +59,29 @@ */ static int igd_gen(VFIOPCIDevice *vdev) { - if ((vdev->device_id & 0xfff) == 0xa84) { - return 8; /* Broxton */ + if ((vdev->device_id & 0xffe) == 0xa84) { + return 9; /* Broxton/Apollo Lake, Gemini Lake */ } switch (vdev->device_id & 0xff00) { - /* SandyBridge, IvyBridge, ValleyView, Haswell */ - case 0x0100: - case 0x0400: - case 0x0a00: - case 0x0c00: - case 0x0d00: - case 0x0f00: + case 0x0100: /* SandyBridge, IvyBridge */ return 6; - /* BroadWell, CherryView, SkyLake, KabyLake */ - case 0x1600: - case 0x1900: - case 0x2200: - case 0x5900: + case 0x0400: /* Haswell */ + case 0x0a00: /* Haswell */ + case 0x0c00: /* Haswell */ + case 0x0d00: /* Haswell */ + case 0x0f00: /* Valleyview/Bay Trail */ + return 7; + case 0x1600: /* Broadwell */ + case 0x2200: /* Cherryview */ return 8; - /* CoffeeLake */ - case 0x3e00: + case 0x1900: /* Skylake */ + case 0x5900: /* Kaby Lake */ + case 0x3e00: /* Coffee Lake */ return 9; - /* ElkhartLake */ - case 0x4500: + case 0x4500: /* Elkhart Lake */ return 11; - /* TigerLake */ - case 0x9A00: + case 0x9A00: /* Tiger Lake */ return 12; }
Define the igd device generations according to i915 kernel driver to avoid confusion, and adjust comment placement to clearly reflect the relationship between ids and devices. Signed-off-by: Tomita Moeko <tomitamoeko@gmail.com> --- hw/vfio/igd.c | 36 ++++++++++++++++-------------------- 1 file changed, 16 insertions(+), 20 deletions(-)