linux-kernel - Re: [PATCH v1 3/5] media: venus: do not destroy video session during queue setup

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 9 Nov 2018 11:59:49 +0200
From:   Stanimir Varbanov <stanimir.varbanov@...aro.org>
To:     Srinu Gorle <sgorle@...eaurora.org>, stanimir.varbanov@...aro.org,
        hverkuil@...all.nl, mchehab@...nel.org,
        linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-arm-msm@...r.kernel.org
Cc:     acourbot@...omium.org, vgarodia@...eaurora.org
Subject: Re: [PATCH v1 3/5] media: venus: do not destroy video session during
 queue setup

Hi Srinu,

On 9/29/18 3:00 PM, Srinu Gorle wrote:
> - open and close video sessions for plane properties is incorrect.

Could you rephrase this statement? I really don't understand what you mean.

> - add check to ensure, same instance persist from driver open to close.

This assumption is wrong. The v4l client can change the codec by SFMT
without close the device node, in that case we have to destroy and
create a new session with new codec.

> 
> Signed-off-by: Srinu Gorle <sgorle@...eaurora.org>
> ---
>  drivers/media/platform/qcom/venus/hfi.c  | 3 +++
>  drivers/media/platform/qcom/venus/vdec.c | 2 ++
>  2 files changed, 5 insertions(+)
> 
> diff --git a/drivers/media/platform/qcom/venus/hfi.c b/drivers/media/platform/qcom/venus/hfi.c
> index 36a4784..59c34ba 100644
> --- a/drivers/media/platform/qcom/venus/hfi.c
> +++ b/drivers/media/platform/qcom/venus/hfi.c
> @@ -207,6 +207,9 @@ int hfi_session_init(struct venus_inst *inst, u32 pixfmt)
>  	const struct hfi_ops *ops = core->ops;
>  	int ret;
>  
> +	if (inst->state >= INST_INIT && inst->state < INST_STOP)
> +		return 0;

In fact you want to be able to call session_init multiple times but
deinit the session only once? The hfi.c layer is designed to follow the
states as they are expected by the firmware side, if you want to call
session_init multiple times just make a wrapper in the vdec.c with
reference counting.

> +
>  	inst->hfi_codec = to_codec_type(pixfmt);
>  	reinit_completion(&inst->done);
>  
> diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c
> index afe3b36..0035cf2 100644
> --- a/drivers/media/platform/qcom/venus/vdec.c
> +++ b/drivers/media/platform/qcom/venus/vdec.c
> @@ -700,6 +700,8 @@ static int vdec_num_buffers(struct venus_inst *inst, unsigned int *in_num,
>  
>  	*out_num = HFI_BUFREQ_COUNT_MIN(&bufreq, ver);
>  
> +	return 0;

OK in present implementation I decided that the codec is settled when
streamon on both queues is called (i.e. the final session_init is made
in streamon). IMO the correct one is to init the session in
reqbuf(output) and deinit session in reqbuf(output, count=0)?

The problem I see when you skip session_deinit is that the codec cannot
be changed without closing the video node.

> +
>  deinit:
>  	hfi_session_deinit(inst);
>  
> 

-- 
regards,
Stan