| Message ID | 20220404163533.707508-1-p.zabel@pengutronix.de |
|---|---|
| State | New |
| Headers | show |
| Series | [1/7] media: coda: set output buffer bytesused to appease v4l2-compliance | expand |
Le lundi 04 avril 2022 à 18:35 +0200, Philipp Zabel a écrit : > The V4L2 specification states: > > "If the application sets this to 0 for an output stream, then bytesused > will be set to the size of the buffer (see the length field of this > struct) by the driver." > > Since we set allow_zero_bytesused, we have to handle this ourselves. > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com> > --- > drivers/media/platform/chips-media/coda-bit.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/media/platform/chips-media/coda-bit.c b/drivers/media/platform/chips-media/coda-bit.c > index c484c008ab02..705a179ea8f0 100644 > --- a/drivers/media/platform/chips-media/coda-bit.c > +++ b/drivers/media/platform/chips-media/coda-bit.c > @@ -381,6 +381,9 @@ void coda_fill_bitstream(struct coda_ctx *ctx, struct list_head *buffer_list) > /* Dump empty buffers */ > if (!vb2_get_plane_payload(&src_buf->vb2_buf, 0)) { > src_buf = v4l2_m2m_src_buf_remove(ctx->fh.m2m_ctx); > + vb2_set_plane_payload(&src_buf->vb2_buf, 0, > + vb2_plane_size(&src_buf->vb2_buf, > + 0)); > v4l2_m2m_buf_done(src_buf, VB2_BUF_STATE_DONE); > continue; > }
Le lundi 04 avril 2022 à 18:35 +0200, Philipp Zabel a écrit : > Use v4l2_disable_ioctl() to disable the VIDIOC_TRY_ENCODER_CMD and > VIDIOC_ENCODER_CMD ioctls on decoder video devices and the > VIDIOC_TRY_DECODER_CMD and VIDIOC_DECODER_CMD ioctls on encoder > video devices. > > This allows to drop the coda_try_encoder/decoder_cmd() functions > and to use v4l2_m2m_ioctl_try_encoder/decoder_cmd() directly. > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> Nice cleanup. Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com> > --- > .../media/platform/chips-media/coda-common.c | 38 ++++++------------- > 1 file changed, 12 insertions(+), 26 deletions(-) > > diff --git a/drivers/media/platform/chips-media/coda-common.c b/drivers/media/platform/chips-media/coda-common.c > index 6d2989504b33..dc75133b0ead 100644 > --- a/drivers/media/platform/chips-media/coda-common.c > +++ b/drivers/media/platform/chips-media/coda-common.c > @@ -1091,17 +1091,6 @@ static int coda_s_selection(struct file *file, void *fh, > } > } > > -static int coda_try_encoder_cmd(struct file *file, void *fh, > - struct v4l2_encoder_cmd *ec) > -{ > - struct coda_ctx *ctx = fh_to_ctx(fh); > - > - if (ctx->inst_type != CODA_INST_ENCODER) > - return -ENOTTY; > - > - return v4l2_m2m_ioctl_try_encoder_cmd(file, fh, ec); > -} > - > static void coda_wake_up_capture_queue(struct coda_ctx *ctx) > { > struct vb2_queue *dst_vq; > @@ -1120,7 +1109,7 @@ static int coda_encoder_cmd(struct file *file, void *fh, > struct vb2_v4l2_buffer *buf; > int ret; > > - ret = coda_try_encoder_cmd(file, fh, ec); > + ret = v4l2_m2m_ioctl_try_encoder_cmd(file, fh, ec); > if (ret < 0) > return ret; > > @@ -1149,17 +1138,6 @@ static int coda_encoder_cmd(struct file *file, void *fh, > return 0; > } > > -static int coda_try_decoder_cmd(struct file *file, void *fh, > - struct v4l2_decoder_cmd *dc) > -{ > - struct coda_ctx *ctx = fh_to_ctx(fh); > - > - if (ctx->inst_type != CODA_INST_DECODER) > - return -ENOTTY; > - > - return v4l2_m2m_ioctl_try_decoder_cmd(file, fh, dc); > -} > - > static bool coda_mark_last_meta(struct coda_ctx *ctx) > { > struct coda_buffer_meta *meta; > @@ -1216,7 +1194,7 @@ static int coda_decoder_cmd(struct file *file, void *fh, > bool wakeup; > int ret; > > - ret = coda_try_decoder_cmd(file, fh, dc); > + ret = v4l2_m2m_ioctl_try_decoder_cmd(file, fh, dc); > if (ret < 0) > return ret; > > @@ -1498,9 +1476,9 @@ static const struct v4l2_ioctl_ops coda_ioctl_ops = { > .vidioc_g_selection = coda_g_selection, > .vidioc_s_selection = coda_s_selection, > > - .vidioc_try_encoder_cmd = coda_try_encoder_cmd, > + .vidioc_try_encoder_cmd = v4l2_m2m_ioctl_try_encoder_cmd, > .vidioc_encoder_cmd = coda_encoder_cmd, > - .vidioc_try_decoder_cmd = coda_try_decoder_cmd, > + .vidioc_try_decoder_cmd = v4l2_m2m_ioctl_try_decoder_cmd, > .vidioc_decoder_cmd = coda_decoder_cmd, > > .vidioc_g_parm = coda_g_parm, > @@ -2904,6 +2882,14 @@ static int coda_register_device(struct coda_dev *dev, int i) > v4l2_disable_ioctl(vfd, VIDIOC_G_CROP); > v4l2_disable_ioctl(vfd, VIDIOC_S_CROP); > > + if (dev->devtype->vdevs[i]->type == CODA_INST_ENCODER) { > + v4l2_disable_ioctl(vfd, VIDIOC_DECODER_CMD); > + v4l2_disable_ioctl(vfd, VIDIOC_TRY_DECODER_CMD); > + } else { > + v4l2_disable_ioctl(vfd, VIDIOC_ENCODER_CMD); > + v4l2_disable_ioctl(vfd, VIDIOC_TRY_ENCODER_CMD); > + } > + > ret = video_register_device(vfd, VFL_TYPE_VIDEO, 0); > if (!ret) > v4l2_info(&dev->v4l2_dev, "%s registered as %s\n",
Le lundi 04 avril 2022 à 18:35 +0200, Philipp Zabel a écrit : > Set default colorspace to SRGB for JPEG encoder and decoder devices, > to fix the following v4l2-compliance test failure: > > test VIDIOC_TRY_FMT: OK > fail: v4l2-test-formats.cpp(818): fmt_raw.g_colorspace() != V4L2_COLORSPACE_SRGB > > Also explicitly set transfer function, YCbCr encoding and quantization > range, as required by v4l2-compliance for the JPEG encoded side. Note that this will perhaps hit some GStreamer bugs that are being discussed. Documenting for the users: https://gitlab.freedesktop.org/gstreamer/gstreamer/-/issues/1118 https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/1406 Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com> > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> > --- > .../media/platform/chips-media/coda-common.c | 36 +++++++++++++------ > 1 file changed, 25 insertions(+), 11 deletions(-) > > diff --git a/drivers/media/platform/chips-media/coda-common.c b/drivers/media/platform/chips-media/coda-common.c > index 4a7346ed771e..c068c16d1eb4 100644 > --- a/drivers/media/platform/chips-media/coda-common.c > +++ b/drivers/media/platform/chips-media/coda-common.c > @@ -732,13 +732,22 @@ static int coda_try_fmt_vid_cap(struct file *file, void *priv, > return 0; > } > > -static void coda_set_default_colorspace(struct v4l2_pix_format *fmt) > +static void coda_set_default_colorspace(struct coda_ctx *ctx, > + struct v4l2_pix_format *fmt) > { > enum v4l2_colorspace colorspace; > > - if (fmt->pixelformat == V4L2_PIX_FMT_JPEG) > - colorspace = V4L2_COLORSPACE_JPEG; > - else if (fmt->width <= 720 && fmt->height <= 576) > + if (ctx->cvd->src_formats[0] == V4L2_PIX_FMT_JPEG || > + ctx->cvd->dst_formats[0] == V4L2_PIX_FMT_JPEG || > + fmt->pixelformat == V4L2_PIX_FMT_JPEG) { > + fmt->colorspace = V4L2_COLORSPACE_SRGB; > + fmt->xfer_func = V4L2_XFER_FUNC_SRGB; > + fmt->ycbcr_enc = V4L2_YCBCR_ENC_601; > + fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE; > + return; > + } > + > + if (fmt->width <= 720 && fmt->height <= 576) > colorspace = V4L2_COLORSPACE_SMPTE170M; > else > colorspace = V4L2_COLORSPACE_REC709; > @@ -763,7 +772,7 @@ static int coda_try_fmt_vid_out(struct file *file, void *priv, > return ret; > > if (f->fmt.pix.colorspace == V4L2_COLORSPACE_DEFAULT) > - coda_set_default_colorspace(&f->fmt.pix); > + coda_set_default_colorspace(ctx, &f->fmt.pix); > > q_data_dst = get_q_data(ctx, V4L2_BUF_TYPE_VIDEO_CAPTURE); > codec = coda_find_codec(dev, f->fmt.pix.pixelformat, q_data_dst->fourcc); > @@ -1640,13 +1649,18 @@ static void set_default_params(struct coda_ctx *ctx) > csize = coda_estimate_sizeimage(ctx, usize, max_w, max_h); > > ctx->params.codec_mode = ctx->codec->mode; > - if (ctx->cvd->src_formats[0] == V4L2_PIX_FMT_JPEG) > - ctx->colorspace = V4L2_COLORSPACE_JPEG; > - else > + if (ctx->cvd->src_formats[0] == V4L2_PIX_FMT_JPEG || > + ctx->cvd->dst_formats[0] == V4L2_PIX_FMT_JPEG) { > + ctx->colorspace = V4L2_COLORSPACE_SRGB; > + ctx->xfer_func = V4L2_XFER_FUNC_SRGB; > + ctx->ycbcr_enc = V4L2_YCBCR_ENC_601; > + ctx->quantization = V4L2_QUANTIZATION_FULL_RANGE; > + } else { > ctx->colorspace = V4L2_COLORSPACE_REC709; > - ctx->xfer_func = V4L2_XFER_FUNC_DEFAULT; > - ctx->ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT; > - ctx->quantization = V4L2_QUANTIZATION_DEFAULT; > + ctx->xfer_func = V4L2_XFER_FUNC_DEFAULT; > + ctx->ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT; > + ctx->quantization = V4L2_QUANTIZATION_DEFAULT; > + } > ctx->params.framerate = 30; > > /* Default formats for output and input queues */
Le lundi 04 avril 2022 à 18:35 +0200, Philipp Zabel a écrit : > Allow to call G_PARM with type == V4L2_BUF_TYPE_VIDEO_CAPTURE, > to fix the following v4l2-compliance test failure: > > fail: v4l2-test-formats.cpp(1344): ret && node->has_frmintervals > test VIDIOC_G/S_PARM: FAIL So basically the rate written in the bitstream (if any) will be the same as the target real-time rate, which matches my reading of the new spec as what default behaviour we should have. Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com> > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> > --- > drivers/media/platform/chips-media/coda-common.c | 3 --- > 1 file changed, 3 deletions(-) > > diff --git a/drivers/media/platform/chips-media/coda-common.c b/drivers/media/platform/chips-media/coda-common.c > index c068c16d1eb4..33fcd8c7d72b 100644 > --- a/drivers/media/platform/chips-media/coda-common.c > +++ b/drivers/media/platform/chips-media/coda-common.c > @@ -1341,9 +1341,6 @@ static int coda_g_parm(struct file *file, void *fh, struct v4l2_streamparm *a) > struct coda_ctx *ctx = fh_to_ctx(fh); > struct v4l2_fract *tpf; > > - if (a->type != V4L2_BUF_TYPE_VIDEO_OUTPUT) > - return -EINVAL; > - > a->parm.output.capability = V4L2_CAP_TIMEPERFRAME; > tpf = &a->parm.output.timeperframe; > tpf->denominator = ctx->params.framerate & CODA_FRATE_RES_MASK;
Hi Nicolas, thank you for the review. You bring up a good point here, I think this part of the spec is not very easy to understand. On Di, 2022-04-05 at 10:22 -0400, Nicolas Dufresne wrote: > Le lundi 04 avril 2022 à 18:35 +0200, Philipp Zabel a écrit : > > Allow to call S_PARM with type == V4L2_BUF_TYPE_VIDEO_OUTPUT, > > to fix the following v4l2-compliance test failure: > > > > fail: v4l2-test-formats.cpp(1413): got error 22 > > when setting parms for buftype 1 > > test VIDIOC_G/S_PARM: FAIL > > That one may be missing something though. As you don't implement performance > target, you need to override the value somehow with the value you wrote into the > bitstream no ? Otherwise we just ignore what userland sets silently ? You are right that we don't implement any performance targets. But the spec also says [1]: Changing the OUTPUT frame interval also sets the framerate that the encoder uses to encode the video. So setting the frame interval to 1/24 (or 24 frames per second) will produce a coded video stream that can be played back at that speed. The frame interval for the OUTPUT queue is just a hint, the application may provide raw frames at a different rate. It can be used by the driver to help schedule multiple encoders running in parallel. In the next step the CAPTURE frame interval can optionally be changed to a different value. This is useful for off-line encoding were the coded frame interval can be different from the rate at which raw frames are supplied. And Changing the CAPTURE frame interval sets the framerate for the coded video. It does not set the rate at which buffers arrive on the CAPTURE queue, that depends on how fast the encoder is and how fast raw frames are queued on the OUTPUT queue. [1] https://docs.kernel.org/userspace-api/media/v4l/dev-encoder.html?highlight=stateful%20encoder So far I'm only implementing the OUTPUT->CAPTURE rate passthrough part to make v4l2-compliance happy. Since the "frame interval for the OUTPUT queue is just a hint" and the spec only says that "the encoder may adjust it to match hardware requirements", I felt free to just ignore the raw frame interval part for now. My understanding is that the driver may limit this value to the achievable real time encoding speed (if it even knows). One thing I'm not doing according to spec right now is that calling S_PARM on CAPTURE will also change the OUTPUT interval, as the driver just stores them in the same variable. Also the driver does not set the V4L2_FMT_FLAG_ENC_CAP_FRAME_INTERVAL to signal it supports S_PARM on CAPTURE. My understanding is that the CAPTURE frame interval is the value that should be written to the bitstream and that should be used for the bitrate control algorithm. > I might not have got exactly how this case is supposed to be handled. > Looking for feedback on what is proper behaviour for drivers that do > not implement performance targets (resource reservation). Same here. I'd love to learn whether my understanding of the spec is correct or not. regards Philipp
Le mardi 05 avril 2022 à 17:03 +0200, Philipp Zabel a écrit : > Hi Nicolas, > > thank you for the review. You bring up a good point here, I think this > part of the spec is not very easy to understand. > > On Di, 2022-04-05 at 10:22 -0400, Nicolas Dufresne wrote: > > Le lundi 04 avril 2022 à 18:35 +0200, Philipp Zabel a écrit : > > > Allow to call S_PARM with type == V4L2_BUF_TYPE_VIDEO_OUTPUT, > > > to fix the following v4l2-compliance test failure: > > > > > > fail: v4l2-test-formats.cpp(1413): got error 22 > > > when setting parms for buftype 1 > > > test VIDIOC_G/S_PARM: FAIL > > > > That one may be missing something though. As you don't implement performance > > target, you need to override the value somehow with the value you wrote into the > > bitstream no ? Otherwise we just ignore what userland sets silently ? > > You are right that we don't implement any performance targets. > But the spec also says [1]: > > Changing the OUTPUT frame interval also sets the framerate that the > encoder uses to encode the video. So setting the frame interval to > 1/24 (or 24 frames per second) will produce a coded video stream that > can be played back at that speed. The frame interval for the OUTPUT > queue is just a hint, the application may provide raw frames at a > different rate. It can be used by the driver to help schedule > multiple encoders running in parallel. > > In the next step the CAPTURE frame interval can optionally be changed > to a different value. This is useful for off-line encoding were the > coded frame interval can be different from the rate at which raw > frames are supplied. > > And > > Changing the CAPTURE frame interval sets the framerate for the coded > video. It does not set the rate at which buffers arrive on the > CAPTURE queue, that depends on how fast the encoder is and how fast > raw frames are queued on the OUTPUT queue. > > [1] https://docs.kernel.org/userspace-api/media/v4l/dev-encoder.html?highlight=stateful%20encoder > > So far I'm only implementing the OUTPUT->CAPTURE rate passthrough part > to make v4l2-compliance happy. > > Since the "frame interval for the OUTPUT queue is just a hint" and the > spec only says that "the encoder may adjust it to match hardware > requirements", I felt free to just ignore the raw frame interval part > for now. > My understanding is that the driver may limit this value to the > achievable real time encoding speed (if it even knows). > > One thing I'm not doing according to spec right now is that calling > S_PARM on CAPTURE will also change the OUTPUT interval, as the driver > just stores them in the same variable. > Also the driver does not set the V4L2_FMT_FLAG_ENC_CAP_FRAME_INTERVAL > to signal it supports S_PARM on CAPTURE. > My understanding is that the CAPTURE frame interval is the value that > should be written to the bitstream and that should be used for the > bitrate control algorithm. This is another very good point, if a driver does not set V4L2_FMT_FLAG_ENC_CAP_FRAME_INTERVAL, why can't it return ENOTTY for S_PARAM(CAPTURE) ? Is the test even correct? > > > I might not have got exactly how this case is supposed to be handled. > > Looking for feedback on what is proper behaviour for drivers that do > > not implement performance targets (resource reservation). > > Same here. I'd love to learn whether my understanding of the spec is > correct or not. > > regards > Philipp
Hi Hans, On Do, 2022-04-21 at 11:44 +0200, Hans Verkuil wrote: > On 04/04/2022 18:35, Philipp Zabel wrote: > > The V4L2 specification states: > > > > "If the application sets this to 0 for an output stream, then bytesused > > will be set to the size of the buffer (see the length field of this > > struct) by the driver." > > > > Since we set allow_zero_bytesused, we have to handle this ourselves. > > > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> > > --- > > drivers/media/platform/chips-media/coda-bit.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/drivers/media/platform/chips-media/coda-bit.c b/drivers/media/platform/chips-media/coda-bit.c > > index c484c008ab02..705a179ea8f0 100644 > > --- a/drivers/media/platform/chips-media/coda-bit.c > > +++ b/drivers/media/platform/chips-media/coda-bit.c > > @@ -381,6 +381,9 @@ void coda_fill_bitstream(struct coda_ctx *ctx, struct list_head *buffer_list) > > /* Dump empty buffers */ > > if (!vb2_get_plane_payload(&src_buf->vb2_buf, 0)) { > > src_buf = v4l2_m2m_src_buf_remove(ctx->fh.m2m_ctx); > > + vb2_set_plane_payload(&src_buf->vb2_buf, 0, > > + vb2_plane_size(&src_buf->vb2_buf, > > + 0)); > > Would it be possible to stop using allow_zero_bytesused altogether? > > Are there still applications that rely on zero-sized output buffers to stop the > decoder? This was used by GStreamer 1.8. The code is still left in current versions, but is never executed unless the decoder stop command fails: https://gitlab.freedesktop.org/gstreamer/gstreamer/-/blob/main/subprojects/gst-plugins-good/sys/v4l2/gstv4l2videodec.c#L446-454 Whether there are still any applications using GStreamer 1.8 for V4L2 video decoding on devices that get kernel updates, I don't know. > I'm not actually sure that I want this in the driver, perhaps v4l2-compliance > can be modified to turn a fail into a warn if the driver is the coda driver. Same for nxp/imx-jpeg, qcom/venus and samsung/s5p-mfc? > Patching the driver is hiding the fact that the coda driver does something > non-standard for legacy reasons. It doesn't make sense either to change > bytesused to the buffer size since there really is nothing in the buffer. > > v4l2-compliance already has checks for two drivers, search for is_vivid and > is_uvcvideo. Ok. > I'm skipping this patch for now. regards Philipp
On 21/04/2022 12:24, Philipp Zabel wrote: > Hi Hans, > > On Do, 2022-04-21 at 11:44 +0200, Hans Verkuil wrote: >> On 04/04/2022 18:35, Philipp Zabel wrote: >>> The V4L2 specification states: >>> >>> "If the application sets this to 0 for an output stream, then bytesused >>> will be set to the size of the buffer (see the length field of this >>> struct) by the driver." >>> >>> Since we set allow_zero_bytesused, we have to handle this ourselves. >>> >>> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> >>> --- >>> drivers/media/platform/chips-media/coda-bit.c | 3 +++ >>> 1 file changed, 3 insertions(+) >>> >>> diff --git a/drivers/media/platform/chips-media/coda-bit.c b/drivers/media/platform/chips-media/coda-bit.c >>> index c484c008ab02..705a179ea8f0 100644 >>> --- a/drivers/media/platform/chips-media/coda-bit.c >>> +++ b/drivers/media/platform/chips-media/coda-bit.c >>> @@ -381,6 +381,9 @@ void coda_fill_bitstream(struct coda_ctx *ctx, struct list_head *buffer_list) >>> /* Dump empty buffers */ >>> if (!vb2_get_plane_payload(&src_buf->vb2_buf, 0)) { >>> src_buf = v4l2_m2m_src_buf_remove(ctx->fh.m2m_ctx); >>> + vb2_set_plane_payload(&src_buf->vb2_buf, 0, >>> + vb2_plane_size(&src_buf->vb2_buf, >>> + 0)); >> >> Would it be possible to stop using allow_zero_bytesused altogether? >> >> Are there still applications that rely on zero-sized output buffers to stop the >> decoder? > > This was used by GStreamer 1.8. The code is still left in current > versions, but is never executed unless the decoder stop command fails: > > https://gitlab.freedesktop.org/gstreamer/gstreamer/-/blob/main/subprojects/gst-plugins-good/sys/v4l2/gstv4l2videodec.c#L446-454 > > Whether there are still any applications using GStreamer 1.8 for V4L2 > video decoding on devices that get kernel updates, I don't know. > >> I'm not actually sure that I want this in the driver, perhaps v4l2-compliance >> can be modified to turn a fail into a warn if the driver is the coda driver. > > Same for nxp/imx-jpeg, qcom/venus and samsung/s5p-mfc? Yes for venus and s5p, but why would imx-jpeg use this? It makes no sense for a jpeg codec. I think it should just be removed for imx-jpeg. IMHO, once a decoder supports the STOP command, it should no longer set allow_zero_bytesused to true. But that decision is up to you for the coda driver. Regards, Hans > >> Patching the driver is hiding the fact that the coda driver does something >> non-standard for legacy reasons. It doesn't make sense either to change >> bytesused to the buffer size since there really is nothing in the buffer. >> >> v4l2-compliance already has checks for two drivers, search for is_vivid and >> is_uvcvideo. > > Ok. > >> I'm skipping this patch for now. > > regards > Philipp
On Di, 2022-04-26 at 07:52 +0200, Hans Verkuil wrote: [...] > > > Are there still applications that rely on zero-sized output buffers to stop the > > > decoder? > > > > This was used by GStreamer 1.8. The code is still left in current > > versions, but is never executed unless the decoder stop command fails: > > > > https://gitlab.freedesktop.org/gstreamer/gstreamer/-/blob/main/subprojects/gst-plugins-good/sys/v4l2/gstv4l2videodec.c#L446-454 > > > > Whether there are still any applications using GStreamer 1.8 for V4L2 > > video decoding on devices that get kernel updates, I don't know. > > > > > I'm not actually sure that I want this in the driver, perhaps v4l2-compliance > > > can be modified to turn a fail into a warn if the driver is the coda driver. > > > > Same for nxp/imx-jpeg, qcom/venus and samsung/s5p-mfc? > > Yes for venus and s5p, but why would imx-jpeg use this? > > It makes no sense for a jpeg codec. I think it should just be removed for imx-jpeg. > > IMHO, once a decoder supports the STOP command, it should no longer set > allow_zero_bytesused to true. But that decision is up to you for the coda > driver. Turns out there is no associated v4l2-compliance failure at all. I'd just drop this patch for now and keep the allow_zero_bytesused flag as-is. regards Philipp
diff --git a/drivers/media/platform/chips-media/coda-bit.c b/drivers/media/platform/chips-media/coda-bit.c index c484c008ab02..705a179ea8f0 100644 --- a/drivers/media/platform/chips-media/coda-bit.c +++ b/drivers/media/platform/chips-media/coda-bit.c @@ -381,6 +381,9 @@ void coda_fill_bitstream(struct coda_ctx *ctx, struct list_head *buffer_list) /* Dump empty buffers */ if (!vb2_get_plane_payload(&src_buf->vb2_buf, 0)) { src_buf = v4l2_m2m_src_buf_remove(ctx->fh.m2m_ctx); + vb2_set_plane_payload(&src_buf->vb2_buf, 0, + vb2_plane_size(&src_buf->vb2_buf, + 0)); v4l2_m2m_buf_done(src_buf, VB2_BUF_STATE_DONE); continue; }
The V4L2 specification states: "If the application sets this to 0 for an output stream, then bytesused will be set to the size of the buffer (see the length field of this struct) by the driver." Since we set allow_zero_bytesused, we have to handle this ourselves. Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> --- drivers/media/platform/chips-media/coda-bit.c | 3 +++ 1 file changed, 3 insertions(+)