<!-- 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>
***: harrow has quit IRC (Quit: Leaving)
sailus: <u>bparrot</u>: Please don't add s_power() support to sensor drivers.
<br> Use runtime PM instead.
<br> Receiver drivers however should still call s_power on the sub-devices on a starting pipeline.
hverkuil: <u>jernej</u>: you mentioned that you would post a v2, so that's what I am waiting for.
***: tfiga has quit IRC (Ping timeout: 244 seconds)
<br> jernej has quit IRC (Remote host closed the connection)
sailus: <u>hverkuil</u>: Ping?
hverkuil: pong
sailus: About the patch adding locking to subdev ops.
<br> I thought it'd be faster to discuss it here than over e-mail.
<br> The intent is really to provide similar functionality from the framework as regular V4L2 drivers have: simple drivers don't need to care about serialising their ops, as long as they provide a lock.
<br> We don't have that for subdevs.
hverkuil: And I agree with that for any calls coming in via v4l-subdevX.
syoung: <u>hverkuil</u>: for the v4l-utils, I haven't tested ir-keytable with cec for a while. Have you tested this and/or should I test it?
<br> v4l-utils release
sailus: <u>hverkuil</u>: How about through kAPI?
<br> For the subdev drivers don't make a difference between the two...
hverkuil: <u>sailus</u>: But doing that for calls via v4l2_subdev_call (or derivative macros) I feel is dangerous. These have always been unlocked and I am afraid of unforeseen locking issues.
<br> If a subdev creates a device node, then userspace should configure the subdev, not platform drivers. There are only a few subdev ops that are still called from platform drivers in that case.
sailus: Using the lock is an explicit decision by a subdev driver.
<br> The kAPI is there for drivers to use; well, in practice there are not many ops that are used by that still means the drivers have to handle locking themselves without any help from the framework.
<br> This applies to any driver supporting VIDIOC_SUBDEV_*_FMT IOCTLs.
<br> And I'd like to move that to the framework where it'd be much less hassle & bugs.
<br> See e.g. the patch against the smiapp driver where the explicit locking is removed.
<br> I presume there are also drivers that do not do locking at all, which is a bug.
hverkuil: My concern is with bridge drivers that use a subdev, but prevent it from creating a v4l-subdevX device. If you add locking to that subdev, then I'm not sure if that is completely safe.
sailus: I'm not sure what do you mean.
<br> The only difference is that the device node for uAPI isn't available to the user.
<br> There may not be a need to serialise the ops in that case but there's still no harm from doing so.
<br> Well, nested calls still need to be considered.
<br> This is also the reason why drivers need to explicitly enable this by setting the lock field.
<br> That said, even currently drivers must handle such cases on their own, so this actually doesn't change an existing practice.
hverkuil: For example: some subdevs call v4l2_subdev_notify_event() to propagate an event to userspace. If a bridge driver calls a subdev op with the lock, and that op calls v4l2_subdev_notify_event, and the bridge driver does something that causes another call into the subdev. You'd have a deadlock.
<br> The control framework also bypasses this new subdev lock, you'd have to take it in the s_ctrl etc. ops in the subdev.
<br> Basically I am not certain that this will be 100% safe.
sailus: Controls are handled separately, they do not use v4l2_subdev_call().
<br> In reality a typical driver would use the same mutex for both purposes.
<br> On your example of v4l2_subdev_notify_event() --- do you mean that a bridge driver would call e.g. a sensor driver's op (with the sensor driver's mutex taken) and that event delivered to the bridge driver (by a function call) would again call something on the sensor driver?
<br> A driver is not expected to be able to call v4l2_subdev_notify_event() on other subdevs, is it? That sounds like a wrong thing to do.
hverkuil: Could be. There certainly is nothing that prevents you from doing that. The interaction between subdev calls and the control framework is I think more likely to cause problems.
<br> I think you need to take the subdev lock in subdev_do_ioctl() before calling the control framework.
sailus: It's still something that the drivers need to take into account, whether or not they use any help from the framework in serialising access to their data.
<br> Uh, no. The control framework takes its own mutex --- which the drivers should set to point to the same.
<br> For some drivers the data dealt with by controls is distinct from what's being touched by the other IOCTLs but I wouldn't assume that.
hverkuil: My problem is that I am not sure that changing v4l2_subdev_call to take a lock is 100% safe. Especially older drivers often do weird and wonderful things and I don't trust them at all.
<br> And the control framework takes the lock of the control handler. That's a different lock.
sailus: It may be the same lock.
<br> I also wouldn't convert an older driver without making sure changing the locking scheme is a safe thing to do.
<br> Do note that drivers must explicitly make use of this feature.
<br> Basically these arguments are the same that you could make against the framework providing help with locking on V4L2 video nodes. :-)
hverkuil: The problem is not in the subdev driver, it's in how it is called from bridge/platform drivers. And you don't know in which bridge/platform drivers the subdev will be called. After all, end-users can use a subdev with a different driver just by changing the dts.
<br> No, because there we know that it is always called from userspace. Drivers do not call the ioctl function directly.
<br> Here there are multiple points of entry.
sailus: I don't see how moving that locking to the framework side fundamentally changes this, as drivers are expected to serialise access to their data structures today.
<br> If we'd have deadlock issues with this, we'd have them today as well.
<br> I agree the use of events requires some more attention with this locking scheme. But it's not really different: a driver calling the function with its own mutex taken comes with equal risk.
hverkuil: BTW, note that the macros in v4l2-device.h do not use the v4l2_subdev_call macro: they should probably start to use it.
sailus: Yes, they should.
<br> Generally these macros are only used by non-MC-enabled drivers so in practice that wouldn't be an issue.
hverkuil: <u>sailus</u>: urgh, pvrusb2 calls some subdev ops directly.
sailus: Ouch.
<br> I didn't see that coming.
<br> I'll check other drivers, too.
hverkuil: git grep -e 'sd.*ops->' drivers/media
sailus: I'll convert the macros in v4l2-device.h as well.
hverkuil: OK, make a v2 with the v4l2_device_call macros fixed, pvrusb2 fixed, locking in the control framework (probably only needed in v4l2-subdev.c), and checking the notify_event calls.
<br> To be honest, I don't expect problems with notify_event, but it has to be verified.
<br> I'll take another look at this when v2 is posted.
sailus: The notify_event matter is driver specific, as it's up to a driver to call it.
<br> I think a warning note in documentation would be appropriate.
hverkuil: Certainly.
sailus: The control handler already has its own locking scheme; there's no need to take the subdev mutex for controls.
hverkuil: To be precise: subdev drivers can call v4l2_subdev_notify, which in turn calls the notify callback of struct v4l2_device. And those notify callbacks should be analyzed.
<br> Not true.
sailus: How so?
hverkuil: When you set a control, then s_ctrl is called inside the subdev. That touches the hardware and so should be serialized with e.g. set_fmt calls.
sailus: For drivers it's most convenient to use the same mutex both on the control handler and the subdev ops.
<br> If you take the subdev lock, and then the control lock (which are the same), a deadlock will follow.
<br> Again, nothing that wouldn't already happen today with existing drivers.
hverkuil: The lock in the control handler is just a lock for that struct. A driver may have multiple control handlers, so this really does not fly.
sailus: I'm talking about regular drivers.
<br> It's possible for a driver to use several mutexes, yes, but then it's up to the driver author to make sure the locking scheme works.
<br> Even smiapp uses just a single mutex even if it has two control handlers and three subdevs.
hverkuil: I think I misunderstand you, or vice versa.
<br> Ah, I think the fault is at my side.
<br> You replace hdl->lock after the handler is initialized with a subdev lock.
sailus: Yes, many subdev drivers do that.
<br> They could also simply make use of the mutex in the control handler, to avoid having more than one around.
<br> But that's a separate matter.
hverkuil: You have to be really careful not to call v4l2_ctrl_s/g_ctrl in such drivers, but always the unlocked variant.
sailus: Yes, that's what's being done.
<br> The hardware and device's data are the same; and it's easier to get things right if the mutexes are the same as well.
ndufresne: ezequielg, tfiga: fyi, Microsoft got DXVA_Slice_H264_Long and DXVA_Slice_H264_Short with some signaling, which seams to pretty match the delta we see between Hantro and Cedrus
<br> (I'm reviewing a dxva backeng for GStreamer, which I hope I'll be able to share base classes with for the v4l2 support)
jernej: <u>hverkuil</u>: I mean this series: https://patchwork.kernel.org/cover/11123627/
<br> I plan to base my Cedrus multi-slice v2 series on that
hverkuil: <u>jernej</u>: I recommend that you just add it to your own v2 series (just keep my authorship intact).
<br> It is probably easiest to merge everything in one go.
<br> And if your patches on top take longer, then I can always just merge my three patches first.
jernej: <u>hverkuil</u>: If you ask me, you can merge those 3 patches immediately. I thought you are waiting on review from paulk-leonov or mripard
hverkuil: We're in the merge window waiting for v5.4-rc1 to be released. During that time we don't merge patches, so that's why these patches are still not in a pull request.
<br> Once rc1 is released and merged back into our master I'll start posting PRs.
jernej: ok, but if I include your patches to my series, my review-by tags don't apply anymore since I have to add signed-off-by to each patch, right?
<br> anyway, I guess I'll post after merge window closes
***: benjiG has left
<br> Kwiboo has quit IRC (Quit: .)
<br> b-rad has quit IRC (Ping timeout: 258 seconds)