Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Fri, 14 Feb 2020 17:46:42 +0100
From:   Thierry Reding <thierry.reding@gmail.com>
To:     Sowjanya Komatineni <skomatineni@nvidia.com>
Cc:     jonathanh@nvidia.com, frankc@nvidia.com, hverkuil@xs4all.nl,
        helen.koike@collabora.com, sboyd@kernel.org,
        linux-media@vger.kernel.org, devicetree@vger.kernel.org,
        linux-clk@vger.kernel.org, linux-tegra@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [RFC PATCH v2 4/6] media: tegra: Add Tegra210 Video input driver
Message-ID: <20200214164642.GA1310813@ulmo>
References: <1580937806-17376-1-git-send-email-skomatineni@nvidia.com>
 <1580937806-17376-5-git-send-email-skomatineni@nvidia.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha256;
        protocol="application/pgp-signature"; boundary="lrZ03NoBR/3+SXJZ"
Content-Disposition: inline
In-Reply-To: <1580937806-17376-5-git-send-email-skomatineni@nvidia.com>
User-Agent: Mutt/1.13.1 (2019-12-14)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


--lrZ03NoBR/3+SXJZ
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Wed, Feb 05, 2020 at 01:23:24PM -0800, Sowjanya Komatineni wrote:
[...]
> +static int tegra_channel_capture_frame(struct tegra_vi_channel *chan,
> +				       struct tegra_channel_buffer *buf)
> +{
> +	int err = 0;
> +	u32 thresh, value, frame_start;
> +	int bytes_per_line = chan->format.bytesperline;
> +
> +	/* program buffer address by using surface 0 */
> +	vi_csi_write(chan, TEGRA_VI_CSI_SURFACE0_OFFSET_MSB, 0x0);
> +	vi_csi_write(chan, TEGRA_VI_CSI_SURFACE0_OFFSET_LSB, buf->addr);
> +	vi_csi_write(chan, TEGRA_VI_CSI_SURFACE0_STRIDE, bytes_per_line);
> +
> +	/* increase syncpoint max */
> +	thresh = host1x_syncpt_incr_max(chan->sp, 1);
> +
> +	/* program syncpoint */
> +	frame_start = VI_CSI_PP_FRAME_START(chan->portno);
> +	value = VI_CFG_VI_INCR_SYNCPT_COND(frame_start) |
> +		host1x_syncpt_id(chan->sp);
> +	tegra_vi_write(chan, TEGRA_VI_CFG_VI_INCR_SYNCPT, value);

Okay, so this programs the VI to increment the given syncpoint upon
frame start? What is that VI_CSI_PP_FRAME_START(chan->portno) exactly?

> +
> +	vi_csi_write(chan, TEGRA_VI_CSI_SINGLE_SHOT, SINGLE_SHOT_CAPTURE);

And now we start capturing in single-shot mode.

> +
> +	/* move buffer to capture done queue */
> +	spin_lock(&chan->done_lock);
> +	list_add_tail(&buf->queue, &chan->done);
> +	spin_unlock(&chan->done_lock);
> +
> +	/* wait up kthread for capture done */
> +	wake_up_interruptible(&chan->done_wait);

But this I don't understand. You wake up the kthread...

> +
> +	/* use syncpoint to wake up */
> +	err = host1x_syncpt_wait(chan->sp, thresh,
> +				 TEGRA_VI_SYNCPT_WAIT_TIMEOUT, &value);

... and then wait for the syncpoint to reach the given threshold? Isn't
that the wrong way around? Don't we need to wait for the syncpoint
increment *before* we wake up the kthread that will return the buffer
to userspace?

> +	if (err) {
> +		dev_err(&chan->video.dev,
> +			"frame start syncpt timeout: %d\n", err);
> +		tegra_channel_capture_error_status(chan);
> +	}
> +
> +	return err;
> +}
> +
> +static int tegra_channel_capture_done(struct tegra_vi_channel *chan,
> +				      struct tegra_channel_buffer *buf)
> +{
> +	struct vb2_v4l2_buffer *vb = &buf->buf;
> +	u32 thresh, value, mw_ack_done;
> +	int ret = 0;
> +
> +	/* increase syncpoint max */
> +	thresh = host1x_syncpt_incr_max(chan->sp, 1);
> +
> +	/* program syncpoint */
> +	mw_ack_done = VI_CSI_MW_ACK_DONE(chan->portno);
> +	value = VI_CFG_VI_INCR_SYNCPT_COND(mw_ack_done) |
> +		host1x_syncpt_id(chan->sp);
> +	tegra_vi_write(chan, TEGRA_VI_CFG_VI_INCR_SYNCPT, value);
> +
> +	if (!vi_csi_read(chan, TEGRA_VI_CSI_SINGLE_SHOT))
> +		vi_csi_write(chan, TEGRA_VI_CSI_SINGLE_SHOT,
> +			     SINGLE_SHOT_CAPTURE);
> +
> +	/* use syncpoint to wake up */
> +	ret = host1x_syncpt_wait(chan->sp, thresh,
> +				 TEGRA_VI_SYNCPT_WAIT_TIMEOUT, &value);
> +	if (ret)
> +		dev_err(&chan->video.dev,
> +			"MW_ACK_DONE syncpoint timeout: %d\n", ret);

Actually... there's another syncpoint wait here, so I guess this will
stall until VI has actually completed writing the captured frame to
memory.

> +
> +	/* captured one frame */
> +	vb->sequence = chan->sequence++;
> +	vb->field = V4L2_FIELD_NONE;
> +	vb->vb2_buf.timestamp = ktime_get_ns();
> +	vb2_buffer_done(&vb->vb2_buf,
> +			ret < 0 ? VB2_BUF_STATE_ERROR : VB2_BUF_STATE_DONE);

So it's really only at this point that we return the buffer to
userspace, which should be after the hardware is done writing to the
buffer, so this should be fine.

That said, I'm wondering if host1x_syncpt_wait() is a good interface
for this use-case. We don't really have anything else right now, but
I think we may be able to add something to have a function called in
case the syncpoint reaches a threshold. Having to spawn two separate
threads with wait queues seems a bit overkill for this.

It's fine to leave this as it is for now, but maybe something to
consider as improvement in the future.

> +	return ret;
> +}
> +
> +static int chan_capture_kthread_start(void *data)
> +{
> +	struct tegra_vi_channel *chan = data;
> +	struct tegra_channel_buffer *buf;
> +	int err = 0;
> +
> +	set_freezable();
> +
> +	while (1) {
> +		try_to_freeze();
> +
> +		wait_event_interruptible(chan->start_wait,
> +					 !list_empty(&chan->capture) ||
> +					 kthread_should_stop());
> +		if (kthread_should_stop())
> +			break;
> +
> +		if (err)
> +			continue;
> +
> +		spin_lock(&chan->start_lock);
> +		if (list_empty(&chan->capture)) {
> +			spin_unlock(&chan->start_lock);
> +			continue;
> +		}
> +
> +		buf = list_entry(chan->capture.next,
> +				 struct tegra_channel_buffer, queue);
> +		list_del_init(&buf->queue);
> +		spin_unlock(&chan->start_lock);
> +		err = tegra_channel_capture_frame(chan, buf);
> +	}
> +
> +	return 0;
> +}
> +
> +static int chan_capture_kthread_done(void *data)
> +{
> +	struct tegra_vi_channel *chan = data;
> +	struct tegra_channel_buffer *buf;
> +	int err = 0;
> +
> +	set_freezable();
> +
> +	while (1) {
> +		try_to_freeze();
> +
> +		wait_event_interruptible(chan->done_wait,
> +					 !list_empty(&chan->done) ||
> +					 kthread_should_stop());
> +
> +		if (kthread_should_stop())
> +			break;
> +
> +		spin_lock(&chan->done_lock);
> +		if (list_empty(&chan->done)) {
> +			spin_unlock(&chan->done_lock);
> +			continue;
> +		}
> +
> +		buf = list_entry(chan->done.next, struct tegra_channel_buffer,
> +				 queue);
> +		if (!buf)
> +			continue;
> +
> +		list_del_init(&buf->queue);
> +		spin_unlock(&chan->done_lock);
> +		err = tegra_channel_capture_done(chan, buf);

What's with the error here? I think we should either handle it in some
way, or just avoid even returning an error if we're not going to deal
with it anyway.

Thierry

--lrZ03NoBR/3+SXJZ
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCAAdFiEEiOrDCAFJzPfAjcif3SOs138+s6EFAl5Gzu4ACgkQ3SOs138+
s6Hd1w/+INwB+uEPRluAC0zs+uoEeVtXd/JX1Fz5YqyKmaJ9Lp/WNj9RHfluVU2K
r7mIGLq2ia8bLekfTdh3+YX6uaHfPIDhlHNsE3yiCseiIH1mIYdNXgUng+8v9KbJ
T8/84ICFZSc2fb4rhYhivXJ61JqGkRK5dh8dl1HVqgvdYmjofivHQCZcUXlUvq1x
q7iBVXE3U762/FYGfA7v09BHejAnT5G2TUcnOkQknFk2XZi+uEgIVgxgz9MMz6Bw
1bf2Za2wbfm4LFO7e2kjahZy/tHdrNOE7Nyz62fbYEybev5G6IDT16kGpsnGpsf2
kJ8ohkuyYxsROI1uxoO5C1Uvsi52worYljuDlXlXuYwtfVFmky4Yyk8diO9b8TmP
6ImPP6v9XO7cyyJG1A97Gk0sm4gCJbbKsNvahA9niPwPqlJsww+qvczhxBVvsIDb
iqc6zFRPOYpPV3oyvjX0sQi27iBF5MsoxN9TpK7Ow7qhnoBmf7jJczVwdeHTCmHE
HWIuaVrTiy9mY02T0GXbxHD9daOgxPFcfVbudsyX1NOxRjtreN16Bfc1bDX0Awlg
VoimBA8iVkP7u3Igpyr3q1FhVrCfdeN9yZBrwI44aiEOa7i6D1sAf1bgC2VZ+L1U
mhfXc3eLZZq3NfKdS2SpQ99Mq46lTV//ORM0LuiRY6k9mQuSqog=
=StqB
-----END PGP SIGNATURE-----

--lrZ03NoBR/3+SXJZ--