<!-- 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>
sailus: Btw. if the user tries to pick an unsupported format, the driver should revert to a known format, not return an error.
***: funman has quit IRC (Ping timeout: 260 seconds)
neg: <u>sailus</u>: I can't find any image size restrictions in the documentation for the R-Car CSI-2 receiver, the R-Car VIN have a restriction on 4096x4096 but can't find anything similar for the CSI-2 recivier.
<br> <u>sailus</u>: The limitation which is documented is the max Mbps / lane which for most Renesas Gen3 boards are 1.5 Gbps / lane. This is validated at s_streme() time as it's quiet hard to do so in set_fmt(). Especially if we start to think about multiplexed streamse which is the next step for rcar-csi2
<br> <u>sailus</u>: and the comment about the format is correct, I will fix that in a follow up patch thanks for noticing.
<br> <u>sailus</u>: Thanks for taking the driver in your tree, you made my day it's been floting around ML for over a year :-)
sailus: <u>neg</u>: You're welcome!
<br> If you submit a fix for that format issue now, I could merge it to the driver patch.
neg: <u>sailus</u>: is it ok if I post such a patch later today/this evening?
sailus: <u>neg</u>: Sure.
<br> If it's more than 1½ hours from now, then I'll check it out only late this evening.
<br> <u>hverkuil</u>: I've pushed my changes on your reqv13 branch to my linuxtv.org repository.
<br> The branch is "reqv13+".
hverkuil: I see it.
sailus: A few of the patches have been chagned --- there are notes in brackets in those. Some have been added as well, a least a few of these are probably better merged to the originals.
<br> Should I post the entire set to the list, or would you prefer me to target the patches against your v13?
<br> If you'd be happy to continue with this as such, then the former would likely be a better option.
<br> Well, the usual review still needs to take place, of course.
<br> <u>hverkuil</u>: I noticed one bug, I'll fix it and re-push.
hverkuil: OK. There is a bit more time since the fence patch series needs to go in before the Request API.
sailus: Ack.
<br> Note that I haven't reviewed the few latter patches in the set yet, just changed the scheme for updating requests before they get queued.
<br> I presume the v4l2-m2m implementation of requests is somewhat opportunistic still.
hverkuil: No, I think its pretty much done. It's been tested, it works.
sailus: I guess some of this could be addressed later on, but I'm certain that allowing processing mixed request-bound and non-request-bound buffers will cause trouble going forward.
<br> It's hard to say how much though.
<br> I think you've probably tested a limited set of what is enabled by the API in ways that you think should work.
inigo: <u>hverkuil</u>: I'm porting my driver to videobuf2 as you recommended. I'm using vivi driver port to videobuf2 as reference
hverkuil: for stateless codecs it works fine to mix. Whether it makes sense for complex HW is a different matter.
inigo: I'm looking here: https://patchwork.linuxtv.org/patch/4591/
sailus: If you mix requests with non-requests, and queue the buffers in a random order, what will happen?
hverkuil: <u>inigo</u>: you mean vivid? vivi is very old and was removed from the kernel a long time ago.
<br> <u>inigo</u>: oh, you use the patch as a template. That's OK.
inigo: well, I mean vivi, it is what is with my kernel version (remember I'm with version 3.4)
<br> In that patch vivi_read function is removed, but in my kernel it is still there
<br> Where can I see all commits made to vivi?
<br> Or if you think I should use another reference, please tell me
hverkuil: <u>sailus</u>: when a buffer without an associated request is processed it just uses the current control settings. It's as if you have a request of just that buffer and nothing else.
sailus: Yes, but the request is ignored.
<br> The fact is that the implementation ignores the requests for video buffers.
<br> I wouldn't rely on the user space on that.
hverkuil: <u>sailus</u>: What request? There is no request. The buffer is just filled as usual and you can dequeue it. Userspace decided to queue a buffer without a request, so I assume userspace knows what it is doing.
<br> <u>sailus</u>: Note that I am perfectly fine with adding restrictions on this when the first driver is submitted. This is just at the low level vb2 framework where I want to allow it (for now), we can restrict this at the higher levels.
sailus: The point is that we must ensure the coherency of kernel internal data structures in the kernel, we may not rely on the user space doing that.
<br> What is being now is that request-bound buffers are not processed as such, and I presume no-one has even tried to figure out what exactly happens as a result.
hverkuil: <u>sailus</u>: I don't know what you mean. As long as a request containing buffers isn't queued, the buffer just sits there in the 'IN_REQUEST' state. Once the request is queued they are fed to the driver as usual. It's similar to VIDIOC_PREPARE_BUF: the buffer is prepared (buffer is locked into memory), but it is not queued to the driver.
<br> <u>sailus</u>: in other words, when buffers are queued to the driver (whether or not they are part of a request) the internal vb2/v4l2-ctrl data structs are completely consistent/coherent.
sailus: Nothing guarantees that in the kernel, the current implementation rather relies on the user space to queue buffers in such an order.
hverkuil: I have a feeling there is some miscommunication here.
<br> When you call VIDIOC_QBUF with a request_fd, then the buffer is added to the request. It is not queued to the driver. That only happens when you queue the request itself.
sailus: But the driver (as well as v4l2-mem2mem) processes buffers, not requests.
<br> That's the problem.
hverkuil: It processes buffers and some (or all) buffers have requests associated with them.
sailus: The buffers it processes together may or may not be related to the same request.
hverkuil: so?
sailus: Don't you see that as a problem?
hverkuil: No? What am I missing?
sailus: The very reason we have requests is that we can associate together bits that generally are orthogonal.
<br> That's what fails here.
hverkuil: Can you give a specific example? (Still no idea what you mean...)
sailus: The buffers in a request are supposed to be processed together, right?
hverkuil: Buffers for the same vb2_queue or buffers for different vb2_queues?
sailus: Different queues.
<br> You can't currently queue multiple buffers into the same queue for a single request.
hverkuil: Yes, you can.
sailus: Is there a use case for that?
hverkuil: They will all be queued to that queue together in the order in which you added them to the request.
<br> It can be useful if you don't need to make any changes between the first and second buffer.
sailus: The buffers that are queued in the vb2 queue are independent of each other.
hverkuil: Again, there was no need to limit this in vb2. We can add restrictions on top, though.
sailus: But they are related to the other buffer in a different queue.
<br> I'd disallow it unless there's a specific reason to allow it.
hverkuil: I'm OK with that, but I prefer to keep the core as generic as possible, and restrict it at the higher levels.
sailus: Fine for me.
<br> I just haven't seen a use case for this and I doubt it actually exists.
hverkuil: There are other use cases I can think of: a deinterlacer for example where you need to feed it two fields at a time. So a request is controls + two buffers.
sailus: Fair enough.
hverkuil: Mixing request and non-request buffers is something that works in the core, but I think it is reasonable to disallow that. At least at first.
<br> And when you can have buffers for multiple queues in a request, then the req_validate function should probably verify that the number of buffers for each queue is consistent. I.e. if you have 2 buffers for one queue and 1 buffer for another where 2 are expected, then that request should be rejected.
<br> Doing that will indeed cause queues to get out-of-sync with one another.
sailus: I think what is missing is an API for the drivers to check what *else* is in a request than what they have queried.
hverkuil: I'm sure we will need to extend the API once we get into more complex scenarios beyond stateless codecs.
<br> no doubt about that.
sailus: I think what happens right now is that such a request never completes...
<br> As a summary, what I want to say is that I'm very happy to see we've this far but there are still bits that need attention.
hverkuil: Stateless codecs are easy. It's complex video pipelines where a lot more work is needed. But that simply *is* complex. It's going to be quite a lot of work to design something to avoid insanely complex drivers.
sailus: I agree.
<br> Right now my focus is just making sure what goes in is sound going forward, so that we won't find ourselves forced to make changes that break existing user space in order to proceed.
<br> Back late(r).
<br> Have a nice evening!
inigo: <u>hverkuil</u>: I don't know where and how to call vb2_dma_contig_init_ctx. Neither what to do with returned value
hverkuil: <u>inigo</u>: look in drivers/media/video/atmel-isi.c.
<br> It's a simple driver, so you can just copy what it does there.
<br> This whole 'allocation context' (basically a struct with a device pointer in it) has been removed in later kernel versions since it was overengineered.
inigo: also for dma-contig?
hverkuil: y
inigo: I have the file you say in my kernel version, so I will look it. However, where should I look for current version of the kernel?
hverkuil: the driver moved to drivers/media/platform/atmel/
inigo: Thanks. I supose it is OK to check it in torvalds Github repo?
hverkuil: yes
inigo: And last question today: if vivi was removed, which one is now the general example to look at?
hverkuil: vivid
<br> drivers/media/platform/vivid/
inigo: Wow, how many files! It was easier with just one file! xD
<br> Thanks, I keep going with the porting
hverkuil: The atmel-isi.c driver is a good example on how to use vb2. For SDTV support you can take a look at the usbtv driver (in drivers/media/usb/usbtv)
<br> It's a simple usb tv capture device, so a good starting point. Although from what I remember your driver code wasn't too bad in that respect.
<br> Did I point you to the v4l2-compliance test utility?
<br> https://git.linuxtv.org/v4l-utils.git/
inigo: Yes, you did
<br> I had done the test, and of course it fails in lot of points
<br> I will try to improve it
hverkuil: Not much point in running it until you've done the vb2 conversion.
<br> After that the remaining fails tend to be fairly trivial. Typically the driver forgets to fill in fields or fills them in incorrectly.
inigo: I think the biggest fault will be that it's not prepared to be opened more than once
hverkuil: Can you point me to the source of your driver again?
inigo: https://github.com/naggety/sunxi-tvin
hverkuil: Most of what's in the _open and _close functions belongs in the vb2 start/stop_streaming callbacks or in probe().
<br> the atmel driver shows (at least in the mainline kernel) what to do if you really need to initialize something on first/last use.
inigo: I've moved stop and mmap_free to stop_generating
<br> and removed stop_generating from close
<br> OK, I'm going to look atmel driver
hverkuil: The mmap stuff disappears when you use vb2. All handled by the vb2 framework.
inigo: True, I have removed it. I must have mistaken with other lines I've moved
ezequielg: <u>hverkuil</u>: iiuc, it's legal for drivers to not pass any video_device or queue mutex. in which case, they are expected to lock themselves.
<br> <u>hverkuil</u>: and so if sta2x11_vip does neither, then it's race-prone isn't it?
<br> calling vb2_is_streaming or any vb2_is_xxx is unsafe.
hverkuil: yeah, that's not good.
<br> this is an old driver.
ezequielg: well, it's fixed easily.
hverkuil: I'm not sure if there is still anyone around who can test this.
ezequielg: hm.
<br> removal candidatE?
hverkuil: Federico Vaga <federico.vaga@gmail.com> was able to test this in the past, I believe.
ezequielg: same question about omap4iss. introduced in 2011, nobody cared to get it out of staging in six years.
hverkuil: Other than the missing mutex this sta driver is fine.
ezequielg: good.
hverkuil: Something for pinchartl to decide. A lot of work went into that, but the chances that anyone will take it the last mile are slim, I think.
<br> ^^^ omap4iss
pinchartl: I know there are users
<br> and I think the driver isn't in a worse state than other drivers in drivers/media/platform/
<br> I probably shouldn't have submitted it through staging in the first place
<br> as to whether someone will continue maintaining it, I don't know
ezequielg: <u>pinchartl</u>: the TODO list doesn't look like staging worth. If it uses vb2 then it's good I think.
pinchartl: <u>ezequielg</u>: it uses vb2, yes
<br> I wouldn't have submitted a vb1 driver :-)
sailus: I think we discussed getting omap4iss out of staging some three years ago but no-one submitted the patches.
<br> So I agree with moving it.