Message-ID: <DM6PR11MB4316A36ACA077501B92C1D0E8DA49@DM6PR11MB4316.namprd11.prod.outlook.com>
Date:   Mon, 20 Feb 2023 03:50:55 +0000
From:   "Wu, Wentong" <wentong.wu@...el.com>
To:     Laurent Pinchart <laurent.pinchart@...asonboard.com>
CC:     Hans de Goede <hdegoede@...hat.com>,
        Sakari Ailus <sakari.ailus@...ux.intel.com>,
        "mchehab@...nel.org" <mchehab@...nel.org>,
        "linux-media@...r.kernel.org" <linux-media@...r.kernel.org>,
        "Pandruvada, Srinivas" <srinivas.pandruvada@...el.com>,
        "pierre-louis.bossart@...ux.intel.com" 
        <pierre-louis.bossart@...ux.intel.com>,
        "Wang, Zhifeng" <zhifeng.wang@...el.com>,
        "Ye, Xiang" <xiang.ye@...el.com>,
        "Qiu, Tian Shu" <tian.shu.qiu@...el.com>,
        "Cao, Bingbu" <bingbu.cao@...el.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH v2 3/3] media: pci: intel: ivsc: Add acquire/release API
 for ivsc



> -----Original Message-----
> From: Laurent Pinchart <laurent.pinchart@...asonboard.com>
> 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?

Thanks,
Wentong

> > > > > I suppose the intended use cases assume a single instance of
> > > > > IVSC (as well as MEI) but there can, and often are, be multiple
> > > > > camera sensors in the system. The decision whether to request
> > > > > pass-through from IVCS can't be done in the camera sensor
> > > > > driver, and should not be visible to the camera sensor driver.
> > > > > Exposing IVSC as a V4L2 sub-device makes this trivial to
> > > > > address, as the IVSC driver's V4L2 sub-device video s_stream() operation
> gets called before streaming is started.
> > > > >
> > > > > The information whether IVSC is found between the camera sensor
> > > > > and the host's CSI-2 receiver (IPU in this case) should come
> > > > > from system firmware and accessed most probably by what is
> > > > > called cio2-bridge at the moment.
> > > > >
> > > > > The privacy status can be a V4L2 control.
> > > > >
> > > > > Also cc Hans.
> > > > >
> > > > >> Signed-off-by: Wentong Wu <wentong.wu@...el.com>
> > > > >> ---
> > > > >>  drivers/media/pci/intel/ivsc/Makefile |  1 +
> > > > >>  drivers/media/pci/intel/ivsc/ivsc.c   | 84
> +++++++++++++++++++++++++++
> > > > >>  include/linux/ivsc.h                  | 55 ++++++++++++++++++
> > > > >>  3 files changed, 140 insertions(+)  create mode 100644
> > > > >> drivers/media/pci/intel/ivsc/ivsc.c
> 
> [snip]
> 
> --
> Regards,
> 
> Laurent Pinchart

Powered by blists - more mailing lists