Re: Sensor sub-device - what are the mandatory ops?
Hi Sakari . Thanks for taking the time to respond. On 17 May 2017 at 00:04, Sakari Ailus wrote: > Hi Dave, > > On Thu, May 11, 2017 at 04:51:27PM +0100, Dave Stevenson wrote: >> Hi All. >> >> As previously discussed, I'm working on a V4L2 driver for the >> CSI-2/CCP2 receiver on BCM283x, as used on Raspberry Pi. >> It's a relatively simple hardware block that writes received data into >> SDRAM, and only accepts connection from one "sensor" sub device, so no >> need to involve the media controller API. (The peripheral can do >> cropping and format conversion between the CSI-2 Bayer formats too, >> but I'm ignoring those for now, and even so they don't really need >> media controller). > > If the sensor exposes multiple sub-devices or you have e.g. a more complex > TV tuner with internal data routing such as some of the ADV chips, you > won't be able to support them in this model. > > On the other hand, using Media controller brings some extra complexity and > requires generally a lot more decision making and configuration from the > user space as well. (This is something that needs to be more or less > resolved over time but how long it'll take I can't say.) > > Just FYI. Thanks for the warning. I was aware that I might be limiting the use of some drivers, but I'm also aware that the majority of users on the Pi are not "power users", and if it takes much more than opening /dev/video0 then they'll lose interest. The CSI-2 sensor drivers that I've looked at haven't had vast complexity. As and when any get added we can look at adding in media controller support. >> I was previously advised by Hans to take am437x as a base, and that >> seems to have worked pretty well when combined with some of the ti-vpe >> driver too. It's up and running, although with some rough edges. I'm >> hoping to sort an RFC in a week or so. >> >> My main issue is determining what calls are mandatory to be supported >> by the sensor sub-device drivers that attach to the CSI-2 receiver. >> I'm either taking the wrong approach, or there seem to be missing ops >> in the drivers I'm trying to use. The set of devices I have available >> are Omnivision OV5647, Toshiba TC358743 HDMI to CSI2 bridge, and >> ADV7282-M analogue video to CSI-2 decoder. >> >> The TC358743 driver doesn't support: >> - enum_mbus_code to report the supported formats >> (MEDIA_BUS_FMT_RGB888_1X24 and MEDIA_BUS_FMT_UYVY8_1X16) > > Without knowing any details on the chip nor its driver, if it supports the > two, it'd be reasonable that it also provided such enumeration of mbus > codes. It does support both formats, so, I'll submit that patch. >> - s_power. The docs [1] say the device must be powered up before >> calling v4l2_subdev_video_ops->s_stream, but is s_power optional so >> ENOIOCTLCMD is not to be considered a failure? > > Correct. Easy enough to handle. >> - enum_frame_size >> and doesn't set the state->mbus_fmt_code until after >> v4l2_async_register_subdev. A connected subdevice calling get_fmt >> during the notifier.complete callback gets a code of 0. >> >> The OV5647 driver doesn't support: >> - set_fmt or get_fmt. I can't see any code that returns the 640x480 >> sensor resolution that is listed in the commit text. > > get_fmt is essential. Without that link validation can't work. If the driver > doesn't support setting the format, the get_fmt callback can be used for > set_fmt as well. Another patch to submit then. Using get_fmt in the absence of set_fmt feels a little nasty, but probably a sensible precaution. >> - g_mbus_config, so no information on the number of CSI-2 lanes in use >> beyond that in DT. Do we just assume all lines specified in DT are in >> use in this situation? In which case should the driver be checking >> that the configured number of lanes matches the register set it will >> request over I2C, as a mismatch will result in it not working? > > The assumption is that all lanes are in use. g_mbus_config is from SoC > camera and, well, frankly should not look like how it looks now. It's used > by a couple of drivers and needs to be replaced by more generic and > informative means to let the receiver know the frame structure the > transmitter is about to start sending. So it is what it is, but isn't mandatory. Dropping it is likely to cause issues with the TC358743. It receives HDMI data in at whatever rate is required for the format, stores it in a small FIFO, and when a trigger point is reached in the FIFO it is read out over CSI2 with N data lanes at a defined rate. Under or over flow that FIFO and you get corrupted images. It dynamically sets how many data lanes to use to roughly match the HDMI and CSI2 data rates without causing FIFO issues. (I am getting issues with some resolution/framerate/pixelformat combinations, so something seems a little off. That's for a different discussion though). I'll update my driver to take the DT config, and update if g_mbus_config is supported. I'll drop my OV5647 patch for this. >> -
Re: Sensor sub-device - what are the mandatory ops?
Hi Dave, On Thu, May 11, 2017 at 04:51:27PM +0100, Dave Stevenson wrote: > Hi All. > > As previously discussed, I'm working on a V4L2 driver for the > CSI-2/CCP2 receiver on BCM283x, as used on Raspberry Pi. > It's a relatively simple hardware block that writes received data into > SDRAM, and only accepts connection from one "sensor" sub device, so no > need to involve the media controller API. (The peripheral can do > cropping and format conversion between the CSI-2 Bayer formats too, > but I'm ignoring those for now, and even so they don't really need > media controller). If the sensor exposes multiple sub-devices or you have e.g. a more complex TV tuner with internal data routing such as some of the ADV chips, you won't be able to support them in this model. On the other hand, using Media controller brings some extra complexity and requires generally a lot more decision making and configuration from the user space as well. (This is something that needs to be more or less resolved over time but how long it'll take I can't say.) Just FYI. > I was previously advised by Hans to take am437x as a base, and that > seems to have worked pretty well when combined with some of the ti-vpe > driver too. It's up and running, although with some rough edges. I'm > hoping to sort an RFC in a week or so. > > My main issue is determining what calls are mandatory to be supported > by the sensor sub-device drivers that attach to the CSI-2 receiver. > I'm either taking the wrong approach, or there seem to be missing ops > in the drivers I'm trying to use. The set of devices I have available > are Omnivision OV5647, Toshiba TC358743 HDMI to CSI2 bridge, and > ADV7282-M analogue video to CSI-2 decoder. > > The TC358743 driver doesn't support: > - enum_mbus_code to report the supported formats > (MEDIA_BUS_FMT_RGB888_1X24 and MEDIA_BUS_FMT_UYVY8_1X16) Without knowing any details on the chip nor its driver, if it supports the two, it'd be reasonable that it also provided such enumeration of mbus codes. > - s_power. The docs [1] say the device must be powered up before > calling v4l2_subdev_video_ops->s_stream, but is s_power optional so > ENOIOCTLCMD is not to be considered a failure? Correct. > - enum_frame_size > and doesn't set the state->mbus_fmt_code until after > v4l2_async_register_subdev. A connected subdevice calling get_fmt > during the notifier.complete callback gets a code of 0. > > The OV5647 driver doesn't support: > - set_fmt or get_fmt. I can't see any code that returns the 640x480 > sensor resolution that is listed in the commit text. get_fmt is essential. Without that link validation can't work. If the driver doesn't support setting the format, the get_fmt callback can be used for set_fmt as well. > - g_mbus_config, so no information on the number of CSI-2 lanes in use > beyond that in DT. Do we just assume all lines specified in DT are in > use in this situation? In which case should the driver be checking > that the configured number of lanes matches the register set it will > request over I2C, as a mismatch will result in it not working? The assumption is that all lanes are in use. g_mbus_config is from SoC camera and, well, frankly should not look like how it looks now. It's used by a couple of drivers and needs to be replaced by more generic and informative means to let the receiver know the frame structure the transmitter is about to start sending. > - enum_frame_size > > ADV7180/7282-M > - enum_frame_size > > I've listed enum_frame_size as that is what TI VPE driver uses in > cal_try_fmt_vid_cap. It seems more sensible to pass the request in to > set_fmt with which = V4L2_SUBDEV_FORMAT_TRY, so is this actually an > issue with the TI driver doing the wrong thing? (FORMAT_TRY seems to > work reasonably). There must be a gap somewhere --- enum_frame_size provides a list of supported frame sizes by the hardware. That shouldn't be dependent on the device configuration. Nor it can --- the context provided has no clue about it. I think this was mostly added to mirror the original sub-device API in kernel: its sole purpose was to implement VIDIOC_ENUM_FRAMESIZES. > > Those are the issues I've hit on those 3 drivers. Is there a defintive > list of what must be supported by drivers, and any checklist for > drivers during review? I'm afraid there isn't. There tend to be many different kinds of devices and they need a little bit different parts of the APIs. And as a result the interoperability of reciver - sensor pairs has not been quite up to what one could have expected. Still, that has significantly improved over recent years, e.g. due to conversion of SoC camera drivers to sub-device drivers. > > I have patches for the TC358743 and OV5647 which I can post to the > list if it is agreed that the above are issues rather than me doing > the wrong thing. > > > Follow-up question on g_mbus_format. The V4L2_MBUS_CSI2_x_LANE defines > appear to have been specfied though they should be use
Sensor sub-device - what are the mandatory ops?
Hi All. As previously discussed, I'm working on a V4L2 driver for the CSI-2/CCP2 receiver on BCM283x, as used on Raspberry Pi. It's a relatively simple hardware block that writes received data into SDRAM, and only accepts connection from one "sensor" sub device, so no need to involve the media controller API. (The peripheral can do cropping and format conversion between the CSI-2 Bayer formats too, but I'm ignoring those for now, and even so they don't really need media controller). I was previously advised by Hans to take am437x as a base, and that seems to have worked pretty well when combined with some of the ti-vpe driver too. It's up and running, although with some rough edges. I'm hoping to sort an RFC in a week or so. My main issue is determining what calls are mandatory to be supported by the sensor sub-device drivers that attach to the CSI-2 receiver. I'm either taking the wrong approach, or there seem to be missing ops in the drivers I'm trying to use. The set of devices I have available are Omnivision OV5647, Toshiba TC358743 HDMI to CSI2 bridge, and ADV7282-M analogue video to CSI-2 decoder. The TC358743 driver doesn't support: - enum_mbus_code to report the supported formats (MEDIA_BUS_FMT_RGB888_1X24 and MEDIA_BUS_FMT_UYVY8_1X16) - s_power. The docs [1] say the device must be powered up before calling v4l2_subdev_video_ops->s_stream, but is s_power optional so ENOIOCTLCMD is not to be considered a failure? - enum_frame_size and doesn't set the state->mbus_fmt_code until after v4l2_async_register_subdev. A connected subdevice calling get_fmt during the notifier.complete callback gets a code of 0. The OV5647 driver doesn't support: - set_fmt or get_fmt. I can't see any code that returns the 640x480 sensor resolution that is listed in the commit text. - g_mbus_config, so no information on the number of CSI-2 lanes in use beyond that in DT. Do we just assume all lines specified in DT are in use in this situation? In which case should the driver be checking that the configured number of lanes matches the register set it will request over I2C, as a mismatch will result in it not working? - enum_frame_size ADV7180/7282-M - enum_frame_size I've listed enum_frame_size as that is what TI VPE driver uses in cal_try_fmt_vid_cap. It seems more sensible to pass the request in to set_fmt with which = V4L2_SUBDEV_FORMAT_TRY, so is this actually an issue with the TI driver doing the wrong thing? (FORMAT_TRY seems to work reasonably). Those are the issues I've hit on those 3 drivers. Is there a defintive list of what must be supported by drivers, and any checklist for drivers during review? I have patches for the TC358743 and OV5647 which I can post to the list if it is agreed that the above are issues rather than me doing the wrong thing. Follow-up question on g_mbus_format. The V4L2_MBUS_CSI2_x_LANE defines appear to have been specfied though they should be used as a bitmask, but based on existing drivers (mainly TC358743) only one is allowed to be set to denote the actual number of lanes used. Is that the correct interpretation? If so I guess we need error checking on the flags passed in. One last question. Putting a user's hat on, what is the expected mapping of vidioc_s_input to s_routing? Looking at my use case, the CSI-2 receiver driver is the code that creates /dev/videoN, but it otherwise is just a proxy for the sensor device. It therefore makes the user's life easy if calls such as input, EDID, dv_timings, and std functions are just passed straight through to the sensor, so the user can ignore the subdev API. For input there appears to be no way to produce an implementation of vidioc_enum_input. Looking at the ADV7282-M (uses ADV7180 driver), I can't see any way of reading out the valid input numbers as would be needed for enum_input. vidioc_g_input can be done by the CSI-2 receiver driver assuming/setting to input 0 during probe, and then caching the last set value, but that feels a little nasty. Have I missed something there? Thanks in advance. Dave [1] https://linuxtv.org/downloads/v4l-dvb-apis-new/kapi/csi2.html#receiver-drivers
