On 27/09/2021 02:58, Laurent Pinchart wrote:
Hello,
On Thu, Sep 16, 2021 at 12:36:33PM +0300, Tomi Valkeinen wrote:
On 16/09/2021 11:08, Jacopo Mondi wrote:
On Thu, Sep 16, 2021 at 09:52:42AM +0300, Tomi Valkeinen wrote:
On 16/09/2021 09:17, Tomi Valkeinen wrote:
On 15/09/2021 12:44, Jacopo Mondi wrote:
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'.
I'd like to propose a third approach (or at least I believe it's a third
one). I agree with Jacopo that the state structure should not have a
which field, that's a layering violation. The state is a state,
regardless of whether it holds TRY or ACTIVE data. What are the current
blockers that would prevent that ?
Oh, I agree with that too. I didn't add the 'which' field to state
because I thought it's good =). It is supposed to be temporary solution.
init_cfg() is the issue here.
It think I had these options:
- Change init_cfg() to take 'which' as a parameter
- Change init_cfg() implementations to not use 'which'
- Add new version for new drivers, init_cfg_state() or such, which
either gets the which as a parameter or doesn't use which.
- Add 'which' to state.
I chose the fourth one as it's a very small change, and can be removed
easily in the future when the underlying problem is solved.
However, I think .init_cfg() should be used to initialize *all* states,
both TRY and ACTIVE. That will simplify driver implementation (but
centralizing the initialization in a single place) and ensure that the
default value for the ACTIVE state always matches the default value for
the TRY state, which I think is important.
I agree.
When it comes to calling v4l2_subdev_alloc_state() in drivers, I also
agree with Jacopo, I'm not extremely fond of it. This should be
automatic, drivers shouldn't have to care about allocating the ACTIVE
state. However, I also agree with Tomi that there's no real place to do
this today. I think we need to restructure subdev initialization, which
currently has very few rules. This could take the form, for instance, of
I agree with your agreeings above too.
1. calling v4l2_subdev_init() as an early subdev init function
2. setting fields of v4l2_subdev manually
3. initializing the media_entity embedded in the subdev
4. calling a new "initialization finalization" function that will
initialize the state embedded in v4l2_subdev (as opposed to
allocating it dynamically as done today)
Why is the allocated/embedded relevant? The drivers won't know which one
is used.
That's the simplest option as it really ressembles what we do today, and
would more or less only require embedded the state in v4l2_subdev and
renaming v4l2_subdev_alloc_state() to v4l2_subdev_init_done() (or
similar, that's likely not a great name).
Yes. But I don't see embedding relevant. So we could just rename
v4l2_subdev_alloc_state() and we should be there.
A more intrusive change would restructure the subdev initialization
further, by handling the media_entity initialization transparently
(which would require passing more arguments to v4l2_subdev_init(),
probably creating a new version of that function). That's more complex
and I think it's out of scope for this series, but it may still be
useful to keep the long term goal in mind to try and align with the
direction.
Yes, there are many things that can be improved further. But in the
context of this series, I'd like to keep the improvements such that they
require no changes to the existing drivers.
Tomi
