RE: [PATCH 3/4] MFC: Add MFC 5.1 V4L2 driver
I have some opinion about the usage of wait_event_interruptible_timeout()
k.deb...@samsung.com 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 000..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 majord...@vger.kernel.org
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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 3/4] MFC: Add MFC 5.1 V4L2 driver
Hi,
I commented as belows,
And you missed one important things 'cause there were my comments
in the very long email which is strongly fixed in the reset seq.
Yes, thanks for your suggestion.
[]
+#define READL(offset) readl(dev-regs_base + (offset))
+#define WRITEL(data, offset) writel((data), dev-regs_base +
(offset))
+#define OFFSETA(x) (((x) - dev-port_a) 11)
+#define OFFSETB(x) (((x) - dev-port_b) 11)
+
+/* Reset the device */
+static int s5p_mfc_cmd_reset(struct s5p_mfc_dev *dev)
+{
+ unsigned int mc_status;
+ unsigned long timeout;
+ mfc_debug(s5p_mfc_cmd_reset++\n);
+ /* Stop procedure */
+ WRITEL(0x3f7, S5P_FIMV_SW_RESET); /* reset VI */
Ahm, This (WRITEL(0x3f7, S5P_FIMV_SW_RESET)) might be a problem.
In the reset seq. of MFC driver, we checked out
That FW(s5pc110-mfc.fw in the s5p_mfc_load_firmware()) runned by RISC
core
at this point could access invalid address. It should be removed.
Thanks for pointing this out. I will remove it in the next version.
[snip]
Best regards
--
Kamil Debski
Linux Platform Group
Samsung Poland RD Center
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 3/4] MFC: Add MFC 5.1 V4L2 driver
Hello,
I have some opinion about the usage of
wait_event_interruptible_timeout()
k.deb...@samsung.com 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 000..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)
I can see a situation when handling a signal might be useful. If for some
reason (corrupt firmware file for example) the MFC fails to initialize then
the
application will seem frozen to the user. The user can press Ctrl-C and
expects the app to terminate. This would not happen if the used wait
procedure
was non interruptible.
Therefore I think adding code to handle signal is better than changing this
wait
to non interruptible.
+
+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]
Best regards,
--
Kamil Debski
Linux Platform Group
Samsung Poland RD Center
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 3/4] MFC: Add MFC 5.1 V4L2 driver
Hi, Kamil
This is second feedback about the HW op related code.
(s5p_mfc_opr.c s5p_mfc.c)
Hi, Peter
Thanks for the comments. I have replied to them below. I would be grateful
if you could cut out non relevant parts of the code in your replies. It was
difficult to find your comments in such a big email. I hope I have found all
of them.
[...]
+s5p_mfc_set_dec_stream_buffer(ctx, \
+ vb2_plane_paddr(temp_vb, 0), 0, \
+ temp_vb-v4l2_planes[0].bytesused);
+dev-curr_ctx = ctx-num;
+s5p_mfc_clean_ctx_int_flags(ctx);
+s5p_mfc_decode_one_frame(ctx,
+temp_vb-v4l2_planes[0].bytesused == 0);
+} else if (ctx-state == MFCINST_DEC_INIT) {
+/* Preparing decoding - getting instance number */
+mfc_debug(Getting instance number\n);
+dev-curr_ctx = ctx-num;
+s5p_mfc_clean_ctx_int_flags(ctx);
+/* s5p_mfc_set_dec_temp_buffers(ctx);
+ * Removed per request by Peter, check if MFC works OK
*/
Yes, you don't have to set s5p_mfc_set_dec_temp_buffers(ctx)at this point
'cause this is only required before parsing header of the stream except for
setting shared memory,
so, I think, separating 'setting S5P_FIMV_SI_CH0_HOST_WR_ADR' from the
s5p_mfc_set_dec_temp_buffers() is needed. So I mean
remove 'setting S5P_FIMV_SI_CH0_HOST_WR_ADR' from
s5p_mfc_set_dec_temp_buffers(ctx);, then add 'setting
S5P_FIMV_SI_CH0_HOST_WR_ADR'
in the 3 functions (s5p_mfc_set_dec_frame_buffer(),s5p_mfc_init_decode(),
s5p_mfc_decode_one_frame())
I also commented suggested loc. of 'setting S5P_FIMV_SI_CH0_HOST_WR_ADR'
Right, I've changed this.
+ret = s5p_mfc_open_inst(ctx);
+if (ret) {
+mfc_err(Failed to create a new
instance.\n);
+ctx-state = MFCINST_DEC_ERROR;
+}
+} else if (ctx-state == MFCINST_DEC_RETURN_INST) {
+/* Closing decoding instance */
+mfc_debug(Returning instance number\n);
+dev-curr_ctx = ctx-num;
+s5p_mfc_clean_ctx_int_flags(ctx);
+ret = s5p_mfc_return_inst_no(ctx);
+if (ret) {
+mfc_err(Failed to return an instance.\n);
+ctx-state = MFCINST_DEC_ERROR;
+}
[...]
+}
+/* Init videobuf2 queue for CAPTURE */
+ret = vb2_queue_init(ctx-vq_dst, s5p_mfc_qops,
+ dev-alloc_ctx[0], dev-irqlock,
+ V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE, ctx);
+if (ret) {
+mfc_err(Failed to initialize videobuf2 queue (capture)\n);
+goto out_open_3;
+}
+/* Init videobuf2 queue for OUTPUT */
+ret = vb2_queue_init(ctx-vq_src, s5p_mfc_qops,
+ dev-alloc_ctx[1], dev-irqlock,
+ V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE, ctx);
+if (ret) {
+mfc_err(Failed to initialize videobuf2 queue (output)\n);
+goto out_open_3;
+}
About using dev-irqlock, (spinlock_t *drv_lock) in the vb2_queue_init() is
removed
according to the Marek's patch (from the Hans Verkuil request)
: http://www.spinics.net/lists/linux-media/msg23902.html
Does it mean that we don't have to use drv_lock in the driver ?
What actually purpose of drv_lock that you used in the MFC driver ?
I'll remove this when videobuf2 will be merged. The version posted by Marek
is still not final, so it is better to wait until a final version is worked
out in the community.
+vb2_set_alloc_ctx(ctx-vq_dst, dev-alloc_ctx[1], 1);
+init_waitqueue_head(ctx-queue);
+mutex_unlock(dev-mfc_mutex);
+mfc_debug(s5p_mfc_open--\n);
+return ret;
+/* Deinit when failure occured */
+out_open_3:
+if (atomic_read(dev-num_inst) == 1) {
+clk_disable(dev-clock1);
[...]
+mfc_debug(s5p_mfc_alloc_dec_temp_buffers++\n);
+mfc_ctx-desc_phys = cma_alloc(mfc_ctx-dev-v4l2_dev.dev,
+MFC_CMA_BANK1, DESC_BUF_SIZE, 2048);
+if (IS_ERR_VALUE(mfc_ctx-desc_phys)) {
+mfc_ctx-desc_phys = 0;
+mfc_err(Allocating DESC buffer failed.\n);
+return -ENOMEM;
+}
+desc_virt = ioremap_nocache(mfc_ctx-desc_phys, DESC_BUF_SIZE);
In case of ioremap_xx() function, you might meet that msg as below
Don't allow RAM to be mapped - this causes problems with ARMv6+
(arch/arm/mm/ioremap.c). so we should not use this function for this case.
I suggest that you use phys_to_virt() with some cache op. such as
cache_clean for writing case(ex memset) cache_invalidate for reading
case.
(exreading shared mem)
It is necessary
RE: [PATCH 3/4] MFC: Add MFC 5.1 V4L2 driver
Thank you for your reply about my comments.
Refer to as below.
Hi,
I don't know if this is necessary. MFC_NUM_CONTEXTS can be fixed at
the maximum number allowed by MFC hw: 16. I highly doubt someone will
open that many contexts. Increasing this number will not significantly
increase storage space used by MFC if no contexts are used. It will
only change size of one pointer array ( struct s5p_mfc_ctx
*ctx[MFC_NUM_CONTEXTS]; ).
In many project, user can open many contexts according to the scenario
of not only phone but also netbook, tablet. 'cause MFC supports multiple
instance. That's why I suggested this param. be configurable.
I see no problem with setting it to the maximum number allowed by MFC - 16.
There would be no use of setting a smaller value. Except a _minimal_ saving
of memory.
+
+/* Check whether a context should be run on hardware */ int
+s5p_mfc_ctx_ready(struct s5p_mfc_ctx *ctx) {
+mfc_debug(s5p_mfc_ctx_ready: src=%d, dst=%d, state=%d\n,
+ ctx-src_queue_cnt, ctx-dst_queue_cnt,
ctx-state);
+/* Context is to parse header */
+if (ctx-src_queue_cnt = 1 ctx-state ==
MFCINST_DEC_GOT_INST)
+return 1;
+/* Context is to decode a frame */
+if (ctx-src_queue_cnt = 1 ctx-state ==
MFCINST_DEC_RUNNING
+ctx-dst_queue_cnt = ctx-
--
Kamil Debski
Linux Platform Group
Samsung Poland RD Center
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 3/4] MFC: Add MFC 5.1 V4L2 driver
Hi, Kamil
This is third feedback about watchdog timer.
(s5p_mfc.c)
Hi, Peter
Thanks for pointing that out, enabling and disabling watchdog in
open/release is reasonable.
[...]
+platform_set_drvdata(pdev, dev);
+dev-hw_lock = 0;
+dev-watchdog_workqueue = create_singlethread_workqueue(s5p-mfc);
+INIT_WORK(dev-watchdog_work, s5p_mfc_watchdog_worker);
+atomic_set(dev-watchdog_cnt, 0);
+init_timer(dev-watchdog_timer);
+dev-watchdog_timer.data = 0;
+dev-watchdog_timer.function = s5p_mfc_watchdog;
+dev-watchdog_timer.expires = jiffies +
+ msecs_to_jiffies(MFC_WATCHDOG_INTERVAL);
+add_timer(dev-watchdog_timer);
Watch_dog single thread runs right after probing MFC, but this doesn't look
like
nice way in terms of purpose of this timer which is for error handling in
the
middle of decoding. What about moving point running this timer to the
open().
And it should be stopped in release time. Of course, dev-num_inst should
be
considered.
Yes, I agree. I've changed that.
+
+dev-alloc_ctx = vb2_cma_init_multi(pdev-dev, 2, s5p_mem_types,
+s5p_mem_alignments);
+if (IS_ERR(dev-alloc_ctx)) {
+mfc_err(Couldn't prepare allocator context.\n);
+ret = PTR_ERR(dev-alloc_ctx);
+goto alloc_ctx_fail;
+}
+
+ret = video_register_device(vfd, VFL_TYPE_GRABBER, 0);
+if (ret) {
+v4l2_err(dev-v4l2_dev, Failed to register video
device\n);
+video_device_release(vfd);
+goto rel_vdev;
+}
--
Kamil Debski
Linux Platform Group
Samsung Poland RD Center
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 3/4] MFC: Add MFC 5.1 V4L2 driver
I commented as belows,
And you missed one important things 'cause there were my comments
in the very long email which is strongly fixed in the reset seq.
[]
+#define READL(offset)readl(dev-regs_base + (offset))
+#define WRITEL(data, offset) writel((data), dev-regs_base + (offset))
+#define OFFSETA(x) (((x) - dev-port_a) 11)
+#define OFFSETB(x) (((x) - dev-port_b) 11)
+
+/* Reset the device */
+static int s5p_mfc_cmd_reset(struct s5p_mfc_dev *dev)
+{
+ unsigned int mc_status;
+ unsigned long timeout;
+ mfc_debug(s5p_mfc_cmd_reset++\n);
+ /* Stop procedure */
+ WRITEL(0x3f7, S5P_FIMV_SW_RESET); /* reset VI */
Ahm, This (WRITEL(0x3f7, S5P_FIMV_SW_RESET)) might be a problem.
In the reset seq. of MFC driver, we checked out
That FW(s5pc110-mfc.fw in the s5p_mfc_load_firmware()) runned by RISC core
at this point could access invalid address. It should be removed.
Hi, Kamil
This is second feedback about the HW op related code.
(s5p_mfc_opr.c s5p_mfc.c)
Hi, Peter
Thanks for the comments. I have replied to them below. I would be grateful
if you could cut out non relevant parts of the code in your replies. It
was
difficult to find your comments in such a big email. I hope I have found
all
of them.
[...]
+ s5p_mfc_set_dec_stream_buffer(ctx, \
+vb2_plane_paddr(temp_vb, 0), 0, \
+temp_vb-v4l2_planes[0].bytesused);
+ dev-curr_ctx = ctx-num;
+ s5p_mfc_clean_ctx_int_flags(ctx);
+ s5p_mfc_decode_one_frame(ctx,
+ temp_vb-v4l2_planes[0].bytesused == 0);
+ } else if (ctx-state == MFCINST_DEC_INIT) {
+ /* Preparing decoding - getting instance number */
+ mfc_debug(Getting instance number\n);
+ dev-curr_ctx = ctx-num;
+ s5p_mfc_clean_ctx_int_flags(ctx);
+/*s5p_mfc_set_dec_temp_buffers(ctx);
+ *Removed per request by Peter, check if MFC
works OK
*/
Yes, you don't have to set s5p_mfc_set_dec_temp_buffers(ctx)at this point
'cause this is only required before parsing header of the stream except
for
setting shared memory,
so, I think, separating 'setting S5P_FIMV_SI_CH0_HOST_WR_ADR' from the
s5p_mfc_set_dec_temp_buffers() is needed. So I mean
remove 'setting S5P_FIMV_SI_CH0_HOST_WR_ADR' from
s5p_mfc_set_dec_temp_buffers(ctx);, then add 'setting
S5P_FIMV_SI_CH0_HOST_WR_ADR'
in the 3 functions (s5p_mfc_set_dec_frame_buffer(),s5p_mfc_init_decode(),
s5p_mfc_decode_one_frame())
I also commented suggested loc. of 'setting S5P_FIMV_SI_CH0_HOST_WR_ADR'
Right, I've changed this.
+ ret = s5p_mfc_open_inst(ctx);
+ if (ret) {
+ mfc_err(Failed to create a new
instance.\n);
+ ctx-state = MFCINST_DEC_ERROR;
+ }
+ } else if (ctx-state == MFCINST_DEC_RETURN_INST) {
+ /* Closing decoding instance */
+ mfc_debug(Returning instance number\n);
+ dev-curr_ctx = ctx-num;
+ s5p_mfc_clean_ctx_int_flags(ctx);
+ ret = s5p_mfc_return_inst_no(ctx);
+ if (ret) {
+ mfc_err(Failed to return an instance.\n);
+ ctx-state = MFCINST_DEC_ERROR;
+ }
[...]
+ }
+ /* Init videobuf2 queue for CAPTURE */
+ ret = vb2_queue_init(ctx-vq_dst, s5p_mfc_qops,
+ dev-alloc_ctx[0], dev-irqlock,
+ V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE, ctx);
+ if (ret) {
+ mfc_err(Failed to initialize videobuf2 queue (capture)\n);
+ goto out_open_3;
+ }
+ /* Init videobuf2 queue for OUTPUT */
+ ret = vb2_queue_init(ctx-vq_src, s5p_mfc_qops,
+ dev-alloc_ctx[1], dev-irqlock,
+ V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE, ctx);
+ if (ret) {
+ mfc_err(Failed to initialize videobuf2 queue (output)\n);
+ goto out_open_3;
+ }
About using dev-irqlock, (spinlock_t *drv_lock) in the vb2_queue_init()
is
removed
according to the Marek's patch (from the Hans Verkuil request)
: http://www.spinics.net/lists/linux-media/msg23902.html
Does it mean that we don't have to use drv_lock in the driver ?
What actually purpose of drv_lock that you used in the MFC driver ?
I'll remove this when videobuf2 will be merged. The version posted by
Marek
is still not final, so it is better to wait until a final version is
worked
out in the community.
OK, I will wait until final version is released.
+ vb2_set_alloc_ctx(ctx-vq_dst, dev-alloc_ctx[1], 1);
+ init_waitqueue_head(ctx-queue);
+
RE: [PATCH 3/4] MFC: Add MFC 5.1 V4L2 driver
Hi, Peter. I have some first comments about this code. and Generally, Making patch set separately will be more helpful to everyone. Thank you for your comments. k.deb...@samsung.com wrote: Multi Format Codec 5.1 is a module available on S5PC110 and S5PC210 Samsung SoCs. Hardware is capable of handling a range of video codecs and this driver provides V4L2 interface for video decoding. Signed-off-by: Kamil Debski k.deb...@samsung.com Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com [...] diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig index 6d0bd36..1d0b91e 100644 --- a/drivers/media/video/Kconfig +++ b/drivers/media/video/Kconfig @@ -1047,4 +1047,12 @@ config VIDEO_SAMSUNG_S5P_FIMC This is a v4l2 driver for the S5P camera interface (video postprocessor) +config VIDEO_SAMSUNG_S5P_MFC + tristate Samsung S5P MFC 5.0 Video Codec + depends on VIDEO_V4L2 CMA + select VIDEOBUF2_CMA + default n + help + MFC 5.0 driver for V4L2. + endif # V4L_MEM2MEM_DRIVERS What about unifying MFC version as a MFC 5.1, because we are using MFC HW ver.(MFC 5.1.x) in the C110/C210 chip. Right, will fix this. [...] diff --git a/drivers/media/video/s5p-mfc/regs-mfc5.h b/drivers/media/video/s5p-mfc/regs-mfc5.h new file mode 100644 index 000..8c628ad --- /dev/null +++ b/drivers/media/video/s5p-mfc/regs-mfc5.h @@ -0,0 +1,305 @@ +/* + * + * Register definition file for Samsung MFC V4.0 V5.0 Interface (FIMV) driver + * + * 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. +*/ Incorrect comment in the header, MFC V4.0 is not included Right again. + +#ifndef _REGS_FIMV_H +#define _REGS_FIMV_H + +#define S5P_FIMV_REG_SIZE (S5P_FIMV_END_ADDR - S5P_FIMV_START_ADDR) +#define S5P_FIMV_REG_COUNT ((S5P_FIMV_END_ADDR - S5P_FIMV_START_ADDR) / 4) + +#define S5P_FIMV_START_ADDR0x +#define S5P_FIMV_END_ADDR 0xe008 + +#define S5P_FIMV_SW_RESET 0x +#define S5P_FIMV_RISC_HOST_INT 0x0008 +/* Command from HOST to RISC */ +#define S5P_FIMV_HOST2RISC_CMD 0x0030 +#define S5P_FIMV_HOST2RISC_ARG10x0034 +#define S5P_FIMV_HOST2RISC_ARG20x0038 +#define S5P_FIMV_HOST2RISC_ARG30x003c +#define S5P_FIMV_HOST2RISC_ARG40x0040 +/* Command from RISC to HOST */ +#define S5P_FIMV_RISC2HOST_CMD 0x0044 +#define S5P_FIMV_RISC2HOST_ARG10x0048 +#define S5P_FIMV_RISC2HOST_ARG20x004c +#define S5P_FIMV_RISC2HOST_ARG30x0050 +#define S5P_FIMV_RISC2HOST_ARG40x0054 + +#define S5P_FIMV_FW_VERSION0x0058 +#define S5P_FIMV_SYS_MEM_SZ0x005c +#define S5P_FIMV_FW_STATUS 0x0080 +/* Memory controller register */ +#define S5P_FIMV_MC_DRAMBASE_ADR_A 0x0508 +#define S5P_FIMV_MC_DRAMBASE_ADR_B 0x050c +#define S5P_FIMV_MC_STATUS 0x0510 + +/* In case of 2 master */ This comment(In case of 2 master) is meaningless. it was used at the beginning step of development. Will fix this too. +/* Common register */ +#define S5P_FIMV_SYS_MEM_ADR 0x0600 /* firmware buffer */ +#define S5P_FIMV_CPB_BUF_ADR 0x0604 /* stream buffer */ +#define S5P_FIMV_DESC_BUF_ADR 0x0608 /* descriptor buffer */ +/* H264 decoding */ +#define S5P_FIMV_VERT_NB_MV_ADR0x068c /* vertical neighbor motion vector */ +#define S5P_FIMV_VERT_NB_IP_ADR0x0690 /* neighbor pixels for intra [...] displayed pic */ +#define S5P_FIMV_SI_DISPLAY_C_ADR 0x2014 /* chroma address of displayed pic */ +#define S5P_FIMV_SI_DEC_FRM_SIZE 0x2018 /* the number of frames decoded */ S5P_FIMV_SI_DEC_FRM_SIZE does actually means consumed number of bytes to decode a frame Ok, I've changed the name of the define to reflect the purpose of the register. +#define S5P_FIMV_SI_DISPLAY_STATUS 0x201c /* status of decoded picture */ +#define S5P_FIMV_SI_FRAME_TYPE 0x2020 /* frame type such as skip/I/P/B */ + +#define S5P_FIMV_SI_CH0_SB_ST_ADR 0x2044 /* start addr of stream buf */ +#define S5P_FIMV_SI_CH0_SB_FRM_SIZE0x2048 /* size of stream buf */ +#define S5P_FIMV_SI_CH0_DESC_ADR 0x204c /* addr of descriptor buf */ +/* Encoder for MPEG4 */ +#define S5P_FIMV_ENC_MPEG4_QUART_PXL 0xe008 /* qpel interpolation ctrl */ + +/* Additional */ +#define S5P_FIMV_SI_CH0_DPB_CONF_CTRL 0x2068 /* DPB Config Control Register */ +#define S5P_FIMV_SI_CH0_RELEASE_BUF 0x2060 /* DPB release buffer register */ +#define S5P_FIMV_SI_CH0_HOST_WR_ADR0x2064 S5P_FIMV_SI_CH0_HOST_WR_ADR means 'address of shared memory' if comments is needed Added the comment. [...] diff --git
RE: [PATCH 3/4] MFC: Add MFC 5.1 V4L2 driver
Thank you for your reply about my comments. Refer to as below. k.deb...@samsung.com wrote: Hi, Peter. I have some first comments about this code. and Generally, Making patch set separately will be more helpful to everyone. Thank you for your comments. k.deb...@samsung.com wrote: Multi Format Codec 5.1 is a module available on S5PC110 and S5PC210 Samsung SoCs. Hardware is capable of handling a range of video codecs and this driver provides V4L2 interface for video decoding. Signed-off-by: Kamil Debski k.deb...@samsung.com Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com [...] diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig index 6d0bd36..1d0b91e 100644 --- a/drivers/media/video/Kconfig +++ b/drivers/media/video/Kconfig @@ -1047,4 +1047,12 @@ config VIDEO_SAMSUNG_S5P_FIMC This is a v4l2 driver for the S5P camera interface (video postprocessor) +config VIDEO_SAMSUNG_S5P_MFC + tristate Samsung S5P MFC 5.0 Video Codec + depends on VIDEO_V4L2 CMA + select VIDEOBUF2_CMA + default n + help + MFC 5.0 driver for V4L2. + endif # V4L_MEM2MEM_DRIVERS What about unifying MFC version as a MFC 5.1, because we are using MFC HW ver.(MFC 5.1.x) in the C110/C210 chip. Right, will fix this. [...] diff --git a/drivers/media/video/s5p-mfc/regs-mfc5.h b/drivers/media/video/s5p-mfc/regs-mfc5.h new file mode 100644 index 000..8c628ad --- /dev/null +++ b/drivers/media/video/s5p-mfc/regs-mfc5.h @@ -0,0 +1,305 @@ +/* + * + * Register definition file for Samsung MFC V4.0 V5.0 Interface (FIMV) driver + * + * 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. +*/ Incorrect comment in the header, MFC V4.0 is not included Right again. + +#ifndef _REGS_FIMV_H +#define _REGS_FIMV_H + +#define S5P_FIMV_REG_SIZE(S5P_FIMV_END_ADDR - S5P_FIMV_START_ADDR) +#define S5P_FIMV_REG_COUNT ((S5P_FIMV_END_ADDR - S5P_FIMV_START_ADDR) / 4) + +#define S5P_FIMV_START_ADDR 0x +#define S5P_FIMV_END_ADDR0xe008 + +#define S5P_FIMV_SW_RESET0x +#define S5P_FIMV_RISC_HOST_INT 0x0008 +/* Command from HOST to RISC */ +#define S5P_FIMV_HOST2RISC_CMD 0x0030 +#define S5P_FIMV_HOST2RISC_ARG1 0x0034 +#define S5P_FIMV_HOST2RISC_ARG2 0x0038 +#define S5P_FIMV_HOST2RISC_ARG3 0x003c +#define S5P_FIMV_HOST2RISC_ARG4 0x0040 +/* Command from RISC to HOST */ +#define S5P_FIMV_RISC2HOST_CMD 0x0044 +#define S5P_FIMV_RISC2HOST_ARG1 0x0048 +#define S5P_FIMV_RISC2HOST_ARG2 0x004c +#define S5P_FIMV_RISC2HOST_ARG3 0x0050 +#define S5P_FIMV_RISC2HOST_ARG4 0x0054 + +#define S5P_FIMV_FW_VERSION 0x0058 +#define S5P_FIMV_SYS_MEM_SZ 0x005c +#define S5P_FIMV_FW_STATUS 0x0080 +/* Memory controller register */ +#define S5P_FIMV_MC_DRAMBASE_ADR_A 0x0508 +#define S5P_FIMV_MC_DRAMBASE_ADR_B 0x050c +#define S5P_FIMV_MC_STATUS 0x0510 + +/* In case of 2 master */ This comment(In case of 2 master) is meaningless. it was used at the beginning step of development. Will fix this too. +/* Common register */ +#define S5P_FIMV_SYS_MEM_ADR 0x0600 /* firmware buffer */ +#define S5P_FIMV_CPB_BUF_ADR 0x0604 /* stream buffer */ +#define S5P_FIMV_DESC_BUF_ADR0x0608 /* descriptor buffer */ +/* H264 decoding */ +#define S5P_FIMV_VERT_NB_MV_ADR 0x068c /* vertical neighbor motion vector */ +#define S5P_FIMV_VERT_NB_IP_ADR 0x0690 /* neighbor pixels for intra [...] displayed pic */ +#define S5P_FIMV_SI_DISPLAY_C_ADR 0x2014 /* chroma address of displayed pic */ +#define S5P_FIMV_SI_DEC_FRM_SIZE 0x2018 /* the number of frames decoded */ S5P_FIMV_SI_DEC_FRM_SIZE does actually means consumed number of bytes to decode a frame Ok, I've changed the name of the define to reflect the purpose of the register. +#define S5P_FIMV_SI_DISPLAY_STATUS 0x201c /* status of decoded picture */ +#define S5P_FIMV_SI_FRAME_TYPE 0x2020 /* frame type such as skip/I/P/B */ + +#define S5P_FIMV_SI_CH0_SB_ST_ADR0x2044 /* start addr of stream buf */ +#define S5P_FIMV_SI_CH0_SB_FRM_SIZE 0x2048 /* size of stream buf */ +#define S5P_FIMV_SI_CH0_DESC_ADR 0x204c /* addr of descriptor buf */ +/* Encoder for MPEG4 */ +#define S5P_FIMV_ENC_MPEG4_QUART_PXL 0xe008 /* qpel interpolation ctrl */ + +/* Additional */ +#define S5P_FIMV_SI_CH0_DPB_CONF_CTRL 0x2068 /* DPB Config Control Register */ +#define S5P_FIMV_SI_CH0_RELEASE_BUF 0x2060
