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

[Jaeryul Oh <jaeryul.oh@samsung.com>]

Thank you for your reply about my comments. 
Refer to as below.

k.debski@samsung.com wrote:
> Hi, Peter.
> 
> > I have some first comments about this code.
> > and Generally, Making patch set separately will be more helpful to
> > everyone.
> 
> Thank you for your comments.
> 
> > k.debski@samsung.com wrote:
> > > Multi Format Codec 5.1 is a module available on S5PC110 and S5PC210
> > > Samsung SoCs. Hardware is capable of handling a range of video codecs
> > > and this driver provides V4L2 interface for video decoding.
> > >
> > > Signed-off-by: Kamil Debski <k.debski@samsung.com>
> > > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> 
> [...]
> 
> > >
> > > diff --git a/drivers/media/video/Kconfig
> > b/drivers/media/video/Kconfig
> > > index 6d0bd36..1d0b91e 100644
> > > --- a/drivers/media/video/Kconfig
> > > +++ b/drivers/media/video/Kconfig
> > > @@ -1047,4 +1047,12 @@ config  VIDEO_SAMSUNG_S5P_FIMC
> > >  	  This is a v4l2 driver for the S5P camera interface
> > >  	  (video postprocessor)
> > >
> > > +config VIDEO_SAMSUNG_S5P_MFC
> > > +	tristate "Samsung S5P MFC 5.0 Video Codec"
> > > +	depends on VIDEO_V4L2 && CMA
> > > +	select VIDEOBUF2_CMA
> > > +	default n
> > > +	help
> > > +	    MFC 5.0 driver for V4L2.
> > > +
> > >  endif # V4L_MEM2MEM_DRIVERS
> >
> > What about unifying MFC version as a MFC 5.1, because we are using MFC
> > HW
> > ver.(MFC 5.1.x)
> > in the C110/C210 chip.
> 
> Right, will fix this.
> 
> [...]
> 
> > > diff --git a/drivers/media/video/s5p-mfc/regs-mfc5.h
> > > b/drivers/media/video/s5p-mfc/regs-mfc5.h
> > > new file mode 100644
> > > index 0000000..8c628ad
> > > --- /dev/null
> > > +++ b/drivers/media/video/s5p-mfc/regs-mfc5.h
> > > @@ -0,0 +1,305 @@
> > > +/*
> > > + *
> > > + * Register definition file for Samsung MFC V4.0 & V5.0 Interface
> > (FIMV)
> > > driver
> > > + *
> > > + * Kamil Debski, Copyright (c) 2010 Samsung Electronics
> > > + * 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.
> > > +*/
> >
> > Incorrect comment in the header, MFC V4.0 is not included
> >
> 
> Right again.
> 
> > > +
> > > +#ifndef _REGS_FIMV_H
> > > +#define _REGS_FIMV_H
> > > +
> > > +#define S5P_FIMV_REG_SIZE	(S5P_FIMV_END_ADDR -
S5P_FIMV_START_ADDR)
> > > +#define S5P_FIMV_REG_COUNT	((S5P_FIMV_END_ADDR -
> > > S5P_FIMV_START_ADDR) / 4)
> > > +
> > > +#define S5P_FIMV_START_ADDR	0x0000
> > > +#define S5P_FIMV_END_ADDR	0xe008
> > > +
> > > +#define S5P_FIMV_SW_RESET	0x0000
> > > +#define S5P_FIMV_RISC_HOST_INT	0x0008
> > > +/* Command from HOST to RISC */
> > > +#define S5P_FIMV_HOST2RISC_CMD	0x0030
> > > +#define S5P_FIMV_HOST2RISC_ARG1	0x0034
> > > +#define S5P_FIMV_HOST2RISC_ARG2	0x0038
> > > +#define S5P_FIMV_HOST2RISC_ARG3	0x003c
> > > +#define S5P_FIMV_HOST2RISC_ARG4	0x0040
> > > +/* Command from RISC to HOST */
> > > +#define S5P_FIMV_RISC2HOST_CMD	0x0044
> > > +#define S5P_FIMV_RISC2HOST_ARG1	0x0048
> > > +#define S5P_FIMV_RISC2HOST_ARG2	0x004c
> > > +#define S5P_FIMV_RISC2HOST_ARG3	0x0050
> > > +#define S5P_FIMV_RISC2HOST_ARG4	0x0054
> > > +
> > > +#define S5P_FIMV_FW_VERSION	0x0058
> > > +#define S5P_FIMV_SYS_MEM_SZ	0x005c
> > > +#define S5P_FIMV_FW_STATUS	0x0080
> > > +/* Memory controller register */
> > > +#define S5P_FIMV_MC_DRAMBASE_ADR_A	0x0508
> > > +#define S5P_FIMV_MC_DRAMBASE_ADR_B	0x050c
> > > +#define S5P_FIMV_MC_STATUS	0x0510
> > > +
> > > +/***** In case of 2 master *****/
> >
> > This comment(In case of 2 master) is meaningless. it was used at the
> > beginning
> > step of development.
> 
> Will fix this too.
> 
> > > +/* Common register */
> > > +#define S5P_FIMV_SYS_MEM_ADR	0x0600 /* firmware buffer */
> > > +#define S5P_FIMV_CPB_BUF_ADR	0x0604 /* stream buffer */
> > > +#define S5P_FIMV_DESC_BUF_ADR	0x0608 /* descriptor buffer */
> > > +/* H264 decoding */
> > > +#define S5P_FIMV_VERT_NB_MV_ADR	0x068c /* vertical neighbor motion
> > > vector */
> > > +#define S5P_FIMV_VERT_NB_IP_ADR	0x0690 /* neighbor pixels for
> intra
> 
> [...]
> 
> > displayed pic
> > > */
> > > +#define S5P_FIMV_SI_DISPLAY_C_ADR 0x2014 /* chroma address of
> > displayed
> > > pic */
> > > +#define S5P_FIMV_SI_DEC_FRM_SIZE 0x2018 /* the number of frames
> > decoded
> > > */
> 
> > S5P_FIMV_SI_DEC_FRM_SIZE does actually means
> > "consumed number of bytes to decode a frame"
> 
> Ok, I've changed the name of the define to reflect the purpose of the
> register.
> 
> > > +#define S5P_FIMV_SI_DISPLAY_STATUS 0x201c /* status of decoded
> > picture */
> > > +#define S5P_FIMV_SI_FRAME_TYPE	0x2020 /* frame type such as
> > skip/I/P/B
> > > */
> > > +
> > > +#define S5P_FIMV_SI_CH0_SB_ST_ADR	0x2044 /* start addr of
> > stream buf
> > > */
> > > +#define S5P_FIMV_SI_CH0_SB_FRM_SIZE	0x2048 /* size of stream
> buf
> > */
> > > +#define S5P_FIMV_SI_CH0_DESC_ADR	0x204c /* addr of descriptor buf
> > > */
> > > +/* Encoder for MPEG4 */
> > > +#define S5P_FIMV_ENC_MPEG4_QUART_PXL	0xe008 /* qpel
> interpolation
> > ctrl
> > > */
> > > +
> > > +/* Additional */
> > > +#define S5P_FIMV_SI_CH0_DPB_CONF_CTRL   0x2068 /* DPB Config Control
> > > Register */
> > > +#define S5P_FIMV_SI_CH0_RELEASE_BUF     0x2060 /* DPB release buffer
> > > register */
> > > +#define S5P_FIMV_SI_CH0_HOST_WR_ADR	0x2064
> >
> > S5P_FIMV_SI_CH0_HOST_WR_ADR means 'address of shared memory'
> > if comments is needed
> 
> Added the comment.
> 
> [...]
> 
> > > diff --git a/drivers/media/video/s5p-mfc/s5p_mfc.c
> > > b/drivers/media/video/s5p-mfc/s5p_mfc.c
> > > new file mode 100644
> > > index 0000000..f549ab6
> > > --- /dev/null
> > > +++ b/drivers/media/video/s5p-mfc/s5p_mfc.c
> 
> [...]
> 
> > > +
> > > +void s5p_mfc_error_cleanup_queue(struct list_head *lh, \
> > > +						struct vb2_queue *vq)
> > > +{
> > > +	struct vb2_buffer *b;
> > > +	int i;
> > > +	spin_lock(&dev->irqlock);
> > > +	while (!list_empty(lh)) {
> > > +		b = list_entry(lh->next, struct vb2_buffer, drv_entry);
> > > +		for (i = 0; i < b->num_planes; i++)
> > > +			vb2_set_plane_payload(b, i, 0);
> > > +		spin_unlock(&dev->irqlock);
> > > +		vb2_buffer_done(b, VB2_BUF_STATE_ERROR);
> > > +		spin_lock(&dev->irqlock);
> > > +		list_del(&b->drv_entry);
> > > +	}
> > > +	spin_unlock(&dev->irqlock);
> > > +}
> >
> > How about adding new line b/w local var defintions & running code thru
> > whole function for readability.
> 
> Ok, can do that.
> 
> >
> > > +
> > > +void s5p_mfc_watchdog(unsigned long arg)
> > > +{
> > > +	if (test_bit(0, &dev->hw_lock))
> > > +		atomic_inc(&dev->watchdog_cnt);
> > > +	if (atomic_read(&dev->watchdog_cnt) >= MFC_WATCHDOG_CNT) {
> > > +		/* This means that hw is busy and no interrupts were
> > > +		 * generated by hw for the Nth time of running this
> > > +		 * watchdog timer. This usually means a serious hw
> > > +		 * error. Now it is time to kill all instances and
> > > +		 * reset the MFC. */
> > > +		mfc_err("Time out during waiting for HW.\n");
> > > +		queue_work(dev->watchdog_workqueue, &dev->watchdog_work);
> > > +	}
> > > +	dev->watchdog_timer.expires +=
> > > msecs_to_jiffies(MFC_WATCHDOG_INTERVAL);
> > > +	add_timer(&dev->watchdog_timer);
> > > +}
> > > +
> 
> [...]
> 
> > > +			if (mutex_locked)
> > > +				mutex_unlock(dev->mfc_mutex);
> > > +			return;
> > > +		}
> > > +	}
> > > +	if (mutex_locked)
> > > +		mutex_unlock(dev->mfc_mutex);
> > > +}
> >
> > Does MFC_NUM_CONTEXTS means max num of contexts ? what about
> > MFC_MAX_CONTEXT_NUM ?
> > and this number is absolutely dependent on what max size used,
> > what kinds of codec used, etc so, you had better use configuration
> > param.
> > for example>
> >    #ifdef CONFIG_VIDEO_MFC_MAX_INSTANCE
> >    #define MFC_MAX_CONTEXT_NUM (CONFIG_VIDEO_MFC_MAX_INSTANCE)
> >    #endif
> 
> I don't know if this is necessary. MFC_NUM_CONTEXTS can be fixed at
> the maximum number allowed by MFC hw: 16. I highly doubt someone
> will open that many contexts. Increasing this number will not
> significantly increase storage space used by MFC if no contexts are
> used. It will only change size of one pointer array
> ( struct s5p_mfc_ctx *ctx[MFC_NUM_CONTEXTS]; ).
> 
In many project, user can open many contexts according to the scenario of
not only phone but also netbook, tablet. 'cause MFC supports multiple
instance.
That's why I suggested this param. be configurable.
> >
> > > +
> > > +/* Check whether a context should be run on hardware */
> > > +int s5p_mfc_ctx_ready(struct s5p_mfc_ctx *ctx)
> > > +{
> > > +	mfc_debug("s5p_mfc_ctx_ready: src=%d, dst=%d, state=%d\n",
> > > +		  ctx->src_queue_cnt, ctx->dst_queue_cnt, ctx->state);
> > > +	/* Context is to parse header */
> > > +	if (ctx->src_queue_cnt >= 1 && ctx->state ==
> > MFCINST_DEC_GOT_INST)
> > > +		return 1;
> > > +	/* Context is to decode a frame */
> > > +	if (ctx->src_queue_cnt >= 1 && ctx->state == MFCINST_DEC_RUNNING
> > &&
> > > +					ctx->dst_queue_cnt >= ctx-
> > >dpb_count)
> > > +		return 1;
> > > +	/* Context is to return last frame */
> > > +	if (ctx->state == MFCINST_DEC_FINISHING &&
> > > +	    ctx->dst_queue_cnt >= ctx->dpb_count)
> > > +		return 1;
> > > +	/* Context is to set buffers */
> > > +	if (ctx->src_queue_cnt >= 1 &&
> > > +	    ctx->state == MFCINST_DEC_HEAD_PARSED &&
> > > +	    ctx->capture_state == QUEUE_BUFS_MMAPED)
> > > +		return 1;
> > > +	mfc_debug("s5p_mfc_ctx_ready: ctx is not ready.\n");
> > > +	return 0;
> > > +}
> > > +
> 
> [...]
> 
> > > +/* Reqeust buffers */
> > > +static int vidioc_reqbufs(struct file *file, void *priv,
> > > +					  struct v4l2_requestbuffers
> > *reqbufs)
> > > +{
> > > +	struct s5p_mfc_ctx *ctx = priv;
> > > +	int ret = 0;
> > > +	mfc_debug("vidioc_reqbufs++\n");
> > > +	mfc_debug("Memory type: %d\n", reqbufs->memory);
> > > +	if (reqbufs->memory != V4L2_MEMORY_MMAP) {
> > > +		mfc_err("Only V4L2_MEMORY_MAP is supported.\n");
> > > +		return -EINVAL;
> > > +	}
> > > +	if (reqbufs->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) {
> > > +		/* Can only request buffers after an instance has been
> > > opened.*/
> > > +		if (ctx->state == MFCINST_DEC_GOT_INST) {
> > > +			/* Decoding */
> > > +			if (ctx->output_state != QUEUE_FREE) {
> > > +				mfc_err("Bufs have already been
> > requested.\n");
> > > +				return -EINVAL;
> > > +			}
> > > +			ret = vb2_reqbufs(&ctx->vq_src, reqbufs);
> > > +			if (ret) {
> > > +				mfc_err("vb2_reqbufs on output failed.\n");
> > > +				return ret;
> > > +			}
> > > +			mfc_debug("vb2_reqbufs: %d\n", ret);
> > > +			ctx->output_state = QUEUE_BUFS_REQUESTED;
> > > +		}
> > > +	}
> > > +	if (reqbufs->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) {
> > > +		if (ctx->capture_state != QUEUE_FREE) {
> > > +			mfc_err("Bufs have already been requested.\n");
> > > +			return -EINVAL;
> > > +		}
> > > +		ret = vb2_reqbufs(&ctx->vq_dst, reqbufs);
> > > +		if (ret) {
> > > +			mfc_err("vb2_reqbufs on output failed.\n");
> >
> > error message should be mfc_err("vb2_reqbufs on capture failed.\n");
> 
> Right.
> 
> >
> > > +			return ret;
> > > +		}
> > > +		if (reqbufs->count < ctx->dpb_count) {
> > > +			mfc_err("Not enough buffers allocated.\n");
> > > +			reqbufs->count = 0;
> > > +			ret = vb2_reqbufs(&ctx->vq_dst, reqbufs);
> > > +			return -ENOMEM;
> > > +		}
> > > +		ctx->total_dpb_count = reqbufs->count;
> >
> > if 'ctx->total_dpb_count = reqbufs->count;' is here in code, Hmm....
> > let's suppose that reqbufs->count = 16 & ctx->dpb_count = 10,
> > in that case, ctx->total_dpb_count is 16,
> > but *buf_count(in the s5p_mfc_buf_negotiate()) is 15
> > so, it is not matched & it affect __vb2_queue_alloc(xx, num_buffers,
> > xx)
> > i think, is is due to location of 'the ctx->total_dpb_count = reqbufs-
> > >count;'
> > and No warning message for exceeding MFC_MAX_EXTRA_DPB
> > what do you think about that ?
> 
> The number of buffers requested by the user is stored in reqbufs->count.
> Then vb2_reqbufs() is called, which calls the s5p_mfc_buf_negotiate()
> callback function. It may change the number of buffers to allocate.
> Then vb2_reqbufs() tries to allocate the number of buffers returned in
> *buf_count, but it may allocate less if it runs out of memory. If so it
> will adjust reqbufs->count accordingly.
> 
> Now the driver has to check if reqbufs->count is less than the minimum
> number of buffers required by MFC. If so it has to free the allocated
> buffers and fail with -ENOMEM, as further processing is impossible.
> The number of allocated buffers can be smaller than *buf_count set in
> s5p_mfc_buf_negotiate() yet still enough for MFC to work. So I think
> this place to set ctx->total_dpb_count is good.
> 
> In the scenario you have given as an example the s5p_mfc_buf_negotiate()
> would change the value given by the user application from 16 to 15 and
> this would be the number that vb2_reqbufs tries to allocate. If there
> is little memory then it could allocate only 14 buffers, 15 if there is
> enough memory. Then ctx->total_dpb_count would be set to reqbufs->count
> after call to vb2_reqbufs (which is 14 or 15 depending on the available
> memory).
Yes, what you said looks right. I missed some code sequence.
Thanks
> 
> > > +		ret = s5p_mfc_alloc_dec_buffers(ctx);
> > > +		if (ret) {
> > > +			mfc_err("Failed to allocate decoding buffers.\n");
> > > +			reqbufs->count = 0;
> > > +			ret = vb2_reqbufs(&ctx->vq_dst, reqbufs);
> > > +			return -ENOMEM;
> > > +		}
> > > +		ctx->capture_state = QUEUE_BUFS_REQUESTED;
> > > +	}
> > > +	mfc_debug("vidioc_reqbufs--\n");
> > > +	return ret;
> > > +}
> > > +
> 
> [...]
> 
> > > +/* Handle frame decoding interrupt */
> > > +static void s5p_mfc_handle_frame_int(struct s5p_mfc_ctx *ctx, \
> > > +					unsigned int reason, unsigned int
> > err)
> > > +{
> 
> [...]
> 
> > > +	/* A frame has been decoded and is in the buffer  */
> > > +	if (dst_frame_status == S5P_FIMV_DEC_STATUS_DISPLAY_ONLY ||
> > > +	    dst_frame_status == S5P_FIMV_DEC_STATUS_DECODING_DISPLAY) {
> > > +		ctx->sequence++;
> > > +		/* If frame is same as previous then skip and do not
> > dequeue
> > > */
> > > +		if (dec_frame_type !=  S5P_FIMV_DECODE_FRAME_SKIPPED) {
> > > +		/* The MFC returns address of the buffer, now we have to
> > > +		 * check which videobuf does it correspond to */
> > > +		list_for_each_entry(dst_buf, &ctx->dst_queue, drv_entry) {
> > > +			mfc_debug("Listing: %d\n", dst_buf->v4l2_buf.index);
> > > +			/* This is the buffer we're looking for */
> > > +			mfc_debug("paddr: %p mfc: %p\n",
> > > +					(void *)vb2_plane_paddr(dst_buf, 1),
> > > +							(void
> > *)dst_ret_addr);
> >
> > what is the purpose of this debug message ?
> > mfc_debug("paddr: %p mfc: %p\n",(void *)vb2_plane_paddr(dst_buf, 1),
> >                                                                 (void
> > *)dst_ret_addr);
> > do you want to compare Y address?
> > it should be like this, vb2_plane_paddr(dst_buf, 0) with dst_ret_addr.
> > and dst_y_ret_addr is better than dst_ret_addr,
> > making debug msg clear is important 'cause codec debugging is not easy.
> 
> You're right that this debug is unclear and displays the wrong value.
> Changing dst_ret_addr name to dst_ret_addr_y seems very reasonable to
> make the code more clear. I should remove many debug messages before
> posting the final version. They were very useful during development,
> but they may clutter the code too much.
Let's get make better together
> 
> > > +			if (vb2_plane_paddr(dst_buf, 0) == dst_ret_addr) {
> > > +				list_del(&dst_buf->drv_entry);
> > > +				ctx->dst_queue_cnt--;
> > > +				mfc_debug("Flag before: %lx (%d)\n",
> > > +							ctx->dec_dst_flag,
> > > +						dst_buf->v4l2_buf.index);
> > > +				dst_buf->v4l2_buf.sequence = ctx->sequence;
> 
> 
> [...]
> 
> > > +/* Interrupt processing */
> > > +static irqreturn_t s5p_mfc_irq(int irq, void *priv)
> > > +{
> > > +	struct vb2_buffer *src_buf;
> > > +	struct s5p_mfc_ctx *ctx;
> > > +	unsigned int reason;
> > > +	unsigned int err;
> > > +	mfc_debug("s5p_mfc_irq++\n");
> > > +	/* Reset the timeout watchdog */
> > > +	atomic_set(&dev->watchdog_cnt, 0);
> > > +	ctx = dev->ctx[dev->curr_ctx];
> > > +	/* Get the reason of interrupt and the error code */
> > > +	reason = s5p_mfc_get_int_reason();
> > > +	err = s5p_mfc_get_int_err();
> > > +	mfc_debug("Int reason: %d (error: %08x)\n", reason, err);
> > > +	switch (reason) {
> > > +	case S5P_FIMV_R2H_CMD_DECODE_ERR_RET:
> > > +		/* An error has occured */
> > > +		if (ctx->state == MFCINST_DEC_RUNNING && err >= 145)
> >
> > What about using MACRO instead of using 145(no)
> 
> Yes, will definitely change this.
> 
> [...]
> 
> > > +/* MFC probe function */
> > > +static int s5p_mfc_probe(struct platform_device *pdev)
> > > +{
> > > +	struct video_device *vfd;
> > > +	struct resource *res;
> > > +	int ret = -ENOENT;
> > > +	size_t size;
> > > +	mfc_debug("s5p_mfc_probe++\n");
> > > +	dev = kzalloc(sizeof *dev, GFP_KERNEL);
> > > +	if (!dev) {
> > > +		dev_err(&pdev->dev, "Not enough memoty for MFC device.\n");
> >
> > incorrect spelling (memoty)
> 
> Yes, thanks.
> 
> [...]
> 
> > > diff --git a/drivers/media/video/s5p-mfc/s5p_mfc_logmsg.h
> > > b/drivers/media/video/s5p-mfc/s5p_mfc_logmsg.h
> > > new file mode 100644
> > > index 0000000..90fa84c
> > > --- /dev/null
> > > +++ b/drivers/media/video/s5p-mfc/s5p_mfc_logmsg.h
> 
> [...]
> 
> > > +/* Uncomment the line below do enable debug messages */
> > > +/* #define CONFIG_VIDEO_MFC50_DEBUG */
> >
> > why don't you make configuration(CONFIG_VIDEO_MFC51_DEBUG)
> > in the Kconfig.
> 
> I am planning to do this as a module parameter, so it could
> be set when loading the module.
> 
> Best regards,
> Kamil Debski
>