Messages in this thread | | | Subject | Re: [PATCH v3 3/9] media: venus: adding core part and helper functions | From | Hans Verkuil <> | Date | Mon, 14 Nov 2016 11:25:36 +0100 |
| |
On 11/14/2016 11:11 AM, Stanimir Varbanov wrote: > Hi Hans, > > <cut> > >>>> >>>>> +void vidc_vb2_stop_streaming(struct vb2_queue *q) >>>>> +{ >>>>> + struct venus_inst *inst = vb2_get_drv_priv(q); >>>>> + struct venus_core *core = inst->core; >>>>> + struct device *dev = core->dev; >>>>> + struct vb2_queue *other_queue; >>>>> + struct vidc_buffer *buf, *n; >>>>> + enum vb2_buffer_state state; >>>>> + int ret; >>>>> + >>>>> + if (q->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) >>>>> + other_queue = &inst->bufq_cap; >>>>> + else >>>>> + other_queue = &inst->bufq_out; >>>>> + >>>>> + if (!vb2_is_streaming(other_queue)) >>>>> + return; >>>> >>>> This seems wrong to me: this return means that the buffers of queue q are never >>>> released. Either drop this 'if' or release both queues when the last queue >>>> stops streaming. I think dropping the 'if' is best. >>> >>> I have done this way because hfi_session_stop must be called only once, >>> and buffers will be released on first streamoff for both queues. >> >> Are you sure the buffers are released for both queues? I may have missed that when >> reviewing. > > yes, hfi_session_stop will instruct the firmware to stop using provided > buffers and return ownership to the host driver by fill_buf_done and > empty_buf_done callbacks. > >> >> I would recommend to call hfi_session_stop when the first stop_streaming is called, >> not when it is called for both queues. I say this because stopping streaming without >> releasing the buffers is likely to cause problems. > > this is what I tried to implement with above > !vb2_is_streaming(other_queue) thing.
That doesn't work: if the application calls STREAMON(CAPTURE) followed by STREAMOFF(CAPTURE) without ever starting the OUTPUT queue, this will not clean up the capture queue.
Regards,
Hans
> >> >> Did you turn on CONFIG_VIDEO_ADV_DEBUG? If it is on, and you don't release buffers >> then I think you will see warnings in the kernel log. > > OK I will enable it to be sure that warnings are missing. >
| |