From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754338AbdAROi6 (ORCPT ); Wed, 18 Jan 2017 09:38:58 -0500 Received: from mailout1.w1.samsung.com ([210.118.77.11]:59391 "EHLO mailout1.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751975AbdAROiE (ORCPT ); Wed, 18 Jan 2017 09:38:04 -0500 X-AuditID: cbfec7ef-f79d26d00000420c-91-587f7d9a0ccb Subject: Re: [PATCH] [media] s5p-mfc: Align stream buffer and CPB buffer to 512 To: Smitha T Murthy , linux-arm-kernel@lists.infradead.org, linux-media@vger.kernel.org, linux-kernel@vger.kernel.org Cc: kyungmin.park@samsung.com, kamil@wypas.org, jtp.park@samsung.com, mchehab@kernel.org, pankaj.dubey@samsung.com, krzk@kernel.org, m.szyprowski@samsung.com From: Andrzej Hajda Message-id: Date: Wed, 18 Jan 2017 15:37:09 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.5.1 MIME-version: 1.0 In-reply-to: <1484732223-24670-1-git-send-email-smitha.t@samsung.com> Content-type: text/plain; charset=windows-1252 Content-transfer-encoding: 7bit X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFlrKKsWRmVeSWpSXmKPExsWy7djP87qzausjDC49FbM4svYqk8XME+2s FufPb2C3ONv0ht1i0+NrrBaXd81hs+jZsJXVYu2Ru+wWyzb9YbJYtPULu8XdPdsYHbg9Nq3q ZPPYvKTeo2/LKkaPz5vkPK4caWQPYI3isklJzcksSy3St0vgypj19RxjwTmJiitvzrI3MG4U 6WLk5JAQMJF43nGfBcIWk7hwbz1bFyMXh5DAMkaJ6UsXsEA4nxklnjxrYIfpmH9oFzNc1fxt G5hAEkICzxgl3nwRBLGFBQIllt57xgpSJCLQzyixa+stsA5mgYVARe2PwEaxCWhK/N18kw3E 5hWwkzhx6yQriM0ioCqxedcGsBpRgQiJQ8duM0PUCEr8mHwP7FhOAVeJ6Ycg6pkFDCRmTDnM BGHLS2xe8xZsmYTAMXaJXx/3ACU4gBxZiU0HmCFMF4nX2/kgvhGWeHV8C9RnMhKXJ3ezQLR2 M0p86j/BDuFMYZT492EGM0SVtcTh4xehFvNJTNo2HWoor0RHmxCE6SGxaRU0fB0lelcdhIbW DEaJKU83Mk1glJ+F5J1ZSF6YheSFBYzMqxhFUkuLc9NTiw31ihNzi0vz0vWS83M3MQITz+l/ x9/vYHzaHHKIUYCDUYmHt6OoPkKINbGsuDL3EKMEB7OSCO+2KqAQb0piZVVqUX58UWlOavEh RmkOFiVx3r0LroQLCaQnlqRmp6YWpBbBZJk4OKUaGAOvmqvH/eC0dL0tec1oO1e/fOae9LLs iUekm1Y0vwlq4j/qlCms1nQxev/Hy9sD15q29Bvpz3BrCP1exKPkv8dAiSdwpwK/8R/3TyVh b2ylVus8v/T20HGdACexfZt3JJyRvu29cOGPc49kOlQfyryY7snwzOv6M+Fli/L3u3kWTTLr XK26XImlOCPRUIu5qDgRABoJRIk4AwAA X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFjrMIsWRmVeSWpSXmKPExsVy+t/xy7oTa+sjDJq22FgcWXuVyWLmiXZW i/PnN7BbnG16w26x6fE1VovLu+awWfRs2MpqsfbIXXaLZZv+MFks2vqF3eLunm2MDtwem1Z1 snlsXlLv0bdlFaPH501yHleONLIHsEa52WSkJqakFimk5iXnp2TmpdsqhYa46VooKeQl5qba KkXo+oYEKSmUJeaUAnlGBmjAwTnAPVhJ3y7BLWPW13OMBeckKq68OcvewLhRpIuRk0NCwERi /qFdzBC2mMSFe+vZuhi5OIQEljBK3Nq+gRXCecYosWPzBkaQKmEBf4n+a3fAqkQE+hklDrxv ZwdJCAnMYpR4uD4YJMEssJBR4ljfT7AONgFNib+bb7KB2LwCdhInbp1kBbFZBFQlNu/aANTM wSEqECHRcDgdokRQ4sfkeywgNqeAq8T0QyDlHEAz9STuX9QCCTMLyEtsXvOWeQIj0E6EjlkI VbOQVC1gZF7FKJJaWpybnltspFecmFtcmpeul5yfu4kRGIHbjv3csoOx613wIUYBDkYlHt6O ovoIIdbEsuLK3EOMEhzMSiK826qAQrwpiZVVqUX58UWlOanFhxhNgT6YyCwlmpwPTA55JfGG JobmloZGxhYW5kZGSuK8Uz9cCRcSSE8sSc1OTS1ILYLpY+LglGpgzMx+0lus3nhCdpHkJc1V JVc/ZgcKf+96INZ35Yvv1G139ofMejDz1Ek2A+cAs1iRbbL/lVbE2c5Zubh3z80Sr9OWYfJ6 +T8LnqpsVXUX+yJz8WR5Q8US47VnZ38NU7z+pMv1gpvyZKPonPylek/MU2Z94M/fe33+Qg/3 Vdtd7vScmXr2opPJGyWW4oxEQy3mouJEADutu0/WAgAA X-MTR: 20000000000000000@CPGS X-CMS-MailID: 20170118143711eucas1p27dbb118e89daf28e42b8ac64cb206d23 X-Msg-Generator: CA X-Sender-IP: 182.198.249.180 X-Local-Sender: =?UTF-8?B?QW5kcnplaiBIYWpkYRtTUlBPTC1LZXJuZWwgKFRQKRvsgrw=?= =?UTF-8?B?7ISx7KCE7J6QG1NlbmlvciBTb2Z0d2FyZSBFbmdpbmVlcg==?= X-Global-Sender: =?UTF-8?B?QW5kcnplaiBIYWpkYRtTUlBPTC1LZXJuZWwgKFRQKRtTYW1z?= =?UTF-8?B?dW5nIEVsZWN0cm9uaWNzG1NlbmlvciBTb2Z0d2FyZSBFbmdpbmVlcg==?= X-Sender-Code: =?UTF-8?B?QzEwG0VIURtDMTBDRDAyQ0QwMjczOTI=?= CMS-TYPE: 201P X-HopCount: 7 X-CMS-RootMailID: 20170118094212epcas5p22e588016d2b330dcd0b99b6e1012c744 X-RootMTR: 20170118094212epcas5p22e588016d2b330dcd0b99b6e1012c744 References: <1484732223-24670-1-git-send-email-smitha.t@samsung.com> Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Smitha, On 18.01.2017 10:37, Smitha T Murthy wrote: > >From MFCv6 onwards encoder stream buffer and decoder CPB buffer Unexpected char at the beginning. > need to be aligned with 512. Patch below adds checks only if buffer size is multiple of 512, am I right? If yes, please precise the subject, for example "...CPB buffer size need to be...". > > Signed-off-by: Smitha T Murthy > --- > drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c | 9 +++++++++ > drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.h | 3 +++ > 2 files changed, 12 insertions(+), 0 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 d6f207e..57da798 100644 > --- a/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c > +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c > @@ -408,8 +408,15 @@ static int s5p_mfc_set_dec_stream_buffer_v6(struct s5p_mfc_ctx *ctx, > struct s5p_mfc_dev *dev = ctx->dev; > const struct s5p_mfc_regs *mfc_regs = dev->mfc_regs; > struct s5p_mfc_buf_size *buf_size = dev->variant->buf_size; > + size_t cpb_buf_size; > > mfc_debug_enter(); > + cpb_buf_size = ALIGN(buf_size->cpb, CPB_ALIGN); Since buf_size->cpb is constant of know size there is no need to align it here. > + if (strm_size >= set_strm_size_max(cpb_buf_size)) { > + mfc_debug(2, "Decrease strm_size : %u -> %zu, gap : %d\n", > + strm_size, set_strm_size_max(cpb_buf_size), CPB_ALIGN); > + strm_size = set_strm_size_max(cpb_buf_size); > + } As I understand strm_size here is a size of buffer to be decoded, why it cannot be equal to buf_size->cpb? Commit message says nothing about it. > mfc_debug(2, "inst_no: %d, buf_addr: 0x%08x,\n" > "buf_size: 0x%08x (%d)\n", > ctx->inst_no, buf_addr, strm_size, strm_size); > @@ -519,6 +526,8 @@ static int s5p_mfc_set_enc_stream_buffer_v6(struct s5p_mfc_ctx *ctx, > struct s5p_mfc_dev *dev = ctx->dev; > const struct s5p_mfc_regs *mfc_regs = dev->mfc_regs; > > + size = ALIGN(size, 512); > + Shouldn't be CPB_ALIGN instead of 512? And more importantly size is a length of buffer for encoded stream, by up-aligning you tell MFC that it can write beyond the buffer, it could potentially overwrite random memory? Am I right? > writel(addr, mfc_regs->e_stream_buffer_addr); /* 16B align */ > writel(size, mfc_regs->e_stream_buffer_size); > > diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.h b/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.h > index 8055848..16a7b1d 100644 > --- a/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.h > +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.h > @@ -40,6 +40,9 @@ > #define FRAME_DELTA_H264_H263 1 > #define TIGHT_CBR_MAX 10 > > +#define CPB_ALIGN 512 > +#define set_strm_size_max(cpb_max) ((cpb_max) - CPB_ALIGN) Name of the macro is misleading. Regards Andrzej > + > struct s5p_mfc_hw_ops *s5p_mfc_init_hw_ops_v6(void); > const struct s5p_mfc_regs *s5p_mfc_init_regs_v6_plus(struct s5p_mfc_dev *dev); > #endif /* S5P_MFC_OPR_V6_H_ */