On 07/03/2022 11:51, Hans Verkuil wrote:
Hi Tomi,
On 3/7/22 10:16, Tomi Valkeinen wrote:
Hi Hans,
On 07/03/2022 10:36, Hans Verkuil wrote:
On 3/7/22 08:16, Tomi Valkeinen wrote:
Hi Hans,
On 04/03/2022 15:34, Hans Verkuil wrote:
Hi Tomi,
On 3/1/22 11:55, Tomi Valkeinen wrote:
Add v4l2_subdev_call_state_active() macro to help calling subdev ops
that take a subdev state as a parameter. Normally the v4l2 framework
will lock and pass the correct subdev state to the subdev ops, but there
are legacy situations where this is not the case (e.g. non-MC video
device driver calling set_fmt in a source subdev).
As this macro is only needed for legacy use cases, the macro is added in
a new header file, v4l2-subdev-legacy.h.
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@xxxxxxxxxxxxxxxx>
---
include/media/v4l2-subdev-legacy.h | 42 ++++++++++++++++++++++++++++++
1 file changed, 42 insertions(+)
create mode 100644 include/media/v4l2-subdev-legacy.h
diff --git a/include/media/v4l2-subdev-legacy.h b/include/media/v4l2-subdev-legacy.h
new file mode 100644
index 000000000000..6a61e579b629
--- /dev/null
+++ b/include/media/v4l2-subdev-legacy.h
@@ -0,0 +1,42 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * V4L2 sub-device legacy support header.
+ *
+ * Copyright (C) 2022 Tomi Valkeinen <tomi.valkeinen@xxxxxxxxxxxxxxxx>
+ */
+
+#ifndef _V4L2_SUBDEV_LEGACY_H
+#define _V4L2_SUBDEV_LEGACY_H
+
+/**
+ * v4l2_subdev_call_state_active - call an operation of a v4l2_subdev which
+ * takes state as a parameter, passing the
+ * subdev its active state.
+ *
+ * @sd: pointer to the &struct v4l2_subdev
+ * @o: name of the element at &struct v4l2_subdev_ops that contains @f.
+ * Each element there groups a set of callbacks functions.
+ * @f: callback function to be called.
+ * The callback functions are defined in groups, according to
+ * each element at &struct v4l2_subdev_ops.
+ * @args: arguments for @f.
+ *
+ * This is similar to v4l2_subdev_call(), except that this version can only be
+ * used for ops that take a subdev state as a parameter. The macro will get the
+ * active state and lock it before calling the op, and unlock it after the
+ * call.
+ */
You should explain why this is a legacy macro and, ideally, what would need to
be done to get rid of it. The first is in the commit log, but nobody reads that :-)
But if just using it in a non-MC video device driver constitutes 'legacy' use,
then I disagree with that. There are many non-MC video device drivers, nothing
legacy about that.
It's difficult to define all the scenarios where this can be used, but the ones I can imagine fall under legacy (depending on how you define that, though).
I use this in CAL driver, which supports non-MC (legacy) and MC. CAL has a bunch of video devices (one for each DMA engine) and two CSI-2 PHY devices (v4l2 subdevs).
When operating in MC mode, the userspace will call, e.g., set_fmt in the PHY subdev, and so forth.
But in non-MC case the userspace calls VIDIOC_S_FMT in the video dev, and the video dev has to propagate that to the PHY subdev. I do this propagation using the v4l2_subdev_call_state_active macro.
I don't know if there are other drivers that support both non-MC and MC modes. I could also just move this macro to the CAL driver, and we could add this to the v4l2 framework if we see other drivers using similar constructs.
It is common to have non-MC drivers that call set_fmt of a subdev.
Wouldn't they all need to use this helper macro? If so, then this is NOT a
legacy use, it's just a non-MC driver use.
These non-MC drivers that call set_fmt of a subdev, they're video device drivers, right? In other words, there are no subdev drivers that call set_fmt on other subdevs?
Probably not, but I am not 100% certain. There are a few nested subdev cases, but I don't remember which.
This does get a bit complex, keeping the old and new code working together. In this context, I think we have three different "classes":
1. non-MC
2. MC, no state support
3. MC, state support
Are you talking about subdev drivers or bridge drivers? It's a bit confusing. I'm assuming it can be either.
Yes, I mean either one.
We have classes 1 and 2 in upstream, and 3 will be enabled with this series (and expanded with the streams series).
Classes 1 and 2 continue working as before. If you have a pipeline with only class 3 drivers, it works without any legacy "hacks". The problems come when you combine 1 or 2 with 3. Or possibly the problems appear only when combining class 1 and class 3, as class 2 drivers are not supposed to call subdev ops which take a state parameter on other subdevs.
A class 3 driver expects to get either a try or an active state as a parameter, but class 1 drivers pass NULL for the active state. If you write a class 3 driver and want it to work with class 1 (without any changes to those drivers), you must do extra plumbing in the ops functions, to catch the NULL state case and get & lock the state yourself. If you do that, this macro is not needed.
Alternatively, class 1 drivers could be changed to use this macro, so that a possible class 3 driver in the pipeline would work without additional code. But there are a lot of class 1 drivers, and thus modifications, and I wasn't planning to go that way.
The CAL driver I mentioned supports both class 1 and class 3 (via a module parameter) in the video dev driver, and the class 1 mode uses this macro as CAL's PHY subdev (part of the same driver) is a class 3 subdev. The class 1 support is legacy support in CAL's case.
So... Depending on what kind of driver combinations we want to support, this may or may not be legacy, depending on how you define legacy =).
Let me try to explain what my concerns are. Eventually I would really like all subdevs to be capable of working with MC bridge drivers, i.e. have state support. Bridge drivers can be either MC or non-MC.
So I would like to know:
1.1) How to convert a subdev driver to a MC state-aware subdev driver?
I presume you mean how to convert an MC subdev driver to state-aware MC
driver. If you have a non-MC subdev driver, then that first needs to be
converted to an MC driver, which is out of scope here.
Here's an example commit where I convert OV10635 to streams:
https://git.kernel.org/pub/scm/linux/kernel/git/tomba/linux.git/commit/?h=streams/work-v11&id=ffb08af15f04ef2ce0a00bb356e957c839d6bccb
However, the commit is on top of the streams series and I add support
for multiple streams in addition to the active state, so 1) it's not as
simple as it could be (e.g. get_frame_desc() and set_routing() ops could
be left out), and 2) it's not using the old-style v4l2_subdev_pad_config
(which is the only option on top of this series), but the new routes &
streams.
So the example doesn't quite answer to your question... I haven't looked
at implementing a driver which would be state-aware but not
streams-aware, but I think it's essentially just:
- Use v4l2_subdev_init_finalize() to create the active state storage
- Always use state->pads instead of something stored in the driver's
private data for active case.
1.2) What is the legacy code that such a MC state-aware subdev driver has to keep in order to work with older bridge drivers that do not support such subdev drivers? (i.e. they pass NULL as the state)
I believe the only extra code needed is to handle the state == NULL
case. This means adding code to each subdev op which has the state as a
parameter, and doing, perhaps, something like this:
my_set_fmt(sd, _state, fmt)
{
state = _state
if (!_state)
state = v4l2_subdev_lock_and_get_active_state(sd);
... use 'state' here ...
if (!_state)
v4l2_subdev_unlock_state(state);
}
Maybe we can somehow macro-ify the above, which creates a wrapper for
the op. Or, as I mentioned, we could try to change all the drivers that
do those calls, so that they use the macro in this patch instead.
2.1) How to convert a bridge driver (either non-MC or MC, but no state support) to properly support a fully converted subdev (MC state-aware) driver?
Converting non-MC driver to an MC driver is out of the context here, as
it's not related to the active state. An MC bridge driver should work
fine with state-aware subdev drivers, as the bridge driver should not
call any of the subdev's state-related ops.
To make a non-MC bridge driver support state-aware subdev drivers, they
can use the macro in this patch.
2.2) What is the legacy code that such a bridge driver has to keep in order to work with older subdev drivers that are not yet MC state-aware?
The older subdev drivers should keep working without any extra code.
The code needed for 1.2 and 2.2 (helper functions/macros) is legacy code, and can be marked as such.
> If this is clear, then we can work towards converting both subdev and
bridge drivers and eventually (might take years!) get rid of the legacy
code.
Removing support for case 2 is probably something that we want to do sooner than later.
For the CAL driver I do not consider non-MC support as legacy. It's legacy in the context of the CAL driver only, but API-wise it is not since there are many non-MC bridge drivers.
That's true, but also, non-MC bridge drivers do not need to use this
function if the subdev drivers use the method shown in 1.2. I think this
is the question here:
- Change all the callers and use the macro in this patch. Then the macro
is not legacy.
- Change the callees, in which case this macro is needed only in some
cases where, for whatever reason, a specific callee has not been changed
(yet?). In this case it's legacy.
Changing the callers would be a nicer option, I think, but I also fear
that it's very difficult and easily brings in bugs. I haven't looked
closely, but I think it would be a big patch.
And it's not clear to me if there's a benefit: do all those drivers ever
need to interact a state-aware subdev driver? If they do, maybe there
are only a few such subdev drivers, and it's not a big issue to have the
lock/unlock code in those state-aware subdev drivers. All the other
state-aware subdev drivers would not need the legacy support code as
they're used only in more modern pipelines.
Tomi
