linux-media.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* Re: [PATCH v7 5/6] [media] s5p-mfc: MFCv6 register definitions
@ 2012-10-01  4:37 Arun Kumar K
  2012-10-02  5:28 ` JeongTae Park
  0 siblings, 1 reply; 5+ messages in thread
From: Arun Kumar K @ 2012-10-01  4:37 UTC (permalink / raw)
  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

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>

^ permalink raw reply	[flat|nested] 5+ messages in thread
* RE: [PATCH v7 5/6] [media] s5p-mfc: MFCv6 register definitions
@ 2012-10-02  6:03 Arun Kumar K
  2012-10-02  8:24 ` Sylwester Nawrocki
  0 siblings, 1 reply; 5+ messages in thread
From: Arun Kumar K @ 2012-10-02  6:03 UTC (permalink / raw)
  To: Jeongtae Park, 'Sylwester Nawrocki'
  Cc: linux-media@vger.kernel.org, Kamil Debski, Jang-Hyuck Kim,
	peter Oh, NAVEEN KRISHNA CHATRADHI, Marek Szyprowski,
	Sylwester Nawrocki, kmpark@infradead.org, SUNIL JOSHI

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

^ permalink raw reply	[flat|nested] 5+ messages in thread
end of thread, other threads:[~2012-10-02  8:24 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
     [not found] <1348851868-7698-1-git-send-email-arun.kk@samsung.com>
     [not found] ` <1348851868-7698-6-git-send-email-arun.kk@samsung.com>
2012-09-29 15:35   ` [PATCH v7 5/6] [media] s5p-mfc: MFCv6 register definitions Sylwester Nawrocki
2012-10-01  4:37 Arun Kumar K
2012-10-02  5:28 ` JeongTae Park
  -- strict thread matches above, loose matches on Subject: below --
2012-10-02  6:03 Arun Kumar K
2012-10-02  8:24 ` Sylwester Nawrocki

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).