[PATCH 0/4] [media] s5p-mfc: remove a few little bugs in driver

[PATCH 0/4] [media] s5p-mfc: remove a few little bugs in driver

[ayaka]

Thank for review the previous patches, but the current driver still
need the patches below to make it work. I have used gstreamer
to test those patches, it works fine at exynos4412 platform.
As I don't have further platform, I am sure whether it will works
on exynos7, but I think it would be.

The s5p-mfc: Add handling of buffer freeing reqbufs request
comes from chromium project, a developer from that project told me
that he will send this patch to upstream, but I found it not
be merged into mainline, so I send it.

ayaka (4):
  [media] s5p-mfc: Add handling of buffer freeing reqbufs request
  [media] s5p-mfc: remove unnecessary check in try_fmt
  [media] s5p-mfc: don't close instance after free OUTPUT buffers
  [media] s5p-mfc: fix a typo in s5p_mfc_dec

 drivers/media/platform/s5p-mfc/s5p_mfc_dec.c |  3 +--
 drivers/media/platform/s5p-mfc/s5p_mfc_enc.c | 12 +++---------
 2 files changed, 4 insertions(+), 11 deletions(-)

-- 
2.5.0

[PATCH 1/4] [media] s5p-mfc: Add handling of buffer freeing reqbufs request

[ayaka]

The encoder forget the work to call hardware to release its buffers.
This patch comes from chromium project. I just change its code
style.

Signed-off-by: ayaka <ayaka@soulik.info>
---
 drivers/media/platform/s5p-mfc/s5p_mfc_enc.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c b/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
