Hi Sylwester, On Friday 09 September 2011 19:37:55 Sylwester Nawrocki wrote: > On 09/08/2011 12:21 PM, Laurent Pinchart wrote: > > On Tuesday 06 September 2011 23:07:43 Sakari Ailus wrote: > >> On Tue, Sep 06, 2011 at 09:10:17PM +0200, Sylwester Nawrocki wrote: > >>> On 09/05/2011 05:55 PM, Sakari Ailus wrote: > >>>> Hi all, > >>>> > >>>> I recently came across a few issues in the definitions of > >>>> v4l2_subdev_format and v4l2_mbus_framefmt when I was working on sensor > >>>> control that I wanted to bring up here. The appropriate structure > >>>> right now look like this: > >>>> > >>>> include/linux/v4l2-subdev.h: > >>>> ---8<--- > >>>> /** > >>>> > >>>> * struct v4l2_subdev_format - Pad-level media bus format > >>>> * @which: format type (from enum v4l2_subdev_format_whence) > >>>> * @pad: pad number, as reported by the media API > >>>> * @format: media bus format (format code and frame size) > >>>> */ > >>>> > >>>> struct v4l2_subdev_format { > >>>> > >>>> __u32 which; > >>>> __u32 pad; > >>>> struct v4l2_mbus_framefmt format; > >>>> __u32 reserved[8]; > >>>> > >>>> }; > >>>> ---8<--- > >>>> > >>>> include/linux/v4l2-mediabus.h: > >>>> ---8<--- > >>>> /** > >>>> > >>>> * struct v4l2_mbus_framefmt - frame format on the media bus > >>>> * @width: frame width > >>>> * @height: frame height > >>>> * @code: data format code (from enum v4l2_mbus_pixelcode) > >>>> * @field: used interlacing type (from enum v4l2_field) > >>>> * @colorspace: colorspace of the data (from enum v4l2_colorspace) > >>>> */ > >>>> > >>>> struct v4l2_mbus_framefmt { > >>>> > >>>> __u32 width; > >>>> __u32 height; > >>>> __u32 code; > >>>> __u32 field; > >>>> __u32 colorspace; > >>>> __u32 reserved[7]; > >>>> > >>>> }; > >>>> ---8<--- > >>>> > >>>> Offering a lower level interface for sensors which allows better > >>>> control of them from the user space involves providing the link > >>>> frequency to the user space. While the link frequency will be a > >>>> control, together with the bus type and number of lanes (on serial > >>>> links), this will define the pixel clock. > >>>> > >>>> <URL:http://www.spinics.net/lists/linux-media/msg36492.html> > >>>> > >>>> After adding pixel clock to v4l2_mbus_framefmt there will be six > >>>> reserved fields left, one of which will be further possibly consumed > >>>> by maximum image size: > >>>> > >>>> <URL:http://www.spinics.net/lists/linux-media/msg35949.html> > >>> > >>> Yes, thanks for remembering about it. I have done some experiments with > >>> a sensor producing JPEG data and I'd like to add '__u32 framesamples' > >>> field to struct v4l2_mbus_framefmt, even though it solves only part of > >>> the problem. I'm not sure when I'll be able to get this finished > >>> though. I've just attached the initial patch now. > >>> > >>>> Frame blanking (horizontal and vertical) and number of lanes might be > >>>> needed in the struct as well in the future, bringing the reserved > >>>> count down to two. I find this alarmingly low for a relatively new > >>>> structure definition which will potentially have a few different uses > >>>> in the future. > >>> > >>> Sorry, could you explain why we need to put the blanking information in > >>> struct v4l2_mbus_framefmt ? I thought it had been initially agreed that > >>> the control framework will be used for this. > >> > >> Configuration of blanking will be implemented as controls, yes. > >> > >> Bandwidth calculation in the ISP driver may well need to know more > >> detailed information than just the maximum pixel rate. Averge rate over > >> certain period may also be important. > >> > >> For example, take a sensor which is able to produce pixel rate of 200 > >> Mp/s. In the OMAP 3 ISP only the CSI2 block will be able to process > >> pixels at such rate. The ISP driver needs this information to be able > >> to decide whether it's safe to start streaming or not. > >> > >> Higher momentary pixel rates are still possible as there are buffers > >> between some of the blocks. When using downscaling on sensors this gets > >> more tricky. There may be bursts of data which may overflow these > >> buffers since the sensors do not output data at amortised rate. > >> Information on the sensor (bursts) and size of the buffers is at least > >> required to assess this question. > >> > >> I have a vague feeling we may need some of this data as part of the > >> v4l2_mbus_framefmt before we have a solution. > > > > Do we really need to make all this (including the proposed framesamples > > field) part of v4l2_mbus_framefmt ? My understanding is that the > > information needs to be propagated down the pipeline to verify pipeline > > validity at streamon time and to configure blocks down in the chain. > > That's an in-kernel requirement, wouldn't it be better to use an > > in-kernel API for that instead of requiring userspace to do the job ? > > I'll hold on for a moment on commenting the handling of blanking > information and the pixel clock in user space, but as far as memory buffer > size negotiation between drivers is concerned it always felt more > appropriate to me to handle such things in the kernel and isolate that > from user space. > > Actually we need to retrieve the size of the buffer during allocation time > in the host driver. So even if we have added maximum buffer size > information to struct v4l2_mbus_framefmt the format would have to be > queried internally from a sensor subdev. > > I can't really think of any usage of the v4l2_mbus_framefmt::framesamples > field in user space ATM. The MIPI CSI receiver drivers which transfer data > directly to memory will, AFAIU, always expose a video node, so those > subdevs could possibly negotiate the buffer size in kernel with sensor > subdev directly.
This has my preference. We can add a field to an in-kernel structure for this, or add a new subdev operation. As this will be a pure in-kernel API it will be easier to change it later if needed. -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html