[v2] media: imx-jpeg: Leave a blank space before the configuration data

[PATCH v2] media: imx-jpeg: Leave a blank space before the configuration data

Posted by Ming Qian 3 years, 11 months ago

There is a hardware bug that it will load
the first 128 bytes of configuration data twice,
it will led to some configure error.
so shift the configuration data 128 bytes,
and make the first 128 bytes all zero,
then hardware will load the 128 zero twice,
and ignore them as garbage.
then the configuration data can be loaded correctly

Signed-off-by: Ming Qian <ming.qian@nxp.com>
Reviewed-by: Mirela Rabulea <mirela.rabulea@nxp.com>
Reviewed-by: Tommaso Merciai <tommaso.merciai@amarulasolutions.com>
---
v2
- add some comments about why the 0x80 offset is needed
 drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
index 734e1b65fbc7..c0fd030d0f19 100644
--- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
+++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
@@ -519,6 +519,7 @@ static bool mxc_jpeg_alloc_slot_data(struct mxc_jpeg_dev *jpeg,
 				     GFP_ATOMIC);
 	if (!cfg_stm)
 		goto err;
+	memset(cfg_stm, 0, MXC_JPEG_MAX_CFG_STREAM);
 	jpeg->slot_data[slot].cfg_stream_vaddr = cfg_stm;
 
 skip_alloc:
