From mboxrd@z Thu Jan 1 00:00:00 1970 From: mgottam@codeaurora.org Subject: Re: [PATCH] media: venus: add support for key frame Date: Wed, 24 Oct 2018 19:07:45 +0530 Message-ID: References: <1539071634-1644-1-git-send-email-mgottam@codeaurora.org> <40d15ea4-48e2-b2c7-1d70-68dcc1b08990@linaro.org> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: Sender: linux-kernel-owner@vger.kernel.org To: Tomasz Figa Cc: Alexandre Courbot , Stanimir Varbanov , Hans Verkuil , Mauro Carvalho Chehab , Linux Media Mailing List , Linux Kernel Mailing List , linux-arm-msm , vgarodia@codeaurora.org List-Id: linux-arm-msm@vger.kernel.org On 2018-10-23 08:37, Tomasz Figa wrote: > On Mon, Oct 22, 2018 at 3:15 PM Alexandre Courbot > wrote: >> >> On Fri, Oct 12, 2018 at 5:10 PM Stanimir Varbanov >> wrote: >> > >> > >> > >> > On 10/12/2018 11:06 AM, Alexandre Courbot wrote: >> > > On Fri, Oct 12, 2018 at 4:37 PM Stanimir Varbanov >> > > wrote: >> > >> >> > >> Hi Alex, >> > >> >> > >> On 10/12/2018 08:26 AM, Alexandre Courbot wrote: >> > >>> On Tue, Oct 9, 2018 at 4:54 PM Malathi Gottam wrote: >> > >>>> >> > >>>> When client requests for a keyframe, set the property >> > >>>> to hardware to generate the sync frame. >> > >>>> >> > >>>> Signed-off-by: Malathi Gottam >> > >>>> --- >> > >>>> 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.