[PATCH 0/3] Various fixes for s5p-mfc driver

[PATCH 0/3] Various fixes for s5p-mfc driver

[Nicolas Dufresne]

This patchset fixes ability to drain the decoder due to use of wrong
enumeration name and fixes implementation of display delay controls
for MFC firmware v6 and higher.

Note that there is no need in the display delay fix for trying to be
backward compatible with what the comment was saying since the control
properties was preventing it. There was basically no way other then
setting a large delay value to get the frames in display order.

Nicolas Dufresne (3):
  s5p-mfc-v6+: Use display_delay_enable CID
  s5p-mfc-dec: Don't use encoder stop command
  media-doc: Fix MFC display delay control doc

 Documentation/DocBook/media/v4l/controls.xml    | 11 +++++------
 drivers/media/platform/s5p-mfc/s5p_mfc_dec.c    |  2 +-
 drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c |  6 +-----
 3 files changed, 7 insertions(+), 12 deletions(-)

-- 
2.1.0

[PATCH 1/3] s5p-mfc-v6+: Use display_delay_enable CID

[Nicolas Dufresne]

The MFC driver has two controls, DISPLAY_DELAY and DISPLAY_DELAY_ENABLE
that allow forcing the decoder to return a decoded frame sooner
regardless of the order. The added support for firmware version 6 and
higher was not taking into account the DISPLAY_DELAY_ENABLE boolean.
Instead it had a comment stating that DISPLAY_DELAY should be set to a
negative value to disable it. This is not possible since the control
range is from 0 to 65535. This feature was also supposed to be disabled
by default in order to produce frames in display order.

Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
---
 drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c b/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c
index 92032a0..0675515 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c
@@ -1340,11 +1340,7 @@ static int s5p_mfc_init_decode_v6(struct s5p_mfc_ctx *ctx)
 	/* FMO_ASO_CTRL - 0: Enable, 1: Disable */
 	reg |= (fmo_aso_ctrl << S5P_FIMV_D_OPT_FMO_ASO_CTRL_MASK_V6);
 
