Re: [PATCH 4/8] vfio/igd: align generation with i915 kernel driver

[Alex Williamson]

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