Some time ago phjanderson (from Freaktab forum) found a solution to this:
http://www.freaktab.com/showthread.php?5939-Fixing-RK3188-video-playback/page2&p=81489#post81489
It basically eliminated a blocking condition on a function on rk_fb.c, which also happened to apparently solve the stutter problem. This was called phjanderson's Vsync fix and lots of ROMs have added this patch since.
I then set out to test it on Linux along with my framebuffer fix (see https://github.com/Galland/3188-SRC-AP6210/commit/a3ddf7e46c4426e6cc67e09d7eee2a14baeea1f0 ) just to find that after a random amount of minutes after boot, there appeared a new thread called "fb-vsync" that utilized 100% of one of the 4 cores on the RK3188, regardless of the system being idle or never even opening a video.
To me, this is unacceptable since it forces the CPU to run at higher frequencies all the time, as well as consume lots more of power and a 25% performance penalty hit.
I initially thought it had something to do with Linux' screen usage or even with my framebuffer fix. However it did happen without the framebuffer fix, so I just let it out of my kernels.
However, I recently read on a forum ( http://www.rikomagic.co.uk/forum/viewtopic.php?p=15682&sid=e872d9b5e003da9082af7976a28fa50c#p15682 ) that Android people are seeing also the 25% CPU usage constant (1 core out of 4) due to this thread.
So I heavily suspect this patch is great for fixing the Vsync video stutter problems, but at the cost of losing 25% of the CPU performance, consuming more power all the time (+heat!), and thus reducing the component's life.
If you happen to have flashed a rooted ROM with Vsync fix and have an Android monitor installed (able to track system threads), please write a comment sharing if you've been able to see the thread in action.
UPDATE:
I'll try to explain a bit the code related to this problem.
Most of it is located in rk_fb.c where:
1) upon registering fb0 (the main framebuffer) a thread called "fb-vsync" is created.
2) This thread executes the function "rk_fb_wait_for_vsync_thread"
3) This function is an "infinite" loop that emits a "vsync" notification to user-space apps subscribed to it, whenever a Vsync happens, meanwhile it waits idle.
How does it wait for a Vsync?
Through the use of "wait_event_interruptible" with arguments:
- waitqueue: this is the event queue we're hanging on: dev_drv->vsync_info.wait
- condition:
!ktime_equal(timestamp, dev_drv->vsync_info.timestamp) && dev_drv->vsync_info.active
The 2nd part of the condition (dev_drv->vsync_info.active) should always be met since it is set to 1 just after creating this thread.
What about the 1st part? It compares:
a) variable "timestamp": which is constant!! and set to the value of "dev_drv->vsync_info.timestamp" as it was just before calling "wait_event_interruptible".
b) dev_drv->vsync_info.timestamp as it IS any time the waitqueue is awaken
Naturally this condition is meant to become true (and exit the wait event to send the "vsync" notification) at the next Vsync.
Why? Because "dev_drv->vsync_info.timestamp" is updated by the lcdc interrupt service routine, which at the same time wakes up our "wait_event_interruptible" above!
Then the !ktime_equal compares the "timestamp" variable that holds the PREVIOUSLY set vsync_info.timestamp (before the wait_event is triggered) with the CURRENT, just updated, vsync_info.timestamp.
Hence the condition may indeed become true.
What did phjanderson's fix do? It just made this wait event to always have a true condition, hence the "while" loop is constantly executing (no wait) and notifying a "vsync".
How does it interact with Galland's framebuffer fix? Note that in the lcdc isr, the fb fix excludes the wake_up_... call, so what it does is make the wait event above to never be woken up. Kind of the opposite to phjanderson's.
The union of both fixes is... unfortunate too :) so I just left out phjanderson's fix, since the isr changes are needed for the overall fb fix.
Hi Galland,
ReplyDeleteI think I have found the problem with the vsync and the mentioned fix of phjanderson.
The fix of phjanderson was to remove !ktime_equal(timestamp, dev_drv->vsync_info.timestamp) && in rk_fb.c.
Cause this is part of a while loop, it leads to permanently vsyncs, every loop. The reason for the deleted part was, to prevent exactly this, by comparing last vsync timestamp with current timestamp, and only if timestamps are not equal to trigger the vsync.
After looking at the code, I think the problem has to be fixed another way. Look 2 lines above the if statement in rk_fb.c.
ktime_t timestamp = dev_drv->vsync_info.timestamp;
This leads to !ktime_equal(timestamp, dev_drv->vsync_info.timestamp) never to be true.
I think it should be more like this:
ktime_t timestamp = ktime_get(); //This should set the timestamp on a correct value for comparison
I hope that it helps, cause I don't have a RK3188 to test it on my own.
Best Regards
JochenKauz
Thanks very much for the advanced insight! however I'm not sure I follow your thought.
DeleteFrom my point of view, since timestamp is a "constant" (doesn't change) in that comparison, whereas "dev_drv->vsync_info.timestamp" is a device variable that changes (it holds the last Vsync time every time it's checked), the condition may indeed become true: upon next Vsync, as apparently expected :S
I'll try to explain..
Deletelook at https://github.com/Galland/Linux3188.../rk3188_lcdc.c
There you find the following:
static irqreturn_t rk3188_lcdc_isr(intirq,void*dev_id)
{
struct rk3188_lcdc_device *lcdc_dev =
(struct rk3188_lcdc_device *)dev_id;
ktime_t timestamp = ktime_get();
lcdc_msk_reg(lcdc_dev, INT_STATUS, m_FS_INT_CLEAR, v_FS_INT_CLEAR(1));
if(lcdc_dev->driver.wait_fs) //three buffer ,no need to wait for sync
{
spin_lock(&(lcdc_dev->driver.cpl_lock));
complete(&(lcdc_dev->driver.frame_done));
spin_unlock(&(lcdc_dev->driver.cpl_lock));
}
lcdc_dev->driver.vsync_info.timestamp = timestamp;
wake_up_interruptible_all(&lcdc_dev->driver.vsync_info.wait);
return IRQ_HANDLED;
}
If this runs, driver.vsync_info.timestamp is set to ktime_get() (the current timestamp)
in rk_fb.c
ktime_t timestamp = dev_drv->vsync_info.timestamp;
int ret = wait_event_interruptible(dev_drv->vsync_info.wait,
!ktime_equal(timestamp, dev_drv->vsync_info.timestamp) &&
dev_drv->vsync_info.active);
That can not work.
!ktime_equal(timestamp, dev_drv->vsync_info.timestamp)
compares timestamp to dev_drv->vsync_info.timestamp. Both are the same.
This can only work if timestamp is set to current timestamp, if not, the comparison would be senseless.
The comparison is made to prevent to trigger the event every time the loop is run, it should only be run if timestamp has changed.
I hope you understand the point?
I've updated the post since you seem to be missing the meaning of ISRs and the wait_event you're talking about. It's all kernel stuff.
DeleteI did not want to offend you in any way and you did not mention in your post that you made a different fix for that problem.
DeleteSorry for the comments, you could delete it.
No offense at all :)
DeleteHowever my fix is for a different problem (lack of framebuffer console on Linux), not for the video vsync.
This comment has been removed by the author.
ReplyDeleteHi guys,
ReplyDeleteThanks for the analysis, but.... It's a bit outdated though, we already ditched the phjanderson fix for sam321's fix, removing the ability to disable vsync through rk_fb_ioctl:
https://github.com/phjanderson/Kerne...p/rk_fb.c#L388
I do not think such fixes are necessary when running Linux on the RK3188 as the problem seems to be in a part of the android system (hwcomposer/surfaceflinger) that keeps disabling the vsync, even while playing video (probably for power saving reasons).
Peter
The URL should be:
Deletehttps://github.com/phjanderson/Kernel-3188/blob/master/drivers/video/rockchip/rk_fb.c#L388
Thank you! leolas told me about sam321's solution but I couldn't find what it was :)
DeletePersonally, I'm not such a big fan of forums as information is scattered over zillions of pages in between loads of irrelevant posts. I'm often annoyed myself how difficult it is to find good information. It's better than nothing though ;-)
DeleteAbsolutely agree! It should be mandatory that, after a forum thread reaches some conclusion, to write that solution in a place apart in stone (or a wiki for that matter), imho :)
Delete