Re: [PATCH] media: venus: add support for key frame

From: mgottam
Date: Wed Oct 24 2018 - 09:37:54 EST


On 2018-10-23 08:37, Tomasz Figa wrote:
On Mon, Oct 22, 2018 at 3:15 PM Alexandre Courbot <acourbot@xxxxxxxxxxxx> wrote:

On Fri, Oct 12, 2018 at 5:10 PM Stanimir Varbanov
<stanimir.varbanov@xxxxxxxxxx> wrote:
>
>
>
> On 10/12/2018 11:06 AM, Alexandre Courbot wrote:
> > On Fri, Oct 12, 2018 at 4:37 PM Stanimir Varbanov
> > <stanimir.varbanov@xxxxxxxxxx> wrote:
> >>
> >> Hi Alex,
> >>
> >> On 10/12/2018 08:26 AM, Alexandre Courbot wrote:
> >>> On Tue, Oct 9, 2018 at 4:54 PM Malathi Gottam <mgottam@xxxxxxxxxxxxxx> wrote:
> >>>>
> >>>> When client requests for a keyframe, set the property
> >>>> to hardware to generate the sync frame.
> >>>>
> >>>> Signed-off-by: Malathi Gottam <mgottam@xxxxxxxxxxxxxx>
> >>>> ---
> >>>> drivers/media/platform/qcom/venus/venc_ctrls.c | 13 +++++++++++++
> >>>> 1 file changed, 13 insertions(+)
> >>>>
> >>>> diff --git a/drivers/media/platform/qcom/venus/venc_ctrls.c b/drivers/media/platform/qcom/venus/venc_ctrls.c
> >>>> index 45910172..f332c8e 100644
> >>>> --- a/drivers/media/platform/qcom/venus/venc_ctrls.c
> >>>> +++ b/drivers/media/platform/qcom/venus/venc_ctrls.c
> >>>> @@ -81,6 +81,8 @@ static int venc_op_s_ctrl(struct v4l2_ctrl *ctrl)
> >>>> struct venc_controls *ctr = &inst->controls.enc;
> >>>> u32 bframes;
> >>>> int ret;
> >>>> + void *ptr;
> >>>> + u32 ptype;
> >>>>
> >>>> switch (ctrl->id) {
> >>>> case V4L2_CID_MPEG_VIDEO_BITRATE_MODE:
> >>>> @@ -173,6 +175,14 @@ static int venc_op_s_ctrl(struct v4l2_ctrl *ctrl)
> >>>>
> >>>> ctr->num_b_frames = bframes;
> >>>> break;
> >>>> + case V4L2_CID_MPEG_VIDEO_FORCE_KEY_FRAME:
> >>>> + ptype = HFI_PROPERTY_CONFIG_VENC_REQUEST_SYNC_FRAME;
> >>>> + ret = hfi_session_set_property(inst, ptype, ptr);
> >>>
> >>> The test bot already said it, but ptr is passed to
> >>> hfi_session_set_property() uninitialized. And as can be expected the
> >>> call returns -EINVAL on my board.
> >>>
> >>> Looking at other uses of HFI_PROPERTY_CONFIG_VENC_REQUEST_SYNC_FRAME I
> >>> see that the packet sent to the firmware does not have room for an
> >>> argument, so I tried to pass NULL but got the same result.
> >>
> >> yes, because pdata cannot be NULL. I'd suggest to make a pointer to
> >> struct hfi_enable and pass it to the set_property function.
> >
> > FWIW I also tried doing this and got the same error, strange...
> >
>
> OK, when you calling the v4l control? It makes sense when you calling
> it, because set_property checks does the session is on START state (i.e.
> streamon on both queues).

Do you mean that the property won't be actually applied unless both
queues are streaming? In that case maybe it would make sense for the
driver to save controls set before that and apply them when the
conditions allow them to be effective?

Right. The driver cannot just drop a control setting on the floor if
it's not ready to apply it.

However, the V4L2 control framework already provides a tool to handle this:
- the driver can ignore any .s_ctrl() calls when it can't apply the controls,
- the driver must call v4l2_ctrl_handler_setup() when it initialized
the hardware, so that all the control values are applied in one go.

Best regards,
Tomasz
Yes V4L2 control framework validates the ctrls before being set.

But these controls are initialized before session initialization. So s_ctrl tries to set property "HFI_PROPERTY_CONFIG_VENC_REQUEST_SYNC_FRAME" as a part of initializing all controls(session still in UNINIT state). But this is not any client request.

So we can keep a check to
1. ensure that its client requesting sync frame and
2. session in START state (both planes are streaming)

I will update the patch with these changes.