[v2,08/15] media: s5p-mfc: Move firmware allocation to DMA configure function

Message ID 1487597944-2000-9-git-send-email-m.szyprowski@samsung.com
State New
Headers show
Series
  • [v2,01/15] media: s5p-mfc: Remove unused structures and dead code
Related show

Commit Message

Marek Szyprowski Feb. 20, 2017, 1:38 p.m.
To complete DMA memory configuration for MFC device, allocation of the
firmware buffer is needed, because some parameters are dependant on its base
address. Till now, this has been handled in the s5p_mfc_alloc_firmware()
function. This patch moves that logic to s5p_mfc_configure_dma_memory() to
keep DMA memory related operations in a single place. This way
s5p_mfc_alloc_firmware() is simplified and does what it name says. The
other consequence of this change is moving s5p_mfc_alloc_firmware() call
from the s5p_mfc_probe() function to the s5p_mfc_configure_dma_memory().

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>

---
 drivers/media/platform/s5p-mfc/s5p_mfc.c      | 62 +++++++++++++++++++++------
 drivers/media/platform/s5p-mfc/s5p_mfc_ctrl.c | 31 --------------
 2 files changed, 49 insertions(+), 44 deletions(-)

-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Javier Martinez Canillas Feb. 20, 2017, 1:49 p.m. | #1
Hello Marek,

On 02/20/2017 10:38 AM, Marek Szyprowski wrote:
> To complete DMA memory configuration for MFC device, allocation of the

> firmware buffer is needed, because some parameters are dependant on its base

> address. Till now, this has been handled in the s5p_mfc_alloc_firmware()

> function. This patch moves that logic to s5p_mfc_configure_dma_memory() to

> keep DMA memory related operations in a single place. This way

> s5p_mfc_alloc_firmware() is simplified and does what it name says. The

> other consequence of this change is moving s5p_mfc_alloc_firmware() call

> from the s5p_mfc_probe() function to the s5p_mfc_configure_dma_memory().

> 

> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>

> ---


Reviewed-by: Javier Martinez Canillas <javier@osg.samsung.com>


Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Shuah Khan Feb. 22, 2017, 6:07 p.m. | #2
On Mon, Feb 20, 2017 at 6:38 AM, Marek Szyprowski
<m.szyprowski@samsung.com> wrote:
> To complete DMA memory configuration for MFC device, allocation of the

> firmware buffer is needed, because some parameters are dependant on its base

> address. Till now, this has been handled in the s5p_mfc_alloc_firmware()

> function. This patch moves that logic to s5p_mfc_configure_dma_memory() to

> keep DMA memory related operations in a single place. This way

> s5p_mfc_alloc_firmware() is simplified and does what it name says. The

> other consequence of this change is moving s5p_mfc_alloc_firmware() call

> from the s5p_mfc_probe() function to the s5p_mfc_configure_dma_memory().


Overall looks good. This patch makes subtle change in the dma and firwmare
initialization sequence. Might be okay, but wanted to call out just in case,

Before this change:
vb2_dma_contig_set_max_seg_size() is done for both iommu and non-iommu
case before s5p_mfc_alloc_firmware(). With this change setting
dma_contig max size happens after s5p_mfc_alloc_firmware(). From what
I can tell this might not be an issue.
vb2_dma_contig_clear_max_seg_size() still happens after
s5p_mfc_release_firmware(), so that part hasn't changed.

Do any of the dma_* calls made from s5p_mfc_alloc_firmware() and later
during the dma congiguration sequence depend on dmap_parms being
allocated? Doesn't looks like it from what I can tell, but safe to
ask.

thanks,
-- Shuah

>

> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>

> ---

>  drivers/media/platform/s5p-mfc/s5p_mfc.c      | 62 +++++++++++++++++++++------

>  drivers/media/platform/s5p-mfc/s5p_mfc_ctrl.c | 31 --------------

>  2 files changed, 49 insertions(+), 44 deletions(-)

>

> diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc.c b/drivers/media/platform/s5p-mfc/s5p_mfc.c

> index bc1aeb25ebeb..4403487a494a 100644

> --- a/drivers/media/platform/s5p-mfc/s5p_mfc.c

> +++ b/drivers/media/platform/s5p-mfc/s5p_mfc.c

