* [PATCH v2 0/3] media: cedrus: improvements
@ 2019-10-02 19:35 Jernej Skrabec
2019-10-02 19:35 ` [PATCH v2 1/3] media: cedrus: Fix decoding for some H264 videos Jernej Skrabec
` (3 more replies)
0 siblings, 4 replies; 17+ messages in thread
From: Jernej Skrabec @ 2019-10-02 19:35 UTC (permalink / raw)
To: mripard, paul.kocialkowski
Cc: mchehab, hverkuil-cisco, gregkh, wens, linux-media, devel,
linux-arm-kernel, linux-kernel
This is continuation of https://lkml.org/lkml/2019/5/30/1459 with several
patches removed (2 merged, others needs redesign) and one added.
Patch 1 fixes h264 playback issue which happens in rare cases.
Patch 2 sets PPS default reference index count in register from PPS
control. Currently it was set to values from slice control.
Patch 3 replaces direct accesses to capture queue from m2m contex with
helpers which was designed exactly for that. It's also safer since
helpers do some checks.
Best regards,
Jernej
Jernej Skrabec (3):
media: cedrus: Fix decoding for some H264 videos
media: cedrus: Fix H264 default reference index count
media: cedrus: Use helpers to access capture queue
drivers/staging/media/sunxi/cedrus/cedrus.h | 8 +++-
.../staging/media/sunxi/cedrus/cedrus_h264.c | 46 ++++++++++++++-----
.../staging/media/sunxi/cedrus/cedrus_regs.h | 3 ++
3 files changed, 44 insertions(+), 13 deletions(-)
--
2.23.0
^ permalink raw reply [flat|nested] 17+ messages in thread* [PATCH v2 1/3] media: cedrus: Fix decoding for some H264 videos 2019-10-02 19:35 [PATCH v2 0/3] media: cedrus: improvements Jernej Skrabec @ 2019-10-02 19:35 ` Jernej Skrabec 2019-10-02 21:54 ` Paul Kocialkowski 2019-10-02 19:35 ` [PATCH v2 2/3] media: cedrus: Fix H264 default reference index count Jernej Skrabec ` (2 subsequent siblings) 3 siblings, 1 reply; 17+ messages in thread From: Jernej Skrabec @ 2019-10-02 19:35 UTC (permalink / raw) To: mripard, paul.kocialkowski Cc: mchehab, hverkuil-cisco, gregkh, wens, linux-media, devel, linux-arm-kernel, linux-kernel It seems that for some H264 videos at least one bitstream parsing trigger must be called in order to be decoded correctly. There is no explanation why this helps, but it was observed that two sample videos with this fix are now decoded correctly and there is no regression with others. Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> --- .../staging/media/sunxi/cedrus/cedrus_h264.c | 30 +++++++++++++++++-- .../staging/media/sunxi/cedrus/cedrus_regs.h | 3 ++ 2 files changed, 30 insertions(+), 3 deletions(-) diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c index d6a782703c9b..bd848146eada 100644 --- a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c @@ -6,6 +6,7 @@ * Copyright (c) 2018 Bootlin */ +#include <linux/delay.h> #include <linux/types.h> #include <media/videobuf2-dma-contig.h> @@ -289,6 +290,28 @@ static void cedrus_write_pred_weight_table(struct cedrus_ctx *ctx, } } +/* + * It turns out that using VE_H264_VLD_OFFSET to skip bits is not reliable. In + * rare cases frame is not decoded correctly. However, setting offset to 0 and + * skipping appropriate amount of bits with flush bits trigger always works. + */ +static void cedrus_skip_bits(struct cedrus_dev *dev, int num) +{ + int count = 0; + + while (count < num) { + int tmp = min(num - count, 32); + + cedrus_write(dev, VE_H264_TRIGGER_TYPE, + VE_H264_TRIGGER_TYPE_FLUSH_BITS | + VE_H264_TRIGGER_TYPE_N_BITS(tmp)); + while (cedrus_read(dev, VE_H264_STATUS) & VE_H264_STATUS_VLD_BUSY) + udelay(1); + + count += tmp; + } +} + static void cedrus_set_params(struct cedrus_ctx *ctx, struct cedrus_run *run) { @@ -299,12 +322,11 @@ static void cedrus_set_params(struct cedrus_ctx *ctx, struct vb2_buffer *src_buf = &run->src->vb2_buf; struct cedrus_dev *dev = ctx->dev; dma_addr_t src_buf_addr; - u32 offset = slice->header_bit_size; - u32 len = (slice->size * 8) - offset; + u32 len = slice->size * 8; u32 reg; cedrus_write(dev, VE_H264_VLD_LEN, len); - cedrus_write(dev, VE_H264_VLD_OFFSET, offset); + cedrus_write(dev, VE_H264_VLD_OFFSET, 0); src_buf_addr = vb2_dma_contig_plane_dma_addr(src_buf, 0); cedrus_write(dev, VE_H264_VLD_END, @@ -323,6 +345,8 @@ static void cedrus_set_params(struct cedrus_ctx *ctx, cedrus_write(dev, VE_H264_TRIGGER_TYPE, VE_H264_TRIGGER_TYPE_INIT_SWDEC); + cedrus_skip_bits(dev, slice->header_bit_size); + if (((pps->flags & V4L2_H264_PPS_FLAG_WEIGHTED_PRED) && (slice->slice_type == V4L2_H264_SLICE_TYPE_P || slice->slice_type == V4L2_H264_SLICE_TYPE_SP)) || diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_regs.h b/drivers/staging/media/sunxi/cedrus/cedrus_regs.h index 3329f9aaf975..b52926a54025 100644 --- a/drivers/staging/media/sunxi/cedrus/cedrus_regs.h +++ b/drivers/staging/media/sunxi/cedrus/cedrus_regs.h @@ -538,13 +538,16 @@ VE_H264_CTRL_SLICE_DECODE_INT) #define VE_H264_TRIGGER_TYPE 0x224 +#define VE_H264_TRIGGER_TYPE_N_BITS(x) (((x) & 0x3f) << 8) #define VE_H264_TRIGGER_TYPE_AVC_SLICE_DECODE (8 << 0) #define VE_H264_TRIGGER_TYPE_INIT_SWDEC (7 << 0) +#define VE_H264_TRIGGER_TYPE_FLUSH_BITS (3 << 0) #define VE_H264_STATUS 0x228 #define VE_H264_STATUS_VLD_DATA_REQ_INT VE_H264_CTRL_VLD_DATA_REQ_INT #define VE_H264_STATUS_DECODE_ERR_INT VE_H264_CTRL_DECODE_ERR_INT #define VE_H264_STATUS_SLICE_DECODE_INT VE_H264_CTRL_SLICE_DECODE_INT +#define VE_H264_STATUS_VLD_BUSY BIT(8) #define VE_H264_STATUS_INT_MASK VE_H264_CTRL_INT_MASK -- 2.23.0 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v2 1/3] media: cedrus: Fix decoding for some H264 videos 2019-10-02 19:35 ` [PATCH v2 1/3] media: cedrus: Fix decoding for some H264 videos Jernej Skrabec @ 2019-10-02 21:54 ` Paul Kocialkowski 2019-10-15 17:16 ` Jernej Škrabec 0 siblings, 1 reply; 17+ messages in thread From: Paul Kocialkowski @ 2019-10-02 21:54 UTC (permalink / raw) To: Jernej Skrabec Cc: mripard, mchehab, hverkuil-cisco, gregkh, wens, linux-media, devel, linux-arm-kernel, linux-kernel [-- Attachment #1: Type: text/plain, Size: 4588 bytes --] Hi, On Wed 02 Oct 19, 21:35, Jernej Skrabec wrote: > It seems that for some H264 videos at least one bitstream parsing > trigger must be called in order to be decoded correctly. There is no > explanation why this helps, but it was observed that two sample videos > with this fix are now decoded correctly and there is no regression with > others. I understand there might be some magic going on under the hood here, but I would be interested in trying to understand what's really going on. For instance, comparing register dumps of the whole H264 block before/after calling the hardware parser could help, and comparing that to using the previous code (without hardware parsing). I could try and have a look if you have an available sample for testing the erroneous case! Another minor thing: do you have some idea of whether the udelay call adds significant delay in the process? Cheers and thanks for the patch! Paul > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > --- > .../staging/media/sunxi/cedrus/cedrus_h264.c | 30 +++++++++++++++++-- > .../staging/media/sunxi/cedrus/cedrus_regs.h | 3 ++ > 2 files changed, 30 insertions(+), 3 deletions(-) > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c > index d6a782703c9b..bd848146eada 100644 > --- a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c > @@ -6,6 +6,7 @@ > * Copyright (c) 2018 Bootlin > */ > > +#include <linux/delay.h> > #include <linux/types.h> > > #include <media/videobuf2-dma-contig.h> > @@ -289,6 +290,28 @@ static void cedrus_write_pred_weight_table(struct cedrus_ctx *ctx, > } > } > > +/* > + * It turns out that using VE_H264_VLD_OFFSET to skip bits is not reliable. In > + * rare cases frame is not decoded correctly. However, setting offset to 0 and > + * skipping appropriate amount of bits with flush bits trigger always works. > + */ > +static void cedrus_skip_bits(struct cedrus_dev *dev, int num) > +{ > + int count = 0; > + > + while (count < num) { > + int tmp = min(num - count, 32); > > + > + cedrus_write(dev, VE_H264_TRIGGER_TYPE, > + VE_H264_TRIGGER_TYPE_FLUSH_BITS | > + VE_H264_TRIGGER_TYPE_N_BITS(tmp)); > + while (cedrus_read(dev, VE_H264_STATUS) & VE_H264_STATUS_VLD_BUSY) > + udelay(1); > + > + count += tmp; > + } > +} > + > static void cedrus_set_params(struct cedrus_ctx *ctx, > struct cedrus_run *run) > { > @@ -299,12 +322,11 @@ static void cedrus_set_params(struct cedrus_ctx *ctx, > struct vb2_buffer *src_buf = &run->src->vb2_buf; > struct cedrus_dev *dev = ctx->dev; > dma_addr_t src_buf_addr; > - u32 offset = slice->header_bit_size; > - u32 len = (slice->size * 8) - offset; > + u32 len = slice->size * 8; > u32 reg; > > cedrus_write(dev, VE_H264_VLD_LEN, len); > - cedrus_write(dev, VE_H264_VLD_OFFSET, offset); > + cedrus_write(dev, VE_H264_VLD_OFFSET, 0); > > src_buf_addr = vb2_dma_contig_plane_dma_addr(src_buf, 0); > cedrus_write(dev, VE_H264_VLD_END, > @@ -323,6 +345,8 @@ static void cedrus_set_params(struct cedrus_ctx *ctx, > cedrus_write(dev, VE_H264_TRIGGER_TYPE, > VE_H264_TRIGGER_TYPE_INIT_SWDEC); > > + cedrus_skip_bits(dev, slice->header_bit_size); > + > if (((pps->flags & V4L2_H264_PPS_FLAG_WEIGHTED_PRED) && > (slice->slice_type == V4L2_H264_SLICE_TYPE_P || > slice->slice_type == V4L2_H264_SLICE_TYPE_SP)) || > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_regs.h b/drivers/staging/media/sunxi/cedrus/cedrus_regs.h > index 3329f9aaf975..b52926a54025 100644 > --- a/drivers/staging/media/sunxi/cedrus/cedrus_regs.h > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_regs.h > @@ -538,13 +538,16 @@ > VE_H264_CTRL_SLICE_DECODE_INT) > > #define VE_H264_TRIGGER_TYPE 0x224 > +#define VE_H264_TRIGGER_TYPE_N_BITS(x) (((x) & 0x3f) << 8) > #define VE_H264_TRIGGER_TYPE_AVC_SLICE_DECODE (8 << 0) > #define VE_H264_TRIGGER_TYPE_INIT_SWDEC (7 << 0) > +#define VE_H264_TRIGGER_TYPE_FLUSH_BITS (3 << 0) > > #define VE_H264_STATUS 0x228 > #define VE_H264_STATUS_VLD_DATA_REQ_INT VE_H264_CTRL_VLD_DATA_REQ_INT > #define VE_H264_STATUS_DECODE_ERR_INT VE_H264_CTRL_DECODE_ERR_INT > #define VE_H264_STATUS_SLICE_DECODE_INT VE_H264_CTRL_SLICE_DECODE_INT > +#define VE_H264_STATUS_VLD_BUSY BIT(8) > > #define VE_H264_STATUS_INT_MASK VE_H264_CTRL_INT_MASK > > -- > 2.23.0 > [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 1/3] media: cedrus: Fix decoding for some H264 videos 2019-10-02 21:54 ` Paul Kocialkowski @ 2019-10-15 17:16 ` Jernej Škrabec 2019-10-22 9:10 ` Paul Kocialkowski 0 siblings, 1 reply; 17+ messages in thread From: Jernej Škrabec @ 2019-10-15 17:16 UTC (permalink / raw) To: Paul Kocialkowski Cc: mripard, mchehab, hverkuil-cisco, gregkh, wens, linux-media, devel, linux-arm-kernel, linux-kernel Hi! Sorry for late reponse, technical issues... Dne sreda, 02. oktober 2019 ob 23:54:47 CEST je Paul Kocialkowski napisal(a): > Hi, > > On Wed 02 Oct 19, 21:35, Jernej Skrabec wrote: > > It seems that for some H264 videos at least one bitstream parsing > > trigger must be called in order to be decoded correctly. There is no > > explanation why this helps, but it was observed that two sample videos > > with this fix are now decoded correctly and there is no regression with > > others. > > I understand there might be some magic going on under the hood here, but I > would be interested in trying to understand what's really going on. Me too. > > For instance, comparing register dumps of the whole H264 block before/after > calling the hardware parser could help, and comparing that to using the > previous code (without hardware parsing). Please understand that I was working on this on and off for almost half a year and checked many times all register values. At one point I tried libvdpau- sunxi which has no problem with sample video. Still, all relevant register values were the same. In a desperate attempt, I tried with HW header parsing which magically solved the issue. After that, I reused values provided in controls and then finally I made minimal solution as suggested in this patch. > > I could try and have a look if you have an available sample for testing the > erroneous case! Of course: http://jernej.libreelec.tv/videos/h264/test.mkv > > Another minor thing: do you have some idea of whether the udelay call adds > significant delay in the process? I didn't notice any issue with it. Do you have any better idea? I just didn't want to make empty loop and udelay is the shortest delay that is provided by the kernel API. Best regards, Jernej > > Cheers and thanks for the patch! > > Paul > > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > > --- > > > > .../staging/media/sunxi/cedrus/cedrus_h264.c | 30 +++++++++++++++++-- > > .../staging/media/sunxi/cedrus/cedrus_regs.h | 3 ++ > > 2 files changed, 30 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c > > b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c index > > d6a782703c9b..bd848146eada 100644 > > --- a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c > > @@ -6,6 +6,7 @@ > > > > * Copyright (c) 2018 Bootlin > > */ > > > > +#include <linux/delay.h> > > > > #include <linux/types.h> > > > > #include <media/videobuf2-dma-contig.h> > > > > @@ -289,6 +290,28 @@ static void cedrus_write_pred_weight_table(struct > > cedrus_ctx *ctx,> > > } > > > > } > > > > +/* > > + * It turns out that using VE_H264_VLD_OFFSET to skip bits is not > > reliable. In + * rare cases frame is not decoded correctly. However, > > setting offset to 0 and + * skipping appropriate amount of bits with > > flush bits trigger always works. + */ > > +static void cedrus_skip_bits(struct cedrus_dev *dev, int num) > > +{ > > + int count = 0; > > + > > + while (count < num) { > > + int tmp = min(num - count, 32); > > > > + > > + cedrus_write(dev, VE_H264_TRIGGER_TYPE, > > + VE_H264_TRIGGER_TYPE_FLUSH_BITS | > > + VE_H264_TRIGGER_TYPE_N_BITS(tmp)); > > + while (cedrus_read(dev, VE_H264_STATUS) & VE_H264_STATUS_VLD_BUSY) > > + udelay(1); > > + > > + count += tmp; > > + } > > +} > > + > > > > static void cedrus_set_params(struct cedrus_ctx *ctx, > > > > struct cedrus_run *run) > > > > { > > > > @@ -299,12 +322,11 @@ static void cedrus_set_params(struct cedrus_ctx > > *ctx, > > > > struct vb2_buffer *src_buf = &run->src->vb2_buf; > > struct cedrus_dev *dev = ctx->dev; > > dma_addr_t src_buf_addr; > > > > - u32 offset = slice->header_bit_size; > > - u32 len = (slice->size * 8) - offset; > > + u32 len = slice->size * 8; > > > > u32 reg; > > > > cedrus_write(dev, VE_H264_VLD_LEN, len); > > > > - cedrus_write(dev, VE_H264_VLD_OFFSET, offset); > > + cedrus_write(dev, VE_H264_VLD_OFFSET, 0); > > > > src_buf_addr = vb2_dma_contig_plane_dma_addr(src_buf, 0); > > cedrus_write(dev, VE_H264_VLD_END, > > > > @@ -323,6 +345,8 @@ static void cedrus_set_params(struct cedrus_ctx *ctx, > > > > cedrus_write(dev, VE_H264_TRIGGER_TYPE, > > > > VE_H264_TRIGGER_TYPE_INIT_SWDEC); > > > > + cedrus_skip_bits(dev, slice->header_bit_size); > > + > > > > if (((pps->flags & V4L2_H264_PPS_FLAG_WEIGHTED_PRED) && > > > > (slice->slice_type == V4L2_H264_SLICE_TYPE_P || > > > > slice->slice_type == V4L2_H264_SLICE_TYPE_SP)) || > > > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_regs.h > > b/drivers/staging/media/sunxi/cedrus/cedrus_regs.h index > > 3329f9aaf975..b52926a54025 100644 > > --- a/drivers/staging/media/sunxi/cedrus/cedrus_regs.h > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_regs.h > > @@ -538,13 +538,16 @@ > > > > VE_H264_CTRL_SLICE_DECODE_INT) > > > > #define VE_H264_TRIGGER_TYPE 0x224 > > > > +#define VE_H264_TRIGGER_TYPE_N_BITS(x) (((x) & 0x3f) << 8) > > > > #define VE_H264_TRIGGER_TYPE_AVC_SLICE_DECODE (8 << 0) > > #define VE_H264_TRIGGER_TYPE_INIT_SWDEC (7 << 0) > > > > +#define VE_H264_TRIGGER_TYPE_FLUSH_BITS (3 << 0) > > > > #define VE_H264_STATUS 0x228 > > #define VE_H264_STATUS_VLD_DATA_REQ_INT VE_H264_CTRL_VLD_DATA_REQ_INT > > #define VE_H264_STATUS_DECODE_ERR_INT VE_H264_CTRL_DECODE_ERR_INT > > #define VE_H264_STATUS_SLICE_DECODE_INT VE_H264_CTRL_SLICE_DECODE_INT > > > > +#define VE_H264_STATUS_VLD_BUSY BIT(8) > > > > #define VE_H264_STATUS_INT_MASK VE_H264_CTRL_INT_MASK ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 1/3] media: cedrus: Fix decoding for some H264 videos 2019-10-15 17:16 ` Jernej Škrabec @ 2019-10-22 9:10 ` Paul Kocialkowski 0 siblings, 0 replies; 17+ messages in thread From: Paul Kocialkowski @ 2019-10-22 9:10 UTC (permalink / raw) To: Jernej Škrabec Cc: mripard, mchehab, hverkuil-cisco, gregkh, wens, linux-media, devel, linux-arm-kernel, linux-kernel [-- Attachment #1: Type: text/plain, Size: 5877 bytes --] Hi, On Tue 15 Oct 19, 19:16, Jernej Škrabec wrote: > Please understand that I was working on this on and off for almost half a year > and checked many times all register values. At one point I tried libvdpau- > sunxi which has no problem with sample video. Still, all relevant register > values were the same. In a desperate attempt, I tried with HW header parsing > which magically solved the issue. After that, I reused values provided in > controls and then finally I made minimal solution as suggested in this patch. Okay thanks for the details. I think I've delayed this for far too long already so I think we should get it in without further delay. The patch apparently no longer applies on top of media/master, but feel free to send out a rebased series with: Acked-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com> Let's leave out 2/3 though, I think I will submit a series adding the flag as indication for the per-slice value in the uAPI and use it in cedrus. Cheers, Paul > > > > I could try and have a look if you have an available sample for testing the > > erroneous case! > > Of course: http://jernej.libreelec.tv/videos/h264/test.mkv > > > > > Another minor thing: do you have some idea of whether the udelay call adds > > significant delay in the process? > > I didn't notice any issue with it. Do you have any better idea? I just didn't > want to make empty loop and udelay is the shortest delay that is provided by > the kernel API. > > Best regards, > Jernej > > > > > Cheers and thanks for the patch! > > > > Paul > > > > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > > > --- > > > > > > .../staging/media/sunxi/cedrus/cedrus_h264.c | 30 +++++++++++++++++-- > > > .../staging/media/sunxi/cedrus/cedrus_regs.h | 3 ++ > > > 2 files changed, 30 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c > > > b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c index > > > d6a782703c9b..bd848146eada 100644 > > > --- a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c > > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c > > > @@ -6,6 +6,7 @@ > > > > > > * Copyright (c) 2018 Bootlin > > > */ > > > > > > +#include <linux/delay.h> > > > > > > #include <linux/types.h> > > > > > > #include <media/videobuf2-dma-contig.h> > > > > > > @@ -289,6 +290,28 @@ static void cedrus_write_pred_weight_table(struct > > > cedrus_ctx *ctx,> > > > } > > > > > > } > > > > > > +/* > > > + * It turns out that using VE_H264_VLD_OFFSET to skip bits is not > > > reliable. In + * rare cases frame is not decoded correctly. However, > > > setting offset to 0 and + * skipping appropriate amount of bits with > > > flush bits trigger always works. + */ > > > +static void cedrus_skip_bits(struct cedrus_dev *dev, int num) > > > +{ > > > + int count = 0; > > > + > > > + while (count < num) { > > > + int tmp = min(num - count, 32); > > > > > > + > > > + cedrus_write(dev, VE_H264_TRIGGER_TYPE, > > > + VE_H264_TRIGGER_TYPE_FLUSH_BITS | > > > + VE_H264_TRIGGER_TYPE_N_BITS(tmp)); > > > + while (cedrus_read(dev, VE_H264_STATUS) & > VE_H264_STATUS_VLD_BUSY) > > > + udelay(1); > > > + > > > + count += tmp; > > > + } > > > +} > > > + > > > > > > static void cedrus_set_params(struct cedrus_ctx *ctx, > > > > > > struct cedrus_run *run) > > > > > > { > > > > > > @@ -299,12 +322,11 @@ static void cedrus_set_params(struct cedrus_ctx > > > *ctx, > > > > > > struct vb2_buffer *src_buf = &run->src->vb2_buf; > > > struct cedrus_dev *dev = ctx->dev; > > > dma_addr_t src_buf_addr; > > > > > > - u32 offset = slice->header_bit_size; > > > - u32 len = (slice->size * 8) - offset; > > > + u32 len = slice->size * 8; > > > > > > u32 reg; > > > > > > cedrus_write(dev, VE_H264_VLD_LEN, len); > > > > > > - cedrus_write(dev, VE_H264_VLD_OFFSET, offset); > > > + cedrus_write(dev, VE_H264_VLD_OFFSET, 0); > > > > > > src_buf_addr = vb2_dma_contig_plane_dma_addr(src_buf, 0); > > > cedrus_write(dev, VE_H264_VLD_END, > > > > > > @@ -323,6 +345,8 @@ static void cedrus_set_params(struct cedrus_ctx *ctx, > > > > > > cedrus_write(dev, VE_H264_TRIGGER_TYPE, > > > > > > VE_H264_TRIGGER_TYPE_INIT_SWDEC); > > > > > > + cedrus_skip_bits(dev, slice->header_bit_size); > > > + > > > > > > if (((pps->flags & V4L2_H264_PPS_FLAG_WEIGHTED_PRED) && > > > > > > (slice->slice_type == V4L2_H264_SLICE_TYPE_P || > > > > > > slice->slice_type == V4L2_H264_SLICE_TYPE_SP)) || > > > > > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_regs.h > > > b/drivers/staging/media/sunxi/cedrus/cedrus_regs.h index > > > 3329f9aaf975..b52926a54025 100644 > > > --- a/drivers/staging/media/sunxi/cedrus/cedrus_regs.h > > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_regs.h > > > @@ -538,13 +538,16 @@ > > > > > > > VE_H264_CTRL_SLICE_DECODE_INT) > > > > > > #define VE_H264_TRIGGER_TYPE 0x224 > > > > > > +#define VE_H264_TRIGGER_TYPE_N_BITS(x) (((x) & 0x3f) << 8) > > > > > > #define VE_H264_TRIGGER_TYPE_AVC_SLICE_DECODE (8 << 0) > > > #define VE_H264_TRIGGER_TYPE_INIT_SWDEC (7 << 0) > > > > > > +#define VE_H264_TRIGGER_TYPE_FLUSH_BITS (3 << 0) > > > > > > #define VE_H264_STATUS 0x228 > > > #define VE_H264_STATUS_VLD_DATA_REQ_INT > VE_H264_CTRL_VLD_DATA_REQ_INT > > > #define VE_H264_STATUS_DECODE_ERR_INT > VE_H264_CTRL_DECODE_ERR_INT > > > #define VE_H264_STATUS_SLICE_DECODE_INT > VE_H264_CTRL_SLICE_DECODE_INT > > > > > > +#define VE_H264_STATUS_VLD_BUSY BIT(8) > > > > > > #define VE_H264_STATUS_INT_MASK > VE_H264_CTRL_INT_MASK > > > > [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v2 2/3] media: cedrus: Fix H264 default reference index count 2019-10-02 19:35 [PATCH v2 0/3] media: cedrus: improvements Jernej Skrabec 2019-10-02 19:35 ` [PATCH v2 1/3] media: cedrus: Fix decoding for some H264 videos Jernej Skrabec @ 2019-10-02 19:35 ` Jernej Skrabec 2019-10-02 22:06 ` Paul Kocialkowski 2019-10-02 19:35 ` [PATCH v2 3/3] media: cedrus: Use helpers to access capture queue Jernej Skrabec 2019-10-02 22:23 ` [PATCH v2 0/3] media: cedrus: improvements Paul Kocialkowski 3 siblings, 1 reply; 17+ messages in thread From: Jernej Skrabec @ 2019-10-02 19:35 UTC (permalink / raw) To: mripard, paul.kocialkowski Cc: mchehab, hverkuil-cisco, gregkh, wens, linux-media, devel, linux-arm-kernel, linux-kernel Reference index count in VE_H264_PPS should come from PPS control. However, this is not really important, because reference index count is in our case always overridden by that from slice header. Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> --- drivers/staging/media/sunxi/cedrus/cedrus_h264.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c index bd848146eada..4a0e69855c7f 100644 --- a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c @@ -364,12 +364,8 @@ static void cedrus_set_params(struct cedrus_ctx *ctx, // picture parameters reg = 0; - /* - * FIXME: the kernel headers are allowing the default value to - * be passed, but the libva doesn't give us that. - */ - reg |= (slice->num_ref_idx_l0_active_minus1 & 0x1f) << 10; - reg |= (slice->num_ref_idx_l1_active_minus1 & 0x1f) << 5; + reg |= (pps->num_ref_idx_l0_default_active_minus1 & 0x1f) << 10; + reg |= (pps->num_ref_idx_l1_default_active_minus1 & 0x1f) << 5; reg |= (pps->weighted_bipred_idc & 0x3) << 2; if (pps->flags & V4L2_H264_PPS_FLAG_ENTROPY_CODING_MODE) reg |= VE_H264_PPS_ENTROPY_CODING_MODE; -- 2.23.0 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v2 2/3] media: cedrus: Fix H264 default reference index count 2019-10-02 19:35 ` [PATCH v2 2/3] media: cedrus: Fix H264 default reference index count Jernej Skrabec @ 2019-10-02 22:06 ` Paul Kocialkowski 2019-10-03 5:16 ` Jernej Škrabec 0 siblings, 1 reply; 17+ messages in thread From: Paul Kocialkowski @ 2019-10-02 22:06 UTC (permalink / raw) To: Jernej Skrabec Cc: mripard, mchehab, hverkuil-cisco, gregkh, wens, linux-media, devel, linux-arm-kernel, linux-kernel [-- Attachment #1: Type: text/plain, Size: 1706 bytes --] Hi, On Wed 02 Oct 19, 21:35, Jernej Skrabec wrote: > Reference index count in VE_H264_PPS should come from PPS control. > However, this is not really important, because reference index count is > in our case always overridden by that from slice header. Thanks for the fixup! Our libva userspace and v4l2-request testing tool currently don't provide this, but I have a pending merge request adding it for the hantro so it's good to go. Acked-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com> Cheers, Paul > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > --- > drivers/staging/media/sunxi/cedrus/cedrus_h264.c | 8 ++------ > 1 file changed, 2 insertions(+), 6 deletions(-) > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c > index bd848146eada..4a0e69855c7f 100644 > --- a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c > @@ -364,12 +364,8 @@ static void cedrus_set_params(struct cedrus_ctx *ctx, > > // picture parameters > reg = 0; > - /* > - * FIXME: the kernel headers are allowing the default value to > - * be passed, but the libva doesn't give us that. > - */ > - reg |= (slice->num_ref_idx_l0_active_minus1 & 0x1f) << 10; > - reg |= (slice->num_ref_idx_l1_active_minus1 & 0x1f) << 5; > + reg |= (pps->num_ref_idx_l0_default_active_minus1 & 0x1f) << 10; > + reg |= (pps->num_ref_idx_l1_default_active_minus1 & 0x1f) << 5; > reg |= (pps->weighted_bipred_idc & 0x3) << 2; > if (pps->flags & V4L2_H264_PPS_FLAG_ENTROPY_CODING_MODE) > reg |= VE_H264_PPS_ENTROPY_CODING_MODE; > -- > 2.23.0 > [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 2/3] media: cedrus: Fix H264 default reference index count 2019-10-02 22:06 ` Paul Kocialkowski @ 2019-10-03 5:16 ` Jernej Škrabec 2019-10-03 20:28 ` Paul Kocialkowski 0 siblings, 1 reply; 17+ messages in thread From: Jernej Škrabec @ 2019-10-03 5:16 UTC (permalink / raw) To: Paul Kocialkowski Cc: mripard, mchehab, hverkuil-cisco, gregkh, wens, linux-media, devel, linux-arm-kernel, linux-kernel Dne četrtek, 03. oktober 2019 ob 00:06:50 CEST je Paul Kocialkowski napisal(a): > Hi, > > On Wed 02 Oct 19, 21:35, Jernej Skrabec wrote: > > Reference index count in VE_H264_PPS should come from PPS control. > > However, this is not really important, because reference index count is > > in our case always overridden by that from slice header. > > Thanks for the fixup! > > Our libva userspace and v4l2-request testing tool currently don't provide > this, but I have a pending merge request adding it for the hantro so it's > good to go. Actually, I think this is just cosmetic and it would work even if it would be always 0. We always override this number in SHS2 register with VE_H264_SHS2_NUM_REF_IDX_ACTIVE_OVRD flag and recently there was a patch merged to clarify that value in slice parameters should be the one that's set on default value if override flag is not set in bitstream: https://git.linuxtv.org/media_tree.git/commit/? id=187ef7c5c78153acdce8c8714e5918b1018c710b Well, we could always compare default and value in slice parameters, but I really don't see the benefit of doing that extra work. Best regards, Jernej > > Acked-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com> > > Cheers, > > Paul > > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > > --- > > > > drivers/staging/media/sunxi/cedrus/cedrus_h264.c | 8 ++------ > > 1 file changed, 2 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c > > b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c index > > bd848146eada..4a0e69855c7f 100644 > > --- a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c > > @@ -364,12 +364,8 @@ static void cedrus_set_params(struct cedrus_ctx *ctx, > > > > // picture parameters > > reg = 0; > > > > - /* > > - * FIXME: the kernel headers are allowing the default value to > > - * be passed, but the libva doesn't give us that. > > - */ > > - reg |= (slice->num_ref_idx_l0_active_minus1 & 0x1f) << 10; > > - reg |= (slice->num_ref_idx_l1_active_minus1 & 0x1f) << 5; > > + reg |= (pps->num_ref_idx_l0_default_active_minus1 & 0x1f) << 10; > > + reg |= (pps->num_ref_idx_l1_default_active_minus1 & 0x1f) << 5; > > > > reg |= (pps->weighted_bipred_idc & 0x3) << 2; > > if (pps->flags & V4L2_H264_PPS_FLAG_ENTROPY_CODING_MODE) > > > > reg |= VE_H264_PPS_ENTROPY_CODING_MODE; ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 2/3] media: cedrus: Fix H264 default reference index count 2019-10-03 5:16 ` Jernej Škrabec @ 2019-10-03 20:28 ` Paul Kocialkowski 2019-10-03 20:44 ` Jernej Škrabec 0 siblings, 1 reply; 17+ messages in thread From: Paul Kocialkowski @ 2019-10-03 20:28 UTC (permalink / raw) To: Jernej Škrabec Cc: mripard, mchehab, hverkuil-cisco, gregkh, wens, linux-media, devel, linux-arm-kernel, linux-kernel [-- Attachment #1: Type: text/plain, Size: 3434 bytes --] Hi, On Thu 03 Oct 19, 07:16, Jernej Škrabec wrote: > Dne četrtek, 03. oktober 2019 ob 00:06:50 CEST je Paul Kocialkowski > napisal(a): > > Hi, > > > > On Wed 02 Oct 19, 21:35, Jernej Skrabec wrote: > > > Reference index count in VE_H264_PPS should come from PPS control. > > > However, this is not really important, because reference index count is > > > in our case always overridden by that from slice header. > > > > Thanks for the fixup! > > > > Our libva userspace and v4l2-request testing tool currently don't provide > > this, but I have a pending merge request adding it for the hantro so it's > > good to go. > > Actually, I think this is just cosmetic and it would work even if it would be > always 0. We always override this number in SHS2 register with > VE_H264_SHS2_NUM_REF_IDX_ACTIVE_OVRD flag and recently there was a patch merged > to clarify that value in slice parameters should be the one that's set on > default value if override flag is not set in bitstream: > https://git.linuxtv.org/media_tree.git/commit/? > id=187ef7c5c78153acdce8c8714e5918b1018c710b > > Well, we could always compare default and value in slice parameters, but I > really don't see the benefit of doing that extra work. Thanks for the detailed explanation! So I just realized that for HEVC, I didn't even include the default value in PPS and only went for the per-slice value. The HEVC hardware block apparently only needs the fields once at slice level, and by looking at the spec, only one of the two set of fields will be used. So perhaps we could do the same for H.264 and only have the set of fields once in the slice params, so that both codecs are consistent. Userspace can just check the flag to know whether it should put the PPS default or slice-specific value in the slice-specific control. What do you think? Cheers, Paul > Best regards, > Jernej > > > > > Acked-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com> > > > > Cheers, > > > > Paul > > > > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > > > --- > > > > > > drivers/staging/media/sunxi/cedrus/cedrus_h264.c | 8 ++------ > > > 1 file changed, 2 insertions(+), 6 deletions(-) > > > > > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c > > > b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c index > > > bd848146eada..4a0e69855c7f 100644 > > > --- a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c > > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c > > > @@ -364,12 +364,8 @@ static void cedrus_set_params(struct cedrus_ctx *ctx, > > > > > > // picture parameters > > > reg = 0; > > > > > > - /* > > > - * FIXME: the kernel headers are allowing the default value to > > > - * be passed, but the libva doesn't give us that. > > > - */ > > > - reg |= (slice->num_ref_idx_l0_active_minus1 & 0x1f) << 10; > > > - reg |= (slice->num_ref_idx_l1_active_minus1 & 0x1f) << 5; > > > + reg |= (pps->num_ref_idx_l0_default_active_minus1 & 0x1f) << 10; > > > + reg |= (pps->num_ref_idx_l1_default_active_minus1 & 0x1f) << 5; > > > > > > reg |= (pps->weighted_bipred_idc & 0x3) << 2; > > > if (pps->flags & V4L2_H264_PPS_FLAG_ENTROPY_CODING_MODE) > > > > > > reg |= VE_H264_PPS_ENTROPY_CODING_MODE; > > > > -- Paul Kocialkowski, Bootlin Embedded Linux and kernel engineering https://bootlin.com [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 2/3] media: cedrus: Fix H264 default reference index count 2019-10-03 20:28 ` Paul Kocialkowski @ 2019-10-03 20:44 ` Jernej Škrabec 2019-10-03 20:58 ` Paul Kocialkowski 0 siblings, 1 reply; 17+ messages in thread From: Jernej Škrabec @ 2019-10-03 20:44 UTC (permalink / raw) To: Paul Kocialkowski Cc: mripard, mchehab, hverkuil-cisco, gregkh, wens, linux-media, devel, linux-arm-kernel, linux-kernel Dne četrtek, 03. oktober 2019 ob 22:28:46 CEST je Paul Kocialkowski napisal(a): > Hi, > > On Thu 03 Oct 19, 07:16, Jernej Škrabec wrote: > > Dne četrtek, 03. oktober 2019 ob 00:06:50 CEST je Paul Kocialkowski > > > > napisal(a): > > > Hi, > > > > > > On Wed 02 Oct 19, 21:35, Jernej Skrabec wrote: > > > > Reference index count in VE_H264_PPS should come from PPS control. > > > > However, this is not really important, because reference index count > > > > is > > > > in our case always overridden by that from slice header. > > > > > > Thanks for the fixup! > > > > > > Our libva userspace and v4l2-request testing tool currently don't > > > provide > > > this, but I have a pending merge request adding it for the hantro so > > > it's > > > good to go. > > > > Actually, I think this is just cosmetic and it would work even if it would > > be always 0. We always override this number in SHS2 register with > > VE_H264_SHS2_NUM_REF_IDX_ACTIVE_OVRD flag and recently there was a patch > > merged to clarify that value in slice parameters should be the one that's > > set on default value if override flag is not set in bitstream: > > https://git.linuxtv.org/media_tree.git/commit/? > > id=187ef7c5c78153acdce8c8714e5918b1018c710b > > > > Well, we could always compare default and value in slice parameters, but I > > really don't see the benefit of doing that extra work. > > Thanks for the detailed explanation! So I just realized that for HEVC, I > didn't even include the default value in PPS and only went for the > per-slice value. The HEVC hardware block apparently only needs the fields > once at slice level, and by looking at the spec, only one of the two set of > fields will be used. > > So perhaps we could do the same for H.264 and only have the set of fields > once in the slice params, so that both codecs are consistent. Userspace can > just check the flag to know whether it should put the PPS default or > slice-specific value in the slice-specific control. > > What do you think? I think that there would be less confusion if only value in slice params would exists. But since Philipp rather made clarification in documentation, maybe he sees benefit having both values? Best regards, Jernej > > Cheers, > > Paul > > > Best regards, > > Jernej > > > > > Acked-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com> > > > > > > Cheers, > > > > > > Paul > > > > > > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > > > > --- > > > > > > > > drivers/staging/media/sunxi/cedrus/cedrus_h264.c | 8 ++------ > > > > 1 file changed, 2 insertions(+), 6 deletions(-) > > > > > > > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c > > > > b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c index > > > > bd848146eada..4a0e69855c7f 100644 > > > > --- a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c > > > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c > > > > @@ -364,12 +364,8 @@ static void cedrus_set_params(struct cedrus_ctx > > > > *ctx, > > > > > > > > // picture parameters > > > > reg = 0; > > > > > > > > - /* > > > > - * FIXME: the kernel headers are allowing the default value to > > > > - * be passed, but the libva doesn't give us that. > > > > - */ > > > > - reg |= (slice->num_ref_idx_l0_active_minus1 & 0x1f) << 10; > > > > - reg |= (slice->num_ref_idx_l1_active_minus1 & 0x1f) << 5; > > > > + reg |= (pps->num_ref_idx_l0_default_active_minus1 & 0x1f) << 10; > > > > + reg |= (pps->num_ref_idx_l1_default_active_minus1 & 0x1f) << 5; > > > > > > > > reg |= (pps->weighted_bipred_idc & 0x3) << 2; > > > > if (pps->flags & V4L2_H264_PPS_FLAG_ENTROPY_CODING_MODE) > > > > > > > > reg |= VE_H264_PPS_ENTROPY_CODING_MODE; ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 2/3] media: cedrus: Fix H264 default reference index count 2019-10-03 20:44 ` Jernej Škrabec @ 2019-10-03 20:58 ` Paul Kocialkowski 2019-10-03 21:19 ` Jernej Škrabec 0 siblings, 1 reply; 17+ messages in thread From: Paul Kocialkowski @ 2019-10-03 20:58 UTC (permalink / raw) To: Jernej Škrabec Cc: mripard, mchehab, hverkuil-cisco, gregkh, wens, linux-media, devel, linux-arm-kernel, linux-kernel [-- Attachment #1: Type: text/plain, Size: 4917 bytes --] Hi, On Thu 03 Oct 19, 22:44, Jernej Škrabec wrote: > Dne četrtek, 03. oktober 2019 ob 22:28:46 CEST je Paul Kocialkowski > napisal(a): > > Hi, > > > > On Thu 03 Oct 19, 07:16, Jernej Škrabec wrote: > > > Dne četrtek, 03. oktober 2019 ob 00:06:50 CEST je Paul Kocialkowski > > > > > > napisal(a): > > > > Hi, > > > > > > > > On Wed 02 Oct 19, 21:35, Jernej Skrabec wrote: > > > > > Reference index count in VE_H264_PPS should come from PPS control. > > > > > However, this is not really important, because reference index count > > > > > is > > > > > in our case always overridden by that from slice header. > > > > > > > > Thanks for the fixup! > > > > > > > > Our libva userspace and v4l2-request testing tool currently don't > > > > provide > > > > this, but I have a pending merge request adding it for the hantro so > > > > it's > > > > good to go. > > > > > > Actually, I think this is just cosmetic and it would work even if it would > > > be always 0. We always override this number in SHS2 register with > > > VE_H264_SHS2_NUM_REF_IDX_ACTIVE_OVRD flag and recently there was a patch > > > merged to clarify that value in slice parameters should be the one that's > > > set on default value if override flag is not set in bitstream: > > > https://git.linuxtv.org/media_tree.git/commit/? > > > id=187ef7c5c78153acdce8c8714e5918b1018c710b > > > > > > Well, we could always compare default and value in slice parameters, but I > > > really don't see the benefit of doing that extra work. > > > > Thanks for the detailed explanation! So I just realized that for HEVC, I > > didn't even include the default value in PPS and only went for the > > per-slice value. The HEVC hardware block apparently only needs the fields > > once at slice level, and by looking at the spec, only one of the two set of > > fields will be used. > > > > So perhaps we could do the same for H.264 and only have the set of fields > > once in the slice params, so that both codecs are consistent. Userspace can > > just check the flag to know whether it should put the PPS default or > > slice-specific value in the slice-specific control. > > > > What do you think? > > I think that there would be less confusion if only value in slice params would > exists. But since Philipp rather made clarification in documentation, maybe he > sees benefit having both values? Actually I just caught up with the discussion from thread: media: uapi: h264: Add num_ref_idx_active_override_flag which explains that we need to pass the default fields for hardware that parses the slice header itself and we need the non-default fields and flag for other cases. To cover the case of hardware that does slice header parsing, I guess it would also work to use the slice-specific values in place of the pps default values in the hardware register for that. But it feels quite confusing and a lot less straightforward than having all the fields and the override flag exposed. So I think I should fix HEVC support accordingly, just in case the same situation arises for HEVC. Cheers, Paul > Best regards, > Jernej > > > > > Cheers, > > > > Paul > > > > > Best regards, > > > Jernej > > > > > > > Acked-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com> > > > > > > > > Cheers, > > > > > > > > Paul > > > > > > > > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > > > > > --- > > > > > > > > > > drivers/staging/media/sunxi/cedrus/cedrus_h264.c | 8 ++------ > > > > > 1 file changed, 2 insertions(+), 6 deletions(-) > > > > > > > > > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c > > > > > b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c index > > > > > bd848146eada..4a0e69855c7f 100644 > > > > > --- a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c > > > > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c > > > > > @@ -364,12 +364,8 @@ static void cedrus_set_params(struct cedrus_ctx > > > > > *ctx, > > > > > > > > > > // picture parameters > > > > > reg = 0; > > > > > > > > > > - /* > > > > > - * FIXME: the kernel headers are allowing the default value to > > > > > - * be passed, but the libva doesn't give us that. > > > > > - */ > > > > > - reg |= (slice->num_ref_idx_l0_active_minus1 & 0x1f) << 10; > > > > > - reg |= (slice->num_ref_idx_l1_active_minus1 & 0x1f) << 5; > > > > > + reg |= (pps->num_ref_idx_l0_default_active_minus1 & 0x1f) << 10; > > > > > + reg |= (pps->num_ref_idx_l1_default_active_minus1 & 0x1f) << 5; > > > > > > > > > > reg |= (pps->weighted_bipred_idc & 0x3) << 2; > > > > > if (pps->flags & V4L2_H264_PPS_FLAG_ENTROPY_CODING_MODE) > > > > > > > > > > reg |= VE_H264_PPS_ENTROPY_CODING_MODE; > > > > -- Paul Kocialkowski, Bootlin Embedded Linux and kernel engineering https://bootlin.com [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 2/3] media: cedrus: Fix H264 default reference index count 2019-10-03 20:58 ` Paul Kocialkowski @ 2019-10-03 21:19 ` Jernej Škrabec 2019-10-03 21:32 ` Paul Kocialkowski 0 siblings, 1 reply; 17+ messages in thread From: Jernej Škrabec @ 2019-10-03 21:19 UTC (permalink / raw) To: Paul Kocialkowski Cc: mripard, mchehab, hverkuil-cisco, gregkh, wens, linux-media, devel, linux-arm-kernel, linux-kernel Dne četrtek, 03. oktober 2019 ob 22:58:57 CEST je Paul Kocialkowski napisal(a): > Hi, > > On Thu 03 Oct 19, 22:44, Jernej Škrabec wrote: > > Dne četrtek, 03. oktober 2019 ob 22:28:46 CEST je Paul Kocialkowski > > > > napisal(a): > > > Hi, > > > > > > On Thu 03 Oct 19, 07:16, Jernej Škrabec wrote: > > > > Dne četrtek, 03. oktober 2019 ob 00:06:50 CEST je Paul Kocialkowski > > > > > > > > napisal(a): > > > > > Hi, > > > > > > > > > > On Wed 02 Oct 19, 21:35, Jernej Skrabec wrote: > > > > > > Reference index count in VE_H264_PPS should come from PPS control. > > > > > > However, this is not really important, because reference index > > > > > > count > > > > > > is > > > > > > in our case always overridden by that from slice header. > > > > > > > > > > Thanks for the fixup! > > > > > > > > > > Our libva userspace and v4l2-request testing tool currently don't > > > > > provide > > > > > this, but I have a pending merge request adding it for the hantro so > > > > > it's > > > > > good to go. > > > > > > > > Actually, I think this is just cosmetic and it would work even if it > > > > would > > > > be always 0. We always override this number in SHS2 register with > > > > VE_H264_SHS2_NUM_REF_IDX_ACTIVE_OVRD flag and recently there was a > > > > patch > > > > merged to clarify that value in slice parameters should be the one > > > > that's > > > > set on default value if override flag is not set in bitstream: > > > > https://git.linuxtv.org/media_tree.git/commit/? > > > > id=187ef7c5c78153acdce8c8714e5918b1018c710b > > > > > > > > Well, we could always compare default and value in slice parameters, > > > > but I > > > > really don't see the benefit of doing that extra work. > > > > > > Thanks for the detailed explanation! So I just realized that for HEVC, I > > > didn't even include the default value in PPS and only went for the > > > per-slice value. The HEVC hardware block apparently only needs the > > > fields > > > once at slice level, and by looking at the spec, only one of the two set > > > of > > > fields will be used. > > > > > > So perhaps we could do the same for H.264 and only have the set of > > > fields > > > once in the slice params, so that both codecs are consistent. Userspace > > > can > > > just check the flag to know whether it should put the PPS default or > > > slice-specific value in the slice-specific control. > > > > > > What do you think? > > > > I think that there would be less confusion if only value in slice params > > would exists. But since Philipp rather made clarification in > > documentation, maybe he sees benefit having both values? > > Actually I just caught up with the discussion from thread: > media: uapi: h264: Add num_ref_idx_active_override_flag > > which explains that we need to pass the default fields for hardware that > parses the slice header itself and we need the non-default fields and flag > for other cases. > > To cover the case of hardware that does slice header parsing, I guess it > would also work to use the slice-specific values in place of the pps > default values in the hardware register for that. But it feels quite > confusing and a lot less straightforward than having all the fields and the > override flag exposed. I wasn't aware of that patch and related discussion. Ok, so it make sense to have both values. However, does it make sense to use default values in Cedrus? > > So I think I should fix HEVC support accordingly, just in case the same > situation arises for HEVC. Seems reasonable. Does that mean there will be another revision of HEVC patches? If so, I think slice_segment_addr should also be included in slice params, so multi-slice frames can be supported at later time. Best regards, Jernej > > Cheers, > > Paul > > > Best regards, > > Jernej > > > > > Cheers, > > > > > > Paul > > > > > > > Best regards, > > > > Jernej > > > > > > > > > Acked-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com> > > > > > > > > > > Cheers, > > > > > > > > > > Paul > > > > > > > > > > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > > > > > > --- > > > > > > > > > > > > drivers/staging/media/sunxi/cedrus/cedrus_h264.c | 8 ++------ > > > > > > 1 file changed, 2 insertions(+), 6 deletions(-) > > > > > > > > > > > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c > > > > > > b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c index > > > > > > bd848146eada..4a0e69855c7f 100644 > > > > > > --- a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c > > > > > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c > > > > > > @@ -364,12 +364,8 @@ static void cedrus_set_params(struct > > > > > > cedrus_ctx > > > > > > *ctx, > > > > > > > > > > > > // picture parameters > > > > > > reg = 0; > > > > > > > > > > > > - /* > > > > > > - * FIXME: the kernel headers are allowing the default value to > > > > > > - * be passed, but the libva doesn't give us that. > > > > > > - */ > > > > > > - reg |= (slice->num_ref_idx_l0_active_minus1 & 0x1f) << 10; > > > > > > - reg |= (slice->num_ref_idx_l1_active_minus1 & 0x1f) << 5; > > > > > > + reg |= (pps->num_ref_idx_l0_default_active_minus1 & 0x1f) << 10; > > > > > > + reg |= (pps->num_ref_idx_l1_default_active_minus1 & 0x1f) << 5; > > > > > > > > > > > > reg |= (pps->weighted_bipred_idc & 0x3) << 2; > > > > > > if (pps->flags & V4L2_H264_PPS_FLAG_ENTROPY_CODING_MODE) > > > > > > > > > > > > reg |= VE_H264_PPS_ENTROPY_CODING_MODE; ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 2/3] media: cedrus: Fix H264 default reference index count 2019-10-03 21:19 ` Jernej Škrabec @ 2019-10-03 21:32 ` Paul Kocialkowski 0 siblings, 0 replies; 17+ messages in thread From: Paul Kocialkowski @ 2019-10-03 21:32 UTC (permalink / raw) To: Jernej Škrabec Cc: mripard, mchehab, hverkuil-cisco, gregkh, wens, linux-media, devel, linux-arm-kernel, linux-kernel [-- Attachment #1: Type: text/plain, Size: 6684 bytes --] On Thu 03 Oct 19, 23:19, Jernej Škrabec wrote: > Dne četrtek, 03. oktober 2019 ob 22:58:57 CEST je Paul Kocialkowski > napisal(a): > > Hi, > > > > On Thu 03 Oct 19, 22:44, Jernej Škrabec wrote: > > > Dne četrtek, 03. oktober 2019 ob 22:28:46 CEST je Paul Kocialkowski > > > > > > napisal(a): > > > > Hi, > > > > > > > > On Thu 03 Oct 19, 07:16, Jernej Škrabec wrote: > > > > > Dne četrtek, 03. oktober 2019 ob 00:06:50 CEST je Paul Kocialkowski > > > > > > > > > > napisal(a): > > > > > > Hi, > > > > > > > > > > > > On Wed 02 Oct 19, 21:35, Jernej Skrabec wrote: > > > > > > > Reference index count in VE_H264_PPS should come from PPS control. > > > > > > > However, this is not really important, because reference index > > > > > > > count > > > > > > > is > > > > > > > in our case always overridden by that from slice header. > > > > > > > > > > > > Thanks for the fixup! > > > > > > > > > > > > Our libva userspace and v4l2-request testing tool currently don't > > > > > > provide > > > > > > this, but I have a pending merge request adding it for the hantro so > > > > > > it's > > > > > > good to go. > > > > > > > > > > Actually, I think this is just cosmetic and it would work even if it > > > > > would > > > > > be always 0. We always override this number in SHS2 register with > > > > > VE_H264_SHS2_NUM_REF_IDX_ACTIVE_OVRD flag and recently there was a > > > > > patch > > > > > merged to clarify that value in slice parameters should be the one > > > > > that's > > > > > set on default value if override flag is not set in bitstream: > > > > > https://git.linuxtv.org/media_tree.git/commit/? > > > > > id=187ef7c5c78153acdce8c8714e5918b1018c710b > > > > > > > > > > Well, we could always compare default and value in slice parameters, > > > > > but I > > > > > really don't see the benefit of doing that extra work. > > > > > > > > Thanks for the detailed explanation! So I just realized that for HEVC, I > > > > didn't even include the default value in PPS and only went for the > > > > per-slice value. The HEVC hardware block apparently only needs the > > > > fields > > > > once at slice level, and by looking at the spec, only one of the two set > > > > of > > > > fields will be used. > > > > > > > > So perhaps we could do the same for H.264 and only have the set of > > > > fields > > > > once in the slice params, so that both codecs are consistent. Userspace > > > > can > > > > just check the flag to know whether it should put the PPS default or > > > > slice-specific value in the slice-specific control. > > > > > > > > What do you think? > > > > > > I think that there would be less confusion if only value in slice params > > > would exists. But since Philipp rather made clarification in > > > documentation, maybe he sees benefit having both values? > > > > Actually I just caught up with the discussion from thread: > > media: uapi: h264: Add num_ref_idx_active_override_flag > > > > which explains that we need to pass the default fields for hardware that > > parses the slice header itself and we need the non-default fields and flag > > for other cases. > > > > To cover the case of hardware that does slice header parsing, I guess it > > would also work to use the slice-specific values in place of the pps > > default values in the hardware register for that. But it feels quite > > confusing and a lot less straightforward than having all the fields and the > > override flag exposed. > > I wasn't aware of that patch and related discussion. Ok, so it make sense to > have both values. However, does it make sense to use default values in Cedrus? Well, since the hardware exposes fields for both and the flag for H264, let's do the straightforward thing and just pass the values through, even though we can easily predict which will get used in the end. For HEVC, we'll just have to check for the flag and put the right set of values in the slice-specific register. > > So I think I should fix HEVC support accordingly, just in case the same > > situation arises for HEVC. > > Seems reasonable. Does that mean there will be another revision of HEVC > patches? If so, I think slice_segment_addr should also be included in slice > params, so multi-slice frames can be supported at later time. I would be in favor of fixing this as a follow-up patch instead, so that we don't delay getting the series in. As you said, more work will be needed anyway for multi-slice support, so I don't see the point of holding the series for this particular improvment. Cheers, Paul > Best regards, > Jernej > > > > > Cheers, > > > > Paul > > > > > Best regards, > > > Jernej > > > > > > > Cheers, > > > > > > > > Paul > > > > > > > > > Best regards, > > > > > Jernej > > > > > > > > > > > Acked-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com> > > > > > > > > > > > > Cheers, > > > > > > > > > > > > Paul > > > > > > > > > > > > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > > > > > > > --- > > > > > > > > > > > > > > drivers/staging/media/sunxi/cedrus/cedrus_h264.c | 8 ++------ > > > > > > > 1 file changed, 2 insertions(+), 6 deletions(-) > > > > > > > > > > > > > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c > > > > > > > b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c index > > > > > > > bd848146eada..4a0e69855c7f 100644 > > > > > > > --- a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c > > > > > > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c > > > > > > > @@ -364,12 +364,8 @@ static void cedrus_set_params(struct > > > > > > > cedrus_ctx > > > > > > > *ctx, > > > > > > > > > > > > > > // picture parameters > > > > > > > reg = 0; > > > > > > > > > > > > > > - /* > > > > > > > - * FIXME: the kernel headers are allowing the default value to > > > > > > > - * be passed, but the libva doesn't give us that. > > > > > > > - */ > > > > > > > - reg |= (slice->num_ref_idx_l0_active_minus1 & 0x1f) << 10; > > > > > > > - reg |= (slice->num_ref_idx_l1_active_minus1 & 0x1f) << 5; > > > > > > > + reg |= (pps->num_ref_idx_l0_default_active_minus1 & 0x1f) << 10; > > > > > > > + reg |= (pps->num_ref_idx_l1_default_active_minus1 & 0x1f) << 5; > > > > > > > > > > > > > > reg |= (pps->weighted_bipred_idc & 0x3) << 2; > > > > > > > if (pps->flags & V4L2_H264_PPS_FLAG_ENTROPY_CODING_MODE) > > > > > > > > > > > > > > reg |= VE_H264_PPS_ENTROPY_CODING_MODE; > > > > -- Paul Kocialkowski, Bootlin Embedded Linux and kernel engineering https://bootlin.com [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v2 3/3] media: cedrus: Use helpers to access capture queue 2019-10-02 19:35 [PATCH v2 0/3] media: cedrus: improvements Jernej Skrabec 2019-10-02 19:35 ` [PATCH v2 1/3] media: cedrus: Fix decoding for some H264 videos Jernej Skrabec 2019-10-02 19:35 ` [PATCH v2 2/3] media: cedrus: Fix H264 default reference index count Jernej Skrabec @ 2019-10-02 19:35 ` Jernej Skrabec 2019-10-02 22:14 ` Paul Kocialkowski 2019-10-02 22:23 ` [PATCH v2 0/3] media: cedrus: improvements Paul Kocialkowski 3 siblings, 1 reply; 17+ messages in thread From: Jernej Skrabec @ 2019-10-02 19:35 UTC (permalink / raw) To: mripard, paul.kocialkowski Cc: mchehab, hverkuil-cisco, gregkh, wens, linux-media, devel, linux-arm-kernel, linux-kernel Accessing capture queue structue directly is not safe. Use helpers for that. Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> --- drivers/staging/media/sunxi/cedrus/cedrus.h | 8 ++++++-- drivers/staging/media/sunxi/cedrus/cedrus_h264.c | 8 ++++++-- 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.h b/drivers/staging/media/sunxi/cedrus/cedrus.h index 986e059e3202..c45fb9a7ad07 100644 --- a/drivers/staging/media/sunxi/cedrus/cedrus.h +++ b/drivers/staging/media/sunxi/cedrus/cedrus.h @@ -197,12 +197,16 @@ static inline dma_addr_t cedrus_buf_addr(struct vb2_buffer *buf, static inline dma_addr_t cedrus_dst_buf_addr(struct cedrus_ctx *ctx, int index, unsigned int plane) { - struct vb2_buffer *buf; + struct vb2_buffer *buf = NULL; + struct vb2_queue *vq; if (index < 0) return 0; - buf = ctx->fh.m2m_ctx->cap_q_ctx.q.bufs[index]; + vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, V4L2_BUF_TYPE_VIDEO_CAPTURE); + if (vq) + buf = vb2_get_buffer(vq, index); + return buf ? cedrus_buf_addr(buf, &ctx->dst_fmt, plane) : 0; } diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c index 4a0e69855c7f..4650982c69a8 100644 --- a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c @@ -97,7 +97,7 @@ static void cedrus_write_frame_list(struct cedrus_ctx *ctx, const struct v4l2_ctrl_h264_decode_params *decode = run->h264.decode_params; const struct v4l2_ctrl_h264_slice_params *slice = run->h264.slice_params; const struct v4l2_ctrl_h264_sps *sps = run->h264.sps; - struct vb2_queue *cap_q = &ctx->fh.m2m_ctx->cap_q_ctx.q; + struct vb2_queue *cap_q; struct cedrus_buffer *output_buf; struct cedrus_dev *dev = ctx->dev; unsigned long used_dpbs = 0; @@ -105,6 +105,8 @@ static void cedrus_write_frame_list(struct cedrus_ctx *ctx, unsigned int output = 0; unsigned int i; + cap_q = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, V4L2_BUF_TYPE_VIDEO_CAPTURE); + memset(pic_list, 0, sizeof(pic_list)); for (i = 0; i < ARRAY_SIZE(decode->dpb); i++) { @@ -168,12 +170,14 @@ static void _cedrus_write_ref_list(struct cedrus_ctx *ctx, enum cedrus_h264_sram_off sram) { const struct v4l2_ctrl_h264_decode_params *decode = run->h264.decode_params; - struct vb2_queue *cap_q = &ctx->fh.m2m_ctx->cap_q_ctx.q; + struct vb2_queue *cap_q; struct cedrus_dev *dev = ctx->dev; u8 sram_array[CEDRUS_MAX_REF_IDX]; unsigned int i; size_t size; + cap_q = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, V4L2_BUF_TYPE_VIDEO_CAPTURE); + memset(sram_array, 0, sizeof(sram_array)); for (i = 0; i < num_ref; i++) { -- 2.23.0 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v2 3/3] media: cedrus: Use helpers to access capture queue 2019-10-02 19:35 ` [PATCH v2 3/3] media: cedrus: Use helpers to access capture queue Jernej Skrabec @ 2019-10-02 22:14 ` Paul Kocialkowski 0 siblings, 0 replies; 17+ messages in thread From: Paul Kocialkowski @ 2019-10-02 22:14 UTC (permalink / raw) To: Jernej Skrabec Cc: mripard, mchehab, hverkuil-cisco, gregkh, wens, linux-media, devel, linux-arm-kernel, linux-kernel [-- Attachment #1: Type: text/plain, Size: 3100 bytes --] Hi, On Wed 02 Oct 19, 21:35, Jernej Skrabec wrote: > Accessing capture queue structue directly is not safe. Use helpers for > that. Looks good to me, thanks! Acked-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com> Cheers, Paul > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > --- > drivers/staging/media/sunxi/cedrus/cedrus.h | 8 ++++++-- > drivers/staging/media/sunxi/cedrus/cedrus_h264.c | 8 ++++++-- > 2 files changed, 12 insertions(+), 4 deletions(-) > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.h b/drivers/staging/media/sunxi/cedrus/cedrus.h > index 986e059e3202..c45fb9a7ad07 100644 > --- a/drivers/staging/media/sunxi/cedrus/cedrus.h > +++ b/drivers/staging/media/sunxi/cedrus/cedrus.h > @@ -197,12 +197,16 @@ static inline dma_addr_t cedrus_buf_addr(struct vb2_buffer *buf, > static inline dma_addr_t cedrus_dst_buf_addr(struct cedrus_ctx *ctx, > int index, unsigned int plane) > { > - struct vb2_buffer *buf; > + struct vb2_buffer *buf = NULL; > + struct vb2_queue *vq; > > if (index < 0) > return 0; > > - buf = ctx->fh.m2m_ctx->cap_q_ctx.q.bufs[index]; > + vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, V4L2_BUF_TYPE_VIDEO_CAPTURE); > + if (vq) > + buf = vb2_get_buffer(vq, index); > + > return buf ? cedrus_buf_addr(buf, &ctx->dst_fmt, plane) : 0; > } > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c > index 4a0e69855c7f..4650982c69a8 100644 > --- a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c > @@ -97,7 +97,7 @@ static void cedrus_write_frame_list(struct cedrus_ctx *ctx, > const struct v4l2_ctrl_h264_decode_params *decode = run->h264.decode_params; > const struct v4l2_ctrl_h264_slice_params *slice = run->h264.slice_params; > const struct v4l2_ctrl_h264_sps *sps = run->h264.sps; > - struct vb2_queue *cap_q = &ctx->fh.m2m_ctx->cap_q_ctx.q; > + struct vb2_queue *cap_q; > struct cedrus_buffer *output_buf; > struct cedrus_dev *dev = ctx->dev; > unsigned long used_dpbs = 0; > @@ -105,6 +105,8 @@ static void cedrus_write_frame_list(struct cedrus_ctx *ctx, > unsigned int output = 0; > unsigned int i; > > + cap_q = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, V4L2_BUF_TYPE_VIDEO_CAPTURE); > + > memset(pic_list, 0, sizeof(pic_list)); > > for (i = 0; i < ARRAY_SIZE(decode->dpb); i++) { > @@ -168,12 +170,14 @@ static void _cedrus_write_ref_list(struct cedrus_ctx *ctx, > enum cedrus_h264_sram_off sram) > { > const struct v4l2_ctrl_h264_decode_params *decode = run->h264.decode_params; > - struct vb2_queue *cap_q = &ctx->fh.m2m_ctx->cap_q_ctx.q; > + struct vb2_queue *cap_q; > struct cedrus_dev *dev = ctx->dev; > u8 sram_array[CEDRUS_MAX_REF_IDX]; > unsigned int i; > size_t size; > > + cap_q = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, V4L2_BUF_TYPE_VIDEO_CAPTURE); > + > memset(sram_array, 0, sizeof(sram_array)); > > for (i = 0; i < num_ref; i++) { > -- > 2.23.0 > [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 0/3] media: cedrus: improvements 2019-10-02 19:35 [PATCH v2 0/3] media: cedrus: improvements Jernej Skrabec ` (2 preceding siblings ...) 2019-10-02 19:35 ` [PATCH v2 3/3] media: cedrus: Use helpers to access capture queue Jernej Skrabec @ 2019-10-02 22:23 ` Paul Kocialkowski 2019-10-03 5:21 ` Jernej Škrabec 3 siblings, 1 reply; 17+ messages in thread From: Paul Kocialkowski @ 2019-10-02 22:23 UTC (permalink / raw) To: Jernej Skrabec Cc: mripard, mchehab, hverkuil-cisco, gregkh, wens, linux-media, devel, linux-arm-kernel, linux-kernel [-- Attachment #1: Type: text/plain, Size: 1368 bytes --] Hi, On Wed 02 Oct 19, 21:35, Jernej Skrabec wrote: > This is continuation of https://lkml.org/lkml/2019/5/30/1459 with several > patches removed (2 merged, others needs redesign) and one added. Thanks for the continued effort on this, these fixes are greatly appreciated (and more generally, all the work you are putting into cedrus)! Although I've been out of the loop on cedrus, it is very nice to see development happening. Keep up the good work! Cheers, Paul > Patch 1 fixes h264 playback issue which happens in rare cases. > > Patch 2 sets PPS default reference index count in register from PPS > control. Currently it was set to values from slice control. > > Patch 3 replaces direct accesses to capture queue from m2m contex with > helpers which was designed exactly for that. It's also safer since > helpers do some checks. > > Best regards, > Jernej > > Jernej Skrabec (3): > media: cedrus: Fix decoding for some H264 videos > media: cedrus: Fix H264 default reference index count > media: cedrus: Use helpers to access capture queue > > drivers/staging/media/sunxi/cedrus/cedrus.h | 8 +++- > .../staging/media/sunxi/cedrus/cedrus_h264.c | 46 ++++++++++++++----- > .../staging/media/sunxi/cedrus/cedrus_regs.h | 3 ++ > 3 files changed, 44 insertions(+), 13 deletions(-) > > -- > 2.23.0 > [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 0/3] media: cedrus: improvements 2019-10-02 22:23 ` [PATCH v2 0/3] media: cedrus: improvements Paul Kocialkowski @ 2019-10-03 5:21 ` Jernej Škrabec 0 siblings, 0 replies; 17+ messages in thread From: Jernej Škrabec @ 2019-10-03 5:21 UTC (permalink / raw) To: Paul Kocialkowski Cc: mripard, mchehab, hverkuil-cisco, gregkh, wens, linux-media, devel, linux-arm-kernel, linux-kernel Dne četrtek, 03. oktober 2019 ob 00:23:07 CEST je Paul Kocialkowski napisal(a): > Hi, > > On Wed 02 Oct 19, 21:35, Jernej Skrabec wrote: > > This is continuation of https://lkml.org/lkml/2019/5/30/1459 with several > > patches removed (2 merged, others needs redesign) and one added. > > Thanks for the continued effort on this, these fixes are greatly appreciated > (and more generally, all the work you are putting into cedrus)! > > Although I've been out of the loop on cedrus, it is very nice to see > development happening. Keep up the good work! Thanks! Those fixes are just cleaned up version of patches I'm using in LibreELEC for some time now. I hate maintaining out-of-tree patches over a long period, so pushing them upstream is beneficial for all :) I'll send more improvements once your HEVC patches are merged. Best regards, Jernej > > Cheers, > > Paul > > > Patch 1 fixes h264 playback issue which happens in rare cases. > > > > Patch 2 sets PPS default reference index count in register from PPS > > control. Currently it was set to values from slice control. > > > > Patch 3 replaces direct accesses to capture queue from m2m contex with > > helpers which was designed exactly for that. It's also safer since > > helpers do some checks. > > > > Best regards, > > Jernej > > > > Jernej Skrabec (3): > > media: cedrus: Fix decoding for some H264 videos > > media: cedrus: Fix H264 default reference index count > > media: cedrus: Use helpers to access capture queue > > > > drivers/staging/media/sunxi/cedrus/cedrus.h | 8 +++- > > .../staging/media/sunxi/cedrus/cedrus_h264.c | 46 ++++++++++++++----- > > .../staging/media/sunxi/cedrus/cedrus_regs.h | 3 ++ > > 3 files changed, 44 insertions(+), 13 deletions(-) ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2019-10-22 9:10 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-10-02 19:35 [PATCH v2 0/3] media: cedrus: improvements Jernej Skrabec 2019-10-02 19:35 ` [PATCH v2 1/3] media: cedrus: Fix decoding for some H264 videos Jernej Skrabec 2019-10-02 21:54 ` Paul Kocialkowski 2019-10-15 17:16 ` Jernej Škrabec 2019-10-22 9:10 ` Paul Kocialkowski 2019-10-02 19:35 ` [PATCH v2 2/3] media: cedrus: Fix H264 default reference index count Jernej Skrabec 2019-10-02 22:06 ` Paul Kocialkowski 2019-10-03 5:16 ` Jernej Škrabec 2019-10-03 20:28 ` Paul Kocialkowski 2019-10-03 20:44 ` Jernej Škrabec 2019-10-03 20:58 ` Paul Kocialkowski 2019-10-03 21:19 ` Jernej Škrabec 2019-10-03 21:32 ` Paul Kocialkowski 2019-10-02 19:35 ` [PATCH v2 3/3] media: cedrus: Use helpers to access capture queue Jernej Skrabec 2019-10-02 22:14 ` Paul Kocialkowski 2019-10-02 22:23 ` [PATCH v2 0/3] media: cedrus: improvements Paul Kocialkowski 2019-10-03 5:21 ` Jernej Škrabec
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).