[PATCH] media: s5p-mfc: Corrected NV12M/NV21M plane-sizes

[PATCH] media: s5p-mfc: Corrected NV12M/NV21M plane-sizes

[Aakarsh Jain]

There is a possibility of getting page fault if the overall
buffer size is not aligned to 256bytes. Since MFC does read
operation only and it won't corrupt the data values even if
it reads the extra bytes.
Corrected luma and chroma plane sizes for V4L2_PIX_FMT_NV12M
and V4L2_PIX_FMT_NV21M pixel format.

Signed-off-by: Aakarsh Jain <aakarsh.jain@samsung.com>
---
 .../media/platform/samsung/s5p-mfc/s5p_mfc_opr_v6.c    | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/media/platform/samsung/s5p-mfc/s5p_mfc_opr_v6.c b/drivers/media/platform/samsung/s5p-mfc/s5p_mfc_opr_v6.c
index 73f7af674c01..03c957221fc4 100644
--- a/drivers/media/platform/samsung/s5p-mfc/s5p_mfc_opr_v6.c
+++ b/drivers/media/platform/samsung/s5p-mfc/s5p_mfc_opr_v6.c
@@ -498,8 +498,8 @@ static void s5p_mfc_dec_calc_dpb_size_v6(struct s5p_mfc_ctx *ctx)
 	case V4L2_PIX_FMT_NV21M:
 		ctx->stride[0] = ALIGN(ctx->img_width, S5P_FIMV_NV12MT_HALIGN_V6);
 		ctx->stride[1] = ALIGN(ctx->img_width, S5P_FIMV_NV12MT_HALIGN_V6);
-		ctx->luma_size = calc_plane(ctx->stride[0], ctx->img_height);
-		ctx->chroma_size = calc_plane(ctx->stride[1], (ctx->img_height / 2));
+		ctx->luma_size = calc_plane(ctx->img_width, ctx->img_height);
+		ctx->chroma_size = calc_plane(ctx->img_width, (ctx->img_height >> 1));
 		break;
 	case V4L2_PIX_FMT_YUV420M:
 	case V4L2_PIX_FMT_YVU420M:
