Linux-Media Archive on lore.kernel.org
 help / color / Atom feed
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 v3 00/12] media: staging: rkisp1: various bug fixes
Date: Tue, 22 Sep 2020 13:33:50 +0200
Message-ID: <20200922113402.12442-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-start arrives, the frame sequence is incremented and
when frame-out arrives, the stats and params isr are called.
Also the vb.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 arrives
so that the first params 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 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, params buffers are added to the buffers list on qbuf and
then when a stream start from selfpath/mainpath then 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 test-various-bug-fixes-v3 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 v2:
* patches 11,12 in v3 are new, they fix the TODO issue "review and comment every lock"
* patches 1,2,4 from v2 are already applied so remove from v3
* patch 13 from v2: "media: staging: rkisp1: call media_pipeline_start/stop from stats and params" - dropped from this version
* patch "media: staging: rkisp1: params: use the new effect value in cproc config"
was reorder to be just before patch "media: staging: rkisp1: params: avoid using buffer if params is not streaming"
* patch 4: declare function 'rkisp1_params_apply_params_cfg' as static to fix a warning reported by 'kernel test robot <lkp@intel.com>'
* patch 5: rephrase commit message and inline doc as suggested by Helen.
* patch 6: add a closing "}" to "if" condition and remove useless inline comment
* patch 7: remove unneeded closing "}" to "if"


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 (12):
  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: use the new effect value in cproc
    config
  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: params: no need to lock default config
  media: staging: rkisp1: use the right variants of spin_lock
  media: staging: rkisp1: cap: protect access to buf with the spin lock

 drivers/staging/media/rkisp1/TODO             |   1 -
 drivers/staging/media/rkisp1/rkisp1-capture.c |  19 ++-
 drivers/staging/media/rkisp1/rkisp1-common.h  |   7 +-
 drivers/staging/media/rkisp1/rkisp1-dev.c     |   2 +
 drivers/staging/media/rkisp1/rkisp1-isp.c     |  22 ++--
 drivers/staging/media/rkisp1/rkisp1-params.c  | 118 ++++++++----------
 drivers/staging/media/rkisp1/rkisp1-stats.c   |   5 +-
 7 files changed, 76 insertions(+), 98 deletions(-)

-- 
2.17.1


             reply index

Thread overview: 27+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-09-22 11:33 Dafna Hirschfeld [this message]
2020-09-22 11:33 ` [PATCH v3 01/12] media: staging: rkisp1: params: upon stream stop, iterate a local list to return the buffers Dafna Hirschfeld
2020-09-25 19:08   ` Tomasz Figa
2020-09-22 11:33 ` [PATCH v3 02/12] media: staging: rkisp1: params: in the isr, return if buffer list is empty Dafna Hirschfeld
2020-09-22 11:33 ` [PATCH v3 03/12] media: staging: rkisp1: params: use the new effect value in cproc config Dafna Hirschfeld
2020-09-22 11:33 ` [PATCH v3 04/12] media: staging: rkisp1: params: avoid using buffer if params is not streaming Dafna Hirschfeld
2020-09-25 20:16   ` Tomasz Figa
2020-09-22 11:33 ` [PATCH v3 05/12] media: staging: rkisp1: params: set vb.sequence to be the isp's frame_sequence + 1 Dafna Hirschfeld
2020-09-22 11:33 ` [PATCH v3 06/12] media: staging: rkisp1: remove atomic operations for frame sequence Dafna Hirschfeld
2020-09-25 20:42   ` Tomasz Figa
2020-10-02  9:16     ` Dafna Hirschfeld
2020-10-02 15:30       ` Tomasz Figa
2020-11-06 14:43         ` Helen Koike
2020-11-10 10:40           ` Tomasz Figa
2020-09-22 11:33 ` [PATCH v3 07/12] media: staging: rkisp1: isp: add a warning and debugfs var for irq delay Dafna Hirschfeld
2020-09-22 11:33 ` [PATCH v3 08/12] media: staging: rkisp1: isp: don't enable signal RKISP1_CIF_ISP_FRAME_IN Dafna Hirschfeld
2020-09-22 11:33 ` [PATCH v3 09/12] media: staging: rkisp1: stats: protect write to 'is_streaming' in start_streaming cb Dafna Hirschfeld
2020-09-25 20:47   ` Tomasz Figa
2020-09-22 11:34 ` [PATCH v3 10/12] media: staging: rkisp1: params: no need to lock default config Dafna Hirschfeld
2020-09-22 11:34 ` [PATCH v3 11/12] media: staging: rkisp1: use the right variants of spin_lock Dafna Hirschfeld
2020-09-25 20:51   ` Tomasz Figa
2020-09-22 11:34 ` [PATCH v3 12/12] media: staging: rkisp1: cap: protect access to buf with the spin lock Dafna Hirschfeld
2020-09-25 20:53   ` Tomasz Figa
2020-09-27  9:43   ` Mauro Carvalho Chehab
2020-09-27  9:54     ` Dafna Hirschfeld
2020-09-27 11:05       ` Mauro Carvalho Chehab
2020-09-28 15:29         ` Dafna Hirschfeld

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=20200922113402.12442-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: link

Linux-Media Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-media/0 linux-media/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-media linux-media/ https://lore.kernel.org/linux-media \
		linux-media@vger.kernel.org
	public-inbox-index linux-media

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-media


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git