RE: [PATCH] s5p-mfc: encoder could free buffers

2014-07-03 Thread Kamil Debski
Hi Ayaka,

Thank you for your patch, however I see some things that need to be
corrected.

 From: linux-media-ow...@vger.kernel.org [mailto:linux-media-
 ow...@vger.kernel.org] On Behalf Of ayaka
 Sent: Thursday, June 26, 2014 4:15 PM
 To: linux-media@vger.kernel.org
 Cc: kyungmin.p...@samsung.com; jtp.p...@samsung.com;
 m.che...@samsung.com
 Subject: [PATCH] s5p-mfc: encoder could free buffers
 
 [PATCH] s5p-mfc: encoder could free buffers

The above line is not necessary.

 
 The patch is necessary or the buffers could be freeed but it would

Typo - should be freed

 break the state of encoder in s5p-mfc. It is also need by some
 application which would detect the buffer allocation way, like
 gstreamer.

I would rephrase the whole commit message. Maybe something along the lines
Add handling of buffer freeing reqbufs request to the encoder of s5p-mfc.

 ---
 drivers/media/platform/s5p-mfc/s5p_mfc_enc.c | 8 
 1 file changed, 8 insertions(+)
 
 diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
 b/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
 index d26b248..1a7518f 100644
 --- a/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
 +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
 @@ -1166,6 +1166,10 @@ static int vidioc_reqbufs(struct file *file,
 void *priv, mfc_err(error in vb2_reqbufs() for E(D)\n); return ret; }
 + if (reqbufs-count == 0) {
 + mfc_debug(2, Freeing buffers\n);
 + return ret;
 + }

Indentation is wrong here, please correct.
Also, I think you should set ctx-capture_state to QUEUE_FREE.

 ctx-capture_state = QUEUE_BUFS_REQUESTED;
 
 ret = s5p_mfc_hw_call(ctx-dev-mfc_ops,
 @@ -1200,6 +1204,10 @@ static int vidioc_reqbufs(struct file *file,
 void *priv, mfc_err(error in vb2_reqbufs() for E(S)\n); return ret; }
 + if (reqbufs-count == 0) {
 + mfc_debug(2, Freeing buffers\n);
 + return ret;
 + }

Indentation is wrong here, please correct.
Also, I think you should set ctx-capture_state to QUEUE_FREE.

 ctx-output_state = QUEUE_BUFS_REQUESTED;
 } else {
 mfc_err(invalid buf type\n);

In addition to the above, for capture this patch will not work. The
following
check will prevent buffers from being freed:

if (ctx-capture_state != QUEUE_FREE) {
 mfc_err(invalid capture state: %d\n,
 ctx-capture_state);
 return -EINVAL;
 }

Also, you should provide a proper Signed-off-by with your full name and
email
address. Did you use git send-email to send this patch?

Best wishes,
-- 
Kamil Debski
Samsung RD Institute Poland


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


[PATCH] s5p-mfc: encoder could free buffers

2014-06-26 Thread ayaka
[PATCH] s5p-mfc: encoder could free buffers

The patch is necessary or the buffers could be freeed but
it would break the state of encoder in s5p-mfc. It is also need
by some application which would detect the buffer allocation
way, like gstreamer.
---
drivers/media/platform/s5p-mfc/s5p_mfc_enc.c | 8 
1 file changed, 8 insertions(+)

diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
b/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
index d26b248..1a7518f 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
@@ -1166,6 +1166,10 @@ static int vidioc_reqbufs(struct file *file, void
*priv,
mfc_err(error in vb2_reqbufs() for E(D)\n);
return ret;
}
+ if (reqbufs-count == 0) {
+ mfc_debug(2, Freeing buffers\n);
+ return ret;
+ }
ctx-capture_state = QUEUE_BUFS_REQUESTED;

ret = s5p_mfc_hw_call(ctx-dev-mfc_ops,
@@ -1200,6 +1204,10 @@ static int vidioc_reqbufs(struct file *file, void
*priv,
mfc_err(error in vb2_reqbufs() for E(S)\n);
return ret;
}
+ if (reqbufs-count == 0) {
+ mfc_debug(2, Freeing buffers\n);
+ return ret;
+ }
ctx-output_state = QUEUE_BUFS_REQUESTED;
} else {
mfc_err(invalid buf type\n);
-- 
1.9.1

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