@@ -755,7 +756,13 @@ static unsigned int mxc_jpeg_setup_cfg_stream(void *cfg_stream_vaddr,
 					      u32 fourcc,
 					      u16 w, u16 h)
 {
-	unsigned int offset = 0;
+	/*
+	 * There is a hardware issue that first 128 bytes of configuration data
+	 * can't be loaded correctly.
+	 * To avoid this issue, we need to write the configuration from
+	 * an offset which should be no less than 0x80 (128 bytes).
+	 */
+	unsigned int offset = 0x80;
 	u8 *cfg = (u8 *)cfg_stream_vaddr;
 	struct mxc_jpeg_sof *sof;
 	struct mxc_jpeg_sos *sos;
-- 
2.36.1

Re: [PATCH v2] media: imx-jpeg: Leave a blank space before the configuration data

Posted by Fabio Estevam 3 years, 11 months ago

Hi Ming,

On Fri, May 27, 2022 at 7:25 AM Ming Qian <ming.qian@nxp.com> wrote:
>
> There is a hardware bug that it will load
> the first 128 bytes of configuration data twice,
> it will led to some configure error.
> so shift the configuration data 128 bytes,
> and make the first 128 bytes all zero,
> then hardware will load the 128 zero twice,
> and ignore them as garbage.
> then the configuration data can be loaded correctly
>
> Signed-off-by: Ming Qian <ming.qian@nxp.com>
> Reviewed-by: Mirela Rabulea <mirela.rabulea@nxp.com>
> Reviewed-by: Tommaso Merciai <tommaso.merciai@amarulasolutions.com>

Fixes tag?

RE: [EXT] Re: [PATCH v2] media: imx-jpeg: Leave a blank space before the configuration data

Posted by Ming Qian 3 years, 11 months ago

> From: Fabio Estevam <festevam@gmail.com>
> Sent: 2022年5月27日 19:12
> To: Ming Qian <ming.qian@nxp.com>
> Cc: Mauro Carvalho Chehab <mchehab@kernel.org>; Mirela Rabulea (OSS)
> <mirela.rabulea@oss.nxp.com>; Hans Verkuil <hverkuil-cisco@xs4all.nl>;
> Shawn Guo <shawnguo@kernel.org>; Sascha Hauer <s.hauer@pengutronix.de>;
> Sascha Hauer <kernel@pengutronix.de>; dl-linux-imx <linux-imx@nxp.com>;
> linux-media <linux-media@vger.kernel.org>; linux-kernel
> <linux-kernel@vger.kernel.org>; open list:OPEN FIRMWARE AND FLATTENED
> DEVICE TREE BINDINGS <devicetree@vger.kernel.org>; moderated
> list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE
> <linux-arm-kernel@lists.infradead.org>
> Subject: [EXT] Re: [PATCH v2] media: imx-jpeg: Leave a blank space before the
> configuration data
> 
> Caution: EXT Email
> 
> Hi Ming,
> 
> On Fri, May 27, 2022 at 7:25 AM Ming Qian <ming.qian@nxp.com> wrote:
> >
> > There is a hardware bug that it will load the first 128 bytes of
> > configuration data twice, it will led to some configure error.
> > so shift the configuration data 128 bytes, and make the first 128
> > bytes all zero, then hardware will load the 128 zero twice, and ignore
> > them as garbage.
> > then the configuration data can be loaded correctly
> >
> > Signed-off-by: Ming Qian <ming.qian@nxp.com>
> > Reviewed-by: Mirela Rabulea <mirela.rabulea@nxp.com>
> > Reviewed-by: Tommaso Merciai <tommaso.merciai@amarulasolutions.com>
> 
> Fixes tag?

Hi Fabio,
    It's a hardware issue, so I'm not sure is it a driver issue that I fix it.
    Or I just check which patch includes the code I changed, and add the fix tag?

Ming

Re: [EXT] Re: [PATCH v2] media: imx-jpeg: Leave a blank space before the configuration data

Posted by Nicolas Dufresne 3 years, 11 months ago

Le vendredi 27 mai 2022 à 11:33 +0000, Ming Qian a écrit :
> > From: Fabio Estevam <festevam@gmail.com>
> > Sent: 2022年5月27日 19:12
> > To: Ming Qian <ming.qian@nxp.com>
> > Cc: Mauro Carvalho Chehab <mchehab@kernel.org>; Mirela Rabulea (OSS)
> > <mirela.rabulea@oss.nxp.com>; Hans Verkuil <hverkuil-cisco@xs4all.nl>;
> > Shawn Guo <shawnguo@kernel.org>; Sascha Hauer <s.hauer@pengutronix.de>;
> > Sascha Hauer <kernel@pengutronix.de>; dl-linux-imx <linux-imx@nxp.com>;
> > linux-media <linux-media@vger.kernel.org>; linux-kernel
> > <linux-kernel@vger.kernel.org>; open list:OPEN FIRMWARE AND FLATTENED
> > DEVICE TREE BINDINGS <devicetree@vger.kernel.org>; moderated
> > list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE
> > <linux-arm-kernel@lists.infradead.org>
> > Subject: [EXT] Re: [PATCH v2] media: imx-jpeg: Leave a blank space before
> > the
> > configuration data
> > 
> > Caution: EXT Email
> > 
> > Hi Ming,
> > 
> > On Fri, May 27, 2022 at 7:25 AM Ming Qian <ming.qian@nxp.com> wrote:
> > > 
> > > There is a hardware bug that it will load the first 128 bytes of
> > > configuration data twice, it will led to some configure error.
> > > so shift the configuration data 128 bytes, and make the first 128
> > > bytes all zero, then hardware will load the 128 zero twice, and ignore
> > > them as garbage.
> > > then the configuration data can be loaded correctly
> > > 
> > > Signed-off-by: Ming Qian <ming.qian@nxp.com>
> > > Reviewed-by: Mirela Rabulea <mirela.rabulea@nxp.com>
> > > Reviewed-by: Tommaso Merciai <tommaso.merciai@amarulasolutions.com>
> > 
> > Fixes tag?
> 
> Hi Fabio,
>     It's a hardware issue, so I'm not sure is it a driver issue that I fix it.
>     Or I just check which patch includes the code I changed, and add the fix
> tag?

You can use Fixes tag even though there was no software bug. The point of the
tag is to help locate how far we can backport this patch in order to let stable
kernel users benefit.

regards,
Nicolas

> 
> Ming

RE: [EXT] Re: [PATCH v2] media: imx-jpeg: Leave a blank space before the configuration data

Posted by Ming Qian 3 years, 11 months ago

> From: Nicolas Dufresne <nicolas@ndufresne.ca>
> Sent: 2022年5月28日 3:26
> To: Ming Qian <ming.qian@nxp.com>; Fabio Estevam <festevam@gmail.com>
> Cc: Mauro Carvalho Chehab <mchehab@kernel.org>; Mirela Rabulea (OSS)
> <mirela.rabulea@oss.nxp.com>; Hans Verkuil <hverkuil-cisco@xs4all.nl>;
> Shawn Guo <shawnguo@kernel.org>; Sascha Hauer <s.hauer@pengutronix.de>;
> Sascha Hauer <kernel@pengutronix.de>; dl-linux-imx <linux-imx@nxp.com>;
> linux-media <linux-media@vger.kernel.org>; linux-kernel
> <linux-kernel@vger.kernel.org>; open list:OPEN FIRMWARE AND FLATTENED
> DEVICE TREE BINDINGS <devicetree@vger.kernel.org>; moderated
> list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE
> <linux-arm-kernel@lists.infradead.org>
> Subject: Re: [EXT] Re: [PATCH v2] media: imx-jpeg: Leave a blank space before
> the configuration data
> 
> Caution: EXT Email
> 
> Le vendredi 27 mai 2022 à 11:33 +0000, Ming Qian a écrit :
> > > From: Fabio Estevam <festevam@gmail.com>
> > > Sent: 2022年5月27日 19:12
> > > To: Ming Qian <ming.qian@nxp.com>
> > > Cc: Mauro Carvalho Chehab <mchehab@kernel.org>; Mirela Rabulea (OSS)
> > > <mirela.rabulea@oss.nxp.com>; Hans Verkuil
> > > <hverkuil-cisco@xs4all.nl>; Shawn Guo <shawnguo@kernel.org>; Sascha
> > > Hauer <s.hauer@pengutronix.de>; Sascha Hauer
> > > <kernel@pengutronix.de>; dl-linux-imx <linux-imx@nxp.com>;
> > > linux-media <linux-media@vger.kernel.org>; linux-kernel
> > > <linux-kernel@vger.kernel.org>; open list:OPEN FIRMWARE AND
> > > FLATTENED DEVICE TREE BINDINGS <devicetree@vger.kernel.org>;
> > > moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE
> > > <linux-arm-kernel@lists.infradead.org>
> > > Subject: [EXT] Re: [PATCH v2] media: imx-jpeg: Leave a blank space
> > > before the configuration data
> > >
> > > Caution: EXT Email
> > >
> > > Hi Ming,
> > >
> > > On Fri, May 27, 2022 at 7:25 AM Ming Qian <ming.qian@nxp.com> wrote:
> > > >
> > > > There is a hardware bug that it will load the first 128 bytes of
> > > > configuration data twice, it will led to some configure error.
> > > > so shift the configuration data 128 bytes, and make the first 128
> > > > bytes all zero, then hardware will load the 128 zero twice, and
> > > > ignore them as garbage.
> > > > then the configuration data can be loaded correctly
> > > >
> > > > Signed-off-by: Ming Qian <ming.qian@nxp.com>
> > > > Reviewed-by: Mirela Rabulea <mirela.rabulea@nxp.com>
> > > > Reviewed-by: Tommaso Merciai
> > > > <tommaso.merciai@amarulasolutions.com>
> > >
> > > Fixes tag?
> >
> > Hi Fabio,
> >     It's a hardware issue, so I'm not sure is it a driver issue that I fix it.
> >     Or I just check which patch includes the code I changed, and add
> > the fix tag?
> 
> You can use Fixes tag even though there was no software bug. The point of the
> tag is to help locate how far we can backport this patch in order to let stable
> kernel users benefit.
> 
> regards,
> Nicolas
> 

Hi Nicolas,
    Thanks for your information, I'll add a fix tag in v3.
Ming.

> >
> > Ming

Re: [PATCH v2] media: imx-jpeg: Leave a blank space before the configuration data

Posted by Tommaso Merciai 3 years, 11 months ago

On Fri, May 27, 2022 at 06:24:44PM +0800, Ming Qian wrote:
> There is a hardware bug that it will load
> the first 128 bytes of configuration data twice,
> it will led to some configure error.
> so shift the configuration data 128 bytes,
> and make the first 128 bytes all zero,
> then hardware will load the 128 zero twice,
> and ignore them as garbage.
> then the configuration data can be loaded correctly
> 
> Signed-off-by: Ming Qian <ming.qian@nxp.com>
> Reviewed-by: Mirela Rabulea <mirela.rabulea@nxp.com>
> Reviewed-by: Tommaso Merciai <tommaso.merciai@amarulasolutions.com>
> ---
> v2
> - add some comments about why the 0x80 offset is needed
>  drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
> index 734e1b65fbc7..c0fd030d0f19 100644
> --- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
> +++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
> @@ -519,6 +519,7 @@ static bool mxc_jpeg_alloc_slot_data(struct mxc_jpeg_dev *jpeg,
>  				     GFP_ATOMIC);
>  	if (!cfg_stm)
>  		goto err;
> +	memset(cfg_stm, 0, MXC_JPEG_MAX_CFG_STREAM);
>  	jpeg->slot_data[slot].cfg_stream_vaddr = cfg_stm;
>  
>  skip_alloc:
> @@ -755,7 +756,13 @@ static unsigned int mxc_jpeg_setup_cfg_stream(void *cfg_stream_vaddr,
>  					      u32 fourcc,
>  					      u16 w, u16 h)
>  {
> -	unsigned int offset = 0;
> +	/*
> +	 * There is a hardware issue that first 128 bytes of configuration data
> +	 * can't be loaded correctly.
> +	 * To avoid this issue, we need to write the configuration from
> +	 * an offset which should be no less than 0x80 (128 bytes).
> +	 */
> +	unsigned int offset = 0x80;
>  	u8 *cfg = (u8 *)cfg_stream_vaddr;
>  	struct mxc_jpeg_sof *sof;
>  	struct mxc_jpeg_sos *sos;
> -- 
> 2.36.1
> 

Looks good to me!

Thanks,
Tommaso

-- 
Tommaso Merciai
Embedded Linux Engineer
tommaso.merciai@amarulasolutions.com
__________________________________

Amarula Solutions SRL
Via Le Canevare 30, 31100 Treviso, Veneto, IT
T. +39 042 243 5310
info@amarulasolutions.com
www.amarulasolutions.com