| Message ID | 20190117162008.25217-9-stanimir.varbanov@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Venus stateful Codec API | expand |
On Fri, Jan 18, 2019 at 1:21 AM Stanimir Varbanov <stanimir.varbanov@linaro.org> wrote: > > Until now we returned num_output_bufs set during reqbuf but > that could be wrong when we implement stateful Codec API. So > get the minimum buffers for capture from HFI. This is supposed > to be called after stream header parsing, i.e. after dequeue > v4l2 event for change resolution. > > Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org> > --- > drivers/media/platform/qcom/venus/vdec_ctrls.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/drivers/media/platform/qcom/venus/vdec_ctrls.c b/drivers/media/platform/qcom/venus/vdec_ctrls.c > index f4604b0cd57e..e1da87bf52bc 100644 > --- a/drivers/media/platform/qcom/venus/vdec_ctrls.c > +++ b/drivers/media/platform/qcom/venus/vdec_ctrls.c > @@ -16,6 +16,7 @@ > #include <media/v4l2-ctrls.h> > > #include "core.h" > +#include "helpers.h" > #include "vdec.h" > > static int vdec_op_s_ctrl(struct v4l2_ctrl *ctrl) > @@ -47,7 +48,9 @@ static int vdec_op_g_volatile_ctrl(struct v4l2_ctrl *ctrl) > { > struct venus_inst *inst = ctrl_to_inst(ctrl); > struct vdec_controls *ctr = &inst->controls.dec; > + struct hfi_buffer_requirements bufreq; > union hfi_get_property hprop; > + enum hfi_version ver = inst->core->res->hfi_version; > u32 ptype = HFI_PROPERTY_PARAM_PROFILE_LEVEL_CURRENT; > int ret; > > @@ -71,7 +74,9 @@ static int vdec_op_g_volatile_ctrl(struct v4l2_ctrl *ctrl) > ctrl->val = ctr->post_loop_deb_mode; > break; > case V4L2_CID_MIN_BUFFERS_FOR_CAPTURE: > - ctrl->val = inst->num_output_bufs; > + ret = venus_helper_get_bufreq(inst, HFI_BUFFER_OUTPUT, &bufreq); > + if (!ret) > + ctrl->val = HFI_BUFREQ_COUNT_MIN(&bufreq, ver); What happens if venus_helper_get_bufreq() returns an error? It seems that we just happily continue with whatever the previous value of ctrl->val was. It seems like we do the same with other controls as well. I think you can fix this globally by initializing ret to 0 at the beginning of the function, and then returning ret instead of 0 at the end. That way all errors would be propagated. Of course please check that this is relevant for other controls following this scheme before doing so. :)
Hi Alex, Thanks for the comments! On 1/24/19 10:43 AM, Alexandre Courbot wrote: > On Fri, Jan 18, 2019 at 1:21 AM Stanimir Varbanov > <stanimir.varbanov@linaro.org> wrote: >> >> Until now we returned num_output_bufs set during reqbuf but >> that could be wrong when we implement stateful Codec API. So >> get the minimum buffers for capture from HFI. This is supposed >> to be called after stream header parsing, i.e. after dequeue >> v4l2 event for change resolution. >> >> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org> >> --- >> drivers/media/platform/qcom/venus/vdec_ctrls.c | 7 ++++++- >> 1 file changed, 6 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/media/platform/qcom/venus/vdec_ctrls.c b/drivers/media/platform/qcom/venus/vdec_ctrls.c >> index f4604b0cd57e..e1da87bf52bc 100644 >> --- a/drivers/media/platform/qcom/venus/vdec_ctrls.c >> +++ b/drivers/media/platform/qcom/venus/vdec_ctrls.c >> @@ -16,6 +16,7 @@ >> #include <media/v4l2-ctrls.h> >> >> #include "core.h" >> +#include "helpers.h" >> #include "vdec.h" >> >> static int vdec_op_s_ctrl(struct v4l2_ctrl *ctrl) >> @@ -47,7 +48,9 @@ static int vdec_op_g_volatile_ctrl(struct v4l2_ctrl *ctrl) >> { >> struct venus_inst *inst = ctrl_to_inst(ctrl); >> struct vdec_controls *ctr = &inst->controls.dec; >> + struct hfi_buffer_requirements bufreq; >> union hfi_get_property hprop; >> + enum hfi_version ver = inst->core->res->hfi_version; >> u32 ptype = HFI_PROPERTY_PARAM_PROFILE_LEVEL_CURRENT; >> int ret; >> >> @@ -71,7 +74,9 @@ static int vdec_op_g_volatile_ctrl(struct v4l2_ctrl *ctrl) >> ctrl->val = ctr->post_loop_deb_mode; >> break; >> case V4L2_CID_MIN_BUFFERS_FOR_CAPTURE: >> - ctrl->val = inst->num_output_bufs; >> + ret = venus_helper_get_bufreq(inst, HFI_BUFFER_OUTPUT, &bufreq); >> + if (!ret) >> + ctrl->val = HFI_BUFREQ_COUNT_MIN(&bufreq, ver); > > What happens if venus_helper_get_bufreq() returns an error? It seems > that we just happily continue with whatever the previous value of > ctrl->val was. It seems like we do the same with other controls as > well. I agree that this is wrong, I think no-one userspace client used that g_ctrls controls :) > > I think you can fix this globally by initializing ret to 0 at the > beginning of the function, and then returning ret instead of 0 at the > end. That way all errors would be propagated. Of course please check > that this is relevant for other controls following this scheme before > doing so. :) > yes, I will do!
diff --git a/drivers/media/platform/qcom/venus/vdec_ctrls.c b/drivers/media/platform/qcom/venus/vdec_ctrls.c index f4604b0cd57e..e1da87bf52bc 100644 --- a/drivers/media/platform/qcom/venus/vdec_ctrls.c +++ b/drivers/media/platform/qcom/venus/vdec_ctrls.c @@ -16,6 +16,7 @@ #include <media/v4l2-ctrls.h> #include "core.h" +#include "helpers.h" #include "vdec.h" static int vdec_op_s_ctrl(struct v4l2_ctrl *ctrl) @@ -47,7 +48,9 @@ static int vdec_op_g_volatile_ctrl(struct v4l2_ctrl *ctrl) { struct venus_inst *inst = ctrl_to_inst(ctrl); struct vdec_controls *ctr = &inst->controls.dec; + struct hfi_buffer_requirements bufreq; union hfi_get_property hprop; + enum hfi_version ver = inst->core->res->hfi_version; u32 ptype = HFI_PROPERTY_PARAM_PROFILE_LEVEL_CURRENT; int ret; @@ -71,7 +74,9 @@ static int vdec_op_g_volatile_ctrl(struct v4l2_ctrl *ctrl) ctrl->val = ctr->post_loop_deb_mode; break; case V4L2_CID_MIN_BUFFERS_FOR_CAPTURE: - ctrl->val = inst->num_output_bufs; + ret = venus_helper_get_bufreq(inst, HFI_BUFFER_OUTPUT, &bufreq); + if (!ret) + ctrl->val = HFI_BUFREQ_COUNT_MIN(&bufreq, ver); break; default: return -EINVAL;
Until now we returned num_output_bufs set during reqbuf but that could be wrong when we implement stateful Codec API. So get the minimum buffers for capture from HFI. This is supposed to be called after stream header parsing, i.e. after dequeue v4l2 event for change resolution. Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org> --- drivers/media/platform/qcom/venus/vdec_ctrls.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)