Received-SPF: pass (google.com: domain of linux-kernel+bounces-61384-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <9c5b7b2c-8a66-4173-dfe9-5724ec5f733d@ti.com>
Date: Mon, 12 Feb 2024 16:27:28 +0530
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.11.0
Subject: Re: [PATCH v2] dt-bindings: media: Add sram-size Property for Wave5
Content-Language: en-US
To: Nicolas Dufresne <nicolas@ndufresne.ca>, Brandon Brnich <b-brnich@ti.com>,
        Nishanth Menon <nm@ti.com>
CC: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>,
        Nas Chung
	<nas.chung@chipsnmedia.com>,
        Jackson Lee <jackson.lee@chipsnmedia.com>,
        Mauro
 Carvalho Chehab <mchehab@kernel.org>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Conor Dooley
	<conor+dt@kernel.org>, <linux-media@vger.kernel.org>,
        <devicetree@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        Vignesh
 Raghavendra <vigneshr@ti.com>,
        Darren Etheridge <detheridge@ti.com>
References: <20240201184238.2542695-1-b-brnich@ti.com>
 <1209b7cf-5be2-4107-aa6b-d67a32ea3737@linaro.org>
 <20240202125257.p4astjuxpzr5ltjs@dragster>
 <8091a8cf-c1c0-49b0-b136-1ad0d185aa6a@linaro.org>
 <20240202155813.szxvi7bfp5xh7rvw@babble>
 <adfef53c-d64e-4855-ab61-101b6fa419e5@linaro.org>
 <20240205141255.z5kybm42qld44tdz@portfolio>
 <20240205192003.3qns6cxqurqnnj7c@udba0500997>
 <ab029558-fc04-854e-1f97-785f5cec0681@ti.com>
 <fccdc181727307f52a36f3bb621d6a4e192096da.camel@ndufresne.ca>
From: Devarsh Thakkar <devarsht@ti.com>
In-Reply-To: <fccdc181727307f52a36f3bb621d6a4e192096da.camel@ndufresne.ca>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8bit

Hi Nicolas,

On 09/02/24 23:12, Nicolas Dufresne wrote:
> Le jeudi 08 février 2024 à 11:52 +0530, Devarsh Thakkar a écrit :
>> I think even with the approach selected in [1] i.e. referring the
>> mmio-sram node using DT property, you can still use dynamic SRAM
>> allocation.
>> The driver can still allocate from global sram pool dynamically using
>> of_gen_pool API as being explained here [3] i.e allocate when first
>> instance is opened and free up later when no instances are running.
>>
>> But I agree with Nishanth's point too that we may not want to give all
>> of SRAM to VPU. For e.g. on AM62A we have 64KiB SRAM and a 1080p
>> use-case requires 48KiB and even higher for 4K so if there is another
>> peripheral who is referring this sram node, then it may not get enough
>> as VPU will hog the major chunk (or all) of it while it is running and
>> this is where an optional property like sram-size will help to cap the
>> max sram usage for VPU and so this helps especially on platforms with
>> limited SRAM availability.
>>
>> As I understand, the sram size allocation is dependent on resolution and
>> once programmed can't be changed until all instances of VPU are done,
>> and we can't predict how many instances user will launch and with what
>> resolutions.
>>
>> So here's the flow we had thought of some time back :
>> 1) Define worst case sram size (per 4K use-case as I believe that's the
>> max for CnM wave521c) as a macro in driver
>>
>> Then the condition for determining sram size to be allocated should be
>> as below  :
>>
>> 2) When first instance of VPU is opened, allocate as per sram-size if
>> sram-size property is specified.
>>
>> 3) If sram-size is not specified then :
>>    -> Allocate as per worst case size macro defined in driver from sram
>> pool,
>>    -> If worst case size of SRAM > max SRAM size, then allocate
>>       max SRAM size
>>
>> 4). When all of the instances of VPU are closed, then free up all
>> allocated SRAM.
>>
>> [3] :
>> https://wiki.analog.com/resources/tools-software/linuxdsp/docs/linux-kernel-and-drivers/sram
> 
> Only issue here is that DT is not a use case configuration file. That DT
> parameter is meant for HW that simply cannot be operated without it. This is
> also edgy, because it also means that it should only be used if that information
> is not static and vary unpredictably per SoC, which seems generally unlikely. 
> 
> The Wave5 IP *can* work without it, so it should resort to something more
> dynamic. User configuration should be sorted out at the OS level.
> 
> Nicolas

Thanks, yeah that makes sense. The sram allocation size is dependent on
resolution so I think we should employ a fallback model for sram allocation as
described below :

1) Driver code should enumerate required sram sizes for each of the standard
resolutions (for e.g. #define 1080P_SRAM_SIZE 48128, #define 4K_SRAM_SIZE
66368 and so on...) and try to allocate per highest enumerated resolution
first) from sram pool using gen_pool_alloc when first instance of VPU is opened.

2) If the allocation fails, then driver should try to fallback to lower
resolution sram allocation size (viz. 1080P_SRAM_SIZE) for sram allocation.

3) When all of the VPU instances get closed, then free up all allocated SRAM
back to the pool so that other peripherals can use it.

Regards
Devarsh