<!-- 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>
***: makomk has quit IRC (Ping timeout: 268 seconds)
<br> lucaceresoli has quit IRC (Quit: Leaving)
lucaceresoli: <u>sailus</u>: would you be available now?
sailus: <u>lucaceresoli</u>: Yeah.
lucaceresoli: <u>sailus</u>: thanks. as discussed over e-mail, patch 6 (https://patchwork.linuxtv.org/patch/50183/) can use regmap_bulk_write and get rid of prepare_reg()
<br> <u>sailus</u>: however prepare_reg() is used in patch 8... (https://patchwork.linuxtv.org/patch/50181/)
<br> <u>sailus</u>: so I think there are 2 options:
<br> <u>sailus</u>: 1. still add prepare_reg (not in patch 6 but in another patch)
sailus: I'm not a huge fan of prepare_reg() --- it's risky to use, as it depends on the caller to provide two numerical arguments that result in accessing the register array.
lucaceresoli: <u>sailus</u>: 2. nuke prepare_reg, and extend the current struct reg_8 to hold multibyte registers
<br> <u>sailus</u>: (agreed, prepare_reg is a bit tricky
sailus: It'd be nice to add at least checks against that. Although if it's only used in a single function, that might be reasonable as such.
<br> Patterns tend to be copied across drivers though.
<br> How about writing the registers one by one, as most other drivers do?
<br> It could be nice to have a small framework for handing CCI writes, so that drivers would need to care less about such matters but I think that'd be out of scope of this patchset.
lucaceresoli: <u>sailus</u>: that would look a bit ugly, in imx274_apply_trimming I'm writing 9 registers, so it would be:
<br> bulk_write(...)
<br> if (err)
<br> goto fail;
<br>
<br> <repeat x9 times>
sailus: It could also be like:
<br> if (!err)
<br> bulk_write(...)
<br> ret = bulk_write(...)
<br> I mean.
<br> There's some repeat there, but it's still much less prone to memory access errors.
<br> (And check ret there, not err.)
lucaceresoli: well, yes.
<br> <u>sailus</u>: I was considering option 2 because:
<br> <u>sailus</u>: - imx274_write_table can be extended to multibyte regs with ~3 added lines
sailus: If that's easy, then why not?
lucaceresoli: <u>sailus</u>: - it coalesces consecutive multibyte reg writes in a single bulk write (e.g. 3 2-byte regs become one 6-byte bulk write)
sailus: It'd be likely much cleaner as well.
<br> Yes, it'd be nice if this was available for other drivers as well. :-)
lucaceresoli: <u>sailus</u>: the only drawback is all the big tables will take more memory. less than double though, because multibyte regs will take 1 8-byte entry w.r.t. 2 4-byte entries as today
<br> <u>sailus</u>: it would be cleaner to me at least, my brain prefers table-looking code for repetitive tasks like this
sailus: I'm not arguing against making this look simpler...
<br> Actually you only write registers that are a most 16-bit in imx274_apply_trimming.
<br> If you change the register value to 16 bits, wouldn't this address the issue without increasing the memory footprint?
lucaceresoli: <u>sailus</u>: you mean if I pass .val_bits = 8 to imx274_regmap_config? no, it would screw up 8-bit register writes.
sailus: Hmm, val_bits is already 8?
<br> Isn't it?
lucaceresoli: <u>sailus</u>: it is 8 currently
sailus: Yes...? :-)
lucaceresoli: <u>sailus</u>: perhaps I didn't understand what you mean by "If you change the register value to 16 bits, wouldn't this address the issue without increasing the memory footprint?"
sailus: Just to clarify: I meant the type of the val field in reg_8.
lucaceresoli: <u>sailus</u>: oh, I git it!
<br> <u>sailus</u>: s/git/got/
<br> <u>sailus</u>: hm, no, because otherwise _write_table will always write two bytes, even for 8-bit regs
sailus: Right.
<br> Well, this won't be that many lines of code in any case.
<br> I think the best option would be to write a nice framework for this that other drivers could use as well.
lucaceresoli: <u>sailus</u>: very few added _lines_, the only concern is we'll have more data (in the binary)
sailus: Otherwise I'd just do what other drivers do, i.e. write the registers one by one.
lucaceresoli: <u>sailus</u>: about making it available to other drivers, I'd love doing it
<br> <u>sailus</u>: but not in this patchset, it would be so much out of scope... do you agree?
sailus: Yes.
lucaceresoli: <u>sailus</u>: ok, so, I'll see how it can be done later, or in parallel, to this patchest
<br> <u>sailus</u>: for the time being, would you prefer if I just write registers one by one?
sailus: It'd be better IMO.
lucaceresoli: <u>sailus</u>: OK
sailus: Even if you can say prepare_reg() use in that driver is fine, there's a chance of it being copied for new drivers.
<br> This driver as well as others may then be converted later on to optimise the writes, once the helper library would be around.
lucaceresoli: <u>sailus</u>: about the "library", perhaps it should be in the regmap framework?
<br> <u>sailus</u>: in theory it has nothing specific to v4l2 drivers
sailus: CCI is rather specific to V4L2 --- that's what most sensors implement.
<br> I'm certainly fine with the regmap approach though, if it can be made fit there reasonably.
<br> Probably so.
<br> CCI allows registers that have varying sizes which is very uncommon.
<br> And it's a commonplace to have camera sensors with 32-, 16- and 8-bit registers, or even 24-bit.
<br> Elsewhere it's really rare.
lucaceresoli: <u>sailus</u>: er... CCI stands for...?
sailus: AFAIR it's for Camera Control Interface.
<br> Basically I²C plus some extra definitions on top. Such as 16- or 8-bit register address and varying register width (at least 8, 16 and 32).
lucaceresoli: <u>sailus</u>: ok, thanks
<br> <u>sailus</u>: well, I think the plan is clear now. back to work, thank you for your time! :)
hverkuil: <u>ezequielg</u>: the codec driver I used to test your mem2mem patches is here: https://git.linuxtv.org/hverkuil/media_tree.git/log/?h=vicodec
sailus: <u>lucaceresoli</u>: You're welcome! :-)
hverkuil: It's a virtual codec driver, similar to vim2m/vivid/vimc.
sailus: <u>lucaceresoli</u>: I reviewed the rest of the 8th patch, let me know if you have questions regarding the comments.
<br> (Or otherwise.)
lucaceresoli: <u>sailus</u>: seen you review, thanks. I'll be replying soon
<br> <u>sailus</u>: oh, actually the sentence "Could be here, for binning. %d -> %u --- it's unsigned." is not clear. What "could be here"?
sailus: Ah, I commented on other prints that they should be in the framework instead.
<br> This one's fine; binning is only known by the driver.
lucaceresoli: <u>sailus</u>: oh, I see. sent reply
sailus: Looks good to me.
***: harrow` has quit IRC (Quit: Leaving)
hverkuil: <u>pinchartl</u>: just to make sure I understand this correctly: entity->name should be unique within the media device topology, right? And userspace should not hardcode entity IDs or pad indices, since they can change over time (e.g. different IDs/indices can be returned in the next kernel version)
pinchartl: <u>hverkuil</u>: correct
<br> how entity names should be constructed is (unfortunately) currently undefined
<br> but they should be unique
hverkuil: OK. I'll prepare patches for the MC uapi documentation, since this is not mentioned anywhere AFAICS.
pinchartl: and that's how userspace should identify entities
<br> for pads it's a different matter
<br> pads don't have names
<br> so we can only rely on the ID
<br> or rather index
hverkuil: OK, so that must be stable. If new pads are added in the future, then those have to be added at the end, you cannot insert a new pad in the middle, right?
pinchartl: that's right, but it sounds a bit like an ill-defined API
hverkuil: ('in the middle' == 'in the middle of the pads array')
pinchartl: specially with MEDIA_IOC_G_TOPOLOGY
<br> there's no pad index anymore
<br> only pad IDs
hverkuil: Well, that's what my patch series adds back :-)
pinchartl: which, if I remeber correctly, are global, and thus not stable
<br> ok :-)
<br> I still have to review it. sorry for the delay :-/
***: rshanmu has quit IRC (Ping timeout: 260 seconds)
hverkuil: <u>pinchartl</u>: so applications can hardcode an entity name and expect that to work in the future as well (given the same hardware of course).
pinchartl: <u>hverkuil</u>: I would expect so, but it would be nice to think about how to proceed in case we need to rename entities
<br> (for whatever reason)
<br> it's dinner time, I'll be back in a bit
***: Killerkid has quit IRC (Quit: ZNC - http://znc.in)
sailus: <u>hverkuil</u>: Pad indices are not expected to change.
<br> That is certain to break user space; pads are not enumerable in detail as entities are.
<br> Ah, I guess this is sorted out based on reading the above discussion.
***: hverkuil has left
<br> hverkuil has left
<br> benjiG1 has left
<br> blinky42 has quit IRC (Ping timeout: 256 seconds)
<br> rubdos has quit IRC (*.net *.split)
<br> ribalda has quit IRC (*.net *.split)
<br> kbingham[m] has quit IRC (*.net *.split)
<br> trumee has quit IRC (Read error: Connection reset by peer)
ezequielg: oh, no Hans around.
***: tornade_ has left
ndufresn`: ezequielg, it could be because of the net-splits though, there was only 14 people an hour ago on #gstreamer
ezequielg: ndufresn`: yeah, could be.
cristian_c: <u>ezequielg</u>: is blinky42 Hans De Goede?