| Message ID | HE1PR06MB40116FEF3EBE4706E426A5FFAC610@HE1PR06MB4011.eurprd06.prod.outlook.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: hantro: H264 fixes and improvements | expand |
On Tue, 29 Oct 2019 01:24:47 +0000 Jonas Karlman <jonas@kwiboo.se> wrote: > From: Francois Buergisser <fbuergisser@chromium.org> > > The setting of the motion vectors usage and the setting of motion > vectors address are currently done under different conditions. > > When decoding pre-recorded videos, this results of leaving the motion > vectors address unset, resulting in faulty memory accesses. Fix it > by using the same condition everywhere, which matches the profiles > that support motion vectors. > > Fixes: dea0a82f3d22 ("media: hantro: Add support for H264 decoding on G1") > Signed-off-by: Francois Buergisser <fbuergisser@chromium.org> > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> > Signed-off-by: Jonas Karlman <jonas@kwiboo.se> Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com> Tested-by: Boris Brezillon <boris.brezillon@collabora.com> This fix should apply cleanly to media/fixes. Would be great to have it queued for 5.4-rc6 so we don't have to backport it manually. > --- > drivers/staging/media/hantro/hantro_g1_h264_dec.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/staging/media/hantro/hantro_g1_h264_dec.c b/drivers/staging/media/hantro/hantro_g1_h264_dec.c > index 29130946dea4..a1cb18680200 100644 > --- a/drivers/staging/media/hantro/hantro_g1_h264_dec.c > +++ b/drivers/staging/media/hantro/hantro_g1_h264_dec.c > @@ -35,7 +35,7 @@ static void set_params(struct hantro_ctx *ctx) > if (sps->flags & V4L2_H264_SPS_FLAG_MB_ADAPTIVE_FRAME_FIELD) > reg |= G1_REG_DEC_CTRL0_SEQ_MBAFF_E; > reg |= G1_REG_DEC_CTRL0_PICORD_COUNT_E; > - if (dec_param->nal_ref_idc) > + if (sps->profile_idc > 66 && dec_param->nal_ref_idc) > reg |= G1_REG_DEC_CTRL0_WRITE_MVS_E; > > if (!(sps->flags & V4L2_H264_SPS_FLAG_FRAME_MBS_ONLY) && > @@ -245,7 +245,7 @@ static void set_buffers(struct hantro_ctx *ctx) > vdpu_write_relaxed(vpu, dst_dma, G1_REG_ADDR_DST); > > /* Higher profiles require DMV buffer appended to reference frames. */ > - if (ctrls->sps->profile_idc > 66) { > + if (ctrls->sps->profile_idc > 66 && ctrls->decode->nal_ref_idc) { > size_t pic_size = ctx->h264_dec.pic_size; > size_t mv_offset = round_up(pic_size, 8); >
On Tue, 29 Oct 2019 at 02:25, Jonas Karlman <jonas@kwiboo.se> wrote: > > From: Francois Buergisser <fbuergisser@chromium.org> > > The setting of the motion vectors usage and the setting of motion > vectors address are currently done under different conditions. > > When decoding pre-recorded videos, this results of leaving the motion > vectors address unset, resulting in faulty memory accesses. Fix it > by using the same condition everywhere, which matches the profiles > that support motion vectors. > > Fixes: dea0a82f3d22 ("media: hantro: Add support for H264 decoding on G1") > Signed-off-by: Francois Buergisser <fbuergisser@chromium.org> > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> > Signed-off-by: Jonas Karlman <jonas@kwiboo.se> > --- > drivers/staging/media/hantro/hantro_g1_h264_dec.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/staging/media/hantro/hantro_g1_h264_dec.c b/drivers/staging/media/hantro/hantro_g1_h264_dec.c > index 29130946dea4..a1cb18680200 100644 > --- a/drivers/staging/media/hantro/hantro_g1_h264_dec.c > +++ b/drivers/staging/media/hantro/hantro_g1_h264_dec.c > @@ -35,7 +35,7 @@ static void set_params(struct hantro_ctx *ctx) > if (sps->flags & V4L2_H264_SPS_FLAG_MB_ADAPTIVE_FRAME_FIELD) > reg |= G1_REG_DEC_CTRL0_SEQ_MBAFF_E; > reg |= G1_REG_DEC_CTRL0_PICORD_COUNT_E; > - if (dec_param->nal_ref_idc) > + if (sps->profile_idc > 66 && dec_param->nal_ref_idc) > reg |= G1_REG_DEC_CTRL0_WRITE_MVS_E; > > if (!(sps->flags & V4L2_H264_SPS_FLAG_FRAME_MBS_ONLY) && > @@ -245,7 +245,7 @@ static void set_buffers(struct hantro_ctx *ctx) > vdpu_write_relaxed(vpu, dst_dma, G1_REG_ADDR_DST); > > /* Higher profiles require DMV buffer appended to reference frames. */ > - if (ctrls->sps->profile_idc > 66) { > + if (ctrls->sps->profile_idc > 66 && ctrls->decode->nal_ref_idc) { How about a one-line function (purposely not a macro, to have type-checking) ? I think this should emphasize the fact that the condition needs to be the same. Thanks, Ezequiel > size_t pic_size = ctx->h264_dec.pic_size; > size_t mv_offset = round_up(pic_size, 8); > > -- > 2.17.1 >
On Sun, 3 Nov 2019 at 00:09, Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> wrote: > > On Tue, 29 Oct 2019 at 02:25, Jonas Karlman <jonas@kwiboo.se> wrote: > > > > From: Francois Buergisser <fbuergisser@chromium.org> > > > > The setting of the motion vectors usage and the setting of motion > > vectors address are currently done under different conditions. > > > > When decoding pre-recorded videos, this results of leaving the motion > > vectors address unset, resulting in faulty memory accesses. Fix it > > by using the same condition everywhere, which matches the profiles > > that support motion vectors. > > > > Fixes: dea0a82f3d22 ("media: hantro: Add support for H264 decoding on G1") > > Signed-off-by: Francois Buergisser <fbuergisser@chromium.org> > > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> > > Signed-off-by: Jonas Karlman <jonas@kwiboo.se> > > --- > > drivers/staging/media/hantro/hantro_g1_h264_dec.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/staging/media/hantro/hantro_g1_h264_dec.c b/drivers/staging/media/hantro/hantro_g1_h264_dec.c > > index 29130946dea4..a1cb18680200 100644 > > --- a/drivers/staging/media/hantro/hantro_g1_h264_dec.c > > +++ b/drivers/staging/media/hantro/hantro_g1_h264_dec.c > > @@ -35,7 +35,7 @@ static void set_params(struct hantro_ctx *ctx) > > if (sps->flags & V4L2_H264_SPS_FLAG_MB_ADAPTIVE_FRAME_FIELD) > > reg |= G1_REG_DEC_CTRL0_SEQ_MBAFF_E; > > reg |= G1_REG_DEC_CTRL0_PICORD_COUNT_E; > > - if (dec_param->nal_ref_idc) > > + if (sps->profile_idc > 66 && dec_param->nal_ref_idc) > > reg |= G1_REG_DEC_CTRL0_WRITE_MVS_E; > > > > if (!(sps->flags & V4L2_H264_SPS_FLAG_FRAME_MBS_ONLY) && > > @@ -245,7 +245,7 @@ static void set_buffers(struct hantro_ctx *ctx) > > vdpu_write_relaxed(vpu, dst_dma, G1_REG_ADDR_DST); > > > > /* Higher profiles require DMV buffer appended to reference frames. */ > > - if (ctrls->sps->profile_idc > 66) { > > + if (ctrls->sps->profile_idc > 66 && ctrls->decode->nal_ref_idc) { > > How about a one-line function (purposely not a macro, > to have type-checking) ? I think this should emphasize the fact that > the condition > needs to be the same. > Oops, just saw the next patch. Nevermind.
diff --git a/drivers/staging/media/hantro/hantro_g1_h264_dec.c b/drivers/staging/media/hantro/hantro_g1_h264_dec.c index 29130946dea4..a1cb18680200 100644 --- a/drivers/staging/media/hantro/hantro_g1_h264_dec.c +++ b/drivers/staging/media/hantro/hantro_g1_h264_dec.c @@ -35,7 +35,7 @@ static void set_params(struct hantro_ctx *ctx) if (sps->flags & V4L2_H264_SPS_FLAG_MB_ADAPTIVE_FRAME_FIELD) reg |= G1_REG_DEC_CTRL0_SEQ_MBAFF_E; reg |= G1_REG_DEC_CTRL0_PICORD_COUNT_E; - if (dec_param->nal_ref_idc) + if (sps->profile_idc > 66 && dec_param->nal_ref_idc) reg |= G1_REG_DEC_CTRL0_WRITE_MVS_E; if (!(sps->flags & V4L2_H264_SPS_FLAG_FRAME_MBS_ONLY) && @@ -245,7 +245,7 @@ static void set_buffers(struct hantro_ctx *ctx) vdpu_write_relaxed(vpu, dst_dma, G1_REG_ADDR_DST); /* Higher profiles require DMV buffer appended to reference frames. */ - if (ctrls->sps->profile_idc > 66) { + if (ctrls->sps->profile_idc > 66 && ctrls->decode->nal_ref_idc) { size_t pic_size = ctx->h264_dec.pic_size; size_t mv_offset = round_up(pic_size, 8);