Hi Kamil, Thanks for the review comments. I will address all the comments and will post v4 version.
Regards Arun ------- Original Message ------- Sender : Kamil Debski<k.deb...@samsung.com> Software Engineer/SPRC-Linux Platform (SSD)/Samsung Electronics Date : Aug 06, 2012 18:50 (GMT+05:30) Title : RE: [PATCH v3 4/4] [media] s5p-mfc: New files for MFCv6 support Hi Arun, Please find my comments below. Best wishes, -- Kamil Debski Linux Platform Group Samsung Poland R&D Center > From: Arun Kumar K [mailto:arun...@samsung.com] > Sent: 23 July 2012 14:29 > > From: Jeongtae Park <jtp.p...@samsung.com> > > New register definitions, commands and hardware operations > file for MFCv6 support. > > Signed-off-by: Jeongtae Park <jtp.p...@samsung.com> > Singed-off-by: Janghyuck Kim <janghyuck....@samsung.com> > Singed-off-by: Jaeryul Oh <jaeryul...@samsung.com> > Signed-off-by: Naveen Krishna Chatradhi <ch.nav...@samsung.com> > Signed-off-by: Arun Kumar K <arun...@samsung.com> > --- > drivers/media/video/s5p-mfc/regs-mfc-v6.h | 392 ++++++ > drivers/media/video/s5p-mfc/s5p_mfc_cmd_v6.c | 155 +++ > drivers/media/video/s5p-mfc/s5p_mfc_cmd_v6.h | 22 + > drivers/media/video/s5p-mfc/s5p_mfc_opr_v6.c | 1921 ++++++++++++++++++++++++++ > drivers/media/video/s5p-mfc/s5p_mfc_opr_v6.h | 50 + > 5 files changed, 2540 insertions(+), 0 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_cmd_v6.h > 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 > [snip] > diff --git a/drivers/media/video/s5p-mfc/s5p_mfc_opr_v6.c > b/drivers/media/video/s5p-mfc/s5p_mfc_opr_v6.c > new file mode 100644 > index 0000000..86c8645 > --- /dev/null > +++ b/drivers/media/video/s5p-mfc/s5p_mfc_opr_v6.c > @@ -0,0 +1,1921 @@ [snip] > + > +/* Allocate codec buffers */ > +int s5p_mfc_alloc_codec_buffers_v6(struct s5p_mfc_ctx *ctx) > +{ > + struct s5p_mfc_dev *dev = ctx->dev; > + unsigned int mb_width, mb_height; > + > + mb_width = mb_width(ctx->img_width); > + mb_height = mb_height(ctx->img_height); > + > + if (ctx->type == MFCINST_DECODER) { > + mfc_debug(2, "Luma size:%d Chroma size:%d MV size:%d ", > + ctx->luma_size, ctx->chroma_size, ctx->mv_size); > + mfc_debug(2, "Totals bufs: %d ", ctx->total_dpb_count); > + } else if (ctx->type == MFCINST_ENCODER) { > + ctx->tmv_buffer_size = 2 * ALIGN((mb_width + 1) * > + (mb_height + 1) * 8, 16); > + ctx->luma_dpb_size = ALIGN((mb_width * mb_height) * 256, 256); > + ctx->chroma_dpb_size = ALIGN((mb_width * mb_height) * 128, 256); > + ctx->me_buffer_size = ALIGN(((((ctx->img_width+63)/64) * 16) * > + (((ctx->img_height+63)/64) * 16)) + > + ((((mb_width*mb_height)+31)/32) * 16), 256); Let's stop right here. There are too many magic numbers, all of them need a nice #define name. > + > + mfc_debug(2, "recon luma size: %d chroma size: %d ", > + ctx->luma_dpb_size, ctx->chroma_dpb_size); > + } else { > + return -EINVAL; > + } > + > + /* Codecs have different memory requirements */ > + switch (ctx->codec_mode) { > + case S5P_MFC_CODEC_H264_DEC: > + case S5P_MFC_CODEC_H264_MVC_DEC: > + ctx->scratch_buf_size = (mb_width * 192) + 64; > + ctx->scratch_buf_size = ALIGN(ctx->scratch_buf_size, 256); > + ctx->bank1_size = > + ctx->scratch_buf_size + > + (ctx->mv_count * ctx->mv_size); > + break; > + case S5P_MFC_CODEC_MPEG4_DEC: > + /* mb_width * (mb_height * 64 + 144) + 8192 * mb_height + > + * 41088 */ > + ctx->scratch_buf_size = mb_width * (mb_height * 64 + 144) + > + ((2048 + 15)/16 * mb_height * 64) + > + ((2048 + 15)/16 * 256 + 8320); > + ctx->scratch_buf_size = ALIGN(ctx->scratch_buf_size, 256); > + ctx->bank1_size = ctx->scratch_buf_size; > + break; > + case S5P_MFC_CODEC_VC1RCV_DEC: > + case S5P_MFC_CODEC_VC1_DEC: > + ctx->scratch_buf_size = 2096 * (mb_width + mb_height + 1); > + ctx->scratch_buf_size = ALIGN(ctx->scratch_buf_size, 256); > + ctx->bank1_size = ctx->scratch_buf_size; > + break; > + case S5P_MFC_CODEC_MPEG2_DEC: > + ctx->bank1_size = 0; > + ctx->bank2_size = 0; > + break; > + case S5P_MFC_CODEC_H263_DEC: > + ctx->scratch_buf_size = mb_width * 400; > + ctx->scratch_buf_size = ALIGN(ctx->scratch_buf_size, 256); > + ctx->bank1_size = ctx->scratch_buf_size; > + break; > + case S5P_MFC_CODEC_VP8_DEC: > + ctx->scratch_buf_size = mb_width * 32 + mb_height * 128 + 34816; > + ctx->scratch_buf_size = ALIGN(ctx->scratch_buf_size, 256); > + ctx->bank1_size = ctx->scratch_buf_size; > + break; > + case S5P_MFC_CODEC_H264_ENC: > + ctx->scratch_buf_size = (mb_width * 64) + > + ((mb_width + 1) * 16) + (4096 * 16); > + ctx->scratch_buf_size = ALIGN(ctx->scratch_buf_size, 256); > + ctx->bank1_size = > + ctx->scratch_buf_size + ctx->tmv_buffer_size + > + (ctx->dpb_count * (ctx->luma_dpb_size + > + ctx->chroma_dpb_size + ctx->me_buffer_size)); > + ctx->bank2_size = 0; > + break; > + case S5P_MFC_CODEC_MPEG4_ENC: > + case S5P_MFC_CODEC_H263_ENC: > + ctx->scratch_buf_size = (mb_width * 16) + ((mb_width + 1) * 16); > + ctx->scratch_buf_size = ALIGN(ctx->scratch_buf_size, 256); > + ctx->bank1_size = > + ctx->scratch_buf_size + ctx->tmv_buffer_size + > + (ctx->dpb_count * (ctx->luma_dpb_size + > + ctx->chroma_dpb_size + ctx->me_buffer_size)); > + ctx->bank2_size = 0; > + break; > + default: > + break; > + } > + > + /* Allocate only if memory from bank 1 is necessary */ > + if (ctx->bank1_size > 0) { > + ctx->bank1_buf = vb2_dma_contig_memops.alloc( > + dev->alloc_ctx[MFC_BANK1_ALLOC_CTX], ctx->bank1_size); > + if (IS_ERR(ctx->bank1_buf)) { > + ctx->bank1_buf = 0; > + pr_err("Buf alloc for decoding failed (port A) "); > + return -ENOMEM; > + } > + ctx->bank1_phys = s5p_mfc_mem_cookie( > + dev->alloc_ctx[MFC_BANK1_ALLOC_CTX], ctx->bank1_buf); > + BUG_ON(ctx->bank1_phys & ((1 << MFC_BANK1_ALIGN_ORDER) - 1)); > + } > + > + return 0; > +} > + [snip] > + > +static int calc_plane(int width, int height) > +{ > + int mbX, mbY; > + > + mbX = (width + 15)/16; > + mbY = (height + 15)/16; > + > + if (width * height < 2048 * 1024) > + mbY = (mbY + 1) / 2 * 2; > + > + return (mbX * 16) * (mbY * 16); > +} The magic numbers above should be defined in the headers file and have readable and descriptive names. [snip] > +/* Decode a single frame */ > +int s5p_mfc_decode_one_frame_v6(struct s5p_mfc_ctx *ctx, > + enum s5p_mfc_decode_arg last_frame) > +{ > + struct s5p_mfc_dev *dev = ctx->dev; > + > + WRITEL(0xffffffff, S5P_FIMV_D_AVAILABLE_DPB_FLAG_LOWER_V6); > + WRITEL(0xffffffff, S5P_FIMV_D_AVAILABLE_DPB_FLAG_UPPER_V6); This cannot be done this way. Come on, the system of marking which buffer is queued by the application is already in place (look at the s5p_mfc_opr_v5.c file). If all buffers are marked accessible to MFC hardware then there is no guarantee that buffers dequeued and used by user space are not overwritten. > + WRITEL(ctx->slice_interface & 0x1, S5P_FIMV_D_SLICE_IF_ENABLE_V6); > + > + WRITEL(ctx->inst_no, S5P_FIMV_INSTANCE_ID_V6); > + /* Issue different commands to instance basing on whether it > + * is the last frame or not. */ > + switch (last_frame) { > + case 0: > + s5p_mfc_cmd_host2risc(dev, S5P_FIMV_CH_FRAME_START_V6, NULL); > + break; > + case 1: > + s5p_mfc_cmd_host2risc(dev, S5P_FIMV_CH_LAST_FRAME_V6, NULL); > + break; > + default: > + mfc_err("Unsupported last frame arg. "); > + return -EINVAL; > + } > + > + mfc_debug(2, "Decoding a usual frame. "); > + return 0; > +} > + [snip] <p> </p><p> </p>N‹§²æìr¸›yúèšØb²X¬¶Ç§vØ^–)Þº{.nÇ+‰·¥Š{±™çbj)í…æèw* jg¬±¨ ¶‰šŽŠÝ¢j/�êäz¹Þ–Šà2ŠÞ™¨èÚ&¢)ß¡«a¶Úþø ®G«�éh®æj:+v‰¨Šwè†Ù¥
