Re: [REVIEW PATCH 11/14] OMAP34XXCAM: Add driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hans Verkuil wrote:
Sergio has posted earlier a patchset containing a driver for using the
ISP to process images from memory to memory. The ISP driver is used
roughly the same way as with the omap34xxcam and real sensors. The
interface towards the userspace offered by the driver, however, is
different, you probably saw it (preview and resizer wrappers).

My opinion has been that the memory-to-memory operation of the ISP
should also offer V4L2 interface. V4L2, however, doesn't support such
devices at the moment. The only differences that I can see is that

1. the input is a video buffer instead of sensor and

2. the source format needs to be specified somehow since the ISP can
also do format conversion. So it's output and input at the same time.

But if we had one video device per ISP, then memory-to-memory operation
would be just one... input or output or what? :)

Earlier we were thinking of creating one device node for it.

This sounds like a codec interface as 'described' here:

http://www.xs4all.nl/~hverkuil/spec/v4l2.html#CODEC

It would be a first for V4L2 to have a driver that can do this, but I agree that that would be a single device that has both 'output' and 'capture'.

Ok. Although this work most probably will be left for future at this point.

Currently you can have just one device node using the ISP open.
omap34xxcam_open() calls isp_get() which fails if the ISP use count was
non-zero (means one).

Or did I misunderstood something?

Oh dear. Please don't use 'use counts'. It is perfectly acceptable and desirable to have multiple opens on the same video node. Only one file

Use counts are really bad and totally unnecessary. Only if another file handle is in streaming mode (and when using VIDIOC_S_PRIORITY) does it make sense to return -EBUSY for certain ioctls or read/write operations. Otherwise you shouldn't limit the user from opening the same device node as many times as he wants and use that to query the video device.

?

Having a use count doesn't prevent multiple file handles nor otherwise artificially limit functionality. We need to be able to shut down the slaves when they are no longer needed. IMO having an use count to do this is fine (unless otherwise proven).

Also the camera driver does try_module_get() to the slaves when it's opened by the first user. module_put() is called on those when the last user goes away.

We'd also like to get rid of the current way of directly telling the slaves what their power state should be. Rather we'd like to tell the slaves what's expected from them. This could translate to open/release/streamon/streamoff commands. To be able to do this, the use count is required --- unless this task is given to the slaves (v4l2_subdevs).

BTW, I looked at omap24xxcam_open(): data like fh->pix does *not* belong to the filehandle struct, it should be part of the top-level data structure.

That's fixed in the omap34xxcam.c. :)

You want to be able to do simple things like querying a video node for the currently selected format. You can't do that if the format is stored in the filehandle! E.g.: you are streaming and you want to run v4l2-ctl --get-fmt-video to check what video format is being used. Things like this must be supported by a well-written v4l2 driver. Again, sadly quite a few v4l2 drivers do this wrong as well :-(

I also see that cam->users is not decreased by one if omap24xxcam_sensor_enable() fails.

Note that I'm looking at the code in the v4l-dvb repository, the linux-omap git tree might have fixed that already.

I'm afraid it's still there. Will fix that.

Thanks.

--
Sakari Ailus
sakari.ailus@xxxxxxxxxxxxxxxxxxxxxxxxxx

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Index of Archives]     [Linux Arm (vger)]     [ARM Kernel]     [ARM MSM]     [Linux Tegra]     [Linux WPAN Networking]     [Linux Wireless Networking]     [Maemo Users]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Trails]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux