RE: [PATCH v7 5/6] [media] s5p-mfc: MFCv6 register definitions

[Arun Kumar K <arun.kk@samsung.com>]

Hi Jeongtae Park,

I have verified the calculation.
The ALIGN macro is giving a wrong result and I used the macro DIV_ROUND_UP
in the v8 patchset which is giving the proper result.

Regards
Arun

On Tue, Oct 2, 2012 at 10:58 AM, JeongTae Park <jtp.park@samsung.com> wrote:
> Hi Arun and Sylwester,
>
> Please make sure that below calculations are same.
>
>> +#define S5P_FIMV_ME_BUFFER_SIZE_V6(imw, imh, mbw, mbh) \r
>> +                                             (((((imw+63)/64) * 16) * \r
>> +                                             (((imh+63)/64) * 16)) + \r
>> +                                             ((((mbw*mbh)+31)/32) * 16))
>
>
> #define S5P_FIMV_ME_BUFFER_SIZE_V6(imw, imh, mbw, mbh) \r
>         ((ALIGN(imw, 64) *  ALIGN(imh, 64) * 256) + (ALIGN((mbw) * (mbh), 32) * 16))
>
> Best Regards
> /jtpark
>
>> -----Original Message-----
>> From: Arun Kumar K [mailto:arun.kk@samsung.com]
>> Sent: Monday, October 01, 2012 1:37 PM
>> To: Sylwester Nawrocki
>> Cc: linux-media@vger.kernel.org; Kamil Debski; Jeongtae Park; Jang-Hyuck Kim; peter Oh; NAVEEN KRISHNA
>> CHATRADHI; Marek Szyprowski; Sylwester Nawrocki; kmpark@infradead.org; SUNIL JOSHI
>> Subject: Re: [PATCH v7 5/6] [media] s5p-mfc: MFCv6 register definitions
>>
>> Hi Sylwester,
>> Thank you for the comments.
>> Will make necessary changes and post updated patchset.
>>
>> Regards
>> Arun
>>
>> ------- Original Message -------
>> Sender : Sylwester Nawrocki<sylvester.nawrocki@gmail.com>
>> Date   : Sep 29, 2012 21:05 (GMT+05:30)
>> Title  : Re: [PATCH v7 5/6] [media] s5p-mfc: MFCv6 register definitions
>>
>> Hi Arun,
>>
>> I have a few minor comments.
>>
>> On 09/28/2012 07:04 PM, Arun Kumar K wrote:
>> > From: Jeongtae Park<jtp.park@samsung.com>
>> >
>> > Adds register definitions for MFC v6.x firmware
>> >
>> > Signed-off-by: Jeongtae Park<jtp.park@samsung.com>
>> > Signed-off-by: Janghyuck Kim<janghyuck.kim@samsung.com>
>> > Signed-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>
>> > ---
>> >   drivers/media/platform/s5p-mfc/regs-mfc-v6.h |  408 ++++++++++++++++++++++++++
>> >   1 files changed, 408 insertions(+), 0 deletions(-)
>> >   create mode 100644 drivers/media/platform/s5p-mfc/regs-mfc-v6.h
>> >
>> > diff --git a/drivers/media/platform/s5p-mfc/regs-mfc-v6.h b/drivers/media/platform/s5p-mfc/regs-mfc-
>> v6.h
>> > new file mode 100644
>> > index 0000000..cce1841
>> > --- /dev/null
>> > +++ b/drivers/media/platform/s5p-mfc/regs-mfc-v6.h
>> > @@ -0,0 +1,408 @@
>> > +/*
>> > + * Register definition file for Samsung MFC V6.x Interface (FIMV) driver
>> > + *
>> > + * Copyright (c) 2012 Samsung Electronics
>>
>> I believe the proper notation is
>>
>>       Copyright (c) 2012 Samsung Electronics Co., Ltd.
>>
>> Please make sure it's correct in all files added in this series.
>>
>> > + *         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.
>> > + */
>> > +
>> > +#ifndef _REGS_FIMV_V6_H
>> > +#define _REGS_FIMV_V6_H
>>
>> Please add
>>
>> #include <linux/kernel.h>
>> #include <linux/sizes.h>
>>
>> > +#define S5P_FIMV_REG_SIZE_V6       (S5P_FIMV_END_ADDR - S5P_FIMV_START_ADDR)
>> > +#define S5P_FIMV_REG_COUNT_V6      ((S5P_FIMV_END_ADDR - S5P_FIMV_START_ADDR) / 4)
>> > +
>> > +/* Number of bits that the buffer address should be shifted for particular
>> > + * MFC buffers.  */
>> > +#define S5P_FIMV_MEM_OFFSET_V6             0
>> > +
>> > +#define S5P_FIMV_START_ADDR_V6             0x0000
>> > +#define S5P_FIMV_END_ADDR_V6               0xfd80
>> > +
>> > +#define S5P_FIMV_REG_CLEAR_BEGIN_V6        0xf000
>> > +#define S5P_FIMV_REG_CLEAR_COUNT_V6        1024
>> > +
>> > +/* Codec Common Registers */
>> > +#define S5P_FIMV_RISC_ON_V6                        0x0000
>> > +#define S5P_FIMV_RISC2HOST_INT_V6          0x003C
>>
>> Could you make sure all hex numbers are in lower case in this file ?
>>
>> ...
>> > +#define S5P_FIMV_NUM_TMV_BUFFERS_V6                2
>> > +
>> > +#define S5P_FIMV_MAX_FRAME_SIZE_V6                 (2048 * 1024)
>>
>> (2 * SZ_1M)
>>
>> > +#define S5P_FIMV_NUM_PIXELS_IN_MB_ROW_V6           16
>> > +#define S5P_FIMV_NUM_PIXELS_IN_MB_COL_V6           16
>> > +
>> > +/* Buffer size requirements defined by hardware */
>> > +#define S5P_FIMV_TMV_BUFFER_SIZE_V6(w, h)  ((w + 1) * (h + 1) * 8)
>>
>> The arguments should be in parentheses in the expressions, i.e.
>>
>> #define S5P_FIMV_TMV_BUFFER_SIZE_V6(w, h)     (((w) + 1) * ((h) + 1) * 8)
>>
>> > +#define S5P_FIMV_ME_BUFFER_SIZE_V6(imw, imh, mbw, mbh) \r
>> > +                                           (((((imw+63)/64) * 16) * \r
>> > +                                           (((imh+63)/64) * 16)) + \r
>> > +                                           ((((mbw*mbh)+31)/32) * 16))
>>
>> Could be rewritten as:
>>
>> #define S5P_FIMV_ME_BUFFER_SIZE_V6(imw, imh, mbw, mbh) \r
>>       ((ALIGN(imw, 64) *  ALIGN(imh, 64) * 256) + (ALIGN((mbw) * (mbh), 32) * 16))
>>
>>
>> > +#define S5P_FIMV_SCRATCH_BUF_SIZE_H264_DEC_V6(w, h)        ((w * 192) + 64)
>> > +#define S5P_FIMV_SCRATCH_BUF_SIZE_MPEG4_DEC_V6(w, h) \r
>> > +                                           (w * (h * 64 + 144) + \r
>> > +                                           ((2048 + 15)/16 * h * 64) + \r
>> > +                                           ((2048 + 15)/16 * 256 + 8320))
>>
>>       (w * (h * 64 + 144) + (2048/16 * h * 64) + (2048/16 * 256 + 8320))
>>
>> > +#define S5P_FIMV_SCRATCH_BUF_SIZE_VC1_DEC_V6(w, h) (2096 * (w + h + 1))
>> > +#define S5P_FIMV_SCRATCH_BUF_SIZE_H263_DEC_V6(w, h)        (w * 400)
>> > +#define S5P_FIMV_SCRATCH_BUF_SIZE_VP8_DEC_V6(w, h) \r
>> > +                                           (w * 32 + h * 128 + \r
>> > +                                           ((w + 1) / 2) * 64 + 2112)
>>
>> Unnecessarily broken into two lines.
>>
>> > +#define S5P_FIMV_SCRATCH_BUF_SIZE_H264_ENC_V6(w, h) \r
>> > +                                           ((w * 64) + ((w + 1) * 16) + \r
>> > +                                           (4096 * 16))
>>
>> Ditto.
>>
>> > +#define S5P_FIMV_SCRATCH_BUF_SIZE_MPEG4_ENC_V6(w, h) \r
>> > +                                           ((w * 16) + ((w + 1) * 16))
>> > +
>> > +/* MFC Context buffer sizes */
>> > +#define MFC_CTX_BUF_SIZE_V6                0x7000          /*  28KB */
>>
>> Perhaps we could use the SZ_* macro definitions, e.g. (28 * SZ_1K) ?
>>
>> > +#define MFC_H264_DEC_CTX_BUF_SIZE_V6       0x200000        /* 1.6MB */
>>
>> (1600 * SZ_1K) ...
>>
>> > +#define MFC_OTHER_DEC_CTX_BUF_SIZE_V6      0x5000          /*  20KB */
>> > +#define MFC_H264_ENC_CTX_BUF_SIZE_V6       0x19000         /* 100KB */
>> > +#define MFC_OTHER_ENC_CTX_BUF_SIZE_V6      0x3000          /*  12KB */
>> > +
>> > +/* MFCv6 variant defines */
>> > +#define MAX_FW_SIZE_V6                     0x100000        /* 1MB */
>> > +#define MAX_CPB_SIZE_V6                    0x300000        /* 3MB */
>>
>> ... (3 * SZ_1M)
>>
>> > +#define MFC_VERSION_V6                     0x61
>> > +#define MFC_NUM_PORTS_V6           1
>> > +
>> > +#endif /* _REGS_FIMV_V6_H */
>>
>> Thanks,
>> Sylwester
>> <p>&nbsp;</p><p>&nbsp;</p>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html