From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from mailout2.w1.samsung.com ([210.118.77.12]:64837 "EHLO mailout2.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754593Ab2IXMRM (ORCPT ); Mon, 24 Sep 2012 08:17:12 -0400 Message-id: <50604F3C.2060006@samsung.com> Date: Mon, 24 Sep 2012 14:17:00 +0200 From: Tomasz Stanislawski MIME-version: 1.0 To: Hans Verkuil Cc: linux-media@vger.kernel.org, dri-devel@lists.freedesktop.org, airlied@redhat.com, m.szyprowski@samsung.com, kyungmin.park@samsung.com, laurent.pinchart@ideasonboard.com, sumit.semwal@ti.com, daeinki@gmail.com, daniel.vetter@ffwll.ch, robdclark@gmail.com, pawel@osciak.com, linaro-mm-sig@lists.linaro.org, remi@remlab.net, subashrp@gmail.com, mchehab@redhat.com, g.liakhovetski@gmx.de, dmitriyz@google.com, s.nawrocki@samsung.com, k.debski@samsung.com, linux-doc@vger.kernel.org Subject: Re: [PATCHv8 02/26] Documentation: media: description of DMABUF importing in V4L2 References: <1344958496-9373-1-git-send-email-t.stanislaws@samsung.com> <1344958496-9373-3-git-send-email-t.stanislaws@samsung.com> <201208221247.36471.hverkuil@xs4all.nl> In-reply-to: <201208221247.36471.hverkuil@xs4all.nl> Content-type: text/plain; charset=ISO-8859-15 Content-transfer-encoding: 7bit Sender: linux-media-owner@vger.kernel.org List-ID: Hi Hans, Thank you for review. On 08/22/2012 12:47 PM, Hans Verkuil wrote: > On Tue August 14 2012 17:34:32 Tomasz Stanislawski wrote: >> This patch adds description and usage examples for importing >> DMABUF file descriptor in V4L2. >> >> Signed-off-by: Tomasz Stanislawski >> Signed-off-by: Kyungmin Park >> CC: linux-doc@vger.kernel.org >> --- >> Documentation/DocBook/media/v4l/compat.xml | 4 + >> Documentation/DocBook/media/v4l/io.xml | 180 ++++++++++++++++++++ >> .../DocBook/media/v4l/vidioc-create-bufs.xml | 3 +- >> Documentation/DocBook/media/v4l/vidioc-qbuf.xml | 15 ++ >> Documentation/DocBook/media/v4l/vidioc-reqbufs.xml | 47 ++--- >> 5 files changed, 226 insertions(+), 23 deletions(-) >> [snip] >> +&v4l2-plane; in the multi-planar API case). The driver must be switched into >> +DMABUF I/O mode by calling the &VIDIOC-REQBUFS; with the desired buffer type. >> +No buffers (planes) are allocated beforehand, consequently they are not indexed >> +and cannot be queried like mapped buffers with the >> +VIDIOC_QUERYBUF ioctl. > > I disagree with that. Userptr buffers can use QUERYBUF just fine. Even for the > userptr you still have to fill in the buffer index when calling QBUF. > > So I see no reason why you couldn't use QUERYBUF in the DMABUF case. The only > difference is that the fd field is undefined (set to -1 perhaps?) if the bufffer > isn't queued. > > QUERYBUF can be very useful for debugging, for example to see what the status > is of each buffer and how many are queued. > Ok. I agree that QUERYBUF can be useful for debugging. The value of fd field should be the last value passed using QBUF. It would simplify streaming because an application would not have to keep the file descriptor around. >> + >> + >> + Initiating streaming I/O with DMABUF file descriptors >> + >> + >> +&v4l2-requestbuffers; reqbuf; >> + >> +memset (&reqbuf, 0, sizeof (reqbuf)); >> +reqbuf.type = V4L2_BUF_TYPE_VIDEO_CAPTURE; >> +reqbuf.memory = V4L2_MEMORY_DMABUF; >> +reqbuf.count = 1; >> + >> +if (ioctl (fd, &VIDIOC-REQBUFS;, &reqbuf) == -1) { >> + if (errno == EINVAL) >> + printf ("Video capturing or DMABUF streaming is not supported\n"); >> + else >> + perror ("VIDIOC_REQBUFS"); >> + >> + exit (EXIT_FAILURE); > > Let's stick to the kernel coding style, so no ' ' before '(' in function calls. > Same for the other program examples below. > The ' ' before function was used for userptr and mmap usage examples. These examples should be fixed too. >> +} >> + >> + >> + >> + Buffer (plane) file descriptor is passed on the fly with the > > s/Buffer/The buffer/ > >> +&VIDIOC-QBUF; ioctl. In case of multiplanar buffers, every plane can be > > 'Can be', 'should be' or 'must be'? Does it ever make sense to have the same > fd for different planes? Do we have restrictions on this in the userptr case? > I think that we should keep to 'can be'. I see no good reason to prevent the same dmabuf to be used for different planes. Allowing reusing of dmabufs with assistance of data_offset field would allow to pass a 2-planar YUV420 from V4L2-single-plane API to a driver with V4L2-multi-plane API. [snip] >> diff --git a/Documentation/DocBook/media/v4l/vidioc-qbuf.xml b/Documentation/DocBook/media/v4l/vidioc-qbuf.xml >> index 77ff5be..436d21c 100644 >> --- a/Documentation/DocBook/media/v4l/vidioc-qbuf.xml >> +++ b/Documentation/DocBook/media/v4l/vidioc-qbuf.xml >> @@ -109,6 +109,21 @@ they cannot be swapped out to disk. Buffers remain locked until >> dequeued, until the &VIDIOC-STREAMOFF; or &VIDIOC-REQBUFS; ioctl is >> called, or until the device is closed. >> >> + To enqueue a DMABUF buffer applications >> +set the memory field to >> +V4L2_MEMORY_DMABUF and the m.fd >> +field to a file descriptor associated with a DMABUF buffer. When the >> +multi-planar API is used m.fd of the passed array of > > multi-planar API is used the m.fd fields of the passed array of > >> +&v4l2-plane; have to be used instead. When VIDIOC_QBUF is >> +called with a pointer to this structure the driver sets the >> +V4L2_BUF_FLAG_QUEUED flag and clears the >> +V4L2_BUF_FLAG_MAPPED and >> +V4L2_BUF_FLAG_DONE flags in the >> +flags field, or it returns an error code. This >> +ioctl locks the buffer. Buffers remain locked until dequeued, until the >> +&VIDIOC-STREAMOFF; or &VIDIOC-REQBUFS; ioctl is called, or until the device is >> +closed. > > You need to explain what a 'locked buffer' means. I propose following definition: "Locking a buffer means passing it to a driver for an access by hardware. "If an application accesses (reads/writes) a locked buffer then the result is undefined." What is your opinion about proposed definition? > >> + >> Applications call the VIDIOC_DQBUF >> ioctl to dequeue a filled (capturing) or displayed (output) buffer >> from the driver's outgoing queue. They just set the >> diff --git a/Documentation/DocBook/media/v4l/vidioc-reqbufs.xml b/Documentation/DocBook/media/v4l/vidioc-reqbufs.xml >> index d7c9505..20f4323 100644 >> --- a/Documentation/DocBook/media/v4l/vidioc-reqbufs.xml >> +++ b/Documentation/DocBook/media/v4l/vidioc-reqbufs.xml >> @@ -48,28 +48,30 @@ >> >> Description >> >> - This ioctl is used to initiate memory >> -mapped or user pointer >> -I/O. Memory mapped buffers are located in device memory and must be >> -allocated with this ioctl before they can be mapped into the >> -application's address space. User buffers are allocated by >> -applications themselves, and this ioctl is merely used to switch the >> -driver into user pointer I/O mode and to setup some internal structures. >> +This ioctl is used to initiate memory mapped, >> +user pointer or > +linkend="dmabuf">DMABUF based I/O. Memory mapped buffers are located in >> +device memory and must be allocated with this ioctl before they can be mapped >> +into the application's address space. User buffers are allocated by >> +applications themselves, and this ioctl is merely used to switch the driver >> +into user pointer I/O mode and to setup some internal structures. >> +Similarly, DMABUF buffers are allocated by applications through a device >> +driver, and this ioctl only configures the driver into DMABUF I/O mode without >> +performing any direct allocation. >> >> - To allocate device buffers applications initialize all >> -fields of the v4l2_requestbuffers structure. >> -They set the type field to the respective >> -stream or buffer type, the count field to >> -the desired number of buffers, memory >> -must be set to the requested I/O method and the reserved array >> -must be zeroed. When the ioctl >> -is called with a pointer to this structure the driver will attempt to allocate >> -the requested number of buffers and it stores the actual number >> -allocated in the count field. It can be >> -smaller than the number requested, even zero, when the driver runs out >> -of free memory. A larger number is also possible when the driver requires >> -more buffers to function correctly. For example video output requires at least two buffers, >> -one displayed and one filled by the application. >> + To allocate device buffers applications initialize all fields of the >> +v4l2_requestbuffers structure. They set the >> +type field to the respective stream or buffer type, >> +the count field to the desired number of buffers, >> +memory must be set to the requested I/O method and >> +the reserved array must be zeroed. When the ioctl is >> +called with a pointer to this structure the driver will attempt to allocate the >> +requested number of buffers and it stores the actual number allocated in the >> +count field. It can be smaller than the number >> +requested, even zero, when the driver runs out of free memory. A larger number >> +is also possible when the driver requires more buffers to function correctly. >> +For example video output requires at least two buffers, one displayed and one >> +filled by the application. >> When the I/O method is not supported the ioctl >> returns an &EINVAL;. >> >> @@ -102,7 +104,8 @@ as the &v4l2-format; type field. See > __u32 >> memory >> Applications set this field to >> -V4L2_MEMORY_MMAP or >> +V4L2_MEMORY_MMAP, >> +V4L2_MEMORY_DMABUF or >> V4L2_MEMORY_USERPTR. See > />. >> >> > > You also have to update the VIDIOC_CREATE_BUFS ioctl documentation! > > I think the VIDIOC_PREPARE_BUF ioctl documentation is OK, but you might want to > check that yourself, just in case. > Ok. Regards, Tomasz Stanislawski > Regards, > > Hans >