| Message ID | 20201129060517.2029659-1-frkoenig@chromium.org |
|---|---|
| State | New |
| Headers | show |
| Series | venus: venc: Add VIDIOC_TRY_ENCODER_CMD support | expand |
On Sun, Nov 29, 2020 at 3:05 PM Fritz Koenig <frkoenig@chromium.org> wrote: > > V4L2_ENC_CMD_STOP and V4L2_ENC_CMD_START are already > supported. Add a way to query for support. I think your Signed-off-by is missing (checkpatch.pl should warn you about such problems). > > --- > drivers/media/platform/qcom/venus/venc.c | 26 ++++++++++++++++++++++++ > 1 file changed, 26 insertions(+) > > diff --git a/drivers/media/platform/qcom/venus/venc.c b/drivers/media/platform/qcom/venus/venc.c > index 2ddfeddf98514..e05db3c4bfb24 100644 > --- a/drivers/media/platform/qcom/venus/venc.c > +++ b/drivers/media/platform/qcom/venus/venc.c > @@ -507,6 +507,27 @@ static int venc_enum_frameintervals(struct file *file, void *fh, > return 0; > } > > +static int > +venc_try_encoder_cmd(struct file *file, void *fh, struct v4l2_encoder_cmd *cmd) > +{ > + struct venus_inst *inst = to_inst(file); > + struct device *dev = inst->core->dev_dec; > + > + switch (cmd->cmd) { > + case V4L2_ENC_CMD_STOP: > + case V4L2_ENC_CMD_START: > + if (cmd->flags != 0) { > + dev_dbg(dev, "flags=%u are not supported", cmd->flags); > + return -EINVAL; > + } > + break; > + default: > + return -EINVAL; > + } > + > + return 0; > +} > + > static int > venc_encoder_cmd(struct file *file, void *fh, struct v4l2_encoder_cmd *cmd) I am not seeing venc_encoder_cmd() in the media tree, does this patch depend on others that are not yet merged? If so they should be submitted together as a series. > { > @@ -514,6 +535,10 @@ venc_encoder_cmd(struct file *file, void *fh, struct v4l2_encoder_cmd *cmd) > struct hfi_frame_data fdata = {0}; > int ret = 0; > > + ret = venc_try_encoder_cmd(file, fh, cmd); > + if (ret < 0) > + return ret; > + v4l2_m2m_ioctl_try_encoder_cmd() is called right below, and AFAICT does the same thing as the newly-defined venc_try_encoder_cmd(). So IIUC this patch can be turned into a one-liner that does just the following: @@ -575,6 +600,7 @@ static const struct v4l2_ioctl_ops venc_ioctl_ops = { .vidioc_subscribe_event = v4l2_ctrl_subscribe_event, .vidioc_unsubscribe_event = v4l2_event_unsubscribe, .vidioc_encoder_cmd = venc_encoder_cmd, + .vidioc_try_encoder_cmd = v4l2_m2m_ioctl_try_encoder_cmd, }; > ret = v4l2_m2m_ioctl_try_encoder_cmd(file, fh, cmd); > if (ret) > return ret; > @@ -575,6 +600,7 @@ static const struct v4l2_ioctl_ops venc_ioctl_ops = { > .vidioc_subscribe_event = v4l2_ctrl_subscribe_event, > .vidioc_unsubscribe_event = v4l2_event_unsubscribe, > .vidioc_encoder_cmd = venc_encoder_cmd, > + .vidioc_try_encoder_cmd = venc_try_encoder_cmd, > }; > > static int venc_set_properties(struct venus_inst *inst) > -- > 2.29.2.454.gaff20da3a2-goog >
On Mon, Nov 30, 2020 at 7:24 PM Alexandre Courbot <acourbot@chromium.org> wrote: > > On Sun, Nov 29, 2020 at 3:05 PM Fritz Koenig <frkoenig@chromium.org> wrote: > > > > V4L2_ENC_CMD_STOP and V4L2_ENC_CMD_START are already > > supported. Add a way to query for support. > > I think your Signed-off-by is missing (checkpatch.pl should warn you > about such problems). > > > > > --- > > drivers/media/platform/qcom/venus/venc.c | 26 ++++++++++++++++++++++++ > > 1 file changed, 26 insertions(+) > > > > diff --git a/drivers/media/platform/qcom/venus/venc.c b/drivers/media/platform/qcom/venus/venc.c > > index 2ddfeddf98514..e05db3c4bfb24 100644 > > --- a/drivers/media/platform/qcom/venus/venc.c > > +++ b/drivers/media/platform/qcom/venus/venc.c > > @@ -507,6 +507,27 @@ static int venc_enum_frameintervals(struct file *file, void *fh, > > return 0; > > } > > > > +static int > > +venc_try_encoder_cmd(struct file *file, void *fh, struct v4l2_encoder_cmd *cmd) > > +{ > > + struct venus_inst *inst = to_inst(file); > > + struct device *dev = inst->core->dev_dec; > > + > > + switch (cmd->cmd) { > > + case V4L2_ENC_CMD_STOP: > > + case V4L2_ENC_CMD_START: > > + if (cmd->flags != 0) { > > + dev_dbg(dev, "flags=%u are not supported", cmd->flags); > > + return -EINVAL; > > + } > > + break; > > + default: > > + return -EINVAL; > > + } > > + > > + return 0; > > +} > > + > > static int > > venc_encoder_cmd(struct file *file, void *fh, struct v4l2_encoder_cmd *cmd) > > I am not seeing venc_encoder_cmd() in the media tree, does this patch > depend on others that are not yet merged? If so they should be > submitted together as a series. > Sorry, I'm still a little unsure of procedures here. There is another patch set[1] posted and I thought it was missing this part. It turns out I had not applied the whole set to my tree. > > { > > @@ -514,6 +535,10 @@ venc_encoder_cmd(struct file *file, void *fh, struct v4l2_encoder_cmd *cmd) > > struct hfi_frame_data fdata = {0}; > > int ret = 0; > > > > + ret = venc_try_encoder_cmd(file, fh, cmd); > > + if (ret < 0) > > + return ret; > > + > > v4l2_m2m_ioctl_try_encoder_cmd() is called right below, and AFAICT > does the same thing as the newly-defined venc_try_encoder_cmd(). So > IIUC this patch can be turned into a one-liner that does just the > following: > > @@ -575,6 +600,7 @@ static const struct v4l2_ioctl_ops venc_ioctl_ops = { > .vidioc_subscribe_event = v4l2_ctrl_subscribe_event, > .vidioc_unsubscribe_event = v4l2_event_unsubscribe, > .vidioc_encoder_cmd = venc_encoder_cmd, > + .vidioc_try_encoder_cmd = v4l2_m2m_ioctl_try_encoder_cmd, > }; > Yes, that's how it is in the current patch[2], which is why I may have missed it. (I'm embarrassed because I reviewed that patch and then posted mine.) > > ret = v4l2_m2m_ioctl_try_encoder_cmd(file, fh, cmd); > > if (ret) > > return ret; > > @@ -575,6 +600,7 @@ static const struct v4l2_ioctl_ops venc_ioctl_ops = { > > .vidioc_subscribe_event = v4l2_ctrl_subscribe_event, > > .vidioc_unsubscribe_event = v4l2_event_unsubscribe, > > .vidioc_encoder_cmd = venc_encoder_cmd, > > + .vidioc_try_encoder_cmd = venc_try_encoder_cmd, > > }; > > > > static int venc_set_properties(struct venus_inst *inst) > > -- > > 2.29.2.454.gaff20da3a2-goog > > [1] : https://patchwork.kernel.org/project/linux-media/list/?series=382113 [2]: https://patchwork.kernel.org/project/linux-media/patch/20201111143755.24541-7-stanimir.varbanov@linaro.org/
On Tue, Dec 1, 2020 at 1:07 PM Fritz Koenig <frkoenig@chromium.org> wrote: > > On Mon, Nov 30, 2020 at 7:24 PM Alexandre Courbot <acourbot@chromium.org> wrote: > > > > On Sun, Nov 29, 2020 at 3:05 PM Fritz Koenig <frkoenig@chromium.org> wrote: > > > > > > V4L2_ENC_CMD_STOP and V4L2_ENC_CMD_START are already > > > supported. Add a way to query for support. > > > > I think your Signed-off-by is missing (checkpatch.pl should warn you > > about such problems). > > > > > > > > --- > > > drivers/media/platform/qcom/venus/venc.c | 26 ++++++++++++++++++++++++ > > > 1 file changed, 26 insertions(+) > > > > > > diff --git a/drivers/media/platform/qcom/venus/venc.c b/drivers/media/platform/qcom/venus/venc.c > > > index 2ddfeddf98514..e05db3c4bfb24 100644 > > > --- a/drivers/media/platform/qcom/venus/venc.c > > > +++ b/drivers/media/platform/qcom/venus/venc.c > > > @@ -507,6 +507,27 @@ static int venc_enum_frameintervals(struct file *file, void *fh, > > > return 0; > > > } > > > > > > +static int > > > +venc_try_encoder_cmd(struct file *file, void *fh, struct v4l2_encoder_cmd *cmd) > > > +{ > > > + struct venus_inst *inst = to_inst(file); > > > + struct device *dev = inst->core->dev_dec; > > > + > > > + switch (cmd->cmd) { > > > + case V4L2_ENC_CMD_STOP: > > > + case V4L2_ENC_CMD_START: > > > + if (cmd->flags != 0) { > > > + dev_dbg(dev, "flags=%u are not supported", cmd->flags); > > > + return -EINVAL; > > > + } > > > + break; > > > + default: > > > + return -EINVAL; > > > + } > > > + > > > + return 0; > > > +} > > > + > > > static int > > > venc_encoder_cmd(struct file *file, void *fh, struct v4l2_encoder_cmd *cmd) > > > > I am not seeing venc_encoder_cmd() in the media tree, does this patch > > depend on others that are not yet merged? If so they should be > > submitted together as a series. > > > > Sorry, I'm still a little unsure of procedures here. There is another > patch set[1] posted > and I thought it was missing this part. It turns out I had not > applied the whole set to > my tree. > > > > { > > > @@ -514,6 +535,10 @@ venc_encoder_cmd(struct file *file, void *fh, struct v4l2_encoder_cmd *cmd) > > > struct hfi_frame_data fdata = {0}; > > > int ret = 0; > > > > > > + ret = venc_try_encoder_cmd(file, fh, cmd); > > > + if (ret < 0) > > > + return ret; > > > + > > > > v4l2_m2m_ioctl_try_encoder_cmd() is called right below, and AFAICT > > does the same thing as the newly-defined venc_try_encoder_cmd(). So > > IIUC this patch can be turned into a one-liner that does just the > > following: > > > > @@ -575,6 +600,7 @@ static const struct v4l2_ioctl_ops venc_ioctl_ops = { > > .vidioc_subscribe_event = v4l2_ctrl_subscribe_event, > > .vidioc_unsubscribe_event = v4l2_event_unsubscribe, > > .vidioc_encoder_cmd = venc_encoder_cmd, > > + .vidioc_try_encoder_cmd = v4l2_m2m_ioctl_try_encoder_cmd, > > }; > > > Yes, that's how it is in the current patch[2], which is why I may have > missed it. > (I'm embarrassed because I reviewed that patch and then posted mine.) Ah, ack - no worries, I am also far from being on top of everything and missed the connection with that patchset. :) I guess we can drop this one then. > > > > ret = v4l2_m2m_ioctl_try_encoder_cmd(file, fh, cmd); > > > if (ret) > > > return ret; > > > @@ -575,6 +600,7 @@ static const struct v4l2_ioctl_ops venc_ioctl_ops = { > > > .vidioc_subscribe_event = v4l2_ctrl_subscribe_event, > > > .vidioc_unsubscribe_event = v4l2_event_unsubscribe, > > > .vidioc_encoder_cmd = venc_encoder_cmd, > > > + .vidioc_try_encoder_cmd = venc_try_encoder_cmd, > > > }; > > > > > > static int venc_set_properties(struct venus_inst *inst) > > > -- > > > 2.29.2.454.gaff20da3a2-goog > > > > > [1] : https://patchwork.kernel.org/project/linux-media/list/?series=382113 > [2]: https://patchwork.kernel.org/project/linux-media/patch/20201111143755.24541-7-stanimir.varbanov@linaro.org/
diff --git a/drivers/media/platform/qcom/venus/venc.c b/drivers/media/platform/qcom/venus/venc.c index 2ddfeddf98514..e05db3c4bfb24 100644 --- a/drivers/media/platform/qcom/venus/venc.c +++ b/drivers/media/platform/qcom/venus/venc.c @@ -507,6 +507,27 @@ static int venc_enum_frameintervals(struct file *file, void *fh, return 0; } +static int +venc_try_encoder_cmd(struct file *file, void *fh, struct v4l2_encoder_cmd *cmd) +{ + struct venus_inst *inst = to_inst(file); + struct device *dev = inst->core->dev_dec; + + switch (cmd->cmd) { + case V4L2_ENC_CMD_STOP: + case V4L2_ENC_CMD_START: + if (cmd->flags != 0) { + dev_dbg(dev, "flags=%u are not supported", cmd->flags); + return -EINVAL; + } + break; + default: + return -EINVAL; + } + + return 0; +} + static int venc_encoder_cmd(struct file *file, void *fh, struct v4l2_encoder_cmd *cmd) { @@ -514,6 +535,10 @@ venc_encoder_cmd(struct file *file, void *fh, struct v4l2_encoder_cmd *cmd) struct hfi_frame_data fdata = {0}; int ret = 0; + ret = venc_try_encoder_cmd(file, fh, cmd); + if (ret < 0) + return ret; + ret = v4l2_m2m_ioctl_try_encoder_cmd(file, fh, cmd); if (ret) return ret; @@ -575,6 +600,7 @@ static const struct v4l2_ioctl_ops venc_ioctl_ops = { .vidioc_subscribe_event = v4l2_ctrl_subscribe_event, .vidioc_unsubscribe_event = v4l2_event_unsubscribe, .vidioc_encoder_cmd = venc_encoder_cmd, + .vidioc_try_encoder_cmd = venc_try_encoder_cmd, }; static int venc_set_properties(struct venus_inst *inst)