Re: [PATCH] media: imx-jpeg: Add support for descriptor allocation from SRAM

["Ming Qian(OSS)" <ming.qian@oss.nxp.com>]

Hi Marek,

On 8/21/2025 12:29 AM, Marek Vasut wrote:
> [You don't often get email from marek.vasut@mailbox.org. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> 
> Add support for optional allocation of bitstream descriptors from SRAM
> instead of DRAM. In case the encoder/decoder DT node contains 'sram'
> property which points to 'mmio-sram', the driver will attempt to use
> the SRAM instead of DRAM for descriptor allocation, which might improve
> performance.
> 
> This however helps on i.MX95 with sporadic SLOTn_STATUS IMG_RD_ERR bit 11
> being triggered during JPEG encoding. The following pipeline triggers the
> problem when descriptors get allocated from DRAM, the pipeline often hangs
> after a few seconds and the encoder driver indicates "timeout, cancel it" :

It's a hardware bug in i.MX95 A0, and the i.MX95 B0 has fixed this issue.

Using sram instead of dram only improves timing, but doesn't completely 
circumvent the hardware bug

Regards,
Ming

> 
> gst-launch-1.0 videotestsrc ! video/x-raw,width=256,height=256,format=YUY2 ! \
>                 queue ! v4l2jpegenc ! queue ! fakesink
> 
> Signed-off-by: Marek Vasut <marek.vasut@mailbox.org>
> ---
> Cc: Fabio Estevam <festevam@gmail.com>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
> Cc: Ming Qian <ming.qian@oss.nxp.com>
> Cc: Mirela Rabulea <mirela.rabulea@nxp.com>
> Cc: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> Cc: Pengutronix Kernel Team <kernel@pengutronix.de>
> Cc: Sascha Hauer <s.hauer@pengutronix.de>
> Cc: Shawn Guo <shawnguo@kernel.org>
> Cc: imx@lists.linux.dev
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-media@vger.kernel.org
> ---
>   .../media/platform/nxp/imx-jpeg/mxc-jpeg.c    | 69 +++++++++++--------
>   .../media/platform/nxp/imx-jpeg/mxc-jpeg.h    |  1 +
>   2 files changed, 42 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
> index aef1d6473eb8d..0095c2182ed39 100644
> --- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
> +++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
> @@ -44,6 +44,7 @@
>   #include <linux/module.h>
>   #include <linux/io.h>
>   #include <linux/clk.h>
> +#include <linux/genalloc.h>
>   #include <linux/of_platform.h>
>   #include <linux/platform_device.h>
>   #include <linux/slab.h>
> @@ -783,32 +784,40 @@ static int mxc_get_free_slot(struct mxc_jpeg_slot_data *slot_data)
>          return -1;
>   }
> 
> +static void mxc_jpeg_free(struct mxc_jpeg_dev *jpeg, size_t size, void *addr, dma_addr_t handle)
> +{
> +       if (jpeg->sram_pool)
> +               gen_pool_free(jpeg->sram_pool, (unsigned long)addr, size);
> +       else
> +               dma_free_coherent(jpeg->dev, size, addr, handle);
> +}
> +
>   static void mxc_jpeg_free_slot_data(struct mxc_jpeg_dev *jpeg)
>   {
>          /* free descriptor for decoding/encoding phase */
> -       dma_free_coherent(jpeg->dev, sizeof(struct mxc_jpeg_desc),
> -                         jpeg->slot_data.desc,
> -                         jpeg->slot_data.desc_handle);
> +       mxc_jpeg_free(jpeg, sizeof(struct mxc_jpeg_desc),
> +                     jpeg->slot_data.desc,
> +                     jpeg->slot_data.desc_handle);
>          jpeg->slot_data.desc = NULL;
>          jpeg->slot_data.desc_handle = 0;
> 
>          /* free descriptor for encoder configuration phase / decoder DHT */
> -       dma_free_coherent(jpeg->dev, sizeof(struct mxc_jpeg_desc),
> -                         jpeg->slot_data.cfg_desc,
> -                         jpeg->slot_data.cfg_desc_handle);
> +       mxc_jpeg_free(jpeg, sizeof(struct mxc_jpeg_desc),
> +                     jpeg->slot_data.cfg_desc,
> +                     jpeg->slot_data.cfg_desc_handle);
>          jpeg->slot_data.cfg_desc_handle = 0;
>          jpeg->slot_data.cfg_desc = NULL;
> 
>          /* free configuration stream */
> -       dma_free_coherent(jpeg->dev, MXC_JPEG_MAX_CFG_STREAM,
> -                         jpeg->slot_data.cfg_stream_vaddr,
> -                         jpeg->slot_data.cfg_stream_handle);
> +       mxc_jpeg_free(jpeg, MXC_JPEG_MAX_CFG_STREAM,
> +                     jpeg->slot_data.cfg_stream_vaddr,
> +                     jpeg->slot_data.cfg_stream_handle);
>          jpeg->slot_data.cfg_stream_vaddr = NULL;
>          jpeg->slot_data.cfg_stream_handle = 0;
> 
> -       dma_free_coherent(jpeg->dev, jpeg->slot_data.cfg_dec_size,
> -                         jpeg->slot_data.cfg_dec_vaddr,
> -                         jpeg->slot_data.cfg_dec_daddr);
> +       mxc_jpeg_free(jpeg, jpeg->slot_data.cfg_dec_size,
> +                     jpeg->slot_data.cfg_dec_vaddr,
> +                     jpeg->slot_data.cfg_dec_daddr);
>          jpeg->slot_data.cfg_dec_size = 0;
>          jpeg->slot_data.cfg_dec_vaddr = NULL;
>          jpeg->slot_data.cfg_dec_daddr = 0;
> @@ -816,6 +825,14 @@ static void mxc_jpeg_free_slot_data(struct mxc_jpeg_dev *jpeg)
>          jpeg->slot_data.used = false;
>   }
> 
> +static struct mxc_jpeg_desc *mxc_jpeg_alloc(struct mxc_jpeg_dev *jpeg, size_t size, dma_addr_t *handle)
> +{
> +       if (jpeg->sram_pool)
> +               return gen_pool_dma_zalloc(jpeg->sram_pool, size, handle);
> +       else
> +               return dma_alloc_coherent(jpeg->dev, size, handle, GFP_ATOMIC);
> +}
> +
>   static bool mxc_jpeg_alloc_slot_data(struct mxc_jpeg_dev *jpeg)
>   {
>          struct mxc_jpeg_desc *desc;
> @@ -826,37 +843,29 @@ static bool mxc_jpeg_alloc_slot_data(struct mxc_jpeg_dev *jpeg)
>                  goto skip_alloc; /* already allocated, reuse it */
> 
>          /* allocate descriptor for decoding/encoding phase */
> -       desc = dma_alloc_coherent(jpeg->dev,
> -                                 sizeof(struct mxc_jpeg_desc),
> -                                 &jpeg->slot_data.desc_handle,
> -                                 GFP_ATOMIC);
> +       desc = mxc_jpeg_alloc(jpeg, sizeof(struct mxc_jpeg_desc),
> +                             &jpeg->slot_data.desc_handle);
>          if (!desc)
>                  goto err;
>          jpeg->slot_data.desc = desc;
> 
>          /* allocate descriptor for configuration phase (encoder only) */
> -       cfg_desc = dma_alloc_coherent(jpeg->dev,
> -                                     sizeof(struct mxc_jpeg_desc),
> -                                     &jpeg->slot_data.cfg_desc_handle,
> -                                     GFP_ATOMIC);
> +       cfg_desc = mxc_jpeg_alloc(jpeg, sizeof(struct mxc_jpeg_desc),
> +                                 &jpeg->slot_data.cfg_desc_handle);
>          if (!cfg_desc)
>                  goto err;
>          jpeg->slot_data.cfg_desc = cfg_desc;
> 
>          /* allocate configuration stream */
> -       cfg_stm = dma_alloc_coherent(jpeg->dev,
> -                                    MXC_JPEG_MAX_CFG_STREAM,
> -                                    &jpeg->slot_data.cfg_stream_handle,
> -                                    GFP_ATOMIC);
> +       cfg_stm = mxc_jpeg_alloc(jpeg, MXC_JPEG_MAX_CFG_STREAM,
> +                                &jpeg->slot_data.cfg_stream_handle);
>          if (!cfg_stm)
>                  goto err;
>          jpeg->slot_data.cfg_stream_vaddr = cfg_stm;
> 
>          jpeg->slot_data.cfg_dec_size = MXC_JPEG_PATTERN_WIDTH * MXC_JPEG_PATTERN_HEIGHT * 2;
> -       jpeg->slot_data.cfg_dec_vaddr = dma_alloc_coherent(jpeg->dev,
> -                                                          jpeg->slot_data.cfg_dec_size,
> -                                                          &jpeg->slot_data.cfg_dec_daddr,
> -                                                          GFP_ATOMIC);
> +       jpeg->slot_data.cfg_dec_vaddr = mxc_jpeg_alloc(jpeg, jpeg->slot_data.cfg_dec_size,
> +                                                      &jpeg->slot_data.cfg_dec_daddr);
>          if (!jpeg->slot_data.cfg_dec_vaddr)
>                  goto err;
> 
> @@ -2902,6 +2911,10 @@ static int mxc_jpeg_probe(struct platform_device *pdev)
>          jpeg->dev = dev;
>          jpeg->mode = mode;
> 
> +       /* SRAM pool is optional */
> +       jpeg->sram_pool = of_gen_pool_get(pdev->dev.of_node, "sram", 0);
> +       dev_info(dev, "Using DMA descriptor pool in %cRAM\n", jpeg->sram_pool ? 'S' : 'D');
> +
>          /* Get clocks */
>          ret = devm_clk_bulk_get_all(&pdev->dev, &jpeg->clks);
>          if (ret < 0) {
> diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.h b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.h
> index 7f0910fc9b47e..311f2f2ac519f 100644
> --- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.h
> +++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.h
> @@ -142,6 +142,7 @@ struct mxc_jpeg_dev {
>          int                             num_domains;
>          struct device                   **pd_dev;
>          struct device_link              **pd_link;
> +       struct gen_pool                 *sram_pool;
>   };
> 
>   /**
> --
> 2.50.1
>