Re: [PATCH v2 1/5] v4l2-mem2mem: Fix missing v4l2_m2m_try_run call

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 27/07/18 15:35, Hans Verkuil wrote:
> On 25/07/18 19:15, Ezequiel Garcia wrote:
>> Commit 34dbb848d5e4 ("media: mem2mem: Remove excessive try_run call")
>> removed a redundant call to v4l2_m2m_try_run but instead introduced
>> a bug. Consider the following case:
>>
>>  1) Context A schedules, queues and runs job A.
>>  2) While the m2m device is running, context B schedules
>>     and queues job B. Job B cannot run, because it has to
>>     wait for job A.
>>  3) Job A completes, calls v4l2_m2m_job_finish, and tries
>>     to queue a job for context A, but since the context is
>>     empty it won't do anything.
>>
>> In this scenario, queued job B will never run. Fix this by calling
>> v4l2_m2m_try_run from v4l2_m2m_try_schedule.
>>
>> While here, add more documentation to these functions.
>>
>> Fixes: 34dbb848d5e4 ("media: mem2mem: Remove excessive try_run call")
>> Signed-off-by: Ezequiel Garcia <ezequiel@xxxxxxxxxxxxx>
> 
> So just to be clear: this first patch fixes a regression and can be applied
> separately from the other patches?
> 
>> ---
>>  drivers/media/v4l2-core/v4l2-mem2mem.c | 32 +++++++++++++++++++++++---
>>  1 file changed, 29 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c b/drivers/media/v4l2-core/v4l2-mem2mem.c
>> index 5f9cd5b74cda..dfd796621b06 100644
>> --- a/drivers/media/v4l2-core/v4l2-mem2mem.c
>> +++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
>> @@ -209,15 +209,23 @@ static void v4l2_m2m_try_run(struct v4l2_m2m_dev *m2m_dev)
>>  	m2m_dev->curr_ctx->job_flags |= TRANS_RUNNING;
>>  	spin_unlock_irqrestore(&m2m_dev->job_spinlock, flags);
>>  
>> +	dprintk("Running job on m2m_ctx: %p\n", m2m_dev->curr_ctx);
> 
> Is this intended? It feels out of place in this patch.
> 
>>  	m2m_dev->m2m_ops->device_run(m2m_dev->curr_ctx->priv);
>>  }
>>  
>> -void v4l2_m2m_try_schedule(struct v4l2_m2m_ctx *m2m_ctx)
>> +/*
>> + * __v4l2_m2m_try_queue() - queue a job
>> + * @m2m_dev: m2m device
>> + * @m2m_ctx: m2m context
>> + *
>> + * Check if this context is ready to queue a job.
>> + *
>> + * This function can run in interrupt context.
>> + */
>> +static void __v4l2_m2m_try_queue(struct v4l2_m2m_dev *m2m_dev, struct v4l2_m2m_ctx *m2m_ctx)
>>  {
>> -	struct v4l2_m2m_dev *m2m_dev;
>>  	unsigned long flags_job, flags_out, flags_cap;
>>  
>> -	m2m_dev = m2m_ctx->m2m_dev;
>>  	dprintk("Trying to schedule a job for m2m_ctx: %p\n", m2m_ctx);
>>  
>>  	if (!m2m_ctx->out_q_ctx.q.streaming
>> @@ -275,7 +283,25 @@ void v4l2_m2m_try_schedule(struct v4l2_m2m_ctx *m2m_ctx)
>>  	m2m_ctx->job_flags |= TRANS_QUEUED;
>>  
>>  	spin_unlock_irqrestore(&m2m_dev->job_spinlock, flags_job);
>> +}
>> +
>> +/**
>> + * v4l2_m2m_try_schedule() - schedule and possibly run a job for any context
>> + * @m2m_ctx: m2m context
>> + *
>> + * Check if this context is ready to queue a job. If suitable,
>> + * run the next queued job on the mem2mem device.
>> + *
>> + * This function shouldn't run in interrupt context.
>> + *
>> + * Note that v4l2_m2m_try_schedule() can schedule one job for this context,
>> + * and then run another job for another context.
>> + */
>> +void v4l2_m2m_try_schedule(struct v4l2_m2m_ctx *m2m_ctx)
>> +{
>> +	struct v4l2_m2m_dev *m2m_dev = m2m_ctx->m2m_dev;
>>  
>> +	__v4l2_m2m_try_queue(m2m_dev, m2m_ctx);
> 
> Why introduce __v4l2_m2m_try_queue? Why not keep it in here?
> 
>>  	v4l2_m2m_try_run(m2m_dev);

I'm completely confused: v4l2_m2m_try_schedule() already calls v4l2_m2m_try_run()!

Either my brain has crashed due to the heatwave I'm suffering through, or there
is something amiss with this patch.

Regards,

	Hans

>>  }
>>  EXPORT_SYMBOL_GPL(v4l2_m2m_try_schedule);
>>
> 
> Regards,
> 
> 	Hans
> 




[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux