<!-- Some styling for better description lists --><style type='text/css'>dt { font-weight: bold;float: left;display:inline;margin-right: 1em} dd { display:block; margin-left: 2em}</style>
hverkuil1: hi guys!
mkrufky: hello!
mchehab: hi
<br> <u>mkrufky</u>: there are two dvb patches that I would like you to review
syoung: hi everyone
mchehab: (it is big, but most of the stuff is just hard work, no brain)
<br> https://patchwork.linuxtv.org/patch/50778/
<br> https://patchwork.linuxtv.org/patch/50779/
<br> there is an obvious trivial error on a memset() there (I fixed locally already)
<br> sizeof(info) instead of sizeof(*info)
hverkuil1: <u>pinchartl</u>: ping
mchehab: they basically make tuner and frontend to always specify frontend frequency properties in Hz - no matter if the frontend if for satellite or not
hverkuil1: <u>mchehab</u>: I plan to post v16 of the request API later today. I've incorporated all your comments on the documentation, and it contains a bunch of fixes (thanks to Paul for testing with the cedrus driver).
mchehab: <u>hverkuil1</u>: thank you!
<br> btw, I'm planning to test that dvb vb2 patch likely today or tomorrow
hverkuil1: <u>FYI</u>: I have a week vacation next week, so I won't join this meeting next Thursday.
<br> I'll likely post at least one pull request tomorrow.
<br> <u>sailus</u>: ping
mkrufky: ah yes I was actually just reading those patches now, mchehab
mchehab: ok, thanks for looking on it!
<br> this change will help to better support tuners/demods that are trully universal...
<br> being able to work with both satellite and non-satellite delivery systems
<br> (the number of such hardware seems to be increasing)
mkrufky: heh i saw the patch from the kbuild robot about the memset patch and approved it, didn't realize at first it was against your series
<br> do existing userspace apps need to make a change for this?
mchehab: that is indeed a good question, yet to be answered
<br> I suspect that existing applications simply ignore frequency ranges
mkrufky: heh. i can test it with atsc
mchehab: as the Kernel returns an error if you try to set up an out of range frequency
<br> I'm currently modifying dvb-utils for it to print the frequency ranges
<br> that will help testing it
<br> dvb-fe-tool will now output this at the end:
<br> Frequency range for the current standard:
<br> <u>From</u>: 45.0x10^6 Hz
<br> <u>To</u>: 860x10^6 Hz
<br> <u>Step</u>: 62.5x10^3 Hz
mkrufky: I think we can be a little more user friendly, there
hverkuil1: <u>mchehab</u>: this https://patchwork.linuxtv.org/patch/49633/ is marked Accepted. Did you accept it after all, or was the state changed incorrectly?
mkrufky: 45.0x10^6 Hz => 45 MHz, 62.5x10^3 => 62.5 KHz
mchehab: agreed, but doing that would be a little harder, i guess
mkrufky: if you needed review before merge on that patch, the reviewer would likely ask for this before merge :-)
mchehab: (well, it would require to touch on a complex function)
<br> too late
<br> f29433ed5a0c..a4598f9310fc master -> master
<br> Making it smarter would require a redesign of dvb_fe_snprintf_eng()
<br> with is a very complex function
<br> this is used to display other stats
mkrufky: can you pass a char* to this function as an arg?
mchehab: we can teach it to use the metric system
<br> in order to replace the exponent
mkrufky: assuming yes, you can have a func that takes hz as int and returns a char * and you must free it after use
mchehab: (09:12:24) hverkuil1: mchehab: this https://patchwork.linuxtv.org/patch/49633/ is marked Accepted. Did you accept it after all, or was the state changed incorrectly?
<br> I don't remember if I merged this or not
hverkuil1: Doesn't appear to be the case, I just did a git pull of the master branch.
mchehab: no, I didn't
<br> yeah, it was incorrectly tagged. sorry for that
hverkuil1: that said, I really think this should be merged. There is nothing wrong with it.
mchehab: <u>mkrufky</u>: actually, the caller passes a char * buffer
mkrufky: so then it sounds like it would be easy to make this change :-)
mchehab: it doesn't know units, but it should not be that hard to teach it to use k, M, G, m, micro, nano, ...
<br> but this function is exported through the API
<br> so, we'll need to use some noname strategy to avoid breaking binary compat
<br> I'll think a little bit about that
mkrufky: what if, instead of changing every tuner range to HZ, instead, to add a new field to indicate the magnitude. This can be Hz, KHz, MHz, etc
<br> this way, the ranges could remain unaltered
mchehab: hverkuil1, syoung & others: sorry for the somewhat technical discussions
mkrufky: and userspace can remain unchanged
mchehab: I'll think about it
mkrufky: although the patch as-is may solve a problem, it might be better to solve it without affecting userspace
mchehab: I think I'll teach it internally to know about this:
<br> https://image.slidesharecdn.com/systemofunitssistandardinternationalbritishcgsfpstemperaturepressuregasconstantboyleslaw-151010102044-lva1-app6891/95/system-of-units-7-638.jpg?cb=1444473917
<br> and make a separate function call if one wants to use the new way
mkrufky: that looks like overkill but maybe would be awesome
<br> but re the kernel side of things, i'd lean towards the extra field indicating magnitude rather than breaking userspace apps
mchehab: the uAPI didn't chage... it will keep returning kHz for Satellite, Hz, for non-satellite
mkrufky: meanwhile, im not saying all userspace apps would break - but if any of them read these fields, that will be broken - its risky
mchehab: there's a suptile change, though...
mkrufky: (subtle)
<br> :-)
mchehab: on applications that know how to switch the delivery system, if they use frequency ranges, it will need to call FE_GET_INFO after switching, in order to get the right information
<br> that is not the case of Kaffeine, as it doesn't use frequency ranges
<br> I don't know if other apps do that
<br> please notice that the subtile change will happen only after we start adding devices with universal frontends that support both -S and -T (or -S and -C) frontend
<br> it is somewhat obvious that, if you want to know the frequency ranges for a delivery system, you need to select it first
mkrufky: yes
mchehab: but apps may be assuming that all delivery systems are equal
<br> in terms of frequency ranges
<br> on such cases, once we add a device with both -S and -T, if the app doesn't trust the Kernel to validate the frequency range, such app will not work properly with both standards
<br> but...
<br> the cabling system usually is either be connected to a Satellite or to a Terrestrial/Cable
<br> so, changing delsys actually require manual intervention
<br> (that's actually not true for the JockerTV device -- it has two separate inputs)
<br> hverkuil1, syoung: anything else?
hverkuil1: yes
mchehab: it seems that pinchartl and sailus aren't there
hverkuil1: I'm still waiting for a review of this patch series from pinchartl: https://www.mail-archive.com/linux-media@vger.kernel.org/msg133178.html
<br> It's been Acked by sailus, and I believe you are OK with it as well?
mchehab: yes, I'm ok
hverkuil1: I really, really want to get the first four patches in for 4.19. Without it the G_TOPOLOGY is effectively unusable for complex camera pipelines.
mchehab: well, if pinchartl won't be able to review it in time, let's assume lazy ack and move on
hverkuil1: I suggest that if there is no review when I'm back from vacation in about a week and a half, that I just make a pull request for it.
mchehab: according to our rules, there are enough acks for core changes
<br> works for me
<br> but I need to mention something:
<br> I'll be in KR between Jul, 13-22
<br> under a very restricted network
<br> I probably won't be able to apply patches along that week
<br> (nor participate on this meeting)
syoung: <u>mchehab</u>: nothing from me, other than I'm hoping you don't mind applying the fixes PR for v4.18 I sent yesterday
hverkuil1: OK. You know what: I'll post the pull request tomorrow late in the day before I start my week vacation.
<br> You can decide when to apply.
mchehab: ok
hverkuil1: That leaves this patch: https://patchwork.linuxtv.org/patch/49633. I've changed the status, but we're waiting for feedback from you.
<br> If you can look at it again today/tomorrow, then that would be great.
<br> That's it from my side.
mchehab: I should check how standard selection happens with omap3 devices with tvp5150 and tvp5151
<br> I guess I'll ping Javier, as he worked on a company that manufactures such devices
<br> he may know better how this currently works
<br> my main concern is just to avoid breaking something
<br> if this is not the case, I'm ok with the patch
hverkuil1: it only works with omap3 as long as you don't need to change standards. There is just no support for that at all.
<br> git grep \\\.s_std drivers/media/platform/omap3isp/ gives you nothing.
mchehab: I remember Javier mentioning that changing the standard works
<br> but that was a very long time ago
<br> anyway, just forwarded the path to him, c/c everyone
<br> let's wait for his answer
<br> feel free to ping him if you want to fast-track it
<br> perhaps you're right and setting standard depends on an OOT patch
<br> if this is the case, we can go ahead and apply it
<br> as we don't care not breaking OOT stuff :-p
hverkuil1: Argh, tvp5150 does autodetect of the standard (line 1519 of tvp5150.c) which is out-of-spec: this should never be done since this can change the required buffer size.
<br> That's why it works.
mchehab: yes, by default, it autodetects
<br> if I remember well, this won't change the buffer size... tvp5150 can scale
<br> but it was a long time ago since I looked on their datasheets
<br> not 100% sure if it can upscale vertical resolution
hverkuil1: Regardless, this is not common behavior and for other i2c devices this will not work.
<br> The normal sequence is to either explicitly select the standard with S_STD or do S_STD(QUERYSTD), but QUERYSTD is unfortunately optional.
mchehab: if I remember well, there is a problem with tvp5150... some standards only work if standard autodetection is enabled
<br> (the less common ones, like PAL-NC, PAL60 - but I don't remember the exact details anymore)
<br> I guess tvp5150 was written before we decided to do S_STD(QUERYSTD)
<br> If I remember correctly, tvp5150 only autodetects standard on autodetection mode
<br> so, a straigth forward implementation of QUERYSTD is probably not feasible there
<br> the driver would need to switch to auto mode, wait for a while, and then return from QUERYSTD
hverkuil1: <u>mchehab</u>: I think there is at least one other driver that does exactly that.
mchehab: (again, I may be wrong... I don't look on their datasheets for a long time)
hverkuil1: That only works if you are not streaming, otherwise QUERYSTD returns EBUSY.
mchehab: if you have some time and want to implement that, feel free to do so... unfortunately, I don't have any universal analog RF generator here for testing it
<br> (by the time I wrote the code, I remember I used an old camera that was able to generate some weid output formats, like PAL60)
<br> that camera is a long gone
hverkuil1: I have no interest in hacking that driver :-)
mchehab: :-D
<br> I was imagining that you would be answering something like that
<br> I would keep than that tvp5150 unusual behavior
<br> s/than/then/
<br> as touching it right now would require some time and some testing environment
syoung: btw I don't have any rc-core changes planned, other than a few very minor things. I'm working on the user-space BPF changes, and I will probably need some kernel BPF changes as well, so that's what I'm working on right now.
mchehab: <u>mkrufky</u>: change applied. it will now display:
<br> Frequency range for the current standard:
<br> <u>From</u>: 45.0 MHz
<br> <u>To</u>: 860 MHz
<br> <u>Step</u>: 62.5 kHz
<br> <u>syoung</u>: ok
<br> metric multiplying factors added from +24 to -24 (only the muliples of 3)
<br> out of that range, it will print like 1^27
<br> Now, it will also print the symbol range for cable or satellite:
<br> Frequency range for the current standard:
<br> <u>From</u>: 950 MHz
<br> <u>To</u>: 2.15 GHz
<br> <u>Tolerance</u>: 5.00 MHz
<br> Symbol rate ranges for the current standard:
<br> <u>From</u>: 1.00 MBauds
<br> <u>To</u>: 45.0 MBauds
<br> v4l-utils patches uptreamed already
mkrufky: nice
pinchartl: <u>hverkuil1</u>: pong
hverkuil1: <u>pinchartl</u>: just read back. I was just wondering if you will have time at all to look at my 'MC inconsistencies' patch series. Sakari and Mauro are OK with it.
pinchartl: hopefully tomorrow morning, otherwise on Monday afternoon while waiting at the airport
hverkuil1: OK. If I don't see any comments by Monday, then I'll post a pull request on Tuesday.
***: ChanServ sets mode: +v hverkuil
<br> ChanServ sets mode: +v hverkuil