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

[Dmitry Baryshkov]

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@xxxxxxxxxxxxxxxx>
> > > > > > > Signed-off-by: Vishnu Reddy <busanna.reddy@xxxxxxxxxxxxxxxx>
> > > > > > > Signed-off-by: Vikash Garodia <vikash.garodia@xxxxxxxxxxxxxxxx>
> > > > > > > ---
> > > > > > >     .../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.

> 
> Regards,
> Vikash

-- 
With best wishes
Dmitry