| Message ID | 1476446894-4220-3-git-send-email-thierry.escande@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Thierry, Thanks for the set. A few comments below. On Fri, Oct 14, 2016 at 02:08:14PM +0200, Thierry Escande wrote: > From: Pawel Osciak <posciak@chromium.org> > > When this flag is set for CAPTURE queues by the driver on calling > vb2_queue_init(), it forces the buffers on the queue to be > allocated/mapped with DMA_BIDIRECTIONAL direction flag, instead of > DMA_FROM_DEVICE. This allows the device not only to write to the > buffers, but also read out from them. This may be useful e.g. for codec > hardware, which may be using CAPTURE buffers as reference to decode > other buffers. Just out of curiosity --- when do you return these buffers back to the user? Once they're no longer needed as reference frames? > > This flag is ignored for OUTPUT queues, as we don't want to allow HW to > be able to write to OUTPUT buffers. > > Signed-off-by: Pawel Osciak <posciak@chromium.org> > Tested-by: Pawel Osciak <posciak@chromium.org> > Reviewed-by: Tomasz Figa <tfiga@chromium.org> > Signed-off-by: Thierry Escande <thierry.escande@collabora.com> > --- > drivers/media/v4l2-core/videobuf2-v4l2.c | 8 ++++++-- > include/media/videobuf2-core.h | 4 ++++ > 2 files changed, 10 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/v4l2-core/videobuf2-v4l2.c b/drivers/media/v4l2-core/videobuf2-v4l2.c > index fde1e2d..9255291 100644 > --- a/drivers/media/v4l2-core/videobuf2-v4l2.c > +++ b/drivers/media/v4l2-core/videobuf2-v4l2.c > @@ -659,8 +659,12 @@ int vb2_queue_init(struct vb2_queue *q) > * queues will always initialize waiting_for_buffers to false. > */ > q->quirk_poll_must_check_waiting_for_buffers = true; > - q->dma_dir = V4L2_TYPE_IS_OUTPUT(q->type) > - ? DMA_TO_DEVICE : DMA_FROM_DEVICE; > + > + if (V4L2_TYPE_IS_OUTPUT(q->type)) > + q->dma_dir = DMA_TO_DEVICE; > + else > + q->dma_dir = q->use_dma_bidirectional > + ? DMA_BIDIRECTIONAL : DMA_FROM_DEVICE; > > return vb2_core_queue_init(q); > } > diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h > index 38410dd..e613c74 100644 > --- a/include/media/videobuf2-core.h > +++ b/include/media/videobuf2-core.h > @@ -433,6 +433,9 @@ struct vb2_buf_ops { > * @quirk_poll_must_check_waiting_for_buffers: Return POLLERR at poll when QBUF > * has not been called. This is a vb1 idiom that has been adopted > * also by vb2. > + * @use_dma_bidirectional: use DMA_BIDIRECTIONAL for CAPTURE buffers; this > + * allows HW to read from the CAPTURE buffers in > + * addition to writing; ignored for OUTPUT queues > * @lock: pointer to a mutex that protects the vb2_queue struct. The > * driver can set this to a mutex to let the v4l2 core serialize > * the queuing ioctls. If the driver wants to handle locking > @@ -500,6 +503,7 @@ struct vb2_queue { > unsigned fileio_write_immediately:1; > unsigned allow_zero_bytesused:1; > unsigned quirk_poll_must_check_waiting_for_buffers:1; > + unsigned use_dma_bidirectional:1; This field is in the same struct as dma_dir which it directly affects. How about adding a macro instead to give you the queue DMA direction instead? E.g. #define vb2_dma_dir(q) \ (V4L2_TYPE_IS_OUTPUT((q)->type) ? DMA_TO_DEVICE : \ (q)->use_dma_bidirectional ? DMA_BIDIRECTIONAL : DMA_FROM_DEVICE) I would call this capture_dma_bidirectional as it only affects capture. Or simply choose DMA_BIDIRECTIONAL whenever the flag is set. I wonder what others think. > > struct mutex *lock; > void *owner;
Hi Sakari, On 17/10/2016 12:06, Sakari Ailus wrote: > Hi Thierry, > > Thanks for the set. A few comments below. > > On Fri, Oct 14, 2016 at 02:08:14PM +0200, Thierry Escande wrote: >> From: Pawel Osciak <posciak@chromium.org> >> >> When this flag is set for CAPTURE queues by the driver on calling >> vb2_queue_init(), it forces the buffers on the queue to be >> allocated/mapped with DMA_BIDIRECTIONAL direction flag, instead of >> DMA_FROM_DEVICE. This allows the device not only to write to the >> buffers, but also read out from them. This may be useful e.g. for codec >> hardware, which may be using CAPTURE buffers as reference to decode >> other buffers. > > Just out of curiosity --- when do you return these buffers back to the user? > Once they're no longer needed as reference frames? Tbh, I don't now. This is used by a rockchip vpu driver not yet upstreamed in the chromeos v4.4 kernel tree. Pawel might answer this question I guess. > >> >> This flag is ignored for OUTPUT queues, as we don't want to allow HW to >> be able to write to OUTPUT buffers. >> >> Signed-off-by: Pawel Osciak <posciak@chromium.org> >> Tested-by: Pawel Osciak <posciak@chromium.org> >> Reviewed-by: Tomasz Figa <tfiga@chromium.org> >> Signed-off-by: Thierry Escande <thierry.escande@collabora.com> >> --- >> drivers/media/v4l2-core/videobuf2-v4l2.c | 8 ++++++-- >> include/media/videobuf2-core.h | 4 ++++ >> 2 files changed, 10 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/media/v4l2-core/videobuf2-v4l2.c b/drivers/media/v4l2-core/videobuf2-v4l2.c >> index fde1e2d..9255291 100644 >> --- a/drivers/media/v4l2-core/videobuf2-v4l2.c >> +++ b/drivers/media/v4l2-core/videobuf2-v4l2.c >> @@ -659,8 +659,12 @@ int vb2_queue_init(struct vb2_queue *q) >> * queues will always initialize waiting_for_buffers to false. >> */ >> q->quirk_poll_must_check_waiting_for_buffers = true; >> - q->dma_dir = V4L2_TYPE_IS_OUTPUT(q->type) >> - ? DMA_TO_DEVICE : DMA_FROM_DEVICE; >> + >> + if (V4L2_TYPE_IS_OUTPUT(q->type)) >> + q->dma_dir = DMA_TO_DEVICE; >> + else >> + q->dma_dir = q->use_dma_bidirectional >> + ? DMA_BIDIRECTIONAL : DMA_FROM_DEVICE; >> >> return vb2_core_queue_init(q); >> } >> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h >> index 38410dd..e613c74 100644 >> --- a/include/media/videobuf2-core.h >> +++ b/include/media/videobuf2-core.h >> @@ -433,6 +433,9 @@ struct vb2_buf_ops { >> * @quirk_poll_must_check_waiting_for_buffers: Return POLLERR at poll when QBUF >> * has not been called. This is a vb1 idiom that has been adopted >> * also by vb2. >> + * @use_dma_bidirectional: use DMA_BIDIRECTIONAL for CAPTURE buffers; this >> + * allows HW to read from the CAPTURE buffers in >> + * addition to writing; ignored for OUTPUT queues >> * @lock: pointer to a mutex that protects the vb2_queue struct. The >> * driver can set this to a mutex to let the v4l2 core serialize >> * the queuing ioctls. If the driver wants to handle locking >> @@ -500,6 +503,7 @@ struct vb2_queue { >> unsigned fileio_write_immediately:1; >> unsigned allow_zero_bytesused:1; >> unsigned quirk_poll_must_check_waiting_for_buffers:1; >> + unsigned use_dma_bidirectional:1; > > This field is in the same struct as dma_dir which it directly affects. > > How about adding a macro instead to give you the queue DMA direction > instead? > > E.g. > > #define vb2_dma_dir(q) \ > (V4L2_TYPE_IS_OUTPUT((q)->type) ? DMA_TO_DEVICE : \ > (q)->use_dma_bidirectional ? DMA_BIDIRECTIONAL : DMA_FROM_DEVICE) > > I would call this capture_dma_bidirectional as it only affects capture. Or > simply choose DMA_BIDIRECTIONAL whenever the flag is set. Sure. Will do. Regards, Thierry -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Oct 18, 2016 at 06:08:53PM +0200, Thierry Escande wrote:
> >#define vb2_dma_dir(q) \
^
VB2_DMA_DIR(), as most of our other macros use capitals as well.
diff --git a/drivers/media/v4l2-core/videobuf2-v4l2.c b/drivers/media/v4l2-core/videobuf2-v4l2.c index fde1e2d..9255291 100644 --- a/drivers/media/v4l2-core/videobuf2-v4l2.c +++ b/drivers/media/v4l2-core/videobuf2-v4l2.c @@ -659,8 +659,12 @@ int vb2_queue_init(struct vb2_queue *q) * queues will always initialize waiting_for_buffers to false. */ q->quirk_poll_must_check_waiting_for_buffers = true; - q->dma_dir = V4L2_TYPE_IS_OUTPUT(q->type) - ? DMA_TO_DEVICE : DMA_FROM_DEVICE; + + if (V4L2_TYPE_IS_OUTPUT(q->type)) + q->dma_dir = DMA_TO_DEVICE; + else + q->dma_dir = q->use_dma_bidirectional + ? DMA_BIDIRECTIONAL : DMA_FROM_DEVICE; return vb2_core_queue_init(q); } diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h index 38410dd..e613c74 100644 --- a/include/media/videobuf2-core.h +++ b/include/media/videobuf2-core.h @@ -433,6 +433,9 @@ struct vb2_buf_ops { * @quirk_poll_must_check_waiting_for_buffers: Return POLLERR at poll when QBUF * has not been called. This is a vb1 idiom that has been adopted * also by vb2. + * @use_dma_bidirectional: use DMA_BIDIRECTIONAL for CAPTURE buffers; this + * allows HW to read from the CAPTURE buffers in + * addition to writing; ignored for OUTPUT queues * @lock: pointer to a mutex that protects the vb2_queue struct. The * driver can set this to a mutex to let the v4l2 core serialize * the queuing ioctls. If the driver wants to handle locking @@ -500,6 +503,7 @@ struct vb2_queue { unsigned fileio_write_immediately:1; unsigned allow_zero_bytesused:1; unsigned quirk_poll_must_check_waiting_for_buffers:1; + unsigned use_dma_bidirectional:1; struct mutex *lock; void *owner;