linux-kernel - Re: [PATCH] media: s5p-mfc: set V4L2_BUF_FLAG

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 for Android: free password hash cracker in your pocket

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

Message-ID: <21733d1ad3c2f79a5646b4f1c541dfabda0978be.camel@ndufresne.ca>
Date:   Tue, 02 Jun 2020 11:09:33 -0400
From:   Nicolas Dufresne <nicolas@...fresne.ca>
To:     Andriy Gelman <andriy.gelman@...il.com>
Cc:     Kyungmin Park <kyungmin.park@...sung.com>,
        Kamil Debski <kamil@...as.org>,
        Jeongtae Park <jtp.park@...sung.com>,
        Andrzej Hajda <a.hajda@...sung.com>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        linux-arm-kernel@...ts.infradead.org, linux-media@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] media: s5p-mfc: set V4L2_BUF_FLAG_LAST flag on final
 buffer

Hi Andriy,

thanks for you patch.

Le samedi 02 mai 2020 à 15:40 -0400, Andriy Gelman a écrit :
> From: Andriy Gelman <andriy.gelman@...il.com>
> 
> As per V4L2 api, the final buffer should set V4L2_BUF_FLAG_LAST flag.
> 
> Signed-off-by: Andriy Gelman <andriy.gelman@...il.com>
> ---
>  drivers/media/platform/s5p-mfc/s5p_mfc.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc.c b/drivers/media/platform/s5p-mfc/s5p_mfc.c
> index 5c2a23b953a4..b3d9b3a523fe 100644
> --- a/drivers/media/platform/s5p-mfc/s5p_mfc.c
> +++ b/drivers/media/platform/s5p-mfc/s5p_mfc.c
> @@ -614,6 +614,7 @@ static void s5p_mfc_handle_stream_complete(struct s5p_mfc_ctx *ctx)
>  		list_del(&mb_entry->list);
>  		ctx->dst_queue_cnt--;
>  		vb2_set_plane_payload(&mb_entry->b->vb2_buf, 0, 0);
> +		mb_entry->b->flags |= V4L2_BUF_FLAG_LAST;
>  		vb2_buffer_done(&mb_entry->b->vb2_buf, VB2_BUF_STATE_DONE);

The empty buffer is only there for backward compatibility. As the spec
says, userspace should completely ignore this buffer. I bet it will
still have the effect to set last_buffer_dequeued = true in vb2,
unblocking poll() operations and allowing for the queue to unblock and
return EPIPE on further DQBUF.

Perhaps you should make sure the if both the src and dst queues are
empty/done by the time cmd_stop is called it will still work. Other
drivers seems to handle this, but this one does not seems to have a
special case for that, which may hang userspace in a different way.

What you should do to verify this patch is correct, and that your
userpace does not rely on legacy path is that it should always be able
to detect the end of the drain with a EPIPE on DQBUF. LAST_BUF is just
an early signalling, but may not occur if there was nothing left to
produce (except for MFC which maybe be crafting a buffer in all cases,
but that's going a roundtrip through the HW, which is not clear will
work if the dst queue was empty).

As shared on IRC, you have sent these patch to FFMPEG:

https://patchwork.ffmpeg.org/project/ffmpeg/patch/20200429212942.28797-2-andriy.gelman@gmail.com/

This should have been clarified as supporting legacy drivers / older
kernel with Samsung driver. Seems like a fair patch. And you added:

https://patchwork.ffmpeg.org/project/ffmpeg/patch/20200429212942.28797-1-andriy.gelman@gmail.com/

This one should maybe add the clarification that this is an
optimization to skip an extra poll/dqbuf dance, but that end of
draining will ultimately be catched by EPIPE on dqbuf for the described
cases. Remains valid enhancement to ffmpeg imho.

>  	}
>