All of lore.kernel.org
 help / color / mirror / Atom feed
* Re: [PATCH v2 2/2] [media] s5p-mfc: update MFC v4l2 driver to support MFC6.x
@ 2012-07-12 12:43 Arun Kumar K
  0 siblings, 0 replies; 5+ messages in thread
From: Arun Kumar K @ 2012-07-12 12:43 UTC (permalink / raw)
  To: Kyungmin Park
  Cc: linux-media, Jeongtae Park, Jang-Hyuck Kim, peter Oh,
	NAVEEN KRISHNA CHATRADHI, Marek Szyprowski, Kamil Debski,
	Sylwester Nawrocki, hans.verkuil, mchehab

Hi Kyungmin Park,

Thank you for the review.
Please find my comments inline.

On Fri, Jul 6, 2012 at 7:51 PM, Kyungmin Park <kmpark@infradead.org> wrote:
> Hi,
>
> On Fri, Jul 6, 2012 at 11:00 PM, Arun Kumar K <arun.kk@samsung.com> wrote:
>> From: Jeongtae Park <jtp.park@samsung.com>
>>
>> Multi Format Codec 6.x is a hardware video coding acceleration
>> module fount in new Exynos5 SoC series.
>> It is capable of handling a range of video codecs and this driver
>> provides a V4L2 interface for video decoding and encoding.
>>
>> Signed-off-by: Jeongtae Park <jtp.park@samsung.com>
>> Singed-off-by: Janghyuck Kim <janghyuck.kim@samsung.com>
>> Singed-off-by: Jaeryul Oh <jaeryul.oh@samsung.com>
>> Signed-off-by: Naveen Krishna Chatradhi <ch.naveen@samsung.com>
>> Signed-off-by: Arun Kumar K <arun.kk@samsung.com>
>> Cc: Marek Szyprowski <m.szyprowski@samsung.com>
>> Cc: Kamil Debski <k.debski@samsung.com>
>> ---
>>  drivers/media/video/Kconfig                  |   16 +-
>>  drivers/media/video/s5p-mfc/Makefile         |    7 +-
>>  drivers/media/video/s5p-mfc/regs-mfc-v6.h    |  676 ++++++++++
>>  drivers/media/video/s5p-mfc/regs-mfc.h       |   29 +
>>  drivers/media/video/s5p-mfc/s5p_mfc.c        |  163 ++-
>>  drivers/media/video/s5p-mfc/s5p_mfc_cmd.c    |    6 +-
>>  drivers/media/video/s5p-mfc/s5p_mfc_cmd.h    |    3 +
>>  drivers/media/video/s5p-mfc/s5p_mfc_cmd_v6.c |   96 ++
>>  drivers/media/video/s5p-mfc/s5p_mfc_common.h |  123 ++-
>>  drivers/media/video/s5p-mfc/s5p_mfc_ctrl.c   |  160 ++-
>>  drivers/media/video/s5p-mfc/s5p_mfc_ctrl.h   |    1 +
>>  drivers/media/video/s5p-mfc/s5p_mfc_dec.c    |  210 +++-
>>  drivers/media/video/s5p-mfc/s5p_mfc_dec.h    |    1 +
>>  drivers/media/video/s5p-mfc/s5p_mfc_enc.c    |  191 ++--
>>  drivers/media/video/s5p-mfc/s5p_mfc_enc.h    |    1 +
>>  drivers/media/video/s5p-mfc/s5p_mfc_intr.c   |    1 -
>>  drivers/media/video/s5p-mfc/s5p_mfc_opr.c    |  278 +++--
>>  drivers/media/video/s5p-mfc/s5p_mfc_opr.h    |   25 +-
>>  drivers/media/video/s5p-mfc/s5p_mfc_opr_v6.c | 1697 ++++++++++++++++++++++++++
>>  drivers/media/video/s5p-mfc/s5p_mfc_opr_v6.h |  140 +++
>>  drivers/media/video/s5p-mfc/s5p_mfc_pm.c     |    6 +-
>>  drivers/media/video/s5p-mfc/s5p_mfc_shm.c    |   28 +-
>>  drivers/media/video/s5p-mfc/s5p_mfc_shm.h    |   13 +-
>>  drivers/media/video/v4l2-ctrls.c             |    1 -
>>  24 files changed, 3476 insertions(+), 396 deletions(-)
>
> Doesn't it too big for one patch? Can you split it into several patches?
>

