[linux-sunxi] Re: [PATCH 2/2] media: cedrus: Allow using the current dst buffer as reference
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 >>> --- >>> 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 >>> >>> #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 =
[linux-sunxi] Re: [PATCH 2/2] media: cedrus: Allow using the current dst buffer as reference
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 > > --- > > 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 > > > > #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, >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,
[linux-sunxi] Re: [PATCH 2/2] media: cedrus: Allow using the current dst buffer as reference
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 > --- > 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 > > #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, >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, >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); > > -- You received this message because you are subscribed to the Google Groups "linux-sunxi" group. To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.