| Message ID | 20240806115714.29828-1-aakarsh.jain@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: s5p-mfc: Corrected NV12M/NV21M plane-sizes | expand |
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:
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: # 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
> -----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.
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:
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:
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(-)