RE: [PATCH 3/4] MFC: Add MFC 5.1 V4L2 driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



>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.


>> +	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.

>> +	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.

>> +
>> +/* 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux