linux-kernel - Re: [PATCH v2 3/8] hantro: Use v4l2_m2m_buf_done_and_job

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:   Wed, 25 Mar 2020 09:22:04 +0100
From:   Hans Verkuil <hverkuil@...all.nl>
To:     Ezequiel Garcia <ezequiel@...labora.com>,
        linux-media@...r.kernel.org, linux-rockchip@...ts.infradead.org,
        linux-kernel@...r.kernel.org
Cc:     Tomasz Figa <tfiga@...omium.org>,
        Nicolas Dufresne <nicolas@...fresne.ca>, kernel@...labora.com,
        Jonas Karlman <jonas@...boo.se>,
        Heiko Stuebner <heiko@...ech.de>,
        Alexandre Courbot <acourbot@...omium.org>,
        Jeffrey Kardatzke <jkardatzke@...omium.org>,
        Rob Herring <robh@...nel.org>
Subject: Re: [PATCH v2 3/8] hantro: Use v4l2_m2m_buf_done_and_job_finish

On 3/18/20 2:21 PM, Ezequiel Garcia wrote:
> Let the core sort out the nuances of returning buffers
> to userspace, by using the v4l2_m2m_buf_done_and_job_finish
> helper.
> 
> This change also removes usage of buffer sequence fields,
> which shouldn't have any meaning for stateless decoders.

Uh, why remove this? For one, doesn't this cause fails in v4l2-compliance?
Also, while I agree that it is not terribly useful, it doesn't hurt, does it?

And the V4L2 spec makes no exception for stateless codecs with respect to the
sequence field.

Nacked-by: Hans Verkuil <hverkuil-cisco@...all.nl>

Regards,

	Hans

> 
> Signed-off-by: Ezequiel Garcia <ezequiel@...labora.com>
> ---
>  drivers/staging/media/hantro/hantro_drv.c | 27 ++++++++---------------
>  1 file changed, 9 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/staging/media/hantro/hantro_drv.c b/drivers/staging/media/hantro/hantro_drv.c
> index 0b1200fc0e1a..ec889d755cd6 100644
> --- a/drivers/staging/media/hantro/hantro_drv.c
> +++ b/drivers/staging/media/hantro/hantro_drv.c
> @@ -94,32 +94,23 @@ static void hantro_job_finish(struct hantro_dev *vpu,
>  			      unsigned int bytesused,
>  			      enum vb2_buffer_state result)
>  {
> -	struct vb2_v4l2_buffer *src, *dst;
>  	int ret;
>  
>  	pm_runtime_mark_last_busy(vpu->dev);
>  	pm_runtime_put_autosuspend(vpu->dev);
>  	clk_bulk_disable(vpu->variant->num_clocks, vpu->clocks);
>  
> -	src = v4l2_m2m_src_buf_remove(ctx->fh.m2m_ctx);
> -	dst = v4l2_m2m_dst_buf_remove(ctx->fh.m2m_ctx);
> -
> -	if (WARN_ON(!src))
> -		return;
> -	if (WARN_ON(!dst))
> -		return;
> -
> -	src->sequence = ctx->sequence_out++;
> -	dst->sequence = ctx->sequence_cap++;
> -
> -	ret = ctx->buf_finish(ctx, &dst->vb2_buf, bytesused);
> -	if (ret)
> -		result = VB2_BUF_STATE_ERROR;
> +	if (ctx->buf_finish) {
> +		struct vb2_v4l2_buffer *dst;
>  
> -	v4l2_m2m_buf_done(src, result);
> -	v4l2_m2m_buf_done(dst, result);
> +		dst = v4l2_m2m_next_dst_buf(ctx->fh.m2m_ctx);
> +		ret = ctx->buf_finish(ctx, &dst->vb2_buf, bytesused);
> +		if (ret)
> +			result = VB2_BUF_STATE_ERROR;
> +	}
>  
> -	v4l2_m2m_job_finish(vpu->m2m_dev, ctx->fh.m2m_ctx);
> +	v4l2_m2m_buf_done_and_job_finish(ctx->dev->m2m_dev, ctx->fh.m2m_ctx,
> +					 result);
>  }
>  
>  void hantro_irq_done(struct hantro_dev *vpu, unsigned int bytesused,
>