Date:   Wed, 1 Mar 2023 02:09:03 +0200
From:   Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To:     Hans de Goede <hdegoede@redhat.com>
Cc:     "Wu, Wentong" <wentong.wu@intel.com>,
        Sakari Ailus <sakari.ailus@linux.intel.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/6Xn9Uvy7+MhqSd@pendragon.ideasonboard.com>
References: <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>
 <Y/MxGRrKsgVat3zZ@kekkonen.localdomain>
 <DM6PR11MB4316B3105394F258D8205DBC8DA49@DM6PR11MB4316.namprd11.prod.outlook.com>
 <dfa64234-ddac-d88a-3127-a18da145bdf0@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <dfa64234-ddac-d88a-3127-a18da145bdf0@redhat.com>
Precedence: bulk

On Mon, Feb 20, 2023 at 10:03:08AM +0100, Hans de Goede wrote:
> On 2/20/23 09:57, Wu, Wentong wrote:
> >> -----Original Message-----
> >> From: Sakari Ailus <sakari.ailus@linux.intel.com>
> >> 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@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.
> > 
> > Thanks, I will.
> > 
> >> Do you mean accessing the sensor via I²C also requires acquiring the sensor
> >> from IVSC?
> > 
> > Yes
> 
> Hmm, that is going to be a problem since  we really want to have
> independent driver for the 2 which are not aware of each other.
> 
> I think that maybe we can model this part of the ivsc functionality
> as an i2c-mux. But then we will need to somehow change the parent
> of the i2c device for the sensor to the output of this mux ...
> 
> I guess this means adding some code (some hack likely) to
> drivers/platform/x86/intel/int3472/discrete.c which' proe function 
> is already guaranteed to run before the sensor's i2c-client gets
> instantiated, because of the ACPI _DEP on the INT3472 ACPI device
> in the DSDT.
> 
> This is going to be a tricky problem to tackle.

From a framework point of view I would also recommend i2c-mux, but when
it comes to the ACPI integration... I can feel your pain and I'm sorry
about that :-(

-- 
Regards,

Laurent Pinchart