RE: [PATCH v2 3/3] media: pci: intel: ivsc: Add acquire/release API for ivsc

[Wu, Wentong]

> -----Original Message-----
> From: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> Sent: Monday, February 20, 2023 4:37 PM
> 
> Hi Wentong,
> 
> On Mon, Feb 20, 2023 at 03:50:55AM +0000, Wu, Wentong wrote:
> >
> >
> > > -----Original Message-----
> > > From: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> > > Sent: Friday, February 17, 2023 7:19 PM
> > >
> > > Hi Wentong,
> > >
> > > On Fri, Feb 17, 2023 at 06:10:22AM +0000, Wu, Wentong wrote:
> > > > On Sent: Wednesday, February 15, 2023 5:46 PM, Laurent Pinchart wrote:
> > > > > On Wed, Feb 15, 2023 at 10:03:29AM +0100, Hans de Goede wrote:
> > > > > > On 2/14/23 17:06, Sakari Ailus wrote:
> > > > > > > On Mon, Feb 13, 2023 at 10:23:47AM +0800, Wentong Wu wrote:
> > > > > > >> IVSC directly connects to camera sensor on source side, and
> > > > > > >> on output side it not only connects ISH via I2C, but also
> > > > > > >> exposes MIPI
> > > > > > >> CSI-2 interface to output camera sensor data. IVSC can use
> > > > > > >> the camera sensor data to do AI algorithm, and send the results to
> ISH.
> > > > > > >> On the other end, IVSC can share camera sensor to host by
> > > > > > >> routing the raw camera sensor data to the exposed MIPI
> > > > > > >> CSI-2 interface. But they can not work at the same time, so
> > > > > > >> software APIs are defined to sync the ownership.
> > > > > > >>
> > > > > > >> This commit defines the interfaces between IVSC and camera
> > > > > > >> sensor driver in include/linux/ivsc.h. The camera driver
> > > > > > >> controls ownership of the CSI-2 link and sensor with the
> > > > > > >> acquire/release APIs. When acquiring camera, lane number
> > > > > > >> and link freq are also required by IVSC frame router.
> > > > > > >
> > > > > > > The more I learn about this system, the more I'm inclined to
> > > > > > > think this functionality should be exposed as a V4L2 sub-device.
> > > > > > > IVSC doesn't really do anything to the data (as long as it
> > > > > > > directs it towards the CSI-2 receiver in the SoC), but it is
> > > > > > > definitely part of the image pipeline.
> > > > > >
> > > > > > Yes I happened to discuss this exact same thing with Laurent
> > > > > > at FOSDEM and we also came to the conclusion that the IVSC
> > > > > > chip should be modeled as a V4L2 sub-device.
> > > > >
> > > > > Agreed.
> > > >
> > > > Thanks for your quick review and conclusion, I'm fresh to media
> > > > sub-system, is there any convention that I should follow to
> > > > upstream this kind of v4l2 sub-device driver so that not too much
> > > > back and forth?
> > >
> > > This is a tentative proposal, as I'm not very familiar with the
> > > hardware
> > > architecture:
> > >
> > > - The subdev should have two pads, a sink pad connected to the camera
> > >   sensor, and a source pad connected to the CSI-2 receiver in the IPU.
> > >
> > > - As for any new driver, the subdev driver should use the active state
> > >   managed by the v4l2-subdev core. This requires calling
> > >   v4l2_subdev_init_finalize() at probe time. See
> > >   https://git.linuxtv.org/media_tree.git/commit/?id=a2514b9a634a for an
> > >   example of a subdev driver converted to this mechanism.
> > >
> > > - As we're talking about CSI-2, the subdev driver should use the streams
> > >   API that was recently merged, and in particular support the
> > >   .get_frame_desc(), .set_routing(), .enable_streams() and
> > >   .disable_streams() operations.
> > >
> > > - I don't see a need to support V4L2 controls in the subdev driver, but
> > >   I may be missing something.
> > >
> > > - The driver should be validated with v4l2-compliance, part of
> > >   v4l-utils.
> > >
> >
> > Thanks for the detail, but I have one more question, during probe of
> > sensor(v4l2-sudev) driver, it will configure camera sensor connected
> > to IVSC via I2C, but before that it should acquire camera sensor's
> > ownership from IVSC, how v4l2 framework guarantee this?
> 
> Please wrap the lines at 74 characters or so when replying.

Thanks, I will.

> 
> Do you mean accessing the sensor via I²C also requires acquiring the sensor
> from IVSC?

Yes

> 
> --
> Regards,
> 
> Sakari Ailus