Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <67a5a826-5ba5-340b-cd80-2995e88d1790@gmail.com>
Date:   Fri, 23 Dec 2022 15:27:54 +0300
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.6.0
Subject: Re: [PATCH v2 21/21] staging: media: tegra-video: add tegra20 variant
Content-Language: en-US
From:   Dmitry Osipenko <digetx@gmail.com>
To:     Luca Ceresoli <luca.ceresoli@bootlin.com>
Cc:     David Airlie <airlied@gmail.com>, Daniel Vetter <daniel@ffwll.ch>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Thierry Reding <thierry.reding@gmail.com>,
        Jonathan Hunter <jonathanh@nvidia.com>,
        Sowjanya Komatineni <skomatineni@nvidia.com>,
        Mauro Carvalho Chehab <mchehab@kernel.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Hans Verkuil <hverkuil-cisco@xs4all.nl>,
        linux-media@vger.kernel.org, linux-tegra@vger.kernel.org,
        dri-devel@lists.freedesktop.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-staging@lists.linux.dev,
        Thomas Petazzoni <thomas.petazzoni@bootlin.com>,
        Paul Kocialkowski <paul.kocialkowski@bootlin.com>,
        Richard Leitner <richard.leitner@skidata.com>
References: <20221128152336.133953-1-luca.ceresoli@bootlin.com>
 <20221128152336.133953-22-luca.ceresoli@bootlin.com>
 <30e6b040-aa82-f6a3-1ff6-baa2c0dcb0e2@gmail.com>
 <20221222100341.5882c19c@booty>
 <a64e2aea-2e06-a72e-5ced-a86de4458c61@gmail.com>
In-Reply-To: <a64e2aea-2e06-a72e-5ced-a86de4458c61@gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk

23.12.2022 15:15, Dmitry Osipenko пишет:
> 22.12.2022 12:03, Luca Ceresoli пишет:
>> Hello Dmitry,
>>
>> On Wed, 21 Dec 2022 00:40:20 +0300
>> Dmitry Osipenko <digetx@gmail.com> 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?
> 
> Older Tegra VI doesn't have ganged ports, but two memory/CSI channels.
> It feels to me that Tegra VI can capture both channels independently,
> though downstream driver stack used only one of the channels, IIRC.
> 
> There is a VI header file from nvddk in downstream kernel, which is
> pretty much the doc by itself.
> 
> https://nv-tegra.nvidia.com/r/gitweb?p=linux-2.6.git;a=blob;f=arch/arm/mach-tegra/include/ap20/arvi.h;h=6ce52e8e9a7213e33466d34a71cf3af2b6944b8a;

Although, after a bit closer look, I see that there is only one port
selector there. Hence there only one port can be active at a time.