LinuxTV Patchwork [08/10] venus: vdec_ctrls: get real minimum buffers for capture

login
register
mail settings
Submitter Stanimir Varbanov
Date Jan. 17, 2019, 4:20 p.m.
Message ID <20190117162008.25217-9-stanimir.varbanov@linaro.org>
Download mbox | patch
Permalink /patch/53985/
State New
Headers show

Comments

Stanimir Varbanov - Jan. 17, 2019, 4:20 p.m.
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(-)
Alexandre Courbot - Jan. 24, 2019, 8:43 a.m.
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. :)
Stanimir Varbanov - Jan. 24, 2019, 9:59 a.m.
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!

Patch

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;

Privacy Policy