Hi Kamil, Thank you for the review. Please find my comments inline. On Tue, Jul 10, 2012 at 8:40 PM, Kamil Debski <k.debski@xxxxxxxxxxx> wrote: > > Hi Arun, > > Please find some additional comments below. > > > From: Arun Kumar K [mailto:arun.kk@xxxxxxxxxxx] > > Sent: 06 July 2012 16:00 > > [snip] > > > > diff --git a/drivers/media/video/s5p-mfc/Makefile > > b/drivers/media/video/s5p-mfc/Makefile > > index d066340..0308d74 100644 > > --- a/drivers/media/video/s5p-mfc/Makefile > > +++ b/drivers/media/video/s5p-mfc/Makefile > > @@ -1,5 +1,6 @@ > > obj-$(CONFIG_VIDEO_SAMSUNG_S5P_MFC) := s5p-mfc.o > > -s5p-mfc-y += s5p_mfc.o s5p_mfc_intr.o s5p_mfc_opr.o > > +s5p-mfc-y += s5p_mfc.o s5p_mfc_intr.o > > s5p-mfc-y += s5p_mfc_dec.o s5p_mfc_enc.o > > -s5p-mfc-y += s5p_mfc_ctrl.o s5p_mfc_cmd.o > > -s5p-mfc-y += s5p_mfc_pm.o s5p_mfc_shm.o > > +s5p-mfc-y += s5p_mfc_ctrl.o s5p_mfc_pm.o > > +obj-$(CONFIG_VIDEO_SAMSUNG_S5P_MFC_V5) += s5p_mfc_opr.o s5p_mfc_cmd.o > > s5p_mfc_shm.o > > +obj-$(CONFIG_VIDEO_SAMSUNG_S5P_MFC_V6) += s5p_mfc_opr_v6.o > > s5p_mfc_cmd_v6.o > > diff --git a/drivers/media/video/s5p-mfc/regs-mfc-v6.h > > b/drivers/media/video/s5p-mfc/regs-mfc-v6.h > > new file mode 100644 > > index 0000000..f22a159 > > This Makefile does not work when compiling the driver as a module. > (I also wrote about this in my previous email) > Yes. I will fix it. > [snip] > > > > > #endif /* _REGS_FIMV_H */ > > diff --git a/drivers/media/video/s5p-mfc/s5p_mfc.c > > b/drivers/media/video/s5p-mfc/s5p_mfc.c > > index 9bb68e7..bec94bc 100644 > > --- a/drivers/media/video/s5p-mfc/s5p_mfc.c > > +++ b/drivers/media/video/s5p-mfc/s5p_mfc.c > > [snip] > > > @@ -285,12 +276,13 @@ static void s5p_mfc_handle_frame(struct > > s5p_mfc_ctx > > *ctx, > > > > dst_frame_status = s5p_mfc_get_dspl_status() > > & > > S5P_FIMV_DEC_STATUS_DECODING_STATUS_MASK; > > - res_change = s5p_mfc_get_dspl_status() > > - & S5P_FIMV_DEC_STATUS_RESOLUTION_MASK; > > + res_change = (s5p_mfc_get_dspl_status() > > + & S5P_FIMV_DEC_STATUS_RESOLUTION_MASK) > > + >> S5P_FIMV_DEC_STATUS_RESOLUTION_SHIFT; > > mfc_debug(2, "Frame Status: %x\n", dst_frame_status); > > if (ctx->state == MFCINST_RES_CHANGE_INIT) > > ctx->state = MFCINST_RES_CHANGE_FLUSH; > > - if (res_change) { > > + if (res_change && res_change != 3) { > > Maybe > If (res_change == 1 || res_change == 2) { > would be better, at least it would be more clear. > Sure I will change it that way. > [snip] > > > > diff --git a/drivers/media/video/s5p-mfc/s5p_mfc_common.h > > b/drivers/media/video/s5p-mfc/s5p_mfc_common.h > > index bd5706a..8c646f4 100644 > > --- a/drivers/media/video/s5p-mfc/s5p_mfc_common.h > > +++ b/drivers/media/video/s5p-mfc/s5p_mfc_common.h > > [snip] > > > @@ -499,37 +563,42 @@ struct s5p_mfc_ctx { > > int display_delay; > > int display_delay_enable; > > int after_packed_pb; > > + int sei_fp_parse; > > > > int dpb_count; > > int total_dpb_count; > > > > /* Buffers */ > > - void *ctx_buf; > > - size_t ctx_phys; > > - size_t ctx_ofs; > > - size_t ctx_size; > > - > > - void *desc_buf; > > - size_t desc_phys; > > - > > - > > - void *shm_alloc; > > - void *shm; > > - size_t shm_ofs; > > + unsigned int ctx_size; > > + struct s5p_mfc_priv_buf ctx; > > + struct s5p_mfc_priv_buf dsc; > > + struct s5p_mfc_priv_buf shm; > > I think that ctx_size could be integrated in struct s5p_mfc_priv_buf. > Also - why unsigned int, where in other places you use size_t for size? > I think it should be consistent. I would choose size_t. > Yes. size_t is better. I will change it. > > > > struct s5p_mfc_enc_params enc_params; > > > > size_t enc_dst_buf_size; > > + size_t luma_dpb_size; > > + size_t chroma_dpb_size; > > + size_t me_buffer_size; > > + size_t tmv_buffer_size; > > > > ^^ You use size_t here. > > [snip] > > > diff --git a/drivers/media/video/s5p-mfc/s5p_mfc_ctrl.c > > b/drivers/media/video/s5p-mfc/s5p_mfc_ctrl.c > > index 08a5cfe..65ff15d 100644 > > --- a/drivers/media/video/s5p-mfc/s5p_mfc_ctrl.c > > +++ b/drivers/media/video/s5p-mfc/s5p_mfc_ctrl.c > > @@ -15,7 +15,6 @@ > > #include <linux/firmware.h> > > #include <linux/jiffies.h> > > #include <linux/sched.h> > > -#include "regs-mfc.h" > > #include "s5p_mfc_cmd.h" > > #include "s5p_mfc_common.h" > > #include "s5p_mfc_debug.h" > > @@ -38,12 +37,12 @@ int s5p_mfc_alloc_and_load_firmware(struct > > s5p_mfc_dev *dev) > > * into kernel. */ > > mfc_debug_enter(); > > err = request_firmware((const struct firmware **)&fw_blob, > > - "s5p-mfc.fw", dev->v4l2_dev.dev); > > + "mfc_fw.bin", dev->v4l2_dev.dev); > > Another name change? This is getting ridiculous. Nein, nein, nein ! ;) > If you _*really*_ need such a change then go ahead and try to convince > me, but I tell you - it's going to be hard. > Ok. I will retain the old name :) And as discussed will add new name s5p-mfc-v6.fw for V6. > > if (err != 0) { > > mfc_err("Firmware is not present in the /lib/firmware > > directory nor compiled in kernel\n"); > > return -EINVAL; > > } > > - dev->fw_size = ALIGN(fw_blob->size, FIRMWARE_ALIGN); > > + dev->fw_size = dev->variant->buf_size->fw; > > Why is size taken from there instead of the size of the firmware file? > Even if there was some point to do it this way then you really *should* > check if the firmware read from the file fits in the allocated buffer. > > This is a straight way to a buffer overflow error. All I need is to > prepare an extra big firmware file and "viola!" we have a buffer overflow > that could be fatal to the system. Yes. Will fix it. > > > if (s5p_mfc_bitproc_buf) { > > mfc_err("Attempting to allocate firmware when it seems > > that > > it is already loaded\n"); > > release_firmware(fw_blob); > > @@ -77,28 +76,33 @@ int s5p_mfc_alloc_and_load_firmware(struct > > s5p_mfc_dev *dev) > > return -EIO; > > } > > dev->bank1 = s5p_mfc_bitproc_phys; > > - b_base = vb2_dma_contig_memops.alloc( > > - dev->alloc_ctx[MFC_BANK2_ALLOC_CTX], 1 << > > MFC_BANK2_ALIGN_ORDER); > > - if (IS_ERR(b_base)) { > > - vb2_dma_contig_memops.put(s5p_mfc_bitproc_buf); > > - s5p_mfc_bitproc_phys = 0; > > - s5p_mfc_bitproc_buf = NULL; > > - mfc_err("Allocating bank2 base failed\n"); > > - release_firmware(fw_blob); > > - return -ENOMEM; > > - } > > - bank2_base_phys = s5p_mfc_mem_cookie( > > - dev->alloc_ctx[MFC_BANK2_ALLOC_CTX], b_base); > > - vb2_dma_contig_memops.put(b_base); > > - if (bank2_base_phys & ((1 << MFC_BASE_ALIGN_ORDER) - 1)) { > > - mfc_err("The base memory for bank 2 is not aligned to > > 128KB\n"); > > - vb2_dma_contig_memops.put(s5p_mfc_bitproc_buf); > > - s5p_mfc_bitproc_phys = 0; > > - s5p_mfc_bitproc_buf = NULL; > > - release_firmware(fw_blob); > > - return -EIO; > > + if (HAS_PORTNUM(dev) && IS_TWOPORT(dev)) { > > + b_base = vb2_dma_contig_memops.alloc( > > + dev->alloc_ctx[MFC_BANK2_ALLOC_CTX], > > + 1 << MFC_BANK2_ALIGN_ORDER); > > + if (IS_ERR(b_base)) { > > + vb2_dma_contig_memops.put(s5p_mfc_bitproc_buf); > > + s5p_mfc_bitproc_phys = 0; > > + s5p_mfc_bitproc_buf = 0; > > + mfc_err("Allocating bank2 base failed\n"); > > + release_firmware(fw_blob); > > + return -ENOMEM; > > + } > > + bank2_base_phys = s5p_mfc_mem_cookie( > > + dev->alloc_ctx[MFC_BANK2_ALLOC_CTX], b_base); > > + vb2_dma_contig_memops.put(b_base); > > + if (bank2_base_phys & ((1 << MFC_BASE_ALIGN_ORDER) - 1)) { > > + mfc_err("The base memory for bank 2 is not aligned > > to > > 128KB\n"); > > + vb2_dma_contig_memops.put(s5p_mfc_bitproc_buf); > > + s5p_mfc_bitproc_phys = 0; > > + s5p_mfc_bitproc_buf = 0; > > + release_firmware(fw_blob); > > + return -EIO; > > + } > > + dev->bank2 = bank2_base_phys; > > + } else { > > + dev->bank2 = dev->bank1; > > } > > - dev->bank2 = bank2_base_phys; > > memcpy(s5p_mfc_bitproc_virt, fw_blob->data, fw_blob->size); > > wmb(); > > release_firmware(fw_blob); > > @@ -116,7 +120,7 @@ int s5p_mfc_reload_firmware(struct s5p_mfc_dev *dev) > > * into kernel. */ > > mfc_debug_enter(); > > err = request_firmware((const struct firmware **)&fw_blob, > > - "s5p-mfc.fw", dev->v4l2_dev.dev); > > + "mfc_fw.bin", dev->v4l2_dev.dev); > > Ditto. > > [snip] > > > > diff --git a/drivers/media/video/s5p-mfc/s5p_mfc_ctrl.h > > b/drivers/media/video/s5p-mfc/s5p_mfc_ctrl.h > > index 61dc23b..b72c8c6 100644 > > --- a/drivers/media/video/s5p-mfc/s5p_mfc_ctrl.h > > +++ b/drivers/media/video/s5p-mfc/s5p_mfc_ctrl.h > > [snip] > > > > > @@ -336,21 +364,35 @@ static int vidioc_g_fmt(struct file *file, void > > *priv, struct v4l2_format *f) > > /* Try format */ > > static int vidioc_try_fmt(struct file *file, void *priv, struct > > v4l2_format *f) > > { > > + struct s5p_mfc_dev *dev = video_drvdata(file); > > struct s5p_mfc_fmt *fmt; > > > > - if (f->type != V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) { > > - mfc_err("This node supports decoding only\n"); > > - return -EINVAL; > > - } > > - fmt = find_format(f, MFC_FMT_DEC); > > - if (!fmt) { > > - mfc_err("Unsupported format\n"); > > - return -EINVAL; > > - } > > - if (fmt->type != MFC_FMT_DEC) { > > - mfc_err("\n"); > > - return -EINVAL; > > + mfc_debug(2, "Type is %d\n", f->type); > > + if (f->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) { > > + fmt = find_format(f, MFC_FMT_DEC); > > + if (!fmt) { > > + mfc_err("Unsupported format for source.\n"); > > + return -EINVAL; > > + } > > + if (!IS_MFCV6(dev) && (fmt->fourcc == V4L2_PIX_FMT_VP8)) { > > + mfc_err("Not supported format.\n"); > > + return -EINVAL; > > + } > > + } else if (f->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) { > > + fmt = find_format(f, MFC_FMT_RAW); > > + if (!fmt) { > > + mfc_err("Unsupported format for destination.\n"); > > + return -EINVAL; > > + } > > + if (IS_MFCV6(dev) && (fmt->fourcc == V4L2_PIX_FMT_NV12MT)) > > { > > Ok. Let's see. > If IS_MFCV6(dev) is 1 and > fmt->fourcc == V4L2_PIX_FMT_NV12MT is 0 > then the following code is not run. > > > + mfc_err("Not supported format.\n"); > > + return -EINVAL; > > And we get here. > > > + } else if (fmt->fourcc != V4L2_PIX_FMT_NV12MT) { > > fmt->fourcc == V4L2_PIX_FMT_NV12MT is still 0, so > fmt->fourcc != V4L2_PIX_FMT_NV12MT is 1 and this code is run > > > + mfc_err("Not supported format.\n"); > > + return -EINVAL; > > + } > > } > > + > > return 0; > > } > > My question is - what targets did you run this code on? Did you run it > on Exynos5 with MFC v6? Arun, it is important that you test the patches > that you send to the mailing list. Maybe you have sent different files > than the ones you have tested? > Yes this is a bug in the code. The patch was tested well on Exynos5 with MFCv6 using the v4l example test app. Unfortuately the test app doesnt call s_fmt on CAPTURE_MPLANE and this issue never surfaced. > [snip] > > diff --git a/drivers/media/video/s5p-mfc/s5p_mfc_enc.c > b/drivers/media/video/s5p-mfc/s5p_mfc_enc.c > index 03d8334..645a8ef 100644 > --- a/drivers/media/video/s5p-mfc/s5p_mfc_enc.c > +++ b/drivers/media/video/s5p-mfc/s5p_mfc_enc.c > @@ -24,48 +24,63 @@ > #include <linux/workqueue.h> > #include <media/v4l2-ctrls.h> > #include <media/videobuf2-core.h> > -#include "regs-mfc.h" > #include "s5p_mfc_common.h" > #include "s5p_mfc_debug.h" > #include "s5p_mfc_enc.h" > #include "s5p_mfc_intr.h" > -#include "s5p_mfc_opr.h" > + > +#define DEF_SRC_FMT 2 > +#define DEF_DST_FMT 4 > > I would add ENC/DEC to the name of this define as it has confused me > because there are two symbols with the same name in different files. > Same applies to the s5p_mfc_dec.c file. > Ok. Will change it. > [snip] > > diff --git a/drivers/media/video/s5p-mfc/s5p_mfc_enc.h > b/drivers/media/video/s5p-mfc/s5p_mfc_enc.h > index 405bdd3..413f22f 100644 > --- a/drivers/media/video/s5p-mfc/s5p_mfc_enc.h > +++ b/drivers/media/video/s5p-mfc/s5p_mfc_enc.h > @@ -19,5 +19,6 @@ const struct v4l2_ioctl_ops > *get_enc_v4l2_ioctl_ops(void); > struct s5p_mfc_fmt *get_enc_def_fmt(bool src); > int s5p_mfc_enc_ctrls_setup(struct s5p_mfc_ctx *ctx); > void s5p_mfc_enc_ctrls_delete(struct s5p_mfc_ctx *ctx); > +void s5p_mfc_enc_init(struct s5p_mfc_ctx *ctx); > > #endif /* S5P_MFC_ENC_H_ */ > diff --git a/drivers/media/video/s5p-mfc/s5p_mfc_intr.c > b/drivers/media/video/s5p-mfc/s5p_mfc_intr.c > index 8f2f8bf..dfdc558 100644 > --- a/drivers/media/video/s5p-mfc/s5p_mfc_intr.c > +++ b/drivers/media/video/s5p-mfc/s5p_mfc_intr.c > @@ -17,7 +17,6 @@ > #include <linux/io.h> > #include <linux/sched.h> > #include <linux/wait.h> > -#include "regs-mfc.h" > #include "s5p_mfc_common.h" > #include "s5p_mfc_debug.h" > #include "s5p_mfc_intr.h" > > > > diff --git a/drivers/media/video/s5p-mfc/s5p_mfc_opr.c > > b/drivers/media/video/s5p-mfc/s5p_mfc_opr.c > > index e6217cb..1fd9c92 100644 > > --- a/drivers/media/video/s5p-mfc/s5p_mfc_opr.c > > +++ b/drivers/media/video/s5p-mfc/s5p_mfc_opr.c > > @@ -12,15 +12,12 @@ > > * published by the Free Software Foundation. > > */ > > > > -#include "regs-mfc.h" > > -#include "s5p_mfc_cmd.h" > > #include "s5p_mfc_common.h" > > +#include "s5p_mfc_cmd.h" > > #include "s5p_mfc_ctrl.h" > > #include "s5p_mfc_debug.h" > > #include "s5p_mfc_intr.h" > > -#include "s5p_mfc_opr.h" > > #include "s5p_mfc_pm.h" > > -#include "s5p_mfc_shm.h" > > #include <asm/cacheflush.h> > > #include <linux/delay.h> > > #include <linux/dma-mapping.h> > > @@ -37,39 +34,31 @@ > > /* Allocate temporary buffers for decoding */ > > int s5p_mfc_alloc_dec_temp_buffers(struct s5p_mfc_ctx *ctx) > > { > > - void *desc_virt; > > struct s5p_mfc_dev *dev = ctx->dev; > > + struct s5p_mfc_buf_size_v5 *buf_size = > > dev->variant->buf_size->priv; > > > > - ctx->desc_buf = vb2_dma_contig_memops.alloc( > > - dev->alloc_ctx[MFC_BANK1_ALLOC_CTX], > > DESC_BUF_SIZE); > > - if (IS_ERR_VALUE((int)ctx->desc_buf)) { > > - ctx->desc_buf = NULL; > > + ctx->dsc.alloc = vb2_dma_contig_memops.alloc( > > + dev->alloc_ctx[MFC_BANK1_ALLOC_CTX], > > + buf_size->dsc); > > + if (IS_ERR_VALUE((int)ctx->dsc.alloc)) { > > + ctx->dsc.alloc = NULL; > > mfc_err("Allocating DESC buffer failed\n"); > > return -ENOMEM; > > } > > - ctx->desc_phys = s5p_mfc_mem_cookie( > > - dev->alloc_ctx[MFC_BANK1_ALLOC_CTX], > > ctx->desc_buf); > > - BUG_ON(ctx->desc_phys & ((1 << MFC_BANK1_ALIGN_ORDER) - 1)); > > - desc_virt = vb2_dma_contig_memops.vaddr(ctx->desc_buf); > > - if (desc_virt == NULL) { > > - vb2_dma_contig_memops.put(ctx->desc_buf); > > - ctx->desc_phys = 0; > > - ctx->desc_buf = NULL; > > - mfc_err("Remapping DESC buffer failed\n"); > > - return -ENOMEM; > > - } > > - memset(desc_virt, 0, DESC_BUF_SIZE); > > - wmb(); > > Why was this removed? > The driver should work with older firmware versions on Exynos4/S5PC110. > I remember that zeroing the desc buffer was (and still is?) mandatory. > This was removed in the original patch itself. I tested it on Exynos4 origen board and it worked without any problems. Anyway memset seems logical and I will revert this change. > [snip] > > > > > @@ -1395,3 +1471,21 @@ void s5p_mfc_cleanup_queue(struct list_head *lh, > > struct vb2_queue *vq) > > } > > } > > > > +void s5p_mfc_clear_int_flags(struct s5p_mfc_dev *dev) > > +{ > > + mfc_write(dev, 0, S5P_FIMV_RISC_HOST_INT); > > + mfc_write(dev, 0, S5P_FIMV_RISC2HOST_CMD); > > + mfc_write(dev, 0xffff, S5P_FIMV_SI_RTN_CHID); > > +} > > + > > +void s5p_mfc_write_info(struct s5p_mfc_ctx *ctx, unsigned int data, > > + unsigned int ofs) > > +{ > > + s5p_mfc_write_shm(ctx, data, ofs); > > +} > > + > > +unsigned int s5p_mfc_read_info(struct s5p_mfc_ctx *ctx, > > + unsigned int ofs) > > +{ > > + return s5p_mfc_read_shm(ctx, ofs); > > About s5p_mfc_*_shm functions. If you are adding this kind of support for > two variants of MFC then maybe it would be a good idea to remove the > s5p_mfc_shm.c and s5p_mfc_shm.h files altogether. All register definitions > are already in the regs-mfc.h and regs-mfc-v6.h files. Instead of calling > s5p_mfc_*_shm from s5p_mfc_*_info it might be better to do the hardware > calls there. Also s5p_mfc_init_shm can be move to > s5p_mfc_alloc_instance_buffer. > Yes right. I will change it. > > +} > > [snip] > > > diff --git a/drivers/media/video/s5p-mfc/s5p_mfc_pm.c > > b/drivers/media/video/s5p-mfc/s5p_mfc_pm.c > > index 738a607..4fa0b54 100644 > > --- a/drivers/media/video/s5p-mfc/s5p_mfc_pm.c > > +++ b/drivers/media/video/s5p-mfc/s5p_mfc_pm.c > > @@ -20,7 +20,11 @@ > > #include "s5p_mfc_debug.h" > > #include "s5p_mfc_pm.h" > > > > -#define MFC_CLKNAME "sclk_mfc" > > +#if defined(CONFIG_VIDEO_SAMSUNG_S5P_MFC_V5) > > +#define MFC_CLKNAME "sclk_mfc" > > +#elif defined(CONFIG_VIDEO_SAMSUNG_S5P_MFC_V6) > > +#define MFC_CLKNAME "aclk_333" > > +#endif > > #define MFC_GATE_CLK_NAME "mfc" > > Maybe it would be better to check the variant during init > than use a define. > Yes. I will change it. Regards Arunÿôèº{.nÇ+‰·Ÿ®‰†+%ŠËÿ±éݶ¥Šwÿº{.nÇ+‰·¥Š{±þg?‰¯âžØ^n‡r¡ö¦zË?ëh™¨èÚ&£ûàz¿äz¹Þ—ú+€Ê+zf£¢·hšˆ§~††Ûiÿÿï?êÿ‘êçz_è®æj:+v‰¨þ)ߣøm
