| ↑back Search ←Prev date Next date→ Show only urls | (Click on time to select a line by its url) |
| Who | What | When |
|---|---|---|
| *** | book` has quit IRC (Quit: Leaving) | [02:07] |
| ............................................................................................................ (idle for 8h58mn) | ||
| marc|gonzalez | mchehab: been banging my head over drivers/media/dvb-core/dvb_frontend.c (from an old v4.4 kernel to make matters worse)
The tuning parts of DVB are implemented via timers and polling, right? Even when the demod/tuner pair supports interrupts | [11:05] |
| ......... (idle for 44mn) | ||
| Hmm dunno why I dropped the connection... | [11:50] | |
| mchehab | as I said, it depends on the driver
there are 3 ways for zigzag | [11:54] |
| marc|gonzalez | si2168
dvb_frontend_swzigzag_autotune Or rather dvb_frontend_swzigzag() i.e. software zigzag IIUC | [11:54] |
| mchehab | it is controlled by a callback which returns either DVBFE_ALGO_SW, DVBFE_ALGO_HW, DVBFE_ALGO_CUSTOM | [11:57] |
| marc|gonzalez | mchehab: I don't understand the user API. 1) User requests to tune to a frequency via FE_SET_PROPERTY ioctl, then polls the status for LOCK via FE_READ_STATUS ioctl ? | [11:57] |
| mchehab | yes
the frontend will apply either one of the 3 possible tuning algorithms older devices need some help from the DVB core to tune, using "SW" zigzag | [11:57] |
| marc|gonzalez | if fe->ops.get_frontend_algo is empty, then it jumps to dvb_frontend_swzigzag() unconditionnally | [11:58] |
| mchehab | a few drivers have a different algorithm coded at the driver, with a partial HW support (CUSTOM)
as a fallback, the core will manually do the sigzag (SW) | [11:59] |
| marc|gonzalez | The part that is strange to me, is that both user-space and kernel dvb_frontend_thread are banging on read_status of si2168
Sounds like a recipe for races? | [12:00] |
| mchehab | this is the default if the driver doesn't specifiy the algorithm
not sure about si2168, but read_status can just return the latest cached data, without calling the frontend | [12:00] |
| marc|gonzalez | Basically what I'm seeing is that after the user-space has scanned the last frequency it cared about, the dvb_frontend_thread keeps looping in state FESTATE_TUNING_FAST | [12:01] |
| mchehab | I won't doubt that si2168 would do zigzag in hardware (but this needs to be checked on its datasheets, to be sure) | [12:02] |
| marc|gonzalez | I suppose I can check the zigzag, since I'm stumped right now :( | [12:02] |
| mchehab | drivers are conservative: if we don't know for sure that the hardware supports zigzag, the Kernel will do it | [12:02] |
| marc|gonzalez | b-rad: would sure like to hear from you, because my dual T/T2 scan is not working with si2168 | [12:03] |
| mchehab | the zigzag logic is there to handle frequency variations, due to doppler effect and such | [12:03] |
| marc|gonzalez | mchehab: I understand the justification for zigzag, what I'm saying is that there seems to be a condition that's not triggering that's keeping the state machine in the zigzag state | [12:04] |
| mchehab | without that, channel scanning may end not detecting certain channels, or losing them | [12:04] |
| marc|gonzalez | mchehab: but if everything is implemented with timers and polling, then either we waste resources polling too often, or we might increase the latency of operations, no? | [12:04] |
| mchehab | marc|gonzalez: if you're driving a car while watching TV, the frequency will keep changing as you accelerate, break or move the car to a different direction
so, zigzag should continue running | [12:05] |
| marc|gonzalez | I see...
So perhaps I'm barking up the wrong tree... Back to the drawing board then | [12:05] |
| mchehab | yep. this is something that it is needed... the SW zigzag is a fallback when the driver doesn't know anything better | [12:06] |
| marc|gonzalez | "something that is needed" handling interrupts? or HW zigzag? | [12:06] |
| mchehab | well, if hardware interrupts are there (and driver developers have enough knowledge about the hardware), the CUSTOM algo could be used
or if the hardware does it iself, the entire SW zigzag logic can be disabled | [12:07] |
| marc|gonzalez | I'm not talking just about zigzag, for example si2168 can raise an interrupt when it locks several signals | [12:08] |
| mchehab | if you do a:
$ git grep -l DVBFE_ALGO_HW drivers/media/ you'll see some drivers that do it in hardware (usually, this is actually on some firmware inside the device) well, if si2168 can also raise an interrupt when it loses lock, then the driver could be optimized to only do I2C reads for status when interrupts are rised... updating the status cache. yet, there's a catch: | [12:09] |
| marc|gonzalez | status cache is dev->fe_status ? | [12:12] |
| mchehab | not all bridges may be handling the IRQ lines from the dvb-frontend | [12:12] |
| marc|gonzalez | hmmm, unlikely struct si2168_dev *dev = i2c_get_clientdata(client);
How does the demod driver update the generic status cache? | [12:13] |
| mchehab | marc|gonzalez: it is not like that...
the driver itself will need to handle queries to the status callback and either send an I2C command or return a cached value inside the driver I implemented codes like that on several drivers... but not due to IRQs... the goal were to avoid flooding the I2C bus with lots of read_status per second | [12:14] |
| marc|gonzalez | makes sense. I made the same modification in pca953x: cache input pin values
but that would be an optimization, right now, I'm seeing hard failures :-( | [12:16] |
| mchehab | what do you mean by hard failures? The hardware is failing? | [12:19] |
| marc|gonzalez | Well, the demod is not locking the frequency, even when I give it 10 seconds to lock
I am trying to use a special mode on the si2168 scan DVBT/T2 in parallel using a patch from Brad Basically what we discussed last week I don't want to scan for T, wait X seconds for timeout, then for T2 that might double the scan time BTW, the zigzag thing, is for the tuner, or for the demod? "Configure DTV AGC Automatic Freeze" ? | [12:19] |
| ........................ (idle for 1h57mn) | ||
| b-rad: ping? | [14:24] | |
| mchehab: I think my hunch was right: I made si2168_set_frontend() a noop if we are trying to reset the same frequency. Then scanning for T/T2 works
set_frontend() restarts stuff that's ongoing, so it shouldn't be called too often, I think | [14:35] | |
| .... (idle for 16mn) | ||
| b-rad | hello marc|gonzalez | [14:52] |
| marc|gonzalez | b-rad: save me! | [14:52] |
| b-rad | my patch works and is deployed by a large number of commercial customers and used by loads of end users running my kernels
if it didn't work i would have some very large companies yelling at me | [14:53] |
| marc|gonzalez | b-rad: I do not doubt it.
I do suspect something borked going on in my vendor kernel b-rad: isn't it weird that si2168_set_frontend() is called with the same parameters that it was already called with? Won't that reset the current tuning in progress? I tweaked si2168_set_frontend() to exit immediately if it is invoked with the same parameters as the previous call, and with that change, I am able to tune T/T2 together though I suspect my "tweak" is wrong on many levels | [14:53] |
| b-rad | well, it works currently
i don't have a dvb generator to try and run any tests i think i recall set_frontend being called a couple times not locking after 10s seems to indicate something fubar | [14:57] |
| marc|gonzalez | b-rad: as I said, it's probably something on my side. Nothing you could test for.
BTW, I see there is a NOTDVBT flag that gets set pretty early on Wouldn't it be a good idea to expose such a flag to user-space? They could give up sooner, rather than wait for a LOCK timeout | [15:01] |
| ..................... (idle for 1h43mn) | ||
| mchehab: in the zigzag thingy, the set_frontend gives a slightly different frequency to tune to?
Errr, I mean: the zigzag algo provides a slightly different freq for set_frontend? | [16:46] | |
| mchehab | yes
as I said, f+delta, f-delta, ... | [16:51] |
| marc|gonzalez | because what I'm seeing is set_frontend being called always with the same frequency | [16:52] |
| mchehab | that sounds a bug to me | [16:52] |
| marc|gonzalez | yeah, I just can't put my finger on it, and going slowly insane | [16:52] |
| mchehab | perhaps the freq step thing is wrong
for this frontend causing it to round to the same freq after math | [16:52] |
| marc|gonzalez | I'll check that out
mchehab: at what point is it supposed to say "hmmm this freq is no good, I'll try a close one in zigzag" ?? After dvb_frontend_tune_settings . min_delay_ms ? | [16:53] |
| mchehab | marc|gonzalez: not sure. This code is very old and I never needed to tweak it | [16:57] |
| marc|gonzalez | smirk | [16:57] |
| mchehab | b-rad: do you know if si2168 has its own hardware-implemented sigzag algorithm, or if this should be done in software? | [17:01] |
| marc|gonzalez | mchehab: maybe my user-space app should set FE_TUNE_MODE_ONESHOT ?
mchehab: zigzag is a prop of tuner or demod? | [17:08] |
| mchehab | that's a good question... it depends on both tuner and demod :-) | [17:09] |
| marc|gonzalez | I'll look for zigzag in the docs I have for tuner and demod | [17:10] |
| mchehab | I don't think a flag here would help | [17:10] |
| marc|gonzalez | the flag would disable the zigzagging that's breaking the user-space scan though?? | [17:10] |
| mchehab | disabling zigzag will break watching TV
as I said you some days ago, if you want a "speed tune", the best would be to implement it in Kernelspace I actually meant "speed scan" | [17:11] |
| marc|gonzalez | my knowledge is a bit too limited to go carving stuff up in the driver
in the kernel | [17:12] |
| mchehab | knowing what the hw supports can help making something that would be a lot faster at Kernelspace | [17:12] |
| marc|gonzalez | that's for sure
getting an IRQ when there is a lock or when the demod is sure that a signal is not DVB-T is much quicker than waiting for timeouts | [17:13] |
| mchehab | yes, but this may not work for USB devices
(it should work for platform or PCI ones) if the bridge driver enables the right IRQ lines at the tuner and demod s/enables/enable/ (well, usb has a way of sending INT packets... never used it though, and I guess it is also polling based) https://www.kernel.org/doc/html/latest/driver-api/usb/URB.html#how-to-start-interrupt-int-transfers in any case, using INT URBs would require some changes at the USB bridge drivers and not all drivers will support. So, this should be an optional feature at demod/tuner drivers, that would be enabled only if the bridge supports it If I'm not mistaken, on INT URBs, the USB bridge prepares a buffer to receive int packets... then, when it arrives, the URB callback handler should get them yet, the USB controller will use some sort of polling logic in order to handle it | [17:14] |
| b-rad | i don't see anything related in my fw references mchehab | [17:20] |
| mchehab | b-rad: ok, thanks for checking it.
so, the it shouldn't be safe to disable zigzag on those devices as channels may not be detected if we disable software sigzag | [17:21] |
| .... (idle for 15mn) | ||
| marc|gonzalez | mchehab: it looks like things work better if I increase the s->min_delay_ms = 900; in si2168_get_tune_settings()
maybe it is set too low, and the zigzag algo kicks in too early for some frequencies (and the fact that it zigzags on the same freq is a different bug) | [17:38] |
| mchehab | I suspect that nobody tried to tweak delay_ms for this fe yet
yeah, this parameter is demod-dependent zigzag should be using .frequency_stepsize_hz | [17:47] |
| marc|gonzalez | also I see that the demod datasheet documents a TUNER_TUNE_FREQ command that's supposed to complete in 35ms for -77dbm
mchehab: min_delay is the only param that is set in the driver, though that would explain the 0 step then... | [17:50] |
| mchehab | case SYS_DVBT:
case SYS_DVBT2: case SYS_ISDBT: case SYS_DTMB: fepriv->min_delay = HZ / 20; fepriv->step_size = dvb_frontend_get_stepsize(fe) * 2; fepriv->max_drift = (dvb_frontend_get_stepsize(fe) * 2) + 1; see dtv_set_frontend dvb_frontend_get_stepsize() returns min_delay_ms it sounds a little weird to me to set max_drift equal to step size + 1 | [17:51] |
| marc|gonzalez | was not available in v4.4 as well ;-) | [17:53] |
| mchehab | it sounds that for DVB-T/T2, it tries only f, f+(2*step), f-(2*step) | [17:53] |
| marc|gonzalez | hope I will not lose all my remaining hair! | [17:54] |
| mchehab | the helper function is there to solve some border cases
(actually, it is meant to support hybrid DVB-T/S frontends I guess it is there because the sigzag is a way more important on DVB-S/S2 for those, and for DVB-C, it does: fepriv->step_size = c->symbol_rate / 16000; fepriv->max_drift = c->symbol_rate / 2000; | [17:54] |
| marc|gonzalez | OK, it's 8pm, gotta grab some food. Will pick up tomorrow. TTYL | [17:57] |
| mchehab | e. g. zigzag goes from f...f+8xstep
marc|gonzalez: just to finish today's discussion, if the step_size is zero, the core should either: 1) disable zigzag; 2) guess a step_size; 3) generate a warning | [17:57] |
| marc|gonzalez | is that done today? | [17:59] |
| mchehab | no | [17:59] |
| marc|gonzalez | I can patch v4.4 but that won't help mainline
OK, I can take a look at a patch v4.4 and mainline | [17:59] |
| mchehab | well, please submit a patch upstream
it can then be backported up to 4.4 | [17:59] |
| marc|gonzalez | will do, sounds like a plan | [17:59] |
| mchehab: your list is inclusive or pick one option? | [18:04] | |
| mchehab | inclusive | [18:04] |
| marc|gonzalez | there is no point in a step_size if zigzag is disabled, right? | [18:04] |
| mchehab | well, if the frontend does the right thing and implements the get_frontend_algo() callback, the zigzag code will never be called
the problem is if: - get_frontend_algo() is not implemented or if it returns DVBFE_ALGO_HW - the frontend doesn't specify the step size on such case, IMHO, the core should: - produce a warning that the frontend driver need fixes; - disable step_size please also notice that the code need to do what's there at set_frontend for some standards, it doesn't require a step size specified at the frontend, as it has a fallback logic in other words, I guess a patch should move the code below this comment: get frontend-specific tuning settings /* get frontend-specific tuning settings */ memset(&fetunesettings, 0, sizeof(struct dvb_frontend_tune_settings)); | [18:05] |
| marc|gonzalez | I don't have 5.8-rc1 in front of me :( | [18:09] |
| mchehab | into a separate function and call it before deciding the algo at the frontend's thread
I guess I'll try to write a RFC patch to the ML | [18:09] |
| marc|gonzalez | I can take a look if you CC me
really gotta run now arrg | [18:10] |
| mchehab | ok. ttyl | [18:13] |
| ↑back Search ←Prev date Next date→ Show only urls | (Click on time to select a line by its url) |