| Message ID | 20190109141920.12677-2-paul.kocialkowski@bootlin.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] media: cedrus: Cleanup duplicate declarations from cedrus_dec header | expand |
On 01/09/19 15:19, Paul Kocialkowski wrote: > It was reported that some cases of interleaved video decoding require > using the current destination buffer as a reference. However, this is > no longer possible after the move to vb2_find_timestamp because only > dequeued and done buffers are considered. > > Add a helper in our driver that also considers the current destination > buffer before resorting to vb2_find_timestamp and use it in MPEG-2. This patch looks good, but can you also add checks to handle the case when no buffer with the given timestamp was found? Probably should be done in a third patch. I suspect the driver will crash if an unknown timestamp is passed on to the driver. I would really like to see that corner case fixed. Regards, Hans > > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com> > --- > drivers/staging/media/sunxi/cedrus/cedrus_dec.c | 13 +++++++++++++ > drivers/staging/media/sunxi/cedrus/cedrus_dec.h | 2 ++ > drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c | 10 ++++++---- > 3 files changed, 21 insertions(+), 4 deletions(-) > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_dec.c b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c > index 443fb037e1cf..2c295286766c 100644 > --- a/drivers/staging/media/sunxi/cedrus/cedrus_dec.c > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c > @@ -22,6 +22,19 @@ > #include "cedrus_dec.h" > #include "cedrus_hw.h" > > +int cedrus_reference_index_find(struct vb2_queue *queue, > + struct vb2_buffer *vb2_buf, u64 timestamp) > +{ > + /* > + * Allow using the current capture buffer as reference, which can occur > + * for field-coded pictures. > + */ > + if (vb2_buf->timestamp == timestamp) > + return vb2_buf->index; > + else > + return vb2_find_timestamp(queue, timestamp, 0); > +} > + > void cedrus_device_run(void *priv) > { > struct cedrus_ctx *ctx = priv; > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_dec.h b/drivers/staging/media/sunxi/cedrus/cedrus_dec.h > index d1ae7903677b..8d0fc248220f 100644 > --- a/drivers/staging/media/sunxi/cedrus/cedrus_dec.h > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_dec.h > @@ -16,6 +16,8 @@ > #ifndef _CEDRUS_DEC_H_ > #define _CEDRUS_DEC_H_ > > +int cedrus_reference_index_find(struct vb2_queue *queue, > + struct vb2_buffer *vb2_buf, u64 timestamp); > void cedrus_device_run(void *priv); > > #endif > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c b/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c > index cb45fda9aaeb..81c66a8aa1ac 100644 > --- a/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c > @@ -10,6 +10,7 @@ > #include <media/videobuf2-dma-contig.h> > > #include "cedrus.h" > +#include "cedrus_dec.h" > #include "cedrus_hw.h" > #include "cedrus_regs.h" > > @@ -159,8 +160,8 @@ static void cedrus_mpeg2_setup(struct cedrus_ctx *ctx, struct cedrus_run *run) > cedrus_write(dev, VE_DEC_MPEG_PICBOUNDSIZE, reg); > > /* Forward and backward prediction reference buffers. */ > - forward_idx = vb2_find_timestamp(cap_q, > - slice_params->forward_ref_ts, 0); > + forward_idx = cedrus_reference_index_find(cap_q, &run->dst->vb2_buf, > + slice_params->forward_ref_ts); > > fwd_luma_addr = cedrus_dst_buf_addr(ctx, forward_idx, 0); > fwd_chroma_addr = cedrus_dst_buf_addr(ctx, forward_idx, 1); > @@ -168,8 +169,9 @@ static void cedrus_mpeg2_setup(struct cedrus_ctx *ctx, struct cedrus_run *run) > cedrus_write(dev, VE_DEC_MPEG_FWD_REF_LUMA_ADDR, fwd_luma_addr); > cedrus_write(dev, VE_DEC_MPEG_FWD_REF_CHROMA_ADDR, fwd_chroma_addr); > > - backward_idx = vb2_find_timestamp(cap_q, > - slice_params->backward_ref_ts, 0); > + backward_idx = cedrus_reference_index_find(cap_q, &run->dst->vb2_buf, > + slice_params->backward_ref_ts); > + > bwd_luma_addr = cedrus_dst_buf_addr(ctx, backward_idx, 0); > bwd_chroma_addr = cedrus_dst_buf_addr(ctx, backward_idx, 1); > >
Hi, On Wed, 2019-01-09 at 15:29 +0100, Hans Verkuil wrote: > On 01/09/19 15:19, Paul Kocialkowski wrote: > > It was reported that some cases of interleaved video decoding require > > using the current destination buffer as a reference. However, this is > > no longer possible after the move to vb2_find_timestamp because only > > dequeued and done buffers are considered. > > > > Add a helper in our driver that also considers the current destination > > buffer before resorting to vb2_find_timestamp and use it in MPEG-2. > > This patch looks good, but can you also add checks to handle the case > when no buffer with the given timestamp was found? Probably should be done > in a third patch. > > I suspect the driver will crash if an unknown timestamp is passed on to the > driver. I would really like to see that corner case fixed. You're totally right and I think that more generarlly, the current code flow is rather fragile whenever something wrong happens in our setup() codec op. I think we should at least have that op return an error code and properly deal with it when that occurs. I am planning on making a cleanup series to fix that. Before using timestamps, reference buffer index validation was done in std_validate and now it's fully up to the driver. I wonder if it would be feasible to bring something back in there since all stateless drivers will face the same issue. The conditions set in cedrus_reference_index_find seem generic enough for that, but we should check that std_validate is not called too early, when the queue is in a different state (and the buffer might not be dequeud or done yet). What do you think? Cheers, Paul > > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com> > > --- > > drivers/staging/media/sunxi/cedrus/cedrus_dec.c | 13 +++++++++++++ > > drivers/staging/media/sunxi/cedrus/cedrus_dec.h | 2 ++ > > drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c | 10 ++++++---- > > 3 files changed, 21 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_dec.c b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c > > index 443fb037e1cf..2c295286766c 100644 > > --- a/drivers/staging/media/sunxi/cedrus/cedrus_dec.c > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c > > @@ -22,6 +22,19 @@ > > #include "cedrus_dec.h" > > #include "cedrus_hw.h" > > > > +int cedrus_reference_index_find(struct vb2_queue *queue, > > + struct vb2_buffer *vb2_buf, u64 timestamp) > > +{ > > + /* > > + * Allow using the current capture buffer as reference, which can occur > > + * for field-coded pictures. > > + */ > > + if (vb2_buf->timestamp == timestamp) > > + return vb2_buf->index; > > + else > > + return vb2_find_timestamp(queue, timestamp, 0); > > +} > > + > > void cedrus_device_run(void *priv) > > { > > struct cedrus_ctx *ctx = priv; > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_dec.h b/drivers/staging/media/sunxi/cedrus/cedrus_dec.h > > index d1ae7903677b..8d0fc248220f 100644 > > --- a/drivers/staging/media/sunxi/cedrus/cedrus_dec.h > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_dec.h > > @@ -16,6 +16,8 @@ > > #ifndef _CEDRUS_DEC_H_ > > #define _CEDRUS_DEC_H_ > > > > +int cedrus_reference_index_find(struct vb2_queue *queue, > > + struct vb2_buffer *vb2_buf, u64 timestamp); > > void cedrus_device_run(void *priv); > > > > #endif > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c b/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c > > index cb45fda9aaeb..81c66a8aa1ac 100644 > > --- a/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c > > @@ -10,6 +10,7 @@ > > #include <media/videobuf2-dma-contig.h> > > > > #include "cedrus.h" > > +#include "cedrus_dec.h" > > #include "cedrus_hw.h" > > #include "cedrus_regs.h" > > > > @@ -159,8 +160,8 @@ static void cedrus_mpeg2_setup(struct cedrus_ctx *ctx, struct cedrus_run *run) > > cedrus_write(dev, VE_DEC_MPEG_PICBOUNDSIZE, reg); > > > > /* Forward and backward prediction reference buffers. */ > > - forward_idx = vb2_find_timestamp(cap_q, > > - slice_params->forward_ref_ts, 0); > > + forward_idx = cedrus_reference_index_find(cap_q, &run->dst->vb2_buf, > > + slice_params->forward_ref_ts); > > > > fwd_luma_addr = cedrus_dst_buf_addr(ctx, forward_idx, 0); > > fwd_chroma_addr = cedrus_dst_buf_addr(ctx, forward_idx, 1); > > @@ -168,8 +169,9 @@ static void cedrus_mpeg2_setup(struct cedrus_ctx *ctx, struct cedrus_run *run) > > cedrus_write(dev, VE_DEC_MPEG_FWD_REF_LUMA_ADDR, fwd_luma_addr); > > cedrus_write(dev, VE_DEC_MPEG_FWD_REF_CHROMA_ADDR, fwd_chroma_addr); > > > > - backward_idx = vb2_find_timestamp(cap_q, > > - slice_params->backward_ref_ts, 0); > > + backward_idx = cedrus_reference_index_find(cap_q, &run->dst->vb2_buf, > > + slice_params->backward_ref_ts); > > + > > bwd_luma_addr = cedrus_dst_buf_addr(ctx, backward_idx, 0); > > bwd_chroma_addr = cedrus_dst_buf_addr(ctx, backward_idx, 1); > > > >
On 01/09/19 15:42, Paul Kocialkowski wrote: > Hi, > > On Wed, 2019-01-09 at 15:29 +0100, Hans Verkuil wrote: >> On 01/09/19 15:19, Paul Kocialkowski wrote: >>> It was reported that some cases of interleaved video decoding require >>> using the current destination buffer as a reference. However, this is >>> no longer possible after the move to vb2_find_timestamp because only >>> dequeued and done buffers are considered. >>> >>> Add a helper in our driver that also considers the current destination >>> buffer before resorting to vb2_find_timestamp and use it in MPEG-2. >> >> This patch looks good, but can you also add checks to handle the case >> when no buffer with the given timestamp was found? Probably should be done >> in a third patch. >> >> I suspect the driver will crash if an unknown timestamp is passed on to the >> driver. I would really like to see that corner case fixed. > > You're totally right and I think that more generarlly, the current code > flow is rather fragile whenever something wrong happens in our setup() > codec op. I think we should at least have that op return an error code > and properly deal with it when that occurs. > > I am planning on making a cleanup series to fix that. > > Before using timestamps, reference buffer index validation was done in > std_validate and now it's fully up to the driver. I wonder if it would > be feasible to bring something back in there since all stateless > drivers will face the same issue. The conditions set in > cedrus_reference_index_find seem generic enough for that, but we should > check that std_validate is not called too early, when the queue is in a > different state (and the buffer might not be dequeud or done yet). > > What do you think? I don't think you can do that. Say you queue a buffer that refers to a ref frame F, and F is a buffer that's been dequeued. When std_validate is called, this is fine and valid. Now later (but before the request is processed) buffer F is queued by the application. Now F is no longer valid. So when the driver processes the request, it won't be able to find F anymore. A more interesting question is what you should do if a reference frame isn't found. And should that be documented in the stateless decoder spec? (Or perhaps it's there already, I'm not sure). Regards, Hans > > Cheers, > > Paul > >>> Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com> >>> --- >>> drivers/staging/media/sunxi/cedrus/cedrus_dec.c | 13 +++++++++++++ >>> drivers/staging/media/sunxi/cedrus/cedrus_dec.h | 2 ++ >>> drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c | 10 ++++++---- >>> 3 files changed, 21 insertions(+), 4 deletions(-) >>> >>> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_dec.c b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c >>> index 443fb037e1cf..2c295286766c 100644 >>> --- a/drivers/staging/media/sunxi/cedrus/cedrus_dec.c >>> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c >>> @@ -22,6 +22,19 @@ >>> #include "cedrus_dec.h" >>> #include "cedrus_hw.h" >>> >>> +int cedrus_reference_index_find(struct vb2_queue *queue, >>> + struct vb2_buffer *vb2_buf, u64 timestamp) >>> +{ >>> + /* >>> + * Allow using the current capture buffer as reference, which can occur >>> + * for field-coded pictures. >>> + */ >>> + if (vb2_buf->timestamp == timestamp) >>> + return vb2_buf->index; >>> + else >>> + return vb2_find_timestamp(queue, timestamp, 0); >>> +} >>> + >>> void cedrus_device_run(void *priv) >>> { >>> struct cedrus_ctx *ctx = priv; >>> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_dec.h b/drivers/staging/media/sunxi/cedrus/cedrus_dec.h >>> index d1ae7903677b..8d0fc248220f 100644 >>> --- a/drivers/staging/media/sunxi/cedrus/cedrus_dec.h >>> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_dec.h >>> @@ -16,6 +16,8 @@ >>> #ifndef _CEDRUS_DEC_H_ >>> #define _CEDRUS_DEC_H_ >>> >>> +int cedrus_reference_index_find(struct vb2_queue *queue, >>> + struct vb2_buffer *vb2_buf, u64 timestamp); >>> void cedrus_device_run(void *priv); >>> >>> #endif >>> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c b/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c >>> index cb45fda9aaeb..81c66a8aa1ac 100644 >>> --- a/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c >>> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c >>> @@ -10,6 +10,7 @@ >>> #include <media/videobuf2-dma-contig.h> >>> >>> #include "cedrus.h" >>> +#include "cedrus_dec.h" >>> #include "cedrus_hw.h" >>> #include "cedrus_regs.h" >>> >>> @@ -159,8 +160,8 @@ static void cedrus_mpeg2_setup(struct cedrus_ctx *ctx, struct cedrus_run *run) >>> cedrus_write(dev, VE_DEC_MPEG_PICBOUNDSIZE, reg); >>> >>> /* Forward and backward prediction reference buffers. */ >>> - forward_idx = vb2_find_timestamp(cap_q, >>> - slice_params->forward_ref_ts, 0); >>> + forward_idx = cedrus_reference_index_find(cap_q, &run->dst->vb2_buf, >>> + slice_params->forward_ref_ts); >>> >>> fwd_luma_addr = cedrus_dst_buf_addr(ctx, forward_idx, 0); >>> fwd_chroma_addr = cedrus_dst_buf_addr(ctx, forward_idx, 1); >>> @@ -168,8 +169,9 @@ static void cedrus_mpeg2_setup(struct cedrus_ctx *ctx, struct cedrus_run *run) >>> cedrus_write(dev, VE_DEC_MPEG_FWD_REF_LUMA_ADDR, fwd_luma_addr); >>> cedrus_write(dev, VE_DEC_MPEG_FWD_REF_CHROMA_ADDR, fwd_chroma_addr); >>> >>> - backward_idx = vb2_find_timestamp(cap_q, >>> - slice_params->backward_ref_ts, 0); >>> + backward_idx = cedrus_reference_index_find(cap_q, &run->dst->vb2_buf, >>> + slice_params->backward_ref_ts); >>> + >>> bwd_luma_addr = cedrus_dst_buf_addr(ctx, backward_idx, 0); >>> bwd_chroma_addr = cedrus_dst_buf_addr(ctx, backward_idx, 1); >>> >>>
diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_dec.c b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c index 443fb037e1cf..2c295286766c 100644 --- a/drivers/staging/media/sunxi/cedrus/cedrus_dec.c +++ b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c @@ -22,6 +22,19 @@ #include "cedrus_dec.h" #include "cedrus_hw.h" +int cedrus_reference_index_find(struct vb2_queue *queue, + struct vb2_buffer *vb2_buf, u64 timestamp) +{ + /* + * Allow using the current capture buffer as reference, which can occur + * for field-coded pictures. + */ + if (vb2_buf->timestamp == timestamp) + return vb2_buf->index; + else + return vb2_find_timestamp(queue, timestamp, 0); +} + void cedrus_device_run(void *priv) { struct cedrus_ctx *ctx = priv; diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_dec.h b/drivers/staging/media/sunxi/cedrus/cedrus_dec.h index d1ae7903677b..8d0fc248220f 100644 --- a/drivers/staging/media/sunxi/cedrus/cedrus_dec.h +++ b/drivers/staging/media/sunxi/cedrus/cedrus_dec.h @@ -16,6 +16,8 @@ #ifndef _CEDRUS_DEC_H_ #define _CEDRUS_DEC_H_ +int cedrus_reference_index_find(struct vb2_queue *queue, + struct vb2_buffer *vb2_buf, u64 timestamp); void cedrus_device_run(void *priv); #endif diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c b/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c index cb45fda9aaeb..81c66a8aa1ac 100644 --- a/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c +++ b/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c @@ -10,6 +10,7 @@ #include <media/videobuf2-dma-contig.h> #include "cedrus.h" +#include "cedrus_dec.h" #include "cedrus_hw.h" #include "cedrus_regs.h" @@ -159,8 +160,8 @@ static void cedrus_mpeg2_setup(struct cedrus_ctx *ctx, struct cedrus_run *run) cedrus_write(dev, VE_DEC_MPEG_PICBOUNDSIZE, reg); /* Forward and backward prediction reference buffers. */ - forward_idx = vb2_find_timestamp(cap_q, - slice_params->forward_ref_ts, 0); + forward_idx = cedrus_reference_index_find(cap_q, &run->dst->vb2_buf, + slice_params->forward_ref_ts); fwd_luma_addr = cedrus_dst_buf_addr(ctx, forward_idx, 0); fwd_chroma_addr = cedrus_dst_buf_addr(ctx, forward_idx, 1); @@ -168,8 +169,9 @@ static void cedrus_mpeg2_setup(struct cedrus_ctx *ctx, struct cedrus_run *run) cedrus_write(dev, VE_DEC_MPEG_FWD_REF_LUMA_ADDR, fwd_luma_addr); cedrus_write(dev, VE_DEC_MPEG_FWD_REF_CHROMA_ADDR, fwd_chroma_addr); - backward_idx = vb2_find_timestamp(cap_q, - slice_params->backward_ref_ts, 0); + backward_idx = cedrus_reference_index_find(cap_q, &run->dst->vb2_buf, + slice_params->backward_ref_ts); + bwd_luma_addr = cedrus_dst_buf_addr(ctx, backward_idx, 0); bwd_chroma_addr = cedrus_dst_buf_addr(ctx, backward_idx, 1);
It was reported that some cases of interleaved video decoding require using the current destination buffer as a reference. However, this is no longer possible after the move to vb2_find_timestamp because only dequeued and done buffers are considered. Add a helper in our driver that also considers the current destination buffer before resorting to vb2_find_timestamp and use it in MPEG-2. Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com> --- drivers/staging/media/sunxi/cedrus/cedrus_dec.c | 13 +++++++++++++ drivers/staging/media/sunxi/cedrus/cedrus_dec.h | 2 ++ drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c | 10 ++++++---- 3 files changed, 21 insertions(+), 4 deletions(-)