Frame rate control is always enabled in this driver, so make it silently
support the V4L2_CID_MPEG_VIDEO_FRAME_RC_ENABLE.
Signed-off-by: Jeffrey Kardatzke <[email protected]>
---
drivers/media/platform/qcom/venus/venc_ctrls.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/media/platform/qcom/venus/venc_ctrls.c b/drivers/media/platform/qcom/venus/venc_ctrls.c
index 877c0b3299e9..9ede692f77c5 100644
--- a/drivers/media/platform/qcom/venus/venc_ctrls.c
+++ b/drivers/media/platform/qcom/venus/venc_ctrls.c
@@ -199,6 +199,9 @@ static int venc_op_s_ctrl(struct v4l2_ctrl *ctrl)
}
mutex_unlock(&inst->lock);
break;
+ case V4L2_CID_MPEG_VIDEO_FRAME_RC_ENABLE:
+ // Silently ignore, it's always enabled.
+ break;
default:
return -EINVAL;
}
@@ -351,6 +354,9 @@ int venc_ctrl_init(struct venus_inst *inst)
v4l2_ctrl_new_std(&inst->ctrl_handler, &venc_ctrl_ops,
V4L2_CID_MPEG_VIDEO_FORCE_KEY_FRAME, 0, 0, 0, 0);
+ v4l2_ctrl_new_std(&inst->ctrl_handler, &venc_ctrl_ops,
+ V4L2_CID_MPEG_VIDEO_FRAME_RC_ENABLE, 0, 1, 1, 1);
+
ret = inst->ctrl_handler.error;
if (ret)
goto err;
--
2.25.0.225.g125e21ebc7-goog
Hi Jeff,
Thanks for the patch!
On 2/13/20 11:30 PM, Jeffrey Kardatzke wrote:
> Frame rate control is always enabled in this driver, so make it silently
> support the V4L2_CID_MPEG_VIDEO_FRAME_RC_ENABLE.
>
> Signed-off-by: Jeffrey Kardatzke <[email protected]>
> ---
> drivers/media/platform/qcom/venus/venc_ctrls.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/drivers/media/platform/qcom/venus/venc_ctrls.c b/drivers/media/platform/qcom/venus/venc_ctrls.c
> index 877c0b3299e9..9ede692f77c5 100644
> --- a/drivers/media/platform/qcom/venus/venc_ctrls.c
> +++ b/drivers/media/platform/qcom/venus/venc_ctrls.c
> @@ -199,6 +199,9 @@ static int venc_op_s_ctrl(struct v4l2_ctrl *ctrl)
> }
> mutex_unlock(&inst->lock);
> break;
> + case V4L2_CID_MPEG_VIDEO_FRAME_RC_ENABLE:
> + // Silently ignore, it's always enabled.
Please, use C comments and follow the kernel coding style.
I wonder shouldn't it be better to add rc_enable field in struct
venc_controls and give the user choice to disable the rate control? We
can keep the default to be "enabled".
> + break;
> default:
> return -EINVAL;
> }
> @@ -351,6 +354,9 @@ int venc_ctrl_init(struct venus_inst *inst)
> v4l2_ctrl_new_std(&inst->ctrl_handler, &venc_ctrl_ops,
> V4L2_CID_MPEG_VIDEO_FORCE_KEY_FRAME, 0, 0, 0, 0);
>
> + v4l2_ctrl_new_std(&inst->ctrl_handler, &venc_ctrl_ops,
> + V4L2_CID_MPEG_VIDEO_FRAME_RC_ENABLE, 0, 1, 1, 1);
> +
you forgot to increment the number of controls in the call to
v4l2_ctrl_handler_init.
> ret = inst->ctrl_handler.error;
> if (ret)
> goto err;
>
--
regards,
Stan
Sorry for the duplicate, accidentally used HTML format and it got
bounced from the mailing lists so resending.
On Mon, Feb 17, 2020 at 2:15 AM Stanimir Varbanov
<[email protected]> wrote:
>
> Hi Jeff,
>
> Thanks for the patch!
>
> On 2/13/20 11:30 PM, Jeffrey Kardatzke wrote:
> > Frame rate control is always enabled in this driver, so make it silently
> > support the V4L2_CID_MPEG_VIDEO_FRAME_RC_ENABLE.
> >
> > Signed-off-by: Jeffrey Kardatzke <[email protected]>
> > ---
> > drivers/media/platform/qcom/venus/venc_ctrls.c | 6 ++++++
> > 1 file changed, 6 insertions(+)
> >
> > diff --git a/drivers/media/platform/qcom/venus/venc_ctrls.c b/drivers/media/platform/qcom/venus/venc_ctrls.c
> > index 877c0b3299e9..9ede692f77c5 100644
> > --- a/drivers/media/platform/qcom/venus/venc_ctrls.c
> > +++ b/drivers/media/platform/qcom/venus/venc_ctrls.c
> > @@ -199,6 +199,9 @@ static int venc_op_s_ctrl(struct v4l2_ctrl *ctrl)
> > }
> > mutex_unlock(&inst->lock);
> > break;
> > + case V4L2_CID_MPEG_VIDEO_FRAME_RC_ENABLE:
> > + // Silently ignore, it's always enabled.
>
> Please, use C comments and follow the kernel coding style.
OK, hopefully I've got that now. I didn't see any issues aside from
the comment style though.
I'll upload a new patch shortly.
>
>
> I wonder shouldn't it be better to add rc_enable field in struct
> venc_controls and give the user choice to disable the rate control? We
> can keep the default to be "enabled".
>
That'd be fine. Is there a way to actually disable the rate control though?
>
> > + break;
> > default:
> > return -EINVAL;
> > }
> > @@ -351,6 +354,9 @@ int venc_ctrl_init(struct venus_inst *inst)
> > v4l2_ctrl_new_std(&inst->ctrl_handler, &venc_ctrl_ops,
> > V4L2_CID_MPEG_VIDEO_FORCE_KEY_FRAME, 0, 0, 0, 0);
> >
> > + v4l2_ctrl_new_std(&inst->ctrl_handler, &venc_ctrl_ops,
> > + V4L2_CID_MPEG_VIDEO_FRAME_RC_ENABLE, 0, 1, 1, 1);
> > +
>
> you forgot to increment the number of controls in the call to
> v4l2_ctrl_handler_init.
>
Done, thanks.
>
> > ret = inst->ctrl_handler.error;
> > if (ret)
> > goto err;
> >
>
> --
> regards,
> Stan
Frame rate control is always enabled in this driver, so make it silently
support the V4L2_CID_MPEG_VIDEO_FRAME_RC_ENABLE.
Signed-off-by: Jeffrey Kardatzke <[email protected]>
---
drivers/media/platform/qcom/venus/venc_ctrls.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/media/platform/qcom/venus/venc_ctrls.c b/drivers/media/platform/qcom/venus/venc_ctrls.c
index 877c0b3299e9..15852bc26231 100644
--- a/drivers/media/platform/qcom/venus/venc_ctrls.c
+++ b/drivers/media/platform/qcom/venus/venc_ctrls.c
@@ -199,6 +199,9 @@ static int venc_op_s_ctrl(struct v4l2_ctrl *ctrl)
}
mutex_unlock(&inst->lock);
break;
+ case V4L2_CID_MPEG_VIDEO_FRAME_RC_ENABLE:
+ /* Silently ignore, it's always enabled. */
+ break;
default:
return -EINVAL;
}
@@ -214,7 +217,7 @@ int venc_ctrl_init(struct venus_inst *inst)
{
int ret;
- ret = v4l2_ctrl_handler_init(&inst->ctrl_handler, 30);
+ ret = v4l2_ctrl_handler_init(&inst->ctrl_handler, 31);
if (ret)
return ret;
@@ -351,6 +354,9 @@ int venc_ctrl_init(struct venus_inst *inst)
v4l2_ctrl_new_std(&inst->ctrl_handler, &venc_ctrl_ops,
V4L2_CID_MPEG_VIDEO_FORCE_KEY_FRAME, 0, 0, 0, 0);
+ v4l2_ctrl_new_std(&inst->ctrl_handler, &venc_ctrl_ops,
+ V4L2_CID_MPEG_VIDEO_FRAME_RC_ENABLE, 0, 1, 1, 1);
+
ret = inst->ctrl_handler.error;
if (ret)
goto err;
--
2.25.0.265.gbab2e86ba0-goog
Hi Jeff,
On 2/18/20 9:09 PM, Jeffrey Kardatzke wrote:
> Sorry for the duplicate, accidentally used HTML format and it got
> bounced from the mailing lists so resending.
>
> On Mon, Feb 17, 2020 at 2:15 AM Stanimir Varbanov
> <[email protected]> wrote:
>>
>> Hi Jeff,
>>
>> Thanks for the patch!
>>
>> On 2/13/20 11:30 PM, Jeffrey Kardatzke wrote:
>>> Frame rate control is always enabled in this driver, so make it silently
>>> support the V4L2_CID_MPEG_VIDEO_FRAME_RC_ENABLE.
>>>
>>> Signed-off-by: Jeffrey Kardatzke <[email protected]>
>>> ---
>>> drivers/media/platform/qcom/venus/venc_ctrls.c | 6 ++++++
>>> 1 file changed, 6 insertions(+)
>>>
>>> diff --git a/drivers/media/platform/qcom/venus/venc_ctrls.c b/drivers/media/platform/qcom/venus/venc_ctrls.c
>>> index 877c0b3299e9..9ede692f77c5 100644
>>> --- a/drivers/media/platform/qcom/venus/venc_ctrls.c
>>> +++ b/drivers/media/platform/qcom/venus/venc_ctrls.c
>>> @@ -199,6 +199,9 @@ static int venc_op_s_ctrl(struct v4l2_ctrl *ctrl)
>>> }
>>> mutex_unlock(&inst->lock);
>>> break;
>>> + case V4L2_CID_MPEG_VIDEO_FRAME_RC_ENABLE:
>>> + // Silently ignore, it's always enabled.
>>
>> Please, use C comments and follow the kernel coding style.
>
> OK, hopefully I've got that now. I didn't see any issues aside from
> the comment style though.
> I'll upload a new patch shortly.
>>
>>
>> I wonder shouldn't it be better to add rc_enable field in struct
>> venc_controls and give the user choice to disable the rate control? We
>> can keep the default to be "enabled".
>>
> That'd be fine. Is there a way to actually disable the rate control though?
The rate control property values are here [1], and [2] is where we set
the control.
--
regards,
Stan
[1]
https://elixir.bootlin.com/linux/v5.6-rc2/source/drivers/media/platform/qcom/venus/hfi_helper.h#L229
[2]
https://elixir.bootlin.com/linux/v5.6-rc2/source/drivers/media/platform/qcom/venus/venc.c#L734
Got it, thanks. I'll submit a new patch with that.
On Tue, Feb 18, 2020 at 11:56 PM Stanimir Varbanov
<[email protected]> wrote:
>
> Hi Jeff,
>
> On 2/18/20 9:09 PM, Jeffrey Kardatzke wrote:
> > Sorry for the duplicate, accidentally used HTML format and it got
> > bounced from the mailing lists so resending.
> >
> > On Mon, Feb 17, 2020 at 2:15 AM Stanimir Varbanov
> > <[email protected]> wrote:
> >>
> >> Hi Jeff,
> >>
> >> Thanks for the patch!
> >>
> >> On 2/13/20 11:30 PM, Jeffrey Kardatzke wrote:
> >>> Frame rate control is always enabled in this driver, so make it silently
> >>> support the V4L2_CID_MPEG_VIDEO_FRAME_RC_ENABLE.
> >>>
> >>> Signed-off-by: Jeffrey Kardatzke <[email protected]>
> >>> ---
> >>> drivers/media/platform/qcom/venus/venc_ctrls.c | 6 ++++++
> >>> 1 file changed, 6 insertions(+)
> >>>
> >>> diff --git a/drivers/media/platform/qcom/venus/venc_ctrls.c b/drivers/media/platform/qcom/venus/venc_ctrls.c
> >>> index 877c0b3299e9..9ede692f77c5 100644
> >>> --- a/drivers/media/platform/qcom/venus/venc_ctrls.c
> >>> +++ b/drivers/media/platform/qcom/venus/venc_ctrls.c
> >>> @@ -199,6 +199,9 @@ static int venc_op_s_ctrl(struct v4l2_ctrl *ctrl)
> >>> }
> >>> mutex_unlock(&inst->lock);
> >>> break;
> >>> + case V4L2_CID_MPEG_VIDEO_FRAME_RC_ENABLE:
> >>> + // Silently ignore, it's always enabled.
> >>
> >> Please, use C comments and follow the kernel coding style.
> >
> > OK, hopefully I've got that now. I didn't see any issues aside from
> > the comment style though.
> > I'll upload a new patch shortly.
> >>
> >>
> >> I wonder shouldn't it be better to add rc_enable field in struct
> >> venc_controls and give the user choice to disable the rate control? We
> >> can keep the default to be "enabled".
> >>
> > That'd be fine. Is there a way to actually disable the rate control though?
>
> The rate control property values are here [1], and [2] is where we set
> the control.
>
> --
> regards,
> Stan
>
> [1]
> https://elixir.bootlin.com/linux/v5.6-rc2/source/drivers/media/platform/qcom/venus/hfi_helper.h#L229
> [2]
> https://elixir.bootlin.com/linux/v5.6-rc2/source/drivers/media/platform/qcom/venus/venc.c#L734
--
Jeffrey Kardatzke
[email protected]
Google, Inc.