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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
