Message-ID: <20200925175700.GA3607091@chromium.org>
Date:   Fri, 25 Sep 2020 17:57:00 +0000
From:   Tomasz Figa <tfiga@...omium.org>
To:     Adrian Ratiu <adrian.ratiu@...labora.com>,
        Hans Verkuil <hverkuil-cisco@...all.nl>
Cc:     Xia Jiang <xia.jiang@...iatek.com>,
        Hans Verkuil <hverkuil-cisco@...all.nl>,
        Mauro Carvalho Chehab <mchehab+samsung@...nel.org>,
        kernel@...labora.com, linux-media@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-mediatek@...ts.infradead.org,
        Pi-Hsun Shih <pihsun@...omium.org>,
        Jerry-ch Chen <jerry-ch.chen@...iatek.corp-partner.google.com>
Subject: Re: [PATCH v12 07/29] media: v4l2-mem2mem: add v4l2_m2m_suspend,
 v4l2_m2m_resume

On Fri, Sep 25, 2020 at 08:53:09PM +0300, Adrian Ratiu wrote:
> Hi,
> 
> I'm having a problem with this patch which landed in linux-next.
> 
> On Fri, 14 Aug 2020, Xia Jiang <xia.jiang@...iatek.com> wrote:
> > From: Pi-Hsun Shih <pihsun@...omium.org>
> > 
> > Add two functions that can be used to stop new jobs from being queued /
> > continue running queued job. This can be used while a driver using m2m
> > helper is going to suspend / wake up from resume, and can ensure that
> > there's no job running in suspend process.
> > 
> > BUG=b:143046833 TEST=build
> > 
> > Signed-off-by: Pi-Hsun Shih <pihsun@...omium.org> Signed-off-by:
> > Jerry-ch Chen <jerry-ch.chen@...iatek.corp-partner.google.com>
> > Reviewed-by: Tomasz Figa <tfiga@...omium.org> --- v12: add this relied
> > patch to the series ---  drivers/media/v4l2-core/v4l2-mem2mem.c | 41
> > ++++++++++++++++++++++++++ include/media/v4l2-mem2mem.h  | 22
> > ++++++++++++++ 2 files changed, 63 insertions(+)
> > 
> > diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c
> > b/drivers/media/v4l2-core/v4l2-mem2mem.c index
> > 62ac9424c92a..ddfdb6375064 100644 ---
> > a/drivers/media/v4l2-core/v4l2-mem2mem.c +++
> > b/drivers/media/v4l2-core/v4l2-mem2mem.c @@ -43,6 +43,10 @@
> > module_param(debug, bool, 0644);  #define TRANS_ABORT		(1 << 2)   +/*
> > The job queue is not running new jobs */ +#define QUEUE_PAUSED		(1 << 0)
> > + +  /* Offset base for buffers on the destination queue - used to
> > distinguish   * between source and destination buffers when mmapping -
> > they   receive the same * offsets but for different queues */ @@ -84,6
> > +88,7 @@ static const char * const m2m_entity_name[] = {   *
> > @job_queue:		instances queued to run *   @job_spinlock:	protects
> > job_queue * @job_work:   worker to run queued jobs. + *
> > @job_queue_flags:	flags of the queue status, %QUEUE_PAUSED.   *
> > @m2m_ops:		driver callbacks */  struct v4l2_m2m_dev { @@ -101,6 +106,7
> > @@ struct v4l2_m2m_dev {  	struct list_head	job_queue; spinlock_t
> > job_spinlock; struct work_struct	job_work; +	unsigned
> > long		job_queue_flags;   const struct v4l2_m2m_ops *m2m_ops; }; @@
> > -263,6 +269,12 @@ static void v4l2_m2m_try_run(struct v4l2_m2m_dev
> > *m2m_dev)  		return; }  +	if (m2m_dev->job_queue_flags & QUEUE_PAUSED) {
> > + spin_unlock_irqrestore(&m2m_dev->job_spinlock, flags); +
> > dprintk("Running new jobs is paused\n"); +		return; + } +
> > 	m2m_dev->curr_ctx = list_first_entry(&m2m_dev->job_queue,  struct
> > v4l2_m2m_ctx, queue); m2m_dev->curr_ctx->job_flags |=  TRANS_RUNNING; @@
> > -504,6 +516,7 @@ void v4l2_m2m_buf_done_and_job_finish(struct
> > v4l2_m2m_dev *m2m_dev,   if (WARN_ON(!src_buf || !dst_buf)) goto unlock;
> > +	v4l2_m2m_buf_done(src_buf, state);
> 
> This line looks out of place in this commit and is causing a lot of warnings
> (1 per frame). Any reason in particular why we need this?

Right, it shouldn't be there. Looks like a rebase error.

P.S. Please check your remail client settings as all the lines of your
message seem to be just mixed together, as if someone removed the line
breaks.

Hans, is this something you could fix up or we need to revert and
resend?

Best regards,
Tomasz

> 
> [   87.825061] ------------[ cut here ]------------ [   87.829695] WARNING:
> CPU: 0 PID: 0 at drivers/media/common/videobuf2/videobuf2-core.c:986
> vb2_buffer_done+0x208/0x2a0 [   87.840302] Modules linked in: [ 87.843364]
> CPU: 0 PID: 0 Comm: swapper/0 Tainted: G        W 5.9.0-rc6-next-20200924+
> #472 [   87.852407] Hardware name: NXP i.MX8MQ EVK (DT) [   87.856942]
> pstate: 20000085 (nzCv daIf -PAN -UAO -TCO BTYPE=--) [   87.862953] pc :
> vb2_buffer_done+0x208/0x2a0 [   87.867224] lr :
> v4l2_m2m_buf_done_and_job_finish+0x94/0x140 [   87.872882] sp :
> ffff80001183bd50 [   87.876195] x29: ffff80001183bd50 x28: ffff8000115d1500
> [   87.881512] x27: ffff80001128e018 x26: ffff00009fb4a828  [   87.886828]
> x25: ffff0000a4e13a08 x24: 0000000000000080  [   87.892143] x23:
> 0000000000000005 x22: ffff0000a253bc00  [   87.897457] x21: ffff00009fb4aa98
> x20: ffff00009fb4a800  [   87.902772] x19: ffff0000a24f0000 x18:
> 0000000000000000  [   87.908086] x17: 0000000000000000 x16: 0000000000000000
> [   87.913400] x15: 0000000000000000 x14: 0000000000000500  [   87.918714]
> x13: 0000000000000003 x12: 0000000000000000  [   87.924028] x11:
> 0000000000000040 x10: ffff800011658520  [   87.929340] x9 : ffff800010998464
> x8 : ffff0000a5800270  [   87.934655] x7 : 0000000000000000 x6 :
> ffff00009fb4aa20  [   87.939969] x5 : ffff80001183bd10 x4 : 0000000000000000
> [   87.945285] x3 : 0000000000000000 x2 : 0000000000000000  [   87.950599]
> x1 : 0000000000000005 x0 : 0000000000000005  [   87.955914] Call trace: [
> 87.958364] vb2_buffer_done+0x208/0x2a0 [   87.962288]
> v4l2_m2m_buf_done_and_job_finish+0x94/0x140 [   87.967601]
> hantro_job_finish+0xa8/0xe0 [   87.971524] hantro_irq_done+0x58/0x90 [
> 87.975275] imx8m_vpu_g1_irq+0x8c/0x160 [   87.979201]
> __handle_irq_event_percpu+0x68/0x2a0 [   87.983905]
> handle_irq_event_percpu+0x3c/0xa0 [   87.988347] handle_irq_event+0x50/0xf0
> [   87.992185] handle_fasteoi_irq+0xc0/0x180 [   87.996283]
> generic_handle_irq+0x38/0x50 [   88.000296] __handle_domain_irq+0x6c/0xd0 [
> 88.004393] gic_handle_irq+0x60/0x12c [   88.008143]  el1_irq+0xbc/0x180 [
> 88.011287]  arch_cpu_idle+0x1c/0x30 [   88.014864] do_idle+0x220/0x270 [
> 88.018093]  cpu_startup_entry+0x30/0x70 [ 88.022019]  rest_init+0xe0/0xf0 [
> 88.025250] arch_call_rest_init+0x18/0x24 [   88.029347]
> start_kernel+0x7a4/0x7e0 [   88.033013] CPU: 0 PID: 0 Comm: swapper/0
> Tainted: G        W         5.9.0-rc6-next-20200924+ #472 [   88.042056]
> Hardware name: NXP i.MX8MQ EVK (DT) [ 88.046585] Call trace: [   88.049034]
> dump_backtrace+0x0/0x1b0 [ 88.052697]  show_stack+0x20/0x70 [   88.056014]
> dump_stack+0xd0/0x12c [   88.059418]  __warn+0xfc/0x180 [ 88.062474]
> report_bug+0xfc/0x170 [   88.065875] bug_handler+0x28/0x70 [   88.069276]
> brk_handler+0x70/0xe0 [ 88.072681]  do_debug_exception+0xcc/0x1e0 [
> 88.076776] el1_sync_handler+0xd8/0x140 [   88.080697]  el1_sync+0x80/0x100 [
> 88.083926]  vb2_buffer_done+0x208/0x2a0 [   88.087852]
> v4l2_m2m_buf_done_and_job_finish+0x94/0x140 [   88.093162]
> hantro_job_finish+0xa8/0xe0 [   88.097083] hantro_irq_done+0x58/0x90 [
> 88.100831] imx8m_vpu_g1_irq+0x8c/0x160 [   88.104753]
> __handle_irq_event_percpu+0x68/0x2a0 [   88.109456]
> handle_irq_event_percpu+0x3c/0xa0 [   88.113899] handle_irq_event+0x50/0xf0
> [   88.117734] handle_fasteoi_irq+0xc0/0x180 [   88.121832]
> generic_handle_irq+0x38/0x50 [   88.125841] __handle_domain_irq+0x6c/0xd0 [
> 88.129937] gic_handle_irq+0x60/0x12c [   88.133686]  el1_irq+0xbc/0x180 [
> 88.136827]  arch_cpu_idle+0x1c/0x30 [   88.140403] do_idle+0x220/0x270 [
> 88.143630]  cpu_startup_entry+0x30/0x70 [ 88.147553]  rest_init+0xe0/0xf0 [
> 88.150782] arch_call_rest_init+0x18/0x24 [   88.154879]
> start_kernel+0x7a4/0x7e0 [   88.158542] ---[ end trace 847145312866dff5 ]---
> 
> Regards,
> Adrian
> 
> >  	dst_buf->is_held = src_buf->flags & V4L2_BUF_FLAG_M2M_HOLD_CAPTURE_BUF;
> >  	if (!dst_buf->is_held) {
> >  		v4l2_m2m_dst_buf_remove(m2m_ctx);
> > @@ -528,6 +541,34 @@ void v4l2_m2m_buf_done_and_job_finish(struct v4l2_m2m_dev *m2m_dev,
> >  }
> >  EXPORT_SYMBOL(v4l2_m2m_buf_done_and_job_finish);
> > +void v4l2_m2m_suspend(struct v4l2_m2m_dev *m2m_dev)
> > +{
> > +	unsigned long flags;
> > +	struct v4l2_m2m_ctx *curr_ctx;
> > +
> > +	spin_lock_irqsave(&m2m_dev->job_spinlock, flags);
> > +	m2m_dev->job_queue_flags |= QUEUE_PAUSED;
> > +	curr_ctx = m2m_dev->curr_ctx;
> > +	spin_unlock_irqrestore(&m2m_dev->job_spinlock, flags);
> > +
> > +	if (curr_ctx)
> > +		wait_event(curr_ctx->finished,
> > +			   !(curr_ctx->job_flags & TRANS_RUNNING));
> > +}
> > +EXPORT_SYMBOL(v4l2_m2m_suspend);
> > +
> > +void v4l2_m2m_resume(struct v4l2_m2m_dev *m2m_dev)
> > +{
> > +	unsigned long flags;
> > +
> > +	spin_lock_irqsave(&m2m_dev->job_spinlock, flags);
> > +	m2m_dev->job_queue_flags &= ~QUEUE_PAUSED;
> > +	spin_unlock_irqrestore(&m2m_dev->job_spinlock, flags);
> > +
> > +	v4l2_m2m_try_run(m2m_dev);
> > +}
> > +EXPORT_SYMBOL(v4l2_m2m_resume);
> > +
> >  int v4l2_m2m_reqbufs(struct file *file, struct v4l2_m2m_ctx *m2m_ctx,
> >  		     struct v4l2_requestbuffers *reqbufs)
> >  {
> > diff --git a/include/media/v4l2-mem2mem.h b/include/media/v4l2-mem2mem.h
> > index 98753f00df7e..5a91b548ecc0 100644
> > --- a/include/media/v4l2-mem2mem.h
> > +++ b/include/media/v4l2-mem2mem.h
> > @@ -304,6 +304,28 @@ v4l2_m2m_is_last_draining_src_buf(struct v4l2_m2m_ctx *m2m_ctx,
> >  void v4l2_m2m_last_buffer_done(struct v4l2_m2m_ctx *m2m_ctx,
> >  			       struct vb2_v4l2_buffer *vbuf);
> > +/**
> > + * v4l2_m2m_suspend() - stop new jobs from being run and wait for current job
> > + * to finish
> > + *
> > + * @m2m_dev: opaque pointer to the internal data to handle M2M context
> > + *
> > + * Called by a driver in the suspend hook. Stop new jobs from being run, and
> > + * wait for current running job to finish.
> > + */
> > +void v4l2_m2m_suspend(struct v4l2_m2m_dev *m2m_dev);
> > +
> > +/**
> > + * v4l2_m2m_resume() - resume job running and try to run a queued job
> > + *
> > + * @m2m_dev: opaque pointer to the internal data to handle M2M context
> > + *
> > + * Called by a driver in the resume hook. This reverts the operation of
> > + * v4l2_m2m_suspend() and allows job to be run. Also try to run a queued job if
> > + * there is any.
> > + */
> > +void v4l2_m2m_resume(struct v4l2_m2m_dev *m2m_dev);
> > +
> >  /**
> >   * v4l2_m2m_reqbufs() - multi-queue-aware REQBUFS multiplexer
> >   *
> > -- 
> > 2.18.0

Powered by blists - more mailing lists