Re: [PATCH v1 1/2] staging: Introduce NVIDIA Tegra20 video decoder driver
On 28.09.2017 10:23, Dan Carpenter wrote:
> On Thu, Sep 28, 2017 at 02:28:04AM +0300, Dmitry Osipenko wrote:
+ if (is_baseline_profile)
+ frame->aux_paddr = 0xF4DEAD00;
>>>
>>> The handling of is_baseline_profile is strange to me. It feels like we
>>> should always check it before we use ->aux_paddr but we don't ever do
>>> that.
>>>
>>
>> In a case of baseline profile, aux buffer isn't needed, HW should't use it.
>> Aux
>> phys address is set to a predefined and invalid address, so that in a case of
>> VDE trying to use it, its invalid memory accesses would be reported in KMSG
>> by
>> memory controller driver and the reported invalid addresses would be known
>> to be
>> associated with the aux buffer. I'm not sure what you are meaning.
>
> It's not used perhaps, but we do write it to the hardware, right?
>
That's right. I'm pretty sure HW won't use the aux in a case of baseline
profile, haven't seen an evidence of it. But in order to be on a safe side, the
addresses are initialized to an invalid value, so HW won't have a chance to
silently fetch/trash 'arbitrary' main memory and we will know if it tries to do
it.
if (baseline_profile)
frame->aux_paddr = 0xF4DEAD00;
else {
So here ^ all *used* frame entries are being initialized.
> tegra_vde_write_iram_entry(iram_tables, 0, i, value, aux_paddr);
>
> It's just strange.
>
There up to 16 reference video frames (already decoded) that could be used for
decoding of a video frame. Addresses of reference frames that shouldn't be used
for decoding of the current frame are set to an invalid address. Userspace my
supply wrong frames list or frames list setup code may contain an obscure bug
and we will know about it.
} else {
aux_paddr = 0xFADEAD00;
value = 0;
}
Here ^ all *unused* frame entries are being initialized.
tegra_vde_write_iram_entry(iram_tables, 0, i, value, aux_paddr);
And here ^ these entries are written to the tables that are read by HW.
--
Dmitry
Re: [PATCH v1 1/2] staging: Introduce NVIDIA Tegra20 video decoder driver
On 28.09.2017 10:23, Dan Carpenter wrote:
> On Thu, Sep 28, 2017 at 02:28:04AM +0300, Dmitry Osipenko wrote:
+ if (is_baseline_profile)
+ frame->aux_paddr = 0xF4DEAD00;
>>>
>>> The handling of is_baseline_profile is strange to me. It feels like we
>>> should always check it before we use ->aux_paddr but we don't ever do
>>> that.
>>>
>>
>> In a case of baseline profile, aux buffer isn't needed, HW should't use it.
>> Aux
>> phys address is set to a predefined and invalid address, so that in a case of
>> VDE trying to use it, its invalid memory accesses would be reported in KMSG
>> by
>> memory controller driver and the reported invalid addresses would be known
>> to be
>> associated with the aux buffer. I'm not sure what you are meaning.
>
> It's not used perhaps, but we do write it to the hardware, right?
>
That's right. I'm pretty sure HW won't use the aux in a case of baseline
profile, haven't seen an evidence of it. But in order to be on a safe side, the
addresses are initialized to an invalid value, so HW won't have a chance to
silently fetch/trash 'arbitrary' main memory and we will know if it tries to do
it.
if (baseline_profile)
frame->aux_paddr = 0xF4DEAD00;
else {
So here ^ all *used* frame entries are being initialized.
> tegra_vde_write_iram_entry(iram_tables, 0, i, value, aux_paddr);
>
> It's just strange.
>
There up to 16 reference video frames (already decoded) that could be used for
decoding of a video frame. Addresses of reference frames that shouldn't be used
for decoding of the current frame are set to an invalid address. Userspace my
supply wrong frames list or frames list setup code may contain an obscure bug
and we will know about it.
} else {
aux_paddr = 0xFADEAD00;
value = 0;
}
Here ^ all *unused* frame entries are being initialized.
tegra_vde_write_iram_entry(iram_tables, 0, i, value, aux_paddr);
And here ^ these entries are written to the tables that are read by HW.
--
Dmitry
Re: [PATCH v1 1/2] staging: Introduce NVIDIA Tegra20 video decoder driver
On Thu, Sep 28, 2017 at 02:28:04AM +0300, Dmitry Osipenko wrote: > >> + if (is_baseline_profile) > >> + frame->aux_paddr = 0xF4DEAD00; > > > > The handling of is_baseline_profile is strange to me. It feels like we > > should always check it before we use ->aux_paddr but we don't ever do > > that. > > > > In a case of baseline profile, aux buffer isn't needed, HW should't use it. > Aux > phys address is set to a predefined and invalid address, so that in a case of > VDE trying to use it, its invalid memory accesses would be reported in KMSG by > memory controller driver and the reported invalid addresses would be known to > be > associated with the aux buffer. I'm not sure what you are meaning. It's not used perhaps, but we do write it to the hardware, right? tegra_vde_write_iram_entry(iram_tables, 0, i, value, aux_paddr); It's just strange. regards, dan carpenter
Re: [PATCH v1 1/2] staging: Introduce NVIDIA Tegra20 video decoder driver
On Thu, Sep 28, 2017 at 02:28:04AM +0300, Dmitry Osipenko wrote: > >> + if (is_baseline_profile) > >> + frame->aux_paddr = 0xF4DEAD00; > > > > The handling of is_baseline_profile is strange to me. It feels like we > > should always check it before we use ->aux_paddr but we don't ever do > > that. > > > > In a case of baseline profile, aux buffer isn't needed, HW should't use it. > Aux > phys address is set to a predefined and invalid address, so that in a case of > VDE trying to use it, its invalid memory accesses would be reported in KMSG by > memory controller driver and the reported invalid addresses would be known to > be > associated with the aux buffer. I'm not sure what you are meaning. It's not used perhaps, but we do write it to the hardware, right? tegra_vde_write_iram_entry(iram_tables, 0, i, value, aux_paddr); It's just strange. regards, dan carpenter
Re: [PATCH v1 1/2] staging: Introduce NVIDIA Tegra20 video decoder driver
On 27.09.2017 12:45, Dan Carpenter wrote: >> --- /dev/null >> +++ b/drivers/staging/tegra-vde/Kconfig >> @@ -0,0 +1,6 @@ >> +config TEGRA_VDE >> +tristate "NVIDIA Tegra20 video decoder driver" >> +depends on ARCH_TEGRA_2x_SOC > > Could we get a || COMPILE_TEST here as well? > Good point, I'll address it in V2. -- Dmitry
Re: [PATCH v1 1/2] staging: Introduce NVIDIA Tegra20 video decoder driver
On 27.09.2017 12:45, Dan Carpenter wrote: >> --- /dev/null >> +++ b/drivers/staging/tegra-vde/Kconfig >> @@ -0,0 +1,6 @@ >> +config TEGRA_VDE >> +tristate "NVIDIA Tegra20 video decoder driver" >> +depends on ARCH_TEGRA_2x_SOC > > Could we get a || COMPILE_TEST here as well? > Good point, I'll address it in V2. -- Dmitry
Re: [PATCH v1 1/2] staging: Introduce NVIDIA Tegra20 video decoder driver
On 27.09.2017 12:45, Dan Carpenter wrote:
> I feel like this code is good enough to go into the regular kernel
> instead of staging, but I don't really know what "- properly handle
> decoding faults" means in this context.
As Greg pointed out, this patch should be cc'ed to media maintainers for a
review. I'll cc them on V2, will see what they would suggest. Yeah, probably the
decoding faults isn't a very candidate for a TODO for staging.
Does the driver Oops all the
> time or what?
>
Driver works fine.
> Anyway, minor comments inline.
>
Thank you very much for the awesome review. I agree with the most of the
comments.
> On Tue, Sep 26, 2017 at 01:15:42AM +0300, Dmitry Osipenko wrote:
>> diff --git a/drivers/staging/tegra-vde/Kconfig
>> b/drivers/staging/tegra-vde/Kconfig
>> new file mode 100644
>> index ..b947c012a373
>> --- /dev/null
>> +++ b/drivers/staging/tegra-vde/Kconfig
>> @@ -0,0 +1,6 @@
>> +config TEGRA_VDE
>> +tristate "NVIDIA Tegra20 video decoder driver"
>> +depends on ARCH_TEGRA_2x_SOC
>
> Could we get a || COMPILE_TEST here as well?
>
>> +help
>> +Say Y here to enable support for a NVIDIA Tegra20 video decoder
>> +driver.
>> diff --git a/drivers/staging/tegra-vde/Makefile
>> b/drivers/staging/tegra-vde/Makefile
>> new file mode 100644
>> index ..e7c0df1174bf
>> --- /dev/null
>> +++ b/drivers/staging/tegra-vde/Makefile
>> @@ -0,0 +1 @@
>> +obj-$(CONFIG_TEGRA_VDE) += vde.o
>> diff --git a/drivers/staging/tegra-vde/TODO b/drivers/staging/tegra-vde/TODO
>> new file mode 100644
>> index ..533ddfc5190e
>> --- /dev/null
>> +++ b/drivers/staging/tegra-vde/TODO
>> @@ -0,0 +1,8 @@
>> +All TODO's require reverse-engineering to be done first, it is very
>> +unlikely that NVIDIA would ever release HW specs to public.
>> +
>> +TODO:
>> +- properly handle decoding faults
>> +- support more formats
>
> Adding more formats is not a reason to put this into staging instead of
> the normal drivers/ dir.
>
Well, it feels that the driver isn't very acceptable for the core drivers/. But
maybe it's a wrong feeling. Again, will see what media/ maintainers would
suggest.
>> +static int tegra_vde_setup_context(struct tegra_vde *vde,
>> + struct tegra_vde_h264_decoder_ctx *ctx,
>> + struct video_frame *dpb_frames,
>> + phys_addr_t bitstream_data_paddr,
>> + int bitstream_data_size,
>> + int macroblocks_nb)
>> +{
>> +struct device *dev = vde->miscdev.parent;
>> +u32 value;
>> +
>> +tegra_vde_set_bits(vde->regs,0xA, SXE(0xF0));
>> +tegra_vde_set_bits(vde->regs,0xB, BSEV(CMDQUE_CONTROL));
>> +tegra_vde_set_bits(vde->regs, 0x8002, MBE(0x50));
>> +tegra_vde_set_bits(vde->regs,0xA, MBE(0xA0));
>> +tegra_vde_set_bits(vde->regs,0xA, PPE(0x14));
>> +tegra_vde_set_bits(vde->regs,0xA, PPE(0x28));
>> +tegra_vde_set_bits(vde->regs, 0xA00, MCE(0x08));
>> +tegra_vde_set_bits(vde->regs,0xA, TFE(0x00));
>> +tegra_vde_set_bits(vde->regs,0x5, VDMA(0x04));
>> +
>> +VDE_WR(0x, vde->regs + VDMA(0x1C));
>> +VDE_WR(0x, vde->regs + VDMA(0x00));
>> +VDE_WR(0x0007, vde->regs + VDMA(0x04));
>> +VDE_WR(0x0007, vde->regs + FRAMEID(0x200));
>> +VDE_WR(0x0005, vde->regs + TFE(0x04));
>> +VDE_WR(0x, vde->regs + MBE(0x84));
>> +VDE_WR(0x0010, vde->regs + SXE(0x08));
>> +VDE_WR(0x0150, vde->regs + SXE(0x54));
>> +VDE_WR(0x054C, vde->regs + SXE(0x58));
>> +VDE_WR(0x0E34, vde->regs + SXE(0x5C));
>> +VDE_WR(0x063C063C, vde->regs + MCE(0x10));
>> +VDE_WR(0x0003FC00, vde->regs + BSEV(INTR_STATUS));
>> +VDE_WR(0x150D, vde->regs + BSEV(BSE_CONFIG));
>> +VDE_WR(0x0100, vde->regs + BSEV(BSE_INT_ENB));
>> +VDE_WR(0x, vde->regs + BSEV(0x98));
>> +VDE_WR(0x0060, vde->regs + BSEV(0x9C));
>> +
>> +memset_io(vde->iram + 512, 0, macroblocks_nb / 2);
>> +
>> +tegra_setup_frameidx(vde->regs, dpb_frames, ctx->dpb_frames_nb,
>> + ctx->pic_width_in_mbs, ctx->pic_height_in_mbs);
>> +
>> +tegra_vde_setup_iram_tables(vde->iram, dpb_frames,
>> +ctx->dpb_frames_nb - 1,
>> +ctx->dpb_ref_frames_with_earlier_poc_nb);
>> +
>> +VDE_WR(0x, vde->regs + BSEV(0x8C));
>> +VDE_WR(bitstream_data_paddr + bitstream_data_size,
>> + vde->regs + BSEV(0x54));
>> +
>> +value = ctx->pic_width_in_mbs << 11 | ctx->pic_height_in_mbs << 3;
>> +
>> +VDE_WR(value, vde->regs + BSEV(0x88));
>> +
>> +if (tegra_vde_wait_bsev(vde, false))
>> +return -EIO;
>> +
>> +if (tegra_vde_push_bsev_icmdqueue(vde, 0x83FC, false))
>
> Preserve the error code from tegra_vde_push_bsev_icmdqueue(). It's
> still -EIO so
Re: [PATCH v1 1/2] staging: Introduce NVIDIA Tegra20 video decoder driver
On 27.09.2017 12:45, Dan Carpenter wrote:
> I feel like this code is good enough to go into the regular kernel
> instead of staging, but I don't really know what "- properly handle
> decoding faults" means in this context.
As Greg pointed out, this patch should be cc'ed to media maintainers for a
review. I'll cc them on V2, will see what they would suggest. Yeah, probably the
decoding faults isn't a very candidate for a TODO for staging.
Does the driver Oops all the
> time or what?
>
Driver works fine.
> Anyway, minor comments inline.
>
Thank you very much for the awesome review. I agree with the most of the
comments.
> On Tue, Sep 26, 2017 at 01:15:42AM +0300, Dmitry Osipenko wrote:
>> diff --git a/drivers/staging/tegra-vde/Kconfig
>> b/drivers/staging/tegra-vde/Kconfig
>> new file mode 100644
>> index ..b947c012a373
>> --- /dev/null
>> +++ b/drivers/staging/tegra-vde/Kconfig
>> @@ -0,0 +1,6 @@
>> +config TEGRA_VDE
>> +tristate "NVIDIA Tegra20 video decoder driver"
>> +depends on ARCH_TEGRA_2x_SOC
>
> Could we get a || COMPILE_TEST here as well?
>
>> +help
>> +Say Y here to enable support for a NVIDIA Tegra20 video decoder
>> +driver.
>> diff --git a/drivers/staging/tegra-vde/Makefile
>> b/drivers/staging/tegra-vde/Makefile
>> new file mode 100644
>> index ..e7c0df1174bf
>> --- /dev/null
>> +++ b/drivers/staging/tegra-vde/Makefile
>> @@ -0,0 +1 @@
>> +obj-$(CONFIG_TEGRA_VDE) += vde.o
>> diff --git a/drivers/staging/tegra-vde/TODO b/drivers/staging/tegra-vde/TODO
>> new file mode 100644
>> index ..533ddfc5190e
>> --- /dev/null
>> +++ b/drivers/staging/tegra-vde/TODO
>> @@ -0,0 +1,8 @@
>> +All TODO's require reverse-engineering to be done first, it is very
>> +unlikely that NVIDIA would ever release HW specs to public.
>> +
>> +TODO:
>> +- properly handle decoding faults
>> +- support more formats
>
> Adding more formats is not a reason to put this into staging instead of
> the normal drivers/ dir.
>
Well, it feels that the driver isn't very acceptable for the core drivers/. But
maybe it's a wrong feeling. Again, will see what media/ maintainers would
suggest.
>> +static int tegra_vde_setup_context(struct tegra_vde *vde,
>> + struct tegra_vde_h264_decoder_ctx *ctx,
>> + struct video_frame *dpb_frames,
>> + phys_addr_t bitstream_data_paddr,
>> + int bitstream_data_size,
>> + int macroblocks_nb)
>> +{
>> +struct device *dev = vde->miscdev.parent;
>> +u32 value;
>> +
>> +tegra_vde_set_bits(vde->regs,0xA, SXE(0xF0));
>> +tegra_vde_set_bits(vde->regs,0xB, BSEV(CMDQUE_CONTROL));
>> +tegra_vde_set_bits(vde->regs, 0x8002, MBE(0x50));
>> +tegra_vde_set_bits(vde->regs,0xA, MBE(0xA0));
>> +tegra_vde_set_bits(vde->regs,0xA, PPE(0x14));
>> +tegra_vde_set_bits(vde->regs,0xA, PPE(0x28));
>> +tegra_vde_set_bits(vde->regs, 0xA00, MCE(0x08));
>> +tegra_vde_set_bits(vde->regs,0xA, TFE(0x00));
>> +tegra_vde_set_bits(vde->regs,0x5, VDMA(0x04));
>> +
>> +VDE_WR(0x, vde->regs + VDMA(0x1C));
>> +VDE_WR(0x, vde->regs + VDMA(0x00));
>> +VDE_WR(0x0007, vde->regs + VDMA(0x04));
>> +VDE_WR(0x0007, vde->regs + FRAMEID(0x200));
>> +VDE_WR(0x0005, vde->regs + TFE(0x04));
>> +VDE_WR(0x, vde->regs + MBE(0x84));
>> +VDE_WR(0x0010, vde->regs + SXE(0x08));
>> +VDE_WR(0x0150, vde->regs + SXE(0x54));
>> +VDE_WR(0x054C, vde->regs + SXE(0x58));
>> +VDE_WR(0x0E34, vde->regs + SXE(0x5C));
>> +VDE_WR(0x063C063C, vde->regs + MCE(0x10));
>> +VDE_WR(0x0003FC00, vde->regs + BSEV(INTR_STATUS));
>> +VDE_WR(0x150D, vde->regs + BSEV(BSE_CONFIG));
>> +VDE_WR(0x0100, vde->regs + BSEV(BSE_INT_ENB));
>> +VDE_WR(0x, vde->regs + BSEV(0x98));
>> +VDE_WR(0x0060, vde->regs + BSEV(0x9C));
>> +
>> +memset_io(vde->iram + 512, 0, macroblocks_nb / 2);
>> +
>> +tegra_setup_frameidx(vde->regs, dpb_frames, ctx->dpb_frames_nb,
>> + ctx->pic_width_in_mbs, ctx->pic_height_in_mbs);
>> +
>> +tegra_vde_setup_iram_tables(vde->iram, dpb_frames,
>> +ctx->dpb_frames_nb - 1,
>> +ctx->dpb_ref_frames_with_earlier_poc_nb);
>> +
>> +VDE_WR(0x, vde->regs + BSEV(0x8C));
>> +VDE_WR(bitstream_data_paddr + bitstream_data_size,
>> + vde->regs + BSEV(0x54));
>> +
>> +value = ctx->pic_width_in_mbs << 11 | ctx->pic_height_in_mbs << 3;
>> +
>> +VDE_WR(value, vde->regs + BSEV(0x88));
>> +
>> +if (tegra_vde_wait_bsev(vde, false))
>> +return -EIO;
>> +
>> +if (tegra_vde_push_bsev_icmdqueue(vde, 0x83FC, false))
>
> Preserve the error code from tegra_vde_push_bsev_icmdqueue(). It's
> still -EIO so
Re: [PATCH v1 1/2] staging: Introduce NVIDIA Tegra20 video decoder driver
I feel like this code is good enough to go into the regular kernel
instead of staging, but I don't really know what "- properly handle
decoding faults" means in this context. Does the driver Oops all the
time or what?
Anyway, minor comments inline.
On Tue, Sep 26, 2017 at 01:15:42AM +0300, Dmitry Osipenko wrote:
> diff --git a/drivers/staging/tegra-vde/Kconfig
> b/drivers/staging/tegra-vde/Kconfig
> new file mode 100644
> index ..b947c012a373
> --- /dev/null
> +++ b/drivers/staging/tegra-vde/Kconfig
> @@ -0,0 +1,6 @@
> +config TEGRA_VDE
> + tristate "NVIDIA Tegra20 video decoder driver"
> + depends on ARCH_TEGRA_2x_SOC
Could we get a || COMPILE_TEST here as well?
> + help
> + Say Y here to enable support for a NVIDIA Tegra20 video decoder
> + driver.
> diff --git a/drivers/staging/tegra-vde/Makefile
> b/drivers/staging/tegra-vde/Makefile
> new file mode 100644
> index ..e7c0df1174bf
> --- /dev/null
> +++ b/drivers/staging/tegra-vde/Makefile
> @@ -0,0 +1 @@
> +obj-$(CONFIG_TEGRA_VDE) += vde.o
> diff --git a/drivers/staging/tegra-vde/TODO b/drivers/staging/tegra-vde/TODO
> new file mode 100644
> index ..533ddfc5190e
> --- /dev/null
> +++ b/drivers/staging/tegra-vde/TODO
> @@ -0,0 +1,8 @@
> +All TODO's require reverse-engineering to be done first, it is very
> +unlikely that NVIDIA would ever release HW specs to public.
> +
> +TODO:
> + - properly handle decoding faults
> + - support more formats
Adding more formats is not a reason to put this into staging instead of
the normal drivers/ dir.
> +static int tegra_vde_setup_context(struct tegra_vde *vde,
> +struct tegra_vde_h264_decoder_ctx *ctx,
> +struct video_frame *dpb_frames,
> +phys_addr_t bitstream_data_paddr,
> +int bitstream_data_size,
> +int macroblocks_nb)
> +{
> + struct device *dev = vde->miscdev.parent;
> + u32 value;
> +
> + tegra_vde_set_bits(vde->regs,0xA, SXE(0xF0));
> + tegra_vde_set_bits(vde->regs,0xB, BSEV(CMDQUE_CONTROL));
> + tegra_vde_set_bits(vde->regs, 0x8002, MBE(0x50));
> + tegra_vde_set_bits(vde->regs,0xA, MBE(0xA0));
> + tegra_vde_set_bits(vde->regs,0xA, PPE(0x14));
> + tegra_vde_set_bits(vde->regs,0xA, PPE(0x28));
> + tegra_vde_set_bits(vde->regs, 0xA00, MCE(0x08));
> + tegra_vde_set_bits(vde->regs,0xA, TFE(0x00));
> + tegra_vde_set_bits(vde->regs,0x5, VDMA(0x04));
> +
> + VDE_WR(0x, vde->regs + VDMA(0x1C));
> + VDE_WR(0x, vde->regs + VDMA(0x00));
> + VDE_WR(0x0007, vde->regs + VDMA(0x04));
> + VDE_WR(0x0007, vde->regs + FRAMEID(0x200));
> + VDE_WR(0x0005, vde->regs + TFE(0x04));
> + VDE_WR(0x, vde->regs + MBE(0x84));
> + VDE_WR(0x0010, vde->regs + SXE(0x08));
> + VDE_WR(0x0150, vde->regs + SXE(0x54));
> + VDE_WR(0x054C, vde->regs + SXE(0x58));
> + VDE_WR(0x0E34, vde->regs + SXE(0x5C));
> + VDE_WR(0x063C063C, vde->regs + MCE(0x10));
> + VDE_WR(0x0003FC00, vde->regs + BSEV(INTR_STATUS));
> + VDE_WR(0x150D, vde->regs + BSEV(BSE_CONFIG));
> + VDE_WR(0x0100, vde->regs + BSEV(BSE_INT_ENB));
> + VDE_WR(0x, vde->regs + BSEV(0x98));
> + VDE_WR(0x0060, vde->regs + BSEV(0x9C));
> +
> + memset_io(vde->iram + 512, 0, macroblocks_nb / 2);
> +
> + tegra_setup_frameidx(vde->regs, dpb_frames, ctx->dpb_frames_nb,
> + ctx->pic_width_in_mbs, ctx->pic_height_in_mbs);
> +
> + tegra_vde_setup_iram_tables(vde->iram, dpb_frames,
> + ctx->dpb_frames_nb - 1,
> + ctx->dpb_ref_frames_with_earlier_poc_nb);
> +
> + VDE_WR(0x, vde->regs + BSEV(0x8C));
> + VDE_WR(bitstream_data_paddr + bitstream_data_size,
> +vde->regs + BSEV(0x54));
> +
> + value = ctx->pic_width_in_mbs << 11 | ctx->pic_height_in_mbs << 3;
> +
> + VDE_WR(value, vde->regs + BSEV(0x88));
> +
> + if (tegra_vde_wait_bsev(vde, false))
> + return -EIO;
> +
> + if (tegra_vde_push_bsev_icmdqueue(vde, 0x83FC, false))
Preserve the error code from tegra_vde_push_bsev_icmdqueue(). It's
still -EIO so this doesn't affect runtime.
> + return -EIO;
> +
> + value = 0x0150;
> + value |= ((vde->iram_lists_paddr + 512) >> 2) & 0x;
> +
> + if (tegra_vde_push_bsev_icmdqueue(vde, value, true))
Same for all.
> + return -EIO;
> +
> + if (tegra_vde_push_bsev_icmdqueue(vde, 0x840F054C, false))
> + return -EIO;
> +
> + if (tegra_vde_push_bsev_icmdqueue(vde, 0x8080, false))
> + return -EIO;
> +
> + value = 0x0E34 | ((vde->iram_lists_paddr >> 2) & 0x);
> +
> + if (tegra_vde_push_bsev_icmdqueue(vde, value, true))
> +
Re: [PATCH v1 1/2] staging: Introduce NVIDIA Tegra20 video decoder driver
I feel like this code is good enough to go into the regular kernel
instead of staging, but I don't really know what "- properly handle
decoding faults" means in this context. Does the driver Oops all the
time or what?
Anyway, minor comments inline.
On Tue, Sep 26, 2017 at 01:15:42AM +0300, Dmitry Osipenko wrote:
> diff --git a/drivers/staging/tegra-vde/Kconfig
> b/drivers/staging/tegra-vde/Kconfig
> new file mode 100644
> index ..b947c012a373
> --- /dev/null
> +++ b/drivers/staging/tegra-vde/Kconfig
> @@ -0,0 +1,6 @@
> +config TEGRA_VDE
> + tristate "NVIDIA Tegra20 video decoder driver"
> + depends on ARCH_TEGRA_2x_SOC
Could we get a || COMPILE_TEST here as well?
> + help
> + Say Y here to enable support for a NVIDIA Tegra20 video decoder
> + driver.
> diff --git a/drivers/staging/tegra-vde/Makefile
> b/drivers/staging/tegra-vde/Makefile
> new file mode 100644
> index ..e7c0df1174bf
> --- /dev/null
> +++ b/drivers/staging/tegra-vde/Makefile
> @@ -0,0 +1 @@
> +obj-$(CONFIG_TEGRA_VDE) += vde.o
> diff --git a/drivers/staging/tegra-vde/TODO b/drivers/staging/tegra-vde/TODO
> new file mode 100644
> index ..533ddfc5190e
> --- /dev/null
> +++ b/drivers/staging/tegra-vde/TODO
> @@ -0,0 +1,8 @@
> +All TODO's require reverse-engineering to be done first, it is very
> +unlikely that NVIDIA would ever release HW specs to public.
> +
> +TODO:
> + - properly handle decoding faults
> + - support more formats
Adding more formats is not a reason to put this into staging instead of
the normal drivers/ dir.
> +static int tegra_vde_setup_context(struct tegra_vde *vde,
> +struct tegra_vde_h264_decoder_ctx *ctx,
> +struct video_frame *dpb_frames,
> +phys_addr_t bitstream_data_paddr,
> +int bitstream_data_size,
> +int macroblocks_nb)
> +{
> + struct device *dev = vde->miscdev.parent;
> + u32 value;
> +
> + tegra_vde_set_bits(vde->regs,0xA, SXE(0xF0));
> + tegra_vde_set_bits(vde->regs,0xB, BSEV(CMDQUE_CONTROL));
> + tegra_vde_set_bits(vde->regs, 0x8002, MBE(0x50));
> + tegra_vde_set_bits(vde->regs,0xA, MBE(0xA0));
> + tegra_vde_set_bits(vde->regs,0xA, PPE(0x14));
> + tegra_vde_set_bits(vde->regs,0xA, PPE(0x28));
> + tegra_vde_set_bits(vde->regs, 0xA00, MCE(0x08));
> + tegra_vde_set_bits(vde->regs,0xA, TFE(0x00));
> + tegra_vde_set_bits(vde->regs,0x5, VDMA(0x04));
> +
> + VDE_WR(0x, vde->regs + VDMA(0x1C));
> + VDE_WR(0x, vde->regs + VDMA(0x00));
> + VDE_WR(0x0007, vde->regs + VDMA(0x04));
> + VDE_WR(0x0007, vde->regs + FRAMEID(0x200));
> + VDE_WR(0x0005, vde->regs + TFE(0x04));
> + VDE_WR(0x, vde->regs + MBE(0x84));
> + VDE_WR(0x0010, vde->regs + SXE(0x08));
> + VDE_WR(0x0150, vde->regs + SXE(0x54));
> + VDE_WR(0x054C, vde->regs + SXE(0x58));
> + VDE_WR(0x0E34, vde->regs + SXE(0x5C));
> + VDE_WR(0x063C063C, vde->regs + MCE(0x10));
> + VDE_WR(0x0003FC00, vde->regs + BSEV(INTR_STATUS));
> + VDE_WR(0x150D, vde->regs + BSEV(BSE_CONFIG));
> + VDE_WR(0x0100, vde->regs + BSEV(BSE_INT_ENB));
> + VDE_WR(0x, vde->regs + BSEV(0x98));
> + VDE_WR(0x0060, vde->regs + BSEV(0x9C));
> +
> + memset_io(vde->iram + 512, 0, macroblocks_nb / 2);
> +
> + tegra_setup_frameidx(vde->regs, dpb_frames, ctx->dpb_frames_nb,
> + ctx->pic_width_in_mbs, ctx->pic_height_in_mbs);
> +
> + tegra_vde_setup_iram_tables(vde->iram, dpb_frames,
> + ctx->dpb_frames_nb - 1,
> + ctx->dpb_ref_frames_with_earlier_poc_nb);
> +
> + VDE_WR(0x, vde->regs + BSEV(0x8C));
> + VDE_WR(bitstream_data_paddr + bitstream_data_size,
> +vde->regs + BSEV(0x54));
> +
> + value = ctx->pic_width_in_mbs << 11 | ctx->pic_height_in_mbs << 3;
> +
> + VDE_WR(value, vde->regs + BSEV(0x88));
> +
> + if (tegra_vde_wait_bsev(vde, false))
> + return -EIO;
> +
> + if (tegra_vde_push_bsev_icmdqueue(vde, 0x83FC, false))
Preserve the error code from tegra_vde_push_bsev_icmdqueue(). It's
still -EIO so this doesn't affect runtime.
> + return -EIO;
> +
> + value = 0x0150;
> + value |= ((vde->iram_lists_paddr + 512) >> 2) & 0x;
> +
> + if (tegra_vde_push_bsev_icmdqueue(vde, value, true))
Same for all.
> + return -EIO;
> +
> + if (tegra_vde_push_bsev_icmdqueue(vde, 0x840F054C, false))
> + return -EIO;
> +
> + if (tegra_vde_push_bsev_icmdqueue(vde, 0x8080, false))
> + return -EIO;
> +
> + value = 0x0E34 | ((vde->iram_lists_paddr >> 2) & 0x);
> +
> + if (tegra_vde_push_bsev_icmdqueue(vde, value, true))
> +
Re: [PATCH v1 1/2] staging: Introduce NVIDIA Tegra20 video decoder driver
On 26.09.2017 08:11, Stephen Warren wrote: > On 09/25/2017 05:45 PM, Dmitry Osipenko wrote: >> On 26.09.2017 02:01, Stephen Warren wrote: >>> On 09/25/2017 04:15 PM, Dmitry Osipenko wrote: Video decoder, found on NVIDIA Tegra20 SoC, supports a standard set of video formats like H.264 / MPEG-4 / WMV / VC1. Currently driver supports decoding of CAVLC H.264 only. >>> >>> Note: I don't know anything much about video decoding on Tegra (just NV >>> desktop >>> GPUs, and that was a while ago), but I had a couple small comments on the DT >>> binding: >>> diff --git a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-vde.txt b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-vde.txt >>> +NVIDIA Tegra Video Decoder Engine + +Required properties: +- compatible : "nvidia,tegra20-vde" +- reg : Must contain 2 register ranges: registers and IRAM area. +- reg-names : Must include the following entries: + - regs + - iram >>> >>> I think the IRAM region needs more explanation: What is the region used for >>> and >>> by what? Can it be moved, and if so does the move need to be co-ordinated >>> with >>> any other piece of SW? >> >> IRAM region is used by Video Decoder HW for internal use and some of decoding >> parameters are supplied via IRAM, like frames order list. AFAIK IRAM >> addresses >> are hardwired in HW and aren't movable, it is not 100% but I'm pretty sure. >> Should it be explained in the binding? > > I think this should be briefly mentioned, yes. Otherwise at least people > who don't know the VDE HW well (like me) will wonder why on earth VDE > interacts with IRAM at all. I would have assumed all parameters were > supplied via registers or via descriptors in DRAM. > > Thanks. > I also forgot to mention that VDE scrubs that IRAM region on HW reset. So yeah, it's definitely a part of HW definition. I'll add a brief explanation to the binding. -- Dmitry
Re: [PATCH v1 1/2] staging: Introduce NVIDIA Tegra20 video decoder driver
On 26.09.2017 08:11, Stephen Warren wrote: > On 09/25/2017 05:45 PM, Dmitry Osipenko wrote: >> On 26.09.2017 02:01, Stephen Warren wrote: >>> On 09/25/2017 04:15 PM, Dmitry Osipenko wrote: Video decoder, found on NVIDIA Tegra20 SoC, supports a standard set of video formats like H.264 / MPEG-4 / WMV / VC1. Currently driver supports decoding of CAVLC H.264 only. >>> >>> Note: I don't know anything much about video decoding on Tegra (just NV >>> desktop >>> GPUs, and that was a while ago), but I had a couple small comments on the DT >>> binding: >>> diff --git a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-vde.txt b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-vde.txt >>> +NVIDIA Tegra Video Decoder Engine + +Required properties: +- compatible : "nvidia,tegra20-vde" +- reg : Must contain 2 register ranges: registers and IRAM area. +- reg-names : Must include the following entries: + - regs + - iram >>> >>> I think the IRAM region needs more explanation: What is the region used for >>> and >>> by what? Can it be moved, and if so does the move need to be co-ordinated >>> with >>> any other piece of SW? >> >> IRAM region is used by Video Decoder HW for internal use and some of decoding >> parameters are supplied via IRAM, like frames order list. AFAIK IRAM >> addresses >> are hardwired in HW and aren't movable, it is not 100% but I'm pretty sure. >> Should it be explained in the binding? > > I think this should be briefly mentioned, yes. Otherwise at least people > who don't know the VDE HW well (like me) will wonder why on earth VDE > interacts with IRAM at all. I would have assumed all parameters were > supplied via registers or via descriptors in DRAM. > > Thanks. > I also forgot to mention that VDE scrubs that IRAM region on HW reset. So yeah, it's definitely a part of HW definition. I'll add a brief explanation to the binding. -- Dmitry
Re: [PATCH v1 1/2] staging: Introduce NVIDIA Tegra20 video decoder driver
On 09/25/2017 05:45 PM, Dmitry Osipenko wrote: > On 26.09.2017 02:01, Stephen Warren wrote: >> On 09/25/2017 04:15 PM, Dmitry Osipenko wrote: >>> Video decoder, found on NVIDIA Tegra20 SoC, supports a standard set of >>> video formats like H.264 / MPEG-4 / WMV / VC1. Currently driver supports >>> decoding of CAVLC H.264 only. >> >> Note: I don't know anything much about video decoding on Tegra (just NV >> desktop >> GPUs, and that was a while ago), but I had a couple small comments on the DT >> binding: >> >>> diff --git >>> a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-vde.txt >>> b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-vde.txt >> >>> +NVIDIA Tegra Video Decoder Engine >>> + >>> +Required properties: >>> +- compatible : "nvidia,tegra20-vde" >>> +- reg : Must contain 2 register ranges: registers and IRAM area. >>> +- reg-names : Must include the following entries: >>> + - regs >>> + - iram >> >> I think the IRAM region needs more explanation: What is the region used for >> and >> by what? Can it be moved, and if so does the move need to be co-ordinated >> with >> any other piece of SW? > > IRAM region is used by Video Decoder HW for internal use and some of decoding > parameters are supplied via IRAM, like frames order list. AFAIK IRAM addresses > are hardwired in HW and aren't movable, it is not 100% but I'm pretty sure. > Should it be explained in the binding? I think this should be briefly mentioned, yes. Otherwise at least people who don't know the VDE HW well (like me) will wonder why on earth VDE interacts with IRAM at all. I would have assumed all parameters were supplied via registers or via descriptors in DRAM. Thanks.
Re: [PATCH v1 1/2] staging: Introduce NVIDIA Tegra20 video decoder driver
On 09/25/2017 05:45 PM, Dmitry Osipenko wrote: > On 26.09.2017 02:01, Stephen Warren wrote: >> On 09/25/2017 04:15 PM, Dmitry Osipenko wrote: >>> Video decoder, found on NVIDIA Tegra20 SoC, supports a standard set of >>> video formats like H.264 / MPEG-4 / WMV / VC1. Currently driver supports >>> decoding of CAVLC H.264 only. >> >> Note: I don't know anything much about video decoding on Tegra (just NV >> desktop >> GPUs, and that was a while ago), but I had a couple small comments on the DT >> binding: >> >>> diff --git >>> a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-vde.txt >>> b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-vde.txt >> >>> +NVIDIA Tegra Video Decoder Engine >>> + >>> +Required properties: >>> +- compatible : "nvidia,tegra20-vde" >>> +- reg : Must contain 2 register ranges: registers and IRAM area. >>> +- reg-names : Must include the following entries: >>> + - regs >>> + - iram >> >> I think the IRAM region needs more explanation: What is the region used for >> and >> by what? Can it be moved, and if so does the move need to be co-ordinated >> with >> any other piece of SW? > > IRAM region is used by Video Decoder HW for internal use and some of decoding > parameters are supplied via IRAM, like frames order list. AFAIK IRAM addresses > are hardwired in HW and aren't movable, it is not 100% but I'm pretty sure. > Should it be explained in the binding? I think this should be briefly mentioned, yes. Otherwise at least people who don't know the VDE HW well (like me) will wonder why on earth VDE interacts with IRAM at all. I would have assumed all parameters were supplied via registers or via descriptors in DRAM. Thanks.
Re: [PATCH v1 1/2] staging: Introduce NVIDIA Tegra20 video decoder driver
On 26.09.2017 02:01, Stephen Warren wrote: > On 09/25/2017 04:15 PM, Dmitry Osipenko wrote: >> Video decoder, found on NVIDIA Tegra20 SoC, supports a standard set of >> video formats like H.264 / MPEG-4 / WMV / VC1. Currently driver supports >> decoding of CAVLC H.264 only. > > Note: I don't know anything much about video decoding on Tegra (just NV > desktop > GPUs, and that was a while ago), but I had a couple small comments on the DT > binding: > >> diff --git >> a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-vde.txt >> b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-vde.txt > >> +NVIDIA Tegra Video Decoder Engine >> + >> +Required properties: >> +- compatible : "nvidia,tegra20-vde" >> +- reg : Must contain 2 register ranges: registers and IRAM area. >> +- reg-names : Must include the following entries: >> + - regs >> + - iram > > I think the IRAM region needs more explanation: What is the region used for > and > by what? Can it be moved, and if so does the move need to be co-ordinated with > any other piece of SW? > IRAM region is used by Video Decoder HW for internal use and some of decoding parameters are supplied via IRAM, like frames order list. AFAIK IRAM addresses are hardwired in HW and aren't movable, it is not 100% but I'm pretty sure. Should it be explained in the binding? >> +- clocks : Must contain one entry, for the module clock. >> + See ../clocks/clock-bindings.txt for details. >> +- resets : Must contain an entry for each entry in reset-names. >> + See ../reset/reset.txt for details. >> +- reset-names : Must include the following entries: >> + - vde > > Let's require a clock-names property too. Okay, I'll add this property to the binding. -- Dmitry
Re: [PATCH v1 1/2] staging: Introduce NVIDIA Tegra20 video decoder driver
On 26.09.2017 02:01, Stephen Warren wrote: > On 09/25/2017 04:15 PM, Dmitry Osipenko wrote: >> Video decoder, found on NVIDIA Tegra20 SoC, supports a standard set of >> video formats like H.264 / MPEG-4 / WMV / VC1. Currently driver supports >> decoding of CAVLC H.264 only. > > Note: I don't know anything much about video decoding on Tegra (just NV > desktop > GPUs, and that was a while ago), but I had a couple small comments on the DT > binding: > >> diff --git >> a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-vde.txt >> b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-vde.txt > >> +NVIDIA Tegra Video Decoder Engine >> + >> +Required properties: >> +- compatible : "nvidia,tegra20-vde" >> +- reg : Must contain 2 register ranges: registers and IRAM area. >> +- reg-names : Must include the following entries: >> + - regs >> + - iram > > I think the IRAM region needs more explanation: What is the region used for > and > by what? Can it be moved, and if so does the move need to be co-ordinated with > any other piece of SW? > IRAM region is used by Video Decoder HW for internal use and some of decoding parameters are supplied via IRAM, like frames order list. AFAIK IRAM addresses are hardwired in HW and aren't movable, it is not 100% but I'm pretty sure. Should it be explained in the binding? >> +- clocks : Must contain one entry, for the module clock. >> + See ../clocks/clock-bindings.txt for details. >> +- resets : Must contain an entry for each entry in reset-names. >> + See ../reset/reset.txt for details. >> +- reset-names : Must include the following entries: >> + - vde > > Let's require a clock-names property too. Okay, I'll add this property to the binding. -- Dmitry
Re: [PATCH v1 1/2] staging: Introduce NVIDIA Tegra20 video decoder driver
On 09/25/2017 04:15 PM, Dmitry Osipenko wrote: Video decoder, found on NVIDIA Tegra20 SoC, supports a standard set of video formats like H.264 / MPEG-4 / WMV / VC1. Currently driver supports decoding of CAVLC H.264 only. Note: I don't know anything much about video decoding on Tegra (just NV desktop GPUs, and that was a while ago), but I had a couple small comments on the DT binding: diff --git a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-vde.txt b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-vde.txt +NVIDIA Tegra Video Decoder Engine + +Required properties: +- compatible : "nvidia,tegra20-vde" +- reg : Must contain 2 register ranges: registers and IRAM area. +- reg-names : Must include the following entries: + - regs + - iram I think the IRAM region needs more explanation: What is the region used for and by what? Can it be moved, and if so does the move need to be co-ordinated with any other piece of SW? +- clocks : Must contain one entry, for the module clock. + See ../clocks/clock-bindings.txt for details. +- resets : Must contain an entry for each entry in reset-names. + See ../reset/reset.txt for details. +- reset-names : Must include the following entries: + - vde Let's require a clock-names property too.
Re: [PATCH v1 1/2] staging: Introduce NVIDIA Tegra20 video decoder driver
On 09/25/2017 04:15 PM, Dmitry Osipenko wrote: Video decoder, found on NVIDIA Tegra20 SoC, supports a standard set of video formats like H.264 / MPEG-4 / WMV / VC1. Currently driver supports decoding of CAVLC H.264 only. Note: I don't know anything much about video decoding on Tegra (just NV desktop GPUs, and that was a while ago), but I had a couple small comments on the DT binding: diff --git a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-vde.txt b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-vde.txt +NVIDIA Tegra Video Decoder Engine + +Required properties: +- compatible : "nvidia,tegra20-vde" +- reg : Must contain 2 register ranges: registers and IRAM area. +- reg-names : Must include the following entries: + - regs + - iram I think the IRAM region needs more explanation: What is the region used for and by what? Can it be moved, and if so does the move need to be co-ordinated with any other piece of SW? +- clocks : Must contain one entry, for the module clock. + See ../clocks/clock-bindings.txt for details. +- resets : Must contain an entry for each entry in reset-names. + See ../reset/reset.txt for details. +- reset-names : Must include the following entries: + - vde Let's require a clock-names property too.
