Re: [PATCH v4 3/7] media: s5p-mfc: replace custom reserved memory handling code with generic one

2016-06-08 Thread Liviu Dudau
On Tue, May 24, 2016 at 03:31:26PM +0200, Marek Szyprowski wrote:
> This patch removes custom code for initialization and handling of
> reserved memory regions in s5p-mfc driver and replaces it with generic
> reserved memory regions api.
> 
> s5p-mfc driver now handles two reserved memory regions defined by
> generic reserved memory bindings. Support for non-dt platform has been
> removed, because all supported platforms have been already converted to
> device tree.
> 
> Signed-off-by: Marek Szyprowski 
> ---
>  drivers/media/platform/s5p-mfc/s5p_mfc.c | 138 
> ++-
>  1 file changed, 63 insertions(+), 75 deletions(-)
> 
> diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc.c 
> b/drivers/media/platform/s5p-mfc/s5p_mfc.c
> index d1d9d388..fff5f43 100644
> --- a/drivers/media/platform/s5p-mfc/s5p_mfc.c
> +++ b/drivers/media/platform/s5p-mfc/s5p_mfc.c
> @@ -22,6 +22,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include "s5p_mfc_common.h"
>  #include "s5p_mfc_ctrl.h"
> @@ -1043,66 +1044,71 @@ static const struct v4l2_file_operations s5p_mfc_fops 
> = {
>   .mmap = s5p_mfc_mmap,
>  };
>  
> -static int match_child(struct device *dev, void *data)
> -{
> - if (!dev_name(dev))
> - return 0;
> - return !strcmp(dev_name(dev), (char *)data);
> -}
> -
> +/* DMA memory related helper functions */
>  static void s5p_mfc_memdev_release(struct device *dev)
>  {
> - dma_release_declared_memory(dev);
> + of_reserved_mem_device_release(dev);
>  }
>  
> -static void *mfc_get_drv_data(struct platform_device *pdev);
> -
> -static int s5p_mfc_alloc_memdevs(struct s5p_mfc_dev *dev)
> +static struct device *s5p_mfc_alloc_memdev(struct device *dev,
> +const char *name, unsigned int idx)
>  {
> - unsigned int mem_info[2] = { };
> + struct device *child;
> + int ret;
>  
> - dev->mem_dev_l = devm_kzalloc(&dev->plat_dev->dev,
> - sizeof(struct device), GFP_KERNEL);
> - if (!dev->mem_dev_l) {
> - mfc_err("Not enough memory\n");
> - return -ENOMEM;
> + child = devm_kzalloc(dev, sizeof(struct device), GFP_KERNEL);
> + if (!child)
> + return NULL;
> +
> + device_initialize(child);
> + dev_set_name(child, "%s:%s", dev_name(dev), name);
> + child->parent = dev;
> + child->bus = dev->bus;
> + child->coherent_dma_mask = dev->coherent_dma_mask;
> + child->dma_mask = dev->dma_mask;
> + child->release = s5p_mfc_memdev_release;
> +
> + if (device_add(child) == 0) {
> + ret = of_reserved_mem_device_init_by_idx(child, dev->of_node,
> +  idx);
> + if (ret == 0)
> + return child;
>   }
>  
> - dev_set_name(dev->mem_dev_l, "%s", "s5p-mfc-l");
> - dev->mem_dev_l->release = s5p_mfc_memdev_release;
> - device_initialize(dev->mem_dev_l);
> - of_property_read_u32_array(dev->plat_dev->dev.of_node,
> - "samsung,mfc-l", mem_info, 2);
> - if (dma_declare_coherent_memory(dev->mem_dev_l, mem_info[0],
> - mem_info[0], mem_info[1],
> - DMA_MEMORY_MAP | DMA_MEMORY_EXCLUSIVE) == 0) {
> - mfc_err("Failed to declare coherent memory for\n"
> - "MFC device\n");
> - return -ENOMEM;
> - }
> + put_device(child);
> + return NULL;
> +}
>  
> - dev->mem_dev_r = devm_kzalloc(&dev->plat_dev->dev,
> - sizeof(struct device), GFP_KERNEL);
> - if (!dev->mem_dev_r) {
> - mfc_err("Not enough memory\n");
> - return -ENOMEM;
> - }
> +static int s5p_mfc_configure_dma_memory(struct s5p_mfc_dev *mfc_dev)
> +{
> + struct device *dev = &mfc_dev->plat_dev->dev;
>  
> - dev_set_name(dev->mem_dev_r, "%s", "s5p-mfc-r");
> - dev->mem_dev_r->release = s5p_mfc_memdev_release;
> - device_initialize(dev->mem_dev_r);
> - of_property_read_u32_array(dev->plat_dev->dev.of_node,
> - "samsung,mfc-r", mem_info, 2);
> - if (dma_declare_coherent_memory(dev->mem_dev_r, mem_info[0],
> - mem_info[0], mem_info[1],
> - DMA_MEMORY_MAP | DMA_MEMORY_EXCLUSIVE) == 0) {
> - pr_err("Failed to declare coherent memory for\n"
> - "MFC device\n");
> - return -ENOMEM;
> + /*
> +  * Create and initialize virtual devices for accessing
> +  * reserved memory regions.
> +  */
> + mfc_dev->mem_dev_l = s5p_mfc_alloc_memdev(dev, "left",
> +   MFC_BANK1_ALLOC_CTX);
> + if (!mfc_dev->mem_dev_l)
> + return -ENODEV;
> + mfc_dev->mem_dev_r = s5p_mfc_alloc_memdev(dev, "right",
> +   MFC_BANK2_ALLOC_CTX);
> + if (!mfc_dev->mem_dev_r) {
> +  

Re: [PATCH v4 3/7] media: s5p-mfc: replace custom reserved memory handling code with generic one

2016-05-25 Thread Javier Martinez Canillas
Hello Marek,

On 05/25/2016 11:42 AM, Javier Martinez Canillas wrote:

[snip]

> 
> I couldn't test this exact patch because my Odroid XU4 died but I've tested
> the previous version and the only difference is that the memory reserve is
> made by index now instead of name, so I think you can add:
> 
> Tested-by: Javier Martinez Canillas 
> 

And now I also could test v4 of this patch on an Exynos5800 Peach Chromebook.

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH v4 3/7] media: s5p-mfc: replace custom reserved memory handling code with generic one

2016-05-25 Thread Javier Martinez Canillas
Hello Marek,

On 05/24/2016 09:31 AM, Marek Szyprowski wrote:
> This patch removes custom code for initialization and handling of
> reserved memory regions in s5p-mfc driver and replaces it with generic
> reserved memory regions api.
> 
> s5p-mfc driver now handles two reserved memory regions defined by
> generic reserved memory bindings. Support for non-dt platform has been
> removed, because all supported platforms have been already converted to
> device tree.
> 
> Signed-off-by: Marek Szyprowski 
> ---

Similar question than for patch #2, shouldn't we include the in-kernel DTS
changes with this patch to keep bisectability? Otherwise the mfc-{l,r} mem
allocation will fail after this patch.

The patch looks good to me though:

Reviewed-by: Javier Martinez Canillas 

I couldn't test this exact patch because my Odroid XU4 died but I've tested
the previous version and the only difference is that the memory reserve is
made by index now instead of name, so I think you can add:

Tested-by: Javier Martinez Canillas 

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html