[v2,04/12] staging: media: rkvdec: Block start streaming until both queues run

Ensure that both the CAPTURE and the OUTPUT queue are running (e.g. busy
-> have buffers allocated) before starting the actual streaming process.

Signed-off-by: Sebastian Fricke <sebastian.fricke@collabora.com>
---
 drivers/staging/media/rkvdec/rkvdec.c | 7 +++++++
 1 file changed, 7 insertions(+)
  

Hi Sebastian,

On Thu, Jan 12, 2023 at 9:56 AM Sebastian Fricke
<sebastian.fricke@collabora.com> wrote:
>
> Ensure that both the CAPTURE and the OUTPUT queue are running (e.g. busy
> -> have buffers allocated) before starting the actual streaming process.
>

Usually, you want to write the "why" in the commit description,
instead of the "what",
which is (hopefully) more or less clear by reading the commit change.

The commit description should have enough information to understand
what is the impact of merging the commit (or what is the bug without
the merging the commit).

If you are fixing a spec violation, adding a reference to the spec is important,
if you are fixing a v4l2-compliance, pasting the error, etc.

> Signed-off-by: Sebastian Fricke <sebastian.fricke@collabora.com>
> ---
>  drivers/staging/media/rkvdec/rkvdec.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/drivers/staging/media/rkvdec/rkvdec.c b/drivers/staging/media/rkvdec/rkvdec.c
> index c849f6c20279..e0e95d06e216 100644
> --- a/drivers/staging/media/rkvdec/rkvdec.c
> +++ b/drivers/staging/media/rkvdec/rkvdec.c
> @@ -562,6 +562,13 @@ static int rkvdec_start_streaming(struct vb2_queue *q, unsigned int count)
>         if (V4L2_TYPE_IS_CAPTURE(q->type))
>                 return 0;
>
> +       /*
> +        * Make sure that both the output and the capture queue are running
> +        */
> +       if (rkvdec_queue_busy(ctx, V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) == 0 ||
> +           rkvdec_queue_busy(ctx, V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) == 0)
> +               return -EAGAIN;
> +

IIRC, this is already handled by Videobuf2 core, see the comment for
vb2_start_streaming,
in drivers/media/common/videobuf2/videobuf2-core.c. There's also a
!q->num_buffers check
in vb2_core_streamon.

In general, if you see some kind of bug in a generic logic like this,
it's a red flag that something is wrong at the core level.

Thanks!
Ezequiel

>         desc = ctx->coded_fmt_desc;
>         if (WARN_ON(!desc))
>                 return -EINVAL;
>
> --
> 2.25.1
  

On Thu, Jan 12, 2023 at 11:55:07AM -0300, Ezequiel Garcia wrote:
> Hi Sebastian,
> 
> On Thu, Jan 12, 2023 at 9:56 AM Sebastian Fricke
> <sebastian.fricke@collabora.com> wrote:
> >
> > Ensure that both the CAPTURE and the OUTPUT queue are running (e.g. busy
> > -> have buffers allocated) before starting the actual streaming process.
> >
> 
> Usually, you want to write the "why" in the commit description,
> instead of the "what",
> which is (hopefully) more or less clear by reading the commit change.
> 
> The commit description should have enough information to understand
> what is the impact of merging the commit (or what is the bug without
> the merging the commit).
> 
> If you are fixing a spec violation, adding a reference to the spec is important,
> if you are fixing a v4l2-compliance, pasting the error, etc.
> 

Yeah, and if it's a bug fix then add a Fixes tag as well.  Even if the
bug was added when the driver was first merged, that's still useful
information.

regards,
dan carpenter
  

Hi Sebastian,

Ezequiel and Dan have had some comments, they need addressing.

If, as a result, the logic of this patch remains in place then:

W dniu 12.01.2023 o 13:56, Sebastian Fricke pisze:
> Ensure that both the CAPTURE and the OUTPUT queue are running (e.g. busy
> -> have buffers allocated) before starting the actual streaming process.
> 
> Signed-off-by: Sebastian Fricke <sebastian.fricke@collabora.com>
> ---
>   drivers/staging/media/rkvdec/rkvdec.c | 7 +++++++
>   1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/staging/media/rkvdec/rkvdec.c b/drivers/staging/media/rkvdec/rkvdec.c
> index c849f6c20279..e0e95d06e216 100644
> --- a/drivers/staging/media/rkvdec/rkvdec.c
> +++ b/drivers/staging/media/rkvdec/rkvdec.c
> @@ -562,6 +562,13 @@ static int rkvdec_start_streaming(struct vb2_queue *q, unsigned int count)
>   	if (V4L2_TYPE_IS_CAPTURE(q->type))
>   		return 0;
>   
> +	/*
> +	 * Make sure that both the output and the capture queue are running
> +	 */
> +	if (rkvdec_queue_busy(ctx, V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) == 0 ||
> +	    rkvdec_queue_busy(ctx, V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) == 0)

if you convert to a macro in PATCH 03/12 then the conditions can be rewritten as

!rkvdec_queue_busy(.....)

instead of comparing to 0. (actually, this can be done regardless, but makes 
more sense if the result of rkvdec_queue_busy() is boolean which it is when
it becomes a macro as suggested)

Then you can optionally follow De Morgan's law:

if (!(rkvdec_queue_busy() && rkvdec_queue_busy()))
	return -EAGAIN;

and you get one negation less.

Regards,

Andrzej

> +		return -EAGAIN;
> +
>   	desc = ctx->coded_fmt_desc;
>   	if (WARN_ON(!desc))
>   		return -EINVAL;
>