RE: [PATCH v2] [media] s5p-mfc: Dequeue sequence header after STREAMON

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

 



Hi Arun,

> From: linux-media-owner@xxxxxxxxxxxxxxx [mailto:linux-media-
> owner@xxxxxxxxxxxxxxx] On Behalf Of Arun Kumar K
> Sent: Wednesday, May 14, 2014 10:10 AM
> 
> Hi Hans,
> 
> On 05/14/14 12:39, Hans Verkuil wrote:
> > On 05/14/2014 08:29 AM, Arun Kumar K wrote:
> >> MFCv6 encoder needs specific minimum number of buffers to be queued
> >> in the CAPTURE plane. This minimum number will be known only when
> the
> >> sequence header is generated.
> >> So we used to allow STREAMON on the CAPTURE plane only after
> sequence
> >> header is generated and checked with the minimum buffer requirement.
> >>
> >> But this causes a problem that we call a vb2_buffer_done for the
> >> sequence header buffer before doing a STREAON on the CAPTURE plane.
> >
> > How could this ever have worked? Buffers aren't queued to the driver
> > until STREAMON is called, and calling vb2_buffer_done for a buffer
> > that is not queued first to the driver will mess up internal data (q-
> >queued_count for one).
> >
> 
> This worked till now because __enqueue_in_driver is called first and
> then start_streaming qop is called. In MFCv6, the start_streaming
> driver callback used to wait till sequence header interrupt is received
> and it used to do vb2_buffer_done in that interrupt context. So it
> happened after buffers are enqueued in driver and before completing the
> vb2_streamon.
> 
> >> This used to still work fine until this patch was merged -
> >> b3379c6 : vb2: only call start_streaming if sufficient buffers are
> >> queued
> >
> > Are you testing with CONFIG_VIDEO_ADV_DEBUG set? If not, you should
> do
> > so. That will check whether all the vb2 calls are balanced.
> >
> > BTW, that's a small typo in s5p_mfc_enc.c (search for 'inavlid').
> >
> 
> I got it. Will post a patch fixing them. Thanks for spotting this.
> 
> >> This problem should also come in earlier MFC firmware versions if
> the
> >> application calls STREAMON on CAPTURE with some delay after doing
> >> STREAMON on OUTPUT.
> >
> > You can also play around with the min_buffers_needed field. My
> > rule-of-thumb is that when start_streaming is called everything
> should
> > be ready to stream. It is painful for drivers to have to keep track
> of the 'do I have enough buffers' status.
> >
> > For that reason I introduced the min_buffers_needed field. What I
> > believe you can do here is to set it initially to a large value,
> > preventing start_streaming from being called, and once you really
> know
> > the minimum number of buffers that you need it can be updated again
> to the actual value.
> 
> If a large value is kept in min_buffers_needed, there will be some
> unnecessary memory initialization needed for say 16 full HD raw YUV
> buffers when actual needed is only 4. And once the encoding is started,
> updating the min_buffers_needed to actual value doesnt give any
> advantage as nobody checks for it after that.
> So the whole idea is to not enforce a worst case buffer allocation
> requirement beforehand itself. I hope the current scheme of things
> works well for the requirement.

I was looking in the code of the MFC encoder and handling of this situation
seems wrong to me.

You say that a minimum number of buffers has to be queued for MFC encoder to
work. But this number is not checked in s5p_mfc_ctx_ready in s5p_mfc_enc.c.

It is only checked during reqbufs. This way it does not ensure that there is
a minimum number of buffers queued.

Also there is a control V4L2_CID_MIN_BUFFERS_FOR_CAPTURE, maybe it could be
used
in this context?

Another thing - you say that header is generated to a CAPTURE buffer before
STREAMON on CAPTURE was done. Is this correct? Can the hardware/driver write
to a queued buffer without streaming enabled? Hans, Sylwester?

Arun, is there a way to guess the needed number of buffers from controls?
Isn't this
related with number of B frames? I understand how this affects the number of
buffers for OUTPUT, but I thought that a single CAPTURE buffer is always
enough.
I understood that a generated compressed stream is no longer used after it
was
created and its processing is finished.

I think we need some discussion on this patch.
 
Best wishes,
-- 
Kamil Debski
Samsung R&D Institute Poland



--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux SoC Development]     [Linux Rockchip Development]     [Linux USB Development]     [Video for Linux]     [Linux Audio Users]     [Linux SCSI]     [Yosemite News]

  Powered by Linux