On Mon, Oct 16, 2023 at 10:21:45AM +0000, Sakari Ailus wrote: > On Mon, Oct 16, 2023 at 08:59:15AM +0000, Sakari Ailus wrote: > > On Mon, Oct 16, 2023 at 11:24:52AM +0300, Laurent Pinchart wrote: > > > On Fri, Oct 13, 2023 at 11:13:00AM +0000, Sakari Ailus wrote: > > > > On Fri, Oct 13, 2023 at 02:07:41PM +0300, Laurent Pinchart wrote: > > > > > On Fri, Oct 13, 2023 at 01:44:20PM +0300, Sakari Ailus wrote: > > > > > > There are two sets of functions that return information from sub-device > > > > > > state, one for stream-unaware users and another for stream-aware users. > > > > > > Add support for stream-aware functions to return format, crop and compose > > > > > > information from pad-based array that are functionally equivalent to the > > > > > > old, stream-unaware ones. > > > > > > > > > > > > Also check state is non-NULL, in order to guard against old drivers > > > > > > potentially calling this with NULL state for active formats or selection > > > > > > rectangles. > > > > > > > > > > I'm not too keen on this I'm afraid :-( I think it gets confusing for > > > > > drivers that are not stream-aware to have to call a function that takes > > > > > a stream number. I don't see a problem with keeping two different sets > > > > > of functions, one for stream-aware drivers, and one for other drivers. > > > > > > > > This becomes a nuisance in drivers such as CCS that work with sub-devices > > > > some of which have streams and others which don't. I don't see why we > > > > should have two sets of functions to access the same information, even > > > > though it's stored differently. > > > > > > > > I can add a wrapper using C11 _Generic to make the stream number go away. > > > > > > That could possibly be interesting. > > > > I'll add this for v2. > > C11 _Generic() isn't up to the task as it only deals with argument types. On > the other hand, variadic arguments supports this. It'll look like: > > #define v4l2_subdev_get_crop(state, pad, ...) \ > __v4l2_subdev_get_crop_ ## __VA_OPT__(stream) \ > (state, pad __VA_OPT__(,) __VA_ARGS__) > #define __v4l2_subdev_get_crop_(state, pad) \ > __v4l2_subdev_get_crop(state, pad, 0) > #define __v4l2_subdev_get_crop_stream(state, pad, stream) \ > __v4l2_subdev_get_crop(state, pad, stream) > struct v4l2_rect * > __v4l2_subdev_get_crop(struct v4l2_subdev_state *state, unsigned int pad, > u32 stream); > > Which I'd say is tolerable, given the result is a single function to access > format, crop and compose information in the sub-device state. > > Any thoughts on this? With s/v4l2_subdev_get_crop/v4l2_subdev_state_get_crop/ I'm OK with this. -- Regards, Laurent Pinchart
