<!-- 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>
***: benjiG has left
<br> benjiG has left
pinchartl: <u>hverkuil</u>: I'll reverse the two cases in the documentation patch, as I'm running out of time
<br> regaring the -ENOSPC error code, I've replied to your review
<br> please let me know what you think
mchehab: <u>pinchartl</u>: patch looks simple enough to be merged on early -rc
<br> send me a v2 with Dave's ack and I'll add to 4.11-rc pile of patches
pinchartl: <u>mchehab</u>: thank you. I'll add kerneldoc to the new structure as it was missing, apart from that it will be unchanged
mchehab: ok
<br> I'll likely be sending the next pull request tomorrow or Wed
pinchartl: I'll send v2 today
mchehab: ok, good!
<br> btw, with regards to that documentation patch you're discussing with hverkuil, I agree that you should start with the simple cases first... I tried to do a quick read yesterday, and also found it confusing ;)
pinchartl: I'll do that
<br> let's reorder the two cases later when the majority of drivers will implement the more powerful case :-)
<br> regarding the -ENOSPC error code when calling QBUF, Hans pointed out that drivers return -EINVAL today
<br> I replied that we should have used -ENOSPC
<br> but it's too late
mchehab: i guess the majority of drivers will keep not supporting buffer size changes - as this is not a need for non-SoC drivers
pinchartl: I don't think it would break the API though
mchehab: yes, it is likely too late
pinchartl: quoting my e-mail
<br> "We should have used -ENOSPC, and I believe that switching to -ENOSPC now wouldn't result in any breakage. Can you think of any application that would queue a too small buffer, match the error code against -EINVAL, and have a strategy to recover ? Queuing a too small buffer is an application but in the first place, I don't think application developers will have gone through the trouble of implementing a way
<br> to recover from their own bug instead of fixing them :-)"
mchehab: we had already cases of userspace breakage due to return of different error codes
pinchartl: how about documenting -ENOSPC and mentioning that some existing drivers return -EINVAL ?
<br> yes, I know
<br> but in this case I don't think any application would check the exact error code for that ioctl to implement a recovery strategy
<br> (be back in 15 minutes)
mchehab: maybe, but hard to be sure
<br> (07:44:16) pinchartl: how about documenting -ENOSPC and mentioning that some existing drivers return -EINVAL ?
<br> IMHO, that would be worse
hverkuil: Replied. I agree with Mauro here, drivers have been returning -EINVAL for ages in this specific case, and I see no compelling reason to suddenly change this.
<br> As long as a consistenterror is returned, that's the most important thing. And due to historical reasons that's EINVAL in this case.
mchehab: if it wouldn't cause any regressions (with we don't know), having a different error code could help userpace to be aware about the cause of the error
<br> but, in any case, it should be consistent on all drivers
<br> and should not cause regressions
<br> it is likely too late to change the error code
hverkuil: I really don't think it is important enough to make the effort. And since this *always* returned -EINVAL I think it is too late as well.
<br> I tend to be a bit more flexible with ABI changes than Mauro :-) but here I agree with him.
mchehab: :)
***: benjiG has left
pinchartl: <u>mchehab</u>: hverkuil: the problem with EINVAL is that we use it for too many error causes. applications can't tell whether the ioctl failed because the buffer is too small or from dozens of other causes
<br> but there's no urgency to change that now, it could be done later
<br> the situation won't change in a month, it's been that way for a long time already
mchehab: then let's postpone this decision
pinchartl: agreed
mchehab: by the time we'll discuss this again, it would be great if you could double-check if the main available V4L2 apps would handle it without regressions
<br> (tvtime, xawtv, vlc, qv4l2, MythTv, camorama, gstreamer, ...)
<br> libv4l2
pinchartl: of course
<br> it's a bit hard to check though
<br> in the sense that there's no code path that is supposed to trigger the problem
<br> apps don't queue too small buffers on purpose
<br> v4l2-compliance could, but normal applications don't
<br> that's why I don't think any of them will care
<br> - if an application never queues too small buffers, it won't care about the change in the return code as it will never see it
mchehab: <u>pinchartl</u>: you would be surprised to see how old apps do things :-)
<br> once I got one application that were first allocating buffers, then changing the image size
pinchartl: - otherwise, if the application doesn't discriminate between error codes (e.g. if (ret < 0) without checking the value of errno), it won't care
mchehab: amazingly enough, this worked with bttv driver, as it were always allocating a big buffer, even for small images
<br> until some patch fixed it, causing the app to break
pinchartl: so, only applications that check the error code (e.g. if (errno == EINVAL) or if (errno != EINVAL)) could be affected, and only if they queue too small buffers
<br> I don't think those applications have an error path such as
mchehab: yes. you have a pattern to check :-)
hverkuil: I've never seen anyone complain about not being able to tell whether EINVAL was because of too small a buffer or something else.
pinchartl: if (errno == EINVAL) { queue_a_bigger_buffer(); }
<br> if, as an application writer, I realize that QBUF fails because the buffer is too small, I'll fix it by making the buffer large enough
hverkuil: Changing to ENOSPC is a lot of work (verification, changing existing drivers) for very little benefit. Just my opinion.
pinchartl: not by adding code that catches the error and retries with a bigger buffer
<br> <u>hverkuil</u>: I've pestered many times about -EINVAL being so vague that it required me to add printk's to the kernel to debug an application
mchehab: <u>hverkuil</u>: changing the drivers to return the error code that comes from VB2 instead of replacing by -EINVAL is something that would make sense
<br> (provided that it won't break userspace)
pinchartl: I think it would make sense to move the size check to vb2
<br> it has to be done by all drivers
<br> vb2 is not format-aware
<br> and I don't think it should be
<br> but we could have a vb2 call to set the minimum size
<br> drivers would call it in their s_fmt handler for instance
<br> that way the size check would be consistent
<br> but that's also a separate issue from the documentation patch
<br> I've identified a few similar issues related to buffer handling and error management
mchehab: <u>pinchartl</u>: that sounds more like an ancillary function
pinchartl: I'll try to find time to summarize and address them in the future
mchehab: but such function would need to be aware of the format
pinchartl: it doesn't need to be aware of the format
<br> just of the size
<br> I don't think vb2 should become format-aware
<br> but it should probably be size-aware
<br> as sizes very much relate to buffers
mchehab: I would implement such function at V4L2 core
<br> as it is not really VB or VB2 related
<br> but, if we're willing to implement it, I would put there the logic that gets bits per pixel into it
<br> that would remove a lot of static tables from drivers
<br> but the thing is that it is complex patch
<br> once I tried to write something like that
pinchartl: the problem is that the conversion between width/height/pixelformat to bytesperline and sizeimage can be device-specific, so we can't move it all to a central location
mchehab: but I gave up, because there would be too many things to change
pinchartl: I'd like to have a V4L2 core helper that performs that job in the common case
<br> it would return a size
<br> which could then be passed to vb2
<br> vb2 should handle the size only, and the V4L2 core should take care of resolution/pixelformat
mchehab: that's not device-specific. A given format has a BPP
pinchartl: the padding at end of line can be device-specific
<br> which influences the image size
mchehab: yes, but such logic is easy to handle
<br> drivers may pass to the help function the alignment constraints
<br> e. g. something like: bytesperline should be multiple of 32 bits, sizeimage should be multiple of 64 bits
pinchartl: there are multiple options
<br> my point is that such logic shouldn't be in vb2
<br> but vb2 could be made size-aware
mchehab: <u>pinchartl</u>: that should be a helper function that drivers will call as part of their logic
<br> they may do additional constraints
pinchartl: anyway, not a job for today :-)
<br> I'll resubmit the documentation patch
<br> with -EINVAL
mchehab: OK
pinchartl: <u>mchehab</u>: hverkuil: regarding ENOSPC vs. EINVAL
<br> let's not change the current behaviour
<br> but for the new proposed scheme that doesn't require buffers to be reallocated
<br> shouldn't we go for ENOSPC ?
<br> that won't break userspace as currently we don't support format changes while buffers are allocated
__raven__: hi
<br> i would like to use this https://www.linuxtv.org/wiki/index.php/DVBSKY_S960 on a debian system without gui. which apps would be useful for this purpose?
mchehab: dvbv5-tools
<br> <u>__raven__</u>: https://www.linuxtv.org/wiki/index.php/DVBv5_Tools
pinchartl: <u>mchehab</u>: hverkuil: here's the current proposal (easier to read than a patch) http://ideasonboard.org/kerneldoc/media/uapi/v4l/buffer.html
mchehab: <u>__raven__</u>: I'm planning to allow using Kaffeine to connect to a remote hardware without GUI, but patches are not ready yet
<br> there are some apps that allow you to connect to a headless hardware too, like dvr and tvdaemon
<br> <u>pinchartl</u>: what do you mean by "new proposed scheme that doesn't require buffers to be reallocated"?
<br> VIDIOC_STREAMOFF | VIDIOC_S_EXT_CTRLS | VIDIOC_S_FMT | VIDIOC_QBUF | VIDIOC_STREAMON ?
pinchartl: yes
<br> that one isn't supported today by drievrs
<br> drivers
<br> so it's not used by applications :-)
mchehab: thinking about how you would code it, that seems like a hack :)
<br> as you'll need to track, per file descriptor, if STREAMON was called before, in order to enable returning ENOSPC
pinchartl: hmmmm...
__raven__: <u>mchehab</u>: hm yes a remote tuner would be the other option but i do not want to have huge overhead by installing mythtv or such things
<br> i will look at the articles at them moment
sailus: mchehab, pinchartl: Would you need to do something beyond checking the buffer size (QBUF/PREPARE_BUF) against the current format, and if it's too small, return -ENOSPC?
pinchartl: <u>sailus</u>: if you want to enhance an existing driver to support the new scheme (STREAMOFF - S_FMT - QBUF - STREAMON), how do you tell in the .buf_prepare() handler whether you need to return -ENOSPC or -EINVAL ?
<br> -EINVAL needs to be returned in case the application is using the old scheme
<br> it could be done based on whether buffers are allocated
sailus: Can the condition happen in the old scheme in the first place?
hverkuil: <u>sailus</u>: yes: with dmabuf/userptr.
sailus: <u>hverkuil</u>: Right.
pinchartl: if (buffer is too small) return vb2_is_busy() ? -ENOSPC : -EINVAL;
<br> but that would be weird I think
hverkuil: nack.
pinchartl: <u>hverkuil</u>: don't nack my idea before I have a chance to tell myself it's not good :-)
sailus: I have no problem with -EINVAL, however -ENOSPC would be nicer as -EINVAL is used to tell about a lot of different conditions.
hverkuil: Sorry :-)
sailus: Comparing this to reality of error code documentation --- we didn't have EBUSY documented for VIDIOC_S_FMT until I wrote a patch for it a few days ago.
pinchartl: <u>hverkuil</u>: I've updated http://ideasonboard.org/kerneldoc/media/uapi/v4l/buffer.html, could you check it ?
hverkuil: error documentation is a bit hit-and-miss.
sailus: Were applications relying not to ever receive -EBUSY from VIDIOC_S_FMT?
<br> Was the code wrong or the documentation? :-)
hverkuil: doc is wrong (or more precisely: incomplete)
pinchartl: <u>sailus</u>: on top of http://ideasonboard.org/kerneldoc/media/uapi/v4l/buffer.html I also have a small patch that uses -ENOSPC for the new scheme. I can fold it in if we agree on it, but let's first agree on the rest :)
sailus: That makes writing applications harder.
pinchartl: <u>sailus</u>: what does ? -EINVAL ?
sailus: <u>pinchartl</u>: Not documenting what error codes may be returned in different cases.
<br> Perhaps we should write some documentation on how the applications should handle errors?
<br> Unless that already exists.
pinchartl: it's a good point
<br> we should
<br> <u>hverkuil</u>: I hope you like this version better. the order matches what you've requested
sailus: If for nothing else, have less application breakages if something changes (through bugfixes, for instance).
<br> Or if new error codes are added. Of course we can't affect what application developers do in the end, but a little hint would do no harm.
pinchartl: applications could still handle errors in a non-compliant way and then break
sailus: Indeed.
pinchartl: but it would help avoiding those cases, I agree
sailus: I have no issues with EINVAL if ENOSPC seems unfeasible in practice.
<br> Still, not providing descriptive error codes deteriorates the quality of the API.
hverkuil: <u>pinchartl</u>: this version is much better, thanks.
sailus: hverkuil, mchehab: Your opinion on adding generic documentation on error handling?
hverkuil: <u>sailus</u>: sorry, not sure what you mean?
pinchartl: <u>hverkuil</u>: ok, I'll post it
mchehab: <u>sailus</u>: there *is* a generic documentation for errors
sailus: If there is, all the better. :-)
mchehab: https://linuxtv.org/downloads/v4l-dvb-apis-new/uapi/gen-errors.html
<br> and -ENOBUSY is there
<br> I mean: EBUSY
<br> this is (or should be) referenced on *all* ioctls
sailus: I meant documentation on how applications should handle errors. For instance, an application shouldn't check for a single error code and ignore the rest.
mchehab: see the end of: https://linuxtv.org/downloads/v4l-dvb-apis-new/uapi/v4l/vidioc-g-fmt.html
sailus: That could be one more note in the list for instance.
mchehab: First note at ghe gen-errors.html:
<br> 1. This list is not exhaustive; ioctls may return other error codes. Since errors may have side effects such as a driver reset, applications should abort on unexpected errors, or otherwise assume that the device is in a bad state.
pinchartl: (that's very harsh)
mchehab: if I remember well, we added such notice when KDE plasma broke due to an error at the VIDIOC_ENUM_* that were different than EINVAL
sailus: <u>mchehab</u>: Ack.
mchehab: if the notice there is not clear enough, feel free to add doc improvements :-)
sailus: Well, all the better if it's already there.
<br> I haven't read through every line of the documentation and application developers likely won't end up doing that either.
<br> People usually only read documentation when they run into problems. It's just like assembling Ikea furniture. :-)
larsc: if an app breaks due to not conforming to the recomendations it still breaking userspace, which is a big nono
<br> I think DRM solves this by having to have an app explicity opt-in to changed behavior
pinchartl: <u>larsc</u>: it depends how stupid the application is. I'm pretty sure I can write an application that exercises pretty much any kernel API in a way that will break when the internal implementation changes
larsc: yeah!
pinchartl: cfr. the trace point ABI discussion
<br> v2.3 sent
<br> <u>mchehab</u>: hverkuil: once I get your Ack I'll send a pull request
<br> <u>mchehab</u>: by the way, I plan to include the histogram API in the pull request
<br> Hans and Sakari have reviewed and ack it
<br> s/ack/acked/
mchehab: <u>larsc</u>: yeah, having a way to opt-in would be interesting
<br> some protocols at TCP/IP stack use this concept: there are messages to check if the protocol can do something, and messages to explicitly enable/disable the CAN features
pinchartl: take makes sense for big features, but for small changes it would quickly become awful to handle
mchehab: yes
__raven__: ok i have the dvbsky now running but where to get the satelite list from to scan with dvbv5-scan?
pinchartl: <u>mchehab</u>: I've posted "[PATCH v2] v4l: vsp1: Adapt vsp1_du_setup_lif() interface to use a structure"
<br> fully tested on v4.11-rc1, ready for v4.11-rc2
<br> <u>hverkuil</u>: thanks for your ack !
mchehab: <u>__raven__</u>: satellite tables are at the dtv-scan-tables
<br> https://www.linuxtv.org/wiki/index.php/Dtv-scan-tables
pinchartl: <u>mchehab</u>: would you like to send an ack for "[PATCH v2.3] v4l: Clearly document interactions between formats, controls and buffers" too or should I just send the pull request ?
__raven__: ok tnx
<br> last piece i need is a kind of framebuffer dvb player. do you know any?
mchehab: <u>pinchartl</u>: I'll review it later
pinchartl: <u>__raven__</u>: framebuffer dvb player... that sounds like asking for a wooden spaceship, or a steam-powered pacemaker :-)
<br> <u>mchehab</u>: should I send you the pull request already ? I'll leave on Thursday :-/
__raven__: <u>pinchartl</u>: you do not like wooden spaceships? ;)
pinchartl: <u>__raven__</u>: they have a tendency to burn, which is quite unfortunate if you happen to be within the aforementioned spaceship
mchehab: bbl
pinchartl: <u>mchehab</u>: I hope you haven't just realized that you're in a wooden spaceship
__raven__: the player just should work on a debian system which has no graphical environmend
<br> t
mchehab: <u>pinchartl</u>: I don't have any fb dvb player :-p
pinchartl: :-)
mchehab: I either use text-mode tools or qt5-based Kaffeine
<br> anyway, need to do some errands. bbl
__raven__: hm whats that ERROR FE_SET_PROPERTY: Invalid argument
pinchartl: the infamous -EINVAL error :-D
__raven__: ok i now have a scanned channel list. how to use it now?
<br> <u>pinchartl</u>: what does that error mean?
pinchartl: <u>__raven__</u>: we just had a discussion about how -EINVAL is very vague, my comment was just a reference to that
mchehab: <u>__raven__</u>: dvbv5-zap
<br> <u>pinchartl</u>: in this case, it means exactly Invalid argument
__raven__: <u>mchehab</u>: yes that brings me that error "ERROR: dvb_fe_set_parms failed (Invalid argument)
mchehab: one of the arguments for this ioctl has a wrong value
<br> enabling debug will help showing what values were passed to the Kernel
pinchartl: <u>mchehab</u>: I was kidding :-)
mchehab: yeah, I know :-D
larsc: but it is still pretty ambiguous, you have no idea which value was rejected
<br> but that is an issue with kernel error reporting in general
pinchartl: <u>mchehab</u>: do you need anything else from me for the rotation, histogram and "[PATCH v2] v4l: vsp1: Adapt vsp1_du_setup_lif() interface to use a structure" ?
mchehab: <u>pinchartl</u>: patch looks ok
<br> <u>larsc</u>: yes, standard error report at unix is ambiguous
pinchartl: <u>mchehab</u>: great, thank you ! I'll send a pull request then
<br> oops, sorry
<br> that one you said you would pick up
<br> it's for rotation and histogram that I'll send a pull request when you'll be ready
<br> (unless you would prefer me to send a pull request for that single patch too)
mchehab: <u>pinchartl</u>: better to send a pull request with this single patch with "GIT FIXES for 4.11" on the email subject
pinchartl: ok I'll do that, thanks
mchehab: for me to remember to give priority this one ;)
<br> GIT FIXES string is automatically handled by my scripts - preventing me to forget ;)
<br> I got sidetracked by some errands, so I may be applying it only tomorrow
pinchartl: no problem
mchehab: thanks
***: benjiG has left