[LKP] [drm/mgag200] 90f479ae51: vm-scalability.median -18.8% regression

Thu Aug 1 13:30:59 UTC 2019

On 2019-08-01 8:19 a.m., Rong Chen wrote:
> Hi,
> 
> On 7/31/19 6:21 PM, Michel Dänzer wrote:
>> On 2019-07-31 11:25 a.m., Huang, Ying wrote:
>>> Hi, Daniel,
>>>
>>> Daniel Vetter <daniel at ffwll.ch> writes:
>>>
>>>> On Tue, Jul 30, 2019 at 10:27 PM Dave Airlie <airlied at gmail.com> wrote:
>>>>> On Wed, 31 Jul 2019 at 05:00, Daniel Vetter <daniel at ffwll.ch> wrote:
>>>>>> On Tue, Jul 30, 2019 at 8:50 PM Thomas Zimmermann
>>>>>> <tzimmermann at suse.de> wrote:
>>>>>>> Hi
>>>>>>>
>>>>>>> Am 30.07.19 um 20:12 schrieb Daniel Vetter:
>>>>>>>> On Tue, Jul 30, 2019 at 7:50 PM Thomas Zimmermann
>>>>>>>> <tzimmermann at suse.de> wrote:
>>>>>>>>> Am 29.07.19 um 11:51 schrieb kernel test robot:
>>>>>>>>>> Greeting,
>>>>>>>>>>
>>>>>>>>>> FYI, we noticed a -18.8% regression of vm-scalability.median
>>>>>>>>>> due to commit:>
>>>>>>>>>>
>>>>>>>>>> commit: 90f479ae51afa45efab97afdde9b94b9660dd3e4
>>>>>>>>>> ("drm/mgag200: Replace struct mga_fbdev with generic
>>>>>>>>>> framebuffer emulation")
>>>>>>>>>> https://kernel.googlesource.com/pub/scm/linux/kernel/git/next/linux-next.git
>>>>>>>>>> master
>>>>>>>>> Daniel, Noralf, we may have to revert this patch.
>>>>>>>>>
>>>>>>>>> I expected some change in display performance, but not in VM.
>>>>>>>>> Since it's
>>>>>>>>> a server chipset, probably no one cares much about display
>>>>>>>>> performance.
>>>>>>>>> So that seemed like a good trade-off for re-using shared code.
>>>>>>>>>
>>>>>>>>> Part of the patch set is that the generic fb emulation now maps
>>>>>>>>> and
>>>>>>>>> unmaps the fbdev BO when updating the screen. I guess that's
>>>>>>>>> the cause
>>>>>>>>> of the performance regression. And it should be visible with other
>>>>>>>>> drivers as well if they use a shadow FB for fbdev emulation.
>>>>>>>> For fbcon we should need to do any maps/unamps at all, this is
>>>>>>>> for the
>>>>>>>> fbdev mmap support only. If the testcase mentioned here tests fbdev
>>>>>>>> mmap handling it's pretty badly misnamed :-) And as long as you
>>>>>>>> don't
>>>>>>>> have an fbdev mmap there shouldn't be any impact at all.
>>>>>>> The ast and mgag200 have only a few MiB of VRAM, so we have to
>>>>>>> get the
>>>>>>> fbdev BO out if it's not being displayed. If not being mapped, it
>>>>>>> can be
>>>>>>> evicted and make room for X, etc.
>>>>>>>
>>>>>>> To make this work, the BO's memory is mapped and unmapped in
>>>>>>> drm_fb_helper_dirty_work() before being updated from the shadow
>>>>>>> FB. [1]
>>>>>>> That fbdev mapping is established on each screen update, more or
>>>>>>> less.
>>>>>>>  From my (yet unverified) understanding, this causes the performance
>>>>>>> regression in the VM code.
>>>>>>>
>>>>>>> The original code in mgag200 used to kmap the fbdev BO while it's
>>>>>>> being
>>>>>>> displayed; [2] and the drawing code only mapped it when necessary
>>>>>>> (i.e.,
>>>>>>> not being display). [3]
>>>>>> Hm yeah, this vmap/vunmap is going to be pretty bad. We indeed should
>>>>>> cache this.
>>>>>>
>>>>>>> I think this could be added for VRAM helpers as well, but it's
>>>>>>> still a
>>>>>>> workaround and non-VRAM drivers might also run into such a
>>>>>>> performance
>>>>>>> regression if they use the fbdev's shadow fb.
>>>>>> Yeah agreed, fbdev emulation should try to cache the vmap.
>>>>>>
>>>>>>> Noralf mentioned that there are plans for other DRM clients
>>>>>>> besides the
>>>>>>> console. They would as well run into similar problems.
>>>>>>>
>>>>>>>>> The thing is that we'd need another generic fbdev emulation for
>>>>>>>>> ast and
>>>>>>>>> mgag200 that handles this issue properly.
>>>>>>>> Yeah I dont think we want to jump the gun here.  If you can try to
>>>>>>>> repro locally and profile where we're wasting cpu time I hope that
>>>>>>>> should sched a light what's going wrong here.
>>>>>>> I don't have much time ATM and I'm not even officially at work until
>>>>>>> late Aug. I'd send you the revert and investigate later. I agree
>>>>>>> that
>>>>>>> using generic fbdev emulation would be preferable.
>>>>>> Still not sure that's the right thing to do really. Yes it's a
>>>>>> regression, but vm testcases shouldn run a single line of fbcon or
>>>>>> drm
>>>>>> code. So why this is impacted so heavily by a silly drm change is
>>>>>> very
>>>>>> confusing to me. We might be papering over a deeper and much more
>>>>>> serious issue ...
>>>>> It's a regression, the right thing is to revert first and then work
>>>>> out the right thing to do.
>>>> Sure, but I have no idea whether the testcase is doing something
>>>> reasonable. If it's accidentally testing vm scalability of fbdev and
>>>> there's no one else doing something this pointless, then it's not a
>>>> real bug. Plus I think we're shooting the messenger here.
>>>>
>>>>> It's likely the test runs on the console and printfs stuff out
>>>>> while running.
>>>> But why did we not regress the world if a few prints on the console
>>>> have such a huge impact? We didn't get an entire stream of mails about
>>>> breaking stuff ...
>>> The regression seems not related to the commit.  But we have retested
>>> and confirmed the regression.  Hard to understand what happens.
>> Does the regressed test cause any output on console while it's
>> measuring? If so, it's probably accidentally measuring fbcon/DRM code in
>> addition to the workload it's trying to measure.
>>
> 
> Sorry, I'm not familiar with DRM, we enabled the console to output logs,
> and attached please find the log file.
> 
> "Command line: ... console=tty0 earlyprintk=ttyS0,115200
> console=ttyS0,115200 vga=normal rw"

I assume the

user  :notice: [  xxx.xxxx] xxxxxxxxx bytes / xxxxxxx usecs = xxxxx KB/s

lines are generated by the test?

If so, unless the test is intended to measure console performance, it
should be fixed not to generate output to console (while it's measuring).

-- 
Earthling Michel Dänzer               |              https://www.amd.com
Libre software enthusiast             |             Mesa and X developer