From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
Cc: linux-media@vger.kernel.org, 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
Subject: Re: [PATCH 4/5] media: staging: rkisp1: stats: replace locks wq_lock, irq_lock with one lock
Date: Thu, 21 May 2020 02:48:10 +0300 [thread overview]
Message-ID: <20200520234810.GD25474@pendragon.ideasonboard.com> (raw)
In-Reply-To: <20200512120522.25960-5-dafna.hirschfeld@collabora.com>
Hi Dafna,
Thank you for the patch.
On Tue, May 12, 2020 at 02:05:21PM +0200, Dafna Hirschfeld wrote:
> This patch removes two locks in the rkisp1_stats struct:
> - The mutex 'wq_lock' that is used to protect the buffers list 'stat'
> - The spin_lock 'irq_lock' that is used to protect the
> variable 'is_streaming'
>
> It replaces them with one spin_lock 'stats_lock' that protects
> both the buffers list and the 'is_streaming' variable.
You should explain in the commit message why this is needed.
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> ---
> drivers/staging/media/rkisp1/rkisp1-common.h | 5 ++--
> drivers/staging/media/rkisp1/rkisp1-stats.c | 27 ++++++++++----------
> 2 files changed, 15 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/staging/media/rkisp1/rkisp1-common.h b/drivers/staging/media/rkisp1/rkisp1-common.h
> index c0ab16c6b3db..c635bb0a7727 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-common.h
> +++ b/drivers/staging/media/rkisp1/rkisp1-common.h
> @@ -178,7 +178,7 @@ struct rkisp1_capture {
> /*
> * struct rkisp1_stats - ISP Statistics device
> *
> - * @irq_lock: buffer queue lock
> + * @stats_lock: locks the buffer list 'stat' and 'is_streaming'
> * @stat: stats buffer list
> * @readout_wq: workqueue for statistics information read
> */
> @@ -186,13 +186,12 @@ struct rkisp1_stats {
> struct rkisp1_vdev_node vnode;
> struct rkisp1_device *rkisp1;
>
> - spinlock_t irq_lock;
> + spinlock_t stats_lock; /* locks 'is_streaming', and 'stats' */
If there's a single lock, how about simply calling it 'lock' ?
> struct list_head stat;
> struct v4l2_format vdev_fmt;
> bool is_streaming;
>
> struct workqueue_struct *readout_wq;
> - struct mutex wq_lock;
> };
>
> /*
> diff --git a/drivers/staging/media/rkisp1/rkisp1-stats.c b/drivers/staging/media/rkisp1/rkisp1-stats.c
> index 5578fdeb8a18..e6fb2c5f3b3e 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-stats.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-stats.c
> @@ -130,12 +130,13 @@ static void rkisp1_stats_vb2_buf_queue(struct vb2_buffer *vb)
> container_of(vbuf, struct rkisp1_buffer, vb);
> struct vb2_queue *vq = vb->vb2_queue;
> struct rkisp1_stats *stats_dev = vq->drv_priv;
> + unsigned long flags;
>
> stats_buf->vaddr[0] = vb2_plane_vaddr(vb, 0);
>
> - mutex_lock(&stats_dev->wq_lock);
> + spin_lock_irqsave(&stats_dev->stats_lock, flags);
As I explained in a previous e-mail in this series, you could use
spin_lock_irq() here as there's a guarantee interrupts are not disabled.
Up to you.
> list_add_tail(&stats_buf->queue, &stats_dev->stat);
> - mutex_unlock(&stats_dev->wq_lock);
> + spin_unlock_irqrestore(&stats_dev->stats_lock, flags);
> }
>
> static int rkisp1_stats_vb2_buf_prepare(struct vb2_buffer *vb)
> @@ -156,13 +157,13 @@ static void rkisp1_stats_vb2_stop_streaming(struct vb2_queue *vq)
> unsigned int i;
>
> /* Make sure no new work queued in isr before draining wq */
> - spin_lock_irqsave(&stats->irq_lock, flags);
> + spin_lock_irqsave(&stats->stats_lock, flags);
> stats->is_streaming = false;
> - spin_unlock_irqrestore(&stats->irq_lock, flags);
> + spin_unlock_irqrestore(&stats->stats_lock, flags);
>
> drain_workqueue(stats->readout_wq);
>
> - mutex_lock(&stats->wq_lock);
> + spin_lock_irqsave(&stats->stats_lock, flags);
> for (i = 0; i < RKISP1_ISP_STATS_REQ_BUFS_MAX; i++) {
> if (list_empty(&stats->stat))
> break;
> @@ -171,7 +172,7 @@ static void rkisp1_stats_vb2_stop_streaming(struct vb2_queue *vq)
> list_del(&buf->queue);
> vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_ERROR);
> }
> - mutex_unlock(&stats->wq_lock);
> + spin_unlock_irqrestore(&stats->stats_lock, flags);
> }
>
> static int
> @@ -333,6 +334,7 @@ rkisp1_stats_send_measurement(struct rkisp1_stats *stats,
> unsigned int frame_sequence =
> atomic_read(&stats->rkisp1->isp.frame_sequence);
> u64 timestamp = ktime_get_ns();
> + unsigned long flags;
>
> if (frame_sequence != meas_work->frame_id) {
> dev_warn(stats->rkisp1->dev,
> @@ -341,14 +343,14 @@ rkisp1_stats_send_measurement(struct rkisp1_stats *stats,
> frame_sequence = meas_work->frame_id;
> }
>
> - mutex_lock(&stats->wq_lock);
> + spin_lock_irqsave(&stats->stats_lock, flags);
> /* get one empty buffer */
> if (!list_empty(&stats->stat)) {
> cur_buf = list_first_entry(&stats->stat,
> struct rkisp1_buffer, queue);
> list_del(&cur_buf->queue);
> }
> - mutex_unlock(&stats->wq_lock);
> + spin_unlock_irqrestore(&stats->stats_lock, flags);
>
> if (!cur_buf)
> return;
> @@ -406,7 +408,7 @@ void rkisp1_stats_isr(struct rkisp1_stats *stats, u32 isp_ris)
> unsigned long flags;
> u32 val;
>
> - spin_lock_irqsave(&stats->irq_lock, flags);
> + spin_lock_irqsave(&stats->stats_lock, flags);
>
> val = RKISP1_STATS_MEAS_MASK;
> rkisp1_write(rkisp1, val, RKISP1_CIF_ISP_ICR);
> @@ -436,7 +438,7 @@ void rkisp1_stats_isr(struct rkisp1_stats *stats, u32 isp_ris)
> }
>
> unlock:
> - spin_unlock_irqrestore(&stats->irq_lock, flags);
> + spin_unlock_irqrestore(&stats->stats_lock, flags);
> }
>
> static void rkisp1_init_stats(struct rkisp1_stats *stats)
> @@ -456,10 +458,9 @@ int rkisp1_stats_register(struct rkisp1_stats *stats,
> int ret;
>
> stats->rkisp1 = rkisp1;
> - mutex_init(&stats->wq_lock);
> mutex_init(&node->vlock);
> INIT_LIST_HEAD(&stats->stat);
> - spin_lock_init(&stats->irq_lock);
> + spin_lock_init(&stats->stats_lock);
>
> strscpy(vdev->name, RKISP1_STATS_DEV_NAME, sizeof(vdev->name));
>
> @@ -506,7 +507,6 @@ int rkisp1_stats_register(struct rkisp1_stats *stats,
> err_release_queue:
> vb2_queue_release(vdev->queue);
> mutex_destroy(&node->vlock);
> - mutex_destroy(&stats->wq_lock);
> return ret;
> }
>
> @@ -520,5 +520,4 @@ void rkisp1_stats_unregister(struct rkisp1_stats *stats)
> media_entity_cleanup(&vdev->entity);
> vb2_queue_release(vdev->queue);
> mutex_destroy(&node->vlock);
> - mutex_destroy(&stats->wq_lock);
> }
--
Regards,
Laurent Pinchart
next prev parent reply other threads:[~2020-05-20 23:48 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-05-12 12:05 [PATCH 0/5] media: staging: rkisp1: change workqueue to threaded irq in stats Dafna Hirschfeld
2020-05-12 12:05 ` [PATCH 1/5] media: staging: rkisp1: return IRQ_NONE in isr when irq isn't for ISP Dafna Hirschfeld
2020-05-12 15:42 ` kbuild test robot
2020-05-20 10:58 ` Helen Koike
2020-05-20 20:58 ` Laurent Pinchart
2020-05-12 12:05 ` [PATCH 2/5] media: staging: rkisp1: use a macro for the statistics flags mask Dafna Hirschfeld
2020-05-20 11:03 ` Helen Koike
2020-05-20 23:27 ` Laurent Pinchart
2020-05-12 12:05 ` [PATCH 3/5] media: staging: rkisp1: stats: use spin_lock_irqsave for irq_lock Dafna Hirschfeld
2020-05-20 11:11 ` Helen Koike
2020-05-20 19:22 ` Dafna Hirschfeld
2020-05-20 23:40 ` Laurent Pinchart
2020-05-12 12:05 ` [PATCH 4/5] media: staging: rkisp1: stats: replace locks wq_lock, irq_lock with one lock Dafna Hirschfeld
2020-05-20 23:48 ` Laurent Pinchart [this message]
2020-05-12 12:05 ` [PATCH 5/5] media: staging: rkisp1: replace workqueue with threaded irq for reading statistics registers Dafna Hirschfeld
2020-05-12 16:41 ` kbuild test robot
2020-05-21 0:09 ` Laurent Pinchart
2020-05-21 10:38 ` Tomasz Figa
2020-05-28 19:19 ` Dafna Hirschfeld
2020-05-28 19:35 ` Tomasz Figa
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=20200520234810.GD25474@pendragon.ideasonboard.com \
--to=laurent.pinchart@ideasonboard.com \
--cc=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=linux-media@vger.kernel.org \
--cc=linux-rockchip@lists.infradead.org \
--cc=mchehab@kernel.org \
--cc=sakari.ailus@linux.intel.com \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).