Re: [PATCH 2/6] media: v4l: subdev: Also return pads array information on stream functions

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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:
> > Hi Sakari,
> > 
> > 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?

-- 
Sakari Ailus



[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux