Message ID | 20200619054123.19052-6-paul.elder@ideasonboard.com |
---|---|
State | Superseded |
Headers | show |
Series |
|
Related | show |
Hi Paul, Thank you for the patch. On Fri, Jun 19, 2020 at 02:41:11PM +0900, Paul Elder wrote: > Implement VIDIOC_G_PRIORITY and VIDIOC_S_PRIORITY, according to what > v4l2-compliance wants. How about mentioning the kernel implementation instead of v4l2-compliance ? Something along the lines of Implement VIDIOC_G_PRIORITY and VIDIOC_S_PRIORITY. The behaviour documented in the V4L2 specification doesn't match the implementation in the Linux kernel, implement the latter. > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > --- > Changes in v2: > - use V4L2CameraFile instead of fd and priorities map > --- > src/v4l2/v4l2_camera_proxy.cpp | 42 +++++++++++++++++++++++++++++++++- > src/v4l2/v4l2_camera_proxy.h | 3 +++ > 2 files changed, 44 insertions(+), 1 deletion(-) > > diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp > index 117d7ff..3e95645 100644 > --- a/src/v4l2/v4l2_camera_proxy.cpp > +++ b/src/v4l2/v4l2_camera_proxy.cpp > @@ -37,7 +37,8 @@ V4L2CameraProxy::V4L2CameraProxy(unsigned int index, > std::shared_ptr<Camera> camera) > : refcount_(0), index_(index), bufferCount_(0), currentBuf_(0), > vcam_(std::make_unique<V4L2Camera>(camera)), efd_(-1), > - acquiredCf_(nullptr), initialized_(false) > + v4l2RecordPriorityCf_(nullptr), acquiredCf_(nullptr), > + initialized_(false) > { > querycap(camera); > } > @@ -344,6 +345,37 @@ int V4L2CameraProxy::vidioc_try_fmt(V4L2CameraFile *cf, struct v4l2_format *arg) > return 0; > } > > +int V4L2CameraProxy::vidioc_g_priority(V4L2CameraFile *cf, enum v4l2_priority *arg) > +{ > + LOG(V4L2Compat, Debug) << "Servicing vidioc_g_priority fd = " << cf->efd(); > + > + if (v4l2RecordPriorityCf_) > + *arg = V4L2_PRIORITY_RECORD; > + else > + *arg = cf->priority_; This will please v4l2-compliance, but won't correctly support V4L2_PRIORITY_INTERACTIVE and V4L2_PRIORITY_BACKGROUND. Is that why you mentioned v4l2-compliance and not the kernel in the commit message ? Do you think we should implement the full behaviour ? It shouldn't be too difficult, but would require retrieving the max priority. You could do so by storing a list of V4L2CameraFile in V4L2CameraProxy, or an array of integers containing with the number of V4L2CameraFile for a given priority. To really implement priority support you would need to check if the file has a high enough priority in all the ioctls that require doing so. For the ioctls we support, that would be VIDIOC_S_FMT VIDIOC_S_PRIORITY VIDIOC_S_INPUT VIDIOC_REQBUFS VIDIOC_STREAMON VIDIOC_STREAMOFF The check would simply verify that the file priority is >= max priority, implemented in a helper function. I think that would also simplify the implementation of vidioc_s_priority. > + > + return 0; > +} > + > +int V4L2CameraProxy::vidioc_s_priority(V4L2CameraFile *cf, enum v4l2_priority *arg) > +{ > + LOG(V4L2Compat, Debug) > + << "Servicing vidioc_s_priority fd = " << cf->efd() > + << " prio = " << (int)*arg; static_cast<int> Should you check that the value of *arg is a valid priority ? > + > + if (v4l2RecordPriorityCf_ && v4l2RecordPriorityCf_ != cf) > + return -EBUSY; > + > + if (*arg == V4L2_PRIORITY_RECORD) > + v4l2RecordPriorityCf_ = cf; > + else if (v4l2RecordPriorityCf_ == cf) > + v4l2RecordPriorityCf_ = nullptr; > + > + cf->priority_ = *arg; > + > + return 0; > +} > + > int V4L2CameraProxy::freeBuffers() > { > LOG(V4L2Compat, Debug) << "Freeing libcamera bufs"; > @@ -560,6 +592,8 @@ std::set<unsigned long> V4L2CameraProxy::supportedIoctls_ = { I forgot to mention in the review of the patch that introduced this, I wonder if std::unordered_set would be more efficient. > VIDIOC_G_FMT, > VIDIOC_S_FMT, > VIDIOC_TRY_FMT, > + VIDIOC_G_PRIORITY, > + VIDIOC_S_PRIORITY, > VIDIOC_REQBUFS, > VIDIOC_QUERYBUF, > VIDIOC_QBUF, > @@ -597,6 +631,12 @@ int V4L2CameraProxy::ioctl(V4L2CameraFile *cf, unsigned long request, void *arg) > case VIDIOC_TRY_FMT: > ret = vidioc_try_fmt(cf, static_cast<struct v4l2_format *>(arg)); > break; > + case VIDIOC_G_PRIORITY: > + ret = vidioc_g_priority(cf, static_cast<enum v4l2_priority *>(arg)); > + break; > + case VIDIOC_S_PRIORITY: > + ret = vidioc_s_priority(cf, static_cast<enum v4l2_priority *>(arg)); > + break; > case VIDIOC_REQBUFS: > ret = vidioc_reqbufs(cf, static_cast<struct v4l2_requestbuffers *>(arg)); > break; > diff --git a/src/v4l2/v4l2_camera_proxy.h b/src/v4l2/v4l2_camera_proxy.h > index dd7e793..3260eec 100644 > --- a/src/v4l2/v4l2_camera_proxy.h > +++ b/src/v4l2/v4l2_camera_proxy.h > @@ -51,6 +51,8 @@ private: > int vidioc_g_fmt(V4L2CameraFile *cf, struct v4l2_format *arg); > int vidioc_s_fmt(V4L2CameraFile *cf, struct v4l2_format *arg); > int vidioc_try_fmt(V4L2CameraFile *cf, struct v4l2_format *arg); > + int vidioc_g_priority(V4L2CameraFile *cf, enum v4l2_priority *arg); > + int vidioc_s_priority(V4L2CameraFile *cf, enum v4l2_priority *arg); > int vidioc_reqbufs(V4L2CameraFile *cf, struct v4l2_requestbuffers *arg); > int vidioc_querybuf(V4L2CameraFile *cf, struct v4l2_buffer *arg); > int vidioc_qbuf(V4L2CameraFile *cf, struct v4l2_buffer *arg); > @@ -87,6 +89,7 @@ private: > > int efd_; > > + V4L2CameraFile *v4l2RecordPriorityCf_; > /* > * This is an exclusive lock on the camera. When this is not taken, > * anybody can call any ioctl before reqbufs. reqbufs with count > 0
diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp index 117d7ff..3e95645 100644 --- a/src/v4l2/v4l2_camera_proxy.cpp +++ b/src/v4l2/v4l2_camera_proxy.cpp @@ -37,7 +37,8 @@ V4L2CameraProxy::V4L2CameraProxy(unsigned int index, std::shared_ptr<Camera> camera) : refcount_(0), index_(index), bufferCount_(0), currentBuf_(0), vcam_(std::make_unique<V4L2Camera>(camera)), efd_(-1), - acquiredCf_(nullptr), initialized_(false) + v4l2RecordPriorityCf_(nullptr), acquiredCf_(nullptr), + initialized_(false) { querycap(camera); } @@ -344,6 +345,37 @@ int V4L2CameraProxy::vidioc_try_fmt(V4L2CameraFile *cf, struct v4l2_format *arg) return 0; } +int V4L2CameraProxy::vidioc_g_priority(V4L2CameraFile *cf, enum v4l2_priority *arg) +{ + LOG(V4L2Compat, Debug) << "Servicing vidioc_g_priority fd = " << cf->efd(); + + if (v4l2RecordPriorityCf_) + *arg = V4L2_PRIORITY_RECORD; + else + *arg = cf->priority_; + + return 0; +} + +int V4L2CameraProxy::vidioc_s_priority(V4L2CameraFile *cf, enum v4l2_priority *arg) +{ + LOG(V4L2Compat, Debug) + << "Servicing vidioc_s_priority fd = " << cf->efd() + << " prio = " << (int)*arg; + + if (v4l2RecordPriorityCf_ && v4l2RecordPriorityCf_ != cf) + return -EBUSY; + + if (*arg == V4L2_PRIORITY_RECORD) + v4l2RecordPriorityCf_ = cf; + else if (v4l2RecordPriorityCf_ == cf) + v4l2RecordPriorityCf_ = nullptr; + + cf->priority_ = *arg; + + return 0; +} + int V4L2CameraProxy::freeBuffers() { LOG(V4L2Compat, Debug) << "Freeing libcamera bufs"; @@ -560,6 +592,8 @@ std::set<unsigned long> V4L2CameraProxy::supportedIoctls_ = { VIDIOC_G_FMT, VIDIOC_S_FMT, VIDIOC_TRY_FMT, + VIDIOC_G_PRIORITY, + VIDIOC_S_PRIORITY, VIDIOC_REQBUFS, VIDIOC_QUERYBUF, VIDIOC_QBUF, @@ -597,6 +631,12 @@ int V4L2CameraProxy::ioctl(V4L2CameraFile *cf, unsigned long request, void *arg) case VIDIOC_TRY_FMT: ret = vidioc_try_fmt(cf, static_cast<struct v4l2_format *>(arg)); break; + case VIDIOC_G_PRIORITY: + ret = vidioc_g_priority(cf, static_cast<enum v4l2_priority *>(arg)); + break; + case VIDIOC_S_PRIORITY: + ret = vidioc_s_priority(cf, static_cast<enum v4l2_priority *>(arg)); + break; case VIDIOC_REQBUFS: ret = vidioc_reqbufs(cf, static_cast<struct v4l2_requestbuffers *>(arg)); break; diff --git a/src/v4l2/v4l2_camera_proxy.h b/src/v4l2/v4l2_camera_proxy.h index dd7e793..3260eec 100644 --- a/src/v4l2/v4l2_camera_proxy.h +++ b/src/v4l2/v4l2_camera_proxy.h @@ -51,6 +51,8 @@ private: int vidioc_g_fmt(V4L2CameraFile *cf, struct v4l2_format *arg); int vidioc_s_fmt(V4L2CameraFile *cf, struct v4l2_format *arg); int vidioc_try_fmt(V4L2CameraFile *cf, struct v4l2_format *arg); + int vidioc_g_priority(V4L2CameraFile *cf, enum v4l2_priority *arg); + int vidioc_s_priority(V4L2CameraFile *cf, enum v4l2_priority *arg); int vidioc_reqbufs(V4L2CameraFile *cf, struct v4l2_requestbuffers *arg); int vidioc_querybuf(V4L2CameraFile *cf, struct v4l2_buffer *arg); int vidioc_qbuf(V4L2CameraFile *cf, struct v4l2_buffer *arg); @@ -87,6 +89,7 @@ private: int efd_; + V4L2CameraFile *v4l2RecordPriorityCf_; /* * This is an exclusive lock on the camera. When this is not taken, * anybody can call any ioctl before reqbufs. reqbufs with count > 0
Implement VIDIOC_G_PRIORITY and VIDIOC_S_PRIORITY, according to what v4l2-compliance wants. Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> --- Changes in v2: - use V4L2CameraFile instead of fd and priorities map --- src/v4l2/v4l2_camera_proxy.cpp | 42 +++++++++++++++++++++++++++++++++- src/v4l2/v4l2_camera_proxy.h | 3 +++ 2 files changed, 44 insertions(+), 1 deletion(-)