On 16/09/2021 11:08, Jacopo Mondi wrote:
Hi Tomi,
On Thu, Sep 16, 2021 at 09:52:42AM +0300, Tomi Valkeinen wrote:
On 16/09/2021 09:17, Tomi Valkeinen wrote:
Hi,
On 15/09/2021 12:44, Jacopo Mondi wrote:
Hi Tomi,
On Mon, Aug 30, 2021 at 02:00:42PM +0300, Tomi Valkeinen wrote:
Add a new '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.
We also add v4l2_subdev_alloc_state() and v4l2_subdev_free_state(),
which need to be used by the drivers that support subdev state in struct
v4l2_subdev.
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@xxxxxxxxxxxxxxxx>
---
drivers/media/v4l2-core/v4l2-subdev.c | 21 ++++++++++++++++
include/media/v4l2-subdev.h | 36 +++++++++++++++++++++++++++
2 files changed, 57 insertions(+)
diff --git a/drivers/media/v4l2-core/v4l2-subdev.c
b/drivers/media/v4l2-core/v4l2-subdev.c
index 26a34a8e3d37..e1a794f69815 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_alloc_state(struct v4l2_subdev *sd)
+{
+ struct v4l2_subdev_state *state;
+
+ state = v4l2_alloc_subdev_state(sd);
Replying to this again, as the second email didn't actually cover all the
topics...
So, I think this is one source of confusion about init_cfg.
v4l2_alloc_subdev_state() calls init_cfg() and 'state-aware' driver
are now supposed to allocate their state by calling
v4l2_subdev_alloc_state(), in the same way as the core does for the
file-handle ones.
This will lead to init_cfg to be called for the 'active' (ie owned by
the subdev) state, and then you need to add context to the state (by
adding a 'which' field) to know what state you're dealing with.
According to the init_cfg() documentation
* @init_cfg: initialize the pad config to default values
the op has to be called in order to initialize the per-file-handle
context, not the active one.
I have missed updating the documentation there =).
The documentation above doesn't imply per-file-handle context or TRY case,
afaics. It just says "initialize state to default". Unless "pad config"
always means TRY, which I think it doesn't as the drivers have internally
been using pad configs.
If they do, they would have the 'active' pad_configs allocated or
embedded somewhere in their driver structures, they would not receive
it as parameter. Or have I missed where the core is capable of fishing
the 'right' pad_configs ? I think the same should happen for state.
Yes, that is correct. I was just saying that the documentation doesn't
say that init_cfg is only used for the TRY case, even if that has been
the case in practice.
But it's true that so far init_cfg has only been called for TRY case, and
perhaps that's enough of a reason to keep it so.
I would rather just embed 'struct v4l2_subdev_state' in 'struct
v4l2_subdev', have the core going through the
Why would embedding the state change anything?
'v4l2_subdev_alloc_state()' to initialize the per-fh state, but have
drivers initialize their own state at probe time. If they need for
some reason to access their 'active' state at init_cfg() time, they
caan fish it from their subdev.
If I'm not mistaken this will remove the need to have a which filed in
the state, as I think the 'context' should be inferred from the
'which' argument embedded in the ops-specific structures, and not held
in the state itself.
It's true that the state's which field is mainly (probably only) needed for
handling the init_cfg. It could be solved in other ways too:
- New subdev op to initialize active state
- New subdev op which gets 'which' as a parameter, to initialize both states
(state-aware drivers wouldn't need to implement the old init_cfg)
- Coccinelle to change init_cfg to get the which as a parameter
Without doing any deep thinking, the middle one sounds best to me.
Isn't it simpler if you just don't call init_cfg for the 'active'
state ? Driver will initialize them at probe time and that's it, then
you can remove 'which' from the state (and from routing tables too if I'm
not mistaken).
Routing needs the 'which' similarly to other config structs, like
v4l2_subdev_format.
I did a quick implementation for the second case, which allows me to
remove 'which' from the state.
As for "simpler"... Both ways have pros and cons. I'm my mind this new
way is simpler.
Afaics, in the end (i.e. after doing the v4l2_subdev_call change), the
only bigger difference is how the state-aware drivers implement their
ops. With my approach, they get the correct state and that's it. With
your approach, they need to use a helper function (or do it manually) to
get the state based on 'which'.
Tomi
