Re: [PATCH] media: imx6-mipi-csi2: Call remote subdev get_mbus_config to get active lanes

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Ezequiel,

On Mon, Jan 11, 2021 at 12:11:36PM -0300, Ezequiel Garcia wrote:
> On Fri, 2021-01-08 at 11:10 -0800, Steve Longerbeam wrote:
> > On 12/29/20 2:31 AM, Ezequiel Garcia wrote:
> > > Currently, the CSI2 subdevice is using the data-lanes from the
> > > neareast endpoint to config the CSI2 lanes.
> > > 
> > > While this may work, the proper way to configure the hardware is
> > > to obtain the remote subdevice in v4l2_async_notifier_operations.bound(),
> > > and then call get_mbus_config using the remote subdevice to get
> > > the active lanes.
> > > 
> > > Signed-off-by: Ezequiel Garcia <ezequiel@xxxxxxxxxxxxx>
> > > ---
> > >   drivers/staging/media/imx/TODO             |  12 ---
> > >   drivers/staging/media/imx/imx6-mipi-csi2.c | 101 ++++++++++++++++++---
> > >   2 files changed, 90 insertions(+), 23 deletions(-)
> > > 
> > > diff --git a/drivers/staging/media/imx/TODO b/drivers/staging/media/imx/TODO
> > > index 9cfc1c1e78dc..c575f419204a 100644
> > > --- a/drivers/staging/media/imx/TODO
> > > +++ b/drivers/staging/media/imx/TODO
> > > @@ -2,18 +2,6 @@
> > >   - The Frame Interval Monitor could be exported to v4l2-core for
> > >     general use.
> > >   
> > > -- The CSI subdevice parses its nearest upstream neighbor's device-tree
> > > -  bus config in order to setup the CSI. Laurent Pinchart argues that
> > > -  instead the CSI subdev should call its neighbor's g_mbus_config op
> > > -  (which should be propagated if necessary) to get this info. However
> > > -  Hans Verkuil is planning to remove the g_mbus_config op. For now this
> > > -  driver uses the parsed DT bus config method until this issue is
> > > -  resolved.
> > 
> > This TODO was actually referring to the fwnode endpoint parsing in 
> 
> Ah, OK.
> 
> > But the same conversion to call .get_mbus_config() instead of endpoint 
> > parsing could be done in imx-media-csi.c, but there is one imx6 
> > constraint that is preventing this from happening. The imx6 reference 
> > manual states that if the CSI is receiving from an input parallel bus 
> > that is 16-bits wide, the data must go directly to memory via the SMFC 
> > and not be sent to the IPU's Image Converter ("passthrough" mode):
> > 
> > "37.4.3.9 16 bit camera support
> > 
> > Devices that support 16 bit data bus can be connected to the CSI. This  can be done in one
> > of the following ways.
> > 
> > 16 bit YUV422
> > In this mode the CSI receives 2 components per cycle. The CSI is programmed to
> > accept the data as 16 bit generic data. The captured data will be stored in the memory
> > through the SMFC. The IDMAC needs to be programmed to store 16bit generic data.
> > When the data is read back from the memory for further processing in the IPU it will
> > be read as YUV422 data."
> > 
> > Same is said for RGB data to the CSI.
> > 
> > I'm not sure if this restriction is real or not. If this restriction 
> > were ignored, the fwnode endpoint check "ep->bus.parallel.bus_width >= 
> > 16" could be removed and the only remaining info required to determine 
> > passthrough mode is available from 'struct v4l2_mbus_config' and the 
> > input mbus codes, thus allowing the conversion to .get_mbus_config().
> 
> For the sound of this, the above doesn't affect this patch, right?
> Also, note there's a v2 submitted:
> 
> https://patchwork.linuxtv.org/project/linux-media/patch/20210103154155.318300-1-ezequiel@xxxxxxxxxxxxx/
> 
> Now, there's something I'm not exactly sure about these .get_mbus_config
> conversions, being described in the TODO file.
> 
> The TODO file should only list what's missing to move the driver
> out of staging. Converting to newer APIs doesn't seem a blocker:
> there are a ton of drivers using old APIs out there, which is
> a natural consequence of how the kernel evolve APIs all the time.
> 
> I'm wondering if the other TODO items apply as well, moving
> the Frame Interval Monitor to the v4l2-core is something we
> can always do at any later point. It shouldn't be a requirement
> for destaging.
> 
> There's one thing that we must resolve before de-staging.
> The media controller topology, which is a form of ABI should
> be settled, as that's difficult to change later.
> 
> However, this item is not mentioned in the TODO.
> 
> So, I was thinking we should remove all the current TODO
> items and add something about the media controller topology
> stability requirements.
> 
> What do you think?

If we decide to do so, could you keep the TODO items somewhere ? It's
useful to have a list, they could be moved to the driver source code for
instance.

-- 
Regards,

Laurent Pinchart



[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux