RE: [PATCH 3/4] MFC: Add MFC 5.1 V4L2 driver

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

 



I have some opinion about the usage of wait_event_interruptible_timeout()


k.debski@xxxxxxxxxxx wrote:
[snip]

> +
> diff --git a/drivers/media/video/s5p-mfc/s5p_mfc_intr.c
> b/drivers/media/video/s5p-mfc/s5p_mfc_intr.c
> new file mode 100644
> index 0000000..543f3fb
> --- /dev/null
> +++ b/drivers/media/video/s5p-mfc/s5p_mfc_intr.c
> @@ -0,0 +1,77 @@
> +/*
> + * drivers/media/video/samsung/mfc5/s5p_mfc_intr.c
> + *
> + * C file for Samsung MFC (Multi Function Codec - FIMV) driver
> + * This file contains functions used to wait for command completion.
> + *
> + * Kamil Debski, Copyright (c) 2010 Samsung Electronics
> + * http://www.samsung.com/
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/errno.h>
> +#include <linux/wait.h>
> +#include <linux/sched.h>
> +#include <linux/io.h>
> +#include "regs-mfc5.h"
> +#include "s5p_mfc_intr.h"
> +#include "s5p_mfc_logmsg.h"
> +#include "s5p_mfc_common.h"
> +
> +int s5p_mfc_wait_for_done_dev(struct s5p_mfc_dev *dev, int command)
> +{
> +	if (wait_event_interruptible_timeout(dev->queue,
> +		(dev->int_cond && (dev->int_type == command
> +		|| dev->int_type == S5P_FIMV_R2H_CMD_DECODE_ERR_RET)),
> +		msecs_to_jiffies(MFC_INT_TIMEOUT)) == 0) {
> +		mfc_err("Interrupt (%d dev) timed out.\n", dev->int_type);
> +		return 1;
> +	}
> +	mfc_debug("Finished waiting (dev->queue, %d).\n", dev->int_type);
> +	if (dev->int_type == S5P_FIMV_R2H_CMD_ERROR_RET)
> +		return 1;
> +	return 0;
> +}


You used wait_event_interruptible_timeout() in the driver, but this
function is 
considered not only int. by MFC but also int. by some signal. I'm wondering
whether we have to consider interrupt by signal in the middle of hw
operation.
and also I cannot see some operation(handler) in case of wake-up by signal.
So, why don’t you remove the interruptible function or add some operation
in case of 
wake-up by signal. (refer to the other driver in the kernel)

> +
> +void s5p_mfc_clean_dev_int_flags(struct s5p_mfc_dev *dev)
> +{
> +	dev->int_cond = 0;
> +	dev->int_type = 0;
> +	dev->int_err = 0;
> +}
> +
> +int s5p_mfc_wait_for_done_ctx(struct s5p_mfc_ctx *ctx,
> +				    int command, int interrupt)
> +{
> +	int ret;
> +	if (interrupt) {
> +		ret = wait_event_interruptible_timeout(ctx->queue,
> +				(ctx->int_cond && (ctx->int_type == command
> +			|| ctx->int_type ==
S5P_FIMV_R2H_CMD_DECODE_ERR_RET)),
> +					msecs_to_jiffies(MFC_INT_TIMEOUT));
> +	} else {
> +		ret = wait_event_timeout(ctx->queue,
> +				(ctx->int_cond && (ctx->int_type == command
> +			|| ctx->int_type ==
S5P_FIMV_R2H_CMD_DECODE_ERR_RET)),
> +					msecs_to_jiffies(MFC_INT_TIMEOUT));
> +	}
> +	if (ret == 0) {
> +		mfc_err("Interrupt (%d ctx) timed out.\n", ctx->int_type);
> +		return 1;
> +	}
> +	mfc_debug("Finished waiting (ctx->queue, %d).\n", ctx->int_type);
> +	if (ctx->int_type == S5P_FIMV_R2H_CMD_ERROR_RET)
> +		return 1;
> +	return 0;
> +}
> +
> +void s5p_mfc_clean_ctx_int_flags(struct s5p_mfc_ctx *ctx)
> +{
> +	ctx->int_cond = 0;
> +	ctx->int_type = 0;
> +	ctx->int_err = 0;
> +}

[snip]

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

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


[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux