linux-kernel - Re: [PATCH v5 14/20] staging: media: tegra-video: move MIPI calibration calls from VI to CSI

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20230418100756.329305ca@booty>
Date:   Tue, 18 Apr 2023 10:07:56 +0200
From:   Luca Ceresoli <luca.ceresoli@...tlin.com>
To:     Hans Verkuil <hverkuil-cisco@...all.nl>
Cc:     linux-tegra@...r.kernel.org,
        Thierry Reding <thierry.reding@...il.com>,
        Jonathan Hunter <jonathanh@...dia.com>,
        Sowjanya Komatineni <skomatineni@...dia.com>,
        David Airlie <airlied@...il.com>,
        Daniel Vetter <daniel@...ll.ch>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-media@...r.kernel.org, dri-devel@...ts.freedesktop.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-staging@...ts.linux.dev,
        Laurent Pinchart <laurent.pinchart@...asonboard.com>,
        Thomas Petazzoni <thomas.petazzoni@...tlin.com>,
        Paul Kocialkowski <paul.kocialkowski@...tlin.com>,
        Richard Leitner <richard.leitner@...data.com>,
        Dmitry Osipenko <digetx@...il.com>
Subject: Re: [PATCH v5 14/20] staging: media: tegra-video: move MIPI
 calibration calls from VI to CSI

Hi Hans,

On Fri, 14 Apr 2023 17:51:34 +0200
Hans Verkuil <hverkuil-cisco@...all.nl> wrote:

> Hi Luca,
> 
> I just encountered an error in this patch, so I have rejected the PR I made.
> 
> See below for the details:
> 
> On 07/04/2023 15:38, Luca Ceresoli wrote:
> > The CSI module does not handle all the MIPI lane calibration procedure,
> > leaving a small part of it to the VI module. In doing this,
> > tegra_channel_enable_stream() (vi.c) manipulates the private data of the
> > upstream subdev casting it to struct 'tegra_csi_channel', which will be
> > wrong after introducing a VIP (parallel video input) channel.
> > 
> > This prevents adding support for the VIP module.  It also breaks the
> > logical isolation between modules.
> > 
> > Since the lane calibration requirement does not exist in the parallel input
> > module, moving the calibration function to a per-module op is not
> > optimal. Instead move the calibration procedure in the CSI module, together
> > with the rest of the calibration procedures. After this change,
> > tegra_channel_enable_stream() just calls v4l2_subdev_call() to ask for a
> > stream start/stop to the CSI module, which in turn knows all the
> > CSI-specific details to implement it.
> > 
> > Signed-off-by: Luca Ceresoli <luca.ceresoli@...tlin.com>
> > Reviewed-by: Dmitry Osipenko <digetx@...il.com>
> > 
> > ---
> > 
> > No changes in v5
> > 
> > Changed in v4:
> >  - Added review tags
> > 
> > No changes in v3
> > No changes in v2
> > ---
> >  drivers/staging/media/tegra-video/csi.c | 44 ++++++++++++++++++++
> >  drivers/staging/media/tegra-video/vi.c  | 54 ++-----------------------
> >  2 files changed, 48 insertions(+), 50 deletions(-)
> > 
> > diff --git a/drivers/staging/media/tegra-video/csi.c b/drivers/staging/media/tegra-video/csi.c
> > index 9a03d5ccdf3c..b93fc879ef3a 100644
> > --- a/drivers/staging/media/tegra-video/csi.c
> > +++ b/drivers/staging/media/tegra-video/csi.c
> > @@ -328,12 +328,42 @@ static int tegra_csi_enable_stream(struct v4l2_subdev *subdev)
> >  	}
> >  
> >  	csi_chan->pg_mode = chan->pg_mode;
> > +
> > +	/*
> > +	 * Tegra CSI receiver can detect the first LP to HS transition.
> > +	 * So, start the CSI stream-on prior to sensor stream-on and
> > +	 * vice-versa for stream-off.
> > +	 */
> >  	ret = csi->ops->csi_start_streaming(csi_chan);
> >  	if (ret < 0)
> >  		goto finish_calibration;
> >  
> > +	if (csi_chan->mipi) {
> > +		struct v4l2_subdev *src_subdev;
> > +		/*
> > +		 * TRM has incorrectly documented to wait for done status from
> > +		 * calibration logic after CSI interface power on.
> > +		 * As per the design, calibration results are latched and applied
> > +		 * to the pads only when the link is in LP11 state which will happen
> > +		 * during the sensor stream-on.
> > +		 * CSI subdev stream-on triggers start of MIPI pads calibration.
> > +		 * Wait for calibration to finish here after sensor subdev stream-on.
> > +		 */
> > +		src_subdev = tegra_channel_get_remote_source_subdev(chan);
> > +		ret = v4l2_subdev_call(src_subdev, video, s_stream, true);
> > +		err = tegra_mipi_finish_calibration(csi_chan->mipi);
> > +
> > +		if (ret < 0 && ret != -ENOIOCTLCMD)
> > +			goto disable_csi_stream;  
> 
> If there was an error from s_stream, then tegra_mipi_finish_calibration is called
> and it goes to disable_csi_stream...
> 
> > +
> > +		if (err < 0)
> > +			dev_warn(csi->dev, "MIPI calibration failed: %d\n", err);
> > +	}
> > +
> >  	return 0;
> >  
> > +disable_csi_stream:
> > +	csi->ops->csi_stop_streaming(csi_chan);
> >  finish_calibration:
> >  	if (csi_chan->mipi)
> >  		tegra_mipi_finish_calibration(csi_chan->mipi);  
> 
> ...but here tegra_mipi_finish_calibration() is called again, leading to an unlock
> imbalance.

Many thanks for your testing! Unfortunately I have no Tegra210 hardware
so this never happened here, but with your report the problem got
obvious and, luckily, the fix appeared to be just a oneliner.

v6 just sent! I'm wondering whether there is still hope to get this
6.4...

Best regards,
Luca

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com