Moi, On Tue, Oct 10, 2023 at 11:03:40AM +0300, Tomi Valkeinen wrote: > On 10/10/2023 10:46, Sakari Ailus wrote: > > Moi, > > > > On Tue, Oct 10, 2023 at 10:40:05AM +0300, Tomi Valkeinen wrote: > > > On 10/10/2023 10:23, Sakari Ailus wrote: > > > > Moi, > > > > > > > > On Tue, Oct 10, 2023 at 09:52:48AM +0300, Tomi Valkeinen wrote: > > > > > On 10/10/2023 01:09, Sakari Ailus wrote: > > > > > > Hi folks, > > > > > > > > > > > > This set includes a helper for working with V4L2 sub-device active state as > > > > > > well as a fix for the CCS driver sub-device state patch. > > > > > > > > > > > > since v1: > > > > > > > > > > > > - There were other drivers using __v4l2_subdev_state_alloc(). Ouch. Change > > > > > > those users as well. Note that this function should not be used in > > > > > > drivers, hence API niceness is not a high priority. > > > > > > > > > > > > Sakari Ailus (4): > > > > > > media: v4l: subdev: Set sub-device active state earlier > > > > > > media: v4l: subdev: Add a helper to tell if a sub-device state is > > > > > > active > > > > > > media: ccs: Rework initialising sub-device state > > > > > > media: ccs: Fix a (harmless) lockdep warning > > > > > > > > > > > > drivers/media/i2c/ccs/ccs-core.c | 64 ++++++++++++------- > > > > > > .../platform/renesas/rcar-vin/rcar-v4l2.c | 3 +- > > > > > > .../media/platform/renesas/vsp1/vsp1_entity.c | 3 +- > > > > > > drivers/media/v4l2-core/v4l2-subdev.c | 14 ++-- > > > > > > drivers/staging/media/tegra-video/vi.c | 2 +- > > > > > > include/media/v4l2-subdev.h | 11 +++- > > > > > > 6 files changed, 66 insertions(+), 31 deletions(-) > > > > > > > > > > > > > > > > I'm not familiar with the CCS driver, and you don't explain much why you are > > > > > doing this, so it's a bit unclear to me. But I don't like it. > > > > > > > > > > The idea with the subdev state was that the driver doesn't need to know > > > > > whether it's active-state or try-state. It should behave the same way in > > > > > both cases. This series goes against that. > > > > > > > > > > Can you explain a bit what the issue is and what you are doing in this > > > > > series? > > > > > > > > The driver maintains internal state and that needs to be updated when the > > > > configuration (including what's in sub-device state) changes. Generally the > > > > driver knows (as for the whence field) which state it's dealing with but > > > > that is not the case for init_cfg. > > > > > > So you need to set the subdev state to sd->active_state earlier (in patch 1) > > > so that the driver can use v4l2_subdev_state_whence()? You don't really > > > explain that in the patch descriptions. > > > > > > In other words, if init_cfg() were to get a whence-parameter, this wouldn't > > > be needed? (I don't want to do that, just trying to understand what's going > > > on). > > > > Yes, that's correct. > > Ok. So the driver keeps some private state separately from the subdev state, > and keeps track of only the active state details, and thus it needs to know > the whence in init_cfg so that it can fill in those details only when > dealing with active state. Precisely so. > > > > > Alternatively I could split the internal workings of the driver into active > > > > and try states but I prefer to improve the framework and make the driver > > > > simpler. > > > > > > I agree with the goal, but I don't think this is improving the framework. > > > > > > If we decide that we need to know if a state is an active-state or a > > > try-state, I think it's better to add a whence-field into the state itself. > > > But I'd rather not. > > > > In all other callbacks the whence is known but not in init_cfg. > > Yes, those should be removed =). > > Well, that may not work, but at least we should consider if a specific > callback should ever care about active/try, and if not, remove the whence. > > > Comparing with sd->active_state does this pretty cleanly, without a need to > > change APIs. > > > > > > > > > Deducing the internal configuration solely based on sub-device state is not > > > > really feasible. > > > > > > Am I correct in that what you really need is a way to sub-class the subdev > > > state, so that you can have all that internal state in the subdev state, and > > > thus all the code in the driver can work in "whence-agnostic" manner? That's > > > something we've been thinking about for a long time. > > > > In the case of the CCS driver it's not necessary to store these parameters > > for try states: they are driver internal only. > > That's fine, the sub-classed state can only have driver internal data (in > addition to the standard data). > > I think a follow-up question here is: why do you need to keep track of state > that's only needed for active state? What is that state? Sensor register values effectively. These are programmed just before streaming starts. There's scaling factor, whether scaling is horizontal-only, both vertical and horizontal or if there's no scaling. Similarly for binning. > > > > But the driver needs to be fixed now, not at some point in the future, of > > > course. If the sub-classing would solve the issue (i.e. we have a plan in > > > mind), I'd rather just hack this in the driver, instead of extending the > > > framework, which might easily lead to other drivers going the wrong way too. > > > > > > How about a private flag, set before calling v4l2_subdev_init_finalize(), > > > and unset after the call. ccs_init_cfg() can look at that flag an if it's > > > set, it's initializing an active state. > > > > I dislike using driver specific flags in generic APIs. > > I meant in the driver private state. Add a field to the driver's private > data struct, and use that. Then it's wholly inside the driver, and needs > just a few lines of hack-code, easy to remove later. > > > While trivial sensor drivers have no use for such functionality, I would be > > surprised if there would not be other similar cases. > > Can you open this up a bit? Why do drivers need to know if an operation is > about active or try state? > > Answering my own question: > > In some cases it's implicit: when starting the streaming, it's always active > state. Similarly in, e.g., get_frame_desc. > > Some operations might allow changing HW settings while streaming is enabled. > I guess set_fmt might allow changing some specific things even while > streaming is enabled. And if so, the callback needs to know if this is for > active state or not. > > But why does a driver need to know the whence in init_cfg? With sub-classed > state, init_cfg should work fine for both active-state and try-state. See above. While init_cfg does not (obviously) enable binning or scaling, it still deals with the same configuration parameters. -- Regards, Sakari Ailus
