Hello Marek,
On 02/20/2017 10:23 AM, Marek Szyprowski wrote:
> Hi Javier,
>
> On 2017-02-17 22:13, Javier Martinez Canillas wrote:
>> On 02/14/2017 04:52 AM, Marek Szyprowski wrote:
>>> It turned out that all versions of MFC v6+ hardware doesn't have a strict
>>> requirement for ALL buffers to be allocated on higher addresses than the
>>> firmware base like it was documented for MFC v5. This requirement is true
>>> only for the device and per-context buffers. All video data buffers can be
>>> allocated anywhere for all MFC v6+ versions. Basing on this fact, the
>>> special DMA configuration based on two reserved memory regions is not
>>> really needed for MFC v6+ devices, because the memory requirements for the
>>> firmware, device and per-context buffers can be fulfilled by the simple
>>> probe-time pre-allocated block allocator instroduced in previous patch.
>> s/instroduced/introduced
>>
>>> This patch enables support for such pre-allocated block based allocator
>>> always for MFC v6+ devices. Due to the limitations of the memory management
>>> subsystem the largest supported size of the pre-allocated buffer when no
>>> CMA (Contiguous Memory Allocator) is enabled is 4MiB.
>>>
>>> This patch also removes the requirement to provide two reserved memory
>>> regions for MFC v6+ devices in device tree. Now the driver is fully
>>> functional without them.
>>>
>> Great!
>>
>>> Signed-off-by: Marek Szyprowski <m.szyprowski@xxxxxxxxxxx>
>>> ---
>> The patch looks good to me though and it works on my tests,
>> I've just one comment below.
>>
>> Reviewed-by: Javier Martinez Canillas <javier@xxxxxxxxxxxxxxx>
>> Tested-by: Javier Martinez Canillas <javier@xxxxxxxxxxxxxxx>
>>
>>> Documentation/devicetree/bindings/media/s5p-mfc.txt | 2 +-
>>> drivers/media/platform/s5p-mfc/s5p_mfc.c | 9 ++++++---
>>> 2 files changed, 7 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/media/s5p-mfc.txt b/Documentation/devicetree/bindings/media/s5p-mfc.txt
>>> index 2c901286d818..d3404b5d4d17 100644
>>> --- a/Documentation/devicetree/bindings/media/s5p-mfc.txt
>>> +++ b/Documentation/devicetree/bindings/media/s5p-mfc.txt
>>> @@ -28,7 +28,7 @@ Optional properties:
>>> - memory-region : from reserved memory binding: phandles to two reserved
>>> memory regions, first is for "left" mfc memory bus interfaces,
>>> second if for the "right" mfc memory bus, used when no SYSMMU
>>> - support is available
>>> + support is available; used only by MFC v5 present in Exynos4 SoCs
>>> Obsolete properties:
>>> - samsung,mfc-r, samsung,mfc-l : support removed, please use memory-region
>>> diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc.c b/drivers/media/platform/s5p-mfc/s5p_mfc.c
>>> index 8fc6fe4ba087..36f0aec2a1b3 100644
>>> --- a/drivers/media/platform/s5p-mfc/s5p_mfc.c
>>> +++ b/drivers/media/platform/s5p-mfc/s5p_mfc.c
>>> @@ -1178,9 +1178,12 @@ static void s5p_mfc_unconfigure_2port_memory(struct s5p_mfc_dev *mfc_dev)
>>> static int s5p_mfc_configure_common_memory(struct s5p_mfc_dev *mfc_dev)
>>> {
>>> struct device *dev = &mfc_dev->plat_dev->dev;
>>> - unsigned long mem_size = SZ_8M;
>>> + unsigned long mem_size = SZ_4M;
>>> unsigned int bitmap_size;
>>> + if (IS_ENABLED(CONFIG_DMA_CMA) || exynos_is_iommu_available(dev))
>>> + mem_size = SZ_8M;
>>> +
>>> if (mfc_mem_size)
>>> mem_size = memparse(mfc_mem_size, NULL);
>>>
>> The driver allows the user to set mem_size > SZ_4M using the s5p_mfc.mem
>> parameter even when CMA ins't enabled and IOMMU isn't available. Maybe it
>> should fail early instead and notify the user what's missing?
>
> It will notify user that driver failed to preallocate memory. 4M is the upper
> limit for standard kernel configuration, but afair there were some kconfig
> knobs to force kernel to use larger buckets for buddy allocator (what changes
> the limit). Frankly I would leave it as is.
>
> If user wants to specify s5p-mfc.mem, he already has some knowledge how to
> configure the kernel. I don't think that driver should check all possible
> scenarios of failing and give detailed explanation what was configured
> wrong. You can also enable CMA and configure the 8MiB of the default global
> area. In such configuration preallocation of mfc firmware buffer will also
> fail. Should the driver care about it? IMO it is enough to tell user that
> preallocating of given megabytes failed.
>
Right, it makes sense to leave it at is then indeed and let the users figure
out why the allocation failed with their specific kernel configuration.
> Best regards
Best regards,
--
Javier Martinez Canillas
Open Source Group
Samsung Research America