-	/* When user sets desplay_delay to 0,
-	 * It works as "display_delay enable" and delay set to 0.
-	 * If user wants display_delay disable, It should be
-	 * set to negative value. */
-	if (ctx->display_delay >= 0) {
+	if (ctx->display_delay_enable) {
 		reg |= (0x1 << S5P_FIMV_D_OPT_DDELAY_EN_SHIFT_V6);
 		writel(ctx->display_delay, mfc_regs->d_display_delay);
 	}
-- 
2.1.0

RE: [PATCH 1/3] s5p-mfc-v6+: Use display_delay_enable CID

[Kamil Debski]

> -----Original Message-----
> From: Nicolas Dufresne [mailto:nicolas.dufresne@collabora.com]
> Sent: Monday, December 15, 2014 10:11 PM
> To: linux-media@vger.kernel.org
> Cc: Kamil Debski; Arun Kumar K; Nicolas Dufresne
> Subject: [PATCH 1/3] s5p-mfc-v6+: Use display_delay_enable CID
> 
> The MFC driver has two controls, DISPLAY_DELAY and DISPLAY_DELAY_ENABLE
> that allow forcing the decoder to return a decoded frame sooner
> regardless of the order. The added support for firmware version 6 and
> higher was not taking into account the DISPLAY_DELAY_ENABLE boolean.
> Instead it had a comment stating that DISPLAY_DELAY should be set to a
> negative value to disable it. This is not possible since the control
> range is from 0 to 65535. This feature was also supposed to be disabled
> by default in order to produce frames in display order.
> 
> Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>

Acked-by: Kamil Debski <k.debski@samsung.com>

> ---
>  drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c
> b/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c
> index 92032a0..0675515 100644
> --- a/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c
> +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c
> @@ -1340,11 +1340,7 @@ static int s5p_mfc_init_decode_v6(struct
> s5p_mfc_ctx *ctx)
>  	/* FMO_ASO_CTRL - 0: Enable, 1: Disable */
>  	reg |= (fmo_aso_ctrl << S5P_FIMV_D_OPT_FMO_ASO_CTRL_MASK_V6);
> 
> -	/* When user sets desplay_delay to 0,
> -	 * It works as "display_delay enable" and delay set to 0.
> -	 * If user wants display_delay disable, It should be
> -	 * set to negative value. */
> -	if (ctx->display_delay >= 0) {
> +	if (ctx->display_delay_enable) {
>  		reg |= (0x1 << S5P_FIMV_D_OPT_DDELAY_EN_SHIFT_V6);
>  		writel(ctx->display_delay, mfc_regs->d_display_delay);
>  	}
> --
> 2.1.0

[PATCH 2/3] s5p-mfc-dec: Don't use encoder stop command

[Nicolas Dufresne]

The decoder should handle V4L2_DEC_CMD_STOP to trigger drain,
but it currently expecting V4L2_ENC_CMD_STOP.

Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
---
 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 99e2e84..98304fc 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
@@ -816,7 +816,7 @@ static int vidioc_decoder_cmd(struct file *file, void *priv,
 	unsigned long flags;
 
 	switch (cmd->cmd) {
-	case V4L2_ENC_CMD_STOP:
+	case V4L2_DEC_CMD_STOP:
 		if (cmd->flags != 0)
 			return -EINVAL;
 
-- 
2.1.0

RE: [PATCH 2/3] s5p-mfc-dec: Don't use encoder stop command

[Kamil Debski]

> -----Original Message-----
> From: Nicolas Dufresne [mailto:nicolas.dufresne@collabora.com]
> Sent: Monday, December 15, 2014 10:11 PM
> To: linux-media@vger.kernel.org
> Cc: Kamil Debski; Arun Kumar K; Nicolas Dufresne
> Subject: [PATCH 2/3] s5p-mfc-dec: Don't use encoder stop command
> 
> The decoder should handle V4L2_DEC_CMD_STOP to trigger drain, but it
> currently expecting V4L2_ENC_CMD_STOP.
> 
> Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>

Acked-by: Kamil Debski <k.debski@samsung.com>

> ---
>  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 99e2e84..98304fc 100644
> --- a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
> +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
> @@ -816,7 +816,7 @@ static int vidioc_decoder_cmd(struct file *file,
> void *priv,
>  	unsigned long flags;
> 
>  	switch (cmd->cmd) {
> -	case V4L2_ENC_CMD_STOP:
> +	case V4L2_DEC_CMD_STOP:
>  		if (cmd->flags != 0)
>  			return -EINVAL;
> 
> --
> 2.1.0

[PATCH 3/3] media-doc: Fix MFC display delay control doc

[Nicolas Dufresne]

The V4L2_CID_MPEG_MFC51_VIDEO_DECODER_H264_DISPLAY_DELAY_ENABLE control
is a boolean but was documented as a integer. The documentation was
also slightly miss-leading.

Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
---
 Documentation/DocBook/media/v4l/controls.xml | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/Documentation/DocBook/media/v4l/controls.xml b/Documentation/DocBook/media/v4l/controls.xml
index e013e4b..4e9462f 100644
--- a/Documentation/DocBook/media/v4l/controls.xml
+++ b/Documentation/DocBook/media/v4l/controls.xml
@@ -2692,12 +2692,11 @@ in the S5P family of SoCs by Samsung.
 	      <row><entry></entry></row>
 	      <row>
 		<entry spanname="id"><constant>V4L2_CID_MPEG_MFC51_VIDEO_DECODER_H264_DISPLAY_DELAY_ENABLE</constant>&nbsp;</entry>
-		<entry>integer</entry>
-	      </row><row><entry spanname="descr">If the display delay is enabled then the decoder has to return a
-CAPTURE buffer after processing a certain number of OUTPUT buffers. If this number is low, then it may result in
-buffers not being dequeued in display order. In addition hardware may still use those buffers as reference, thus
-application should not write to those buffers. This feature can be used for example for generating thumbnails of videos.
-Applicable to the H264 decoder.
+		<entry>boolean</entry>
+	      </row><row><entry spanname="descr">If the display delay is enabled then the decoder is forced to return a
+CAPTURE buffer (decoded frame) after processing a certain number of OUTPUT buffers. The delay can be set through
+<constant>V4L2_CID_MPEG_MFC51_VIDEO_DECODER_H264_DISPLAY_DELAY</constant>. This feature can be used for example
+for generating thumbnails of videos. Applicable to the H264 decoder.
 	      </entry>
 	      </row>
 	      <row><entry></entry></row>
-- 
2.1.0

RE: [PATCH 3/3] media-doc: Fix MFC display delay control doc

[Kamil Debski]

> -----Original Message-----
> From: Nicolas Dufresne [mailto:nicolas.dufresne@collabora.com]
> Sent: Monday, December 15, 2014 10:11 PM
> To: linux-media@vger.kernel.org
> Cc: Kamil Debski; Arun Kumar K; Nicolas Dufresne
> Subject: [PATCH 3/3] media-doc: Fix MFC display delay control doc
> 
> The V4L2_CID_MPEG_MFC51_VIDEO_DECODER_H264_DISPLAY_DELAY_ENABLE control
> is a boolean but was documented as a integer. The documentation was
> also slightly miss-leading.
> 
> Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>

Acked-by: Kamil Debski <k.debski@samsung.com>

> ---
>  Documentation/DocBook/media/v4l/controls.xml | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/DocBook/media/v4l/controls.xml
> b/Documentation/DocBook/media/v4l/controls.xml
> index e013e4b..4e9462f 100644
> --- a/Documentation/DocBook/media/v4l/controls.xml
> +++ b/Documentation/DocBook/media/v4l/controls.xml
> @@ -2692,12 +2692,11 @@ in the S5P family of SoCs by Samsung.
>  	      <row><entry></entry></row>
>  	      <row>
>  		<entry
> spanname="id"><constant>V4L2_CID_MPEG_MFC51_VIDEO_DECODER_H264_DISPLAY_
> DELAY_ENABLE</constant>&nbsp;</entry>
> -		<entry>integer</entry>
> -	      </row><row><entry spanname="descr">If the display delay is
> enabled then the decoder has to return a
> -CAPTURE buffer after processing a certain number of OUTPUT buffers. If
> this number is low, then it may result in -buffers not being dequeued
> in display order. In addition hardware may still use those buffers as
> reference, thus -application should not write to those buffers. This
> feature can be used for example for generating thumbnails of videos.
> -Applicable to the H264 decoder.
> +		<entry>boolean</entry>
> +	      </row><row><entry spanname="descr">If the display delay is
> +enabled then the decoder is forced to return a CAPTURE buffer (decoded
> +frame) after processing a certain number of OUTPUT buffers. The delay
> +can be set through
> <constant>V4L2_CID_MPEG_MFC51_VIDEO_DECODER_H264_DISPLAY_DELAY</constan
> t>. This feature can be used for example for generating thumbnails of
> videos. Applicable to the H264 decoder.
>  	      </entry>
>  	      </row>
>  	      <row><entry></entry></row>
> --
> 2.1.0

Re: [PATCH 0/3] Various fixes for s5p-mfc driver

[Nicolas Dufresne]

Just a friendly reminder that this patch is pending review ;-P

cheers,
Nicolas

Le 2014-12-15 16:10, Nicolas Dufresne a écrit :
> This patchset fixes ability to drain the decoder due to use of wrong
> enumeration name and fixes implementation of display delay controls
> for MFC firmware v6 and higher.
>
> Note that there is no need in the display delay fix for trying to be
> backward compatible with what the comment was saying since the control
> properties was preventing it. There was basically no way other then
> setting a large delay value to get the frames in display order.
>
> Nicolas Dufresne (3):
>    s5p-mfc-v6+: Use display_delay_enable CID
>    s5p-mfc-dec: Don't use encoder stop command
>    media-doc: Fix MFC display delay control doc
>
>   Documentation/DocBook/media/v4l/controls.xml    | 11 +++++------
>   drivers/media/platform/s5p-mfc/s5p_mfc_dec.c    |  2 +-
>   drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c |  6 +-----
>   3 files changed, 7 insertions(+), 12 deletions(-)
>

RE: [PATCH 0/3] Various fixes for s5p-mfc driver

[Kamil Debski]

Hi Nicolas,

I usually don't ack patches that I plan to take into my tree, but it might
be a good idea to let know the submitter that patches are good. 

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


> -----Original Message-----
> From: Nicolas Dufresne [mailto:nicolas.dufresne@collabora.com]
> Sent: Thursday, January 08, 2015 12:15 AM
> To: linux-media@vger.kernel.org
> Cc: Kamil Debski; Arun Kumar K
> Subject: Re: [PATCH 0/3] Various fixes for s5p-mfc driver
> 
> Just a friendly reminder that this patch is pending review ;-P
> 
> cheers,
> Nicolas
> 
> Le 2014-12-15 16:10, Nicolas Dufresne a écrit :
> > This patchset fixes ability to drain the decoder due to use of wrong
> > enumeration name and fixes implementation of display delay controls
> > for MFC firmware v6 and higher.
> >
> > Note that there is no need in the display delay fix for trying to be
> > backward compatible with what the comment was saying since the
> control
> > properties was preventing it. There was basically no way other then
> > setting a large delay value to get the frames in display order.
> >
> > Nicolas Dufresne (3):
> >    s5p-mfc-v6+: Use display_delay_enable CID
> >    s5p-mfc-dec: Don't use encoder stop command
> >    media-doc: Fix MFC display delay control doc
> >
> >   Documentation/DocBook/media/v4l/controls.xml    | 11 +++++------
> >   drivers/media/platform/s5p-mfc/s5p_mfc_dec.c    |  2 +-
> >   drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c |  6 +-----
> >   3 files changed, 7 insertions(+), 12 deletions(-)
> >

Re: [PATCH 0/3] Various fixes for s5p-mfc driver

[Nicolas Dufresne]

Le 2015-01-08 07:51, Kamil Debski a écrit :
> I usually don't ack patches that I plan to take into my tree, but it might
> be a good idea to let know the submitter that patches are good
Thanks for letting me know, I may ask informally then next time.

cheers,
Nicolas