Hi Laurent, On 12/02/2014 01:59 PM, Laurent Pinchart wrote: >>>> Basically, >>>> >>>> 1. Create a subdev pad configuration store structure to store the formats >>>> and selection rectangles for each pad. >>> >>> I wouldn't call it a 'store'. Just call it fmt_config or pad_config >>> something like that. > > Sure, the name doesn't matter too much. > >>>> 2. Embed an instance of that structure in v4l2_subdev_fh. >>>> >>>> 3. Modify the subdev pad ops to take a configuration store pointer >>>> instead of a file handle pointer. >>>> >>>> The userspace API implementation (v4l2-subdev.c) would then pass >>>> &fh->store to the pad operations instead of fh. >>>> >>>> Bridge drivers that need to implement TRY_FMT on top of pad ops would >>>> create a temporary store (or temporary stores when multiple subsdevs are >>>> involved), call the pad ops with a pointer to the temporary store to >>>> propagate TRY formats, destroy the store(s) and return the resulting >>>> format. >>> >>> That will work. I think this is a good approach and it shouldn't be too >>> difficult. >> >> Laurent, just so I understand this correctly: does this mean that all >> occurrences of 'struct v4l2_subdev_fh *fh' will be replaced by 'struct >> v4l2_subdev_pad_config *cfg'? > > That's the plan, yes. > >> Is there any reason why the 'fh' should still be passed on? > > We might find out reasons to still pass the fh, but in that case I think they > should be addressed and the fh just dropped from the pad ops arguments. > >> Personally I am in favor of this since the 'fh' always made it hard for >> bridge drivers to use these pad ops. So if we can replace it by something >> that can be used by bridge drivers as well, then that will make it easier >> to move all drivers over to the pad ops. > > Good, looks like we have a plan for world domination :-) > OK, so I couldn't help myself and I did this conversion. The code is here: http://git.linuxtv.org/cgit.cgi/hverkuil/media_tree.git/log/?h=remcrop And it introduces a new struct: struct v4l2_subdev_pad_config { struct v4l2_mbus_framefmt try_fmt; struct v4l2_rect try_crop; struct v4l2_rect try_compose; }; And subdev_fh now looks like this: struct v4l2_subdev_fh { struct v4l2_fh vfh; struct v4l2_subdev_pad_config *pad; }; But I realized that it could be simplified: right now we pass a pointer to the pad config array to the pad ops and the v4l2_subdev_get_try_* functions pick the right entry based on the pad index. Wouldn't it be easier to pass the right pad config directly? So instead of passing fh->pad in v4l2-subdev.c you pass e.g. 'fh->pad + sel->pad'. That way the pad_config pointer points straight to the configuration of the requested pad. This also simplifies things in a bridge driver: it doesn't need to allocate a pad config array, it only has to create pad config structs for those pads that it actually uses. And the v4l2_subdev_get_try_* functions can effectively be removed, since you would just access cfg->try_fmt directly. I never liked those helper functions, so I'm not sorry to see them go. The only reason why this would fail is if drivers need to access the pad_config of other pads than the one currently requested. I have not seen any code that does that, though. And I don't think that is something we want anyway since that would make things extremely complex. Let me know what you think, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
