14:24 < hverkuil> sailus: pinchartl: add a new field to specify an offset. It's something that crops up every so often, so we should just do it.
14:25 < hverkuil> In hindsight data_offset should have been called 'header_size' or something like that. It would have been a more descriptive and accurate name.
14:38 -!- simosx [~simosx@ubuntu/member/simosx] has joined #v4l
14:43 < hverkuil> what would be nice is if vb2 can accept v4l2_buffer structs with type _MPLANE even if the queue is single planar. That way an application can use struct v4l2_plane to pass on data_offset and buf_offset.
14:45 < hverkuil> Although that may be premature: API changes will be necessary in v4l2_buffer to fix the year 2038 problem (affecting the struct timeval), and that may be an opportunity to make a struct v4l2_buffer64 replacement where we rework the struct to make it more flexible.
14:57 < pinchartl> hverkuil: thanks for your comments
14:57 < pinchartl> adding a new field is easy
14:58 < pinchartl> Sakari's point, however, is that keeping adding new fields to implement single use cases might not be very wise in the long term
14:58 < pinchartl> he advocated for a new API instead, using frame descriptors
14:58 < pinchartl> regarding the y2038 problem, I agree with you
14:58 < pinchartl> we should tackle it in the not too distant future
14:59 < hverkuil> In my view these are different things. All you want to do here is tell the driver to not start at the beginning of a buffer, but at an offset, determined by the app.
14:59 < hverkuil> Nothing to do with frame descriptors.
14:59 < hverkuil> I've been asked about this before.
14:59 < pinchartl> I have nothing against a simple solution to my problem :-)
15:00 < pinchartl> but sailus might disagree
15:00 < pinchartl> should the offset be limited to DMABUF ?
15:00 < hverkuil> no, it should be general.
15:00 < pinchartl> it doesn't make much sense to use it for USERPTR, as you can just add the offset in userspace
15:00 < pinchartl> as for MMAP, it will only work with CREATE_BUFS
15:00 < hverkuil> It's probably easier to make it general then making exceptions.
15:01 < pinchartl> as it requires userspace to allocate buffers larger than the image size
15:01 < hverkuil> certainly.
15:01 < hverkuil> that's why we invented createbufs :-)
15:01 < pinchartl> :-)
15:01 < pinchartl> should vb2 try to be smart and compute base + offset for drivers, or should drivers add the offset manually ?
15:02 < pinchartl> I'm mostly thinking about vb2_dma_contig_plane_dma_addr()
15:05 < hverkuil> good question.
15:06 < hverkuil> I'm inclined to let vb2 handle it if possible.
15:06 < hverkuil> That way drivers don't need to be adapted.
15:07 < hverkuil> But drivers may have restrictions (e.g. the buffer must always start at a page boundary, which I know is true for saa7134).
15:07 < pinchartl> drivers must validate buf_offset at prepare time
15:08 < pinchartl> I'll need to check how to handle backward compatibility too
15:09 < hverkuil> I'm not sure if all drivers do that correctly. Especially if you give it a buf_offset of e.g. 1. That will normally never happen (although you can force it with USERPTR if supported).
15:09 < hverkuil> Certainly drivers using dma_contig will normally never encounter such a situation.
15:10 < pinchartl> what should drivers do if buf_offset is invalid ? round it to the closest supported value, or return an error ?
15:10 < pinchartl> I think returning an error makes more sense
15:10 < pinchartl> as a different value than the one requested by userspace doesn't make much sense
15:10 < hverkuil> Perhaps drivers should set an alignment mask in vb2_queue. If it is 0, then buf_offset is not supported (so this way the driver has to opt-in).
15:11 < pinchartl> that's a good idea
15:11 < pinchartl> I'll experiment with that
15:11 < hverkuil> They should return an error, yes.
15:12 < hverkuil> I wonder if it makes sense to have vb2 modify buf_offset to something valid.
15:12 < hverkuil> Probably not.
15:12 < pinchartl> the alignment mask should take care of most validation cases
15:12 < pinchartl> only non power of two constraints would need to be manually checked by drivers
15:12 < pinchartl> that shouldn't be the norm
15:12 < pinchartl> no, I don't think it would
15:12 < hverkuil> yes, and it would simplify drivers as well since they no longer need to test for this.
15:13 < pinchartl> if userspace sets a buf_offset value it will likely not work with any other value
15:13 < pinchartl> there could be an exception though
15:14 < pinchartl> userspace might prefer using buf_offset, with a possibility to fall back to two separate buffers if buf_offset isn't supported
15:14 < pinchartl> so a way to detect that buf_offset isn't supported is needed
15:15 < hverkuil> If it is not supported, then set it to 0, otherwise leave it as is.
15:15 < sailus> Hello!
15:16 < pinchartl> do you mean set it to 0 and return success ?
15:16 < hverkuil> no, set to 0 and return an error
15:17 < hverkuil> so:
15:17 < hverkuil> if buf_offset is supported and the alignment is OK, return 0
15:17 < pinchartl> if I'm not mistaken we don't copy_to_user when an ioctl returns an error
15:17 < hverkuil> if buf_offset is supported and the alignment is wrong, return an error, leaving buf_offset unchanged.
15:17 < hverkuil> if buf_offset is not supported, then set to 0 and return an error.
15:18 < hverkuil> actually, that depends on the ioctl. Some do copy_to_user, even on error.
15:18 < pinchartl> ok
15:19 < pinchartl> it seems that only VIDIOC_QUERY_DV_TIMINGS can do that at the moment
15:19 < pinchartl> (as implemented in video_usercopy)
15:19 < pinchartl> ah, no
15:19 < pinchartl> array args are always copied
15:19 < hverkuil> right.
15:20 < pinchartl> perfect :-)
15:20 < pinchartl> that should ensure backward compatibility
15:21 < hverkuil> I think a new flag should be added to the v4l2_ioctls table when we get more ioctls that have to copy the result to userspace, even on error.
15:21 < pinchartl> kernels without buf_offset support will set it to 0 and return success
15:21 < sailus> I'm fine with adding buf_offset field, as better solutions would probably take quite some time to implement.
15:22 -!- iive [~iive@87.119.101.204.client.entry.bg] has joined #v4l
15:22 -!- iive [~iive@87.119.101.204.client.entry.bg] has quit [Changing host]
15:22 -!- iive [~iive@unaffiliated/iive] has joined #v4l
15:22 < sailus> Just bear in mind that there will be other similar use cases, and they will most probably need other kind of a solution.
15:22 < hverkuil> Do you need buf_offset in the single buffer case as well? We don't really have space for that.
15:22 < pinchartl> I don't need it in the single buffer case
15:22 < pinchartl> my use case is capturing NV12 into a single dmabuf
15:22 < sailus> Thinking about a proper solution...
15:22 < pinchartl> using mplane on the capture driver side
15:23 < pinchartl> sailus: I'm of course open to proposals for alternative solutions
15:23 < sailus> Multi-plane buffer with header as a separate plane would solve the problem.
15:23 < sailus> But formats are specific to the node, not a plane.
15:24 < pinchartl> sailus: I'm not sure to understand what you mean
15:24 < pinchartl> we're not talking about headers here
15:24 < sailus> We'd need plane specific formats. That way we could also tell the user what kind of a header it'd be as it'd have its own format.
15:24 < sailus> Oh, indeed. Your case doesn't involve a header.
15:25 < sailus> But if there was a header, a multi-plane buffer should be used to tell the header is there. And the header would have a separate plane.
15:25 < pinchartl> we've discussed offsets several times face to face, and it seems like each time we can't understand each other :-)
15:25 < sailus> You'd manage with just data_offset in that case, in theory. But I have to read what's said of the data_offset field so this wouldn't re-purpose it too badly.
15:25 < sailus> :-D
15:26 < hverkuil> pinchartl: for your use case you do need to map just part of a dmabuf for one plane, and another part of a dmabuf for another plane. And that may overlap if the boundary of the two planes doesn't fall on a page boundary. Does that even work?
15:28 < pinchartl> I'd have to check that, but I don't think it should be an issue
15:29 < hverkuil> Hmm, I'd start with that if I were you.
15:29 < pinchartl> where do you see a potential issue ? in the dmabuf map API ?
15:29 < hverkuil> yes
15:30 < hverkuil> I don't think anyway ever needed this, so I wonder if there is even support for this.
15:31 < pinchartl> I'll test it :-)
15:32 < sailus> pinchartl: See this simple example:
15:32 < sailus> https://etherpad.fr/p/plane-header-v4l
15:32 < sailus> That's with a header you don't have, so in that case you simply wouldn't have plane 0.
15:33 < sailus> But plane specific formats would be needed, and those we don't have now. I'm not sure whether the current multi-plane implementation could be meaningfully extended for that. There aren't too many planes either.
15:33 < hverkuil> sailus: I have thought about per-plane formats. It was planned originally, but I am not certain if it can be retrofitted today. It would certainly take some research.
15:34 < hverkuil> We should have enough planes. The max is 8 and worst case (without headers) would be 4 planes (alpha, r, g, b or alpha, y, cb, cr), leaving 4 more for headers.
15:34 < pinchartl> sailus: in your example, data-offset for plane 1 actually depends on the memory method
15:34 < pinchartl> for mmap the driver will allocate plane separately
15:35 < pinchartl> so data-offset should be 0
15:35 < pinchartl> for userptr and dmabuf, it could be != 0
15:35 < sailus> I'd have use for around ten right now. But the use case is a bit different, and we should discuss the details of that later.
15:36 < sailus> pinchartl: The example assumes dma-buf.
15:36 < pinchartl> 10 planes ?
15:36 < pinchartl> in that case I'm not sure we should be talking about planes
15:37 < pinchartl> it's planes and meta data buffers
15:37 < pinchartl> I assume there will be configuration (in and out), statistics, ...
15:37 < hverkuil> sailus: I'm definitely open to a good proposal for per-plane formats, but you'll have to do the research :-)
15:37 -!- simosx [~simosx@ubuntu/member/simosx] has quit [Quit: Leaving]
15:37 < sailus> v4l2_pix_format_ext_mplane? :-)
15:37 < pinchartl> sailus: that's for a driver that will never be upstreamed, isn't it ? :-)
15:38 < sailus> That's for a driver that will be upstreamed.
15:38 < sailus> The schedule is open though, and does not depend on me.
15:39 < sailus> Let's assume a hardware ISP which takes per-frame configuration from the user space, as in Android camera API v3.
15:40 < sailus> Everything is per-frame, and it's important that the correct settings are used when capturing a frame.
15:40 < pinchartl> the plan is to use the control framework for this
15:40 < pinchartl> but
15:40 < pinchartl> when you have 100k of register blob data that needs to be passed around for each frame, that becomes an issue
15:40 < sailus> The configuration is kilobytes in size. This includes some tables but also quite a few individal configuration options, which also includes the dimensions of those tables.
15:41 < sailus> I'd be fine with controls but performance is by far my biggest concern.
15:41 < hverkuil> sailus: use the control framework and the proposed request API for that. Latest code is here:
15:41 < pinchartl> it definitely needs to be tested from a performance point of view
15:41 < hverkuil> http://git.linuxtv.org/cgit.cgi/hverkuil/media_tree.git/log/?h=requests
15:42 < pinchartl> sailus: would you be willing/able to check the performances ?
15:42 < sailus> I wonder whether this is feasible in embedded systems.
15:42 < hverkuil> If you have really large amount of data (and no, I don't know where the boundary between large and not-large lies), then you need a DMA solution.
15:42 < sailus> Already *copying* the data is something that might show up in performance measurements.
15:43 < sailus> Let alone copying it around a few times, calling a bunch of functions and doing a large amount of checks for every single value.
15:43 < pinchartl> sailus: I think it needs to be measured
15:43 < hverkuil> But that only makes sense if the hardware can actually DMA that data. If it just copies the data in a buffer, then there is no or little performance gain between using G_EXT_CTRLS or DQBUF.
15:43 < hverkuil> You have to measure first.
15:43 < sailus> Remind you, this is not something that's configured by a test program with a nice UI, but calculated by an algorithm library in the user space.
15:44 < hverkuil> Note: I plan to clean up the request branch soon and post it. But it is in pretty good shape already.
15:45 < sailus> With mmap buffers, there's no need for the driver to even touch the data, other than to validate that the dimensions etc. are correct.
15:45 < hverkuil> sailus: really, measure performance first. Try with the request API, try using planes, then compare.
15:47 -!- spikesmydog [~Richard@5ec166a6.skybroadband.com] has joined #v4l
15:47 < hverkuil> I really don't like the idea of using planes to pass per-frame request data as it is just an opaque blob. Whereas with the request API it's a well-defined data structure.
15:47 -!- physis [~physis@191.33.124.14] has joined #v4l
15:47 < sailus> hverkuil: The format of the data would of course be documented.
15:47 < sailus> hverkuil: I can do the measurements. This will take some calendar time.
15:47 < pinchartl> hverkuil: I think I agree with you, but if performance issues occur, they need to be addressed
15:47 < pinchartl> we need numbers first
15:48 < pinchartl> hverkuil: regarding "[PATCH] uvc: fix cropcap v4l2-compliance failure"
15:48 < hverkuil> Frankly, I rather spend time on improving the control framework performance than hacking it as a plane.
15:48 < sailus> I don't think it's feasible to perform validation for every single value in the configuration.
15:48 < sailus> But we'll see.
15:48 < pinchartl> should I take it in my tree or do you have a branch with pending patches in which you would like to include it ?
15:49 < hverkuil> sailus: just to be explicit: I'll nack patches that do this as a plane, unless backed up by hard numbers.
15:50 < sailus> hverkuil: Ack.
15:50 < hverkuil> sailus: validation only happens when you set it, not when you retrieve it. And when you pass in the data from userspace you will need to validate it regardless of the method used.
15:51 < hverkuil> sailus: and yes, currently when the kernel sets a control value it is validated as well, but if that's a bottleneck then that can be optionally disabled (it seems fair to assume that the kernel know what it is doing).
15:52 -!- physis [~physis@191.33.124.14] has quit [Ping timeout: 265 seconds]
15:52 < sailus> The user space will set if for every frame. There's generally no need to get it back, as the user space already knows which settings it used.
15:52 < pinchartl> hverkuil: I'll take it in my tree :-)
15:52 < hverkuil> pinchartl: you take it.
15:52 -!- Nikhil_D [a0132237@nat/ti/x-elbjjjqvzsbbpoqf] has quit [Remote host closed the connection]
15:53 -!- Nikhil_D [a0132237@nat/ti/x-burhekqrqhnjbjfu] has joined #v4l
15:55 < hverkuil> pinchartl: is the patch adding query_ext_ctrl support to uvc part of a pull request from you, or do you still have to make that pull request?
15:55 < pinchartl> I still have to make it
15:55 < pinchartl> I'm reviewing the patch right now
15:55 < hverkuil> ah, OK. Just checking :-)
15:55 < pinchartl> are you going to send a v2 of the embed video_device series ?
15:56 < hverkuil> no, most are merged and I posted a pull request for the remaining patches (including the uvc one) after testing it.
15:56 < pinchartl> ok
15:57 < pinchartl> no need to ack the patches then :-)
15:57 < hverkuil> https://patchwork.linuxtv.org/patch/29089/
15:57 < pinchartl> how about uvc gadget ?
15:57 < hverkuil> I wanted to test the cx88, bttv, em28xx and uvc patches first before adding them to a pull request due to the popularity of those drivers.
15:58 < hverkuil> I think the uvc gadget patch has already been merged.
15:58 < pinchartl> ok
15:58 < hverkuil> yes, it has
15:59 < hverkuil> 12 more drivers to convert, then video_device_alloc can be removed.
15:59 < pinchartl> \o/
15:59 < pinchartl> we need to address the devm_kzalloc() issue too
15:59 < pinchartl> do you plan to send a mail to lkml about that, or should I do it ?
16:01 -!- spikesmydog [~Richard@5ec166a6.skybroadband.com] has quit [Quit:  Want to be different? Try HydraIRC -> http://www.hydrairc.com <-]
16:01 < hverkuil> It's lower on my todo list. After all, nobody ever complained. If you want to do it, then I certainly won't stop you. I'd rather work on the request API patches, that's more important (at least to me).
16:01 < pinchartl> ok, I'll try to do it then
16:01 < pinchartl> I'll CC you
16:01 < hverkuil> OK, great. Thank you!
16:02 < pinchartl> time to go to the gate
16:02 < pinchartl> (yes, I'm flying again)
16:02 < pinchartl> have a nice weekend
16:02 < pinchartl> and go easy on the easter chocolates eggs :-)
16:03 < hverkuil> have a good flight!