Re: [RFC PATCH v2 4/6] media: tegra: Add Tegra210 Video input driver

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 5669 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]