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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <0ccb01daf798$409fb8f0$c1df2ad0$@samsung.com>
Date: Mon, 26 Aug 2024 14:43:41 +0530
From: "Aakarsh Jain" <aakarsh.jain@...sung.com>
To: "'Nicolas Dufresne'" <nicolas@...fresne.ca>,
	<linux-arm-kernel@...ts.infradead.org>, <linux-media@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>
Cc: <m.szyprowski@...sung.com>, <andrzej.hajda@...el.com>,
	<mchehab@...nel.org>, <hverkuil-cisco@...all.nl>,
	<krzysztof.kozlowski+dt@...aro.org>, <linux-samsung-soc@...r.kernel.org>,
	<gost.dev@...sung.com>, <aswani.reddy@...sung.com>,
	<pankaj.dubey@...sung.com>
Subject: RE: [PATCH] media: s5p-mfc: Corrected NV12M/NV21M plane-sizes



> -----Original Message-----
> From: Aakarsh Jain <aakarsh.jain@...sung.com>
> Sent: 07 August 2024 17:23
> To: 'Nicolas Dufresne' <nicolas@...fresne.ca>; 'linux-arm-
> kernel@...ts.infradead.org' <linux-arm-kernel@...ts.infradead.org>; 'linux-
> media@...r.kernel.org' <linux-media@...r.kernel.org>; 'linux-
> kernel@...r.kernel.org' <linux-kernel@...r.kernel.org>
> Cc: 'm.szyprowski@...sung.com' <m.szyprowski@...sung.com>;
> 'andrzej.hajda@...el.com' <andrzej.hajda@...el.com>;
> 'mchehab@...nel.org' <mchehab@...nel.org>; 'hverkuil-cisco@...all.nl'
> <hverkuil-cisco@...all.nl>; 'krzysztof.kozlowski+dt@...aro.org'
> <krzysztof.kozlowski+dt@...aro.org>; 'linux-samsung-soc@...r.kernel.org'
> <linux-samsung-soc@...r.kernel.org>; 'gost.dev@...sung.com'
> <gost.dev@...sung.com>; 'aswani.reddy@...sung.com'
> <aswani.reddy@...sung.com>; 'pankaj.dubey@...sung.com'
> <pankaj.dubey@...sung.com>
> Subject: RE: [PATCH] media: s5p-mfc: Corrected NV12M/NV21M plane-sizes
> 
> Hi Nocolas,
> 
> > -----Original Message-----
> > From: Nicolas Dufresne <nicolas@...fresne.ca>
> > Sent: 06 August 2024 20:08
> > To: Aakarsh Jain <aakarsh.jain@...sung.com>; linux-arm-
> > kernel@...ts.infradead.org; linux-media@...r.kernel.org; linux-
> > kernel@...r.kernel.org
> > Cc: m.szyprowski@...sung.com; andrzej.hajda@...el.com;
> > mchehab@...nel.org; hverkuil-cisco@...all.nl;
> > krzysztof.kozlowski+dt@...aro.org; linux-samsung-soc@...r.kernel.org;
> > gost.dev@...sung.com; aswani.reddy@...sung.com;
> > pankaj.dubey@...sung.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@...sung.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.