Re: [PATCH v2 2/3] [media] hva: multi-format video encoder V4L2 driver

[Jean Christophe TROTIN <jean-christophe.trotin@st.com>]

Hi Hans,

Thank you for your comments.
I've started to take them into account.
I've got a question about V4L2_FIELD_ANY in buf_prepare (please see below).

[snip]

 >> +static int hva_buf_prepare(struct vb2_buffer *vb)
 >> +{
 >> +     struct hva_ctx *ctx = vb2_get_drv_priv(vb->vb2_queue);
 >> +     struct device *dev = ctx_to_dev(ctx);
 >> +     struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
 >> +
 >> +     if (vb->vb2_queue->type == V4L2_BUF_TYPE_VIDEO_OUTPUT) {
 >> +             struct hva_frame *frame = to_hva_frame(vbuf);
 >> +
 >> +             if (vbuf->field == V4L2_FIELD_ANY)
 >> +                     vbuf->field = V4L2_FIELD_NONE;
 >
 > Anything other than FIELD_NONE should result in an error since no interlaced 
is supported.
 > FIELD_ANY is an incorrect value as well.
 >

In videodev2.h, V4L2_FIELD_ANY is commented as "driver can choose from none, 
top, bottom, interlaced depending on whatever it thinks is approximate ...": I 
understand this comment as if vbuf->field is equal to V4L2_FIELD_ANY, then the 
driver can choose to force it to V4L2_FIELD_NONE. Furthermore, it's coded in the 
same way in vim2m.c (vim2m_buf_prepare).
Finally, if I remove these 2 lines, I've got the following error with the 
v4l2-compliance:
Streaming ioctls:
		VIDIOC_G_INPUT returned -1 (Inappropriate ioctl for device)
		VIDIOC_ENUMINPUT returned -1 (Inappropriate ioctl for device)
	test read/write: OK (Not Supported)
		VIDIOC_QUERYCAP returned 0 (Success)
		[snip]
		VIDIOC_QUERYBUF returned 0 (Success)
		VIDIOC_QBUF returned -1 (Invalid argument)
		fail: 
/local/home/frq08988/views/opensdk-2.1.4.1/sources/v4l-utils/utils/v4l2-compliance/v4l2-test-buffers.cpp(773): 
buf.qbuf(node)
		fail: 
/local/home/frq08988/views/opensdk-2.1.4.1/sources/v4l-utils/utils/v4l2-compliance/v4l2-test-buffers.cpp(971): 
setupM2M(node, m2m_q)
	test MMAP: FAIL

Don't you think that I could keep these two lines?

 >> +             if (vbuf->field != V4L2_FIELD_NONE) {
 >> +                     dev_dbg(dev,
 >> +                             "%s frame[%d] prepare: %d field not supported\n",
 >> +                             ctx->name, vb->index, vbuf->field);
 >> +                     return -EINVAL;
 >> +             }
 >> +
 >> +             if (!frame->prepared) {
 >> +                     /* get memory addresses */
 >> +                     frame->vaddr = vb2_plane_vaddr(&vbuf->vb2_buf, 0);
 >> +                     frame->paddr = vb2_dma_contig_plane_dma_addr(
 >> +                                     &vbuf->vb2_buf, 0);
 >> +                     frame->info = ctx->frameinfo;
 >> +                     frame->prepared = true;
 >> +
 >> +                     dev_dbg(dev,
 >> +                             "%s frame[%d] prepared; virt=%p, phy=%pad\n",
 >> +                             ctx->name, vb->index,
 >> +                             frame->vaddr, &frame->paddr);
 >> +             }
 >> +     } else {
 >> +             struct hva_stream *stream = to_hva_stream(vbuf);
 >> +
 >> +             if (!stream->prepared) {
 >> +                     /* get memory addresses */
 >> +                     stream->vaddr = vb2_plane_vaddr(&vbuf->vb2_buf, 0);
 >> +                     stream->paddr = vb2_dma_contig_plane_dma_addr(
 >> +                                     &vbuf->vb2_buf, 0);
 >> +                     stream->size = vb2_plane_size(&vbuf->vb2_buf, 0);
 >> +                     stream->prepared = true;
 >> +
 >> +                     dev_dbg(dev,
 >> +                             "%s stream[%d] prepared; virt=%p, phy=%pad\n",
 >> +                             ctx->name, vb->index,
 >> +                             stream->vaddr, &stream->paddr);
 >> +             }
 >> +     }
 >> +
 >> +     return 0;
 >> +}
 >> +

[snip]

Regards,
Jean-Christophe.