Re: [PATCH v2 21/21] staging: media: tegra-video: add tegra20 variant
On Fri, 23 Dec 2022 15:35:58 +0300
Dmitry Osipenko wrote:
> 28.11.2022 18:23, Luca Ceresoli пишет:
...
> > +static const struct tegra_vip_ops tegra20_vip_ops = {
> > + .vip_start_streaming = tegra20_vip_start_streaming,
> > +};
> > +
> > +const struct tegra_vip_soc tegra20_vip_soc = {
> > + .ops = _vip_ops,
> > +};
>
> Shouldn't this be placed in vip.c?
Indeed. Which means tegra210_csi_soc can be moved as well, so I'm adding
a small patch to the series to do that.
> Also looks like patch #20 won't link
> because tegra20_vip_soc is defined in patch #21.
You're right, we have a chicken-egg problem here. One solution would be
leaving tegra_vip_of_id_table empty in patch 20 and fill it only in
patch 21, but that would not be bisectable as patch 20 would introduce
code that nobody uses until patch 21. So I think it's better to squash
together patches 20+21.
--
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Re: [PATCH v2 21/21] staging: media: tegra-video: add tegra20 variant
23.12.2022 16:02, Dmitry Osipenko пишет:
> 28.11.2022 18:23, Luca Ceresoli пишет:
>> +static int tegra20_vip_start_streaming(struct tegra_vip_channel *vip_chan)
>> +{
>> +struct tegra_vi_channel *vi_chan =
>> v4l2_get_subdev_hostdata(_chan->subdev);
>> +int width = vi_chan->format.width;
>> +int height = vi_chan->format.height;
>> +
>> +unsigned int main_input_format;
>> +unsigned int yuv_input_format;
>> +
>> +tegra20_vi_get_input_formats(vi_chan, _input_format,
>> _input_format);
>> +
>> +tegra20_vi_write(vi_chan, TEGRA_VI_VI_CORE_CONTROL, 0);
>> +
>> +tegra20_vi_write(vi_chan, TEGRA_VI_VI_INPUT_CONTROL,
>> + VI_INPUT_VIP_INPUT_ENABLE | main_input_format |
>> yuv_input_format);
>> +
>> +tegra20_vi_write(vi_chan, TEGRA_VI_V_DOWNSCALE_CONTROL, 0);
>> +tegra20_vi_write(vi_chan, TEGRA_VI_H_DOWNSCALE_CONTROL, 0);
>> +
>> +tegra20_vi_write(vi_chan, TEGRA_VI_VIP_V_ACTIVE, height <<
>> VI_VIP_V_ACTIVE_PERIOD_SFT);
>> +tegra20_vi_write(vi_chan, TEGRA_VI_VIP_H_ACTIVE,
>> + roundup(width, 2) << VI_VIP_H_ACTIVE_PERIOD_SFT);
>> +
>> +/*
>> + * For VIP, D9..D2 is mapped to the video decoder's P7..P0.
>> + * Disable/mask out the other Dn wires. When not in BT656
>> + * mode we also need the V/H sync.
>> + */
>> +tegra20_vi_write(vi_chan, TEGRA_VI_PIN_INPUT_ENABLE,
>> + GENMASK(9, 2) << VI_PIN_INPUT_VD_SFT |
>> + VI_PIN_INPUT_HSYNC | VI_PIN_INPUT_VSYNC);
>> +tegra20_vi_write(vi_chan, TEGRA_VI_VI_DATA_INPUT_CONTROL,
>> + GENMASK(9, 2) << VI_DATA_INPUT_SFT);
>> +tegra20_vi_write(vi_chan, TEGRA_VI_PIN_INVERSION, 0);
>> +
>> +tegra20_vi_write(vi_chan, TEGRA_VI_CONT_SYNCPT_OUT_1,
>> + VI_CONT_SYNCPT_OUT_1_CONTINUOUS_SYNCPT |
>> + host1x_syncpt_id(vi_chan->out_sp) <<
>> VI_CONT_SYNCPT_OUT_1_SYNCPT_IDX_SFT);
>
> Wondering whether you also need to set up the sypct_incr condition to
> op_done, it should be immediate by default. I see that T210 VI code sets
> up the incr condition.
Found in the doc "Continuous syncpt always use OP_DONE as condition",
the current code is fine.
Re: [PATCH v2 21/21] staging: media: tegra-video: add tegra20 variant
28.11.2022 18:23, Luca Ceresoli пишет:
> +static int tegra20_vip_start_streaming(struct tegra_vip_channel *vip_chan)
> +{
> + struct tegra_vi_channel *vi_chan =
> v4l2_get_subdev_hostdata(_chan->subdev);
> + int width = vi_chan->format.width;
> + int height = vi_chan->format.height;
> +
> + unsigned int main_input_format;
> + unsigned int yuv_input_format;
> +
> + tegra20_vi_get_input_formats(vi_chan, _input_format,
> _input_format);
> +
> + tegra20_vi_write(vi_chan, TEGRA_VI_VI_CORE_CONTROL, 0);
> +
> + tegra20_vi_write(vi_chan, TEGRA_VI_VI_INPUT_CONTROL,
> + VI_INPUT_VIP_INPUT_ENABLE | main_input_format |
> yuv_input_format);
> +
> + tegra20_vi_write(vi_chan, TEGRA_VI_V_DOWNSCALE_CONTROL, 0);
> + tegra20_vi_write(vi_chan, TEGRA_VI_H_DOWNSCALE_CONTROL, 0);
> +
> + tegra20_vi_write(vi_chan, TEGRA_VI_VIP_V_ACTIVE, height <<
> VI_VIP_V_ACTIVE_PERIOD_SFT);
> + tegra20_vi_write(vi_chan, TEGRA_VI_VIP_H_ACTIVE,
> + roundup(width, 2) << VI_VIP_H_ACTIVE_PERIOD_SFT);
> +
> + /*
> + * For VIP, D9..D2 is mapped to the video decoder's P7..P0.
> + * Disable/mask out the other Dn wires. When not in BT656
> + * mode we also need the V/H sync.
> + */
> + tegra20_vi_write(vi_chan, TEGRA_VI_PIN_INPUT_ENABLE,
> + GENMASK(9, 2) << VI_PIN_INPUT_VD_SFT |
> + VI_PIN_INPUT_HSYNC | VI_PIN_INPUT_VSYNC);
> + tegra20_vi_write(vi_chan, TEGRA_VI_VI_DATA_INPUT_CONTROL,
> + GENMASK(9, 2) << VI_DATA_INPUT_SFT);
> + tegra20_vi_write(vi_chan, TEGRA_VI_PIN_INVERSION, 0);
> +
> + tegra20_vi_write(vi_chan, TEGRA_VI_CONT_SYNCPT_OUT_1,
> + VI_CONT_SYNCPT_OUT_1_CONTINUOUS_SYNCPT |
> + host1x_syncpt_id(vi_chan->out_sp) <<
> VI_CONT_SYNCPT_OUT_1_SYNCPT_IDX_SFT);
Wondering whether you also need to set up the sypct_incr condition to
op_done, it should be immediate by default. I see that T210 VI code sets
up the incr condition.
Re: [PATCH v2 21/21] staging: media: tegra-video: add tegra20 variant
28.11.2022 18:23, Luca Ceresoli пишет:
> +/* --
> + * VIP
> + */
> +
> +/*
> + * VIP-specific configuration for stream start.
> + *
> + * Whatever is common among VIP and CSI is done by the VI component (see
> + * tegra20_vi_start_streaming()). Here we do what is VIP-specific.
> + */
> +static int tegra20_vip_start_streaming(struct tegra_vip_channel *vip_chan)
> +{
> + struct tegra_vi_channel *vi_chan =
> v4l2_get_subdev_hostdata(_chan->subdev);
> + int width = vi_chan->format.width;
> + int height = vi_chan->format.height;
> +
> + unsigned int main_input_format;
> + unsigned int yuv_input_format;
> +
> + tegra20_vi_get_input_formats(vi_chan, _input_format,
> _input_format);
> +
> + tegra20_vi_write(vi_chan, TEGRA_VI_VI_CORE_CONTROL, 0);
> +
> + tegra20_vi_write(vi_chan, TEGRA_VI_VI_INPUT_CONTROL,
> + VI_INPUT_VIP_INPUT_ENABLE | main_input_format |
> yuv_input_format);
> +
> + tegra20_vi_write(vi_chan, TEGRA_VI_V_DOWNSCALE_CONTROL, 0);
> + tegra20_vi_write(vi_chan, TEGRA_VI_H_DOWNSCALE_CONTROL, 0);
> +
> + tegra20_vi_write(vi_chan, TEGRA_VI_VIP_V_ACTIVE, height <<
> VI_VIP_V_ACTIVE_PERIOD_SFT);
> + tegra20_vi_write(vi_chan, TEGRA_VI_VIP_H_ACTIVE,
> + roundup(width, 2) << VI_VIP_H_ACTIVE_PERIOD_SFT);
> +
> + /*
> + * For VIP, D9..D2 is mapped to the video decoder's P7..P0.
> + * Disable/mask out the other Dn wires. When not in BT656
> + * mode we also need the V/H sync.
> + */
> + tegra20_vi_write(vi_chan, TEGRA_VI_PIN_INPUT_ENABLE,
> + GENMASK(9, 2) << VI_PIN_INPUT_VD_SFT |
> + VI_PIN_INPUT_HSYNC | VI_PIN_INPUT_VSYNC);
> + tegra20_vi_write(vi_chan, TEGRA_VI_VI_DATA_INPUT_CONTROL,
> + GENMASK(9, 2) << VI_DATA_INPUT_SFT);
> + tegra20_vi_write(vi_chan, TEGRA_VI_PIN_INVERSION, 0);
> +
> + tegra20_vi_write(vi_chan, TEGRA_VI_CONT_SYNCPT_OUT_1,
> + VI_CONT_SYNCPT_OUT_1_CONTINUOUS_SYNCPT |
> + host1x_syncpt_id(vi_chan->out_sp) <<
> VI_CONT_SYNCPT_OUT_1_SYNCPT_IDX_SFT);
> +
> + tegra20_vi_write(vi_chan, TEGRA_VI_CAMERA_CONTROL,
> VI_CAMERA_CONTROL_STOP_CAPTURE);
> +
> + return 0;
> +}
> +
> +static const struct tegra_vip_ops tegra20_vip_ops = {
> + .vip_start_streaming = tegra20_vip_start_streaming,
> +};
> +
> +const struct tegra_vip_soc tegra20_vip_soc = {
> + .ops = _vip_ops,
> +};
Shouldn't this be placed in vip.c? Also looks like patch #20 won't link
because tegra20_vip_soc is defined in patch #21.
Re: [PATCH v2 21/21] staging: media: tegra-video: add tegra20 variant
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 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, );
>>>
>>> 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.
Re: [PATCH v2 21/21] staging: media: tegra-video: add tegra20 variant
22.12.2022 12:03, Luca Ceresoli пишет:
> Hello Dmitry,
>
> On Wed, 21 Dec 2022 00:40:20 +0300
> Dmitry Osipenko 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, );
>>
>> 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;
Re: [PATCH v2 21/21] staging: media: tegra-video: add tegra20 variant
Hello Dmitry,
On Wed, 21 Dec 2022 00:40:20 +0300
Dmitry Osipenko 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, );
>
> 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
Re: [PATCH v2 21/21] staging: media: tegra-video: add tegra20 variant
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, );
You're not using the "value" variable, it should be NULL.
The "chan->out_sp" looks redundant, it duplicates the chan->mw_ack_sp.
AFAICS from the doc, T20 has two VI channels, and thus, two mw_ack_sp,
like T210.
