| Message ID | 20240516161751.479558-3-jfalempe@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/mgag200: Add an option to disable Write-Combine | expand |
Hi, just nits below. Am 16.05.24 um 18:17 schrieb Jocelyn Falempe: > Unfortunately, the G200 ioburst workaround doesn't work on some servers > like Dell poweredge XR11, XR5610, or HPE XL260 > In this case completely disabling WC is the only option to achieve > low-latency. > So this adds a new Kconfig option, to disable WC mapping of the G200 The formatting look off. Maybe make this one single paragraph. No comma after 'option'. > > Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com> > --- > drivers/gpu/drm/mgag200/Kconfig | 10 ++++++++++ > drivers/gpu/drm/mgag200/mgag200_drv.c | 7 +++++++ > 2 files changed, 17 insertions(+) > > diff --git a/drivers/gpu/drm/mgag200/Kconfig b/drivers/gpu/drm/mgag200/Kconfig > index b28c5e4828f47..73ab5730b74d9 100644 > --- a/drivers/gpu/drm/mgag200/Kconfig > +++ b/drivers/gpu/drm/mgag200/Kconfig > @@ -11,3 +11,13 @@ config DRM_MGAG200 > MGA G200 desktop chips and the server variants. It requires 0.3.0 > of the modesetting userspace driver, and a version of mga driver > that will fail on KMS enabled devices. > + > +config DRM_MGAG200_DISABLE_WRITECOMBINE > + bool "Disable Write Combine mapping of VRAM" > + depends on DRM_MGAG200 && PREEMPT_RT > + help > + The VRAM of the G200 is mapped with Write-Combine, to improve No comma after Write-Combine > + performances. However this increases the system latency a lot, even Just say "This can interfere with real-time tasks; even if they are running on other CPU cores then the graphics output." > + for realtime tasks running on other CPU cores. Typically 40us-80us > + latencies are measured with hwlat when Write Combine is enabled. Leave out the next sentence: "Typically ..." The measureed numbers depend on the hardware and everyone is encouraged to test on their own system. You could mention the numbers in the commit description, as you already mention the affected systems there. > + Recommended if you run realtime tasks on a server with a Matrox G200. I still think that we should not encourage anyone to use this option. Maybe say "Enable this option only if you run..." > \ No newline at end of file > diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c b/drivers/gpu/drm/mgag200/mgag200_drv.c > index 3883f25ca4d8b..7461e3f984eff 100644 > --- a/drivers/gpu/drm/mgag200/mgag200_drv.c > +++ b/drivers/gpu/drm/mgag200/mgag200_drv.c > @@ -146,12 +146,19 @@ int mgag200_device_preinit(struct mga_device *mdev) > } > mdev->vram_res = res; > > +#if defined(CONFIG_DRM_MGAG200_DISABLE_WRITECOMBINE) > + drm_info(dev, "Disable Write Combine\n"); I would not print this drm_info() here. The user has selected the config option, so they should know what happens. It's also listed in /proc/mtrr IIRC. Best regards Thomas > + mdev->vram = devm_ioremap(dev->dev, res->start, resource_size(res)); > + if (!mdev->vram) > + return -ENOMEM; > +#else > mdev->vram = devm_ioremap_wc(dev->dev, res->start, resource_size(res)); > if (!mdev->vram) > return -ENOMEM; > > /* Don't fail on errors, but performance might be reduced. */ > devm_arch_phys_wc_add(dev->dev, res->start, resource_size(res)); > +#endif > > return 0; > }
On 17/05/2024 11:39, Thomas Zimmermann wrote: > Hi, > > just nits below. > > Am 16.05.24 um 18:17 schrieb Jocelyn Falempe: >> Unfortunately, the G200 ioburst workaround doesn't work on some servers >> like Dell poweredge XR11, XR5610, or HPE XL260 >> In this case completely disabling WC is the only option to achieve >> low-latency. >> So this adds a new Kconfig option, to disable WC mapping of the G200 > > The formatting look off. Maybe make this one single paragraph. Ok, > > No comma after 'option'. Ok, > >> >> Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com> >> --- >> drivers/gpu/drm/mgag200/Kconfig | 10 ++++++++++ >> drivers/gpu/drm/mgag200/mgag200_drv.c | 7 +++++++ >> 2 files changed, 17 insertions(+) >> >> diff --git a/drivers/gpu/drm/mgag200/Kconfig >> b/drivers/gpu/drm/mgag200/Kconfig >> index b28c5e4828f47..73ab5730b74d9 100644 >> --- a/drivers/gpu/drm/mgag200/Kconfig >> +++ b/drivers/gpu/drm/mgag200/Kconfig >> @@ -11,3 +11,13 @@ config DRM_MGAG200 >> MGA G200 desktop chips and the server variants. It requires 0.3.0 >> of the modesetting userspace driver, and a version of mga driver >> that will fail on KMS enabled devices. >> + >> +config DRM_MGAG200_DISABLE_WRITECOMBINE >> + bool "Disable Write Combine mapping of VRAM" >> + depends on DRM_MGAG200 && PREEMPT_RT >> + help >> + The VRAM of the G200 is mapped with Write-Combine, to improve > No comma after Write-Combine Ok >> + performances. However this increases the system latency a lot, >> even > Just say "This can interfere with real-time tasks; even if they are > running on other CPU cores then the graphics output." Ok > >> + for realtime tasks running on other CPU cores. Typically 40us-80us >> + latencies are measured with hwlat when Write Combine is enabled. > > Leave out the next sentence: "Typically ..." The measureed numbers > depend on the hardware and everyone is encouraged to test on their own > system. You could mention the numbers in the commit description, as you > already mention the affected systems there. Ok > >> + Recommended if you run realtime tasks on a server with a Matrox >> G200. > > I still think that we should not encourage anyone to use this option. > Maybe say "Enable this option only if you run..." Agreed, I will change this. > >> \ No newline at end of file >> diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c >> b/drivers/gpu/drm/mgag200/mgag200_drv.c >> index 3883f25ca4d8b..7461e3f984eff 100644 >> --- a/drivers/gpu/drm/mgag200/mgag200_drv.c >> +++ b/drivers/gpu/drm/mgag200/mgag200_drv.c >> @@ -146,12 +146,19 @@ int mgag200_device_preinit(struct mga_device *mdev) >> } >> mdev->vram_res = res; >> +#if defined(CONFIG_DRM_MGAG200_DISABLE_WRITECOMBINE) >> + drm_info(dev, "Disable Write Combine\n"); > > I would not print this drm_info() here. The user has selected the config > option, so they should know what happens. It's also listed in /proc/mtrr > IIRC. Sure, I can remove the drm_info(). Thanks for the review, I will send a v2 shortly. > > Best regards > Thomas > >> + mdev->vram = devm_ioremap(dev->dev, res->start, resource_size(res)); >> + if (!mdev->vram) >> + return -ENOMEM; >> +#else >> mdev->vram = devm_ioremap_wc(dev->dev, res->start, >> resource_size(res)); >> if (!mdev->vram) >> return -ENOMEM; >> /* Don't fail on errors, but performance might be reduced. */ >> devm_arch_phys_wc_add(dev->dev, res->start, resource_size(res)); >> +#endif >> return 0; >> } >
diff --git a/drivers/gpu/drm/mgag200/Kconfig b/drivers/gpu/drm/mgag200/Kconfig index b28c5e4828f47..73ab5730b74d9 100644 --- a/drivers/gpu/drm/mgag200/Kconfig +++ b/drivers/gpu/drm/mgag200/Kconfig @@ -11,3 +11,13 @@ config DRM_MGAG200 MGA G200 desktop chips and the server variants. It requires 0.3.0 of the modesetting userspace driver, and a version of mga driver that will fail on KMS enabled devices. + +config DRM_MGAG200_DISABLE_WRITECOMBINE + bool "Disable Write Combine mapping of VRAM" + depends on DRM_MGAG200 && PREEMPT_RT + help + The VRAM of the G200 is mapped with Write-Combine, to improve + performances. However this increases the system latency a lot, even + for realtime tasks running on other CPU cores. Typically 40us-80us + latencies are measured with hwlat when Write Combine is enabled. + Recommended if you run realtime tasks on a server with a Matrox G200. \ No newline at end of file diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c b/drivers/gpu/drm/mgag200/mgag200_drv.c index 3883f25ca4d8b..7461e3f984eff 100644 --- a/drivers/gpu/drm/mgag200/mgag200_drv.c +++ b/drivers/gpu/drm/mgag200/mgag200_drv.c @@ -146,12 +146,19 @@ int mgag200_device_preinit(struct mga_device *mdev) } mdev->vram_res = res; +#if defined(CONFIG_DRM_MGAG200_DISABLE_WRITECOMBINE) + drm_info(dev, "Disable Write Combine\n"); + mdev->vram = devm_ioremap(dev->dev, res->start, resource_size(res)); + if (!mdev->vram) + return -ENOMEM; +#else mdev->vram = devm_ioremap_wc(dev->dev, res->start, resource_size(res)); if (!mdev->vram) return -ENOMEM; /* Don't fail on errors, but performance might be reduced. */ devm_arch_phys_wc_add(dev->dev, res->start, resource_size(res)); +#endif return 0; }
Unfortunately, the G200 ioburst workaround doesn't work on some servers like Dell poweredge XR11, XR5610, or HPE XL260 In this case completely disabling WC is the only option to achieve low-latency. So this adds a new Kconfig option, to disable WC mapping of the G200 Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com> --- drivers/gpu/drm/mgag200/Kconfig | 10 ++++++++++ drivers/gpu/drm/mgag200/mgag200_drv.c | 7 +++++++ 2 files changed, 17 insertions(+)