[PATCH 2/2] drm/mgag200: Add an option to disable Write-Combine

[Jocelyn Falempe]

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 at 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;
>>   }
>