I commented as belows, And you missed one important things 'cause there were my comments in the very long email which is strongly fixed in the reset seq.
[....] > +#define READL(offset) readl(dev->regs_base + (offset)) > +#define WRITEL(data, offset) writel((data), dev->regs_base + (offset)) > +#define OFFSETA(x) (((x) - dev->port_a) >> 11) > +#define OFFSETB(x) (((x) - dev->port_b) >> 11) > + > +/* Reset the device */ > +static int s5p_mfc_cmd_reset(struct s5p_mfc_dev *dev) > +{ > + unsigned int mc_status; > + unsigned long timeout; > + mfc_debug("s5p_mfc_cmd_reset++\n"); > + /* Stop procedure */ > + WRITEL(0x3f7, S5P_FIMV_SW_RESET); /* reset VI */ Ahm, This (WRITEL(0x3f7, S5P_FIMV_SW_RESET)) might be a problem. In the reset seq. of MFC driver, we checked out That FW(s5pc110-mfc.fw in the s5p_mfc_load_firmware()) runned by RISC core at this point could access invalid address. It should be removed. > >Hi, Kamil > >This is second feedback about the HW op related code. > >(s5p_mfc_opr.c & s5p_mfc.c) > > Hi, Peter > > Thanks for the comments. I have replied to them below. I would be grateful > if you could cut out non relevant parts of the code in your replies. It > was > difficult to find your comments in such a big email. I hope I have found > all > of them. > > [...] > > >> + s5p_mfc_set_dec_stream_buffer(ctx, \ > >> + vb2_plane_paddr(temp_vb, 0), 0, \ > >> + temp_vb->v4l2_planes[0].bytesused); > >> + dev->curr_ctx = ctx->num; > >> + s5p_mfc_clean_ctx_int_flags(ctx); > >> + s5p_mfc_decode_one_frame(ctx, > >> + temp_vb->v4l2_planes[0].bytesused == 0); > >> + } else if (ctx->state == MFCINST_DEC_INIT) { > >> + /* Preparing decoding - getting instance number */ > >> + mfc_debug("Getting instance number\n"); > >> + dev->curr_ctx = ctx->num; > >> + s5p_mfc_clean_ctx_int_flags(ctx); > >> +/* s5p_mfc_set_dec_temp_buffers(ctx); > >> + * Removed per request by Peter, check if MFC works OK > >*/ > > >Yes, you don't have to set s5p_mfc_set_dec_temp_buffers(ctx)at this point > >'cause this is only required before parsing header of the stream except > for > > >setting shared memory, > >so, I think, separating 'setting S5P_FIMV_SI_CH0_HOST_WR_ADR' from the > >s5p_mfc_set_dec_temp_buffers() is needed. So I mean > >remove 'setting S5P_FIMV_SI_CH0_HOST_WR_ADR' from > >s5p_mfc_set_dec_temp_buffers(ctx);, then add 'setting > >S5P_FIMV_SI_CH0_HOST_WR_ADR' > >in the 3 functions (s5p_mfc_set_dec_frame_buffer(),s5p_mfc_init_decode(), > >s5p_mfc_decode_one_frame()) > >I also commented suggested loc. of 'setting S5P_FIMV_SI_CH0_HOST_WR_ADR' > > Right, I've changed this. > > > >> + ret = s5p_mfc_open_inst(ctx); > >> + if (ret) { > >> + mfc_err("Failed to create a new > instance.\n"); > >> + ctx->state = MFCINST_DEC_ERROR; > >> + } > >> + } else if (ctx->state == MFCINST_DEC_RETURN_INST) { > >> + /* Closing decoding instance */ > >> + mfc_debug("Returning instance number\n"); > >> + dev->curr_ctx = ctx->num; > >> + s5p_mfc_clean_ctx_int_flags(ctx); > >> + ret = s5p_mfc_return_inst_no(ctx); > >> + if (ret) { > >> + mfc_err("Failed to return an instance.\n"); > >> + ctx->state = MFCINST_DEC_ERROR; > >> + } > > [...] > > >> + } > >> + /* Init videobuf2 queue for CAPTURE */ > >> + ret = vb2_queue_init(&ctx->vq_dst, &s5p_mfc_qops, > >> + dev->alloc_ctx[0], &dev->irqlock, > >> + V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE, ctx); > >> + if (ret) { > >> + mfc_err("Failed to initialize videobuf2 queue (capture)\n"); > >> + goto out_open_3; > >> + } > >> + /* Init videobuf2 queue for OUTPUT */ > >> + ret = vb2_queue_init(&ctx->vq_src, &s5p_mfc_qops, > >> + dev->alloc_ctx[1], &dev->irqlock, > >> + V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE, ctx); > >> + if (ret) { > >> + mfc_err("Failed to initialize videobuf2 queue (output)\n"); > >> + goto out_open_3; > >> + } > > >About using dev->irqlock, (spinlock_t *drv_lock) in the vb2_queue_init() > is > >removed > >according to the Marek's patch (from the Hans Verkuil request) > > : http://www.spinics.net/lists/linux-media/msg23902.html > >Does it mean that we don't have to use drv_lock in the driver ? > >What actually purpose of drv_lock that you used in the MFC driver ? > > I'll remove this when videobuf2 will be merged. The version posted by > Marek > is still not final, so it is better to wait until a final version is > worked > out in the community. OK, I will wait until final version is released. > > > >> + vb2_set_alloc_ctx(&ctx->vq_dst, dev->alloc_ctx[1], 1); > >> + init_waitqueue_head(&ctx->queue); > >> + mutex_unlock(dev->mfc_mutex); > >> + mfc_debug("s5p_mfc_open--\n"); > >> + return ret; > >> + /* Deinit when failure occured */ > >> +out_open_3: > >> + if (atomic_read(&dev->num_inst) == 1) { > >> + clk_disable(dev->clock1); > > > [...] > > >> + mfc_debug("s5p_mfc_alloc_dec_temp_buffers++\n"); > >> + mfc_ctx->desc_phys = cma_alloc(mfc_ctx->dev->v4l2_dev.dev, > >> + MFC_CMA_BANK1, DESC_BUF_SIZE, 2048); > >> + if (IS_ERR_VALUE(mfc_ctx->desc_phys)) { > >> + mfc_ctx->desc_phys = 0; > >> + mfc_err("Allocating DESC buffer failed.\n"); > >> + return -ENOMEM; > >> + } > >> + desc_virt = ioremap_nocache(mfc_ctx->desc_phys, DESC_BUF_SIZE); > > >In case of ioremap_xx() function, you might meet that msg as below > >"Don't allow RAM to be mapped - this causes problems with ARMv6+ " > >(arch/arm/mm/ioremap.c). so we should not use this function for this case. > >I suggest that you use phys_to_virt() with some cache op. such as > >cache_clean for writing case(ex> memset) & cache_invalidate for reading > >case. > >(ex>reading shared mem) > >It is necessary for accessing shared memory(DRAM area accessed by ARM & > MFC) > > At this moment we are waiting for the final CMA version and we don't know > whether phys_to_virt will work with it. Now it has been changed from bug > to > a warning by Russel. Do you mean final version of CMA includes that kinds of function (ioremap, phys_to_virt)? Anyway, using with phys_to_virt() doesn't look like having a problem in my environment only if it is used with cache function as below. static void s5p_mfc_mem_cache_clean(const void *start_addr, unsigned long size) { unsigned long paddr; dmac_map_area(start_addr, size, DMA_TO_DEVICE); /* * virtual & phsical addrees mapped directly, so we can convert * the address just using offset */ paddr = __pa((unsigned long)start_addr); outer_clean_range(paddr, paddr + size); } static void s5p_mfc_mem_cache_inv(const void *start_addr, unsigned long size) { unsigned long paddr; paddr = __pa((unsigned long)start_addr); outer_inv_range(paddr, paddr + size); dmac_unmap_area(start_addr, size, DMA_FROM_DEVICE); } > > >> + if (desc_virt == NULL) { > >> + cma_free(mfc_ctx->desc_phys); > >> + mfc_ctx->desc_phys = 0; > >> + mfc_err("Remapping DESC buffer failed.\n"); > >> + return -ENOMEM; > >> + } > >> + /* Zero content of the allocated memory, in future this might be > >> done > >> + * by cma_alloc */ > >> + memset(desc_virt, 0, DESC_BUF_SIZE); > >> + iounmap(desc_virt); > >> + mfc_debug("s5p_mfc_alloc_dec_temp_buffers--\n"); > > [...] > > >> + mfc_debug("s5p_mfc_release_instance_buffer--\n"); > >> +} > >> + > >> +/* Set registers for decoding temporary buffers */ > >> +void s5p_mfc_set_dec_temp_buffers(struct s5p_mfc_ctx *mfc_ctx) > >> +{ > >> + struct s5p_mfc_dev *dev = mfc_ctx->dev; > >> + WRITEL(OFFSETA(mfc_ctx->desc_phys), S5P_FIMV_SI_CH0_DESC_ADR); > >> + WRITEL(CPB_BUF_SIZE, S5P_FIMV_SI_CH0_CPB_SIZE); > >> + WRITEL(DESC_BUF_SIZE, S5P_FIMV_SI_CH0_DESC_SIZE); > >> + WRITEL(mfc_ctx->shared_phys - mfc_ctx->dev->port_a, > >> + S5P_FIMV_SI_CH0_HOST_WR_ADR); > >> +} > > >I mentioned that separating 'setting S5P_FIMV_SI_CH0_HOST_WR_ADR' from > >s5p_mfc_set_dec_temp_buffers() is better in terms of modular design. > >And what about having clear function name ? using 'temp' in the func. > Name > >is not specific > > What name do you suggest? If I name the function > s5p_mfc_set_dec_desc_buffer() > it will contain no information that other registers are set too. > Yes, s5p_mfc_set_dec_desc_buffer() has clear name, what about moving S5P_FIMV_SI_CH0_CPB_SIZE to in the s5p_mfc_set_dec_stream_buffer()? > >> + > >> +/* Set registers for decoding stream buffer */ > >> +int s5p_mfc_set_dec_stream_buffer(struct s5p_mfc_ctx *mfc_ctx, int > >> buf_addr, > >> + unsigned int start_num_byte, unsigned int buf_size) > >> +{ > >> + struct s5p_mfc_dev *dev = mfc_ctx->dev; > >> + mfc_debug("inst_no : %d, buf_addr : 0x%08x, buf_size : 0x%08x > >> (%d)\n", > >> + mfc_ctx->inst_no, buf_addr, buf_size, buf_size); > >> + if (buf_addr & (2048 - 1)) { > >> + mfc_err("Source stream buffer is not aligned correctly.\n"); > >> + return -EINVAL; > >> + } > > [...] > > >> + buf_size2 = mfc_ctx->port_b_size; > >> + mfc_debug("Buf1: %p (%d) Buf2: %p (%d)\n", (void *)buf_addr1, > >> buf_size1, > >> + (void *)buf_addr2, buf_size2); > >> + /* Enable generation of extra info */ > >> +/* *(shared_mem_vir_addr + 0x0038) = 63; */ > >> + mfc_debug("Total DPB COUNT: %d\n", mfc_ctx->total_dpb_count); > >> + mfc_debug("Setting display delay to %d\n", mfc_ctx->display_delay); > >> + dpb = READL(S5P_FIMV_SI_CH0_DPB_CONF_CTRL) & 0xFFFF0000; > >> + WRITEL(mfc_ctx->total_dpb_count | dpb, > >> S5P_FIMV_SI_CH0_DPB_CONF_CTRL); > >> + s5p_mfc_set_dec_temp_buffers(mfc_ctx); > > >I think, all reg. set in the s5p_mfc_set_dec_temp_buffers(mfc_ctx)is not > >required > >at this point. > >What about only adding 'set S5P_FIMV_SI_CH0_HOST_WR_ADR) instead of using > >s5p_mfc_set_dec_temp_buffers()? > > Done. > > >> + switch (mfc_ctx->codec_mode) { > >> + case S5P_FIMV_CODEC_H264_DEC: > >> + WRITEL(OFFSETA(buf_addr1), S5P_FIMV_VERT_NB_MV_ADR); > >> + buf_addr1 += S5P_FIMV_DEC_VERT_NB_MV_SIZE; > >> + buf_size1 -= S5P_FIMV_DEC_VERT_NB_MV_SIZE; > >> + WRITEL(OFFSETA(buf_addr1), S5P_FIMV_VERT_NB_IP_ADR); > >> + buf_addr1 += S5P_FIMV_DEC_NB_IP_SIZE; > >> + buf_size1 -= S5P_FIMV_DEC_NB_IP_SIZE; > >> + break; > >> + case S5P_FIMV_CODEC_MPEG4_DEC: > >> + case S5P_FIMV_CODEC_DIVX311_DEC: > >> + case S5P_FIMV_CODEC_DIVX412_DEC: > >> + case S5P_FIMV_CODEC_DIVX502_DEC: > > Best regards, > -- > Kamil Debski > Linux Platform Group > Samsung Poland R&D Center -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html