[RFC,V4,1/4] media: v4l2-mem2mem: add v4l2_m2m_suspend, v4l2_m2m_resume

Commit Message

Jerry-ch Chen Dec. 4, 2019, 12:47 p.m. UTC

From: Pi-Hsun Shih <[e-mail]>

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 <[e-mail]>
Signed-off-by: Jerry-ch Chen <[e-mail]>
---
 drivers/media/v4l2-core/v4l2-mem2mem.c | 40 ++++++++++++++++++++++++++
 include/media/v4l2-mem2mem.h           | 22 ++++++++++++++
 2 files changed, 62 insertions(+)

Comments

Tomasz Figa May 21, 2020, 5:11 p.m. UTC | #1

Hi Jerry,

On Wed, Dec 04, 2019 at 08:47:29PM +0800, Jerry-ch Chen wrote:
> From: Pi-Hsun Shih <[e-mail]>
> 
> 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 <[e-mail]>
> Signed-off-by: Jerry-ch Chen <[e-mail]>
> ---
>  drivers/media/v4l2-core/v4l2-mem2mem.c | 40 ++++++++++++++++++++++++++
>  include/media/v4l2-mem2mem.h           | 22 ++++++++++++++
>  2 files changed, 62 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c b/drivers/media/v4l2-core/v4l2-mem2mem.c
> index 5bbdec55b7d7..76ba203e0035 100644
> --- a/drivers/media/v4l2-core/v4l2-mem2mem.c
> +++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
> @@ -47,6 +47,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 */
> @@ -88,6 +92,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 {
> @@ -105,6 +110,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;
>  };
> @@ -267,6 +273,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;
> @@ -447,6 +459,34 @@ void v4l2_m2m_job_finish(struct v4l2_m2m_dev *m2m_dev,
>  }
>  EXPORT_SYMBOL(v4l2_m2m_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 5467264771ec..119a195da390 100644
> --- a/include/media/v4l2-mem2mem.h
> +++ b/include/media/v4l2-mem2mem.h
> @@ -183,6 +183,28 @@ v4l2_m2m_buf_done(struct vb2_v4l2_buffer *buf, enum vb2_buffer_state state)
>  	vb2_buffer_done(&buf->vb2_buf, state);
>  }
>  
> +/**
> + * 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

Reviewed-by: Tomasz Figa <[e-mail]>

[Corrected Hans's email address.]
Hans, does this look good to you?

Best regards,
Tomasz

Jerry-ch Chen May 22, 2020, 6:01 a.m. UTC | #2

Hi Tomasz,

On Thu, 2020-05-21 at 17:11 +0000, Tomasz Figa wrote:
> Hi Jerry,
> 
> On Wed, Dec 04, 2019 at 08:47:29PM +0800, Jerry-ch Chen wrote:
> > From: Pi-Hsun Shih <[e-mail]>
> > 
> > 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 <[e-mail]>
> > Signed-off-by: Jerry-ch Chen <[e-mail]>
> > ---
> >  drivers/media/v4l2-core/v4l2-mem2mem.c | 40 ++++++++++++++++++++++++++
> >  include/media/v4l2-mem2mem.h           | 22 ++++++++++++++
> >  2 files changed, 62 insertions(+)
> > 
> > diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c b/drivers/media/v4l2-core/v4l2-mem2mem.c
> > index 5bbdec55b7d7..76ba203e0035 100644
> > --- a/drivers/media/v4l2-core/v4l2-mem2mem.c
> > +++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
> > @@ -47,6 +47,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 */
> > @@ -88,6 +92,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 {
> > @@ -105,6 +110,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;
> >  };
> > @@ -267,6 +273,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;
> > @@ -447,6 +459,34 @@ void v4l2_m2m_job_finish(struct v4l2_m2m_dev *m2m_dev,
> >  }
> >  EXPORT_SYMBOL(v4l2_m2m_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 5467264771ec..119a195da390 100644
> > --- a/include/media/v4l2-mem2mem.h
> > +++ b/include/media/v4l2-mem2mem.h
> > @@ -183,6 +183,28 @@ v4l2_m2m_buf_done(struct vb2_v4l2_buffer *buf, enum vb2_buffer_state state)
> >  	vb2_buffer_done(&buf->vb2_buf, state);
> >  }
> >  
> > +/**
> > + * 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
> 
> Reviewed-by: Tomasz Figa <[e-mail]>
> 
Ok, I've added it in the commit message.

Thanks and Best regards,
Jerry

> [Corrected Hans's email address.]
> Hans, does this look good to you?
> 
> Best regards,
> Tomasz
>

Hans Verkuil June 10, 2020, 10:28 a.m. UTC | #3

On 21/05/2020 19:11, Tomasz Figa wrote:
> Hi Jerry,
> 
> On Wed, Dec 04, 2019 at 08:47:29PM +0800, Jerry-ch Chen wrote:
>> From: Pi-Hsun Shih <[e-mail]>
>>
>> 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 <[e-mail]>
>> Signed-off-by: Jerry-ch Chen <[e-mail]>
>> ---
>>  drivers/media/v4l2-core/v4l2-mem2mem.c | 40 ++++++++++++++++++++++++++
>>  include/media/v4l2-mem2mem.h           | 22 ++++++++++++++
>>  2 files changed, 62 insertions(+)
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c b/drivers/media/v4l2-core/v4l2-mem2mem.c
>> index 5bbdec55b7d7..76ba203e0035 100644
>> --- a/drivers/media/v4l2-core/v4l2-mem2mem.c
>> +++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
>> @@ -47,6 +47,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 */
>> @@ -88,6 +92,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 {
>> @@ -105,6 +110,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;
>>  };
>> @@ -267,6 +273,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;
>> @@ -447,6 +459,34 @@ void v4l2_m2m_job_finish(struct v4l2_m2m_dev *m2m_dev,
>>  }
>>  EXPORT_SYMBOL(v4l2_m2m_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 5467264771ec..119a195da390 100644
>> --- a/include/media/v4l2-mem2mem.h
>> +++ b/include/media/v4l2-mem2mem.h
>> @@ -183,6 +183,28 @@ v4l2_m2m_buf_done(struct vb2_v4l2_buffer *buf, enum vb2_buffer_state state)
>>  	vb2_buffer_done(&buf->vb2_buf, state);
>>  }
>>  
>> +/**
>> + * 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
> 
> Reviewed-by: Tomasz Figa <[e-mail]>
> 
> [Corrected Hans's email address.]
> Hans, does this look good to you?

Yes, this looks good.

Sorry for the late reply.

I assume this will be part of a future patch series that calls these new functions?

Regards,

	Hans

Tomasz Figa June 10, 2020, 10:32 a.m. UTC | #4

On Wed, Jun 10, 2020 at 12:29 PM Hans Verkuil <[e-mail]> wrote:
>
> On 21/05/2020 19:11, Tomasz Figa wrote:
> > Hi Jerry,
> >
> > On Wed, Dec 04, 2019 at 08:47:29PM +0800, Jerry-ch Chen wrote:
> >> From: Pi-Hsun Shih <[e-mail]>
> >>
> >> 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 <[e-mail]>
> >> Signed-off-by: Jerry-ch Chen <[e-mail]>
> >> ---
> >>  drivers/media/v4l2-core/v4l2-mem2mem.c | 40 ++++++++++++++++++++++++++
> >>  include/media/v4l2-mem2mem.h           | 22 ++++++++++++++
> >>  2 files changed, 62 insertions(+)
> >>
> >> diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c b/drivers/media/v4l2-core/v4l2-mem2mem.c
> >> index 5bbdec55b7d7..76ba203e0035 100644
> >> --- a/drivers/media/v4l2-core/v4l2-mem2mem.c
> >> +++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
> >> @@ -47,6 +47,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 */
> >> @@ -88,6 +92,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 {
> >> @@ -105,6 +110,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;
> >>  };
> >> @@ -267,6 +273,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;
> >> @@ -447,6 +459,34 @@ void v4l2_m2m_job_finish(struct v4l2_m2m_dev *m2m_dev,
> >>  }
> >>  EXPORT_SYMBOL(v4l2_m2m_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 5467264771ec..119a195da390 100644
> >> --- a/include/media/v4l2-mem2mem.h
> >> +++ b/include/media/v4l2-mem2mem.h
> >> @@ -183,6 +183,28 @@ v4l2_m2m_buf_done(struct vb2_v4l2_buffer *buf, enum vb2_buffer_state state)
> >>      vb2_buffer_done(&buf->vb2_buf, state);
> >>  }
> >>
> >> +/**
> >> + * 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
> >
> > Reviewed-by: Tomasz Figa <[e-mail]>
> >
> > [Corrected Hans's email address.]
> > Hans, does this look good to you?
>
> Yes, this looks good.
>
> Sorry for the late reply.

No worries! Thanks a lot.

>
> I assume this will be part of a future patch series that calls these new functions?

The mtk-jpeg encoder series depends on this patch as well, so I guess
it would go together with whichever is ready first.

I would also envision someone changing the other existing drivers to
use the helpers, as I'm pretty much sure some of them don't handle
suspend/resume correctly.

Best regards,
Tomasz

Ezequiel Garcia June 10, 2020, 6:52 p.m. UTC | #5

Hi everyone,

Thanks for the patch.

On Wed, 10 Jun 2020 at 07:33, Tomasz Figa <[e-mail]> wrote:
>
> On Wed, Jun 10, 2020 at 12:29 PM Hans Verkuil <[e-mail]> wrote:
> >
> > On 21/05/2020 19:11, Tomasz Figa wrote:
> > > Hi Jerry,
> > >
> > > On Wed, Dec 04, 2019 at 08:47:29PM +0800, Jerry-ch Chen wrote:
> > >> From: Pi-Hsun Shih <[e-mail]>
> > >>
> > >> 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
> > >>

BUG/TEST tags need to be removed.

> > >> Signed-off-by: Pi-Hsun Shih <[e-mail]>
> > >> Signed-off-by: Jerry-ch Chen <[e-mail]>

This mail address probably needs correction?

> > >> ---
> > >>  drivers/media/v4l2-core/v4l2-mem2mem.c | 40 ++++++++++++++++++++++++++
> > >>  include/media/v4l2-mem2mem.h           | 22 ++++++++++++++
> > >>  2 files changed, 62 insertions(+)
> > >>
> > >> diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c b/drivers/media/v4l2-core/v4l2-mem2mem.c
> > >> index 5bbdec55b7d7..76ba203e0035 100644
> > >> --- a/drivers/media/v4l2-core/v4l2-mem2mem.c
> > >> +++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
> > >> @@ -47,6 +47,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 */
> > >> @@ -88,6 +92,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 {
> > >> @@ -105,6 +110,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;
> > >>  };
> > >> @@ -267,6 +273,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;
> > >> @@ -447,6 +459,34 @@ void v4l2_m2m_job_finish(struct v4l2_m2m_dev *m2m_dev,
> > >>  }
> > >>  EXPORT_SYMBOL(v4l2_m2m_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 5467264771ec..119a195da390 100644
> > >> --- a/include/media/v4l2-mem2mem.h
> > >> +++ b/include/media/v4l2-mem2mem.h
> > >> @@ -183,6 +183,28 @@ v4l2_m2m_buf_done(struct vb2_v4l2_buffer *buf, enum vb2_buffer_state state)
> > >>      vb2_buffer_done(&buf->vb2_buf, state);
> > >>  }
> > >>
> > >> +/**
> > >> + * 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
> > >
> > > Reviewed-by: Tomasz Figa <[e-mail]>
> > >
> > > [Corrected Hans's email address.]
> > > Hans, does this look good to you?
> >
> > Yes, this looks good.
> >
> > Sorry for the late reply.
>
> No worries! Thanks a lot.
>
> >
> > I assume this will be part of a future patch series that calls these new functions?
>
> The mtk-jpeg encoder series depends on this patch as well, so I guess
> it would go together with whichever is ready first.
>
> I would also envision someone changing the other existing drivers to
> use the helpers, as I'm pretty much sure some of them don't handle
> suspend/resume correctly.
>

This indeed looks very good. If I understood the issue properly,
the change would be useful for both stateless (e.g. hantro, et al)
and stateful (e.g. coda) codecs.

Hantro uses pm_runtime_force_suspend, and I believe that
could is enough for proper suspend/resume operation.

I'm not seeing any code in CODA to handle this, so not sure
how it's handling suspend/resume.

Maybe we can have CODA as the first user, given it's a well-maintained
driver and should be fairly easy to test.

Regards,
Ezequiel

Tomasz Figa June 10, 2020, 7:03 p.m. UTC | #6

On Wed, Jun 10, 2020 at 03:52:39PM -0300, Ezequiel Garcia wrote:
> Hi everyone,
> 
> Thanks for the patch.
> 
> On Wed, 10 Jun 2020 at 07:33, Tomasz Figa <[e-mail]> wrote:
> >
> > On Wed, Jun 10, 2020 at 12:29 PM Hans Verkuil <[e-mail]> wrote:
> > >
> > > On 21/05/2020 19:11, Tomasz Figa wrote:
> > > > Hi Jerry,
> > > >
> > > > On Wed, Dec 04, 2019 at 08:47:29PM +0800, Jerry-ch Chen wrote:
> > > >> From: Pi-Hsun Shih <[e-mail]>
> > > >>
> > > >> 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.
[snip]
> > >
> > > I assume this will be part of a future patch series that calls these new functions?
> >
> > The mtk-jpeg encoder series depends on this patch as well, so I guess
> > it would go together with whichever is ready first.
> >
> > I would also envision someone changing the other existing drivers to
> > use the helpers, as I'm pretty much sure some of them don't handle
> > suspend/resume correctly.
> >
> 
> This indeed looks very good. If I understood the issue properly,
> the change would be useful for both stateless (e.g. hantro, et al)
> and stateful (e.g. coda) codecs.
> 
> Hantro uses pm_runtime_force_suspend, and I believe that
> could is enough for proper suspend/resume operation.

Unfortunately, no. :(

If the decoder is already decoding a frame, that would forcefully power
off the hardware and possibly even cause a system lockup if we are
unlucky to gate a clock in the middle of a bus transaction.

I just inspected the code now and actually found one more bug in its
power management handling. device_run() calls clk_bulk_enable() before
pm_runtime_get_sync(), but only the latter is guaranteed to actually
power on the relevant power domains, so we end up clocking unpowered
hardware.

> 
> I'm not seeing any code in CODA to handle this, so not sure
> how it's handling suspend/resume.
> 
> Maybe we can have CODA as the first user, given it's a well-maintained
> driver and should be fairly easy to test.

I remember checking a number of drivers using the m2m helpers randomly
and none of them implemented suspend/resume correctly. I suppose that
was not discovered because normally the userspace itself would stop the
operation before the system is suspended, although it's not an API
guarantee.

Best regards,
Tomasz

Ezequiel Garcia June 10, 2020, 7:14 p.m. UTC | #7

On Wed, 10 Jun 2020 at 16:03, Tomasz Figa <[e-mail]> wrote:
>
> On Wed, Jun 10, 2020 at 03:52:39PM -0300, Ezequiel Garcia wrote:
> > Hi everyone,
> >
> > Thanks for the patch.
> >
> > On Wed, 10 Jun 2020 at 07:33, Tomasz Figa <[e-mail]> wrote:
> > >
> > > On Wed, Jun 10, 2020 at 12:29 PM Hans Verkuil <[e-mail]> wrote:
> > > >
> > > > On 21/05/2020 19:11, Tomasz Figa wrote:
> > > > > Hi Jerry,
> > > > >
> > > > > On Wed, Dec 04, 2019 at 08:47:29PM +0800, Jerry-ch Chen wrote:
> > > > >> From: Pi-Hsun Shih <[e-mail]>
> > > > >>
> > > > >> 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.
> [snip]
> > > >
> > > > I assume this will be part of a future patch series that calls these new functions?
> > >
> > > The mtk-jpeg encoder series depends on this patch as well, so I guess
> > > it would go together with whichever is ready first.
> > >
> > > I would also envision someone changing the other existing drivers to
> > > use the helpers, as I'm pretty much sure some of them don't handle
> > > suspend/resume correctly.
> > >
> >
> > This indeed looks very good. If I understood the issue properly,
> > the change would be useful for both stateless (e.g. hantro, et al)
> > and stateful (e.g. coda) codecs.
> >
> > Hantro uses pm_runtime_force_suspend, and I believe that
> > could is enough for proper suspend/resume operation.
>
> Unfortunately, no. :(
>
> If the decoder is already decoding a frame, that would forcefully power
> off the hardware and possibly even cause a system lockup if we are
> unlucky to gate a clock in the middle of a bus transaction.
>

pm_runtime_force_suspend calls pm_runtime_disable, which
says:

"""
 Increment power.disable_depth for the device and if it was zero previously,
 cancel all pending runtime PM requests for the device and wait for all
 operations in progress to complete.
"""

Doesn't this mean it waits for the current job (if there is one) and
prevents any new jobs to be issued?

> I just inspected the code now and actually found one more bug in its
> power management handling. device_run() calls clk_bulk_enable() before
> pm_runtime_get_sync(), but only the latter is guaranteed to actually
> power on the relevant power domains, so we end up clocking unpowered
> hardware.
>

How about we just move clk_enable/disable to runtime PM?

Since we use autosuspend delay, it theoretically has
some impact, which is why I was refraining from doing so.

I can't decide if this impact would be marginal or significant.

> >
> > I'm not seeing any code in CODA to handle this, so not sure
> > how it's handling suspend/resume.
> >
> > Maybe we can have CODA as the first user, given it's a well-maintained
> > driver and should be fairly easy to test.
>
> I remember checking a number of drivers using the m2m helpers randomly
> and none of them implemented suspend/resume correctly. I suppose that
> was not discovered because normally the userspace itself would stop the
> operation before the system is suspended, although it's not an API
> guarantee.
>

Indeed. Do you have any recomendations for how we could
test this case to make sure we are handling it correctly?

> Best regards,
> Tomasz

Tomasz Figa June 10, 2020, 7:26 p.m. UTC | #8

On Wed, Jun 10, 2020 at 9:14 PM Ezequiel Garcia
<[e-mail]> wrote:
>
> On Wed, 10 Jun 2020 at 16:03, Tomasz Figa <[e-mail]> wrote:
> >
> > On Wed, Jun 10, 2020 at 03:52:39PM -0300, Ezequiel Garcia wrote:
> > > Hi everyone,
> > >
> > > Thanks for the patch.
> > >
> > > On Wed, 10 Jun 2020 at 07:33, Tomasz Figa <[e-mail]> wrote:
> > > >
> > > > On Wed, Jun 10, 2020 at 12:29 PM Hans Verkuil <[e-mail]> wrote:
> > > > >
> > > > > On 21/05/2020 19:11, Tomasz Figa wrote:
> > > > > > Hi Jerry,
> > > > > >
> > > > > > On Wed, Dec 04, 2019 at 08:47:29PM +0800, Jerry-ch Chen wrote:
> > > > > >> From: Pi-Hsun Shih <[e-mail]>
> > > > > >>
> > > > > >> 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.
> > [snip]
> > > > >
> > > > > I assume this will be part of a future patch series that calls these new functions?
> > > >
> > > > The mtk-jpeg encoder series depends on this patch as well, so I guess
> > > > it would go together with whichever is ready first.
> > > >
> > > > I would also envision someone changing the other existing drivers to
> > > > use the helpers, as I'm pretty much sure some of them don't handle
> > > > suspend/resume correctly.
> > > >
> > >
> > > This indeed looks very good. If I understood the issue properly,
> > > the change would be useful for both stateless (e.g. hantro, et al)
> > > and stateful (e.g. coda) codecs.
> > >
> > > Hantro uses pm_runtime_force_suspend, and I believe that
> > > could is enough for proper suspend/resume operation.
> >
> > Unfortunately, no. :(
> >
> > If the decoder is already decoding a frame, that would forcefully power
> > off the hardware and possibly even cause a system lockup if we are
> > unlucky to gate a clock in the middle of a bus transaction.
> >
>
> pm_runtime_force_suspend calls pm_runtime_disable, which
> says:
>
> """
>  Increment power.disable_depth for the device and if it was zero previously,
>  cancel all pending runtime PM requests for the device and wait for all
>  operations in progress to complete.
> """
>
> Doesn't this mean it waits for the current job (if there is one) and
> prevents any new jobs to be issued?
>

I'd love if the PM runtime subsystem handled job management of all the
driver subsystems automatically, but at the moment it's not aware of
any jobs. :) The description says as much as it says - it stops any
internal jobs of the PM subsystem - i.e. asynchronous suspend/resume
requests. It doesn't have any awareness of V4L2 M2M jobs.

> > I just inspected the code now and actually found one more bug in its
> > power management handling. device_run() calls clk_bulk_enable() before
> > pm_runtime_get_sync(), but only the latter is guaranteed to actually
> > power on the relevant power domains, so we end up clocking unpowered
> > hardware.
> >
>
> How about we just move clk_enable/disable to runtime PM?
>
> Since we use autosuspend delay, it theoretically has
> some impact, which is why I was refraining from doing so.
>
> I can't decide if this impact would be marginal or significant.
>

I'd also refrain from doing this. Clock gating corresponds to the
bigger part of the power savings from runtime power management, since
it stops the dynamic power consumption and only leaves the static
leakage. That said, the Hantro IP blocks have some internal clock
gating as well, so it might not be as pronounced, depending on the
custom vendor integration logic surrounding the Hantro hardware.

Actually even if autosuspend is not used, the runtime PM subsystem has
some internal back-off mechanism based on measured power on and power
off latencies. The driver should call pm_runtime_get_sync() first and
then enable any necessary clocks. I can see that currently inside the
resume callback we have some hardware accesses. If those really need
to be there, they should be surrounded with appropriate clock enable
and clock disable calls.

> > >
> > > I'm not seeing any code in CODA to handle this, so not sure
> > > how it's handling suspend/resume.
> > >
> > > Maybe we can have CODA as the first user, given it's a well-maintained
> > > driver and should be fairly easy to test.
> >
> > I remember checking a number of drivers using the m2m helpers randomly
> > and none of them implemented suspend/resume correctly. I suppose that
> > was not discovered because normally the userspace itself would stop the
> > operation before the system is suspended, although it's not an API
> > guarantee.
> >
>
> Indeed. Do you have any recomendations for how we could
> test this case to make sure we are handling it correctly?

I'd say that a simple offscreen command line gstreamer/ffmpeg decode
with suspend/resume loop in another session should be able to trigger
some issues.

Best regards,
Tomasz

Ezequiel Garcia June 14, 2020, 10:43 p.m. UTC | #9

On Wed, 10 Jun 2020 at 16:26, Tomasz Figa <[e-mail]> wrote:
>
> On Wed, Jun 10, 2020 at 9:14 PM Ezequiel Garcia
> <[e-mail]> wrote:
> >
> > On Wed, 10 Jun 2020 at 16:03, Tomasz Figa <[e-mail]> wrote:
> > >
> > > On Wed, Jun 10, 2020 at 03:52:39PM -0300, Ezequiel Garcia wrote:
> > > > Hi everyone,
> > > >
> > > > Thanks for the patch.
> > > >
> > > > On Wed, 10 Jun 2020 at 07:33, Tomasz Figa <[e-mail]> wrote:
> > > > >
> > > > > On Wed, Jun 10, 2020 at 12:29 PM Hans Verkuil <[e-mail]> wrote:
> > > > > >
> > > > > > On 21/05/2020 19:11, Tomasz Figa wrote:
> > > > > > > Hi Jerry,
> > > > > > >
> > > > > > > On Wed, Dec 04, 2019 at 08:47:29PM +0800, Jerry-ch Chen wrote:
> > > > > > >> From: Pi-Hsun Shih <[e-mail]>
> > > > > > >>
> > > > > > >> 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.
> > > [snip]
> > > > > >
> > > > > > I assume this will be part of a future patch series that calls these new functions?
> > > > >
> > > > > The mtk-jpeg encoder series depends on this patch as well, so I guess
> > > > > it would go together with whichever is ready first.
> > > > >
> > > > > I would also envision someone changing the other existing drivers to
> > > > > use the helpers, as I'm pretty much sure some of them don't handle
> > > > > suspend/resume correctly.
> > > > >
> > > >
> > > > This indeed looks very good. If I understood the issue properly,
> > > > the change would be useful for both stateless (e.g. hantro, et al)
> > > > and stateful (e.g. coda) codecs.
> > > >
> > > > Hantro uses pm_runtime_force_suspend, and I believe that
> > > > could is enough for proper suspend/resume operation.
> > >
> > > Unfortunately, no. :(
> > >
> > > If the decoder is already decoding a frame, that would forcefully power
> > > off the hardware and possibly even cause a system lockup if we are
> > > unlucky to gate a clock in the middle of a bus transaction.
> > >
> >
> > pm_runtime_force_suspend calls pm_runtime_disable, which
> > says:
> >
> > """
> >  Increment power.disable_depth for the device and if it was zero previously,
> >  cancel all pending runtime PM requests for the device and wait for all
> >  operations in progress to complete.
> > """
> >
> > Doesn't this mean it waits for the current job (if there is one) and
> > prevents any new jobs to be issued?
> >
>
> I'd love if the PM runtime subsystem handled job management of all the
> driver subsystems automatically, but at the moment it's not aware of
> any jobs. :) The description says as much as it says - it stops any
> internal jobs of the PM subsystem - i.e. asynchronous suspend/resume
> requests. It doesn't have any awareness of V4L2 M2M jobs.
>

Doh, of course. I saw "pending requests" and
somehow imagined it would wait for the runtime_put.

I see now that these patches are the way to go.

> > > I just inspected the code now and actually found one more bug in its
> > > power management handling. device_run() calls clk_bulk_enable() before
> > > pm_runtime_get_sync(), but only the latter is guaranteed to actually
> > > power on the relevant power domains, so we end up clocking unpowered
> > > hardware.
> > >
> >
> > How about we just move clk_enable/disable to runtime PM?
> >
> > Since we use autosuspend delay, it theoretically has
> > some impact, which is why I was refraining from doing so.
> >
> > I can't decide if this impact would be marginal or significant.
> >
>
> I'd also refrain from doing this. Clock gating corresponds to the
> bigger part of the power savings from runtime power management, since
> it stops the dynamic power consumption and only leaves the static
> leakage. That said, the Hantro IP blocks have some internal clock
> gating as well, so it might not be as pronounced, depending on the
> custom vendor integration logic surrounding the Hantro hardware.
>

OK, I agree. We need to fix this issue then,
changing the order of the calls.

> Actually even if autosuspend is not used, the runtime PM subsystem has
> some internal back-off mechanism based on measured power on and power
> off latencies. The driver should call pm_runtime_get_sync() first and
> then enable any necessary clocks. I can see that currently inside the
> resume callback we have some hardware accesses. If those really need
> to be there, they should be surrounded with appropriate clock enable
> and clock disable calls.
>

Currently, it's only used by imx8mq, and it's enclosed
by clk_bulk_prepare_enable/disable_unprepare.

I am quite sure the prepare/unprepare usage is an oversight
on our side, but it doesn't do any harm either.

Moving the clock handling to hantro_runtime_resume is possible,
but looks like just low-hanging fruit.

> > > >
> > > > I'm not seeing any code in CODA to handle this, so not sure
> > > > how it's handling suspend/resume.
> > > >
> > > > Maybe we can have CODA as the first user, given it's a well-maintained
> > > > driver and should be fairly easy to test.
> > >
> > > I remember checking a number of drivers using the m2m helpers randomly
> > > and none of them implemented suspend/resume correctly. I suppose that
> > > was not discovered because normally the userspace itself would stop the
> > > operation before the system is suspended, although it's not an API
> > > guarantee.
> > >
> >
> > Indeed. Do you have any recomendations for how we could
> > test this case to make sure we are handling it correctly?
>
> I'd say that a simple offscreen command line gstreamer/ffmpeg decode
> with suspend/resume loop in another session should be able to trigger
> some issues.
>

I can try to fix the above clk/pm issue and take this helper
on the same series, if that's useful.

Thanks,
Ezequiel

Patch

diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c b/drivers/media/v4l2-core/v4l2-mem2mem.c
index 5bbdec55b7d7..76ba203e0035 100644
--- a/drivers/media/v4l2-core/v4l2-mem2mem.c
+++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
@@ -47,6 +47,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 */
@@ -88,6 +92,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 {
@@ -105,6 +110,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;
 };
@@ -267,6 +273,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;
@@ -447,6 +459,34 @@  void v4l2_m2m_job_finish(struct v4l2_m2m_dev *m2m_dev,
 }
 EXPORT_SYMBOL(v4l2_m2m_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 5467264771ec..119a195da390 100644
--- a/include/media/v4l2-mem2mem.h
+++ b/include/media/v4l2-mem2mem.h
@@ -183,6 +183,28 @@  v4l2_m2m_buf_done(struct vb2_v4l2_buffer *buf, enum vb2_buffer_state state)
 	vb2_buffer_done(&buf->vb2_buf, state);
 }
 
+/**
+ * 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
  *