Re: [PATCH v2 4/7] media: iris: add context bank devices using iommu-map

[Vikash Garodia <vikash.garodia@oss.qualcomm.com>]

On 3/6/2026 12:02 AM, Dmitry Baryshkov wrote:
> On Thu, Mar 05, 2026 at 10:56:40PM +0530, Vikash Garodia wrote:
>>
>> On 3/5/2026 7:51 PM, Dmitry Baryshkov wrote:
>>> On Thu, Mar 05, 2026 at 06:19:52PM +0530, Vikash Garodia wrote:
>>>>
>>>> On 3/4/2026 3:55 AM, Dmitry Baryshkov wrote:
>>>>> On Wed, Mar 04, 2026 at 12:16:50AM +0530, Vikash Garodia wrote:
>>>>>>
>>>>>> On 2/28/2026 1:50 AM, Dmitry Baryshkov wrote:
>>>>>>> On Fri, Feb 27, 2026 at 07:41:20PM +0530, Vikash Garodia wrote:
>>>>>>>> Introduce different context banks(CB) and the associated buffer region.
>>>>>>>> Different stream IDs from VPU would be associated to one of these CB.
>>>>>>>> Multiple CBs are needed to increase the IOVA for the video usecases like
>>>>>>>> higher concurrent sessions.
>>>>>>>>
>>>>>>>> Co-developed-by: Vishnu Reddy <busanna.reddy@oss.qualcomm.com>
>>>>>>>> Signed-off-by: Vishnu Reddy <busanna.reddy@oss.qualcomm.com>
>>>>>>>> Signed-off-by: Vikash Garodia <vikash.garodia@oss.qualcomm.com>
>>>>>>>> ---
>>>>>>>>      .../platform/qcom/iris/iris_platform_common.h      | 18 +++++++
>>>>>>>>      drivers/media/platform/qcom/iris/iris_probe.c      | 60 ++++++++++++++++++++--
>>>>>>>>      drivers/media/platform/qcom/iris/iris_resources.c  | 36 +++++++++++++
>>>>>>>>      drivers/media/platform/qcom/iris/iris_resources.h  |  1 +
>>>>>>>>      4 files changed, 111 insertions(+), 4 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/media/platform/qcom/iris/iris_platform_common.h b/drivers/media/platform/qcom/iris/iris_platform_common.h
>>>>>>>> index 5a489917580eb10022fdcb52f7321a915e8b239d..03c50d6e54853fca34d7d32f65d09eb80945fcdd 100644
>>>>>>>> --- a/drivers/media/platform/qcom/iris/iris_platform_common.h
>>>>>>>> +++ b/drivers/media/platform/qcom/iris/iris_platform_common.h
>>>>>>>> @@ -204,6 +204,22 @@ struct icc_vote_data {
>>>>>>>>      	u32 fps;
>>>>>>>>      };
>>>>>>>> +enum iris_buffer_region {
>>>>>>>> +	IRIS_BITSTREAM_REGION		= BIT(0),
>>>>>>>> +	IRIS_NON_PIXEL_REGION		= BIT(1),
>>>>>>>> +	IRIS_PIXEL_REGION		= BIT(2),
>>>>>>>> +	IRIS_SECURE_BITSTREAM_REGION	= BIT(3),
>>>>>>>> +	IRIS_SECURE_NON_PIXEL_REGION	= BIT(4),
>>>>>>>> +	IRIS_SECURE_PIXEL_REGION	= BIT(5),
>>>>>>>
>>>>>>> Can a context bank belong to multiple regions at the same time?
>>>>>>
>>>>>> yes, they would.
>>>>>
>>>>> How? Each set of CBs is defined by a separate function in the DT. How
>>>>> can CB belong to multiple regions? Could you please provide an example?
>>>>
>>>> SM8550 would have same stream id for VPU hardwares (tensilica and vcodec)
>>>> accessing bitstream and non pixel regions. Thereby non_pixel and bitstream
>>>> regions would map to one CB.
>>>
>>> In my opinion it means only one thing: you will have two CBs (one for
>>> non_pixel and one for bitstream) having the same SIDs. An alternative
>>> would be to define fallback rules (if CB foo doesn't exist, use CB bar).
>>>
>>>> While kaanapali would have different stream id for tensilica accessing non
>>>> pixel region and vcodec accessing bitstream region, thereby having different
>>>> CB.
>>>>
>>>>>
>>>>>>
>>>>>>>
>>>>>>>> +};
>>>>>>>> +
>>>>>>>> +struct iris_context_bank {
>>>>>>>> +	struct device *dev;
>>>>>>>
>>>>>>> Separate data and the actual device. Define a wrapper around struct
>>>>>>> device for the actual runtime usage.
>>>>>>
>>>>>> we still have to store the list of dynamically created device. Name can be
>>>>>> used to fetch the device from the list, i think the existing approach is
>>>>>> simpler ?
>>>>>
>>>>> You don't need a list. You have an array of the size, which is known and
>>>>> fixed. You have at most 9 functions, which means less than 9 devices.
>>>>>
>>>>
>>>> as mentioned above, its not the same for all platforms to have one to one
>>>> mapping between CBs and buffer region. Thereby indexing based on array would
>>>> be an issue here
>>>> It would end up something like this, considering [dev region] array,
>>>>
>>>> SM8550
>>>> non_pixel_device  non_pixel_region
>>>> non_pixel_device  bitstream_region
>>>> pixel_device      pixel_region
>>>>
>>>> kaanapali
>>>> non_pixel_device  non_pixel_region
>>>> bitstream_device  bitstream_region
>>>> pixel_device      pixel_region
>>>
>>> I'm sorry, I'm not sure I follow here. Could you please explain? Maybe
>>> by explititly mapping DT function values to iris_buffer_region values?
>>>
>>
>> Kaanapali
>> IRIS_BITSTREAM IRIS_BITSTREAM_REGION
>> IRIS_NON_PIXEL IRIS_NON_PIXEL_REGION	
>> IRIS_PIXEL     IRIS_PIXEL_REGION
>>
>> SM8550
>> IRIS_NON_PIXEL IRIS_NON_PIXEL_REGION | IRIS_BITSTREAM_REGION
>> IRIS_PIXEL     IRIS_PIXEL_REGION
> 
> So, why not:
> 
> Kaanapali:
> 
> iris_cb_dev_bs = iris_cb_dev_alloc(IRIS_BITSTREAM];
> iris_cb_dev_np = iris_cb_dev_alloc(IRIS_NON_PIXEL];
> iris_cb_dev_px = iris_cb_dev_alloc(IRIS_PIXEL];
> 
> core->cb_devs = {
> 	[IRIS_BITSTREAM_REGION] = iris_cb_dev_bs,
> 	[IRIS_NON_PIXEL_REGION] = iris_cb_dev_np,
> 	[IRIS_PIXEL_REGION] = iris_cb_dev_px;
> };
> 
> SM8550:
> 
> iris_cb_dev_np = iris_cb_dev_alloc(IRIS_NON_PIXEL];
> iris_cb_dev_px = iris_cb_dev_alloc(IRIS_PIXEL];
> 
> core->cb_devs = {
> 	[IRIS_BITSTREAM_REGION] = iris_cb_dev_np,
> 	[IRIS_NON_PIXEL_REGION] = iris_cb_dev_np,
> 	[IRIS_PIXEL_REGION] = iris_cb_dev_px;
> };
> 
> 
> Yes, it would require coding of those functions, however afterwards you
> can access necessary CB device simply by doing core->cb_devs[region].
> 
> I think current code is overcomplicated for the sake of having the
> platform flexibility expressed as data.
> 
>>
>>>>
>>>>
>>>>>>
>>>>>>>
>>>>>>>> +	const char *name;
>>>>>>>> +	const u32 f_id;
>>>>>>>> +	const enum iris_buffer_region region;
>>>>>>>> +};
>>>>>>>> +
>>>>>>>>      enum platform_pm_domain_type {
>>>>>>>>      	IRIS_CTRL_POWER_DOMAIN,
>>>>>>>>      	IRIS_HW_POWER_DOMAIN,
>>>>>>>> @@ -246,6 +262,8 @@ struct iris_platform_data {
>>>>>>>>      	u32 inst_fw_caps_enc_size;
>>>>>>>>      	const struct tz_cp_config *tz_cp_config_data;
>>>>>>>>      	u32 tz_cp_config_data_size;
>>>>>>>> +	struct iris_context_bank *cb_data;
>>>>>>>> +	u32 cb_data_size;
>>>>>>>
>>>>>>> Do they differ from platform to platform?
>>>>>> Yes
>>>>>>
>>>>>>> Mark them as const, it should be data only.
>>>>>>
>>>>>> cb_data_size can be marked as const
>>>>>
>>>>> Why is cb_data non-const?
>>>>
>>>> dev is being updated once created dynamically.
>>>
>>> That's a bad idea. Please make the platform description constant.
>>>
>>
>> I can give it a try to move CBs in core struct out of platform data and have
>> a buffer region based lookup array to fetch the device.
> 
> It might be easier to express that as a callback, filling core->cb_devs
> with struct device pointers, as I wrote above.

IIUC, introduce an ops like below and fill ore->cb_devs[] in SOC 
specific data i.e sm8550_data ?

struct iris_platform_ops {
     int (*populate_cb_devs)(struct iris_core *core);
};
struct iris_platform_data {
     const struct iris_platform_ops *ops;
     /* ... other pdata fields ... */
};

At present lemans shares the platform data of sm8550, with the above ops 
approach, that commonality leverage would be lost ?

Please correct if your proposal is different than what i interpreted.

> 
>>
>> Regards,
>> Vikash
>