Hi Hans, On Tuesday 02 December 2014 20:34:22 Hans Verkuil wrote: > 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? You can't do that, because format and selection rectangles on a pad can depend on the formats and selection rectangles on other pads of the same subdev. For instance setting the format on the source pad of a scaler will ignore the media bus code and return the code configured on the sink pad. A full configuration must thus be passed to the pad operations. > 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. Exactly :-) > 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. That's done all over the place, look at the OMAP3 ISP subdev for instance. > Let me know what you think, -- Regards, Laurent Pinchart -- 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
