On 22/03/2022 11:23, Hans Verkuil wrote:
Can you make a patch that converts ov5648.c (or a similar driver) to use
sd->active_state? I'd like to see how that looks.
I have ov5640, I can convert that one.
Right. So to confirm: the only reason a state-ware subdev set_fmt op is called
with a NULL state pointer is if it is called from non-MC bridge drivers? And if
I hope so. There could be MC bridge or subdev drivers that call set_fmt
(or get_fmt) on other subdevs. I cannot figure out why that would be
done, though, and it sounds wrong to me. Afaik, with MC, set_fmt (and
other such calls) always come from the userspace, and each subdev is
configured separately from the others.
But if there are such drivers, they'll get fixed along the non-MC bridge
drivers.
we would update those bridge drivers to always pass a non-NULL state pointer,
then such subdevs no longer need to care about this and can just use the state.
That's correct.
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.
That sounds good to me.
The only legacy bit about the macro, though, is the fact that v4l2_subdev_get_active_state()
can return NULL: that indicates that the subdev driver isn't properly state-aware.
So I think this should be a regular macro in v4l2-subdev.h.
Perhaps with a comment mentioning that v4l2_subdev_get_active_state() can be replaced by
v4l2_subdev_lock_and_get_active_state() once all subdevs are state-aware.
Yes, that's true. I'll add the comment, and move this back to v4l2-subdev.h.
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.
I believe changing the callers is the correct approach. Next to that I want to slowly
convert all subdevs to be state-aware (i.e. use sd->active_state etc). Based on past
experience it is a really bad idea to have all these variations in how subdevs work.
That should be minimized.
I suspect that modifying the callers isn't as bad as you might think. It is also the
sane approach: passing the active_state to the subdev really *is* what you want to do.
It is not a hack, it is the right thing. Adding a workaround in a subdev where a
NULL state is handled separately is, however, a hack, and a hack that is all to easy
to forget to implement in new drivers.
Ok.
I'll send a v6 soon with the changes we've discussed, and I'll take a
look at ov5640.c and changing the callers.
Tomi
