Re: [PATCH] media: venus: support frame rate control

[Jeffrey Kardatzke]

Got it, thanks. I'll submit a new patch with that.

On Tue, Feb 18, 2020 at 11:56 PM Stanimir Varbanov
<stanimir.varbanov@xxxxxxxxxx> 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
> > <stanimir.varbanov@xxxxxxxxxx> 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 <jkardatzke@xxxxxxxxxx>
> >>> ---
> >>>  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
jkardatzke@xxxxxxxxxx
Google, Inc.