[23:21] <koike> sailus, Hi. in the v4l2_subdev_link_validate function, if you can't get the format, 0 is returned, I would like to check why with you (as you are the author of the code) because I am implementing my own link_validate function and I would like to know if I should return 0 as well. Thank you
[10:50] *** hverkuil_ changes his alias to hverkuil
[12:00] *** headless_ changes his alias to headless
[12:15] <pinchartl> hverkuil: it still bothers me a bit. I was wondering whether we couldn't add a flag to struct v4l2_ext_control instead. when set for G_EXT_CTRLS it would indicate that the default value should be read, and when set for S_EXT_CTRLS it would indicate that the user wants to set the control to its default value
[12:15] <pinchartl> that would avoid adding a new ioctl
[12:15] <pinchartl> and would allow setting a control to its default value without having to read it first
[12:16] <pinchartl> I also liked Sakari's proposal of extending the query control ioctl instead of adding a new ioctl
[12:16] <pinchartl> but I agree it makes it a bit more difficult for userspace to reset a control to its default value
[12:16] <pinchartl> not much though
[12:16] <pinchartl> what bothers me is that we treat the default value in a specific way, even though it's already reported by the query control ioctl
[12:17] <pinchartl> one of the benefits of the new proposed ioctl is that it would allow getting the default value of multiple controls in a single call
[12:18] <pinchartl> if we think that's important, wouldn't it make sense to instead create a new query control ioctl that could query multiple controls in a single go, without restricting that to just the default value ?
[13:43] <hverkuil> pinchartl: it's the fact that you can get the default values and set them with just two ioctls that is so nice. Even with a query_ext_ctrls ioctl you still cannot do that.
[13:47] <hverkuil> Using a flag instead of a new ioctl is something I thought about, but I'd have to take one of the only two reserved fields in struct v4l2_ext_controls.
[13:47] <hverkuil> It's also something that only applies to getting values, never for try/set.
[13:49] <pinchartl> yes, the lack of reserved fields is something that bothers me too
[13:49] <pinchartl> they're scarse resources
[13:49] <pinchartl> the flag could apply to set though, as I've explained above
[13:50] <pinchartl> applications could use it to set a control to its default value without having to read it first
[13:50] <pinchartl> the value would be ignored when the flag is set
[13:52] <pinchartl> don't we have a single reserved field in v4l2_ext_controls, not two ?
[13:52] <hverkuil> v4l2_ext_controls has two, v4l2_ext_control has one.
[13:53] <pinchartl> yes, my bad
[13:53] <pinchartl> I was thinking about adding the flag to v4l2_ext_control
[13:54] <hverkuil> It's just ugly. You get exceptions: if the flag is set, then use the default value. But what does that mean for the union? Do you still have to allocate it? And it's tricky in v4l2-ctrls as well.
[13:55] <pinchartl> my concern is that I feel we're really special-casing this while it could be more generic, and I bet it will bite us back when we'll get to the next similar-by-slightly-different use case
[13:55] <pinchartl> I've described the various ideas I had to make this a bit more generic
[13:55] <pinchartl> but none of them are clearly better and 100fb6158erfect
[13:56] <pinchartl> so I won't nack the proposed approach, but I still feel we could do better
[13:56] <pinchartl> (back in 15 minutes)
[14:04] <hverkuil> One option might be to create a V4L2_CTRL_CLASS_DEFAULTS control class that can be used in the ctrl_class field. It's only supported (at least for now) by G_EXT_CTRLS.
[14:08] <hverkuil> it still feels a bit like a hack, but at least it does allow it to be extended to e.g. CLASS_MIN, MAX, STEP should we ever want that.
[14:11] <pinchartl> hmmmm...
[14:11] <pinchartl> a bit hackish indeed
[14:11] <pinchartl> I need to go shopping from groceries before the store closes
[14:11] <pinchartl> I'll be back a bit later
[14:13] <hverkuil> ctrl_class should really be renamed to 'which'
[14:13] <hverkuil> 0 = current values
[14:14] <hverkuil> existing control classes == 'current values for those control classes' (only needed for backwards compat)
[14:14] <hverkuil> V4L2_CTRL_WHICH_DEFAULTS == default control values
[14:15] <hverkuil> and with the request patch series (per frame configuration) you'd get 'control values for the given request ID'
[14:17] <hverkuil> trying to use WHICH_DEFAULTS with S/TRY_EXT_CTRLS would naturally return EACCES since the default control values are read-only.
[18:54] *** demhlyr changes his alias to demhlyr_
[18:55] *** demhlyr_ changes his alias to demhlyr9
[18:55] *** demhlyr9 changes his alias to demhlyr
[21:10] <sailus> koike: Getting the format should only fail if the driver doesn't implement the op. We haven't had a case where that'd happen yet, so I don't think it's a very important question as such.
[21:11] <sailus> However if the entity type is wrong, that's indeed a more serious problem. This apparently isn't handled properly at the moment.
[21:12] <sailus> But to answer your question, I think I'd return zero if you simply can't get the format for a pad.
[22:49] <koike> sailus, thank you for your answer. I think I'll return error, as all the subdevices in my topology should implement the get format ops