> @@ -1110,6 +1110,11 @@ static struct device *s5p_mfc_alloc_memdev(struct device *dev,

>  static int s5p_mfc_configure_dma_memory(struct s5p_mfc_dev *mfc_dev)

>  {

>         struct device *dev = &mfc_dev->plat_dev->dev;

> +       void *bank2_virt;

> +       dma_addr_t bank2_dma_addr;

> +       unsigned long align_size = 1 << MFC_BASE_ALIGN_ORDER;

> +       struct s5p_mfc_priv_buf *fw_buf = &mfc_dev->fw_buf;

> +       int ret;

>

>         /*

>          * When IOMMU is available, we cannot use the default configuration,

> @@ -1122,14 +1127,21 @@ static int s5p_mfc_configure_dma_memory(struct s5p_mfc_dev *mfc_dev)

>         if (exynos_is_iommu_available(dev)) {

>                 int ret = exynos_configure_iommu(dev, S5P_MFC_IOMMU_DMA_BASE,

>                                                  S5P_MFC_IOMMU_DMA_SIZE);

> -               if (ret == 0) {

> -                       mfc_dev->mem_dev[BANK1_CTX] =

> -                               mfc_dev->mem_dev[BANK2_CTX] = dev;

> -                       vb2_dma_contig_set_max_seg_size(dev,

> -                                                       DMA_BIT_MASK(32));

> +               if (ret)

> +                       return ret;

> +

> +               mfc_dev->mem_dev[BANK1_CTX] = mfc_dev->mem_dev[BANK2_CTX] = dev;

> +               ret = s5p_mfc_alloc_firmware(mfc_dev);

> +               if (ret) {

> +                       exynos_unconfigure_iommu(dev);

> +                       return ret;

>                 }

>

> -               return ret;

> +               mfc_dev->dma_base[BANK1_CTX] = fw_buf->dma;

> +               mfc_dev->dma_base[BANK2_CTX] = fw_buf->dma;

> +               vb2_dma_contig_set_max_seg_size(dev, DMA_BIT_MASK(32));

> +

> +               return 0;

>         }

>

>         /*

> @@ -1147,6 +1159,35 @@ static int s5p_mfc_configure_dma_memory(struct s5p_mfc_dev *mfc_dev)

>                 return -ENODEV;

>         }

>

> +       /* Allocate memory for firmware and initialize both banks addresses */

> +       ret = s5p_mfc_alloc_firmware(mfc_dev);

> +       if (ret) {

> +               device_unregister(mfc_dev->mem_dev[BANK2_CTX]);

> +               device_unregister(mfc_dev->mem_dev[BANK1_CTX]);

> +               return ret;

> +       }

> +

> +       mfc_dev->dma_base[BANK1_CTX] = fw_buf->dma;

> +

> +       bank2_virt = dma_alloc_coherent(mfc_dev->mem_dev[BANK2_CTX], align_size,

> +                                       &bank2_dma_addr, GFP_KERNEL);

> +       if (!bank2_virt) {

> +               mfc_err("Allocating bank2 base failed\n");

> +               s5p_mfc_release_firmware(mfc_dev);

> +               device_unregister(mfc_dev->mem_dev[BANK2_CTX]);

> +               device_unregister(mfc_dev->mem_dev[BANK1_CTX]);

> +               return -ENOMEM;

> +       }

> +

> +       /* Valid buffers passed to MFC encoder with LAST_FRAME command

> +        * should not have address of bank2 - MFC will treat it as a null frame.

> +        * To avoid such situation we set bank2 address below the pool address.

> +        */

> +       mfc_dev->dma_base[BANK2_CTX] = bank2_dma_addr - align_size;

> +

> +       dma_free_coherent(mfc_dev->mem_dev[BANK2_CTX], align_size, bank2_virt,

> +                         bank2_dma_addr);

> +

>         vb2_dma_contig_set_max_seg_size(mfc_dev->mem_dev[BANK1_CTX],

>                                         DMA_BIT_MASK(32));

>         vb2_dma_contig_set_max_seg_size(mfc_dev->mem_dev[BANK2_CTX],

> @@ -1159,6 +1200,8 @@ static void s5p_mfc_unconfigure_dma_memory(struct s5p_mfc_dev *mfc_dev)

>  {

>         struct device *dev = &mfc_dev->plat_dev->dev;

>

> +       s5p_mfc_release_firmware(mfc_dev);

> +

>         if (exynos_is_iommu_available(dev)) {

>                 exynos_unconfigure_iommu(dev);

>                 vb2_dma_contig_clear_max_seg_size(dev);

> @@ -1235,10 +1278,6 @@ static int s5p_mfc_probe(struct platform_device *pdev)

>         dev->watchdog_timer.data = (unsigned long)dev;

>         dev->watchdog_timer.function = s5p_mfc_watchdog;

>

> -       ret = s5p_mfc_alloc_firmware(dev);

> -       if (ret)

> -               goto err_res;

> -

>         ret = v4l2_device_register(&pdev->dev, &dev->v4l2_dev);

>         if (ret)

>                 goto err_v4l2_dev_reg;

> @@ -1313,8 +1352,6 @@ static int s5p_mfc_probe(struct platform_device *pdev)

>  err_dec_alloc:

>         v4l2_device_unregister(&dev->v4l2_dev);

>  err_v4l2_dev_reg:

> -       s5p_mfc_release_firmware(dev);

> -err_res:

>         s5p_mfc_final_pm(dev);

>  err_dma:

>         s5p_mfc_unconfigure_dma_memory(dev);

> @@ -1356,7 +1393,6 @@ static int s5p_mfc_remove(struct platform_device *pdev)

>         video_device_release(dev->vfd_enc);

>         video_device_release(dev->vfd_dec);

>         v4l2_device_unregister(&dev->v4l2_dev);

> -       s5p_mfc_release_firmware(dev);

>         s5p_mfc_unconfigure_dma_memory(dev);

>

>         s5p_mfc_final_pm(dev);

> diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_ctrl.c b/drivers/media/platform/s5p-mfc/s5p_mfc_ctrl.c

> index 50d698968049..b0cf3970117a 100644

> --- a/drivers/media/platform/s5p-mfc/s5p_mfc_ctrl.c

> +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_ctrl.c

> @@ -26,9 +26,6 @@

>  /* Allocate memory for firmware */

>  int s5p_mfc_alloc_firmware(struct s5p_mfc_dev *dev)

>  {

> -       void *bank2_virt;

> -       dma_addr_t bank2_dma_addr;

> -       unsigned int align_size = 1 << MFC_BASE_ALIGN_ORDER;

>         struct s5p_mfc_priv_buf *fw_buf = &dev->fw_buf;

>

>         fw_buf->size = dev->variant->buf_size->fw;

> @@ -44,35 +41,7 @@ int s5p_mfc_alloc_firmware(struct s5p_mfc_dev *dev)

>                 mfc_err("Allocating bitprocessor buffer failed\n");

>                 return -ENOMEM;

>         }

> -       dev->dma_base[BANK1_CTX] = fw_buf->dma;

> -

> -       if (HAS_PORTNUM(dev) && IS_TWOPORT(dev)) {

> -               bank2_virt = dma_alloc_coherent(dev->mem_dev[BANK2_CTX],

> -                                      align_size, &bank2_dma_addr, GFP_KERNEL);

> -

> -               if (!bank2_virt) {

> -                       mfc_err("Allocating bank2 base failed\n");

> -                       dma_free_coherent(dev->mem_dev[BANK1_CTX], fw_buf->size,

> -                                         fw_buf->virt, fw_buf->dma);

> -                       fw_buf->virt = NULL;

> -                       return -ENOMEM;

> -               }

> -

> -               /* Valid buffers passed to MFC encoder with LAST_FRAME command

> -                * should not have address of bank2 - MFC will treat it as a null frame.

> -                * To avoid such situation we set bank2 address below the pool address.

> -                */

> -               dev->dma_base[BANK2_CTX] = bank2_dma_addr - align_size;

>

> -               dma_free_coherent(dev->mem_dev[BANK2_CTX], align_size,

> -                                 bank2_virt, bank2_dma_addr);

> -

> -       } else {

> -               /* In this case bank2 can point to the same address as bank1.

> -                * Firmware will always occupy the beginning of this area so it is

> -                * impossible having a video frame buffer with zero address. */

> -               dev->dma_base[BANK2_CTX] = dev->dma_base[BANK1_CTX];

> -       }

>         return 0;

>  }

>

> --

> 1.9.1

>

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Marek Szyprowski Feb. 23, 2017, 7:30 a.m. | #3
Hi Shuah,


On 2017-02-22 19:07, Shuah Khan wrote:
> On Mon, Feb 20, 2017 at 6:38 AM, Marek Szyprowski

> <m.szyprowski@samsung.com> wrote:

>> To complete DMA memory configuration for MFC device, allocation of the

>> firmware buffer is needed, because some parameters are dependant on its base

>> address. Till now, this has been handled in the s5p_mfc_alloc_firmware()

>> function. This patch moves that logic to s5p_mfc_configure_dma_memory() to

>> keep DMA memory related operations in a single place. This way

>> s5p_mfc_alloc_firmware() is simplified and does what it name says. The

>> other consequence of this change is moving s5p_mfc_alloc_firmware() call

>> from the s5p_mfc_probe() function to the s5p_mfc_configure_dma_memory().

> Overall looks good. This patch makes subtle change in the dma and firwmare

> initialization sequence. Might be okay, but wanted to call out just in case,

>

> Before this change:

> vb2_dma_contig_set_max_seg_size() is done for both iommu and non-iommu

> case before s5p_mfc_alloc_firmware(). With this change setting

> dma_contig max size happens after s5p_mfc_alloc_firmware(). From what

> I can tell this might not be an issue.

> vb2_dma_contig_clear_max_seg_size() still happens after

> s5p_mfc_release_firmware(), so that part hasn't changed.

>

> Do any of the dma_* calls made from s5p_mfc_alloc_firmware() and later

> during the dma congiguration sequence depend on dmap_parms being

> allocated? Doesn't looks like it from what I can tell, but safe to

> ask.


Firmware allocation doesn't depend on dma max segment size at all. The only
calls which might depend on it are dma_map_sg(), which are performed much
later as a part of video buffer allocation/preparation in videobuf2, when
dma-buf or user pointer v4l2 modes are selected.

 > [...]


Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch hide | download patch | download mbox

diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc.c b/drivers/media/platform/s5p-mfc/s5p_mfc.c
index bc1aeb25ebeb..4403487a494a 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc.c
@@ -1110,6 +1110,11 @@  static struct device *s5p_mfc_alloc_memdev(struct device *dev,
 static int s5p_mfc_configure_dma_memory(struct s5p_mfc_dev *mfc_dev)
 {
 	struct device *dev = &mfc_dev->plat_dev->dev;
+	void *bank2_virt;
+	dma_addr_t bank2_dma_addr;
+	unsigned long align_size = 1 << MFC_BASE_ALIGN_ORDER;
+	struct s5p_mfc_priv_buf *fw_buf = &mfc_dev->fw_buf;
+	int ret;
 
 	/*
 	 * When IOMMU is available, we cannot use the default configuration,
@@ -1122,14 +1127,21 @@  static int s5p_mfc_configure_dma_memory(struct s5p_mfc_dev *mfc_dev)
 	if (exynos_is_iommu_available(dev)) {
 		int ret = exynos_configure_iommu(dev, S5P_MFC_IOMMU_DMA_BASE,
 						 S5P_MFC_IOMMU_DMA_SIZE);
-		if (ret == 0) {
-			mfc_dev->mem_dev[BANK1_CTX] =
-				mfc_dev->mem_dev[BANK2_CTX] = dev;
-			vb2_dma_contig_set_max_seg_size(dev,
-							DMA_BIT_MASK(32));
+		if (ret)
+			return ret;
+
+		mfc_dev->mem_dev[BANK1_CTX] = mfc_dev->mem_dev[BANK2_CTX] = dev;
+		ret = s5p_mfc_alloc_firmware(mfc_dev);
+		if (ret) {
+			exynos_unconfigure_iommu(dev);
+			return ret;
 		}
 
-		return ret;
+		mfc_dev->dma_base[BANK1_CTX] = fw_buf->dma;
+		mfc_dev->dma_base[BANK2_CTX] = fw_buf->dma;
+		vb2_dma_contig_set_max_seg_size(dev, DMA_BIT_MASK(32));
+
+		return 0;
 	}
 
 	/*
@@ -1147,6 +1159,35 @@  static int s5p_mfc_configure_dma_memory(struct s5p_mfc_dev *mfc_dev)
 		return -ENODEV;
 	}
 
+	/* Allocate memory for firmware and initialize both banks addresses */
+	ret = s5p_mfc_alloc_firmware(mfc_dev);
+	if (ret) {
+		device_unregister(mfc_dev->mem_dev[BANK2_CTX]);
+		device_unregister(mfc_dev->mem_dev[BANK1_CTX]);
+		return ret;
+	}
+
+	mfc_dev->dma_base[BANK1_CTX] = fw_buf->dma;
+
+	bank2_virt = dma_alloc_coherent(mfc_dev->mem_dev[BANK2_CTX], align_size,
+					&bank2_dma_addr, GFP_KERNEL);
+	if (!bank2_virt) {
+		mfc_err("Allocating bank2 base failed\n");
+		s5p_mfc_release_firmware(mfc_dev);
+		device_unregister(mfc_dev->mem_dev[BANK2_CTX]);
+		device_unregister(mfc_dev->mem_dev[BANK1_CTX]);
+		return -ENOMEM;
+	}
+
+	/* Valid buffers passed to MFC encoder with LAST_FRAME command
+	 * should not have address of bank2 - MFC will treat it as a null frame.
+	 * To avoid such situation we set bank2 address below the pool address.
+	 */
+	mfc_dev->dma_base[BANK2_CTX] = bank2_dma_addr - align_size;
+
+	dma_free_coherent(mfc_dev->mem_dev[BANK2_CTX], align_size, bank2_virt,
+			  bank2_dma_addr);
+
 	vb2_dma_contig_set_max_seg_size(mfc_dev->mem_dev[BANK1_CTX],
 					DMA_BIT_MASK(32));
 	vb2_dma_contig_set_max_seg_size(mfc_dev->mem_dev[BANK2_CTX],
@@ -1159,6 +1200,8 @@  static void s5p_mfc_unconfigure_dma_memory(struct s5p_mfc_dev *mfc_dev)
 {
 	struct device *dev = &mfc_dev->plat_dev->dev;
 
+	s5p_mfc_release_firmware(mfc_dev);
+
 	if (exynos_is_iommu_available(dev)) {
 		exynos_unconfigure_iommu(dev);
 		vb2_dma_contig_clear_max_seg_size(dev);
@@ -1235,10 +1278,6 @@  static int s5p_mfc_probe(struct platform_device *pdev)
 	dev->watchdog_timer.data = (unsigned long)dev;
 	dev->watchdog_timer.function = s5p_mfc_watchdog;
 
-	ret = s5p_mfc_alloc_firmware(dev);
-	if (ret)
-		goto err_res;
-
 	ret = v4l2_device_register(&pdev->dev, &dev->v4l2_dev);
 	if (ret)
 		goto err_v4l2_dev_reg;
@@ -1313,8 +1352,6 @@  static int s5p_mfc_probe(struct platform_device *pdev)
 err_dec_alloc:
 	v4l2_device_unregister(&dev->v4l2_dev);
 err_v4l2_dev_reg:
-	s5p_mfc_release_firmware(dev);
-err_res:
 	s5p_mfc_final_pm(dev);
 err_dma:
 	s5p_mfc_unconfigure_dma_memory(dev);
@@ -1356,7 +1393,6 @@  static int s5p_mfc_remove(struct platform_device *pdev)
 	video_device_release(dev->vfd_enc);
 	video_device_release(dev->vfd_dec);
 	v4l2_device_unregister(&dev->v4l2_dev);
-	s5p_mfc_release_firmware(dev);
 	s5p_mfc_unconfigure_dma_memory(dev);
 
 	s5p_mfc_final_pm(dev);
diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_ctrl.c b/drivers/media/platform/s5p-mfc/s5p_mfc_ctrl.c
index 50d698968049..b0cf3970117a 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc_ctrl.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc_ctrl.c
@@ -26,9 +26,6 @@ 
 /* Allocate memory for firmware */
 int s5p_mfc_alloc_firmware(struct s5p_mfc_dev *dev)
 {
-	void *bank2_virt;
-	dma_addr_t bank2_dma_addr;
-	unsigned int align_size = 1 << MFC_BASE_ALIGN_ORDER;
 	struct s5p_mfc_priv_buf *fw_buf = &dev->fw_buf;
 
 	fw_buf->size = dev->variant->buf_size->fw;
@@ -44,35 +41,7 @@  int s5p_mfc_alloc_firmware(struct s5p_mfc_dev *dev)
 		mfc_err("Allocating bitprocessor buffer failed\n");
 		return -ENOMEM;
 	}
-	dev->dma_base[BANK1_CTX] = fw_buf->dma;
-
-	if (HAS_PORTNUM(dev) && IS_TWOPORT(dev)) {
-		bank2_virt = dma_alloc_coherent(dev->mem_dev[BANK2_CTX],
-				       align_size, &bank2_dma_addr, GFP_KERNEL);
-
-		if (!bank2_virt) {
-			mfc_err("Allocating bank2 base failed\n");
-			dma_free_coherent(dev->mem_dev[BANK1_CTX], fw_buf->size,
-					  fw_buf->virt, fw_buf->dma);
-			fw_buf->virt = NULL;
-			return -ENOMEM;
-		}
-
-		/* Valid buffers passed to MFC encoder with LAST_FRAME command
-		 * should not have address of bank2 - MFC will treat it as a null frame.
-		 * To avoid such situation we set bank2 address below the pool address.
-		 */
-		dev->dma_base[BANK2_CTX] = bank2_dma_addr - align_size;
 
-		dma_free_coherent(dev->mem_dev[BANK2_CTX], align_size,
-				  bank2_virt, bank2_dma_addr);
-
-	} else {
-		/* In this case bank2 can point to the same address as bank1.
-		 * Firmware will always occupy the beginning of this area so it is
-		 * impossible having a video frame buffer with zero address. */
-		dev->dma_base[BANK2_CTX] = dev->dma_base[BANK1_CTX];
-	}
 	return 0;
 }