Re: [PATCH v2 2/9] vfio/igd: align generation with i915 kernel driver

["Corvin Köhne" <C.Koehne@beckhoff.com>]

[-- Attachment #1: Type: text/plain, Size: 3561 bytes --]

On Tue, 2024-12-03 at 21:35 +0800, Tomita Moeko wrote:
> CAUTION: External Email!!
> 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.
> 
> The condition of how GTT stolen memory size is calculated is changed
> accordingly as GGMS is in multiple of 2 starting from gen 8.
> 
> Signed-off-by: Tomita Moeko <tomitamoeko@gmail.com>
> ---
>  hw/vfio/igd.c | 44 ++++++++++++++++++++++----------------------
>  1 file changed, 22 insertions(+), 22 deletions(-)
> 
> diff --git a/hw/vfio/igd.c b/hw/vfio/igd.c
> index 6ba3045bf3..2ede72d243 100644
> --- a/hw/vfio/igd.c
> +++ b/hw/vfio/igd.c
> @@ -59,33 +59,33 @@
>   */
>  static int igd_gen(VFIOPCIDevice *vdev)
>  {
> -    if ((vdev->device_id & 0xfff) == 0xa84) {
> -        return 8; /* Broxton */
> +    /*
> +     * Device IDs for Broxton/Apollo Lake are 0x0a84, 0x1a84, 0x1a85, 0x5a84
> +     * and 0x5a85
> +     */
> +    if ((vdev->device_id & 0xffe) == 0xa84) {
> +        return 9;
>      }
>  

I'd slightly prefer matching those ids explicitly. At some point it may even
make more sense to copy the pciids of Linux and reuse them [1].

Note that this is just a suggestion!

[1]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/drm/intel/i915_pciids.h?h=v6.12

>      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;
>      }
>  
> @@ -258,7 +258,7 @@ static int vfio_igd_gtt_max(VFIOPCIDevice *vdev)
>  
>      gmch = vfio_pci_read_config(&vdev->pdev, IGD_GMCH, sizeof(gmch));
>      ggms = (gmch >> (gen < 8 ? 8 : 6)) & 0x3;
> -    if (gen > 6) {
> +    if (gen >= 8) {
>          ggms = 1 << ggms;
>      }
>  
> @@ -668,7 +668,7 @@ void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int
> nr)
>  
>      /* Determine the size of stolen memory needed for GTT */
>      ggms_mb = (gmch >> (gen < 8 ? 8 : 6)) & 0x3;
> -    if (gen > 6) {
> +    if (gen >= 8) {
>          ggms_mb = 1 << ggms_mb;
>      }
>  

Reviewed-by: Corvin Köhne <c.koehne@beckhoff.com>

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]