Re: [PATCH] [media] s5p-mfc: Add Horizontal and Vertical search range for Video Macro Blocks
Hi, On 23/01/14 11:11, Kamil Debski wrote: >> diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c >> > b/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c >> > index 4ff3b6c..a02e7b8 100644 >> > --- a/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c >> > +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c >> > @@ -208,6 +208,24 @@ static struct mfc_control controls[] = { >> >.default_value = 0, >> >}, >> >{ >> > + .id = V4L2_CID_MPEG_VIDEO_HORZ_SEARCH_RANGE, >> > + .type = V4L2_CTRL_TYPE_INTEGER, >> > + .name = "horizontal search range of video macro block", > > This too should be property capitalised. Please mention the motion vectors > too. And additionally length of the name string should not exceed 31 characters. -- Thanks, Sylwester -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] [media] s5p-mfc: Add Horizontal and Vertical search range for Video Macro Blocks
Hi Amit, > From: Amit Grover [mailto:amit.gro...@samsung.com] > Sent: Monday, December 30, 2013 11:43 AM > > This patch adds Controls to set Horizontal and Vertical search range > for Motion Estimation block for Samsung MFC video Encoders. > > Signed-off-by: Swami Nathan > Signed-off-by: Amit Grover > --- > Documentation/DocBook/media/v4l/controls.xml| 14 + > drivers/media/platform/s5p-mfc/s5p_mfc_common.h |2 ++ > drivers/media/platform/s5p-mfc/s5p_mfc_enc.c| 24 > +++ > drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c |8 ++-- > drivers/media/v4l2-core/v4l2-ctrls.c| 14 + > include/uapi/linux/v4l2-controls.h |2 ++ > 6 files changed, 58 insertions(+), 6 deletions(-) > > diff --git a/Documentation/DocBook/media/v4l/controls.xml > b/Documentation/DocBook/media/v4l/controls.xml > index 7a3b49b..70a0f6f 100644 > --- a/Documentation/DocBook/media/v4l/controls.xml > +++ b/Documentation/DocBook/media/v4l/controls.xml > @@ -2258,6 +2258,20 @@ Applicable to the MPEG1, MPEG2, MPEG4 > encoders. > VBV buffer control. > > > + > + > + spanname="id">V4L2_CID_MPEG_VIDEO_HORZ_SEARCH_RANGE > > + integer > + Sets the Horizontal > search range for Video Macro blocks. > + It's expressed in pixels? If so then it should be mentioned here. Also I think this lacks the mention that it is used for motion estimation. Please add a more detailed description. > + > + > + > + spanname="id">V4L2_CID_MPEG_VIDEO_VERT_SEARCH_RANGE > V4L2_CID_MPEG_VIDEO_MV_V_SEARCH_RANGE seems better. > + integer > + Sets the Vertical search > range for Video Macro blocks. > + > + This description is too vague as well. > > >spanname="id">V4L2_CID_MPEG_VIDEO_H264_CPB_SIZE&nb > sp; > diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_common.h > b/drivers/media/platform/s5p-mfc/s5p_mfc_common.h > index 6920b54..f2c13c3 100644 > --- a/drivers/media/platform/s5p-mfc/s5p_mfc_common.h > +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_common.h > @@ -430,6 +430,8 @@ struct s5p_mfc_vp8_enc_params { > struct s5p_mfc_enc_params { > u16 width; > u16 height; > + u32 horz_range; > + u32 vert_range; mv_h_range ? mv_v_range ? > > u16 gop_size; > enum v4l2_mpeg_video_multi_slice_mode slice_mode; > diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c > b/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c > index 4ff3b6c..a02e7b8 100644 > --- a/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c > +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c > @@ -208,6 +208,24 @@ static struct mfc_control controls[] = { > .default_value = 0, > }, > { > + .id = V4L2_CID_MPEG_VIDEO_HORZ_SEARCH_RANGE, > + .type = V4L2_CTRL_TYPE_INTEGER, > + .name = "horizontal search range of video macro block", This too should be property capitalised. Please mention the motion vectors too. > + .minimum = 16, > + .maximum = 128, > + .step = 16, > + .default_value = 32, > + }, > + { > + .id = V4L2_CID_MPEG_VIDEO_VERT_SEARCH_RANGE, > + .type = V4L2_CTRL_TYPE_INTEGER, > + .name = "vertical search range of video macro block", This too should be property capitalised. Please mention the motion vectors too. > + .minimum = 16, > + .maximum = 128, > + .step = 16, > + .default_value = 32, > + }, > + { > .id = V4L2_CID_MPEG_VIDEO_H264_CPB_SIZE, > .type = V4L2_CTRL_TYPE_INTEGER, > .minimum = 0, > @@ -1377,6 +1395,12 @@ static int s5p_mfc_enc_s_ctrl(struct v4l2_ctrl > *ctrl) > case V4L2_CID_MPEG_VIDEO_VBV_SIZE: > p->vbv_size = ctrl->val; > break; > + case V4L2_CID_MPEG_VIDEO_HORZ_SEARCH_RANGE: > + p->horz_range = ctrl->val; > + break; > + case V4L2_CID_MPEG_VIDEO_VERT_SEARCH_RANGE: > + p->vert_range = ctrl->val; > + break; > case V4L2_CID_MPEG_VIDEO_H264_CPB_SIZE: > p->codec.h264.cpb_size = ctrl->val; > break; > 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 461358c..47e1807 100644 > --- a/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c > +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c > @@ -727,14 +727,10 @@ static int s5p_mfc_set_enc_params(struct > s5p_mfc_ctx *ctx) > WRITEL(reg, S5P_FIMV_E_RC_CONFIG_V6); > > /* setting for MV range [16, 256] */ > - reg = 0; > - reg &= ~(0x3FFF); > - reg = 256; > + reg = (p->horz_range & 0x3fff); /* conditional check in app */ > WRITEL(reg, S5P_FIMV_E_MV_HOR_RANGE
Re: [PATCH] [media] s5p-mfc: Add Horizontal and Vertical search range for Video Macro Blocks
Hi Swaminathan, On Thu, Jan 23, 2014 at 10:49 AM, swaminathan wrote: > Hi All, > Is there any review Comments for the patch "[PATCH] [media] s5p-mfc: Add > Horizontal and Vertical search range for Video Macro Blocks" > posted on 30-Dec-2013 ? > > Just a side note, please don’t top post and always reply as plain text. [Snip] > Subject: [PATCH] [media] s5p-mfc: Add Horizontal and Vertical search range > for Video Macro Blocks > > >> This patch adds Controls to set Horizontal and Vertical search range >> for Motion Estimation block for Samsung MFC video Encoders. >> >> Signed-off-by: Swami Nathan >> Signed-off-by: Amit Grover >> --- >> Documentation/DocBook/media/v4l/controls.xml| 14 + >> drivers/media/platform/s5p-mfc/s5p_mfc_common.h |2 ++ >> drivers/media/platform/s5p-mfc/s5p_mfc_enc.c| 24 >> +++ >> drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c |8 ++-- >> drivers/media/v4l2-core/v4l2-ctrls.c| 14 + >> include/uapi/linux/v4l2-controls.h |2 ++ >> 6 files changed, 58 insertions(+), 6 deletions(-) >> This patch from the outset looks OK, but you need to split up into two, first adding a v4l control and second one using it up in the driver. Regards, --Prabhakar Lad -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] [media] s5p-mfc: Add Horizontal and Vertical search range for Video Macro Blocks
Hi All, Is there any review Comments for the patch "[PATCH] [media] s5p-mfc: Add Horizontal and Vertical search range for Video Macro Blocks" posted on 30-Dec-2013 ? Regards, Swaminathan -- From: "Amit Grover" Sent: Monday, December 30, 2013 4:13 PM To: ; ; ; ; ; ; ; ; ; Cc: ; ; ; ; ; ; ; "Swami Nathan" Subject: [PATCH] [media] s5p-mfc: Add Horizontal and Vertical search range for Video Macro Blocks This patch adds Controls to set Horizontal and Vertical search range for Motion Estimation block for Samsung MFC video Encoders. Signed-off-by: Swami Nathan Signed-off-by: Amit Grover --- Documentation/DocBook/media/v4l/controls.xml| 14 + drivers/media/platform/s5p-mfc/s5p_mfc_common.h |2 ++ drivers/media/platform/s5p-mfc/s5p_mfc_enc.c| 24 +++ drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c |8 ++-- drivers/media/v4l2-core/v4l2-ctrls.c| 14 + include/uapi/linux/v4l2-controls.h |2 ++ 6 files changed, 58 insertions(+), 6 deletions(-) diff --git a/Documentation/DocBook/media/v4l/controls.xml b/Documentation/DocBook/media/v4l/controls.xml index 7a3b49b..70a0f6f 100644 --- a/Documentation/DocBook/media/v4l/controls.xml +++ b/Documentation/DocBook/media/v4l/controls.xml @@ -2258,6 +2258,20 @@ Applicable to the MPEG1, MPEG2, MPEG4 encoders. VBV buffer control. + + + spanname="id">V4L2_CID_MPEG_VIDEO_HORZ_SEARCH_RANGE + integer + Sets the Horizontal search range for Video Macro blocks. + + + + + spanname="id">V4L2_CID_MPEG_VIDEO_VERT_SEARCH_RANGE + integer + Sets the Vertical search range for Video Macro blocks. + + spanname="id">V4L2_CID_MPEG_VIDEO_H264_CPB_SIZE diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_common.h b/drivers/media/platform/s5p-mfc/s5p_mfc_common.h index 6920b54..f2c13c3 100644 --- a/drivers/media/platform/s5p-mfc/s5p_mfc_common.h +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_common.h @@ -430,6 +430,8 @@ struct s5p_mfc_vp8_enc_params { struct s5p_mfc_enc_params { u16 width; u16 height; + u32 horz_range; + u32 vert_range; u16 gop_size; enum v4l2_mpeg_video_multi_slice_mode slice_mode; diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c b/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c index 4ff3b6c..a02e7b8 100644 --- a/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c @@ -208,6 +208,24 @@ static struct mfc_control controls[] = { .default_value = 0, }, { + .id = V4L2_CID_MPEG_VIDEO_HORZ_SEARCH_RANGE, + .type = V4L2_CTRL_TYPE_INTEGER, + .name = "horizontal search range of video macro block", + .minimum = 16, + .maximum = 128, + .step = 16, + .default_value = 32, + }, + { + .id = V4L2_CID_MPEG_VIDEO_VERT_SEARCH_RANGE, + .type = V4L2_CTRL_TYPE_INTEGER, + .name = "vertical search range of video macro block", + .minimum = 16, + .maximum = 128, + .step = 16, + .default_value = 32, + }, + { .id = V4L2_CID_MPEG_VIDEO_H264_CPB_SIZE, .type = V4L2_CTRL_TYPE_INTEGER, .minimum = 0, @@ -1377,6 +1395,12 @@ static int s5p_mfc_enc_s_ctrl(struct v4l2_ctrl *ctrl) case V4L2_CID_MPEG_VIDEO_VBV_SIZE: p->vbv_size = ctrl->val; break; + case V4L2_CID_MPEG_VIDEO_HORZ_SEARCH_RANGE: + p->horz_range = ctrl->val; + break; + case V4L2_CID_MPEG_VIDEO_VERT_SEARCH_RANGE: + p->vert_range = ctrl->val; + break; case V4L2_CID_MPEG_VIDEO_H264_CPB_SIZE: p->codec.h264.cpb_size = ctrl->val; break; 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 461358c..47e1807 100644 --- a/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c @@ -727,14 +727,10 @@ static int s5p_mfc_set_enc_params(struct s5p_mfc_ctx *ctx) WRITEL(reg, S5P_FIMV_E_RC_CONFIG_V6); /* setting for MV range [16, 256] */ - reg = 0; - reg &= ~(0x3FFF); - reg = 256; + reg = (p->horz_range & 0x3fff); /* conditional check in app */ WRITEL(reg, S5P_FIMV_E_MV_HOR_RANGE_V6); - reg = 0; - reg &= ~(0x3FFF); - reg = 256; + reg = (p->vert_range & 0x3fff); /* conditional check in app */ WRITEL(reg, S5P_FIMV_E_MV_VER_RANGE_V6); WRITEL(0x0, S5P_FIMV_E_FRAME_INSERTION_V6); diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c index fb46790..7cf23d5 100644 --- a/drivers/media/v4l2-core/v4l2-ctrls.c +++ b/drivers/media/v4l2-core/v4l2-ctrls.c @@ -735,6 +735,8 @@ const char *v4l2_ctrl_get_name(u32 id) case V4L2_CID_MPEG_VIDEO_DEC_PTS: return "Video Decoder PTS"; case V4L2_CID_MPEG_VIDEO_DEC_FRAME: return "Video Decoder Frame Count"; case V4L2_CID_MPEG_VIDEO_VBV_DELAY: return "Initial Delay for VBV Control"; + case V4L2_CID_MPEG_VIDEO_HORZ_SEARCH_RANGE: return "hor search range of video MB"; + case V4L2_CID_MPEG_VIDEO_VERT_SEARCH_RANGE: return "vert search range of video MB"; case V4L2_CID_MPEG_VIDEO_REPEAT