index e65993f..7c9e5f5 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
@@ -1144,7 +1144,10 @@ static int vidioc_reqbufs(struct file *file, void *priv,
 		return -EINVAL;
 	if (reqbufs->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) {
 		if (reqbufs->count == 0) {
+			mfc_debug(2, "Freeing buffers\n");
 			ret = vb2_reqbufs(&ctx->vq_dst, reqbufs);
+			s5p_mfc_hw_call_void(dev->mfc_ops, release_codec_buffers,
+					ctx);
 			ctx->capture_state = QUEUE_FREE;
 			return ret;
 		}
-- 
2.5.0

Re: [PATCH 1/4] [media] s5p-mfc: Add handling of buffer freeing reqbufs request

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2133 bytes --]

Hi ayaka,

[auto build test ERROR on linuxtv-media/master]
[also build test ERROR on v4.5-rc1 next-20160129]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/ayaka/s5p-mfc-Add-handling-of-buffer-freeing-reqbufs-request/20160131-030651
base:   git://linuxtv.org/media_tree.git master
config: xtensa-allyesconfig (attached as .config)
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=xtensa 

All errors (new ones prefixed by >>):

   drivers/media/platform/s5p-mfc/s5p_mfc_enc.c: In function 'vidioc_reqbufs':
>> drivers/media/platform/s5p-mfc/s5p_mfc_enc.c:1141:4: error: implicit declaration of function 's5p_mfc_hw_call_void' [-Werror=implicit-function-declaration]
       s5p_mfc_hw_call_void(dev->mfc_ops, release_codec_buffers,
       ^
>> drivers/media/platform/s5p-mfc/s5p_mfc_enc.c:1141:39: error: 'release_codec_buffers' undeclared (first use in this function)
       s5p_mfc_hw_call_void(dev->mfc_ops, release_codec_buffers,
                                          ^
   drivers/media/platform/s5p-mfc/s5p_mfc_enc.c:1141:39: note: each undeclared identifier is reported only once for each function it appears in
   cc1: some warnings being treated as errors

vim +/s5p_mfc_hw_call_void +1141 drivers/media/platform/s5p-mfc/s5p_mfc_enc.c

  1135			(reqbufs->memory != V4L2_MEMORY_USERPTR))
  1136			return -EINVAL;
  1137		if (reqbufs->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) {
  1138			if (reqbufs->count == 0) {
  1139				mfc_debug(2, "Freeing buffers\n");
  1140				ret = vb2_reqbufs(&ctx->vq_dst, reqbufs);
> 1141				s5p_mfc_hw_call_void(dev->mfc_ops, release_codec_buffers,
  1142						ctx);
  1143				ctx->capture_state = QUEUE_FREE;
  1144				return ret;

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 44066 bytes --]

[PATCH 2/4] [media] s5p-mfc: remove unnecessary check in try_fmt

[ayaka]

We don't need to request the sizeimage or num_planes
in try_fmt.

Signed-off-by: ayaka <ayaka@soulik.info>
---
 drivers/media/platform/s5p-mfc/s5p_mfc_enc.c | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c b/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
index 7c9e5f5..64b92eb 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
@@ -1043,10 +1043,6 @@ static int vidioc_try_fmt(struct file *file, void *priv, struct v4l2_format *f)
 			mfc_err("failed to try output format\n");
 			return -EINVAL;
 		}
-		if (pix_fmt_mp->plane_fmt[0].sizeimage == 0) {
-			mfc_err("must be set encoding output size\n");
-			return -EINVAL;
-		}
 		if ((dev->variant->version_bit & fmt->versions) == 0) {
 			mfc_err("Unsupported format by this MFC version.\n");
 			return -EINVAL;
@@ -1060,11 +1056,6 @@ static int vidioc_try_fmt(struct file *file, void *priv, struct v4l2_format *f)
 			mfc_err("failed to try output format\n");
 			return -EINVAL;
 		}
-
-		if (fmt->num_planes != pix_fmt_mp->num_planes) {
-			mfc_err("failed to try output format\n");
-			return -EINVAL;
-		}
 		if ((dev->variant->version_bit & fmt->versions) == 0) {
 			mfc_err("Unsupported format by this MFC version.\n");
 			return -EINVAL;
-- 
2.5.0

[PATCH 3/4] [media] s5p-mfc: don't close instance after free OUTPUT buffers

[ayaka]

Free buffers in OUTPUT is quite normal to detect the driver's
buffer capacity, it doesn't mean the application want to close
that mfc instance.

Signed-off-by: ayaka <ayaka@soulik.info>
---
 drivers/media/platform/s5p-mfc/s5p_mfc_dec.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
index aebe4fd..609b17b 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
@@ -474,7 +474,6 @@ static int reqbufs_output(struct s5p_mfc_dev *dev, struct s5p_mfc_ctx *ctx,
 		ret = vb2_reqbufs(&ctx->vq_src, reqbufs);
 		if (ret)
 			goto out;
-		s5p_mfc_close_mfc_inst(dev, ctx);
 		ctx->src_bufs_cnt = 0;
 		ctx->output_state = QUEUE_FREE;
 	} else if (ctx->output_state == QUEUE_FREE) {
-- 
2.5.0

RE: [PATCH 3/4] [media] s5p-mfc: don't close instance after free OUTPUT buffers

[Kamil Debski]

Hi,

> From: ayaka [mailto:ayaka@soulik.info]
> Sent: Saturday, January 30, 2016 7:54 PM
>
> Free buffers in OUTPUT is quite normal to detect the driver's buffer
capacity,
> it doesn't mean the application want to close that mfc instance.
> 
> Signed-off-by: ayaka <ayaka@soulik.info>
> ---
>  drivers/media/platform/s5p-mfc/s5p_mfc_dec.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
> b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
> index aebe4fd..609b17b 100644
> --- a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
> +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
> @@ -474,7 +474,6 @@ static int reqbufs_output(struct s5p_mfc_dev *dev,
> struct s5p_mfc_ctx *ctx,
>  		ret = vb2_reqbufs(&ctx->vq_src, reqbufs);
>  		if (ret)
>  			goto out;
> -		s5p_mfc_close_mfc_inst(dev, ctx);
>  		ctx->src_bufs_cnt = 0;
>  		ctx->output_state = QUEUE_FREE;
>  	} else if (ctx->output_state == QUEUE_FREE) {

What exactly do you mean by "detecting buffer capacity"  ? Is it the max
number of buffer
that can be allocated?

Anyway, if the instance is not closed, then in a consecutive call to reqbufs
(with cound != 0)
the instance will be opened for a second time. So either the instance has to
be closed, or
it should be opened in another place.

Best wishes,
-- 
Kamil Debski
Samsung R&D Institute Poland

Re: [PATCH 3/4] [media] s5p-mfc: don't close instance after free OUTPUT buffers

[ayaka]

I reviewed the code again

2016-02-04 22:23, Kamil Debski wrote:
> Hi,
>
>> From: ayaka [mailto:ayaka@soulik.info]
>> Sent: Saturday, January 30, 2016 7:54 PM
>>
>> Free buffers in OUTPUT is quite normal to detect the driver's buffer
> capacity,
>> it doesn't mean the application want to close that mfc instance.
>>
>> Signed-off-by: ayaka <ayaka@soulik.info>
>> ---
>>   drivers/media/platform/s5p-mfc/s5p_mfc_dec.c | 1 -
>>   1 file changed, 1 deletion(-)
>>
>> diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
>> b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
>> index aebe4fd..609b17b 100644
>> --- a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
>> +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
>> @@ -474,7 +474,6 @@ static int reqbufs_output(struct s5p_mfc_dev *dev,
>> struct s5p_mfc_ctx *ctx,
>>   		ret = vb2_reqbufs(&ctx->vq_src, reqbufs);
>>   		if (ret)
>>   			goto out;
>> -		s5p_mfc_close_mfc_inst(dev, ctx);
>>   		ctx->src_bufs_cnt = 0;
>>   		ctx->output_state = QUEUE_FREE;
>>   	} else if (ctx->output_state == QUEUE_FREE) {
> What exactly do you mean by "detecting buffer capacity"  ? Is it the max
> number of buffer
> that can be allocated?
>
> Anyway, if the instance is not closed, then in a consecutive call to reqbufs
> (with cound != 0)
> the instance will be opened for a second time. So either the instance has to
> be closed, or
Does the s5p_mfc_open_mfc_inst() open a new instance?In that case if 
reqbufs->count == 0, it won't open a new instance. Also before allocate 
buffers, it will check whether the instance has been opened.
So I think the instance won't be opened twice.
> it should be opened in another place.
>
> Best wishes,

[PATCH 4/4] [media] s5p-mfc: fix a typo in s5p_mfc_dec

[ayaka]

It is a cosmetic commit.

Signed-off-by: ayaka <ayaka@soulik.info>
---
 drivers/media/platform/s5p-mfc/s5p_mfc_dec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
index 609b17b..cfedf89 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
@@ -572,7 +572,7 @@ static int vidioc_reqbufs(struct file *file, void *priv,
 	struct s5p_mfc_ctx *ctx = fh_to_ctx(priv);
 
 	if (reqbufs->memory != V4L2_MEMORY_MMAP) {
-		mfc_err("Only V4L2_MEMORY_MAP is supported\n");
+		mfc_err("Only V4L2_MEMORY_MMAP is supported\n");
 		return -EINVAL;
 	}
 
-- 
2.5.0