RE: [PATCH v2 2/2] [media] s5p-mfc: update MFC v4l2 driver to support MFC6.x

Arun Kumar K
2012-07-12T12:55:01Z

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@samsung.com> wrote:
>
> Hi Arun,
>
> Please find some additional comments below.
>
> > From: Arun Kumar K [mailto:arun.kk@samsung.com]
> > 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