From: Dafna Hirschfeld <dafna.hirschfeld@collabora.com> To: linux-media@vger.kernel.org Cc: laurent.pinchart@ideasonboard.com, dafna.hirschfeld@collabora.com, helen.koike@collabora.com, ezequiel@collabora.com, hverkuil@xs4all.nl, kernel@collabora.com, dafna3@gmail.com, sakari.ailus@linux.intel.com, linux-rockchip@lists.infradead.org, mchehab@kernel.org, tfiga@chromium.org Subject: [PATCH v2 00/14] media: staging: rkisp1: various bug fixes Date: Sat, 15 Aug 2020 12:37:20 +0200 [thread overview] Message-ID: <20200815103734.31153-1-dafna.hirschfeld@collabora.com> (raw) Fix various bugs mainly in params and stats. The patchset fixes buffers synchronization issues discussed https://patchwork.kernel.org/patch/11066513/#23544763 As discussed with Tomasz, we decide that in order to keep the code simple, we assume that the v-start signal (RKISP1_CIF_ISP_V_START) and the frame-out signal RKISP1_CIF_ISP_FRAME don't arrive together. So when v-starts arrives the frame sequence is incremented and when frame-out arrives, the stats and params isr are called. Also the frame sequence of the params buffer should be the frame to which the params are applied. The params node needs to apply the first params buffer when the streaming from main/selfpath starts before the first frame arrive so that the first buffer will configure the first frame. The way it is implemented is that the first buffer queued to the params node is not added to the list but instead a local copy is saved and the buffer returns to userspace immediately. Then the local copy is applied when main/selfpath stream starts. This implementation is buggy for the following reasons: - The first params buffer is applied and returned to userspace even if userspace never calls to streamon on the params node. - If the first params buffer is queued after the stream started on the params node then it will return to userspace but will never be used. - The frame_sequence of the first buffer is set to -1 if the main/selfpath did not start streaming. To fix this, the buffers are added to the buffers list on qbuf and then when a stream start from selfpath/mainpath the first buffer is removed from the list, applied and returned to userspace. This is done only if the params node is also streaming. Testing: I added a code to libcamera that sets the red, blue, and green gain interchangeably. The frames are then saved to files. To run the code: git clone --single-branch --branch rkisp1-tests-for-params-fixes-patchset https://gitlab.collabora.com/dafna/libcamera.git cd libcamera ninja -C build install meson test rkisp1-ramzor -C build Then adding debug prints in the params node in the kernel that prints the gain values and the current frame http://ix.io/2ue3 Then playing frames with ffplay -f rawvideo -pixel_format nv12 -video_size 640x480 build/frame-bla-<FRAME-NUM>.bin and looking that the frame color matches the debug prints. changes from v1: - patch 1 from v1 is removed - patch 3 from v1 (now patch 5) which refactor the stop_streaming params cb is changed according to discussion with Tomasz - 12 new patches added Dafna Hirschfeld (14): media: staging: rkisp1: call params isr only upon frame out media: staging: rkisp1: params: use rkisp1_param_set_bits to set reg in isr media: staging: rkisp1: params: use the new effect value in cproc config media: staging: rkisp1: params: don't release lock in isr before buffer is done media: staging: rkisp1: params: upon stream stop, iterate a local list to return the buffers media: staging: rkisp1: params: in the isr, return if buffer list is empty media: staging: rkisp1: params: avoid using buffer if params is not streaming media: staging: rkisp1: params: set vb.sequence to be the isp's frame_sequence + 1 media: staging: rkisp1: remove atomic operations for frame sequence media: staging: rkisp1: isp: add a warning and debugfs var for irq delay media: staging: rkisp1: isp: don't enable signal RKISP1_CIF_ISP_FRAME_IN media: staging: rkisp1: stats: protect write to 'is_streaming' in start_streaming cb media: staging: rkisp1: call media_pipeline_start/stop from stats and params media: staging: rkisp1: params: no need to lock default config drivers/staging/media/rkisp1/rkisp1-capture.c | 2 +- drivers/staging/media/rkisp1/rkisp1-common.h | 10 +- drivers/staging/media/rkisp1/rkisp1-dev.c | 2 + drivers/staging/media/rkisp1/rkisp1-isp.c | 39 +++--- drivers/staging/media/rkisp1/rkisp1-params.c | 124 ++++++++---------- drivers/staging/media/rkisp1/rkisp1-stats.c | 14 +- 6 files changed, 96 insertions(+), 95 deletions(-) -- 2.17.1
WARNING: multiple messages have this Message-ID (diff)
From: Dafna Hirschfeld <dafna.hirschfeld@collabora.com> To: linux-media@vger.kernel.org Cc: mchehab@kernel.org, dafna.hirschfeld@collabora.com, dafna3@gmail.com, tfiga@chromium.org, hverkuil@xs4all.nl, linux-rockchip@lists.infradead.org, helen.koike@collabora.com, laurent.pinchart@ideasonboard.com, sakari.ailus@linux.intel.com, kernel@collabora.com, ezequiel@collabora.com Subject: [PATCH v2 00/14] media: staging: rkisp1: various bug fixes Date: Sat, 15 Aug 2020 12:37:20 +0200 [thread overview] Message-ID: <20200815103734.31153-1-dafna.hirschfeld@collabora.com> (raw) Fix various bugs mainly in params and stats. The patchset fixes buffers synchronization issues discussed https://patchwork.kernel.org/patch/11066513/#23544763 As discussed with Tomasz, we decide that in order to keep the code simple, we assume that the v-start signal (RKISP1_CIF_ISP_V_START) and the frame-out signal RKISP1_CIF_ISP_FRAME don't arrive together. So when v-starts arrives the frame sequence is incremented and when frame-out arrives, the stats and params isr are called. Also the frame sequence of the params buffer should be the frame to which the params are applied. The params node needs to apply the first params buffer when the streaming from main/selfpath starts before the first frame arrive so that the first buffer will configure the first frame. The way it is implemented is that the first buffer queued to the params node is not added to the list but instead a local copy is saved and the buffer returns to userspace immediately. Then the local copy is applied when main/selfpath stream starts. This implementation is buggy for the following reasons: - The first params buffer is applied and returned to userspace even if userspace never calls to streamon on the params node. - If the first params buffer is queued after the stream started on the params node then it will return to userspace but will never be used. - The frame_sequence of the first buffer is set to -1 if the main/selfpath did not start streaming. To fix this, the buffers are added to the buffers list on qbuf and then when a stream start from selfpath/mainpath the first buffer is removed from the list, applied and returned to userspace. This is done only if the params node is also streaming. Testing: I added a code to libcamera that sets the red, blue, and green gain interchangeably. The frames are then saved to files. To run the code: git clone --single-branch --branch rkisp1-tests-for-params-fixes-patchset https://gitlab.collabora.com/dafna/libcamera.git cd libcamera ninja -C build install meson test rkisp1-ramzor -C build Then adding debug prints in the params node in the kernel that prints the gain values and the current frame http://ix.io/2ue3 Then playing frames with ffplay -f rawvideo -pixel_format nv12 -video_size 640x480 build/frame-bla-<FRAME-NUM>.bin and looking that the frame color matches the debug prints. changes from v1: - patch 1 from v1 is removed - patch 3 from v1 (now patch 5) which refactor the stop_streaming params cb is changed according to discussion with Tomasz - 12 new patches added Dafna Hirschfeld (14): media: staging: rkisp1: call params isr only upon frame out media: staging: rkisp1: params: use rkisp1_param_set_bits to set reg in isr media: staging: rkisp1: params: use the new effect value in cproc config media: staging: rkisp1: params: don't release lock in isr before buffer is done media: staging: rkisp1: params: upon stream stop, iterate a local list to return the buffers media: staging: rkisp1: params: in the isr, return if buffer list is empty media: staging: rkisp1: params: avoid using buffer if params is not streaming media: staging: rkisp1: params: set vb.sequence to be the isp's frame_sequence + 1 media: staging: rkisp1: remove atomic operations for frame sequence media: staging: rkisp1: isp: add a warning and debugfs var for irq delay media: staging: rkisp1: isp: don't enable signal RKISP1_CIF_ISP_FRAME_IN media: staging: rkisp1: stats: protect write to 'is_streaming' in start_streaming cb media: staging: rkisp1: call media_pipeline_start/stop from stats and params media: staging: rkisp1: params: no need to lock default config drivers/staging/media/rkisp1/rkisp1-capture.c | 2 +- drivers/staging/media/rkisp1/rkisp1-common.h | 10 +- drivers/staging/media/rkisp1/rkisp1-dev.c | 2 + drivers/staging/media/rkisp1/rkisp1-isp.c | 39 +++--- drivers/staging/media/rkisp1/rkisp1-params.c | 124 ++++++++---------- drivers/staging/media/rkisp1/rkisp1-stats.c | 14 +- 6 files changed, 96 insertions(+), 95 deletions(-) -- 2.17.1 _______________________________________________ Linux-rockchip mailing list Linux-rockchip@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-rockchip
next reply other threads:[~2020-08-15 22:02 UTC|newest] Thread overview: 71+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-08-15 10:37 Dafna Hirschfeld [this message] 2020-08-15 10:37 ` [PATCH v2 00/14] media: staging: rkisp1: various bug fixes Dafna Hirschfeld 2020-08-15 10:37 ` [PATCH v2 01/14] media: staging: rkisp1: call params isr only upon frame out Dafna Hirschfeld 2020-08-15 10:37 ` Dafna Hirschfeld 2020-08-17 21:46 ` Helen Koike 2020-08-17 21:46 ` Helen Koike 2020-08-15 10:37 ` [PATCH v2 02/14] media: staging: rkisp1: params: use rkisp1_param_set_bits to set reg in isr Dafna Hirschfeld 2020-08-15 10:37 ` Dafna Hirschfeld 2020-08-17 21:46 ` Helen Koike 2020-08-17 21:46 ` Helen Koike 2020-08-15 10:37 ` [PATCH v2 03/14] media: staging: rkisp1: params: use the new effect value in cproc config Dafna Hirschfeld 2020-08-15 10:37 ` Dafna Hirschfeld 2020-08-17 21:46 ` Helen Koike 2020-08-17 21:46 ` Helen Koike 2020-08-15 10:37 ` [PATCH v2 04/14] media: staging: rkisp1: params: don't release lock in isr before buffer is done Dafna Hirschfeld 2020-08-15 10:37 ` Dafna Hirschfeld 2020-08-17 21:47 ` Helen Koike 2020-08-17 21:47 ` Helen Koike 2020-08-15 10:37 ` [PATCH v2 05/14] media: staging: rkisp1: params: upon stream stop, iterate a local list to return the buffers Dafna Hirschfeld 2020-08-15 10:37 ` Dafna Hirschfeld 2020-08-17 21:47 ` Helen Koike 2020-08-17 21:47 ` Helen Koike 2020-08-20 9:27 ` Hans Verkuil 2020-08-20 9:27 ` Hans Verkuil 2020-08-20 12:16 ` Helen Koike 2020-08-20 12:16 ` Helen Koike 2020-08-15 10:37 ` [PATCH v2 06/14] media: staging: rkisp1: params: in the isr, return if buffer list is empty Dafna Hirschfeld 2020-08-15 10:37 ` Dafna Hirschfeld 2020-08-17 21:47 ` Helen Koike 2020-08-17 21:47 ` Helen Koike 2020-08-15 10:37 ` [PATCH v2 07/14] media: staging: rkisp1: params: avoid using buffer if params is not streaming Dafna Hirschfeld 2020-08-15 10:37 ` Dafna Hirschfeld 2020-08-16 4:28 ` kernel test robot 2020-08-16 4:28 ` kernel test robot 2020-08-16 4:28 ` kernel test robot 2020-08-17 21:47 ` Helen Koike 2020-08-17 21:47 ` Helen Koike 2020-08-15 10:37 ` [PATCH v2 08/14] media: staging: rkisp1: params: set vb.sequence to be the isp's frame_sequence + 1 Dafna Hirschfeld 2020-08-15 10:37 ` Dafna Hirschfeld 2020-08-17 21:47 ` Helen Koike 2020-08-17 21:47 ` Helen Koike 2020-08-15 10:37 ` [PATCH v2 09/14] media: staging: rkisp1: remove atomic operations for frame sequence Dafna Hirschfeld 2020-08-15 10:37 ` Dafna Hirschfeld 2020-08-17 21:48 ` Helen Koike 2020-08-17 21:48 ` Helen Koike 2020-09-17 16:41 ` Dafna Hirschfeld 2020-09-17 16:41 ` Dafna Hirschfeld 2020-08-15 10:37 ` [PATCH v2 10/14] media: staging: rkisp1: isp: add a warning and debugfs var for irq delay Dafna Hirschfeld 2020-08-15 10:37 ` Dafna Hirschfeld 2020-08-17 21:48 ` Helen Koike 2020-08-17 21:48 ` Helen Koike 2020-08-18 6:46 ` Dafna Hirschfeld 2020-08-18 6:46 ` Dafna Hirschfeld 2020-08-15 10:37 ` [PATCH v2 11/14] media: staging: rkisp1: isp: don't enable signal RKISP1_CIF_ISP_FRAME_IN Dafna Hirschfeld 2020-08-15 10:37 ` Dafna Hirschfeld 2020-08-17 21:48 ` Helen Koike 2020-08-17 21:48 ` Helen Koike 2020-08-18 6:37 ` Dafna Hirschfeld 2020-08-18 6:37 ` Dafna Hirschfeld 2020-08-15 10:37 ` [PATCH v2 12/14] media: staging: rkisp1: stats: protect write to 'is_streaming' in start_streaming cb Dafna Hirschfeld 2020-08-15 10:37 ` Dafna Hirschfeld 2020-08-17 21:48 ` Helen Koike 2020-08-17 21:48 ` Helen Koike 2020-08-15 10:37 ` [PATCH v2 13/14] media: staging: rkisp1: call media_pipeline_start/stop from stats and params Dafna Hirschfeld 2020-08-15 10:37 ` Dafna Hirschfeld 2020-08-17 21:48 ` Helen Koike 2020-08-17 21:48 ` Helen Koike 2020-08-15 10:37 ` [PATCH v2 14/14] media: staging: rkisp1: params: no need to lock default config Dafna Hirschfeld 2020-08-15 10:37 ` Dafna Hirschfeld 2020-08-17 21:48 ` Helen Koike 2020-08-17 21:48 ` Helen Koike
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20200815103734.31153-1-dafna.hirschfeld@collabora.com \ --to=dafna.hirschfeld@collabora.com \ --cc=dafna3@gmail.com \ --cc=ezequiel@collabora.com \ --cc=helen.koike@collabora.com \ --cc=hverkuil@xs4all.nl \ --cc=kernel@collabora.com \ --cc=laurent.pinchart@ideasonboard.com \ --cc=linux-media@vger.kernel.org \ --cc=linux-rockchip@lists.infradead.org \ --cc=mchehab@kernel.org \ --cc=sakari.ailus@linux.intel.com \ --cc=tfiga@chromium.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.