media: bcm2835-unicam: Correctly handle error states on stream on/off

[naushir]

On a failure in start_streaming(), the error code would not propagate to
the calling function on all conditions. This would cause the userland
caller to not know of the failure.

Additionally, clk_disable_unprepare() was called unconditionally in
stop_streaming(). This is incorrect in the cases where start_streaming()
fails, and unprepares all clocks as part of the failure cleanup. Ensure
that clk_disable_unprepare() is only called in stop_streaming() if the
clocks are in a prepared state.

Signed-off-by: Naushir Patuck naush@raspberrypi.com

[naushir]

@6by9 and @kbingham

[kbingham]

ouch - that's subtle!
Good spot ;-)

[6by9]

This should really be two patches, but never mind.

With the start_streaming fixed to return an error, does the framework actually call stop_streaming if VIDIOC_STREAMOFF is called multiple times? I don't think it will as https://elixir.bootlin.com/linux/latest/source/drivers/media/common/videobuf2/videobuf2-core.c#L1925 will block it.

However isn't it easier to do a

if (!vb2_is_streaming(vq))
   return 0;

at the start of the function, rather than keeping our own internal state? We should never exit start_streaming without either succeeding, or having undone all changes.

[naushir]

With the start_streaming fixed to return an error, does the framework actually call stop_streaming if VIDIOC_STREAMOFF is called multiple times? I don't think it will as https://elixir.bootlin.com/linux/latest/source/drivers/media/common/videobuf2/videobuf2-core.c#L1925 will block it.

It does seem to go into stop_streaming() with the error return value fixed. I get the clock_disable_unprepare() errors thrown into the kernel logs.

However isn't it easier to do a

if (!vb2_is_streaming(vq))
   return 0;

at the start of the function, rather than keeping our own internal state? We should never exit start_streaming without either succeeding, or having undone all changes.

Sure, I can make that change and push a new version.

[naushir]

However isn't it easier to do a

if (!vb2_is_streaming(vq))
   return 0;

at the start of the function, rather than keeping our own internal state? We should never exit start_streaming without either succeeding, or having undone all changes.

Turns out, this is not the case :)

In unicam_start_streaming, if both nodes are not set to streaming, we pretend the request was successful without touching clocks:

	node->streaming = true;
	if (!(dev->node[IMAGE_PAD].open && dev->node[IMAGE_PAD].streaming &&
	      (!dev->node[METADATA_PAD].open ||
	       dev->node[METADATA_PAD].streaming))) {
		/*
		 * Metadata pad must be enabled before image pad if it is
		 * wanted.
		 */
		unicam_dbg(3, dev, "Not all nodes are streaming yet.");
		return 0;
	}

This causes the internal vb2 state to think we are streaming, so the suggested test in unicam_stop_streaming using the vb2 state will pass, and we go and mess around with the clocks when we should not. Having the state tracked in unicam itself does seem to be the cleanest thing to do.

[6by9]

Ah, that annoying second device node. Who thought that was a good idea?! ;-)

[naushir]

Ah, that annoying second device node. Who thought that was a good idea?! ;-)

Indeed. I'll push an update with the existing changes split into 2 patches.

[kbingham]

Ah, that annoying second device node. Who thought that was a good idea?! ;-)

I'm with you there. I wish metadata travelled with/was associated with the buffer it's representing directly somehow.
Synchronising multiple device nodes for that is such a pain.

[naushir]

Synchronising multiple device nodes for that is such a pain.

Tell me about it!

[kbingham]

Hrm, not quite used to github yet, I thought this was just on the single return bug fix patch, but it looks like here it's on both ;-)
Both look ok to me though I think.

Reviewed-by: Kieran Bingham kieran.bingham@ideasonboard.com