| Message ID | 20241201160938.44355-2-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:31 +0800 Tomita Moeko <tomitamoeko@gmail.com> wrote: > Both intel documentation [1][2] and i915 driver shows GGMS represents > GTT stolen memory size in multiple of 1MB, not 2MB starting from gen 8. > > [1] https://www.intel.com/content/dam/www/public/us/en/documents/datasheets/3rd-gen-core-desktop-vol-2-datasheet.pdf > [2] https://www.intel.com/content/dam/www/public/us/en/documents/datasheets/4th-gen-core-family-desktop-vol-2-datasheet.pdf > > Signed-off-by: Tomita Moeko <tomitamoeko@gmail.com> > --- > hw/vfio/igd.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/hw/vfio/igd.c b/hw/vfio/igd.c > index 4047f4f071..e40e601026 100644 > --- a/hw/vfio/igd.c > +++ b/hw/vfio/igd.c > @@ -268,7 +268,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 > 7) { > ggms = 1 << ggms; > } > > @@ -678,7 +678,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 > 7) { > ggms_mb = 1 << ggms_mb; > } > I'd argue this should be rolled into patch 4. It's not really fixing anything because igd_gen() can't return anything between 6 and 8. This only allows for several device versions that we currently consider to be gen 6 to align with i915 kernel driver generation by calling them generation 7. We'd previously lumped them into generation 6 because there's no functional difference we care about here between 6 & 7. In the next patch you replace this '1 << ggms_mb' with '*= 2', which would be equivalent to 'ggms_mb << 1' and matches your description that the increment is doubled. Is there a separate bug fix that needs to be pulled out here? Also, please send a cover letter for any series longer than a single patch and please configure your tools to thread the series. Thanks, Alex
On Sun, 1 Dec 2024 22:11:29 -0700 Alex Williamson <alex.williamson@redhat.com> wrote: > On Mon, 2 Dec 2024 00:09:31 +0800 > Tomita Moeko <tomitamoeko@gmail.com> wrote: > > > Both intel documentation [1][2] and i915 driver shows GGMS represents > > GTT stolen memory size in multiple of 1MB, not 2MB starting from gen 8. > > > > [1] https://www.intel.com/content/dam/www/public/us/en/documents/datasheets/3rd-gen-core-desktop-vol-2-datasheet.pdf > > [2] https://www.intel.com/content/dam/www/public/us/en/documents/datasheets/4th-gen-core-family-desktop-vol-2-datasheet.pdf > > > > Signed-off-by: Tomita Moeko <tomitamoeko@gmail.com> > > --- > > hw/vfio/igd.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/hw/vfio/igd.c b/hw/vfio/igd.c > > index 4047f4f071..e40e601026 100644 > > --- a/hw/vfio/igd.c > > +++ b/hw/vfio/igd.c > > @@ -268,7 +268,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 > 7) { > > ggms = 1 << ggms; > > } > > > > @@ -678,7 +678,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 > 7) { > > ggms_mb = 1 << ggms_mb; > > } > > > > I'd argue this should be rolled into patch 4. It's not really fixing > anything because igd_gen() can't return anything between 6 and 8. This > only allows for several device versions that we currently consider to > be gen 6 to align with i915 kernel driver generation by calling them > generation 7. We'd previously lumped them into generation 6 because > there's no functional difference we care about here between 6 & 7. > > In the next patch you replace this '1 << ggms_mb' with '*= 2', which > would be equivalent to 'ggms_mb << 1' and matches your description that > the increment is doubled. Is there a separate bug fix that needs to be > pulled out here? > > Also, please send a cover letter for any series longer than a single > patch and please configure your tools to thread the series. Thanks, Disregard this latter comment, I just wasn't copied on the cover letter and didn't have it in my inbox to root the thread. Thanks, Alex
On Mon, 2024-12-02 at 00:09 +0800, Tomita Moeko wrote: > CAUTION: External Email!! > Both intel documentation [1][2] and i915 driver shows GGMS represents > GTT stolen memory size in multiple of 1MB, not 2MB starting from gen > 8. > > [1] > https://www.intel.com/content/dam/www/public/us/en/documents/datasheets/3rd-gen-core-desktop-vol-2-datasheet.pdf > [2] > https://www.intel.com/content/dam/www/public/us/en/documents/datasheets/4th-gen-core-family-desktop-vol-2-datasheet.pdf > > Signed-off-by: Tomita Moeko <tomitamoeko@gmail.com> > --- > hw/vfio/igd.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/hw/vfio/igd.c b/hw/vfio/igd.c > index 4047f4f071..e40e601026 100644 > --- a/hw/vfio/igd.c > +++ b/hw/vfio/igd.c > @@ -268,7 +268,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 > 7) { This seems odd. The commit message talks about gen 8 but it changes the behaviour for gen 7 only. Additionally, ggms_mb is still shifted for gen 8 and later, so it's still increment by steps of 2 MB. Shouldn't this be something like gen < 8? > ggms = 1 << ggms; > } > > @@ -678,7 +678,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 > 7) { > ggms_mb = 1 << ggms_mb; > } > Btw. you could consider adding a link to the source code of i915 too.
On 12/2/24 14:12, Alex Williamson wrote: > On Sun, 1 Dec 2024 22:11:29 -0700 > Alex Williamson <alex.williamson@redhat.com> wrote: > >> On Mon, 2 Dec 2024 00:09:31 +0800 >> Tomita Moeko <tomitamoeko@gmail.com> wrote: >> >>> Both intel documentation [1][2] and i915 driver shows GGMS represents >>> GTT stolen memory size in multiple of 1MB, not 2MB starting from gen 8. >>> >>> [1] https://www.intel.com/content/dam/www/public/us/en/documents/datasheets/3rd-gen-core-desktop-vol-2-datasheet.pdf >>> [2] https://www.intel.com/content/dam/www/public/us/en/documents/datasheets/4th-gen-core-family-desktop-vol-2-datasheet.pdf >>> >>> Signed-off-by: Tomita Moeko <tomitamoeko@gmail.com> >>> --- >>> hw/vfio/igd.c | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/hw/vfio/igd.c b/hw/vfio/igd.c >>> index 4047f4f071..e40e601026 100644 >>> --- a/hw/vfio/igd.c >>> +++ b/hw/vfio/igd.c >>> @@ -268,7 +268,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 > 7) { >>> ggms = 1 << ggms; >>> } >>> >>> @@ -678,7 +678,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 > 7) { >>> ggms_mb = 1 << ggms_mb; >>> } >>> >> >> I'd argue this should be rolled into patch 4. It's not really fixing >> anything because igd_gen() can't return anything between 6 and 8. This >> only allows for several device versions that we currently consider to >> be gen 6 to align with i915 kernel driver generation by calling them >> generation 7. We'd previously lumped them into generation 6 because >> there's no functional difference we care about here between 6 & 7. >> >> In the next patch you replace this '1 << ggms_mb' with '*= 2', which >> would be equivalent to 'ggms_mb << 1' and matches your description that >> the increment is doubled. Is there a separate bug fix that needs to be >> pulled out here? Sorry this was a mistake when I composing my patches. At this time, there was no gen 7, original condition won't cause any issue. It is more suitable to be included in patch 4. I will drop this one in v2. >> Also, please send a cover letter for any series longer than a single >> patch and please configure your tools to thread the series. Thanks, > > Disregard this latter comment, I just wasn't copied on the cover letter > and didn't have it in my inbox to root the thread. Thanks, > > Alex CC of cover letter is not generated by get_mainainer.pl hook. Sorry for not checking it before sending, I will manually set correct CC in v2.
On 12/2/24 16:54, Corvin Köhne wrote: > On Mon, 2024-12-02 at 00:09 +0800, Tomita Moeko wrote: >> CAUTION: External Email!! >> Both intel documentation [1][2] and i915 driver shows GGMS represents >> GTT stolen memory size in multiple of 1MB, not 2MB starting from gen >> 8. >> >> [1] >> https://www.intel.com/content/dam/www/public/us/en/documents/datasheets/3rd-gen-core-desktop-vol-2-datasheet.pdf >> [2] >> https://www.intel.com/content/dam/www/public/us/en/documents/datasheets/4th-gen-core-family-desktop-vol-2-datasheet.pdf >> >> Signed-off-by: Tomita Moeko <tomitamoeko@gmail.com> >> --- >> hw/vfio/igd.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/hw/vfio/igd.c b/hw/vfio/igd.c >> index 4047f4f071..e40e601026 100644 >> --- a/hw/vfio/igd.c >> +++ b/hw/vfio/igd.c >> @@ -268,7 +268,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 > 7) { > > This seems odd. The commit message talks about gen 8 but it changes the > behaviour for gen 7 only. Additionally, ggms_mb is still shifted for gen > 8 and later, so it's still increment by steps of 2 MB. Shouldn't this be > something like gen < 8? Yes it should be. I think putting the change here is not correct, as alex said. I am going to remove this change in v2. >> ggms = 1 << ggms; >> } >> >> @@ -678,7 +678,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 > 7) { >> ggms_mb = 1 << ggms_mb; >> } >> > > Btw. you could consider adding a link to the source code of i915 too. > >
diff --git a/hw/vfio/igd.c b/hw/vfio/igd.c index 4047f4f071..e40e601026 100644 --- a/hw/vfio/igd.c +++ b/hw/vfio/igd.c @@ -268,7 +268,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 > 7) { ggms = 1 << ggms; } @@ -678,7 +678,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 > 7) { ggms_mb = 1 << ggms_mb; }
Both intel documentation [1][2] and i915 driver shows GGMS represents GTT stolen memory size in multiple of 1MB, not 2MB starting from gen 8. [1] https://www.intel.com/content/dam/www/public/us/en/documents/datasheets/3rd-gen-core-desktop-vol-2-datasheet.pdf [2] https://www.intel.com/content/dam/www/public/us/en/documents/datasheets/4th-gen-core-family-desktop-vol-2-datasheet.pdf Signed-off-by: Tomita Moeko <tomitamoeko@gmail.com> --- hw/vfio/igd.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)