Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

linux: DVB fixes #2382

Merged
merged 2 commits into from
Feb 1, 2018
Merged

linux: DVB fixes #2382

merged 2 commits into from
Feb 1, 2018

Conversation

MilhouseVH
Copy link
Contributor

See forum for ksoftirqd commit.

@MilhouseVH MilhouseVH changed the title linux: commit fixes linux: DVB and lan commit fixes Jan 4, 2018
@HiassofT
Copy link
Member

HiassofT commented Jan 4, 2018

Have these fixes been discussed with upstream? Is it confirmed that this is not a bug in the DVB driver(s) that the revert works around?

I'm not too happy adding random reverts/patches of core Linux code without a really strong need to do.

@MilhouseVH MilhouseVH changed the title linux: DVB and lan commit fixes linux: DVB fixes Jan 4, 2018
@MilhouseVH
Copy link
Contributor Author

I agree, I've added merge blocked as this is just for testing purposes as it allows users to get back on to a current kernel and continue testing other stuff. I don't know if anyone has been in contact with upstream, or how best to pursue that.

@polojoe
Copy link

polojoe commented Jan 6, 2018

@MilhouseVH
Regression has been filed upstream https://www.spinics.net/lists/linux-usb/msg164413.html
Maybe you or @popcornmix @HiassofT have something to add.

@MilhouseVH
Copy link
Contributor Author

Excellent thanks @polojoe - I've got nothing more to add at this stage, it looks like they've got all the information they need in the mailing list posts.

@HiassofT
Copy link
Member

HiassofT commented Jan 6, 2018

@polojoe thanks a lot for getting the ball rolling!

I don't have any hardware to reproduce the issues so I can't add anything valuable to the discussion but I'm sure you and upstream devs will find a solution

@MilhouseVH
Copy link
Contributor Author

@polojoe I'm not subscribed to that mailing list (I already get waaay too many emails a day as it is) so please don't hesitate to ping this PR if anything useful is posted, thanks.

@HiassofT
Copy link
Member

HiassofT commented Jan 7, 2018

quick update: Discussion is going on, Linus just posted and seems to favour reverting the ksoftirq commit:
https://www.mail-archive.com/linux-media@vger.kernel.org/msg124282.html

@polojoe
Copy link

polojoe commented Jan 8, 2018

@MilhouseVH
Mauro provided a patch to test.
https://www.mail-archive.com/linux-media@vger.kernel.org/msg124287.html
Could you please make a test build without reverting ksoftirqd commit and with the patch provided?

Please also leave a comment in libreelec forum thread for wider testing on affected hardware.
https://forum.libreelec.tv/thread/4235-dvb-issue-since-le-switched-to-kernel-4-9-x/

@CvH
Copy link
Member

CvH commented Jan 8, 2018

Wondering if https://patchwork.linuxtv.org/patch/46327/ is also a fix for that problem. If so it could be a much bigger problem.

@MilhouseVH
Copy link
Contributor Author

@polojoe I've uploaded build #0107z to test the increased buffers.

@polojoe
Copy link

polojoe commented Jan 8, 2018

Is there a possibility to make usb dumps in le?

@popcornmix
Copy link
Contributor

Personally I'm in favour of the revert. It broke something. There's no evidence of it helping anything.

