Re: [PATCH v2 2/2] media: staging/intel-ipu3: Fix set_fmt error handling
On Mon, Mar 15, 2021 at 01:34:06PM +0100, Ricardo Ribalda wrote: > If there in an error during a set_fmt, do not overwrite the previous > sizes with the invalid config. > > [ 38.662975] ipu3-imgu :00:05.0: swiotlb buffer is full (sz: 4096 bytes) > [ 38.662980] DMA: Out of SW-IOMMU space for 4096 bytes at device > :00:05.0 > [ 38.663010] general protection fault: [#1] PREEMPT SMP > > Cc: sta...@vger.kernel.org > Fixes: 6d5f26f2e045 ("media: staging/intel-ipu3-v4l: reduce kernel stack > usage") > Signed-off-by: Ricardo Ribalda > --- > drivers/staging/media/ipu3/ipu3-v4l2.c | 25 ++--- > 1 file changed, 14 insertions(+), 11 deletions(-) Reviewed-by: Tomasz Figa Best regards, Tomasz ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v5 8/9] arm64: dts: rockchip: add isp0 node for rk3399
On Wed, Oct 14, 2020 at 6:27 PM Helen Koike wrote: > > Hi Tomasz, > > On 9/26/20 10:00 AM, Tomasz Figa wrote: > > Hi Helen, > > > > On Wed, Jul 22, 2020 at 12:55:32PM -0300, Helen Koike wrote: > >> From: Shunqian Zheng > >> > >> RK3399 has two ISPs, but only isp0 was tested. > >> Add isp0 node in rk3399 dtsi > >> > >> Verified with: > >> make ARCH=arm64 dtbs_check > >> DT_SCHEMA_FILES=Documentation/devicetree/bindings/media/rockchip-isp1.yaml > >> > >> Signed-off-by: Shunqian Zheng > >> Signed-off-by: Jacob Chen > >> Signed-off-by: Helen Koike > >> > >> --- > >> > >> V4: > >> - update clock names > >> > >> V3: > >> - clean up clocks > >> > >> V2: > >> - re-order power-domains property > >> > >> V1: > >> This patch was originally part of this patchset: > >> > >> https://patchwork.kernel.org/patch/10267431/ > >> > >> The only difference is: > >> - add phy properties > >> - add ports > >> --- > >> arch/arm64/boot/dts/rockchip/rk3399.dtsi | 25 > >> 1 file changed, 25 insertions(+) > >> > >> diff --git a/arch/arm64/boot/dts/rockchip/rk3399.dtsi > >> b/arch/arm64/boot/dts/rockchip/rk3399.dtsi > >> index dba9641947a3a..ed8ba75dbbce8 100644 > >> --- a/arch/arm64/boot/dts/rockchip/rk3399.dtsi > >> +++ b/arch/arm64/boot/dts/rockchip/rk3399.dtsi > >> @@ -1721,6 +1721,31 @@ vopb_mmu: iommu@ff903f00 { > >> status = "disabled"; > >> }; > >> > >> +isp0: isp0@ff91 { > >> +compatible = "rockchip,rk3399-cif-isp"; > >> +reg = <0x0 0xff91 0x0 0x4000>; > >> +interrupts = ; > >> +clocks = < SCLK_ISP0>, > >> + < ACLK_ISP0_WRAPPER>, > >> + < HCLK_ISP0_WRAPPER>; > >> +clock-names = "isp", "aclk", "hclk"; > >> +iommus = <_mmu>; > >> +phys = <_dphy_rx0>; > >> +phy-names = "dphy"; > >> +power-domains = < RK3399_PD_ISP0>; > > > > Should this have status = "disabled" too? The mipi_dphy_rx0 node is > > disabled by default too, so in the default configuration the driver > > would always fail to probe. > > I'm thinking what is the overall guideline here. > Since isp and mipi_dphy are always present in the rk3399, shouldn't they > always be enabled? > Or since they are only useful if a sensor is present, we should let the dts > of the board to > enable it? I don't have a strong opinion. I'm fine with enabling both by default as well, as it shouldn't hurt. That said, I recall some alternative CIF IP block being present on this SoC as well (and patches posted recently), which AFAIR can't be activated at the same time as the ISP, so perhaps both of the alternatives should be disabled by default? Best regards, Tomasz ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v5 0/9] move Rockchip ISP bindings out of staging / add ISP DT nodes for RK3399
Hi Helen, On Wed, Jul 22, 2020 at 12:55:24PM -0300, Helen Koike wrote: > Move the bindings out of drivers/staging and place them in > Documentation/devicetree/bindings instead. > > Also, add DT nodes for RK3399 and verify with make ARCH=arm64 dtbs_check > and make ARCH=arm64 dt_binding_check. > > Tested by verifying images streamed from Scarlet Chromebook > > Changes in v5: > - Drop unit addresses in dt-bindings example for simplification and fix > errors as suggested by Rob Herring in previous version > - Fix typos > - Re-write clock organization with if/then schema > Besides one comment to patch 8/9, Reviewed-by: Tomasz Figa Best regards, Tomasz ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v5 8/9] arm64: dts: rockchip: add isp0 node for rk3399
Hi Helen, On Wed, Jul 22, 2020 at 12:55:32PM -0300, Helen Koike wrote: > From: Shunqian Zheng > > RK3399 has two ISPs, but only isp0 was tested. > Add isp0 node in rk3399 dtsi > > Verified with: > make ARCH=arm64 dtbs_check > DT_SCHEMA_FILES=Documentation/devicetree/bindings/media/rockchip-isp1.yaml > > Signed-off-by: Shunqian Zheng > Signed-off-by: Jacob Chen > Signed-off-by: Helen Koike > > --- > > V4: > - update clock names > > V3: > - clean up clocks > > V2: > - re-order power-domains property > > V1: > This patch was originally part of this patchset: > > https://patchwork.kernel.org/patch/10267431/ > > The only difference is: > - add phy properties > - add ports > --- > arch/arm64/boot/dts/rockchip/rk3399.dtsi | 25 > 1 file changed, 25 insertions(+) > > diff --git a/arch/arm64/boot/dts/rockchip/rk3399.dtsi > b/arch/arm64/boot/dts/rockchip/rk3399.dtsi > index dba9641947a3a..ed8ba75dbbce8 100644 > --- a/arch/arm64/boot/dts/rockchip/rk3399.dtsi > +++ b/arch/arm64/boot/dts/rockchip/rk3399.dtsi > @@ -1721,6 +1721,31 @@ vopb_mmu: iommu@ff903f00 { > status = "disabled"; > }; > > + isp0: isp0@ff91 { > + compatible = "rockchip,rk3399-cif-isp"; > + reg = <0x0 0xff91 0x0 0x4000>; > + interrupts = ; > + clocks = < SCLK_ISP0>, > + < ACLK_ISP0_WRAPPER>, > + < HCLK_ISP0_WRAPPER>; > + clock-names = "isp", "aclk", "hclk"; > + iommus = <_mmu>; > + phys = <_dphy_rx0>; > + phy-names = "dphy"; > + power-domains = < RK3399_PD_ISP0>; Should this have status = "disabled" too? The mipi_dphy_rx0 node is disabled by default too, so in the default configuration the driver would always fail to probe. Best regards, Tomasz ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] media: staging: ipu3: Fix stale list entries on parameter queue failure
: dead0100 [ 225.360517] R13: 8aa7820f1940 R14: dead0100 R15: 0006 [ 225.360519] FS: 7c1146ffd700() GS:8aa7baa0() knlGS: [ 225.360521] CS: 0010 DS: ES: CR0: 80050033 [ 225.360523] CR2: 7aea3473a000 CR3: 537d6004 CR4: 003606f0 [ 225.360525] Call Trace: [ 225.360528] imgu_vb2_stop_streaming+0xd6/0xf0 [ipu3_imgu] [ 225.360531] __vb2_queue_cancel+0x33/0x22d [videobuf2_common] [ 225.360534] vb2_core_streamoff+0x16/0x78 [videobuf2_common] [ 225.360537] __video_do_ioctl+0x33d/0x42a [ 225.360540] video_usercopy+0x34a/0x615 [ 225.360542] ? video_ioctl2+0x16/0x16 [ 225.360546] v4l2_ioctl+0x46/0x53 [ 225.360548] do_vfs_ioctl+0x50a/0x787 [ 225.360551] ksys_ioctl+0x58/0x83 [ 225.360554] __x64_sys_ioctl+0x1a/0x1e [ 225.360556] do_syscall_64+0x54/0x68 [ 225.360559] entry_SYSCALL_64_after_hwframe+0x44/0xa9 [ 225.360561] RIP: 0033:0x7c118030f497 [ 225.360563] Code: 8a 66 90 48 8b 05 d1 d9 2b 00 64 c7 00 26 00 00 00 48 c7 c0 ff ff ff ff c3 66 2e 0f 1f 84 00 00 00 00 00 b8 10 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d a1 d9 2b 00 f7 d8 64 89 01 48 [ 225.360565] RSP: 002b:7c1146ffa5a8 EFLAGS: 0246 ORIG_RAX: 0010 [ 225.360567] RAX: ffda RBX: 7c1140010018 RCX: 7c118030f497 [ 225.360569] RDX: 7c114001019c RSI: 40045613 RDI: 004c [ 225.360570] RBP: 7c1146ffa700 R08: 7c1140010048 R09: [ 225.360572] R10: R11: 0246 R12: 7c11400101b0 [ 225.360574] R13: 7c1140010200 R14: 7c1140010048 R15: 0001 [ 225.360576] Modules linked in: snd_seq_dummy snd_seq snd_seq_device veth bridge stp llc tun nf_nat_tftp nf_conntrack_tftp nf_nat_ftp nf_conntrack_ftp esp6 ah6 ip6t_REJECT ip6t_ipv6header cmac rfcomm uinput ipu3_imgu(C) ipu3_cio2 iova videobuf2_v4l2 videobuf2_common videobuf2_dma_sg videobuf2_memops ov13858 ov567 Fix this by moving the list_del() call just below the list_first_entry() call when the buffer no longer needs to be in the list. Fixes: 8ecc7c9da013 ("media: staging/intel-ipu3: parameter buffer refactoring") Signed-off-by: Tomasz Figa --- drivers/staging/media/ipu3/ipu3.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/staging/media/ipu3/ipu3.c b/drivers/staging/media/ipu3/ipu3.c index 4d53aad31483..7a1d1881483b 100644 --- a/drivers/staging/media/ipu3/ipu3.c +++ b/drivers/staging/media/ipu3/ipu3.c @@ -261,6 +261,7 @@ int imgu_queue_buffers(struct imgu_device *imgu, bool initial, unsigned int pipe ivb = list_first_entry(_pipe->nodes[node].buffers, struct imgu_vb2_buffer, list); + list_del(>list); vb = >vbb.vb2_buf; r = imgu_css_set_parameters(>css, pipe, vb2_plane_vaddr(vb, 0)); @@ -274,7 +275,6 @@ int imgu_queue_buffers(struct imgu_device *imgu, bool initial, unsigned int pipe vb2_buffer_done(vb, VB2_BUF_STATE_DONE); dev_dbg(>pci_dev->dev, "queue user parameters %d to css.", vb->index); - list_del(>list); } else if (imgu_pipe->queue_enabled[node]) { struct imgu_css_buffer *buf = imgu_queue_getbuf(imgu, node, pipe); -- 2.26.0.110.g2183baf09c-goog ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] media: hantro: Support H264 profile control
On Mon, Feb 3, 2020 at 9:00 PM Hans Verkuil wrote: > > On 2/3/20 12:13 PM, Tomasz Figa wrote: > > On Sat, Jan 18, 2020 at 10:31 PM Nicolas Dufresne > > wrote: > >> > >> Le vendredi 10 janvier 2020 à 13:31 +0100, Hans Verkuil a écrit : > >>> On 11/29/19 1:16 AM, Tomasz Figa wrote: > >>>> On Sat, Nov 23, 2019 at 1:52 AM Nicolas Dufresne > >>>> wrote: > >>>>> Le samedi 23 novembre 2019 à 01:00 +0900, Tomasz Figa a écrit : > >>>>>> On Sat, Nov 23, 2019 at 12:09 AM Nicolas Dufresne > >>>>>> > >>>>>> wrote: > >>>>>>> Le vendredi 22 novembre 2019 à 14:16 +0900, Hirokazu Honda a écrit : > >>>>>>>> The Hantro G1 decoder supports H.264 profiles from Baseline to High, > >>>>>>>> with > >>>>>>>> the exception of the Extended profile. > >>>>>>>> > >>>>>>>> Expose the V4L2_CID_MPEG_VIDEO_H264_PROFILE control, so that the > >>>>>>>> applications can query the driver for the list of supported > >>>>>>>> profiles. > >>>>>>> > >>>>>>> Thanks for this patch. Do you think we could also add the LEVEL > >>>>>>> control > >>>>>>> so the profile/level enumeration becomes complete ? > >>>>>>> > >>>>>>> I'm thinking it would be nice if the v4l2 compliance test make sure > >>>>>>> that codecs do implement these controls (both stateful and stateless), > >>>>>>> it's essential for stack with software fallback, or multiple capable > >>>>>>> codec hardware but with different capabilities. > >>>>>>> > >>>>>> > >>>>>> Level is a difficult story, because it also specifies the number of > >>>>>> macroblocks per second, but for decoders like this the number of > >>>>>> macroblocks per second it can handle depends on things the driver > >>>>>> might be not aware of - clock frequencies, DDR throughput, system > >>>>>> load, etc. > >>>>>> > >>>>>> My take on this is that the decoder driver should advertise the > >>>>>> highest resolution the decoder can handle due to hardware constraints. > >>>>>> Performance related things depend on the integration details and > >>>>>> should be managed elsewhere. For example Android and Chrome OS manage > >>>>>> expected decoding performance in per-board configuration files. > >>>>> > >>>>> When you read datasheet, the HW is always rated to maximum level (and > >>>>> it's a range) with the assumption of a single stream. It seems much > >>>>> easier to expose this as-is, statically then to start doing some math > >>>>> with data that isn't fully exposed to the user. This is about filtering > >>>>> of multiple CODEC instances, it does not need to be rocket science, > >>>>> specially that the amount of missing data is important (e.g. usage of > >>>>> tiles, compression, IPP all have an impact on the final performance). > >>>>> All we want to know in userspace is if this HW is even possibly capable > >>>>> of LEVEL X, and if not, we'll look for another one. > >>>>> > >>>> > >>>> Agreed, one could potentially define it this way, but would it be > >>>> really useful for the userspace and the users? I guess it could enable > >>>> slightly faster fallback to software decoding in the extreme case of > >>>> the hardware not supporting the level at all, but I suspect that the > >>>> majority of cases would be the hardware just being unusably slow. > >>>> > >>>> Also, as I mentioned before, we already return the range of supported > >>>> resolutions, which in practice should map to the part of the level > >>>> that may depend on hardware capabilities rather than performance, so > >>>> exposing levels as well would add redundancy to the information > >>>> exposed. > >>> > >>> There is a lot of discussion here, but it all revolves around a potential > >>> LEVEL control. > >>> > >>> So I gather everyone is OK with merging this patch?
Re: [PATCH] media: hantro: Support H264 profile control
On Sat, Jan 18, 2020 at 10:31 PM Nicolas Dufresne wrote: > > Le vendredi 10 janvier 2020 à 13:31 +0100, Hans Verkuil a écrit : > > On 11/29/19 1:16 AM, Tomasz Figa wrote: > > > On Sat, Nov 23, 2019 at 1:52 AM Nicolas Dufresne > > > wrote: > > > > Le samedi 23 novembre 2019 à 01:00 +0900, Tomasz Figa a écrit : > > > > > On Sat, Nov 23, 2019 at 12:09 AM Nicolas Dufresne > > > > > > > > > > wrote: > > > > > > Le vendredi 22 novembre 2019 à 14:16 +0900, Hirokazu Honda a écrit : > > > > > > > The Hantro G1 decoder supports H.264 profiles from Baseline to > > > > > > > High, > > > > > > > with > > > > > > > the exception of the Extended profile. > > > > > > > > > > > > > > Expose the V4L2_CID_MPEG_VIDEO_H264_PROFILE control, so that the > > > > > > > applications can query the driver for the list of supported > > > > > > > profiles. > > > > > > > > > > > > Thanks for this patch. Do you think we could also add the LEVEL > > > > > > control > > > > > > so the profile/level enumeration becomes complete ? > > > > > > > > > > > > I'm thinking it would be nice if the v4l2 compliance test make sure > > > > > > that codecs do implement these controls (both stateful and > > > > > > stateless), > > > > > > it's essential for stack with software fallback, or multiple capable > > > > > > codec hardware but with different capabilities. > > > > > > > > > > > > > > > > Level is a difficult story, because it also specifies the number of > > > > > macroblocks per second, but for decoders like this the number of > > > > > macroblocks per second it can handle depends on things the driver > > > > > might be not aware of - clock frequencies, DDR throughput, system > > > > > load, etc. > > > > > > > > > > My take on this is that the decoder driver should advertise the > > > > > highest resolution the decoder can handle due to hardware constraints. > > > > > Performance related things depend on the integration details and > > > > > should be managed elsewhere. For example Android and Chrome OS manage > > > > > expected decoding performance in per-board configuration files. > > > > > > > > When you read datasheet, the HW is always rated to maximum level (and > > > > it's a range) with the assumption of a single stream. It seems much > > > > easier to expose this as-is, statically then to start doing some math > > > > with data that isn't fully exposed to the user. This is about filtering > > > > of multiple CODEC instances, it does not need to be rocket science, > > > > specially that the amount of missing data is important (e.g. usage of > > > > tiles, compression, IPP all have an impact on the final performance). > > > > All we want to know in userspace is if this HW is even possibly capable > > > > of LEVEL X, and if not, we'll look for another one. > > > > > > > > > > Agreed, one could potentially define it this way, but would it be > > > really useful for the userspace and the users? I guess it could enable > > > slightly faster fallback to software decoding in the extreme case of > > > the hardware not supporting the level at all, but I suspect that the > > > majority of cases would be the hardware just being unusably slow. > > > > > > Also, as I mentioned before, we already return the range of supported > > > resolutions, which in practice should map to the part of the level > > > that may depend on hardware capabilities rather than performance, so > > > exposing levels as well would add redundancy to the information > > > exposed. > > > > There is a lot of discussion here, but it all revolves around a potential > > LEVEL control. > > > > So I gather everyone is OK with merging this patch? > > I'm ok with this. For me, the level reflects the real time performance > capability as define in the spec, while the width/height constraints usually > represent an addressing capabicity, which may or may not operate real-time. > I'd like to see the level control documentation improved before we start adding it to the drivers. I'll be okay with that then as long as the values are e
Re: [PATCH] media: hantro: Support H264 profile control
On Sat, Nov 23, 2019 at 1:52 AM Nicolas Dufresne wrote: > > Le samedi 23 novembre 2019 à 01:00 +0900, Tomasz Figa a écrit : > > On Sat, Nov 23, 2019 at 12:09 AM Nicolas Dufresne > > wrote: > > > Le vendredi 22 novembre 2019 à 14:16 +0900, Hirokazu Honda a écrit : > > > > The Hantro G1 decoder supports H.264 profiles from Baseline to High, > > > > with > > > > the exception of the Extended profile. > > > > > > > > Expose the V4L2_CID_MPEG_VIDEO_H264_PROFILE control, so that the > > > > applications can query the driver for the list of supported profiles. > > > > > > Thanks for this patch. Do you think we could also add the LEVEL control > > > so the profile/level enumeration becomes complete ? > > > > > > I'm thinking it would be nice if the v4l2 compliance test make sure > > > that codecs do implement these controls (both stateful and stateless), > > > it's essential for stack with software fallback, or multiple capable > > > codec hardware but with different capabilities. > > > > > > > Level is a difficult story, because it also specifies the number of > > macroblocks per second, but for decoders like this the number of > > macroblocks per second it can handle depends on things the driver > > might be not aware of - clock frequencies, DDR throughput, system > > load, etc. > > > > My take on this is that the decoder driver should advertise the > > highest resolution the decoder can handle due to hardware constraints. > > Performance related things depend on the integration details and > > should be managed elsewhere. For example Android and Chrome OS manage > > expected decoding performance in per-board configuration files. > > When you read datasheet, the HW is always rated to maximum level (and > it's a range) with the assumption of a single stream. It seems much > easier to expose this as-is, statically then to start doing some math > with data that isn't fully exposed to the user. This is about filtering > of multiple CODEC instances, it does not need to be rocket science, > specially that the amount of missing data is important (e.g. usage of > tiles, compression, IPP all have an impact on the final performance). > All we want to know in userspace is if this HW is even possibly capable > of LEVEL X, and if not, we'll look for another one. > Agreed, one could potentially define it this way, but would it be really useful for the userspace and the users? I guess it could enable slightly faster fallback to software decoding in the extreme case of the hardware not supporting the level at all, but I suspect that the majority of cases would be the hardware just being unusably slow. Also, as I mentioned before, we already return the range of supported resolutions, which in practice should map to the part of the level that may depend on hardware capabilities rather than performance, so exposing levels as well would add redundancy to the information exposed. > > > > > > Signed-off-by: Hirokazu Honda > > > > --- > > > > drivers/staging/media/hantro/hantro_drv.c | 10 ++ > > > > 1 file changed, 10 insertions(+) > > > > > > > > diff --git a/drivers/staging/media/hantro/hantro_drv.c > > > > b/drivers/staging/media/hantro/hantro_drv.c > > > > index 6d9d41170832..9387619235d8 100644 > > > > --- a/drivers/staging/media/hantro/hantro_drv.c > > > > +++ b/drivers/staging/media/hantro/hantro_drv.c > > > > @@ -355,6 +355,16 @@ static const struct hantro_ctrl controls[] = { > > > > .def = V4L2_MPEG_VIDEO_H264_START_CODE_ANNEX_B, > > > > .max = V4L2_MPEG_VIDEO_H264_START_CODE_ANNEX_B, > > > > }, > > > > + }, { > > > > + .codec = HANTRO_H264_DECODER, > > > > + .cfg = { > > > > + .id = V4L2_CID_MPEG_VIDEO_H264_PROFILE, > > > > + .min = V4L2_MPEG_VIDEO_H264_PROFILE_BASELINE, > > > > + .max = V4L2_MPEG_VIDEO_H264_PROFILE_HIGH, > > > > + .menu_skip_mask = > > > > + BIT(V4L2_MPEG_VIDEO_H264_PROFILE_EXTENDED), > > > > + .def = V4L2_MPEG_VIDEO_H264_PROFILE_MAIN, > > > > + } > > > > }, { > > > > }, > > > > }; ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] media: hantro: Support H264 profile control
On Sat, Nov 23, 2019 at 12:09 AM Nicolas Dufresne wrote: > > Le vendredi 22 novembre 2019 à 14:16 +0900, Hirokazu Honda a écrit : > > The Hantro G1 decoder supports H.264 profiles from Baseline to High, with > > the exception of the Extended profile. > > > > Expose the V4L2_CID_MPEG_VIDEO_H264_PROFILE control, so that the > > applications can query the driver for the list of supported profiles. > > Thanks for this patch. Do you think we could also add the LEVEL control > so the profile/level enumeration becomes complete ? > > I'm thinking it would be nice if the v4l2 compliance test make sure > that codecs do implement these controls (both stateful and stateless), > it's essential for stack with software fallback, or multiple capable > codec hardware but with different capabilities. > Level is a difficult story, because it also specifies the number of macroblocks per second, but for decoders like this the number of macroblocks per second it can handle depends on things the driver might be not aware of - clock frequencies, DDR throughput, system load, etc. My take on this is that the decoder driver should advertise the highest resolution the decoder can handle due to hardware constraints. Performance related things depend on the integration details and should be managed elsewhere. For example Android and Chrome OS manage expected decoding performance in per-board configuration files. > > > > Signed-off-by: Hirokazu Honda > > --- > > drivers/staging/media/hantro/hantro_drv.c | 10 ++ > > 1 file changed, 10 insertions(+) > > > > diff --git a/drivers/staging/media/hantro/hantro_drv.c > > b/drivers/staging/media/hantro/hantro_drv.c > > index 6d9d41170832..9387619235d8 100644 > > --- a/drivers/staging/media/hantro/hantro_drv.c > > +++ b/drivers/staging/media/hantro/hantro_drv.c > > @@ -355,6 +355,16 @@ static const struct hantro_ctrl controls[] = { > > .def = V4L2_MPEG_VIDEO_H264_START_CODE_ANNEX_B, > > .max = V4L2_MPEG_VIDEO_H264_START_CODE_ANNEX_B, > > }, > > + }, { > > + .codec = HANTRO_H264_DECODER, > > + .cfg = { > > + .id = V4L2_CID_MPEG_VIDEO_H264_PROFILE, > > + .min = V4L2_MPEG_VIDEO_H264_PROFILE_BASELINE, > > + .max = V4L2_MPEG_VIDEO_H264_PROFILE_HIGH, > > + .menu_skip_mask = > > + BIT(V4L2_MPEG_VIDEO_H264_PROFILE_EXTENDED), > > + .def = V4L2_MPEG_VIDEO_H264_PROFILE_MAIN, > > + } > > }, { > > }, > > }; > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: media: hantro: Remove call to memset after dma_alloc_coherent
Hi Hans, On Thu, Jul 25, 2019 at 8:50 PM Boris Brezillon wrote: > > On Thu, 25 Jul 2019 08:36:02 +0530 > Hariprasad Kelam wrote: > > > fix below issue reported by coccicheck > > /drivers/staging/media/hantro/hantro_vp8.c:149:16-34: WARNING: > > dma_alloc_coherent use in aux_buf -> cpu already zeroes out memory, so > > memset is not needed > > > > Signed-off-by: Hariprasad Kelam > > Reviewed-by: Boris Brezillon > > > --- > > drivers/staging/media/hantro/hantro_vp8.c | 2 -- > > 1 file changed, 2 deletions(-) > > > > diff --git a/drivers/staging/media/hantro/hantro_vp8.c > > b/drivers/staging/media/hantro/hantro_vp8.c > > index 66c4533..363ddda 100644 > > --- a/drivers/staging/media/hantro/hantro_vp8.c > > +++ b/drivers/staging/media/hantro/hantro_vp8.c > > @@ -151,8 +151,6 @@ int hantro_vp8_dec_init(struct hantro_ctx *ctx) > > if (!aux_buf->cpu) > > return -ENOMEM; > > > > - memset(aux_buf->cpu, 0, aux_buf->size); > > - > > /* > >* Allocate probability table buffer, > >* total 1208 bytes, 4K page is far enough. > Is this something you will pick to your tree? Best regards, Tomasz ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: media: hantro: Remove call to memset after dma_alloc_coherent
On Mon, Aug 19, 2019 at 1:17 PM Tomasz Figa wrote: > > Hi Hans, > > On Thu, Jul 25, 2019 at 8:50 PM Boris Brezillon > wrote: > > > > On Thu, 25 Jul 2019 08:36:02 +0530 > > Hariprasad Kelam wrote: > > > > > fix below issue reported by coccicheck > > > /drivers/staging/media/hantro/hantro_vp8.c:149:16-34: WARNING: > > > dma_alloc_coherent use in aux_buf -> cpu already zeroes out memory, so > > > memset is not needed > > > > > > Signed-off-by: Hariprasad Kelam > > > > Reviewed-by: Boris Brezillon > > > > > --- > > > drivers/staging/media/hantro/hantro_vp8.c | 2 -- > > > 1 file changed, 2 deletions(-) > > > > > > diff --git a/drivers/staging/media/hantro/hantro_vp8.c > > > b/drivers/staging/media/hantro/hantro_vp8.c > > > index 66c4533..363ddda 100644 > > > --- a/drivers/staging/media/hantro/hantro_vp8.c > > > +++ b/drivers/staging/media/hantro/hantro_vp8.c > > > @@ -151,8 +151,6 @@ int hantro_vp8_dec_init(struct hantro_ctx *ctx) > > > if (!aux_buf->cpu) > > > return -ENOMEM; > > > > > > - memset(aux_buf->cpu, 0, aux_buf->size); > > > - > > > /* > > >* Allocate probability table buffer, > > >* total 1208 bytes, 4K page is far enough. > > > > Is this something you will pick to your tree? Ah, sorry, this is already applied. Not sure why searching for it the first time didn't show anything. I guess I need to start repeating my searches by default. Sorry for the noise. Best regards, Tomasz ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [linux-sunxi] [PATCH v2 1/2] media: v4l: Add definitions for the HEVC slice format and controls
On Wed, Jan 30, 2019 at 3:28 PM Ayaka wrote: > > > > Sent from my iPad > > > On Jan 30, 2019, at 11:35 AM, Tomasz Figa wrote: > > > > On Wed, Jan 30, 2019 at 11:29 AM Alexandre Courbot > > wrote: > >> > >>> On Wed, Jan 30, 2019 at 6:41 AM Nicolas Dufresne > >>> wrote: > >>> > >>>> Le mardi 29 janvier 2019 à 16:44 +0900, Alexandre Courbot a écrit : > >>>> On Fri, Jan 25, 2019 at 10:04 PM Paul Kocialkowski > >>>> wrote: > >>>>> Hi, > >>>>> > >>>>>> On Thu, 2019-01-24 at 20:23 +0800, Ayaka wrote: > >>>>>> Sent from my iPad > >>>>>> > >>>>>>> On Jan 24, 2019, at 6:27 PM, Paul Kocialkowski > >>>>>>> wrote: > >>>>>>> > >>>>>>> Hi, > >>>>>>> > >>>>>>>> On Thu, 2019-01-10 at 21:32 +0800, ayaka wrote: > >>>>>>>> I forget a important thing, for the rkvdec and rk hevc decoder, it > >>>>>>>> would > >>>>>>>> requests cabac table, scaling list, picture parameter set and > >>>>>>>> reference > >>>>>>>> picture storing in one or various of DMA buffers. I am not talking > >>>>>>>> about > >>>>>>>> the data been parsed, the decoder would requests a raw data. > >>>>>>>> > >>>>>>>> For the pps and rps, it is possible to reuse the slice header, just > >>>>>>>> let > >>>>>>>> the decoder know the offset from the bitstream bufer, I would > >>>>>>>> suggest to > >>>>>>>> add three properties(with sps) for them. But I think we need a > >>>>>>>> method to > >>>>>>>> mark a OUTPUT side buffer for those aux data. > >>>>>>> > >>>>>>> I'm quite confused about the hardware implementation then. From what > >>>>>>> you're saying, it seems that it takes the raw bitstream elements > >>>>>>> rather > >>>>>>> than parsed elements. Is it really a stateless implementation? > >>>>>>> > >>>>>>> The stateless implementation was designed with the idea that only the > >>>>>>> raw slice data should be passed in bitstream form to the decoder. For > >>>>>>> H.264, it seems that some decoders also need the slice header in raw > >>>>>>> bitstream form (because they take the full slice NAL unit), see the > >>>>>>> discussions in this thread: > >>>>>>> media: docs-rst: Document m2m stateless video decoder interface > >>>>>> > >>>>>> Stateless just mean it won’t track the previous result, but I don’t > >>>>>> think you can define what a date the hardware would need. Even you > >>>>>> just build a dpb for the decoder, it is still stateless, but parsing > >>>>>> less or more data from the bitstream doesn’t stop a decoder become a > >>>>>> stateless decoder. > >>>>> > >>>>> Yes fair enough, the format in which the hardware decoder takes the > >>>>> bitstream parameters does not make it stateless or stateful per-se. > >>>>> It's just that stateless decoders should have no particular reason for > >>>>> parsing the bitstream on their own since the hardware can be designed > >>>>> with registers for each relevant bitstream element to configure the > >>>>> decoding pipeline. That's how GPU-based decoder implementations are > >>>>> implemented (VAAPI/VDPAU/NVDEC, etc). > >>>>> > >>>>> So the format we have agreed on so far for the stateless interface is > >>>>> to pass parsed elements via v4l2 control structures. > >>>>> > >>>>> If the hardware can only work by parsing the bitstream itself, I'm not > >>>>> sure what the best solution would be. Reconstructing the bitstream in > >>>>> the kernel is a pretty bad option, but so is parsing in the kernel or > >>>>> having the data both in parsed and raw forms. Do you see another > >>>>> possibility? > >>>> >
Re: [linux-sunxi] [PATCH v2 1/2] media: v4l: Add definitions for the HEVC slice format and controls
On Wed, Jan 30, 2019 at 11:29 AM Alexandre Courbot wrote: > > On Wed, Jan 30, 2019 at 6:41 AM Nicolas Dufresne wrote: > > > > Le mardi 29 janvier 2019 à 16:44 +0900, Alexandre Courbot a écrit : > > > On Fri, Jan 25, 2019 at 10:04 PM Paul Kocialkowski > > > wrote: > > > > Hi, > > > > > > > > On Thu, 2019-01-24 at 20:23 +0800, Ayaka wrote: > > > > > Sent from my iPad > > > > > > > > > > > On Jan 24, 2019, at 6:27 PM, Paul Kocialkowski > > > > > > wrote: > > > > > > > > > > > > Hi, > > > > > > > > > > > > > On Thu, 2019-01-10 at 21:32 +0800, ayaka wrote: > > > > > > > I forget a important thing, for the rkvdec and rk hevc decoder, > > > > > > > it would > > > > > > > requests cabac table, scaling list, picture parameter set and > > > > > > > reference > > > > > > > picture storing in one or various of DMA buffers. I am not > > > > > > > talking about > > > > > > > the data been parsed, the decoder would requests a raw data. > > > > > > > > > > > > > > For the pps and rps, it is possible to reuse the slice header, > > > > > > > just let > > > > > > > the decoder know the offset from the bitstream bufer, I would > > > > > > > suggest to > > > > > > > add three properties(with sps) for them. But I think we need a > > > > > > > method to > > > > > > > mark a OUTPUT side buffer for those aux data. > > > > > > > > > > > > I'm quite confused about the hardware implementation then. From what > > > > > > you're saying, it seems that it takes the raw bitstream elements > > > > > > rather > > > > > > than parsed elements. Is it really a stateless implementation? > > > > > > > > > > > > The stateless implementation was designed with the idea that only > > > > > > the > > > > > > raw slice data should be passed in bitstream form to the decoder. > > > > > > For > > > > > > H.264, it seems that some decoders also need the slice header in raw > > > > > > bitstream form (because they take the full slice NAL unit), see the > > > > > > discussions in this thread: > > > > > > media: docs-rst: Document m2m stateless video decoder interface > > > > > > > > > > Stateless just mean it won’t track the previous result, but I don’t > > > > > think you can define what a date the hardware would need. Even you > > > > > just build a dpb for the decoder, it is still stateless, but parsing > > > > > less or more data from the bitstream doesn’t stop a decoder become a > > > > > stateless decoder. > > > > > > > > Yes fair enough, the format in which the hardware decoder takes the > > > > bitstream parameters does not make it stateless or stateful per-se. > > > > It's just that stateless decoders should have no particular reason for > > > > parsing the bitstream on their own since the hardware can be designed > > > > with registers for each relevant bitstream element to configure the > > > > decoding pipeline. That's how GPU-based decoder implementations are > > > > implemented (VAAPI/VDPAU/NVDEC, etc). > > > > > > > > So the format we have agreed on so far for the stateless interface is > > > > to pass parsed elements via v4l2 control structures. > > > > > > > > If the hardware can only work by parsing the bitstream itself, I'm not > > > > sure what the best solution would be. Reconstructing the bitstream in > > > > the kernel is a pretty bad option, but so is parsing in the kernel or > > > > having the data both in parsed and raw forms. Do you see another > > > > possibility? > > > > > > Is reconstructing the bitstream so bad? The v4l2 controls provide a > > > generic interface to an encoded format which the driver needs to > > > convert into a sequence that the hardware can understand. Typically > > > this is done by populating hardware-specific structures. Can't we > > > consider that in this specific instance, the hardware-specific > > > structure just happens to be identical to the original bitstream > > > format? > > > > At maximum allowed bitrate for let's say HEVC (940MB/s iirc), yes, it > > would be really really bad. In GStreamer project we have discussed for > > a while (but have never done anything about) adding the ability through > > a bitmask to select which part of the stream need to be parsed, as > > parsing itself was causing some overhead. Maybe similar thing applies, > > though as per our new design, it's the fourcc that dictate the driver > > behaviour, we'd need yet another fourcc for drivers that wants the full > > bitstream (which seems odd if you have already parsed everything, I > > think this need some clarification). > > Note that I am not proposing to rebuild the *entire* bitstream > in-kernel. What I am saying is that if the hardware interprets some > structures (like SPS/PPS) in their raw format, this raw format could > be reconstructed from the structures passed by userspace at negligible > cost. Such manipulation would only happen on a small amount of data. > > Exposing finer-grained driver requirements through a bitmask may > deserve more exploring. Maybe we could end with a spectrum of >
Re: [linux-sunxi] [PATCH v2 1/2] media: v4l: Add definitions for the HEVC slice format and controls
On Tue, Jan 29, 2019 at 5:09 PM Maxime Ripard wrote: > > On Tue, Jan 29, 2019 at 04:44:35PM +0900, Alexandre Courbot wrote: > > On Fri, Jan 25, 2019 at 10:04 PM Paul Kocialkowski > > > On Thu, 2019-01-24 at 20:23 +0800, Ayaka wrote: > > > > > > > > Sent from my iPad > > > > > > > > > On Jan 24, 2019, at 6:27 PM, Paul Kocialkowski > > > > > wrote: > > > > > > > > > > Hi, > > > > > > > > > > > On Thu, 2019-01-10 at 21:32 +0800, ayaka wrote: > > > > > > I forget a important thing, for the rkvdec and rk hevc decoder, it > > > > > > would > > > > > > requests cabac table, scaling list, picture parameter set and > > > > > > reference > > > > > > picture storing in one or various of DMA buffers. I am not talking > > > > > > about > > > > > > the data been parsed, the decoder would requests a raw data. > > > > > > > > > > > > For the pps and rps, it is possible to reuse the slice header, just > > > > > > let > > > > > > the decoder know the offset from the bitstream bufer, I would > > > > > > suggest to > > > > > > add three properties(with sps) for them. But I think we need a > > > > > > method to > > > > > > mark a OUTPUT side buffer for those aux data. > > > > > > > > > > I'm quite confused about the hardware implementation then. From what > > > > > you're saying, it seems that it takes the raw bitstream elements > > > > > rather > > > > > than parsed elements. Is it really a stateless implementation? > > > > > > > > > > The stateless implementation was designed with the idea that only the > > > > > raw slice data should be passed in bitstream form to the decoder. For > > > > > H.264, it seems that some decoders also need the slice header in raw > > > > > bitstream form (because they take the full slice NAL unit), see the > > > > > discussions in this thread: > > > > > media: docs-rst: Document m2m stateless video decoder interface > > > > > > > > Stateless just mean it won’t track the previous result, but I don’t > > > > think you can define what a date the hardware would need. Even you > > > > just build a dpb for the decoder, it is still stateless, but parsing > > > > less or more data from the bitstream doesn’t stop a decoder become a > > > > stateless decoder. > > > > > > Yes fair enough, the format in which the hardware decoder takes the > > > bitstream parameters does not make it stateless or stateful per-se. > > > It's just that stateless decoders should have no particular reason for > > > parsing the bitstream on their own since the hardware can be designed > > > with registers for each relevant bitstream element to configure the > > > decoding pipeline. That's how GPU-based decoder implementations are > > > implemented (VAAPI/VDPAU/NVDEC, etc). > > > > > > So the format we have agreed on so far for the stateless interface is > > > to pass parsed elements via v4l2 control structures. > > > > > > If the hardware can only work by parsing the bitstream itself, I'm not > > > sure what the best solution would be. Reconstructing the bitstream in > > > the kernel is a pretty bad option, but so is parsing in the kernel or > > > having the data both in parsed and raw forms. Do you see another > > > possibility? > > > > Is reconstructing the bitstream so bad? The v4l2 controls provide a > > generic interface to an encoded format which the driver needs to > > convert into a sequence that the hardware can understand. Typically > > this is done by populating hardware-specific structures. Can't we > > consider that in this specific instance, the hardware-specific > > structure just happens to be identical to the original bitstream > > format? > > > > I agree that this is not strictly optimal for that particular > > hardware, but such is the cost of abstractions, and in this specific > > case I don't believe the cost would be particularly high? > > I mean, that argument can be made for the rockchip driver as well. If > reconstructing the bitstream is something we can do, and if we don't > care about being suboptimal for one particular hardware, then why the > rockchip driver doesn't just recreate the bitstream from that API? > > After all, this is just a hardware specific header that happens to be > identical to the original bitstream format I think in another thread (about H.264 I believe), we realized that it could be a good idea to just include the Slice NAL units in the Annex.B format in the buffers and that should work for all the hardware we could think of (given offsets to particular parts inside of the buffer). Wouldn't something similar work here for HEVC? I don't really get the meaning of "raw" for "cabac table, scaling list, picture parameter set and reference picture", since those are parts of the bitstream, which needs to be parsed to obtain those. Best regards, Tomasz ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/2] media: v4l: Add definitions for the HEVC slice format and controls
Hi Paul, On Tue, Aug 28, 2018 at 5:02 PM Paul Kocialkowski wrote: > > This introduces the required definitions for HEVC decoding support with > stateless VPUs. The controls associated to the HEVC slice format provide > the required meta-data for decoding slices extracted from the bitstream. > Sorry for being late to the party. Please see my comments inline. Only high level, because I don't know too much about HEVC. [snip] > +``V4L2_CID_MPEG_VIDEO_HEVC_SPS (struct)`` > +Specifies the Sequence Parameter Set fields (as extracted from the > +bitstream) for the associated HEVC slice data. > +The bitstream parameters are defined according to the ISO/IEC 23008-2 and > +ITU-T Rec. H.265 specifications. > + > +.. c:type:: v4l2_ctrl_hevc_sps > + > +.. cssclass:: longtable > + > +.. flat-table:: struct v4l2_ctrl_hevc_sps > +:header-rows: 0 > +:stub-columns: 0 > +:widths: 1 1 2 > + > +* - __u8 > + - ``chroma_format_idc`` > + - Syntax description inherited from the specification. I wonder if it wouldn't make sense to instead document this in C code using kernel-doc and then have the kernel-doc included in the sphinx doc. It seems to be possible, according to https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html . Such approach would have the advantage of the person looking through C cross reference being able to actually see the documentation of the struct in question and also making it easier to ensure the relevant C code and documentation are in sync. (Although this is UAPI so it would be unlikely to change too often or at all.) [snip] > +``V4L2_CID_MPEG_VIDEO_HEVC_SLICE_PARAMS (struct)`` > +Specifies various slice-specific parameters, especially from the NAL unit > +header, general slice segment header and weighted prediction parameter > +parts of the bitstream. > +The bitstream parameters are defined according to the ISO/IEC 23008-2 and > +ITU-T Rec. H.265 specifications. In the Chromium H.264 controls, we define this as an array control, so that we can include multiple slices in one buffer and each entry of the array has an offset field pointing to the part of the buffer that contains corresponding slice data. I've mentioned this in the discussion on Alex's stateless decoder interface documentation, so let's keep the discussion there, though. [snip] > @@ -1696,6 +1708,11 @@ static int std_validate(const struct v4l2_ctrl *ctrl, > u32 idx, > case V4L2_CTRL_TYPE_H264_DECODE_PARAMS: > return 0; > > + case V4L2_CTRL_TYPE_HEVC_SPS: > + case V4L2_CTRL_TYPE_HEVC_PPS: > + case V4L2_CTRL_TYPE_HEVC_SLICE_PARAMS: > + return 0; > + I wonder to what extent we should be validating this. I can see 3 options: 1) Defer validation to drivers completely. - Potentially error prone and leading to a lot of code duplication? 2) Validate completely. - Might need solving some interesting problems, e.g. validating reference frame indices in DPB against current state of respective VB2 queue... 3) Validate only what we can easily do, defer more complicated validation to the drivers. - Potentially a good middle ground? Best regards, Tomasz ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v8 4/8] media: platform: Add Cedrus VPU decoder driver
On Thu, Sep 6, 2018 at 4:39 PM Hans Verkuil wrote: > > On 09/06/2018 09:25 AM, Tomasz Figa wrote: > > On Thu, Sep 6, 2018 at 4:01 PM Hans Verkuil wrote: > >> > >> On 09/05/2018 06:29 PM, Paul Kocialkowski wrote: > >>> Hi and thanks for the review! > >>> > >>> Le lundi 03 septembre 2018 à 11:11 +0200, Hans Verkuil a écrit : > >>>> On 08/28/2018 09:34 AM, Paul Kocialkowski wrote: > >>>>> +static int cedrus_request_validate(struct media_request *req) > >>>>> +{ > >>>>> + struct media_request_object *obj, *obj_safe; > >>>>> + struct v4l2_ctrl_handler *parent_hdl, *hdl; > >>>>> + struct cedrus_ctx *ctx = NULL; > >>>>> + struct v4l2_ctrl *ctrl_test; > >>>>> + unsigned int i; > >>>>> + > >>>>> + list_for_each_entry_safe(obj, obj_safe, >objects, list) { > >>>> > >>>> You don't need to use the _safe variant during validation. > >>> > >>> Okay, I'll use the regular one then. > >>> > >>>>> + struct vb2_buffer *vb; > >>>>> + > >>>>> + if (vb2_request_object_is_buffer(obj)) { > >>>>> + vb = container_of(obj, struct vb2_buffer, req_obj); > >>>>> + ctx = vb2_get_drv_priv(vb->vb2_queue); > >>>>> + > >>>>> + break; > >>>>> + } > >>>>> + } > >>>> > >>>> Interesting question: what happens if more than one buffer is queued in > >>>> the > >>>> request? This is allowed by the request API and in that case the > >>>> associated > >>>> controls in the request apply to all queued buffers. > >>>> > >>>> Would this make sense at all for this driver? If not, then you need to > >>>> check here if there is more than one buffer in the request and document > >>>> in > >>>> the spec that this is not allowed. > >>> > >>> Well, our driver was written with the (unformal) assumption that we > >>> only deal with a pair of one output and one capture buffer. So I will > >>> add a check for this at request validation time and document it in the > >>> spec. Should that be part of the MPEG-2 PIXFMT documentation (and > >>> duplicated for future formats we add support for)? > >> > >> Can you make a patch for vb2_request_has_buffers() in videobuf2-core.c > >> renaming it to vb2_request_buffer_cnt() and returning the number of buffers > >> in the request? > >> > >> Then you can call it here to check that you have only one buffer. > >> > >> And this has to be documented with the PIXFMT. > >> > >> Multiple buffers are certainly possible in non-codec scenarios (vim2m and > >> vivid happily accept that), so this is an exception that should be > >> documented and checked in the codec driver. > > > > Hmm, isn't it still 1 buffer per 1 queue and just multiple queues > > included in the request? > > No. The request API allows multiple buffers for the same vb2_queue to be > queued for the same request (obviously when the request is committed, the > buffers are queued to the driver in the same order). > > > > > If we indeed allow multiple buffers for the same queue in a request, > > we shouldn't restrict this on a per-driver basis. It's definitely not > > a hardware limitation, since the driver could just do the same as if 2 > > requests with the same controls were given. > > That's how it operates: for all buffers in the request the same controls > apply. But does this make sense for codecs? If the control(s) with the > codec metadata always change for every buffer, then having more than one > buffer in the request is senseless and the driver should check for this > in the validation step. > > If it *does* make sense in some circumstances to have the same metadata > for multiple buffers, then it should be checked if the cedrus driver > handles this correctly. Just FYI, we may want to move this discussion to Alex's RFC with documentation of stateless interface: https://patchwork.kernel.org/patch/10583233/ Best regards, Tomasz ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v8 4/8] media: platform: Add Cedrus VPU decoder driver
On Thu, Sep 6, 2018 at 4:01 PM Hans Verkuil wrote: > > On 09/05/2018 06:29 PM, Paul Kocialkowski wrote: > > Hi and thanks for the review! > > > > Le lundi 03 septembre 2018 à 11:11 +0200, Hans Verkuil a écrit : > >> On 08/28/2018 09:34 AM, Paul Kocialkowski wrote: > >>> +static int cedrus_request_validate(struct media_request *req) > >>> +{ > >>> + struct media_request_object *obj, *obj_safe; > >>> + struct v4l2_ctrl_handler *parent_hdl, *hdl; > >>> + struct cedrus_ctx *ctx = NULL; > >>> + struct v4l2_ctrl *ctrl_test; > >>> + unsigned int i; > >>> + > >>> + list_for_each_entry_safe(obj, obj_safe, >objects, list) { > >> > >> You don't need to use the _safe variant during validation. > > > > Okay, I'll use the regular one then. > > > >>> + struct vb2_buffer *vb; > >>> + > >>> + if (vb2_request_object_is_buffer(obj)) { > >>> + vb = container_of(obj, struct vb2_buffer, req_obj); > >>> + ctx = vb2_get_drv_priv(vb->vb2_queue); > >>> + > >>> + break; > >>> + } > >>> + } > >> > >> Interesting question: what happens if more than one buffer is queued in the > >> request? This is allowed by the request API and in that case the associated > >> controls in the request apply to all queued buffers. > >> > >> Would this make sense at all for this driver? If not, then you need to > >> check here if there is more than one buffer in the request and document in > >> the spec that this is not allowed. > > > > Well, our driver was written with the (unformal) assumption that we > > only deal with a pair of one output and one capture buffer. So I will > > add a check for this at request validation time and document it in the > > spec. Should that be part of the MPEG-2 PIXFMT documentation (and > > duplicated for future formats we add support for)? > > Can you make a patch for vb2_request_has_buffers() in videobuf2-core.c > renaming it to vb2_request_buffer_cnt() and returning the number of buffers > in the request? > > Then you can call it here to check that you have only one buffer. > > And this has to be documented with the PIXFMT. > > Multiple buffers are certainly possible in non-codec scenarios (vim2m and > vivid happily accept that), so this is an exception that should be > documented and checked in the codec driver. Hmm, isn't it still 1 buffer per 1 queue and just multiple queues included in the request? If we indeed allow multiple buffers for the same queue in a request, we shouldn't restrict this on a per-driver basis. It's definitely not a hardware limitation, since the driver could just do the same as if 2 requests with the same controls were given. Best regards, Tomasz ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v6 4/8] media: platform: Add Cedrus VPU decoder driver
On Tue, Aug 7, 2018 at 4:20 PM Paul Kocialkowski wrote: > > Hi, > > On Mon, 2018-08-06 at 23:10 +0900, Tomasz Figa wrote: > > Hi Paul, > > > > On Mon, Aug 6, 2018 at 10:50 PM Paul Kocialkowski > > wrote: > > > > > > Hi Hans and thanks for the review! > > > > > > On Sat, 2018-08-04 at 14:18 +0200, Hans Verkuil wrote: > > > > Hi Paul, > > > > > > > > See below for my review comments. Mostly small fry, the main issue I > > > > found is > > > > that there is no support for VIDIOC_DECODER_CMD. That's the proper way > > > > of > > > > stopping a decoder. Don't rely on the deprecated allow_zero_bytesused > > > > field. > > > > > > Mhh, it looks like this was kept around by negligence, but we do expect > > > that streamoff stops the decoder, not a zero bytesused field. > > > > > > Is it still required to implement the V4L2_DEC_CMD_STOP > > > VIDIOC_DECODER_CMD in that case? I read in the doc that this ioctl > > > should be optional. > > > > If I understand correctly that this decoder is stateless, there should > > be no need for any special flush sequence, since a 1:1 relation > > between OUTPUT and CAPTURE buffers is expected, which means that > > userspace can just stop queuing new OUTPUT buffers and keep dequeuing > > CAPTURE buffers until it matches all OUTPUT buffers queued before. > > This is indeed a stateless decoder and I don't have any particular need > for a particular stop command indeed, since flushing remaining buffers > when stopping is already implemented at streamoff time. > Do you mean implemented in user space or the driver? Obviously the latter is against the API specification, since VIDIOC_STREAMOFF is expected to instantly stop any pending hardware operations and gracefully discard any queued buffers or processing results. > > By the way, I guess we will also need some documentation for the > > stateless codec interface. Do you or Maxime (who sent the H264 part) > > have any plans to work on it? We have some internal documents, which > > should be convertible to rst using pandoc, but we might need some help > > with updating to latest request API and further editing. Alexandre > > (moved from Cc to To) is going to be looking into this. > > As far as I'm concerned, I am interested in contributing to this > documentation although our priorities for the Allwinner VPU effort are > currently focused on H265 support. This might mean that my contributions > to this documentation will be made on a best-effort basis (as opposed to > during the workday). Either way, if someone was to come up with an > initial draft, I'd be happy to review it! I've talked with Alex and he should be able to convert our internal document and post it as the initial draft RFC. Help with review will be definitely appreciated, thanks! Note that we shouldn't repeat the same mistake as with stateful codecs and allow merging drivers without the API being specified. That led to drivers doing this their own ways and having to account for those quirks in the stateful codec API specification we're working on right now. Best regards, Tomasz ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [linux-sunxi] [PATCH v6 4/8] media: platform: Add Cedrus VPU decoder driver
On Wed, Aug 8, 2018 at 12:05 AM Jernej Škrabec wrote: > > Dne torek, 07. avgust 2018 ob 14:31:03 CEST je Paul Kocialkowski napisal(a): > > Hi, > > > > On Fri, 2018-07-27 at 16:58 +0200, Jernej Škrabec wrote: > > > Dne petek, 27. julij 2018 ob 16:03:41 CEST je Jernej Škrabec napisal(a): > > > > Hi! > > > > > > > > Dne sreda, 25. julij 2018 ob 12:02:52 CEST je Paul Kocialkowski > napisal(a): > > > > > This introduces the Cedrus VPU driver that supports the VPU found in > > > > > Allwinner SoCs, also known as Video Engine. It is implemented through > > > > > a v4l2 m2m decoder device and a media device (used for media > > > > > requests). > > > > > So far, it only supports MPEG2 decoding. > > > > > > > > > > Since this VPU is stateless, synchronization with media requests is > > > > > required in order to ensure consistency between frame headers that > > > > > contain metadata about the frame to process and the raw slice data > > > > > that > > > > > is used to generate the frame. > > > > > > > > > > This driver was made possible thanks to the long-standing effort > > > > > carried out by the linux-sunxi community in the interest of reverse > > > > > engineering, documenting and implementing support for Allwinner VPU. > > > > > > > > > > Signed-off-by: Paul Kocialkowski > > > > > --- > > > > > > > > > > > > > > > > > +void cedrus_dst_format_set(struct cedrus_dev *dev, > > > > > + struct v4l2_pix_format_mplane *fmt) > > > > > +{ > > > > > + unsigned int width = fmt->width; > > > > > + unsigned int height = fmt->height; > > > > > + u32 chroma_size; > > > > > + u32 reg; > > > > > + > > > > > + switch (fmt->pixelformat) { > > > > > + case V4L2_PIX_FMT_NV12: > > > > > + chroma_size = ALIGN(width, 32) * ALIGN(height / 2, > > > > > 32); > > > > > > > > After some testing, it turns out that right aligment for untiled format > > > > is > > > > 16. > > > > > > > > > + > > > > > + reg = VE_PRIMARY_OUT_FMT_NV12 | > > > > > + VE_SECONDARY_SPECIAL_OUT_FMT_NV12; > > > > > + cedrus_write(dev, VE_PRIMARY_OUT_FMT, reg); > > > > > + > > > > > + reg = VE_CHROMA_BUF_LEN_SDRT(chroma_size / 2) | > > > > > + VE_SECONDARY_OUT_FMT_SPECIAL; > > > > > + cedrus_write(dev, VE_CHROMA_BUF_LEN, reg); > > > > > + > > > > > + reg = chroma_size / 2; > > > > > + cedrus_write(dev, VE_PRIMARY_CHROMA_BUF_LEN, reg); > > > > > + > > > > > + reg = VE_PRIMARY_FB_LINE_STRIDE_LUMA(ALIGN(width, > > > > > 32)) | > > > > > > > > ^ that one should be aligned to 16 > > > > > > > > > + VE_PRIMARY_FB_LINE_STRIDE_CHROMA(ALIGN(width / > > > > > 2, 16)); > > > > > > It seems that CHROMA has to be aligned to 8 ^ > > > > I think the issue here is that the divider should be applied after the > > alignment, not before, such as: ALIGN(width, 16) / 2, which also > > provides a 8-aligned value. > > > > Feel free to let me know if that causes any particular issue! > > I think this is only semantics, it doesn't really matter if it is aligned to > 16 first and then divided by 2 or divided by 2 and then aligned to 8. It depends if |width| is always expected to be aligned to 2. For example, given |width| = 17, ALIGN(17, 16) = 32, 32 / 2 = 16 17 / 2 = 8, ALIGN(8, 8) = 8 Best regards, Tomasz ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v6 4/8] media: platform: Add Cedrus VPU decoder driver
Hi Paul, On Mon, Aug 6, 2018 at 10:50 PM Paul Kocialkowski wrote: > > Hi Hans and thanks for the review! > > On Sat, 2018-08-04 at 14:18 +0200, Hans Verkuil wrote: > > Hi Paul, > > > > See below for my review comments. Mostly small fry, the main issue I found > > is > > that there is no support for VIDIOC_DECODER_CMD. That's the proper way of > > stopping a decoder. Don't rely on the deprecated allow_zero_bytesused field. > > Mhh, it looks like this was kept around by negligence, but we do expect > that streamoff stops the decoder, not a zero bytesused field. > > Is it still required to implement the V4L2_DEC_CMD_STOP > VIDIOC_DECODER_CMD in that case? I read in the doc that this ioctl > should be optional. If I understand correctly that this decoder is stateless, there should be no need for any special flush sequence, since a 1:1 relation between OUTPUT and CAPTURE buffers is expected, which means that userspace can just stop queuing new OUTPUT buffers and keep dequeuing CAPTURE buffers until it matches all OUTPUT buffers queued before. By the way, I guess we will also need some documentation for the stateless codec interface. Do you or Maxime (who sent the H264 part) have any plans to work on it? We have some internal documents, which should be convertible to rst using pandoc, but we might need some help with updating to latest request API and further editing. Alexandre (moved from Cc to To) is going to be looking into this. Best regards, Tomasz ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel