Re: [RFC/PATCH v6 0/4] Multi Format Codec 5.1 driver for S5PC110 SoC

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

 



Hi Kamil,

I still need to review this carefully since this is the first codec driver.
I had hoped to do this during the weekend, but I didn't manage it. I hope I
can get to it on Friday.

One thing I noticed: you aren't using the control framework in this driver.
Please switch to that. From now on I will NACK any new driver that is not
using that framework. I'm in the process of converting all existing drivers
to it, and I don't want to have to fix new drivers as well :-)

Documentation is in Documentation/video4linux/v4l2-controls.txt.

Regards,

	Hans

On Friday, January 07, 2011 17:25:30 Kamil Debski wrote:
> Hello,
> 
> This is the sixth version of the MFC 5.1 driver, decoding part. The suggestions
> and comments from the group members have been very helpful.
> 
> I would be grateful for your comments. Original cover letter ant detailed change
> log has been attached below.
> 
> Best regards,
> Kamil Debski
> 
> * Changelog:
> 
> ==================
>  Changes since v5
> ==================
> 
> 1) Changes suggested by Hans Verkuil:
> - small change in videodev2.h - corrected control offsets
> - made the code more readable by simplifying if statements and using temporary
>   pointers
> - mfc_mutex is now included in s5p_mfc_dev structure
> - after discussion with Peter Oh modification of fourcc defintions
>  (replaced DX52 and DX53 with DX50)
> 
> 2) Changes suggested by JongHun Han:
> - comsmetic changed of defines in regs-mfc5.h
> - in buffers that have no width adn height, such as the buffer for compressed
>   stream, those values are set to 0 instead of 1
> - remove redundant pointer to MFC registers
> - change name of the union in s5p_mfc_buf from paddr to cookie
> - removed global variable (struct s5p_mfc_dev *dev) and moved to use video_drvdata
> 
> 3) Other changes:
> - added check for values returned after parsing header - in rare circumstances MFC
>   hw returned 0x0 as image size and this could cause problems
> 
> ==================
>  Changes since v4
> ==================
> 
> 1) Changes suggested by Kukjin Kim from:
> - removed comment arch/arm/mach-s5pv210/include/mach/map.h
> - changed device name to s5p-mfc (removed "5", MFC version number)
>   also removed the version number from the name of MFC device file
> - added GPL license to arch/arm/plat-s5p/dev-mfc.c
> - removed unused include file from dev-mfc.c and unnecessary comments
> 
> 2) Cache handling improvement:
> - changed cache handling to use dma_map_single and dma_unmap_single
> 
> ==================
>  Changes since v3
> ==================
> 
> 1) Update to the v6 videobuf2 API (here thanks go to Marek Szyprowski)
> - s5p_mfc_buf_negotiate and s5p_mfc_buf_setup_plane functions
> have been merged
> - queue initialization has been adapted to the new API
> - use of the allocator memops has been changed, now there are single
> memops for all the allocator contexts
> 
> 2) Split of the s5p_mfc_try_run and s5p_mfc_handle_frame_int functions
> - parts of the s5p_mfc_try_run function have been moved to separate
> functions (s5p_mfc_get_new_ctx, s5p_mfc_run_dec_last_frames,
> s5p_mfc_run_dec_frame, s5p_mfc_run_get_inst_no, s5p_mfc_run_return_inst
> s5p_mfc_run_init_dec,s5p_mfc_run_init_dec_buffers)
> - s5p_mfc_handle_frame_int has been split to the following functions:
> s5p_mfc_handle_frame_all_extracted, s5p_mfc_handle_frame_new
> and s5p_mfc_handle_frame to handle different cases
> 
> 3) Remove remaining magic numbers and tidy up comments
> 
> ==================
>  Changes since v2
> ==================
> 
> 1) Update to newest videobuf2 API
> This is the major change from v2. The videobuf2 API will hopefully have no more
> major API changes. Buffer initialization has been moved from buf_prepare
> callback to buf_init to simplify the process. Locking mechanism has been
> modified to the requirements of new videobuf2 version.
> 2) Code cleanup
> Removed more magic contants and replaced them with appropriate defines. Changed
> code to use unlocked_ioctl instead of ioctl in v4l2 file ops.
> 3) Allocators
> All internal buffer allocations are done using the selected vb2 allocator,
> instead of using CMA functions directly.
> 
> ==================
>  Changes since v1
> ==================
> 
> 1) Cleanup accoridng to Peter Oh suggestions on the mailing list (Thanks).
> 
> * Original cover letter:
> 
> ==============
>  Introduction
> ==============
> 
> The purpose of this RFC is to discuss the driver for a hw video codec
> embedded in the new Samusng's SoCs. Multi Format Codec 5.0 is able to
> handle video decoding of in a range of formats.
> 
> So far no hardware codec was supported in V4L2 and this would be the
> first one. I guess there are more similar device that would benefit from
> a V4L2 unified interface. I suggest a separate control class for codec
> devices - V4L2_CTRL_CLASS_CODEC.
> 
> Internally the driver uses videobuf2 framework and CMA memory allocator.
> I am aware that those have not yet been merged, but I wanted to start
> discussion about the driver earlier so it could be merged sooner. The
> driver posted here is the initial version, so I suppose it will require
> more work.
> 
> ==================
>  Device interface
> ==================
> 
> The driver principle is based on the idea of memory-to-memory devices:
> it provides a single video node and each opened file handle gets its own
> private context with separate buffer queues. Each context consist of 2
> buffer queues: OUTPUT (for source buffers, i.e. encoded video frames)
> and CAPTURE (for destination buffers, i.e. decoded raw video frames).
> The process of decoding video data from stream is a bit more complicated
> than typical memory-to-memory processing, that's why the m2m framework
> is not directly used (it is too limited for this case). The main reason
> for this is the fact that the CAPTURE buffers can be dequeued in a
> different order than they queued. The hw block decides which buffer has
> been completely processed. This is due to the structure of most
> compressed video streams - use of B frames causes that decoding and
> display order may be different.
> 
> ==============================
>  Decoding initialization path
> ==============================
> 
> First the OUTPUT queue is initialized. With S_FMT the application
> chooses which video format to decode and what size should be the input
> buffer. Fourcc values have been defined for different codecs e.g.
> V4L2_PIX_FMT_H264 for h264. Then the OUTPUT buffers are requested and
> mmaped. The stream header frame is loaded into the first buffer, queued
> and streaming is enabled. At this point the hardware is able to start
> processing the stream header and afterwards it will have information
> about the video dimensions and the size of the buffers with raw video
> data.
> 
> The next step is setting up the CAPTURE queue and buffers. The width,
> height, buffer size and minimum number of buffers can be read with G_FMT
> call. The application can request more output buffer if necessary. After
> requesting and mmaping buffers the device is ready to decode video
> stream.
> 
> The stream frames (ES frames) are written to the OUTPUT buffers, and
> decoded video frames can be read from the CAPTURE buffers. When no more
> source frames are present a single buffer with bytesused set to 0 should
> be queued. This will inform the driver that processing should be
> finished and it can dequeue all video frames that are still left. The
> number of such frames is dependent on the stream and its internal
> structure (how many frames had to be kept as reference frames for
> decoding, etc).
> 
> ===============
>  Usage summary
> ===============
> 
> This is a step by step summary of the video decoding (from user
> application point of view, with 2 treads and blocking api):
> 
> 01. S_FMT(OUTPUT, V4L2_PIX_FMT_H264, ...)
> 02. REQ_BUFS(OUTPUT, n)
> 03. for i=1..n MMAP(OUTPUT, i)
> 04. put stream header to buffer #1
> 05. QBUF(OUTPUT, #1)
> 06. STREAM_ON(OUTPUT)
> 07. G_FMT(CAPTURE)
> 08. REQ_BUFS(CAPTURE, m)
> 09. for j=1..m MMAP(CAPTURE, j)
> 10. for j=1..m QBUF(CAPTURE, #j)
> 11. STREAM_ON(CAPTURE)
> 
> display thread:
> 12. DQBUF(CAPTURE) -> got decoded video data in buffer #j
> 13. display buffer #j
> 14. QBUF(CAPTURE, #j)
> 15. goto 12
> 
> parser thread:
> 16. put next ES frame to buffer #i
> 17. QBUF(OUTPUT, #i)
> 18. DQBUF(OUTPUT) -> get next empty buffer #i 19. goto 16
> 
> ...
> 
> Similar usage sequence can be achieved with single threaded application
> and non-blocking api with poll() call.
> 
> Branch with MFC, CMA and videobuf2 will be soon available at
> http://git.infradead.org/users/kmpark/linux-2.6-samsung/shortlog/refs/heads/vb2-mfc-fimc
> This tree is based on 2.6.37 rc4.
> 
> Please have a look at the code and the idea of how to introduce codec
> devices to V4L2. Comments will be very much appreciated.
> 
> Patch summary:
> 
> 
> 
> Subject: [PATCH 0/4] *** SUBJECT HERE ***
> 
> *** BLURB HERE ***
> 
> Kamil Debski (4):
>   Changes in include/linux/videodev2.h for MFC
>   MFC: Add MFC 5.1 driver to plat-s5p and mach-s5pv210
>   MFC: Add MFC 5.1 V4L2 driver
>   s5pv210: Enable MFC on Goni
> 
>  arch/arm/mach-s5pv210/Kconfig                   |    1 +
>  arch/arm/mach-s5pv210/clock.c                   |    6 +
>  arch/arm/mach-s5pv210/include/mach/map.h        |    4 +
>  arch/arm/mach-s5pv210/mach-goni.c               |    3 +-
>  arch/arm/plat-s5p/Kconfig                       |    5 +
>  arch/arm/plat-s5p/Makefile                      |    2 +-
>  arch/arm/plat-s5p/dev-mfc.c                     |   49 +
>  arch/arm/plat-samsung/include/plat/devs.h       |    2 +
>  drivers/media/video/Kconfig                     |    8 +
>  drivers/media/video/Makefile                    |    1 +
>  drivers/media/video/s5p-mfc/Makefile            |    3 +
>  drivers/media/video/s5p-mfc/regs-mfc5.h         |  335 ++++
>  drivers/media/video/s5p-mfc/s5p_mfc.c           | 2072 +++++++++++++++++++++++
>  drivers/media/video/s5p-mfc/s5p_mfc_common.h    |  224 +++
>  drivers/media/video/s5p-mfc/s5p_mfc_ctrls.h     |  173 ++
>  drivers/media/video/s5p-mfc/s5p_mfc_debug.h     |   47 +
>  drivers/media/video/s5p-mfc/s5p_mfc_intr.c      |   92 +
>  drivers/media/video/s5p-mfc/s5p_mfc_intr.h      |   26 +
>  drivers/media/video/s5p-mfc/s5p_mfc_memory.h    |   43 +
>  drivers/media/video/s5p-mfc/s5p_mfc_opr.c       |  885 ++++++++++
>  drivers/media/video/s5p-mfc/s5p_mfc_opr.h       |  160 ++
>  include/linux/videodev2.h                       |   45 +
>  25 files changed, 4233 insertions(+), 3 deletions(-)
>  create mode 100644 arch/arm/plat-s5p/dev-mfc.c
>  create mode 100644 drivers/media/video/s5p-mfc/Makefile
>  create mode 100644 drivers/media/video/s5p-mfc/regs-mfc5.h
>  create mode 100644 drivers/media/video/s5p-mfc/s5p_mfc.c
>  create mode 100644 drivers/media/video/s5p-mfc/s5p_mfc_common.h
>  create mode 100644 drivers/media/video/s5p-mfc/s5p_mfc_ctrls.h
>  create mode 100644 drivers/media/video/s5p-mfc/s5p_mfc_debug.h
>  create mode 100644 drivers/media/video/s5p-mfc/s5p_mfc_intr.c
>  create mode 100644 drivers/media/video/s5p-mfc/s5p_mfc_intr.h
>  create mode 100644 drivers/media/video/s5p-mfc/s5p_mfc_memory.h
>  create mode 100644 drivers/media/video/s5p-mfc/s5p_mfc_opr.c
>  create mode 100644 drivers/media/video/s5p-mfc/s5p_mfc_opr.h
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 

-- 
Hans Verkuil - video4linux developer - sponsored by Cisco
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[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