But as the discussion upstream is progressing then waiting a while for their suggestions seems worthwhile. Assuming a fix/revert (that works for us) appears upstream in the near future then we take that. If nothing happens upstream (it doesn't sound like that will be the case) then the revert is the fallback option.

@polojoe
Copy link

polojoe commented Jan 8, 2018

Maybe usb dvb performance could be improved furthermore. Because despite revert I get errors in recordings if I'm streaming videos via lan on rpi at same time. Errors are gone if tvh is recording without lan activity.

@popcornmix
Copy link
Contributor

The "increase buffer" suggestion is interesting. If something like that fixes the problem it might even improve behaviour beyond the original regression (long shot but it's not impossible).

@polojoe
Copy link

polojoe commented Jan 8, 2018

@MilhouseVH
Linus provided a further patch. Could you do a test build?
https://paste.ubuntu.com/26348960/

@MilhouseVH
Copy link
Contributor Author

I've added build #0108b to the thread.

@polojoe
Copy link

polojoe commented Jan 9, 2018

@popcornmix
Jesper Dangaard Brouer made a proposal to improve usb/dvb performance on rpi.
https://www.mail-archive.com/linux-media@vger.kernel.org/msg124360.html

"If you expect/want to get stable performance out of such a small box,
then you (or LibreELEC) need to tune the box for this usage. And it
does not have to be that complicated. First step is to move IRQ
handling for the NIC to another CPU and than the USB port handling the
DVB signal (/proc/irq/*/smp_affinity_list). And then pin the
userspace process (taskset) to another CPU than the one handling
USB-softirq."

Would this be possible to do? Are we able to measure/show latency spikes on rpi with le as later mentioned in the post?

@popcornmix
Copy link
Contributor

From @P33M

The USB FIQ runs on core 1 if there are >1 cores online.
The fake IRQ runs on core 0, as do all other interrupts from outside
the ARM complex. I did toy briefly with trying to use another mailbox
to substitute the fake MPHI IRQ with a real per CPU mailbox IRQ that
was free, but wasn't able to get it working - I think due to churn
around the IRQ mapping code in Linux which borked things on a regular
basis. In theory with 3 mailbox interrupts per core unused, we can
pick and choose where to distribute our fake IRQs to. Tasklets are
also semi-guaranteed to run on the core that the hard IRQ runs on
(unless the tasklet is already running, in which case it won't be
migrated) which would in turn wake up the latency-sensitive buffer
processing a bit sooner.

In theory the userspace process handling DVB (I assume tvheadend or similar) could be pinned to core 2 or 3 which will separate its work from the interrupt based handling of USB in kernel.

@popcornmix
Copy link
Contributor

perf runs on the pi. It lives in the kernel tree but is a standalone application that can be built:
https://github.com/raspberrypi/linux/tree/rpi-4.9.y/tools/perf

A build of perf for LE may be interesting. I'm not an expert on its usage but it can be used to read profiling counters of various parts of the chip.

@MilhouseVH
Copy link
Contributor Author

jahutchi (who identified the original commit causing the issue) is testing with a Revo 3700 (Intel(R) Atom(TM) CPU D525 @ 1.80GHz, quad core) which may not be a powerhouse, but it's also not a Raspberry Pi, so is special tuning really required or is this a red herring?

Prior to 4.9 no tuning was necessary, but now it is (or may be required) - surely that's a good argument for this being a regression, and tuning isn't the solution?

@HiassofT
Copy link
Member

HiassofT commented Jan 9, 2018

There's some indication in the forum post that Linus' patch fixes DVB issues.

Concerning tuning: there was some important lines snipped from Jesper's mail
https://www.mail-archive.com/linux-media@vger.kernel.org/msg124360.html

Are you also recording the stream on the Raspberry Pi?
It seems to me, that you are expecting too much from this small device.

Recording with tvheadend plus playing back the video with kodi could indeed be too much and might require some tuning - just a guess though, never tried that.

When testing things we have to be very careful not to mix up probably unrelated issues. Problems with recording+playback at same time or audio dropouts can be caused by something else. If they are all caused by the softirqd patch your testbuild with that commit reverted should be perfect. If it's not, the issues can be due to other kernel changes, maybe different kodi or tvheadend versions etc.

@MilhouseVH
Copy link
Contributor Author

MilhouseVH commented Jan 9, 2018

When testing things we have to be very careful not to mix up probably unrelated issues.

Agreed. Unfortunately in this case my builds are bleeding edge so userland (Kodi etc.) is a moving target and may introduce bugs that are unrelated to this issue, and LE has changed significantly over the last year since this issue was first raised.

A build of perf for LE may be interesting.

I've uploaded a build of perf for RPi2 and kernel 4.14.10. Download and chmod +x and it should allow timechart and other stats to be captured: link (1.7MB)

perf timechart record
perf timechart

produces an output.svg file which looks pretty and may mean something to someone. :)

perf sched record/perf sched timehist is also working.

Not entirely sure how it should be used, though.

I'll include perf in any future test builds for this DVB issue.

@chewitt
Copy link
Member

chewitt commented Feb 1, 2018

@MilhouseVH @HiassofT did upstream arrive at a solution for this yet? .. it's no issue to revert the commit for now if we know this will be resolved in a future kernel bump.

@HiassofT
Copy link
Member

HiassofT commented Feb 1, 2018

Upstream discussion seems to have stalled a bit. Last post was from Mauro 3 days ago, he's struggling with the upstream dwc2 USB driver (isochronous transfers are broken there). I'm following this via the linux-media list, so I could be missing discussions about it in other lists.

So I'd also say add the revert patch for now.

@MilhouseVH
Copy link
Contributor Author

MilhouseVH commented Feb 1, 2018

Or we could test with the alternative patch from Linus? That way we could compare (to some extent) 8.2.y with kernel 4.11.y and ksoftirq reverted, and 9.0 with kernel 4.14.y and Linus' patch).

Linus patch: https://www.mail-archive.com/linux-media@vger.kernel.org/msg124351.html

@HiassofT
Copy link
Member

HiassofT commented Feb 1, 2018

Good idea, switching to Linus' patch - at least for a while - should provide some more info. We can also compare results with your current/older testbuilds with the revert. Could be helpful to upstream, too.

@MilhouseVH
Copy link
Contributor Author

Yes my current builds include the revert so that will give a more direct comparison - I'll add Linus's patch as a second commit, we can squash later if we decide to keep it.

Copy link
Member

@chewitt chewitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks :)

@chewitt chewitt merged commit ae0e1ec into LibreELEC:master Feb 1, 2018
@chrisnovakovic
Copy link
Contributor

This is currently breaking RPi builds because the patch is already included as part of projects/RPi/patches/linux/linux-01-RPi_support.patch. The easiest way to work around this would be not to apply this patch for RPi builds, but i can't see an easy way to do that via scripts/unpack...

@MilhouseVH
Copy link
Contributor Author

Ah yes... the ksoftirqd patch is in the RPi support patch (which I don't use in my builds in case anyone is wondering). I'll chat with popcornmix about a fix - drop it, or we exclude it from the support patch. Either way I'll push a new support patch once we've decided.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants