| Message ID | 20211119074654.470729-1-wenst@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: hantro: Hook up RK3399 JPEG encoder output | expand |
Le vendredi 19 novembre 2021 à 15:46 +0800, Chen-Yu Tsai a écrit : > The JPEG encoder found in the Hantro H1 encoder block only produces a > raw entropy-encoded scan. The driver is responsible for building a JPEG > compliant bitstream and placing the entropy-encoded scan in it. Right > now the driver uses a bounce buffer for the hardware to output the raw > scan to. > > In commit e765dba11ec2 ("hantro: Move hantro_enc_buf_finish to JPEG > codec_ops.done"), the code that copies the raw scan from the bounce > buffer to the capture buffer was moved, but was only hooked up for the > Hantro H1 (then RK3288) variant. The RK3399 variant was broken, > producing a JPEG bitstream without the scan, and the capture buffer's > .bytesused field unset. > > Fix this by duplicating the code that is executed when the JPEG encoder > finishes encoding a frame. As the encoded length is read back from > hardware, and the variants having different register layouts, the > code is duplicated rather than shared. > > Fixes: e765dba11ec2 ("hantro: Move hantro_enc_buf_finish to JPEG codec_ops.done") > Signed-off-by: Chen-Yu Tsai <wenst@chromium.org> > --- > This was developed on the downstream ChromeOS 5.10 kernel (with a hack > for .data_offset) and tested with ChromeOS's jpeg_encode_accelerator_unittest > patched to accept non-JFIF JPEG streams (https://crrev.com/c/3291480). > > This was then forward-ported to mainline (name and filename changes) and > compile tested only. Tested with GStreamer on top of 5.16-rc1 from media_stage.git. Not perfect but at least the the output it valid. Test command was: gst-launch-1.0 videotestsrc num-buffers=2 ! v4l2jpegenc ! filesink location=test.jpg Notice that I encode two frames, it seems like the draining flow is broken in this driver. GStreamer will queue the frame and issue CMD_START immediately, the driver will skip the encode, leaving me with an empty file. Tested-by: Nicolas Dufresne <nicolas.dufresne@collabora.com> > > --- > .../staging/media/hantro/hantro_h1_jpeg_enc.c | 2 +- > drivers/staging/media/hantro/hantro_hw.h | 3 ++- > .../media/hantro/rockchip_vpu2_hw_jpeg_enc.c | 17 +++++++++++++++++ > drivers/staging/media/hantro/rockchip_vpu_hw.c | 5 +++-- > 4 files changed, 23 insertions(+), 4 deletions(-) > > diff --git a/drivers/staging/media/hantro/hantro_h1_jpeg_enc.c b/drivers/staging/media/hantro/hantro_h1_jpeg_enc.c > index 56cf261a8e95..9cd713c02a45 100644 > --- a/drivers/staging/media/hantro/hantro_h1_jpeg_enc.c > +++ b/drivers/staging/media/hantro/hantro_h1_jpeg_enc.c > @@ -140,7 +140,7 @@ int hantro_h1_jpeg_enc_run(struct hantro_ctx *ctx) > return 0; > } > > -void hantro_jpeg_enc_done(struct hantro_ctx *ctx) > +void hantro_h1_jpeg_enc_done(struct hantro_ctx *ctx) > { > struct hantro_dev *vpu = ctx->dev; > u32 bytesused = vepu_read(vpu, H1_REG_STR_BUF_LIMIT) / 8; > diff --git a/drivers/staging/media/hantro/hantro_hw.h b/drivers/staging/media/hantro/hantro_hw.h > index 267a6d33a47b..60d4602d33ed 100644 > --- a/drivers/staging/media/hantro/hantro_hw.h > +++ b/drivers/staging/media/hantro/hantro_hw.h > @@ -239,7 +239,8 @@ int hantro_h1_jpeg_enc_run(struct hantro_ctx *ctx); > int rockchip_vpu2_jpeg_enc_run(struct hantro_ctx *ctx); > int hantro_jpeg_enc_init(struct hantro_ctx *ctx); > void hantro_jpeg_enc_exit(struct hantro_ctx *ctx); > -void hantro_jpeg_enc_done(struct hantro_ctx *ctx); > +void hantro_h1_jpeg_enc_done(struct hantro_ctx *ctx); > +void rockchip_vpu2_jpeg_enc_done(struct hantro_ctx *ctx); > > dma_addr_t hantro_h264_get_ref_buf(struct hantro_ctx *ctx, > unsigned int dpb_idx); > diff --git a/drivers/staging/media/hantro/rockchip_vpu2_hw_jpeg_enc.c b/drivers/staging/media/hantro/rockchip_vpu2_hw_jpeg_enc.c > index 991213ce1610..5d9ff420f0b5 100644 > --- a/drivers/staging/media/hantro/rockchip_vpu2_hw_jpeg_enc.c > +++ b/drivers/staging/media/hantro/rockchip_vpu2_hw_jpeg_enc.c > @@ -171,3 +171,20 @@ int rockchip_vpu2_jpeg_enc_run(struct hantro_ctx *ctx) > > return 0; > } > + > +void rockchip_vpu2_jpeg_enc_done(struct hantro_ctx *ctx) > +{ > + struct hantro_dev *vpu = ctx->dev; > + u32 bytesused = vepu_read(vpu, VEPU_REG_STR_BUF_LIMIT) / 8; > + struct vb2_v4l2_buffer *dst_buf = hantro_get_dst_buf(ctx); > + > + /* > + * TODO: Rework the JPEG encoder to eliminate the need > + * for a bounce buffer. > + */ > + memcpy(vb2_plane_vaddr(&dst_buf->vb2_buf, 0) + > + ctx->vpu_dst_fmt->header_size, > + ctx->jpeg_enc.bounce_buffer.cpu, bytesused); > + vb2_set_plane_payload(&dst_buf->vb2_buf, 0, > + ctx->vpu_dst_fmt->header_size + bytesused); > +} > diff --git a/drivers/staging/media/hantro/rockchip_vpu_hw.c b/drivers/staging/media/hantro/rockchip_vpu_hw.c > index d4f52957cc53..0c22039162a0 100644 > --- a/drivers/staging/media/hantro/rockchip_vpu_hw.c > +++ b/drivers/staging/media/hantro/rockchip_vpu_hw.c > @@ -343,7 +343,7 @@ static const struct hantro_codec_ops rk3066_vpu_codec_ops[] = { > .run = hantro_h1_jpeg_enc_run, > .reset = rockchip_vpu1_enc_reset, > .init = hantro_jpeg_enc_init, > - .done = hantro_jpeg_enc_done, > + .done = hantro_h1_jpeg_enc_done, > .exit = hantro_jpeg_enc_exit, > }, > [HANTRO_MODE_H264_DEC] = { > @@ -371,7 +371,7 @@ static const struct hantro_codec_ops rk3288_vpu_codec_ops[] = { > .run = hantro_h1_jpeg_enc_run, > .reset = rockchip_vpu1_enc_reset, > .init = hantro_jpeg_enc_init, > - .done = hantro_jpeg_enc_done, > + .done = hantro_h1_jpeg_enc_done, > .exit = hantro_jpeg_enc_exit, > }, > [HANTRO_MODE_H264_DEC] = { > @@ -399,6 +399,7 @@ static const struct hantro_codec_ops rk3399_vpu_codec_ops[] = { > .run = rockchip_vpu2_jpeg_enc_run, > .reset = rockchip_vpu2_enc_reset, > .init = hantro_jpeg_enc_init, > + .done = rockchip_vpu2_jpeg_enc_done, > .exit = hantro_jpeg_enc_exit, > }, > [HANTRO_MODE_H264_DEC] = {
On Sat, Nov 20, 2021 at 12:00 AM Nicolas Dufresne <nicolas@ndufresne.ca> wrote: > > Le vendredi 19 novembre 2021 à 15:46 +0800, Chen-Yu Tsai a écrit : > > The JPEG encoder found in the Hantro H1 encoder block only produces a > > raw entropy-encoded scan. The driver is responsible for building a JPEG > > compliant bitstream and placing the entropy-encoded scan in it. Right > > now the driver uses a bounce buffer for the hardware to output the raw > > scan to. > > > > In commit e765dba11ec2 ("hantro: Move hantro_enc_buf_finish to JPEG > > codec_ops.done"), the code that copies the raw scan from the bounce > > buffer to the capture buffer was moved, but was only hooked up for the > > Hantro H1 (then RK3288) variant. The RK3399 variant was broken, > > producing a JPEG bitstream without the scan, and the capture buffer's > > .bytesused field unset. > > > > Fix this by duplicating the code that is executed when the JPEG encoder > > finishes encoding a frame. As the encoded length is read back from > > hardware, and the variants having different register layouts, the > > code is duplicated rather than shared. > > > > Fixes: e765dba11ec2 ("hantro: Move hantro_enc_buf_finish to JPEG codec_ops.done") > > Signed-off-by: Chen-Yu Tsai <wenst@chromium.org> > > --- > > This was developed on the downstream ChromeOS 5.10 kernel (with a hack > > for .data_offset) and tested with ChromeOS's jpeg_encode_accelerator_unittest > > patched to accept non-JFIF JPEG streams (https://crrev.com/c/3291480). > > > > This was then forward-ported to mainline (name and filename changes) and > > compile tested only. > > Tested with GStreamer on top of 5.16-rc1 from media_stage.git. Not perfect but > at least the the output it valid. Test command was: > > gst-launch-1.0 videotestsrc num-buffers=2 ! v4l2jpegenc ! filesink > location=test.jpg > > Notice that I encode two frames, it seems like the draining flow is broken in > this driver. GStreamer will queue the frame and issue CMD_START immediately, the > driver will skip the encode, leaving me with an empty file. The hantro driver doesn't implement ENC_CMD, which IIRC is used for the draining flow. I guess that's something to fix, since the mem2mem stateful encoder spec seems to require it. Or does that spec not apply to the JPEG encoders? > Tested-by: Nicolas Dufresne <nicolas.dufresne@collabora.com> Thanks! ChenYu
Le lundi 22 novembre 2021 à 11:57 +0800, Chen-Yu Tsai a écrit : > On Sat, Nov 20, 2021 at 12:00 AM Nicolas Dufresne <nicolas@ndufresne.ca> wrote: > > > > Le vendredi 19 novembre 2021 à 15:46 +0800, Chen-Yu Tsai a écrit : > > > The JPEG encoder found in the Hantro H1 encoder block only produces a > > > raw entropy-encoded scan. The driver is responsible for building a JPEG > > > compliant bitstream and placing the entropy-encoded scan in it. Right > > > now the driver uses a bounce buffer for the hardware to output the raw > > > scan to. > > > > > > In commit e765dba11ec2 ("hantro: Move hantro_enc_buf_finish to JPEG > > > codec_ops.done"), the code that copies the raw scan from the bounce > > > buffer to the capture buffer was moved, but was only hooked up for the > > > Hantro H1 (then RK3288) variant. The RK3399 variant was broken, > > > producing a JPEG bitstream without the scan, and the capture buffer's > > > .bytesused field unset. > > > > > > Fix this by duplicating the code that is executed when the JPEG encoder > > > finishes encoding a frame. As the encoded length is read back from > > > hardware, and the variants having different register layouts, the > > > code is duplicated rather than shared. > > > > > > Fixes: e765dba11ec2 ("hantro: Move hantro_enc_buf_finish to JPEG codec_ops.done") > > > Signed-off-by: Chen-Yu Tsai <wenst@chromium.org> > > > --- > > > This was developed on the downstream ChromeOS 5.10 kernel (with a hack > > > for .data_offset) and tested with ChromeOS's jpeg_encode_accelerator_unittest > > > patched to accept non-JFIF JPEG streams (https://crrev.com/c/3291480). > > > > > > This was then forward-ported to mainline (name and filename changes) and > > > compile tested only. > > > > Tested with GStreamer on top of 5.16-rc1 from media_stage.git. Not perfect but > > at least the the output it valid. Test command was: > > > > gst-launch-1.0 videotestsrc num-buffers=2 ! v4l2jpegenc ! filesink > > location=test.jpg > > > > Notice that I encode two frames, it seems like the draining flow is broken in > > this driver. GStreamer will queue the frame and issue CMD_START immediately, the > > driver will skip the encode, leaving me with an empty file. > > The hantro driver doesn't implement ENC_CMD, which IIRC is used for the > draining flow. I guess that's something to fix, since the mem2mem stateful > encoder spec seems to require it. Or does that spec not apply to the JPEG > encoders? I'm pretty sure its required. But this isn't a regression from this patch. > > > Tested-by: Nicolas Dufresne <nicolas.dufresne@collabora.com> > > Thanks! > > ChenYu
On 23/11/2021 20:39, Nicolas Dufresne wrote: > Le lundi 22 novembre 2021 à 11:57 +0800, Chen-Yu Tsai a écrit : >> On Sat, Nov 20, 2021 at 12:00 AM Nicolas Dufresne <nicolas@ndufresne.ca> wrote: >>> >>> Le vendredi 19 novembre 2021 à 15:46 +0800, Chen-Yu Tsai a écrit : >>>> The JPEG encoder found in the Hantro H1 encoder block only produces a >>>> raw entropy-encoded scan. The driver is responsible for building a JPEG >>>> compliant bitstream and placing the entropy-encoded scan in it. Right >>>> now the driver uses a bounce buffer for the hardware to output the raw >>>> scan to. >>>> >>>> In commit e765dba11ec2 ("hantro: Move hantro_enc_buf_finish to JPEG >>>> codec_ops.done"), the code that copies the raw scan from the bounce >>>> buffer to the capture buffer was moved, but was only hooked up for the >>>> Hantro H1 (then RK3288) variant. The RK3399 variant was broken, >>>> producing a JPEG bitstream without the scan, and the capture buffer's >>>> .bytesused field unset. >>>> >>>> Fix this by duplicating the code that is executed when the JPEG encoder >>>> finishes encoding a frame. As the encoded length is read back from >>>> hardware, and the variants having different register layouts, the >>>> code is duplicated rather than shared. >>>> >>>> Fixes: e765dba11ec2 ("hantro: Move hantro_enc_buf_finish to JPEG codec_ops.done") >>>> Signed-off-by: Chen-Yu Tsai <wenst@chromium.org> >>>> --- >>>> This was developed on the downstream ChromeOS 5.10 kernel (with a hack >>>> for .data_offset) and tested with ChromeOS's jpeg_encode_accelerator_unittest >>>> patched to accept non-JFIF JPEG streams (https://crrev.com/c/3291480). >>>> >>>> This was then forward-ported to mainline (name and filename changes) and >>>> compile tested only. >>> >>> Tested with GStreamer on top of 5.16-rc1 from media_stage.git. Not perfect but >>> at least the the output it valid. Test command was: >>> >>> gst-launch-1.0 videotestsrc num-buffers=2 ! v4l2jpegenc ! filesink >>> location=test.jpg >>> >>> Notice that I encode two frames, it seems like the draining flow is broken in >>> this driver. GStreamer will queue the frame and issue CMD_START immediately, the >>> driver will skip the encode, leaving me with an empty file. >> >> The hantro driver doesn't implement ENC_CMD, which IIRC is used for the >> draining flow. I guess that's something to fix, since the mem2mem stateful >> encoder spec seems to require it. Or does that spec not apply to the JPEG >> encoders? > > I'm pretty sure its required. But this isn't a regression from this patch. I don't think it is required for JPEG encoders today. Each frame is independent of any other, so it behaves just like a m2m scaler for example. It doesn't hurt to support it, but it shouldn't be necessary for jpeg codecs. If there is a reason why this is needed, then it would likely also be needed for m2m devices like scalers. Regards, Hans > >> >>> Tested-by: Nicolas Dufresne <nicolas.dufresne@collabora.com> >> >> Thanks! >> >> ChenYu >
Hi Chen-Yu, On Fri, Nov 19, 2021 at 03:46:54PM +0800, Chen-Yu Tsai wrote: > The JPEG encoder found in the Hantro H1 encoder block only produces a > raw entropy-encoded scan. The driver is responsible for building a JPEG > compliant bitstream and placing the entropy-encoded scan in it. Right > now the driver uses a bounce buffer for the hardware to output the raw > scan to. > > In commit e765dba11ec2 ("hantro: Move hantro_enc_buf_finish to JPEG > codec_ops.done"), the code that copies the raw scan from the bounce > buffer to the capture buffer was moved, but was only hooked up for the > Hantro H1 (then RK3288) variant. The RK3399 variant was broken, > producing a JPEG bitstream without the scan, and the capture buffer's > .bytesused field unset. > > Fix this by duplicating the code that is executed when the JPEG encoder > finishes encoding a frame. As the encoded length is read back from > hardware, and the variants having different register layouts, the > code is duplicated rather than shared. > > Fixes: e765dba11ec2 ("hantro: Move hantro_enc_buf_finish to JPEG codec_ops.done") > Signed-off-by: Chen-Yu Tsai <wenst@chromium.org> Thanks a lot for fixing this. Reviewed-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> > --- > This was developed on the downstream ChromeOS 5.10 kernel (with a hack > for .data_offset) and tested with ChromeOS's jpeg_encode_accelerator_unittest > patched to accept non-JFIF JPEG streams (https://crrev.com/c/3291480). > > This was then forward-ported to mainline (name and filename changes) and > compile tested only. > > --- > .../staging/media/hantro/hantro_h1_jpeg_enc.c | 2 +- > drivers/staging/media/hantro/hantro_hw.h | 3 ++- > .../media/hantro/rockchip_vpu2_hw_jpeg_enc.c | 17 +++++++++++++++++ > drivers/staging/media/hantro/rockchip_vpu_hw.c | 5 +++-- > 4 files changed, 23 insertions(+), 4 deletions(-) > > diff --git a/drivers/staging/media/hantro/hantro_h1_jpeg_enc.c b/drivers/staging/media/hantro/hantro_h1_jpeg_enc.c > index 56cf261a8e95..9cd713c02a45 100644 > --- a/drivers/staging/media/hantro/hantro_h1_jpeg_enc.c > +++ b/drivers/staging/media/hantro/hantro_h1_jpeg_enc.c > @@ -140,7 +140,7 @@ int hantro_h1_jpeg_enc_run(struct hantro_ctx *ctx) > return 0; > } > > -void hantro_jpeg_enc_done(struct hantro_ctx *ctx) > +void hantro_h1_jpeg_enc_done(struct hantro_ctx *ctx) > { > struct hantro_dev *vpu = ctx->dev; > u32 bytesused = vepu_read(vpu, H1_REG_STR_BUF_LIMIT) / 8; > diff --git a/drivers/staging/media/hantro/hantro_hw.h b/drivers/staging/media/hantro/hantro_hw.h > index 267a6d33a47b..60d4602d33ed 100644 > --- a/drivers/staging/media/hantro/hantro_hw.h > +++ b/drivers/staging/media/hantro/hantro_hw.h > @@ -239,7 +239,8 @@ int hantro_h1_jpeg_enc_run(struct hantro_ctx *ctx); > int rockchip_vpu2_jpeg_enc_run(struct hantro_ctx *ctx); > int hantro_jpeg_enc_init(struct hantro_ctx *ctx); > void hantro_jpeg_enc_exit(struct hantro_ctx *ctx); > -void hantro_jpeg_enc_done(struct hantro_ctx *ctx); > +void hantro_h1_jpeg_enc_done(struct hantro_ctx *ctx); > +void rockchip_vpu2_jpeg_enc_done(struct hantro_ctx *ctx); > > dma_addr_t hantro_h264_get_ref_buf(struct hantro_ctx *ctx, > unsigned int dpb_idx); > diff --git a/drivers/staging/media/hantro/rockchip_vpu2_hw_jpeg_enc.c b/drivers/staging/media/hantro/rockchip_vpu2_hw_jpeg_enc.c > index 991213ce1610..5d9ff420f0b5 100644 > --- a/drivers/staging/media/hantro/rockchip_vpu2_hw_jpeg_enc.c > +++ b/drivers/staging/media/hantro/rockchip_vpu2_hw_jpeg_enc.c > @@ -171,3 +171,20 @@ int rockchip_vpu2_jpeg_enc_run(struct hantro_ctx *ctx) > > return 0; > } > + > +void rockchip_vpu2_jpeg_enc_done(struct hantro_ctx *ctx) > +{ > + struct hantro_dev *vpu = ctx->dev; > + u32 bytesused = vepu_read(vpu, VEPU_REG_STR_BUF_LIMIT) / 8; > + struct vb2_v4l2_buffer *dst_buf = hantro_get_dst_buf(ctx); > + > + /* > + * TODO: Rework the JPEG encoder to eliminate the need > + * for a bounce buffer. > + */ > + memcpy(vb2_plane_vaddr(&dst_buf->vb2_buf, 0) + > + ctx->vpu_dst_fmt->header_size, > + ctx->jpeg_enc.bounce_buffer.cpu, bytesused); > + vb2_set_plane_payload(&dst_buf->vb2_buf, 0, > + ctx->vpu_dst_fmt->header_size + bytesused); > +} > diff --git a/drivers/staging/media/hantro/rockchip_vpu_hw.c b/drivers/staging/media/hantro/rockchip_vpu_hw.c > index d4f52957cc53..0c22039162a0 100644 > --- a/drivers/staging/media/hantro/rockchip_vpu_hw.c > +++ b/drivers/staging/media/hantro/rockchip_vpu_hw.c > @@ -343,7 +343,7 @@ static const struct hantro_codec_ops rk3066_vpu_codec_ops[] = { > .run = hantro_h1_jpeg_enc_run, > .reset = rockchip_vpu1_enc_reset, > .init = hantro_jpeg_enc_init, > - .done = hantro_jpeg_enc_done, > + .done = hantro_h1_jpeg_enc_done, > .exit = hantro_jpeg_enc_exit, > }, > [HANTRO_MODE_H264_DEC] = { > @@ -371,7 +371,7 @@ static const struct hantro_codec_ops rk3288_vpu_codec_ops[] = { > .run = hantro_h1_jpeg_enc_run, > .reset = rockchip_vpu1_enc_reset, > .init = hantro_jpeg_enc_init, > - .done = hantro_jpeg_enc_done, > + .done = hantro_h1_jpeg_enc_done, > .exit = hantro_jpeg_enc_exit, > }, > [HANTRO_MODE_H264_DEC] = { > @@ -399,6 +399,7 @@ static const struct hantro_codec_ops rk3399_vpu_codec_ops[] = { > .run = rockchip_vpu2_jpeg_enc_run, > .reset = rockchip_vpu2_enc_reset, > .init = hantro_jpeg_enc_init, > + .done = rockchip_vpu2_jpeg_enc_done, > .exit = hantro_jpeg_enc_exit, > }, > [HANTRO_MODE_H264_DEC] = { > -- > 2.34.0.rc2.393.gf8c9666880-goog >
Le jeudi 25 novembre 2021 à 09:44 +0100, Hans Verkuil a écrit : > On 23/11/2021 20:39, Nicolas Dufresne wrote: > > Le lundi 22 novembre 2021 à 11:57 +0800, Chen-Yu Tsai a écrit : > > > On Sat, Nov 20, 2021 at 12:00 AM Nicolas Dufresne <nicolas@ndufresne.ca> wrote: > > > > > > > > Le vendredi 19 novembre 2021 à 15:46 +0800, Chen-Yu Tsai a écrit : > > > > > The JPEG encoder found in the Hantro H1 encoder block only produces a > > > > > raw entropy-encoded scan. The driver is responsible for building a JPEG > > > > > compliant bitstream and placing the entropy-encoded scan in it. Right > > > > > now the driver uses a bounce buffer for the hardware to output the raw > > > > > scan to. > > > > > > > > > > In commit e765dba11ec2 ("hantro: Move hantro_enc_buf_finish to JPEG > > > > > codec_ops.done"), the code that copies the raw scan from the bounce > > > > > buffer to the capture buffer was moved, but was only hooked up for the > > > > > Hantro H1 (then RK3288) variant. The RK3399 variant was broken, > > > > > producing a JPEG bitstream without the scan, and the capture buffer's > > > > > .bytesused field unset. > > > > > > > > > > Fix this by duplicating the code that is executed when the JPEG encoder > > > > > finishes encoding a frame. As the encoded length is read back from > > > > > hardware, and the variants having different register layouts, the > > > > > code is duplicated rather than shared. > > > > > > > > > > Fixes: e765dba11ec2 ("hantro: Move hantro_enc_buf_finish to JPEG codec_ops.done") > > > > > Signed-off-by: Chen-Yu Tsai <wenst@chromium.org> > > > > > --- > > > > > This was developed on the downstream ChromeOS 5.10 kernel (with a hack > > > > > for .data_offset) and tested with ChromeOS's jpeg_encode_accelerator_unittest > > > > > patched to accept non-JFIF JPEG streams (https://crrev.com/c/3291480). > > > > > > > > > > This was then forward-ported to mainline (name and filename changes) and > > > > > compile tested only. > > > > > > > > Tested with GStreamer on top of 5.16-rc1 from media_stage.git. Not perfect but > > > > at least the the output it valid. Test command was: > > > > > > > > gst-launch-1.0 videotestsrc num-buffers=2 ! v4l2jpegenc ! filesink > > > > location=test.jpg > > > > > > > > Notice that I encode two frames, it seems like the draining flow is broken in > > > > this driver. GStreamer will queue the frame and issue CMD_START immediately, the > > > > driver will skip the encode, leaving me with an empty file. > > > > > > The hantro driver doesn't implement ENC_CMD, which IIRC is used for the > > > draining flow. I guess that's something to fix, since the mem2mem stateful > > > encoder spec seems to require it. Or does that spec not apply to the JPEG > > > encoders? > > > > I'm pretty sure its required. But this isn't a regression from this patch. > > I don't think it is required for JPEG encoders today. Each frame is independent of > any other, so it behaves just like a m2m scaler for example. > > It doesn't hurt to support it, but it shouldn't be necessary for jpeg codecs. I believe the requirement in the spec is deliberate to avoid making userland work a night mare. If draining a filled queue requires counting frames on top of using CMD_STOP, you are duplicating the effort. I have a strong opinion about this, I believe consistency is key, the frame work should provide this feature for JPEG decoder/encoder trivially imho and until then, driver should all support the same draining flow. I'm not going to relax this in GStreamer. Nicolas > > If there is a reason why this is needed, then it would likely also be needed for > m2m devices like scalers. > > Regards, > > Hans > > > > > > > > > > Tested-by: Nicolas Dufresne <nicolas.dufresne@collabora.com> > > > > > > Thanks! > > > > > > ChenYu > > >
diff --git a/drivers/staging/media/hantro/hantro_h1_jpeg_enc.c b/drivers/staging/media/hantro/hantro_h1_jpeg_enc.c index 56cf261a8e95..9cd713c02a45 100644 --- a/drivers/staging/media/hantro/hantro_h1_jpeg_enc.c +++ b/drivers/staging/media/hantro/hantro_h1_jpeg_enc.c @@ -140,7 +140,7 @@ int hantro_h1_jpeg_enc_run(struct hantro_ctx *ctx) return 0; } -void hantro_jpeg_enc_done(struct hantro_ctx *ctx) +void hantro_h1_jpeg_enc_done(struct hantro_ctx *ctx) { struct hantro_dev *vpu = ctx->dev; u32 bytesused = vepu_read(vpu, H1_REG_STR_BUF_LIMIT) / 8; diff --git a/drivers/staging/media/hantro/hantro_hw.h b/drivers/staging/media/hantro/hantro_hw.h index 267a6d33a47b..60d4602d33ed 100644 --- a/drivers/staging/media/hantro/hantro_hw.h +++ b/drivers/staging/media/hantro/hantro_hw.h @@ -239,7 +239,8 @@ int hantro_h1_jpeg_enc_run(struct hantro_ctx *ctx); int rockchip_vpu2_jpeg_enc_run(struct hantro_ctx *ctx); int hantro_jpeg_enc_init(struct hantro_ctx *ctx); void hantro_jpeg_enc_exit(struct hantro_ctx *ctx); -void hantro_jpeg_enc_done(struct hantro_ctx *ctx); +void hantro_h1_jpeg_enc_done(struct hantro_ctx *ctx); +void rockchip_vpu2_jpeg_enc_done(struct hantro_ctx *ctx); dma_addr_t hantro_h264_get_ref_buf(struct hantro_ctx *ctx, unsigned int dpb_idx); diff --git a/drivers/staging/media/hantro/rockchip_vpu2_hw_jpeg_enc.c b/drivers/staging/media/hantro/rockchip_vpu2_hw_jpeg_enc.c index 991213ce1610..5d9ff420f0b5 100644 --- a/drivers/staging/media/hantro/rockchip_vpu2_hw_jpeg_enc.c +++ b/drivers/staging/media/hantro/rockchip_vpu2_hw_jpeg_enc.c @@ -171,3 +171,20 @@ int rockchip_vpu2_jpeg_enc_run(struct hantro_ctx *ctx) return 0; } + +void rockchip_vpu2_jpeg_enc_done(struct hantro_ctx *ctx) +{ + struct hantro_dev *vpu = ctx->dev; + u32 bytesused = vepu_read(vpu, VEPU_REG_STR_BUF_LIMIT) / 8; + struct vb2_v4l2_buffer *dst_buf = hantro_get_dst_buf(ctx); + + /* + * TODO: Rework the JPEG encoder to eliminate the need + * for a bounce buffer. + */ + memcpy(vb2_plane_vaddr(&dst_buf->vb2_buf, 0) + + ctx->vpu_dst_fmt->header_size, + ctx->jpeg_enc.bounce_buffer.cpu, bytesused); + vb2_set_plane_payload(&dst_buf->vb2_buf, 0, + ctx->vpu_dst_fmt->header_size + bytesused); +} diff --git a/drivers/staging/media/hantro/rockchip_vpu_hw.c b/drivers/staging/media/hantro/rockchip_vpu_hw.c index d4f52957cc53..0c22039162a0 100644 --- a/drivers/staging/media/hantro/rockchip_vpu_hw.c +++ b/drivers/staging/media/hantro/rockchip_vpu_hw.c @@ -343,7 +343,7 @@ static const struct hantro_codec_ops rk3066_vpu_codec_ops[] = { .run = hantro_h1_jpeg_enc_run, .reset = rockchip_vpu1_enc_reset, .init = hantro_jpeg_enc_init, - .done = hantro_jpeg_enc_done, + .done = hantro_h1_jpeg_enc_done, .exit = hantro_jpeg_enc_exit, }, [HANTRO_MODE_H264_DEC] = { @@ -371,7 +371,7 @@ static const struct hantro_codec_ops rk3288_vpu_codec_ops[] = { .run = hantro_h1_jpeg_enc_run, .reset = rockchip_vpu1_enc_reset, .init = hantro_jpeg_enc_init, - .done = hantro_jpeg_enc_done, + .done = hantro_h1_jpeg_enc_done, .exit = hantro_jpeg_enc_exit, }, [HANTRO_MODE_H264_DEC] = { @@ -399,6 +399,7 @@ static const struct hantro_codec_ops rk3399_vpu_codec_ops[] = { .run = rockchip_vpu2_jpeg_enc_run, .reset = rockchip_vpu2_enc_reset, .init = hantro_jpeg_enc_init, + .done = rockchip_vpu2_jpeg_enc_done, .exit = hantro_jpeg_enc_exit, }, [HANTRO_MODE_H264_DEC] = {
The JPEG encoder found in the Hantro H1 encoder block only produces a raw entropy-encoded scan. The driver is responsible for building a JPEG compliant bitstream and placing the entropy-encoded scan in it. Right now the driver uses a bounce buffer for the hardware to output the raw scan to. In commit e765dba11ec2 ("hantro: Move hantro_enc_buf_finish to JPEG codec_ops.done"), the code that copies the raw scan from the bounce buffer to the capture buffer was moved, but was only hooked up for the Hantro H1 (then RK3288) variant. The RK3399 variant was broken, producing a JPEG bitstream without the scan, and the capture buffer's .bytesused field unset. Fix this by duplicating the code that is executed when the JPEG encoder finishes encoding a frame. As the encoded length is read back from hardware, and the variants having different register layouts, the code is duplicated rather than shared. Fixes: e765dba11ec2 ("hantro: Move hantro_enc_buf_finish to JPEG codec_ops.done") Signed-off-by: Chen-Yu Tsai <wenst@chromium.org> --- This was developed on the downstream ChromeOS 5.10 kernel (with a hack for .data_offset) and tested with ChromeOS's jpeg_encode_accelerator_unittest patched to accept non-JFIF JPEG streams (https://crrev.com/c/3291480). This was then forward-ported to mainline (name and filename changes) and compile tested only. --- .../staging/media/hantro/hantro_h1_jpeg_enc.c | 2 +- drivers/staging/media/hantro/hantro_hw.h | 3 ++- .../media/hantro/rockchip_vpu2_hw_jpeg_enc.c | 17 +++++++++++++++++ drivers/staging/media/hantro/rockchip_vpu_hw.c | 5 +++-- 4 files changed, 23 insertions(+), 4 deletions(-)