| Message ID | 20180911150938.3844-1-mjourdan@baylibre.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Add Amlogic video decoder driver | expand |
Please note: the canvas patches required for this series (and causing the kbuild fail) were merged by Kevin Hilman with a tag. Repo: https://git.kernel.org/pub/scm/linux/kernel/git/khilman/linux-amlogic.git Tag: amlogic-drivers-canvas Regards, Maxime
On 09/11/2018 05:09 PM, Maxime Jourdan wrote: > Hi everyone, > > This patch series adds support for the Amlogic video decoder, > as well as the corresponding dt bindings for GXBB/GXL/GXM chips. > > It features decoding for the following formats: > - MPEG 1 > - MPEG 2 > > The following formats will be added in future patches: > - MJPEG > - MPEG 4 (incl. Xvid, H.263) > - H.264 > - HEVC (incl. 10-bit) > > The following formats' development has still not started, but they are > supported by the hardware: > - VC1 > - VP9 > > The code was made in such a way to allow easy inclusion of those formats > in the future. > > The decoder is single instance. > > Files: > - vdec.c handles the V4L2 M2M logic > - esparser.c manages the hardware bitstream parser > - vdec_helpers.c provides helpers to DONE the dst buffers as well as > various common code used by the codecs > - vdec_1.c manages the VDEC_1 block of the vdec IP > - codec_mpeg12.c enables decoding for MPEG 1/2. > - vdec_platform.c links codec units with vdec units > (e.g vdec_1 with codec_mpeg12) and lists all the available > src/dst formats and requirements (max width/height, etc.), > per compatible chip. > > Firmwares are necessary to run the vdec. They can currently be found at: > https://github.com/chewitt/meson-firmware > > It was tested primarily with ffmpeg's v4l2-m2m implementation. For instance: > $ ffmpeg -c:v mpeg2_v4l2m2m -i sample_mpeg2.mkv -f null - > > Note: This patch series depends on > "[PATCH v3 0/3] soc: amlogic: add meson-canvas" > https://patchwork.kernel.org/cover/10573763/ > > The v4l2-compliance results are available below the patch diff. > > Changes since v1 [0]: > - use named interrupts in the bindings > - rewrite description in the bindings doc > - don't include the dts changes in the patch series > - fill the vb2 queues locks > - fill the video_device lock > - use helpers for wait_prepare and wait_finish vb2_ops > - remove unnecessary usleep in between esparser writes. > Extensive testing of every codec on GXBB/GXL didn't reveal > any fails without it, so just remove it. > - compile v4l2_compliance inside the git repo > - Check for plane number/plane size to pass the latest v4l2-compliance test > - Moved the single instance check (returning -EBUSY) to start/stop streaming > The check was previously in queue_setup but there was no great location to > clear it except for .close(). Actually, you can clear it by called VIDIOC_REQBUFS with count set to 0. That freed all buffers and clears this. Now, the difference between queue_setup and start/stop streaming is that if you do this in queue_setup you'll know early on that the device is busy. It is reasonable to assume that you only allocate buffers when you also want to start streaming, so that it a good place to know this quickly. Whereas with start_streaming you won't know until you call STREAMON, or even later if you start streaming with no buffers queued, since start_streaming won't be called until you have at least 'min_buffers_needed' buffers queued (1 for this driver). So in that case EBUSY won't be returned until the first VIDIOC_QBUF. My preference is to check this in queue_setup, but it is up to you to decide. Just be aware of the difference between the two options. Regards, Hans
2018-09-17 16:51 GMT+02:00 Hans Verkuil <hverkuil@xs4all.nl>: > On 09/11/2018 05:09 PM, Maxime Jourdan wrote: >> - Moved the single instance check (returning -EBUSY) to start/stop streaming >> The check was previously in queue_setup but there was no great location to >> clear it except for .close(). > > Actually, you can clear it by called VIDIOC_REQBUFS with count set to 0. That > freed all buffers and clears this. > > Now, the difference between queue_setup and start/stop streaming is that if you > do this in queue_setup you'll know early on that the device is busy. It is > reasonable to assume that you only allocate buffers when you also want to start > streaming, so that it a good place to know this quickly. > > Whereas with start_streaming you won't know until you call STREAMON, or even later > if you start streaming with no buffers queued, since start_streaming won't > be called until you have at least 'min_buffers_needed' buffers queued (1 for this > driver). So in that case EBUSY won't be returned until the first VIDIOC_QBUF. > > My preference is to check this in queue_setup, but it is up to you to decide. > Just be aware of the difference between the two options. > > Regards, > > Hans I could for instance keep track of which queue(s) have been called with queue_setup, catch calls to VIDIOC_REQBUFS with count set to 0, and clear the current session once both queues have been reset ? You leverage another issue with min_buffers_needed. It's indeed set to 1 but this value is wrong for the CAPTURE queue. The problem is that this value changes depending on the codec and the amount of CAPTURE buffers requested by userspace. Ultimately I want it set to the total amount of CAPTURE buffers, because the hardware needs the full buffer list before starting a decode job. Am I free to change this queue parameter later, or is m2m_queue_init the only place to do it ? Thanks, Maxime
On 09/17/18 18:36, Maxime Jourdan wrote: > 2018-09-17 16:51 GMT+02:00 Hans Verkuil <hverkuil@xs4all.nl>: >> On 09/11/2018 05:09 PM, Maxime Jourdan wrote: >>> - Moved the single instance check (returning -EBUSY) to start/stop streaming >>> The check was previously in queue_setup but there was no great location to >>> clear it except for .close(). >> >> Actually, you can clear it by called VIDIOC_REQBUFS with count set to 0. That >> freed all buffers and clears this. >> >> Now, the difference between queue_setup and start/stop streaming is that if you >> do this in queue_setup you'll know early on that the device is busy. It is >> reasonable to assume that you only allocate buffers when you also want to start >> streaming, so that it a good place to know this quickly. >> >> Whereas with start_streaming you won't know until you call STREAMON, or even later >> if you start streaming with no buffers queued, since start_streaming won't >> be called until you have at least 'min_buffers_needed' buffers queued (1 for this >> driver). So in that case EBUSY won't be returned until the first VIDIOC_QBUF. >> >> My preference is to check this in queue_setup, but it is up to you to decide. >> Just be aware of the difference between the two options. >> >> Regards, >> >> Hans > > I could for instance keep track of which queue(s) have been called > with queue_setup, catch calls to VIDIOC_REQBUFS with count set to 0, > and clear the current session once both queues have been reset ? I see your point, this is rather awkward. The real problem here is that we don't have a 'queue_free' callback. If we'd had that this would be a lot easier. In any case, I am dropping my objections to doing this in start/stop_streaming. > You leverage another issue with min_buffers_needed. It's indeed set to > 1 but this value is wrong for the CAPTURE queue. The problem is that > this value changes depending on the codec and the amount of CAPTURE > buffers requested by userspace. > Ultimately I want it set to the total amount of CAPTURE buffers, > because the hardware needs the full buffer list before starting a > decode job. > Am I free to change this queue parameter later, or is m2m_queue_init > the only place to do it ? It has to be set before the VIDIOC_STREAMON. After that you cannot change it anymore. But I don't think this is all that relevant, since this is something that the job_ready() callback should take care of. min_buffers_needed is really for hardware where the DMA engine cannot start unless that many buffers are queued. But in that case the DMA runs continuously capturing video, whereas here these are jobs and the DMA is only started when you can actually execute a job. Regards, Hans
Le ven. 21 sept. 2018 à 12:51, Hans Verkuil <hverkuil@xs4all.nl> a écrit : > > On 09/17/18 18:36, Maxime Jourdan wrote: > > 2018-09-17 16:51 GMT+02:00 Hans Verkuil <hverkuil@xs4all.nl>: > >> On 09/11/2018 05:09 PM, Maxime Jourdan wrote: > >>> - Moved the single instance check (returning -EBUSY) to start/stop streaming > >>> The check was previously in queue_setup but there was no great location to > >>> clear it except for .close(). > >> > >> Actually, you can clear it by called VIDIOC_REQBUFS with count set to 0. That > >> freed all buffers and clears this. > >> > >> Now, the difference between queue_setup and start/stop streaming is that if you > >> do this in queue_setup you'll know early on that the device is busy. It is > >> reasonable to assume that you only allocate buffers when you also want to start > >> streaming, so that it a good place to know this quickly. > >> > >> Whereas with start_streaming you won't know until you call STREAMON, or even later > >> if you start streaming with no buffers queued, since start_streaming won't > >> be called until you have at least 'min_buffers_needed' buffers queued (1 for this > >> driver). So in that case EBUSY won't be returned until the first VIDIOC_QBUF. > >> > >> My preference is to check this in queue_setup, but it is up to you to decide. > >> Just be aware of the difference between the two options. > >> > >> Regards, > >> > >> Hans > > > > I could for instance keep track of which queue(s) have been called > > with queue_setup, catch calls to VIDIOC_REQBUFS with count set to 0, > > and clear the current session once both queues have been reset ? > > I see your point, this is rather awkward. The real problem here is that > we don't have a 'queue_free' callback. If we'd had that this would be > a lot easier. > > In any case, I am dropping my objections to doing this in start/stop_streaming. Ack. > > You leverage another issue with min_buffers_needed. It's indeed set to > > 1 but this value is wrong for the CAPTURE queue. The problem is that > > this value changes depending on the codec and the amount of CAPTURE > > buffers requested by userspace. > > Ultimately I want it set to the total amount of CAPTURE buffers, > > because the hardware needs the full buffer list before starting a > > decode job. > > Am I free to change this queue parameter later, or is m2m_queue_init > > the only place to do it ? > > It has to be set before the VIDIOC_STREAMON. After that you cannot > change it anymore. > > But I don't think this is all that relevant, since this is something > that the job_ready() callback should take care of. min_buffers_needed > is really for hardware where the DMA engine cannot start unless that > many buffers are queued. But in that case the DMA runs continuously > capturing video, whereas here these are jobs and the DMA is only > started when you can actually execute a job. After doing some testing, overriding min_buffers_needed in queue_setup is what works best for me. When doing the initialization in start_streaming, the complete buffer list needs to be configured in HW. The firmware can then choose any buffer from this list during decoding later on. I do this initialization by iterating with v4l2_m2m_for_each_dst_buf, which requires all CAPTURE buffers to be queued in. Cheers, Maxime > Regards, > > Hans