Re: [PATCH v2 5/7] media: iris: add helper to select context bank device

[Dmitry Baryshkov]

On Wed, Mar 04, 2026 at 12:46:27AM +0530, Vikash Garodia wrote:
> 
> 
> On 2/28/2026 1:57 AM, Dmitry Baryshkov wrote:
> > On Fri, Feb 27, 2026 at 07:41:21PM +0530, Vikash Garodia wrote:
> > > Depending on the buffer type (input, output, internal and interface
> > > queues), associated context bank is selected, if available. Fallback to
> > > parent device for backward compatibility.
> > > 
> > > Co-developed-by: Vishnu Reddy <busanna.reddy@xxxxxxxxxxxxxxxx>
> > > Signed-off-by: Vishnu Reddy <busanna.reddy@xxxxxxxxxxxxxxxx>
> > > Signed-off-by: Vikash Garodia <vikash.garodia@xxxxxxxxxxxxxxxx>
> > > ---
> > >   drivers/media/platform/qcom/iris/iris_buffer.c    |  7 +--
> > >   drivers/media/platform/qcom/iris/iris_buffer.h    |  2 +
> > >   drivers/media/platform/qcom/iris/iris_hfi_queue.c | 16 +++---
> > >   drivers/media/platform/qcom/iris/iris_resources.c | 60 +++++++++++++++++++++++
> > >   drivers/media/platform/qcom/iris/iris_resources.h |  2 +
> > >   drivers/media/platform/qcom/iris/iris_vidc.c      |  4 +-
> > >   6 files changed, 79 insertions(+), 12 deletions(-)
> > > 
> > > @@ -177,3 +178,62 @@ int iris_create_child_device_and_map(struct iris_core *core, struct iris_context
> > >   	return 0;
> > >   }
> > > +
> > > +static enum iris_buffer_region iris_get_region(struct iris_inst *inst,
> > > +					       enum iris_buffer_type buffer_type)
> > > +{
> > > +	switch (buffer_type) {
> > > +	case BUF_INPUT:
> > > +		if (inst && inst->domain == ENCODER)
> > 
> > Can inst be NULL here?
> 
> during queues init/deinit, instances are not created.

Is this function being called during queues init?

> 
> > 
> > > +			return IRIS_PIXEL_REGION;
> > > +		else if (inst && inst->domain == DECODER)
> > > +			return IRIS_BITSTREAM_REGION;
> > 
> > Are there any other possibilities than encoder and decoder?
> 
> will simplify it as
> 
> if (inst) {
>     if (inst->domain == ENCODER)
>         return IRIS_PIXEL_REGION;
>     else
>         return IRIS_BITSTREAM_REGION;
> }
> > 
> > > +		break;
> > > +	case BUF_OUTPUT:
> > > +		if (inst && inst->domain == ENCODER)
> > > +			return IRIS_BITSTREAM_REGION;
> > > +		else if (inst && inst->domain == DECODER)
> > > +			return IRIS_PIXEL_REGION;
> > > +		break;
> > > +	case BUF_BIN:
> > > +		return IRIS_BITSTREAM_REGION;
> > > +	case BUF_DPB:
> > > +	case BUF_PARTIAL:
> > > +	case BUF_SCRATCH_2:
> > > +	case BUF_VPSS:
> > > +		return IRIS_PIXEL_REGION;
> > > +	case BUF_ARP:
> > > +	case BUF_COMV:
> > > +	case BUF_HFI_QUEUE:
> > > +	case BUF_LINE:
> > > +	case BUF_NON_COMV:
> > > +	case BUF_PERSIST:
> > > +		return IRIS_NON_PIXEL_REGION;
> > > +	default:
> > > +		return 0;
> > 
> > dev_err(dev, "unsupported buffer type %x\n", buffer_type)
> > return -EINVAL;
> 
> these are bit fields, returning -EINVAL would still match some bits and can
> make the logic as true. 0 can be defined as IRIS_UNKNOWN_REGION

Yes, sounds good.

> 
> > 
> > > +	}
> > > +
> > > +	return 0;
> > 
> > Drop
> > 
> 
> Ack
> 
> > > +}
> > > +
> > > +struct device *iris_get_cb_dev(struct iris_core *core, struct iris_inst *inst,
> > > +			       enum iris_buffer_type buffer_type)
> > > +{
> > > +	enum iris_buffer_region region;
> > > +	struct device *dev = NULL;
> > > +	int i;
> > > +
> > > +	region = iris_get_region(inst, buffer_type);
> > > +
> > > +	for (i = 0; i < core->iris_platform_data->cb_data_size; i++) {
> > > +		if (core->iris_platform_data->cb_data[i].region & region) {
> > > +			dev = core->iris_platform_data->cb_data[i].dev;
> > > +			break;
> > > +		}
> > 
> > You really seem to overcomplicate things. Replace array search with the
> > indexed array access. Much easier and much better.
> > 
> > enum iris_buffer_region {
> > 	IRIS_PIXEL_REGION,
> > 	IRIS_BITSTREAM_REGION,
> > 	IRIS_NON_PIXEL_REGION,
> > 	// add more when necessary
> > 	IRIS_NUM_REGIONS,
> > };
> > 
> > struct iris_core {
> > 	struct iris_cb_device cb_devices[IRIS_NUM_REGIONS];
> > };
> > 
> > region = iris_get_region(inst, buffer_type);
> > dev = core->cb_devices[region];
> 
> all the regions may/may not be present in all SOC

You can check for dev != NULL afterwards.


-- 
With best wishes
Dmitry