@@ -539,9 +539,11 @@ static void s5p_mfc_dec_calc_dpb_size_v6(struct s5p_mfc_ctx *ctx)
 static void s5p_mfc_enc_calc_src_size_v6(struct s5p_mfc_ctx *ctx)
 {
 	unsigned int mb_width, mb_height;
+	unsigned int default_size;
 
 	mb_width = MB_WIDTH(ctx->img_width);
 	mb_height = MB_HEIGHT(ctx->img_height);
+	default_size = (mb_width * mb_height) * 256;
 
 	if (IS_MFCV12(ctx->dev)) {
 		switch (ctx->src_fmt->fourcc) {
@@ -549,8 +551,8 @@ static void s5p_mfc_enc_calc_src_size_v6(struct s5p_mfc_ctx *ctx)
 		case V4L2_PIX_FMT_NV21M:
 			ctx->stride[0] = ALIGN(ctx->img_width, S5P_FIMV_NV12M_HALIGN_V6);
 			ctx->stride[1] = ALIGN(ctx->img_width, S5P_FIMV_NV12M_HALIGN_V6);
-			ctx->luma_size = ctx->stride[0] * ALIGN(ctx->img_height, 16);
-			ctx->chroma_size =  ctx->stride[0] * ALIGN(ctx->img_height / 2, 16);
+			ctx->luma_size = ALIGN(default_size, 256);
+			ctx->chroma_size = ALIGN(default_size / 2, 256);
 			break;
 		case V4L2_PIX_FMT_YUV420M:
 		case V4L2_PIX_FMT_YVU420M:
-- 
2.17.1

Re: [PATCH] media: s5p-mfc: Corrected NV12M/NV21M plane-sizes

[Nicolas Dufresne]

Hi Jain,

I haven't dig much, but I have a quick question below.

Le mardi 06 août 2024 à 17:27 +0530, Aakarsh Jain a écrit :
> There is a possibility of getting page fault if the overall
> buffer size is not aligned to 256bytes. Since MFC does read
> operation only and it won't corrupt the data values even if
> it reads the extra bytes.
> Corrected luma and chroma plane sizes for V4L2_PIX_FMT_NV12M
> and V4L2_PIX_FMT_NV21M pixel format.

Have you re-run v4l2 compliance ? (better be safe then sorry).

> 
> Signed-off-by: Aakarsh Jain <aakarsh.jain@samsung.com>
> ---
>  .../media/platform/samsung/s5p-mfc/s5p_mfc_opr_v6.c    | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/platform/samsung/s5p-mfc/s5p_mfc_opr_v6.c b/drivers/media/platform/samsung/s5p-mfc/s5p_mfc_opr_v6.c
> index 73f7af674c01..03c957221fc4 100644
> --- a/drivers/media/platform/samsung/s5p-mfc/s5p_mfc_opr_v6.c
> +++ b/drivers/media/platform/samsung/s5p-mfc/s5p_mfc_opr_v6.c
> @@ -498,8 +498,8 @@ static void s5p_mfc_dec_calc_dpb_size_v6(struct s5p_mfc_ctx *ctx)
>  	case V4L2_PIX_FMT_NV21M:
>  		ctx->stride[0] = ALIGN(ctx->img_width, S5P_FIMV_NV12MT_HALIGN_V6);
>  		ctx->stride[1] = ALIGN(ctx->img_width, S5P_FIMV_NV12MT_HALIGN_V6);
> -		ctx->luma_size = calc_plane(ctx->stride[0], ctx->img_height);
> -		ctx->chroma_size = calc_plane(ctx->stride[1], (ctx->img_height / 2));
> +		ctx->luma_size = calc_plane(ctx->img_width, ctx->img_height);
> +		ctx->chroma_size = calc_plane(ctx->img_width, (ctx->img_height >> 1));

These size needs to match the sizes reported through TRY_FMT (and S_FMT)
sizeimage for each planes. Is this code being call withing try_fmt ? Will these
value match or will this change cause the value to miss-match ?

The reason is that correct value is needed for allocating this memory from the
outside (like using a DMAbuf Heap). Perhaps its all right, let me know.

Nicolas

>  		break;
>  	case V4L2_PIX_FMT_YUV420M:
>  	case V4L2_PIX_FMT_YVU420M:
> @@ -539,9 +539,11 @@ static void s5p_mfc_dec_calc_dpb_size_v6(struct s5p_mfc_ctx *ctx)
>  static void s5p_mfc_enc_calc_src_size_v6(struct s5p_mfc_ctx *ctx)
>  {
>  	unsigned int mb_width, mb_height;
> +	unsigned int default_size;
>  
>  	mb_width = MB_WIDTH(ctx->img_width);
>  	mb_height = MB_HEIGHT(ctx->img_height);
> +	default_size = (mb_width * mb_height) * 256;
>  
>  	if (IS_MFCV12(ctx->dev)) {
>  		switch (ctx->src_fmt->fourcc) {
> @@ -549,8 +551,8 @@ static void s5p_mfc_enc_calc_src_size_v6(struct s5p_mfc_ctx *ctx)
>  		case V4L2_PIX_FMT_NV21M:
>  			ctx->stride[0] = ALIGN(ctx->img_width, S5P_FIMV_NV12M_HALIGN_V6);
>  			ctx->stride[1] = ALIGN(ctx->img_width, S5P_FIMV_NV12M_HALIGN_V6);
> -			ctx->luma_size = ctx->stride[0] * ALIGN(ctx->img_height, 16);
> -			ctx->chroma_size =  ctx->stride[0] * ALIGN(ctx->img_height / 2, 16);
> +			ctx->luma_size = ALIGN(default_size, 256);
> +			ctx->chroma_size = ALIGN(default_size / 2, 256);
>  			break;
>  		case V4L2_PIX_FMT_YUV420M:
>  		case V4L2_PIX_FMT_YVU420M:

RE: [PATCH] media: s5p-mfc: Corrected NV12M/NV21M plane-sizes

[Aakarsh Jain]

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

Hi Nocolas,

> -----Original Message-----
> From: Nicolas Dufresne <nicolas@ndufresne.ca>
> Sent: 06 August 2024 20:08
> To: Aakarsh Jain <aakarsh.jain@samsung.com>; linux-arm-
> kernel@lists.infradead.org; linux-media@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Cc: m.szyprowski@samsung.com; andrzej.hajda@intel.com;
> mchehab@kernel.org; hverkuil-cisco@xs4all.nl;
> krzysztof.kozlowski+dt@linaro.org; linux-samsung-soc@vger.kernel.org;
> gost.dev@samsung.com; aswani.reddy@samsung.com;
> pankaj.dubey@samsung.com
> Subject: Re: [PATCH] media: s5p-mfc: Corrected NV12M/NV21M plane-sizes
> 
> Hi Jain,
> 
> I haven't dig much, but I have a quick question below.
> 
> Le mardi 06 août 2024 à 17:27 +0530, Aakarsh Jain a écrit :
> > There is a possibility of getting page fault if the overall buffer
> > size is not aligned to 256bytes. Since MFC does read operation only
> > and it won't corrupt the data values even if it reads the extra bytes.
> > Corrected luma and chroma plane sizes for V4L2_PIX_FMT_NV12M and
> > V4L2_PIX_FMT_NV21M pixel format.
> 
> Have you re-run v4l2 compliance ? (better be safe then sorry).
> 
I ran v4l2-compliance and didn't found any issue wrt to the changes added in this patch.
Please find the v4l2-compliance report attached.

> >
> > Signed-off-by: Aakarsh Jain <aakarsh.jain@samsung.com>
> > ---
> >  .../media/platform/samsung/s5p-mfc/s5p_mfc_opr_v6.c    | 10 ++++++----
> >  1 file changed, 6 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/media/platform/samsung/s5p-mfc/s5p_mfc_opr_v6.c
> > b/drivers/media/platform/samsung/s5p-mfc/s5p_mfc_opr_v6.c
> > index 73f7af674c01..03c957221fc4 100644
> > --- a/drivers/media/platform/samsung/s5p-mfc/s5p_mfc_opr_v6.c
> > +++ b/drivers/media/platform/samsung/s5p-mfc/s5p_mfc_opr_v6.c
> > @@ -498,8 +498,8 @@ static void s5p_mfc_dec_calc_dpb_size_v6(struct
> s5p_mfc_ctx *ctx)
> >  	case V4L2_PIX_FMT_NV21M:
> >  		ctx->stride[0] = ALIGN(ctx->img_width,
> S5P_FIMV_NV12MT_HALIGN_V6);
> >  		ctx->stride[1] = ALIGN(ctx->img_width,
> S5P_FIMV_NV12MT_HALIGN_V6);
> > -		ctx->luma_size = calc_plane(ctx->stride[0], ctx->img_height);
> > -		ctx->chroma_size = calc_plane(ctx->stride[1], (ctx-
> >img_height / 2));
> > +		ctx->luma_size = calc_plane(ctx->img_width, ctx-
> >img_height);
> > +		ctx->chroma_size = calc_plane(ctx->img_width, (ctx-
> >img_height >>
> > +1));
> 
> These size needs to match the sizes reported through TRY_FMT (and S_FMT)
> sizeimage for each planes. Is this code being call withing try_fmt ? Will these
> value match or will this change cause the value to miss-match ?
> 
This code is getting called within try_fmt. In MFC driver we are not returning any sizes in TRY_FMT. We are only validating codec and the pixel format. We are setting luma, chroma and stride size in S_FMT
to inform user space for further buffer allocation. So, this change is not going to cause any mismatch.

> The reason is that correct value is needed for allocating this memory from
> the outside (like using a DMAbuf Heap). Perhaps its all right, let me know.
> 
> Nicolas
> 
> >  		break;
> >  	case V4L2_PIX_FMT_YUV420M:
> >  	case V4L2_PIX_FMT_YVU420M:
> > @@ -539,9 +539,11 @@ static void s5p_mfc_dec_calc_dpb_size_v6(struct
> > s5p_mfc_ctx *ctx)  static void s5p_mfc_enc_calc_src_size_v6(struct
> > s5p_mfc_ctx *ctx)  {
> >  	unsigned int mb_width, mb_height;
> > +	unsigned int default_size;
> >
> >  	mb_width = MB_WIDTH(ctx->img_width);
> >  	mb_height = MB_HEIGHT(ctx->img_height);
> > +	default_size = (mb_width * mb_height) * 256;
> >
> >  	if (IS_MFCV12(ctx->dev)) {
> >  		switch (ctx->src_fmt->fourcc) {
> > @@ -549,8 +551,8 @@ static void s5p_mfc_enc_calc_src_size_v6(struct
> s5p_mfc_ctx *ctx)
> >  		case V4L2_PIX_FMT_NV21M:
> >  			ctx->stride[0] = ALIGN(ctx->img_width,
> S5P_FIMV_NV12M_HALIGN_V6);
> >  			ctx->stride[1] = ALIGN(ctx->img_width,
> S5P_FIMV_NV12M_HALIGN_V6);
> > -			ctx->luma_size = ctx->stride[0] * ALIGN(ctx-
> >img_height, 16);
> > -			ctx->chroma_size =  ctx->stride[0] * ALIGN(ctx-
> >img_height / 2, 16);
> > +			ctx->luma_size = ALIGN(default_size, 256);
> > +			ctx->chroma_size = ALIGN(default_size / 2, 256);
> >  			break;
> >  		case V4L2_PIX_FMT_YUV420M:
> >  		case V4L2_PIX_FMT_YVU420M:


[-- Attachment #2: v4l2-compliance.txt --]
[-- Type: text/plain, Size: 11186 bytes --]

# v4l2-compliance -d /dev/video1
v4l2-compliance 1.22.1, 64 bits, 64-bit time_t

[ 1000.848486] vidioc_g_parm:2312: Setting FPS is only possible for the output queue
[ 1000.852609] s5p-mfc 12880000.mfc: Encoding not initialised
[ 1000.857181] s5p-mfc 12880000.mfc: Encoding not initialised
[ 1000.862794] vidioc_g_parm:2312: Setting FPS is only possible for the output queue
[ 1000.870120] vidioc_try_fmt:1440: failed to try output format
Compliance test for s5p-mfc device /dev/video1:

Driver Info:
        Driver name      : s5p-mfc
        Card type        : s5p-mfc-enc
        Bus info         : platform:12880000.mfc
        Driver version   : 6.10.0
        Capabilities     : 0x84204000
                Video Memory-to-Memory Multiplanar
                Streaming
                Extended Pix Format
                Device Capabilities
        Device Caps      : 0x04204000
                Video Memory-to-Memory Multiplanar
                Streaming
                Extended Pix Format
        Detected Stateful Encoder

Required ioctls:
        test VIDIOC_QUERYCAP: OK
        test invalid ioctls: OK

Allow for multiple opens:
        test second /dev/video1 open: OK
        test VIDIOC_QUERYCAP: OK
        test VIDIOC_G/S_PRIORITY: OK
                fail: v4l2-compliance.cpp(736): !ok
        test for unlimited opens: FAIL

Debug ioctls:
        test VIDIOC_DBG_G/S_REGISTER: OK (Not Supported)
        test VIDIOC_LOG_STATUS: OK (Not Supported)

Input ioctls:
        test VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK (Not Supported)
        test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
        test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported)
        test VIDIOC_ENUMAUDIO: OK (Not Supported)
        test VIDIOC_G/S/ENUMINPUT: OK (Not Supported)
        test VIDIOC_G/S_AUDIO: OK (Not Supported)
        Inputs: 0 Audio Inputs: 0 Tuners: 0

Output ioctls:
        test VIDIOC_G/S_MODULATOR: OK (Not Supported)
        test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
        test VIDIOC_ENUMAUDOUT: OK (Not Supported)
        test VIDIOC_G/S/ENUMOUTPUT: OK (Not Supported)
        test VIDIOC_G/S_AUDOUT: OK (Not Supported)
        Outputs: 0 Audio Outputs: 0 Modulators: 0

Input/Output configuration ioctls:
        test VIDIOC_ENUM/G/S/QUERY_STD: OK (Not Supported)
        test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK (Not Supported)
        test VIDIOC_DV_TIMINGS_CAP: OK (Not Supported)
        test VIDIOC_G/S_EDID: OK (Not Supported)

Control ioctls:
        test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK
        test VIDIOC_QUERYCTRL: OK
                fail: v4l2-test-controls.cpp(473): g_ctrl returned an error (22)
        test VIDIOC_G/S_CTRL: FAIL
                fail: v4l2-test-controls.cpp(704): g_ext_ctrls returned an error (22)
        test VIDIOC_G/S/TRY_EXT_CTRLS: FAIL
                fail: v4l2-test-controls.cpp(872): subscribe event for control 'User Controls' failed
        test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: FAIL
        test VIDIOC_G/S_JPEGCOMP: OK (Not Supported)
        Standard Controls: 107 Private Controls: 11

Format ioctls:
                fail: v4l2-test-formats.cpp(282): node->codec_mask & STATEFUL_ENCODER
        test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: FAIL
                fail: v4l2-test-formats.cpp(1310): is_stateful_enc && !out->capability
        test VIDIOC_G/S_PARM: FAIL
        test VIDIOC_G_FBUF: OK (Not Supported)
                fail: v4l2-test-formats.cpp(474): !pix_mp.width || !pix_mp.height
        test VIDIOC_G_FMT: FAIL
                fail: v4l2-test-formats.cpp(474): !pix_mp.width || !pix_mp.height
        test VIDIOC_TRY_FMT: FAIL
                warn: v4l2-test-formats.cpp(1147): S_FMT cannot handle an invalid pixelformat.
                warn: v4l2-test-formats.cpp(1148): This may or may not be a problem. For more information see:
                warn: v4l2-test-formats.cpp(1149): http://www.mail-archive.com/linux-media@vger.kernel.org/msg56550.html
                fail: v4l2-test-formats.cpp(478): pixelformat 34363248 (H264) for buftype 9 not reported by ENUM_FMT
        test VIDIOC_S_FMT: FAIL
        test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported)
        test Cropping: OK (Not Supported)
        test Composing: OK (Not Supported)
        test Scaling: OK (Not Supported)

Codec ioctls:
                fail: v4l2-test-codecs.cpp(35): node->function[ 1001.213314] s5p_mfc_queue_setup:2426: invalid state: 0
[ 1001.217449] vidioc_reqbufs:1558: error in vb2_reqbufs() for E(D)
[ 1001.223600] vidioc_g_parm:2312: Setting FPS is only possible for the output queue
 != MEDIA_ENT_F_PROC_VIDEO_ENCODER
        test VIDIOC_(TRY_)ENCODER_CMD: FAIL
        test VIDIOC_G_ENC_INDEX: OK (Not Supported)
        test VIDIOC_(TRY_)DECODER_CMD: OK (Not Supported)

Buffer ioctls:
                fail: v4l2-test-buffers.cpp(607): q.reqbufs(node, 1)
        test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: FAIL
                fail: v4l2-test-buffers.cpp(783): VIDIOC_EXPBUF is supported, but the V4L2_MEMORY_MMAP support is missing or malfunctioning.
                fail: v4l2-test-buffers.cpp(784): VIDIOC_EXPBUF is supported, but the V4L2_MEMORY_MMAP support is missing, probably due to earlier failing format tests.
        test VIDIOC_EXPBUF: OK (Not Supported)
        test Requests: OK (Not Supported)

Total for s5p-mfc device /dev/video1: 45, Succeeded: 34, Failed: 11, Warnings: 3
#
# v4l2-compliance -d /dev/video0
# v4l2-compliance -d /dev/video0
v4l2-compliance 1.22.1, 64 bits, 64-bit time_t

[ 1867.640818] vidioc_g_selection:816: Can not get compose information
[ 1867.644432] vidioc_g_selection:816: Can not get compose information
[ 1867.650265] vidioc_g_fmt:397: Format could not be read
[ 1867.655301] vidioc_g_selection:816: Can not get compose information
[ 1867.661511] vidioc_g_selection:816: Can not get compose information
[ 1867.668211] s5p-mfc 12880000.mfc: Decoding not initialised
[ 1867.673235] s5p-mfc 12880000.mfc: Decoding not initialised
[ 1867.678894] vidioc_g_fmt:397: Format could not be read
[ 1867.683811] vidioc_g_selection:816: Can not get compose information
[ 1867.690068] vidioc_g_selection:816: Can not get compose information
[ 1867.696246] vidioc_g_selection:816: Can not get compose information
[ 1867.702533] vidioc_g_selection:816: Can not get compose information
[ 1867.708731] vidioc_g_selection:816: Can not get compose information
[ 1867.715044] vidioc_g_selection:816: Can not get compose information
[ 1867.721412] vidioc_g_selection:816: Can not get compose information
[ 1867.727660] vidioc_g_selection:816: Can not get compose information
[ 1867.733809] vidioc_g_selection:816: Can not get compose information
[ 1867.740145] vidioc_g_selection:816: Can not get compose information
[ 1867.746172] vidioc_try_fmt:429: Unsupported format for destination.
[ 1867.752579] vidioc_g_selection:816: Can not get compose information
[ 1867.758688] vidioc_g_selection:816: Can not get compose information
[ 1867.764938] vidioc_try_fmt:429: Unsupported format for destination.
[ 1867.771261] vidioc_g_selection:816: Can not get compose information
Compliance test for s5p-mfc device /dev/video0:

Driver Info:
        Driver name      : s5p-mfc
        Card type        : s5p-mfc-dec
        Bus info         : platform:12880000.mfc
        Driver version   : 6.10.0
        Capabilities     : 0x84204000
                Video Memory-to-Memory Multiplanar
                Streaming
                Extended Pix Format
                Device Capabilities
        Device Caps      : 0x04204000
                Video Memory-to-Memory Multiplanar
                Streaming
                Extended Pix Format
        Detected Stateful Decoder

Required ioctls:
        test VIDIOC_QUERYCAP: OK
        test invalid ioctls: OK

Allow for multiple opens:
        test second /dev/video0 open: OK
        test VIDIOC_QUERYCAP: OK
        test VIDIOC_G/S_PRIORITY: OK
                fail: v4l2-compliance.cpp(736): !ok
        test for unlimited opens: FAIL

Debug ioctls:
        test VIDIOC_DBG_G/S_REGISTER: OK (Not Supported)
        test VIDIOC_LOG_STATUS: OK (Not Supported)

Input ioctls:
        test VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK (Not Supported)
        test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
        test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported)
        test VIDIOC_ENUMAUDIO: OK (Not Supported)
        test VIDIOC_G/S/ENUMINPUT: OK (Not Supported)
        test VIDIOC_G/S_AUDIO: OK (Not Supported)
        Inputs: 0 Audio Inputs: 0 Tuners: 0

Output ioctls:
        test VIDIOC_G/S_MODULATOR: OK (Not Supported)
        test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
        test VIDIOC_ENUMAUDOUT: OK (Not Supported)
        test VIDIOC_G/S/ENUMOUTPUT: OK (Not Supported)
        test VIDIOC_G/S_AUDOUT: OK (Not Supported)
        Outputs: 0 Audio Outputs: 0 Modulators: 0

Input/Output configuration ioctls:
        test VIDIOC_ENUM/G/S/QUERY_STD: OK (Not Supported)
        test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK (Not Supported)
        test VIDIOC_DV_TIMINGS_CAP: OK (Not Supported)
        test VIDIOC_G/S_EDID: OK (Not Supported)

Control ioctls:
        test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK
        test VIDIOC_QUERYCTRL: OK
                fail: v4l2-test-controls.cpp(473): g_ctrl returned an error (22)
        test VIDIOC_G/S_CTRL: FAIL
                fail: v4l2-test-controls.cpp(704): g_ext_ctrls returned an error (22)
        test VIDIOC_G/S/TRY_EXT_CTRLS: FAIL
                fail: v4l2-test-controls.cpp(872): subscribe event for control 'User Controls' failed
        test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: FAIL
        test VIDIOC_G/S_JPEGCOMP: OK (Not Supported)
        Standard Controls: 7 Private Controls: 2

Format ioctls:
                fail: v4l2-test-formats.cpp(263): fmtdesc.description mismatch: was 'Y/UV 4:2:0 (N-C)', expected 'Y/CbCr 4:2:0 (N-C)'
        test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: FAIL
        test VIDIOC_G/S_PARM: OK (Not Supported)
        test VIDIOC_G_FBUF: OK (Not Supported)
                fail: v4l2-test-formats.cpp(620): Video Capture Multiplanar cap set, but no Video Capture Multiplanar formats defined
        test VIDIOC_G_FMT: FAIL
        test VIDIOC_TRY_FMT: OK (Not Supported)
        test VIDIOC_S_FMT: OK (Not Supported)
        test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported)
        test Cropping: OK (Not Supported)
        test Composing: OK (Not Supported)
        test Scaling: OK (Not Supported)

Codec ioctls:
        test VIDIOC_(TRY_)ENCODER_CMD: OK (Not Supported)
        test VIDIOC_G_ENC_INDEX: OK (Not Supported)
                fail: v4l2-test-codecs.cpp(104): node->function != MEDIA_ENT_F_PROC_VIDEO_DECODER
        test VIDIOC_(TRY_)DECODER_CMD: FAIL

Buffer ioctls:
        test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK
        test VIDIOC_EXPBUF: OK (Not Supported)
        test Requests: OK (Not Supported)

Total for s5p-mfc device /dev/video0: 45, Succeeded: 38, Failed: 7, Warnings: 0

RE: [PATCH] media: s5p-mfc: Corrected NV12M/NV21M plane-sizes

[Aakarsh Jain]

> -----Original Message-----
> From: Aakarsh Jain <aakarsh.jain@samsung.com>
> Sent: 07 August 2024 17:23
> To: 'Nicolas Dufresne' <nicolas@ndufresne.ca>; 'linux-arm-
> kernel@lists.infradead.org' <linux-arm-kernel@lists.infradead.org>; 'linux-
> media@vger.kernel.org' <linux-media@vger.kernel.org>; 'linux-
> kernel@vger.kernel.org' <linux-kernel@vger.kernel.org>
> Cc: 'm.szyprowski@samsung.com' <m.szyprowski@samsung.com>;
> 'andrzej.hajda@intel.com' <andrzej.hajda@intel.com>;
> 'mchehab@kernel.org' <mchehab@kernel.org>; 'hverkuil-cisco@xs4all.nl'
> <hverkuil-cisco@xs4all.nl>; 'krzysztof.kozlowski+dt@linaro.org'
> <krzysztof.kozlowski+dt@linaro.org>; 'linux-samsung-soc@vger.kernel.org'
> <linux-samsung-soc@vger.kernel.org>; 'gost.dev@samsung.com'
> <gost.dev@samsung.com>; 'aswani.reddy@samsung.com'
> <aswani.reddy@samsung.com>; 'pankaj.dubey@samsung.com'
> <pankaj.dubey@samsung.com>
> Subject: RE: [PATCH] media: s5p-mfc: Corrected NV12M/NV21M plane-sizes
> 
> Hi Nocolas,
> 
> > -----Original Message-----
> > From: Nicolas Dufresne <nicolas@ndufresne.ca>
> > Sent: 06 August 2024 20:08
> > To: Aakarsh Jain <aakarsh.jain@samsung.com>; linux-arm-
> > kernel@lists.infradead.org; linux-media@vger.kernel.org; linux-
> > kernel@vger.kernel.org
> > Cc: m.szyprowski@samsung.com; andrzej.hajda@intel.com;
> > mchehab@kernel.org; hverkuil-cisco@xs4all.nl;
> > krzysztof.kozlowski+dt@linaro.org; linux-samsung-soc@vger.kernel.org;
> > gost.dev@samsung.com; aswani.reddy@samsung.com;
> > pankaj.dubey@samsung.com
> > Subject: Re: [PATCH] media: s5p-mfc: Corrected NV12M/NV21M plane-
> sizes
> >
> > Hi Jain,
> >
> > I haven't dig much, but I have a quick question below.
> >
> > Le mardi 06 août 2024 à 17:27 +0530, Aakarsh Jain a écrit :
> > > There is a possibility of getting page fault if the overall buffer
> > > size is not aligned to 256bytes. Since MFC does read operation only
> > > and it won't corrupt the data values even if it reads the extra bytes.
> > > Corrected luma and chroma plane sizes for V4L2_PIX_FMT_NV12M and
> > > V4L2_PIX_FMT_NV21M pixel format.
> >
> > Have you re-run v4l2 compliance ? (better be safe then sorry).
> >
> I ran v4l2-compliance and didn't found any issue wrt to the changes added in
> this patch.
> Please find the v4l2-compliance report attached.
> 
> > >
> > > Signed-off-by: Aakarsh Jain <aakarsh.jain@samsung.com>
> > > ---
> > >  .../media/platform/samsung/s5p-mfc/s5p_mfc_opr_v6.c    | 10 ++++++-
> ---
> > >  1 file changed, 6 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/media/platform/samsung/s5p-mfc/s5p_mfc_opr_v6.c
> > > b/drivers/media/platform/samsung/s5p-mfc/s5p_mfc_opr_v6.c
> > > index 73f7af674c01..03c957221fc4 100644
> > > --- a/drivers/media/platform/samsung/s5p-mfc/s5p_mfc_opr_v6.c
> > > +++ b/drivers/media/platform/samsung/s5p-mfc/s5p_mfc_opr_v6.c
> > > @@ -498,8 +498,8 @@ static void s5p_mfc_dec_calc_dpb_size_v6(struct
> > s5p_mfc_ctx *ctx)
> > >  	case V4L2_PIX_FMT_NV21M:
> > >  		ctx->stride[0] = ALIGN(ctx->img_width,
> > S5P_FIMV_NV12MT_HALIGN_V6);
> > >  		ctx->stride[1] = ALIGN(ctx->img_width,
> > S5P_FIMV_NV12MT_HALIGN_V6);
> > > -		ctx->luma_size = calc_plane(ctx->stride[0], ctx->img_height);
> > > -		ctx->chroma_size = calc_plane(ctx->stride[1], (ctx-
> > >img_height / 2));
> > > +		ctx->luma_size = calc_plane(ctx->img_width, ctx-
> > >img_height);
> > > +		ctx->chroma_size = calc_plane(ctx->img_width, (ctx-
> > >img_height >>
> > > +1));
> >
> > These size needs to match the sizes reported through TRY_FMT (and
> > S_FMT) sizeimage for each planes. Is this code being call withing
> > try_fmt ? Will these value match or will this change cause the value to miss-
> match ?
> >
> This code is getting called within try_fmt. In MFC driver we are not returning
> any sizes in TRY_FMT. We are only validating codec and the pixel format. We
> are setting luma, chroma and stride size in S_FMT to inform user space for
> further buffer allocation. So, this change is not going to cause any mismatch.
> 
> > The reason is that correct value is needed for allocating this memory
> > from the outside (like using a DMAbuf Heap). Perhaps its all right, let me
> know.
> >
> > Nicolas
> >
> > >  		break;
> > >  	case V4L2_PIX_FMT_YUV420M:
> > >  	case V4L2_PIX_FMT_YVU420M:
> > > @@ -539,9 +539,11 @@ static void
> s5p_mfc_dec_calc_dpb_size_v6(struct
> > > s5p_mfc_ctx *ctx)  static void s5p_mfc_enc_calc_src_size_v6(struct
> > > s5p_mfc_ctx *ctx)  {
> > >  	unsigned int mb_width, mb_height;
> > > +	unsigned int default_size;
> > >
> > >  	mb_width = MB_WIDTH(ctx->img_width);
> > >  	mb_height = MB_HEIGHT(ctx->img_height);
> > > +	default_size = (mb_width * mb_height) * 256;
> > >
> > >  	if (IS_MFCV12(ctx->dev)) {
> > >  		switch (ctx->src_fmt->fourcc) {
> > > @@ -549,8 +551,8 @@ static void s5p_mfc_enc_calc_src_size_v6(struct
> > s5p_mfc_ctx *ctx)
> > >  		case V4L2_PIX_FMT_NV21M:
> > >  			ctx->stride[0] = ALIGN(ctx->img_width,
> > S5P_FIMV_NV12M_HALIGN_V6);
> > >  			ctx->stride[1] = ALIGN(ctx->img_width,
> > S5P_FIMV_NV12M_HALIGN_V6);
> > > -			ctx->luma_size = ctx->stride[0] * ALIGN(ctx-
> > >img_height, 16);
> > > -			ctx->chroma_size =  ctx->stride[0] * ALIGN(ctx-
> > >img_height / 2, 16);
> > > +			ctx->luma_size = ALIGN(default_size, 256);
> > > +			ctx->chroma_size = ALIGN(default_size / 2, 256);
> > >  			break;
> > >  		case V4L2_PIX_FMT_YUV420M:
> > >  		case V4L2_PIX_FMT_YVU420M:

Gentle reminder to review this patch.

Re: [PATCH] media: s5p-mfc: Corrected NV12M/NV21M plane-sizes

[Hans Verkuil]

On 06/08/2024 13:57, Aakarsh Jain wrote:
> There is a possibility of getting page fault if the overall
> buffer size is not aligned to 256bytes. Since MFC does read
> operation only and it won't corrupt the data values even if
> it reads the extra bytes.
> Corrected luma and chroma plane sizes for V4L2_PIX_FMT_NV12M
> and V4L2_PIX_FMT_NV21M pixel format.
> 
> Signed-off-by: Aakarsh Jain <aakarsh.jain@samsung.com>
> ---
>  .../media/platform/samsung/s5p-mfc/s5p_mfc_opr_v6.c    | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/platform/samsung/s5p-mfc/s5p_mfc_opr_v6.c b/drivers/media/platform/samsung/s5p-mfc/s5p_mfc_opr_v6.c
> index 73f7af674c01..03c957221fc4 100644
> --- a/drivers/media/platform/samsung/s5p-mfc/s5p_mfc_opr_v6.c
> +++ b/drivers/media/platform/samsung/s5p-mfc/s5p_mfc_opr_v6.c
> @@ -498,8 +498,8 @@ static void s5p_mfc_dec_calc_dpb_size_v6(struct s5p_mfc_ctx *ctx)
>  	case V4L2_PIX_FMT_NV21M:
>  		ctx->stride[0] = ALIGN(ctx->img_width, S5P_FIMV_NV12MT_HALIGN_V6);
>  		ctx->stride[1] = ALIGN(ctx->img_width, S5P_FIMV_NV12MT_HALIGN_V6);
> -		ctx->luma_size = calc_plane(ctx->stride[0], ctx->img_height);
> -		ctx->chroma_size = calc_plane(ctx->stride[1], (ctx->img_height / 2));
> +		ctx->luma_size = calc_plane(ctx->img_width, ctx->img_height);
> +		ctx->chroma_size = calc_plane(ctx->img_width, (ctx->img_height >> 1));

I don't really understand why this is changed. Looking at the implementation of
calc_plane and the various #define values that are used here and in calc_plane,
the number should be the same.

I think the original code makes more sense.

If I missed something, let me know.

>  		break;
>  	case V4L2_PIX_FMT_YUV420M:
>  	case V4L2_PIX_FMT_YVU420M:
> @@ -539,9 +539,11 @@ static void s5p_mfc_dec_calc_dpb_size_v6(struct s5p_mfc_ctx *ctx)
>  static void s5p_mfc_enc_calc_src_size_v6(struct s5p_mfc_ctx *ctx)
>  {
>  	unsigned int mb_width, mb_height;
> +	unsigned int default_size;
>  
>  	mb_width = MB_WIDTH(ctx->img_width);
>  	mb_height = MB_HEIGHT(ctx->img_height);
> +	default_size = (mb_width * mb_height) * 256;
>  
>  	if (IS_MFCV12(ctx->dev)) {
>  		switch (ctx->src_fmt->fourcc) {
> @@ -549,8 +551,8 @@ static void s5p_mfc_enc_calc_src_size_v6(struct s5p_mfc_ctx *ctx)
>  		case V4L2_PIX_FMT_NV21M:
>  			ctx->stride[0] = ALIGN(ctx->img_width, S5P_FIMV_NV12M_HALIGN_V6);
>  			ctx->stride[1] = ALIGN(ctx->img_width, S5P_FIMV_NV12M_HALIGN_V6);
> -			ctx->luma_size = ctx->stride[0] * ALIGN(ctx->img_height, 16);
> -			ctx->chroma_size =  ctx->stride[0] * ALIGN(ctx->img_height / 2, 16);
> +			ctx->luma_size = ALIGN(default_size, 256);
> +			ctx->chroma_size = ALIGN(default_size / 2, 256);

Isn't this effectively the same as doing:

			ctx->luma_size = ALIGN(ctx->luma_size, 256);
			ctx->chroma_size = ALIGN(ctx->chroma_size, 256);

I.e., the bug is that these sizes are not rounded up to a multiple of 256,
so just add that, rather than changing code elsewhere.

I might be wrong, but this seems a much simpler solution.

Regards,

	Hans

>  			break;
>  		case V4L2_PIX_FMT_YUV420M:
>  		case V4L2_PIX_FMT_YVU420M:

RE: [PATCH] media: s5p-mfc: Corrected NV12M/NV21M plane-sizes

[Aakarsh Jain/Aakarsh Jain]

Hi Hans,

-----Original Message-----
From: Hans Verkuil <hverkuil-cisco@xs4all.nl> 
Sent: 11 October 2024 18:17
To: Aakarsh Jain <aakarsh.jain@samsung.com>; linux-arm-kernel@lists.infradead.org; linux-media@vger.kernel.org; linux-kernel@vger.kernel.org
Cc: m.szyprowski@samsung.com; andrzej.hajda@intel.com; mchehab@kernel.org; krzysztof.kozlowski+dt@linaro.org; linux-samsung-soc@vger.kernel.org; gost.dev@samsung.com; aswani.reddy@samsung.com; pankaj.dubey@samsung.com
Subject: Re: [PATCH] media: s5p-mfc: Corrected NV12M/NV21M plane-sizes

On 06/08/2024 13:57, Aakarsh Jain wrote:
> There is a possibility of getting page fault if the overall buffer 
> size is not aligned to 256bytes. Since MFC does read operation only 
> and it won't corrupt the data values even if it reads the extra bytes.
> Corrected luma and chroma plane sizes for V4L2_PIX_FMT_NV12M and 
> V4L2_PIX_FMT_NV21M pixel format.
> 
> Signed-off-by: Aakarsh Jain <aakarsh.jain@samsung.com>
> ---
>  .../media/platform/samsung/s5p-mfc/s5p_mfc_opr_v6.c    | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/platform/samsung/s5p-mfc/s5p_mfc_opr_v6.c 
> b/drivers/media/platform/samsung/s5p-mfc/s5p_mfc_opr_v6.c
> index 73f7af674c01..03c957221fc4 100644
> --- a/drivers/media/platform/samsung/s5p-mfc/s5p_mfc_opr_v6.c
> +++ b/drivers/media/platform/samsung/s5p-mfc/s5p_mfc_opr_v6.c
> @@ -498,8 +498,8 @@ static void s5p_mfc_dec_calc_dpb_size_v6(struct s5p_mfc_ctx *ctx)
>  	case V4L2_PIX_FMT_NV21M:
>  		ctx->stride[0] = ALIGN(ctx->img_width, S5P_FIMV_NV12MT_HALIGN_V6);
>  		ctx->stride[1] = ALIGN(ctx->img_width, S5P_FIMV_NV12MT_HALIGN_V6);
> -		ctx->luma_size = calc_plane(ctx->stride[0], ctx->img_height);
> -		ctx->chroma_size = calc_plane(ctx->stride[1], (ctx->img_height / 2));
> +		ctx->luma_size = calc_plane(ctx->img_width, ctx->img_height);
> +		ctx->chroma_size = calc_plane(ctx->img_width, (ctx->img_height >> 
> +1));

I don't really understand why this is changed. Looking at the implementation of calc_plane and the various #define values that are used here and in calc_plane, the number should be the same.

I think the original code makes more sense.
okay.

If I missed something, let me know.

>  		break;
>  	case V4L2_PIX_FMT_YUV420M:
>  	case V4L2_PIX_FMT_YVU420M:
> @@ -539,9 +539,11 @@ static void s5p_mfc_dec_calc_dpb_size_v6(struct 
> s5p_mfc_ctx *ctx)  static void s5p_mfc_enc_calc_src_size_v6(struct 
> s5p_mfc_ctx *ctx)  {
>  	unsigned int mb_width, mb_height;
> +	unsigned int default_size;
>  
>  	mb_width = MB_WIDTH(ctx->img_width);
>  	mb_height = MB_HEIGHT(ctx->img_height);
> +	default_size = (mb_width * mb_height) * 256;
>  
>  	if (IS_MFCV12(ctx->dev)) {
>  		switch (ctx->src_fmt->fourcc) {
> @@ -549,8 +551,8 @@ static void s5p_mfc_enc_calc_src_size_v6(struct s5p_mfc_ctx *ctx)
>  		case V4L2_PIX_FMT_NV21M:
>  			ctx->stride[0] = ALIGN(ctx->img_width, S5P_FIMV_NV12M_HALIGN_V6);
>  			ctx->stride[1] = ALIGN(ctx->img_width, S5P_FIMV_NV12M_HALIGN_V6);
> -			ctx->luma_size = ctx->stride[0] * ALIGN(ctx->img_height, 16);
> -			ctx->chroma_size =  ctx->stride[0] * ALIGN(ctx->img_height / 2, 16);
> +			ctx->luma_size = ALIGN(default_size, 256);
> +			ctx->chroma_size = ALIGN(default_size / 2, 256);

Isn't this effectively the same as doing:

			ctx->luma_size = ALIGN(ctx->luma_size, 256);
			ctx->chroma_size = ALIGN(ctx->chroma_size, 256);

I.e., the bug is that these sizes are not rounded up to a multiple of 256, so just add that, rather than changing code elsewhere.

I might be wrong, but this seems a much simpler solution.
sure Hans. Will address in v2.

Thanks for review.

Regards,

	Hans

>  			break;
>  		case V4L2_PIX_FMT_YUV420M:
>  		case V4L2_PIX_FMT_YVU420M: