v4l2_m2m_buf_done is called in lat work queue, v4l2_ctrl_request_complete
is called in core queue. The request status of output queue will be set to
MEDIA_REQUEST_STATE_COMPLETE when v4l2_m2m_buf_done is called, leading to
output queue request complete fail. Must move v4l2_ctrl_request_complete
in front of v4l2_m2m_buf_done.
Patch 1 setting request complete before buffer done
Patch 2 change flush decode order when stream off
Patch 3 flush decoder before stream off
Patch 4 using input information to get vb2 buffer
Patch 5 store source vb2 buffer
Patch 6 replace v4l2_m2m_next_src_buf with v4l2_m2m_src_buf_remove
Patch 7 remove media request checking
---
compared with v3:
- fix flush decoder issue when userspace stream off capture queue firstly
- fluster test result same with v3
compared with v2:
- add patch 5/6/7 to fix decode again issue
- add fluster test result with mt8195 platform(same with no changed):
1> ./fluster.py run -d GStreamer-VP8-V4L2SL-Gst1.0 -j1 -t 90
VP8-TEST-VECTORS 59/61
2> ./fluster.py run -d GStreamer-VP9-V4L2SL-Gst1.0 -j1 -t 90
VP9-TEST-VECTORS 276/305
3> ./fluster.py run -d GStreamer-AV1-V4L2SL-Gst1.0 -j1 -t 90
AV1-TEST-VECTORS 237/239
4> ./fluster.py run -d GStreamer-H.264-V4L2SL-Gst1.0 -j1 -t 90
JVT-AVC_V1 95/135
5> ./fluster.py run -d GStreamer-H.265-V4L2SL-Gst1.0 -j1 -t 90
JCT-VC-HEVC_V1 142/147
compared with v1:
- add patch 2/3/4 to fix timing issue.
---
Yunfei Dong (7):
media: mediatek: vcodec: setting request complete before buffer done
media: mediatek: vcodec: change flush decode order when stream off
media: mediatek: vcodec: flush decoder before stream off
media: mediatek: vcodec: using input information to get vb2 buffer
media: mediatek: vcodec: store source vb2 buffer
media: mediatek: vcodec: replace v4l2_m2m_next_src_buf with
v4l2_m2m_src_buf_remove
media: mediatek: vcodec: remove media request checking
.../mediatek/vcodec/decoder/mtk_vcodec_dec.c | 44 ++++++++---------
.../vcodec/decoder/mtk_vcodec_dec_drv.h | 4 +-
.../vcodec/decoder/mtk_vcodec_dec_stateless.c | 48 ++++++++++++++-----
.../vcodec/decoder/vdec/vdec_av1_req_lat_if.c | 18 +++----
.../decoder/vdec/vdec_h264_req_multi_if.c | 4 +-
.../decoder/vdec/vdec_hevc_req_multi_if.c | 4 +-
.../vcodec/decoder/vdec/vdec_vp9_req_lat_if.c | 19 ++++----
.../mediatek/vcodec/decoder/vdec_msg_queue.h | 4 +-
8 files changed, 85 insertions(+), 60 deletions(-)
--
2.46.0
Need to set the buffer status to error if the media request of
each source buffer is NULL, then schedule the work to process
again in case of access NULL pointer.
Signed-off-by: Yunfei Dong <yunfei.dong@mediatek.com>
---
.../vcodec/decoder/mtk_vcodec_dec_stateless.c | 14 ++++++++------
1 file changed, 8 insertions(+), 6 deletions(-)
Flush decoder will reset the driver to flush status. If lat or core
work queue in active before flush when stream off, will lead to get
dst buffer NULL or buff done with one non-existent source buffer.
Flush decoder when stream off no matter output or capture queue
calling stream off firstly.
Signed-off-by: Yunfei Dong <yunfei.dong@mediatek.com>
---
.../mediatek/vcodec/decoder/mtk_vcodec_dec.c | 45 ++++++++++---------
1 file changed, 23 insertions(+), 22 deletions(-)
The buffer remove and buffer done of output queue is separated into
two works, the value of owned_by_drv_count isn't zero when output
queue stream off before flush decode.
Changing the flush decode from capture to output when stream off to
make sure all the output queue buffers are set to done list.
Signed-off-by: Yunfei Dong <yunfei.dong@mediatek.com>
---
.../mediatek/vcodec/decoder/mtk_vcodec_dec.c | 45 +++++++++----------
1 file changed, 22 insertions(+), 23 deletions(-)
vb2 buffer may be removed from ready list when lat try to get next
src buffer, leading to vb2 buffer not the current one. Need to get
vb2 buffer according to current input memory information.
Signed-off-by: Yunfei Dong <yunfei.dong@mediatek.com>
---
.../vcodec/decoder/vdec/vdec_av1_req_lat_if.c | 13 +++++++------
.../vcodec/decoder/vdec/vdec_vp9_req_lat_if.c | 15 +++++++--------
2 files changed, 14 insertions(+), 14 deletions(-)
There isn't lock to protect source buffer when get next src buffer,
if the source buffer is removed for some unknown reason before lat
work queue execute done, will lead to remove source buffer or buffer
done error.
Signed-off-by: Yunfei Dong <yunfei.dong@mediatek.com>
---
.../vcodec/decoder/mtk_vcodec_dec_stateless.c | 30 +++++++++++++------
1 file changed, 21 insertions(+), 9 deletions(-)
@@ -329,7 +330,7 @@ static void mtk_vdec_worker(struct work_struct *work)boolres_chg=false;intret;-vb2_v4l2_src=vb2_v4l2_src?vb2_v4l2_src:v4l2_m2m_next_src_buf(ctx->m2m_ctx);+vb2_v4l2_src=vb2_v4l2_src?vb2_v4l2_src:v4l2_m2m_src_buf_remove(ctx->m2m_ctx);if(!vb2_v4l2_src){v4l2_m2m_job_finish(dev->m2m_dev_dec,ctx->m2m_ctx);mtk_v4l2_vdec_dbg(1,ctx,"[%d] no available source buffer",ctx->id);
@@ -381,17 +382,28 @@ static void mtk_vdec_worker(struct work_struct *work)ctx->current_codec==V4L2_PIX_FMT_VP8_FRAME){if(src_buf_req)v4l2_ctrl_request_complete(src_buf_req,&ctx->ctrl_hdl);-v4l2_m2m_buf_done_and_job_finish(dev->m2m_dev_dec,ctx->m2m_ctx,state);-}else{-if(ret!=-EAGAIN){-v4l2_m2m_src_buf_remove(ctx->m2m_ctx);-ctx->last_vb2_v4l2_src=NULL;-}else{-ctx->last_vb2_v4l2_src=vb2_v4l2_src;-}+vb2_v4l2_dst=v4l2_m2m_dst_buf_remove(ctx->m2m_ctx);+v4l2_m2m_buf_done(vb2_v4l2_dst,state);+v4l2_m2m_buf_done(vb2_v4l2_src,state);v4l2_m2m_job_finish(dev->m2m_dev_dec,ctx->m2m_ctx);+return;}++/* If each codec return -EAGAIN to decode again, need to backup current source+*buffer,thenthedriverwillgetthisbuffernexttime.+*+*Ifeachcodecdecodeerror,musttosetbufferdonewitherrorstatusfor+*thisbufferhavebeenremovedfromreadylist.+*/+ctx->last_vb2_v4l2_src=(ret!=-EAGAIN)?NULL:vb2_v4l2_src;+if(ret&&ret!=-EAGAIN){+if(src_buf_req)+v4l2_ctrl_request_complete(src_buf_req,&ctx->ctrl_hdl);+v4l2_m2m_buf_done(vb2_v4l2_src,state);+}++v4l2_m2m_job_finish(dev->m2m_dev_dec,ctx->m2m_ctx);}staticvoidvb2ops_vdec_stateless_buf_queue(structvb2_buffer*vb)
Store the current vb2 buffer when lat need to decode again.
Then lat work can get the same vb2 buffer directly next time.
Signed-off-by: Yunfei Dong <yunfei.dong@mediatek.com>
---
.../mediatek/vcodec/decoder/mtk_vcodec_dec_drv.h | 2 ++
.../vcodec/decoder/mtk_vcodec_dec_stateless.c | 11 ++++++++---
2 files changed, 10 insertions(+), 3 deletions(-)
From: Nicolas Dufresne <nicolas.dufresne@collabora.com> Date: 2024-08-07 13:09:15
Hi Yunfei,
Le mercredi 07 août 2024 à 16:24 +0800, Yunfei Dong a écrit :
v4l2_m2m_buf_done is called in lat work queue, v4l2_ctrl_request_complete
is called in core queue. The request status of output queue will be set to
MEDIA_REQUEST_STATE_COMPLETE when v4l2_m2m_buf_done is called, leading to
output queue request complete fail. Must move v4l2_ctrl_request_complete
in front of v4l2_m2m_buf_done.
Sebastian and I have analyzed further the issue and the description here does
not seem to match. What happens is that in Stateless decoding, you have to set
header controls out-of-request to negotiate the format at first.
With VP9 notably, the header control is the only control there is. Chromium will
optimize out this and only attach a bitstream buffer to the request. So when the
buffer is mark to done, it is the last object in the request, which implicitly
mark the request as complete.
When v4l2_ctrl_request_complete() is later called, the control code detect that
there is no controls in the request. It then creates an empty control, but
attaching an object to a completed request is not allowed.
Patch 1 setting request complete before buffer done
Patch 2 change flush decode order when stream off
Patch 3 flush decoder before stream off
Patch 4 using input information to get vb2 buffer
Patch 5 store source vb2 buffer
Patch 6 replace v4l2_m2m_next_src_buf with v4l2_m2m_src_buf_remove
Patch 7 remove media request checking
I will give a some testing soon. Can you clarify on if the LAT and the CORE
still runs in parallel after this change ?
Nicolas
---
compared with v3:
- fix flush decoder issue when userspace stream off capture queue firstly
- fluster test result same with v3
compared with v2:
- add patch 5/6/7 to fix decode again issue
- add fluster test result with mt8195 platform(same with no changed):
1> ./fluster.py run -d GStreamer-VP8-V4L2SL-Gst1.0 -j1 -t 90
VP8-TEST-VECTORS 59/61
2> ./fluster.py run -d GStreamer-VP9-V4L2SL-Gst1.0 -j1 -t 90
VP9-TEST-VECTORS 276/305
3> ./fluster.py run -d GStreamer-AV1-V4L2SL-Gst1.0 -j1 -t 90
AV1-TEST-VECTORS 237/239
4> ./fluster.py run -d GStreamer-H.264-V4L2SL-Gst1.0 -j1 -t 90
JVT-AVC_V1 95/135
5> ./fluster.py run -d GStreamer-H.265-V4L2SL-Gst1.0 -j1 -t 90
JCT-VC-HEVC_V1 142/147
compared with v1:
- add patch 2/3/4 to fix timing issue.
---
Yunfei Dong (7):
media: mediatek: vcodec: setting request complete before buffer done
media: mediatek: vcodec: change flush decode order when stream off
media: mediatek: vcodec: flush decoder before stream off
media: mediatek: vcodec: using input information to get vb2 buffer
media: mediatek: vcodec: store source vb2 buffer
media: mediatek: vcodec: replace v4l2_m2m_next_src_buf with
v4l2_m2m_src_buf_remove
media: mediatek: vcodec: remove media request checking
.../mediatek/vcodec/decoder/mtk_vcodec_dec.c | 44 ++++++++---------
.../vcodec/decoder/mtk_vcodec_dec_drv.h | 4 +-
.../vcodec/decoder/mtk_vcodec_dec_stateless.c | 48 ++++++++++++++-----
.../vcodec/decoder/vdec/vdec_av1_req_lat_if.c | 18 +++----
.../decoder/vdec/vdec_h264_req_multi_if.c | 4 +-
.../decoder/vdec/vdec_hevc_req_multi_if.c | 4 +-
.../vcodec/decoder/vdec/vdec_vp9_req_lat_if.c | 19 ++++----
.../mediatek/vcodec/decoder/vdec_msg_queue.h | 4 +-
8 files changed, 85 insertions(+), 60 deletions(-)
Hi Nicolas,
Thanks for your advice.
On Wed, 2024-08-07 at 09:08 -0400, Nicolas Dufresne wrote:
quoted
Hi Yunfei,
Le mercredi 07 août 2024 à 16:24 +0800, Yunfei Dong a écrit :
quoted
v4l2_m2m_buf_done is called in lat work queue,
v4l2_ctrl_request_complete
is called in core queue. The request status of output queue will be
set to
MEDIA_REQUEST_STATE_COMPLETE when v4l2_m2m_buf_done is called,
leading to
output queue request complete fail. Must move
v4l2_ctrl_request_complete
in front of v4l2_m2m_buf_done.
Sebastian and I have analyzed further the issue and the description
here does
not seem to match. What happens is that in Stateless decoding, you
have to set
header controls out-of-request to negotiate the format at first.
With VP9 notably, the header control is the only control there is.
Chromium will
optimize out this and only attach a bitstream buffer to the request.
So when the
buffer is mark to done, it is the last object in the request, which
implicitly
mark the request as complete.
When v4l2_ctrl_request_complete() is later called, the control code
detect that
there is no controls in the request. It then creates an empty
control, but
attaching an object to a completed request is not allowed.
Whether I can write the commit message as below:
"User space will attach the syntaxes and bit-stream buffer to a media
request for stateless decoding, and the syntax controls are the only
v4l2 control request. The request will be marked to complete status
when the buffer is set to done, then request object will be cleaned
from media request.
When v4l2_ctrl_request_complete() is later called, the control request
detect that there is no controls in the request object. It then creates
an empty control request object, but attaching an object to a completed
request is not allowed."
quoted
Patch 1 setting request complete before buffer done
Patch 2 change flush decode order when stream off
Patch 3 flush decoder before stream off
Patch 4 using input information to get vb2 buffer
Patch 5 store source vb2 buffer
Patch 6 replace v4l2_m2m_next_src_buf with v4l2_m2m_src_buf_remove
Patch 7 remove media request checking
I will give a some testing soon. Can you clarify on if the LAT and
the CORE
Have you already helped to do the fluster test?
still runs in parallel after this change ?
Yes, the driver can work in parallel.
Could you please help to review other patches?
Nicolas
Best Regards,
Yunfei Dong
quoted
---
compared with v3:
- fix flush decoder issue when userspace stream off capture queue
firstly
- fluster test result same with v3
compared with v2:
- add patch 5/6/7 to fix decode again issue
- add fluster test result with mt8195 platform(same with no
changed):
1> ./fluster.py run -d GStreamer-VP8-V4L2SL-Gst1.0 -j1 -t 90
VP8-TEST-VECTORS 59/61
2> ./fluster.py run -d GStreamer-VP9-V4L2SL-Gst1.0 -j1 -t 90
VP9-TEST-VECTORS 276/305
3> ./fluster.py run -d GStreamer-AV1-V4L2SL-Gst1.0 -j1 -t 90
AV1-TEST-VECTORS 237/239
4> ./fluster.py run -d GStreamer-H.264-V4L2SL-Gst1.0 -j1 -t 90
JVT-AVC_V1 95/135
5> ./fluster.py run -d GStreamer-H.265-V4L2SL-Gst1.0 -j1 -t 90
JCT-VC-HEVC_V1 142/147
compared with v1:
- add patch 2/3/4 to fix timing issue.
---
Yunfei Dong (7):
media: mediatek: vcodec: setting request complete before buffer
done
media: mediatek: vcodec: change flush decode order when stream
off
media: mediatek: vcodec: flush decoder before stream off
media: mediatek: vcodec: using input information to get vb2
buffer
media: mediatek: vcodec: store source vb2 buffer
media: mediatek: vcodec: replace v4l2_m2m_next_src_buf with
v4l2_m2m_src_buf_remove
media: mediatek: vcodec: remove media request checking
.../mediatek/vcodec/decoder/mtk_vcodec_dec.c | 44 ++++++++-------
--
.../vcodec/decoder/mtk_vcodec_dec_drv.h | 4 +-
.../vcodec/decoder/mtk_vcodec_dec_stateless.c | 48 ++++++++++++++-
----
.../vcodec/decoder/vdec/vdec_av1_req_lat_if.c | 18 +++----
.../decoder/vdec/vdec_h264_req_multi_if.c | 4 +-
.../decoder/vdec/vdec_hevc_req_multi_if.c | 4 +-
.../vcodec/decoder/vdec/vdec_vp9_req_lat_if.c | 19 ++++----
.../mediatek/vcodec/decoder/vdec_msg_queue.h | 4 +-
8 files changed, 85 insertions(+), 60 deletions(-)
From: Sebastian Fricke <hidden> Date: 2024-08-22 15:33:04
Hey Yunfei,
On 07.08.2024 16:24, Yunfei Dong wrote:
The buffer remove and buffer done of output queue is separated into
two works, the value of owned_by_drv_count isn't zero when output
queue stream off before flush decode.
You have to try this again, I cannot make sense out of your message.
What do you mean with:
"The buffer remove and buffer done of output queue is separated into two works"
I suppose this:
the value of owned_by_drv_count isn't zero when output queue stream off
before flush decode.
should be:
therefore the value of `owned_by_drv_count` isn't zero while flushing
the decoder in the STREAMOFF(OUTPUT queue) IOCTL.
right?
Changing the flush decode from capture to output when stream off to
make sure all the output queue buffers are set to done list.
I'd change this section to:
Flushing the decoder during STREAMOFF(OUTPUT) instead of during
STREAMOFF(CAPTURE) makes sure that all buffers on the OUTPUT queue are
set done.
The rest looks fine.
Regards,
Sebastian Fricke
From: Sebastian Fricke <hidden> Date: 2024-08-22 16:12:03
Hey Yunfei,
On 07.08.2024 16:24, Yunfei Dong wrote:
quoted hunk
Flush decoder will reset the driver to flush status. If lat or core
work queue in active before flush when stream off, will lead to get
dst buffer NULL or buff done with one non-existent source buffer.
Flush decoder when stream off no matter output or capture queue
calling stream off firstly.
Signed-off-by: Yunfei Dong <yunfei.dong@mediatek.com>
---
.../mediatek/vcodec/decoder/mtk_vcodec_dec.c | 45 ++++++++++---------
1 file changed, 23 insertions(+), 22 deletions(-)
From: Sebastian Fricke <hidden> Date: 2024-08-23 12:40:54
Hey Yunfei,
I would rename the title to something like this:
media: mediatek: vcodec: Get SRC buffer from bitstream instead of M2M
On 07.08.2024 16:24, Yunfei Dong wrote:
vb2 buffer may be removed from ready list when lat try to get next
src buffer, leading to vb2 buffer not the current one. Need to get
vb2 buffer according to current input memory information.
And I would rewrite the commit log like this:
Getting the SRC buffer from the M2M buffer-queue risks picking a
different SRC buffer than the one used for the current decode operation.
Get the SRC buffer therefore from the bitstream data, which was set up
earlier during the decode.
Did I get that right?
Also could you explain why this change is required in this series?
Regards,
Sebastian Fricke
From: Sebastian Fricke <hidden> Date: 2024-08-23 14:15:14
Hey Yunfei,
On 07.08.2024 16:24, Yunfei Dong wrote:
Store the current vb2 buffer when lat need to decode again.
Then lat work can get the same vb2 buffer directly next time.
I would reword this with:
Store the current source buffer in the specific data for the IC. When
the LAT needs to retry a decode it can pick that buffer directly.
---
Additionally, this is not a good commit description as you just say what
you do, but you don't say WHY this needs to happen, why is it necessary?
What does it improve, is this a preparation patch for another, a fix for
something or an improvement of performance?
more below...
I think last is confusing in this context especially as there is for
example in the m2m_ctx:
* @last_src_buf: indicate the last source buffer for draining
* @next_buf_last: next capture queud buffer will be tagged as last
or:
* v4l2_m2m_last_buf() - return last buffer from the list of ready buffers
I think a better name would be:
* @cur_src_buf: current source buffer with the bitstream data for the latest decode
quoted hunk
* @is_flushing: set to true if flushing is in progress.
*
* @current_codec: current set input codec, in V4L2 pixel format
Please add a comment above this line that explains why this search can
be made, explaining why this buffer is still valid in this call and when
we pick the next source buffer.
Regards,
Sebastian Fricke
quoted hunk
if (!vb2_v4l2_src) {
v4l2_m2m_job_finish(dev->m2m_dev_dec, ctx->m2m_ctx);
mtk_v4l2_vdec_dbg(1, ctx, "[%d] no available source buffer", ctx->id);
From: Sebastian Fricke <hidden> Date: 2024-08-23 15:23:57
Hey Yunfei,
On 07.08.2024 16:24, Yunfei Dong wrote:
There isn't lock to protect source buffer when get next src buffer,
if the source buffer is removed for some unknown reason before lat
work queue execute done, will lead to remove source buffer or buffer
done error.
This is really hard to understand, can try wording this a bit clearer?
Stuff like: if the source buffer is removed ... will lead to remove
source buffer, just leaves me scratching my head.
And there is a spinlock in the m2m framework in `v4l2_m2m_next_buf` so I
suppose you mean something else when you say that there is no lock to
protect the source buffer?
You might not know all reasons but for this commit description you
should at least know one reason. Please highlight a case how this can
happen, so that you can justify the change.
This is another case where you just remove again completely what you
have added in the previous patch.
v4l2_m2m_job_finish(dev->m2m_dev_dec, ctx->m2m_ctx);
+ return;
}
+
+ /* If each codec return -EAGAIN to decode again, need to backup current source
+ * buffer, then the driver will get this buffer next time.
I would reword this like:
/* Store the current source buffer for the next attempt to decode,
* if this decode returned -EAGAIN */
quoted hunk
+ *+ * If each codec decode error, must to set buffer done with error status for+ * this buffer have been removed from ready list.+ */+ ctx->last_vb2_v4l2_src = (ret != -EAGAIN) ? NULL : vb2_v4l2_src;
Okay and here you add the same thing again as in the previous patch but
differently, this collection of commits feels more and more to me like a
work in progress. Please make sure in the future that each commit does
one job and does it completely.
It is not only confussing but also makes it hard to read the changes as
the bigger picture is missing in these tiny commits.
Please try to combine the patches where possible.
Regards,
Sebastian Fricke
From: Sebastian Fricke <hidden> Date: 2024-08-23 15:45:41
Hey Yunfei,
given this new series by Hans:
https://patchwork.linuxtv.org/project/linux-media/list/?series=13427
we might actually be able to find a more performant solution of the
problem, I'll work on that a bit and give you feedback.
Regards,
Sebastian
On 07.08.2024 16:24, Yunfei Dong wrote:
v4l2_m2m_buf_done is called in lat work queue, v4l2_ctrl_request_complete
is called in core queue. The request status of output queue will be set to
MEDIA_REQUEST_STATE_COMPLETE when v4l2_m2m_buf_done is called, leading to
output queue request complete fail. Must move v4l2_ctrl_request_complete
in front of v4l2_m2m_buf_done.
Patch 1 setting request complete before buffer done
Patch 2 change flush decode order when stream off
Patch 3 flush decoder before stream off
Patch 4 using input information to get vb2 buffer
Patch 5 store source vb2 buffer
Patch 6 replace v4l2_m2m_next_src_buf with v4l2_m2m_src_buf_remove
Patch 7 remove media request checking
---
compared with v3:
- fix flush decoder issue when userspace stream off capture queue firstly
- fluster test result same with v3
compared with v2:
- add patch 5/6/7 to fix decode again issue
- add fluster test result with mt8195 platform(same with no changed):
1> ./fluster.py run -d GStreamer-VP8-V4L2SL-Gst1.0 -j1 -t 90
VP8-TEST-VECTORS 59/61
2> ./fluster.py run -d GStreamer-VP9-V4L2SL-Gst1.0 -j1 -t 90
VP9-TEST-VECTORS 276/305
3> ./fluster.py run -d GStreamer-AV1-V4L2SL-Gst1.0 -j1 -t 90
AV1-TEST-VECTORS 237/239
4> ./fluster.py run -d GStreamer-H.264-V4L2SL-Gst1.0 -j1 -t 90
JVT-AVC_V1 95/135
5> ./fluster.py run -d GStreamer-H.265-V4L2SL-Gst1.0 -j1 -t 90
JCT-VC-HEVC_V1 142/147
compared with v1:
- add patch 2/3/4 to fix timing issue.
---
Yunfei Dong (7):
media: mediatek: vcodec: setting request complete before buffer done
media: mediatek: vcodec: change flush decode order when stream off
media: mediatek: vcodec: flush decoder before stream off
media: mediatek: vcodec: using input information to get vb2 buffer
media: mediatek: vcodec: store source vb2 buffer
media: mediatek: vcodec: replace v4l2_m2m_next_src_buf with
v4l2_m2m_src_buf_remove
media: mediatek: vcodec: remove media request checking
.../mediatek/vcodec/decoder/mtk_vcodec_dec.c | 44 ++++++++---------
.../vcodec/decoder/mtk_vcodec_dec_drv.h | 4 +-
.../vcodec/decoder/mtk_vcodec_dec_stateless.c | 48 ++++++++++++++-----
.../vcodec/decoder/vdec/vdec_av1_req_lat_if.c | 18 +++----
.../decoder/vdec/vdec_h264_req_multi_if.c | 4 +-
.../decoder/vdec/vdec_hevc_req_multi_if.c | 4 +-
.../vcodec/decoder/vdec/vdec_vp9_req_lat_if.c | 19 ++++----
.../mediatek/vcodec/decoder/vdec_msg_queue.h | 4 +-
8 files changed, 85 insertions(+), 60 deletions(-)
--
2.46.0
Hi Sebastian,
Thanks for your suggestion.
On Thu, 2024-08-22 at 18:11 +0200, Sebastian Fricke wrote:
Hey Yunfei,
On 07.08.2024 16:24, Yunfei Dong wrote:
quoted
Flush decoder will reset the driver to flush status. If lat or core
work queue in active before flush when stream off, will lead to get
dst buffer NULL or buff done with one non-existent source buffer.
Flush decoder when stream off no matter output or capture queue
calling stream off firstly.
Signed-off-by: Yunfei Dong <yunfei.dong@mediatek.com>
---
.../mediatek/vcodec/decoder/mtk_vcodec_dec.c | 45 ++++++++++----
-----
1 file changed, 23 insertions(+), 22 deletions(-)
diff --git
a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec.c
b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec.c
index 7080ca3e18b0..fc4ee1fb7cd1 100644
---
a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec.c
+++
b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec.c
you just changed this routine in patch 2/7, why was patch 2/7 needed
if
you remove it right away in the next patch?
Patch 2/7 is used to fix that ctrl request complete and buffer done are
separated into two works for output queue, flush decoder on capture
queue may lead to ctrl request complete fail when user space stop
output queue firstly.
Patch 3/7 is used to fix that user space may call stop capture or
output queue first randomly.
Need to combine these two patches together?
Best Regards,
Yunfei Dong
Hi Sebastian,
Thanks for your suggestion.
On Fri, 2024-08-23 at 14:40 +0200, Sebastian Fricke wrote:
Hey Yunfei,
I would rename the title to something like this:
media: mediatek: vcodec: Get SRC buffer from bitstream instead of M2M
On 07.08.2024 16:24, Yunfei Dong wrote:
quoted
vb2 buffer may be removed from ready list when lat try to get next
src buffer, leading to vb2 buffer not the current one. Need to get
vb2 buffer according to current input memory information.
And I would rewrite the commit log like this:
Getting the SRC buffer from the M2M buffer-queue risks picking a
different SRC buffer than the one used for the current decode
operation.
Get the SRC buffer therefore from the bitstream data, which was set
up
earlier during the decode.
Did I get that right?
Also could you explain why this change is required in this series?
Must make sure the src buffer is removed from ready list, in case of
the buffer is removed when core work try to set the buffer done.
Best Regards,
Yunfei Dong
Hi Sebastian,
Thanks for your suggestion.
On Fri, 2024-08-23 at 16:14 +0200, Sebastian Fricke wrote:
Hey Yunfei,
On 07.08.2024 16:24, Yunfei Dong wrote:
quoted
Store the current vb2 buffer when lat need to decode again.
Then lat work can get the same vb2 buffer directly next time.
I would reword this with:
Store the current source buffer in the specific data for the IC. When
the LAT needs to retry a decode it can pick that buffer directly.
---
Additionally, this is not a good commit description as you just say
what
you do, but you don't say WHY this needs to happen, why is it
necessary?
What does it improve, is this a preparation patch for another, a fix
for
something or an improvement of performance?
more below...
@@ -155,6 +155,7 @@ struct mtk_vcodec_dec_pdata { * @last_decoded_picinfo: pic information get from latest decode * @empty_flush_buf: a fake size-0 capture buffer that indicates
flush. Used
* for stateful decoder.
+ * @last_vb2_v4l2_src: the backup of current source buffer.
I think last is confusing in this context especially as there is for
example in the m2m_ctx:
* @last_src_buf: indicate the last source buffer for draining
* @next_buf_last: next capture queud buffer will be tagged as last
or:
* v4l2_m2m_last_buf() - return last buffer from the list of ready
buffers
I think a better name would be:
* @cur_src_buf: current source buffer with the bitstream data for
the latest decode
quoted
* @is_flushing: set to true if flushing is in progress.
*
* @current_codec: current set input codec, in V4L2 pixel format
Please add a comment above this line that explains why this search
can
be made, explaining why this buffer is still valid in this call and
when
we pick the next source buffer.
As the commit wrote, if the driver need to decoder again, need to use
the last remove source buffer to decode again, not to get one new.
Regards,
Yunfei Dong
Regards,
Sebastian Fricke
quoted
if (!vb2_v4l2_src) {
v4l2_m2m_job_finish(dev->m2m_dev_dec, ctx->m2m_ctx);
mtk_v4l2_vdec_dbg(1, ctx, "[%d] no available source
buffer", ctx->id);
Hi Sebastian,
Thanks for your suggestion.
On Fri, 2024-08-23 at 17:23 +0200, Sebastian Fricke wrote:
Hey Yunfei,
On 07.08.2024 16:24, Yunfei Dong wrote:
quoted
There isn't lock to protect source buffer when get next src buffer,
if the source buffer is removed for some unknown reason before lat
work queue execute done, will lead to remove source buffer or
buffer
done error.
This is really hard to understand, can try wording this a bit
clearer?
Stuff like: if the source buffer is removed ... will lead to remove
source buffer, just leaves me scratching my head.
And there is a spinlock in the m2m framework in `v4l2_m2m_next_buf`
so I
suppose you mean something else when you say that there is no lock to
protect the source buffer?
You might not know all reasons but for this commit description you
should at least know one reason. Please highlight a case how this can
happen, so that you can justify the change.
This is another case where you just remove again completely what you
have added in the previous patch.
quoted
v4l2_m2m_job_finish(dev->m2m_dev_dec, ctx->m2m_ctx);
+ return;
}
+
+ /* If each codec return -EAGAIN to decode again, need to backup
current source
+ * buffer, then the driver will get this buffer next time.
I would reword this like:
/* Store the current source buffer for the next attempt to
decode,
* if this decode returned -EAGAIN */
quoted
+ *
+ * If each codec decode error, must to set buffer done with
error status for
+ * this buffer have been removed from ready list.
+ */
+ ctx->last_vb2_v4l2_src = (ret != -EAGAIN) ? NULL :
vb2_v4l2_src;
Okay and here you add the same thing again as in the previous patch
but
differently, this collection of commits feels more and more to me
like a
work in progress. Please make sure in the future that each commit
does
one job and does it completely.
It is not only confussing but also makes it hard to read the changes
as
the bigger picture is missing in these tiny commits.
Please try to combine the patches where possible.
Path 5/7 is used to store the source buffer.
path 6/7 is used to handle decoder again when decoder error.
I will check whether it's possible to combine patch 5 and 6 together.
Best Regards,
Yunfei Dong
Regards,
Sebastian Fricke
quoted
+ if (ret && ret != -EAGAIN) {+ if (src_buf_req)+ v4l2_ctrl_request_complete(src_buf_req, &ctx-
we might actually be able to find a more performant solution of the
problem, I'll work on that a bit and give you feedback.
I already do so many test for different project with these patches,
whether it's possible to merge these patch firstly, then to call hans's
changes to do stress test.
Best Regards,
Yunfei Dong
Regards,
Sebastian
On 07.08.2024 16:24, Yunfei Dong wrote:
quoted
v4l2_m2m_buf_done is called in lat work queue,
v4l2_ctrl_request_complete
is called in core queue. The request status of output queue will be
set to
MEDIA_REQUEST_STATE_COMPLETE when v4l2_m2m_buf_done is called,
leading to
output queue request complete fail. Must move
v4l2_ctrl_request_complete
in front of v4l2_m2m_buf_done.
Patch 1 setting request complete before buffer done
Patch 2 change flush decode order when stream off
Patch 3 flush decoder before stream off
Patch 4 using input information to get vb2 buffer
Patch 5 store source vb2 buffer
Patch 6 replace v4l2_m2m_next_src_buf with v4l2_m2m_src_buf_remove
Patch 7 remove media request checking
---
compared with v3:
- fix flush decoder issue when userspace stream off capture queue
firstly
- fluster test result same with v3
compared with v2:
- add patch 5/6/7 to fix decode again issue
- add fluster test result with mt8195 platform(same with no
changed):
1> ./fluster.py run -d GStreamer-VP8-V4L2SL-Gst1.0 -j1 -t 90
VP8-TEST-VECTORS 59/61
2> ./fluster.py run -d GStreamer-VP9-V4L2SL-Gst1.0 -j1 -t 90
VP9-TEST-VECTORS 276/305
3> ./fluster.py run -d GStreamer-AV1-V4L2SL-Gst1.0 -j1 -t 90
AV1-TEST-VECTORS 237/239
4> ./fluster.py run -d GStreamer-H.264-V4L2SL-Gst1.0 -j1 -t 90
JVT-AVC_V1 95/135
5> ./fluster.py run -d GStreamer-H.265-V4L2SL-Gst1.0 -j1 -t 90
JCT-VC-HEVC_V1 142/147
compared with v1:
- add patch 2/3/4 to fix timing issue.
---
Yunfei Dong (7):
media: mediatek: vcodec: setting request complete before buffer
done
media: mediatek: vcodec: change flush decode order when stream off
media: mediatek: vcodec: flush decoder before stream off
media: mediatek: vcodec: using input information to get vb2 buffer
media: mediatek: vcodec: store source vb2 buffer
media: mediatek: vcodec: replace v4l2_m2m_next_src_buf with
v4l2_m2m_src_buf_remove
media: mediatek: vcodec: remove media request checking
.../mediatek/vcodec/decoder/mtk_vcodec_dec.c | 44 ++++++++------
---
.../vcodec/decoder/mtk_vcodec_dec_drv.h | 4 +-
.../vcodec/decoder/mtk_vcodec_dec_stateless.c | 48 ++++++++++++++
-----
.../vcodec/decoder/vdec/vdec_av1_req_lat_if.c | 18 +++----
.../decoder/vdec/vdec_h264_req_multi_if.c | 4 +-
.../decoder/vdec/vdec_hevc_req_multi_if.c | 4 +-
.../vcodec/decoder/vdec/vdec_vp9_req_lat_if.c | 19 ++++----
.../mediatek/vcodec/decoder/vdec_msg_queue.h | 4 +-
8 files changed, 85 insertions(+), 60 deletions(-)
--
2.46.0