On 15/12/2021 11:38, Jacopo Mondi wrote:
Hi Tomi
On Wed, Dec 15, 2021 at 10:06:16AM +0200, Tomi Valkeinen wrote:
Hi,
On 13/12/2021 23:30, Jacopo Mondi wrote:
Hi Tomi,
On Tue, Nov 30, 2021 at 04:15:00PM +0200, Tomi Valkeinen wrote:
Add a new 'active_state' field to struct v4l2_subdev to which we can
store the active state of a subdev. This will place the subdev
configuration into a known place, allowing us to use the state directly
from the v4l2 framework, thus simplifying the drivers.
Also add functions v4l2_subdev_init_finalize() and
v4l2_subdev_cleanup(), which will allocate and free the active state.
The functions are named in a generic way so that they can be also used
for other subdev initialization work.
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@xxxxxxxxxxxxxxxx>
---
drivers/media/v4l2-core/v4l2-subdev.c | 21 +++++++++++
include/media/v4l2-subdev.h | 54 +++++++++++++++++++++++++++
2 files changed, 75 insertions(+)
diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
index fe49c86a9b02..de160140d63b 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -943,3 +943,24 @@ void v4l2_subdev_notify_event(struct v4l2_subdev *sd,
v4l2_subdev_notify(sd, V4L2_DEVICE_NOTIFY_EVENT, (void *)ev);
}
EXPORT_SYMBOL_GPL(v4l2_subdev_notify_event);
+
+int v4l2_subdev_init_finalize(struct v4l2_subdev *sd)
+{
+ struct v4l2_subdev_state *state;
+
+ state = __v4l2_subdev_state_alloc(sd);
+ if (IS_ERR(state))
+ return PTR_ERR(state);
+
+ sd->active_state = state;
You could spare the local var..
I don't like to write error values to storage fields. Here I only touch the
field when succesfull.
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(v4l2_subdev_init_finalize);
+
+void v4l2_subdev_cleanup(struct v4l2_subdev *sd)
+{
+ __v4l2_subdev_state_free(sd->active_state);
+ sd->active_state = NULL;
+}
+EXPORT_SYMBOL_GPL(v4l2_subdev_cleanup);
diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index e52bf508c75b..87c3126b2754 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -645,6 +645,9 @@ struct v4l2_subdev_ir_ops {
* This structure only needs to be passed to the pad op if the 'which' field
* of the main argument is set to %V4L2_SUBDEV_FORMAT_TRY. For
* %V4L2_SUBDEV_FORMAT_ACTIVE it is safe to pass %NULL.
+ *
+ * Note: This struct is also used in active state, and the try_ prefix is
+ * historical and to be removed.
*/
struct v4l2_subdev_pad_config {
struct v4l2_mbus_framefmt try_fmt;
@@ -898,6 +901,8 @@ struct v4l2_subdev_platform_data {
* @subdev_notifier: A sub-device notifier implicitly registered for the sub-
* device using v4l2_async_register_subdev_sensor().
* @pdata: common part of subdevice platform data
+ * @active_state: active state for the subdev (NULL for subdevs tracking the
+ * state internally)
I would suggest here that init_finalize() should be used
* @active_state: subdev active state. Initialized by calling
v4l2_subdev_init_finalize() by drivers that support
state
Ok.
I would have said "the state API" but then we would need to define
what the state API as a concept :)
*
* Each instance of a subdev driver should create this struct, either
* stand-alone or embedded in a larger struct.
@@ -929,6 +934,19 @@ struct v4l2_subdev {
struct v4l2_async_notifier *notifier;
struct v4l2_async_notifier *subdev_notifier;
struct v4l2_subdev_platform_data *pdata;
+
+ /*
+ * The fields below are private, and should only be accessed via
+ * appropriate functions.
+ */
+
+ /*
+ * TODO: state should most likely be changed from a pointer to an
+ * embedded field. For the time being it's kept as a pointer to more
+ * easily catch uses of state in the cases where the driver doesn't
+ * support it.
what about "... For the time being it's kept as a pointer to allow drivers
to support state as an opt-in feature."
Well, that's not quite correct. Even if it was an embedded field, it would
be an opt-in feature. The point with the pointer is only that if an opt-out
driver accidentally tries to access the field, it'll get NULL, and hopefully
fail.
Ok, what I mean is that being a pointer, it's easy to identify if a
driver has opted-in. Whatever, we're saying the same thing
+ */
+ struct v4l2_subdev_state *active_state;
};
@@ -1217,4 +1235,40 @@ extern const struct v4l2_subdev_ops v4l2_subdev_call_wrappers;
void v4l2_subdev_notify_event(struct v4l2_subdev *sd,
const struct v4l2_event *ev);
+/**
+ * v4l2_subdev_init_finalize() - Finalize the initialization of the subdevice
+ * @sd: The subdev
+ *
+ * This finalizes the initialization of the subdev, including allocation of
+ * the active state for the subdev.
+ *
+ * This must be called by the subdev drivers that use the centralized active
+ * state, after the subdev struct has been initialized and
+ * media_entity_pads_init() has been called.
Or just "It must be called by subdev drivers that support active state
as the last operation before registering the subdev."
That's not quite true either. The driver may do setup operations requiring
the active state before registering the subdev.
Are there other setup operations on the subdev besides the usual
v4l2_.._subdev_init(), media_pads_init() etc that have to be called
after a function that by its name 'finalizes' the initialization ?
Anyway, just a nit, whatever is fine
I was only thinking about the finalization of the state. But as it's now
"init finalize", I think it makes sense that it would be just before
subdev registration. But I'm actually setting up the notifiers between
the v4l2_subdev_init_finalize and v4l2_async_register_subdev... I need
to see if there's any reason for that, or can the
v4l2_subdev_init_finalize be moved to be just before subdev registration.
+ *
+ * Must call v4l2_subdev_cleanup() when the subdev is being removed.
+ */
+int v4l2_subdev_init_finalize(struct v4l2_subdev *sd);
+
+/**
+ * v4l2_subdev_cleanup() - Release the resources needed by the subdevice
+ * @sd: The subdevice
+ *
+ * This will release the resources allocated in v4l2_subdev_init_finalize.
+ */
+void v4l2_subdev_cleanup(struct v4l2_subdev *sd);
+
+/**
+ * v4l2_subdev_get_active_state() - Return the active subdev state for subdevice
+ * @sd: The subdevice
+ *
+ * Return the active state for the subdevice, or NULL if the subdev does not
nit:
The other functions in the file are documented with "Returns" (and
also "This function" seems to be more commonly used that just "This").
Ok, I'll try to get these consistent.
+ * support active state.
+ */
+static inline struct v4l2_subdev_state *
+v4l2_subdev_get_active_state(struct v4l2_subdev *sd)
+{
+ return sd->active_state;
+}
+
This is only used by the core, and by top drivers when doing:
ret = v4l2_subdev_call(sd, pad, enum_frame_size,
v4l2_subdev_get_active_state(sd), &fse);
To pass to the subdev its own state.
I wonder if a macro like
#define v4l2_subdev_call_state(sd, o, f, args) \
({ \
struct v4l2_subdev_state *st = v4l2_subdev_get_active_state(sd); \
v4l2_subdev_call(sd, o, f, st, args); \
})
Could help simplify the state API.
Possibly, but we need to support the TRY case also. So I would call it
something more specific, v4l2_subdev_call_active(), or such.
If the caller intends to pass is a try state (which it has to allocate
explicitly) shouldn't it just use v4l2_subdev_call() ?
Yes. But I think this is pretty confusing already, and in my opinion
adding v4l2_subdev_call_state() would confuse this even more =). I think
at least it has to make it clear that the subdev is being called using
its active state.
But maybe just using v4l2_subdev_call() for now and seeing how this
evolves is best. It's trivial to add the helper later.
Please be patient if I digress a bit more on the API now..
Looking forward in the series we'll end up having 5 functions to
manage state, if I've not missed any
1) struct v4l2_subdev_state *v4l2_subdev_get_active_state(struct v4l2_subdev *sd);
2) struct v4l2_subdev_state *v4l2_subdev_lock_active_state(struct v4l2_subdev *sd);
3) void v4l2_subdev_lock_state(struct v4l2_subdev_state *state);
4) void v4l2_subdev_unlock_state(struct v4l2_subdev_state *state);
5) struct v4l2_subdev_state *v4l2_subdev_lock_and_return_state(struct v4l2_subdev *sd,
struct v4l2_subdev_state *state)
1) and 2) are helpers to get (locked/unlocked) v4l2_subdevice.state,
used in operations which implicitly operates on the active state
(s_stream etc)
I wonder if we shouldn't just have a locked v4l2_subdev_get_state()
and an optional unlocked version which can be used by the core in
place of v4l2_subdev_get_active_state() and by drivers which know what
they're doing by using the unlocked version
I think we need to always make it clear if we're dealing with the active or
try state (when it's relevant). And I also want to be clear if a lock is
taken. v4l2_subdev_get_state() would hide both of those.
It is clear you're dealing with active state if the only argument is
a struct v4l2_subdevice *.
Perhaps, if you understand the internals here. I'd rather be more
explicit and clear in the function names. Again, we can easily change
these later if they don't feel necessary.
If you do not receive a state as an operation parameter you can only
get the one is the subdev, which is by definition the active one.
When it comes to locked, if the usage pattern is that the state is to
be locked before being accessed, and the unlocked case is the
exception (as it is only used by the core and by the top drivers) then
I don't feel so strongly as you about having _locked() in the name.
My reasoning here is that we can have function pairs like get_xxx,
put_xxx, and then it's clear that those match and they need to be paired.
But if we have just a get_xxx, at least my thinking is that it just
returns something, and I'll use it and forget about it when I don't need
it anymore.
But lock_xxx makes it clear I have to unlock it afterwards.
That said, maybe instead of v4l2_subdev_lock_active_state we should have
v4l2_subdev_lock_and_get_active_state. But it gets a bit long...
Anyway, your code, your names..
3) and 4) lock the state at hand (to be used in operations with a
'state' parameter)
5) is currently unused and it still feels a bit ill-defined. If the state
passed in as parameter is NULL then deflect it to sd->state, so I
guess it assumes that the user is a state-aware subdev driver which
needs to protect against in-kernel callers that do no pass in a valid state to
Correct.
the operation call. This implies that if the caller passes in a NULL
state for the TRY format, this gets deflected to the subdev's active
state, something which should not happen, right ? I would be tempted
Yes. That's an illegal call, isn't it? Or does it happen and we need to
protect against it?
to just fail loud if !state and assume if you're moving your subdev to
use state you should be able to fix the caller as well.
That would be nice, but I think it's not realistic. If you convert a sensor
driver to multiplexed streams, are you going to fix (convert to multiplexed
streams) all the bridges and SoC drivers that may use that sensor? How do
you even find all those bridges and SoCs...
Of course no. You fix the one you're using. You're converting a sensor
driver, you can fix the top driver too. Other users of the sensor
driver will probably scream when moving to the next driver release
that has been made state aware, so they'll hopefully fix their top driver
too. After all, this applies equally to downstrems as well and
Well, I'm not a maintainer in linux-media, but I would nack that
approach =). We can't just go breaking other platforms, and hoping other
devs will fix them.
providing an helper to work around issues is not the best idea in my
opinion. Also the helper should be used in the subdev driver in place
of the regular v4l2_subdev_lock_state() to protect just in case
against legacy callers. When will they be moved to use the regular
v4l2_subdev_lock_state() ?
Note that this is needed only when porting an existing and presumably
in-use subdev driver. You don't need this if you write a new driver.
The users of v4l2_subdev_lock_and_return_state are easy to find and easy
to change to v4l2_subdev_lock_state when we decide a particular driver
doesn't need to support legacy subdevs.
I don't like this at all but, afaics, we can't break the existing
platforms. This function is a very simple work-around for the time
being, and easy to drop later.
Once a subdev driver has been moved to be state aware callers that
passes in a NULL state should be fixed. As we can't fix them all,
screaming loud and clearly say what has to be done to move forward is
in my opinion better than introducing a temporary function that
replaces the regular API and that subdev should decide to use just in
case (and which can lead to subtle errors like using the active state
in a TRY context).
If you want to protect against broken callers then what about
doing the "state = state ? : sd->active_state;" dance in
v4l2_subdev_lock_state() with a WARN_ONCE(!state) so that
subdev drivers can use the regular API from day 0 ?
Hmm, I think that is an option. I didn't implement "state = state ? :
sd->active_state;" in the v4l2_subdev_lock_state() as I didn't want
v4l2_subdev_lock_state() to hide an invalid NULL parameter. But adding
WARN_ONCE() would warn about it.
I'm still undecided, though. The WARN_ONCE would come only once for the
whole kernel, per boot, wouldn't it? We could have a macro for
v4l2_subdev_lock_state, but then we'd get lots of warnings. And a full
WARN just because a driver hasn't been updated is a bit harsh. Maybe we
can start with just a normal warning print.
Tomi
