Re: [PATCH v2 21/21] staging: media: tegra-video: add tegra20 variant

[Luca Ceresoli]

Hello Dmitry,

On Wed, 21 Dec 2022 00:40:20 +0300
Dmitry Osipenko <digetx@xxxxxxxxx> wrote:

> 28.11.2022 18:23, Luca Ceresoli пишет:
> > +static int tegra20_channel_capture_frame(struct tegra_vi_channel *chan,
> > +					 struct tegra_channel_buffer *buf)
> > +{
> > +	u32 value;
> > +	int err;
> > +
> > +	chan->next_out_sp_idx++;
> > +
> > +	tegra20_channel_vi_buffer_setup(chan, buf);
> > +
> > +	tegra20_vi_write(chan, TEGRA_VI_CAMERA_CONTROL, VI_CAMERA_CONTROL_VIP_ENABLE);
> > +
> > +	/* Wait for syncpt counter to reach frame start event threshold */
> > +	err = host1x_syncpt_wait(chan->out_sp, chan->next_out_sp_idx,
> > +				 TEGRA_VI_SYNCPT_WAIT_TIMEOUT, &value);  
> 
> You're not using the "value" variable, it should be NULL.

Ah, sure, good catch.

> The "chan->out_sp" looks redundant, it duplicates the chan->mw_ack_sp.

I agree it is redundant and can be improved.

> AFAICS from the doc, T20 has two VI channels, and thus, two mw_ack_sp,
> like T210.

I'm confused by this. In the current driver, each VI channel has an
array of 2 mw_ack_sp, the second of which is only used the ganged
CSI ports. I have no docs mentioning ganged ports so I don't know
exactly how they work and whether T20 might need more than 1 syncpt per
channel or not for CSI. Definitely when using VIP only one such syncpt
per each VI (or per each VIP, as per your reply to patch 1) is needed.

Bottom line: I think I can simply remove the out_sp and in the VIP code
always use chan->mw_ack_sp[0], and document that it's what is called OUT
in VIP terms.

Does this plan seem good?

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