This patch seems incomplete to me? On the one hand you're saying, you only
work with i.MX27, but you've left
static void mx27_camera_frame_done(struct mx2_camera_dev *pcdev, int state)
{
struct videobuf_buffer *vb;
TBH, I don't understand how you've tested this patch: it doesn't compile
for me for i.MX27. And to use EMMA CONFIG_MACH_MX27 has to be on too,
right? Confused...
On Fri, 20 Jan 2012, Javier Martin wrote:
>
> Signed-off-by: Javier Martin <[email protected]>
> ---
> drivers/media/video/mx2_camera.c | 277
> ++++++++++++++++++--------------------
> 1 files changed, 128 insertions(+), 149 deletions(-)
>
> diff --git a/drivers/media/video/mx2_camera.c
> b/drivers/media/video/mx2_camera.c
> index 68038e7..290ac9d 100644
> --- a/drivers/media/video/mx2_camera.c
> +++ b/drivers/media/video/mx2_camera.c
[snip]
> @@ -256,13 +257,25 @@ struct mx2_camera_dev {
> size_t discard_size;
> struct mx2_fmt_cfg *emma_prp;
> u32 frame_count;
> + struct vb2_alloc_ctx *alloc_ctx;
> +};
> +
> +enum mx2_buffer_state {
> + MX2_STATE_NEEDS_INIT = 0,
> + MX2_STATE_PREPARED = 1,
> + MX2_STATE_QUEUED = 2,
> + MX2_STATE_ACTIVE = 3,
> + MX2_STATE_DONE = 4,
> + MX2_STATE_ERROR = 5,
> + MX2_STATE_IDLE = 6,
Are the numerical values important? If not - please, drop. And actually,
you don't need most of these states, I wouldn't be surprised, if you
didn't need them at all. You might want to revise them in a future patch.
[snip]
> @@ -467,59 +479,47 @@ static irqreturn_t mx25_camera_irq(int irq_csi, void
> *data)
> /*
> * Videobuf operations
> */
> -static int mx2_videobuf_setup(struct videobuf_queue *vq, unsigned int *count,
> - unsigned int *size)
> +static int mx2_videobuf_setup(struct vb2_queue *vq,
> + const struct v4l2_format *fmt,
> + unsigned int *count, unsigned int *num_planes,
> + unsigned int sizes[], void *alloc_ctxs[])
> {
> - struct soc_camera_device *icd = vq->priv_data;
> + struct soc_camera_device *icd = soc_camera_from_vb2q(vq);
> + struct soc_camera_host *ici = to_soc_camera_host(icd->parent);
> + struct mx2_camera_dev *pcdev = ici->priv;
> int bytes_per_line = soc_mbus_bytes_per_line(icd->user_width,
> icd->current_fmt->host_fmt);
You choose not to support VIDIOC_CREATE_BUFS? You have to at least return
an error if fmt != NULL. Or consider supporting it - look at mx3_camera.c
or sh_mobile_ceu_camera.c (btw, atmel-isi.c has to be fixed with this
respect too). If you decide to support it, you'll also have to drop
.buf_prepare() (see, e.g., 07f92448045a23d27dbc3ece3abcb6bafc618d43)
[snip]
> @@ -529,46 +529,34 @@ static int mx2_videobuf_prepare(struct videobuf_queue
> *vq,
> * This can be useful if you want to see if we actually fill
> * the buffer with something
> */
> - memset((void *)vb->baddr, 0xaa, vb->bsize);
> + memset((void *)vb2_plane_vaddr(vb, 0),
> + 0xaa, vb2_get_plane_payload(vb, 0));
> #endif
>
> - if (buf->code != icd->current_fmt->code ||
> - vb->width != icd->user_width ||
> - vb->height != icd->user_height ||
> - vb->field != field) {
> + if (buf->code != icd->current_fmt->code) {
> buf->code = icd->current_fmt->code;
> - vb->width = icd->user_width;
> - vb->height = icd->user_height;
> - vb->field = field;
> - vb->state = VIDEOBUF_NEEDS_INIT;
> + buf->state = MX2_STATE_NEEDS_INIT;
This looks broken or most likely redundant to me. The check for a changed
code was there to reallocate the buffer, doesn't seem to make much sense
now.
[snip]
> @@ -686,10 +673,10 @@ static void mx2_videobuf_release(struct videobuf_queue
> *vq,
> * types.
> */
> spin_lock_irqsave(&pcdev->lock, flags);
> - if (vb->state == VIDEOBUF_QUEUED) {
> - list_del(&vb->queue);
> - vb->state = VIDEOBUF_ERROR;
> - } else if (cpu_is_mx25() && vb->state == VIDEOBUF_ACTIVE) {
> + if (buf->state == MX2_STATE_QUEUED || buf->state == MX2_STATE_ACTIVE) {
> + list_del_init(&buf->queue);
> + buf->state = MX2_STATE_NEEDS_INIT;
> + } else if (cpu_is_mx25() && buf->state == MX2_STATE_ACTIVE) {
This doesn't look right. You already have " || buf->state ==
MX2_STATE_ACTIVE" above, so, this latter condition is never entered?
Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