Ok. I will split it in the next patch.

>>  create mode 100644 drivers/media/video/s5p-mfc/regs-mfc-v6.h
>>  create mode 100644 drivers/media/video/s5p-mfc/s5p_mfc_cmd_v6.c
>>  create mode 100644 drivers/media/video/s5p-mfc/s5p_mfc_opr_v6.c
>>  create mode 100644 drivers/media/video/s5p-mfc/s5p_mfc_opr_v6.h
>>
>> diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig
>> index 99937c9..0d7fe77 100644
>> --- a/drivers/media/video/Kconfig
>> +++ b/drivers/media/video/Kconfig
>> @@ -1198,13 +1198,27 @@ config VIDEO_SAMSUNG_S5P_JPEG
>>           This is a v4l2 driver for Samsung S5P and EXYNOS4 JPEG codec
>>
>>  config VIDEO_SAMSUNG_S5P_MFC
>> +       bool
>> +
>> +config VIDEO_SAMSUNG_S5P_MFC_V5
>>         tristate "Samsung S5P MFC 5.1 Video Codec"
>> -       depends on VIDEO_DEV && VIDEO_V4L2 && PLAT_S5P
>> +       depends on VIDEO_DEV && VIDEO_V4L2 && ARCH_EXYNOS4
>> +       select VIDEO_SAMSUNG_S5P_MFC
>>         select VIDEOBUF2_DMA_CONTIG
>>         default n
>>         help
>>             MFC 5.1 driver for V4L2.
>>
>> +config VIDEO_SAMSUNG_S5P_MFC_V6
>
> Yes, I know it's exynos5 series features. however, it's not good idea
> to add new config.
> It already handled platform device with proper name.
> e.g., s5p-mfc-v5, s5p-mfc-v6 and handle it with platform data.
>

Ok. Code changes are required for compiling both v5 and v6 together.
Will incorporate in next patch.

[snip]

>> -#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"
> I think it can handle clkname without new config.
>

Yes I will change it.

Regards
Arun

^ permalink raw reply	[flat|nested] 5+ messages in thread
* RE: [PATCH v2 2/2] [media] s5p-mfc: update MFC v4l2 driver to support MFC6.x
@ 2012-07-12 12:54 Arun Kumar K
  0 siblings, 0 replies; 5+ messages in thread
From: Arun Kumar K @ 2012-07-12 12:54 UTC (permalink / raw)
  To: Kamil Debski, linux-media
  Cc: Jeongtae Park, Jang-Hyuck Kim, peter Oh,
	NAVEEN KRISHNA CHATRADHI, Marek Szyprowski, Sylwester Nawrocki,
	hans.verkuil, mchehab

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

