Date:   Mon, 20 Feb 2023 10:36:41 +0200
From:   Sakari Ailus <sakari.ailus@linux.intel.com>
To:     "Wu, Wentong" <wentong.wu@intel.com>
Cc:     Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
        Hans de Goede <hdegoede@redhat.com>,
        "mchehab@kernel.org" <mchehab@kernel.org>,
        "linux-media@vger.kernel.org" <linux-media@vger.kernel.org>,
        "Pandruvada, Srinivas" <srinivas.pandruvada@intel.com>,
        "pierre-louis.bossart@linux.intel.com" 
        <pierre-louis.bossart@linux.intel.com>,
        "Wang, Zhifeng" <zhifeng.wang@intel.com>,
        "Ye, Xiang" <xiang.ye@intel.com>,
        "Qiu, Tian Shu" <tian.shu.qiu@intel.com>,
        "Cao, Bingbu" <bingbu.cao@intel.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2 3/3] media: pci: intel: ivsc: Add acquire/release API
 for ivsc
Message-ID: <Y/MxGRrKsgVat3zZ@kekkonen.localdomain>
References: <20230213022347.2480307-1-wentong.wu@intel.com>
 <20230213022347.2480307-4-wentong.wu@intel.com>
 <Y+uxbQi7seGf+adP@kekkonen.localdomain>
 <0457831e-a347-8278-01fe-52c011759d90@redhat.com>
 <Y+ypwnRVwReIEjUo@pendragon.ideasonboard.com>
 <DM6PR11MB43166A8D4225C8460C29803B8DA19@DM6PR11MB4316.namprd11.prod.outlook.com>
 <Y+9iwMbW0kk6eJKe@pendragon.ideasonboard.com>
 <DM6PR11MB4316A36ACA077501B92C1D0E8DA49@DM6PR11MB4316.namprd11.prod.outlook.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <DM6PR11MB4316A36ACA077501B92C1D0E8DA49@DM6PR11MB4316.namprd11.prod.outlook.com>
Precedence: bulk

Hi Wentong,

On Mon, Feb 20, 2023 at 03:50:55AM +0000, Wu, Wentong wrote:
> 
> 
> > -----Original Message-----
> > From: Laurent Pinchart <laurent.pinchart@ideasonboard.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?

Please wrap the lines at 74 characters or so when replying.

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

-- 
Regards,

Sakari Ailus