^ permalink raw reply	[flat|nested] 5+ messages in thread
* [PATCH v2 0/2] update MFC v4l2 driver to support MFC6.x
@ 2012-07-06 14:00 Arun Kumar K
  2012-07-06 14:00 ` [PATCH v2 2/2] [media] s5p-mfc: " Arun Kumar K
  0 siblings, 1 reply; 5+ messages in thread
From: Arun Kumar K @ 2012-07-06 14:00 UTC (permalink / raw)
  To: linux-media
  Cc: jtp.park, janghyuck.kim, jaeryul.oh, ch.naveen, arun.kk,
	m.szyprowski, k.debski, s.nawrocki, hans.verkuil, mchehab

This is the v2 series of patches for adding support for MFC v6.x.
In this the new v4l controls added in patch [1] are removed. These can be
added as a separate patch later for providing extra encoder controls for
MFC v6. This also incorporates the review comments received for the original
patch and fixed for backward compatibility with MFC v5.

[1] http://article.gmane.org/gmane.linux.drivers.video-input-infrastructure/45190/

Jeongtae Park (2):
  [media] v4l: add fourcc definitions for new formats
  [media] s5p-mfc: update MFC v4l2 driver to support MFC6.x

 drivers/media/video/Kconfig                  |   16 +-
 drivers/media/video/s5p-mfc/Makefile         |    7 +-
 drivers/media/video/s5p-mfc/regs-mfc-v6.h    |  676 ++++++++++
 drivers/media/video/s5p-mfc/regs-mfc.h       |   29 +
 drivers/media/video/s5p-mfc/s5p_mfc.c        |  163 ++-
 drivers/media/video/s5p-mfc/s5p_mfc_cmd.c    |    6 +-
 drivers/media/video/s5p-mfc/s5p_mfc_cmd.h    |    3 +
 drivers/media/video/s5p-mfc/s5p_mfc_cmd_v6.c |   96 ++
 drivers/media/video/s5p-mfc/s5p_mfc_common.h |  123 ++-
 drivers/media/video/s5p-mfc/s5p_mfc_ctrl.c   |  160 ++-
 drivers/media/video/s5p-mfc/s5p_mfc_ctrl.h   |    1 +
 drivers/media/video/s5p-mfc/s5p_mfc_dec.c    |  210 +++-
 drivers/media/video/s5p-mfc/s5p_mfc_dec.h    |    1 +
 drivers/media/video/s5p-mfc/s5p_mfc_enc.c    |  191 ++--
 drivers/media/video/s5p-mfc/s5p_mfc_enc.h    |    1 +
 drivers/media/video/s5p-mfc/s5p_mfc_intr.c   |    1 -
 drivers/media/video/s5p-mfc/s5p_mfc_opr.c    |  278 +++--
 drivers/media/video/s5p-mfc/s5p_mfc_opr.h    |   25 +-
 drivers/media/video/s5p-mfc/s5p_mfc_opr_v6.c | 1697 ++++++++++++++++++++++++++
 drivers/media/video/s5p-mfc/s5p_mfc_opr_v6.h |  140 +++
 drivers/media/video/s5p-mfc/s5p_mfc_pm.c     |    6 +-
 drivers/media/video/s5p-mfc/s5p_mfc_shm.c    |   28 +-
 drivers/media/video/s5p-mfc/s5p_mfc_shm.h    |   13 +-
 drivers/media/video/v4l2-ctrls.c             |    1 -
 include/linux/videodev2.h                    |    4 +
 25 files changed, 3480 insertions(+), 396 deletions(-)
 create mode 100644 drivers/media/video/s5p-mfc/regs-mfc-v6.h
 create mode 100644 drivers/media/video/s5p-mfc/s5p_mfc_cmd_v6.c
 create mode 100644 drivers/media/video/s5p-mfc/s5p_mfc_opr_v6.c
 create mode 100644 drivers/media/video/s5p-mfc/s5p_mfc_opr_v6.h


^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2012-07-12 12:55 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-07-12 12:43 [PATCH v2 2/2] [media] s5p-mfc: update MFC v4l2 driver to support MFC6.x Arun Kumar K
  -- strict thread matches above, loose matches on Subject: below --
2012-07-12 12:54 Arun Kumar K
2012-07-06 14:00 [PATCH v2 0/2] " Arun Kumar K
2012-07-06 14:00 ` [PATCH v2 2/2] [media] s5p-mfc: " Arun Kumar K
2012-07-06 14:21   ` Kyungmin Park
2012-07-10 15:10   ` Kamil Debski

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.