Re: [PATCH v10 16/17] media: platform: Add mali-c55 parameters video node

[Sakari Ailus <sakari.ailus@linux.intel.com>]

Hi Jacopo,

On Mon, Jun 30, 2025 at 03:59:42PM +0200, Jacopo Mondi wrote:
> Hi Sakari, Dan
> >
> > On 6/24/25 13:21, Daniel Scally wrote:
> > > +static void mali_c55_params_buf_queue(struct vb2_buffer *vb)
> > > +{
> > > +	struct mali_c55_params *params = vb2_get_drv_priv(vb->vb2_queue);
> > > +	struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> > > +	struct mali_c55_params_buf *buf = to_mali_c55_params_buf(vbuf);
> > > +	struct mali_c55 *mali_c55 = params->mali_c55;
> > > +	struct mali_c55_params_buffer *config;
> > > +	struct list_head *queue;
> > > +	size_t block_offset = 0;
> > > +	size_t max_offset;
> > > +
> > > +	/*
> > > +	 * Before accepting the buffer we should check that the data within it
> > > +	 * is valid.
> > > +	 */
> > > +	config = vb2_plane_vaddr(vb, 0);
> > > +
> > > +	if (config->total_size > MALI_C55_PARAMS_MAX_SIZE) {
> > > +		dev_dbg(mali_c55->dev, "Invalid parameters buffer size %u\n",
> > > +			config->total_size);
> > > +		goto err_buffer_done;
> > > +	}
> > > +
> > > +	/* Currently only v1 is supported */
> > > +	if (config->version != MALI_C55_PARAM_BUFFER_V1) {
> > > +		dev_dbg(mali_c55->dev, "Invalid parameters version\n");
> > > +		goto err_buffer_done;
> > > +	}
> > > +
> > > +	max_offset = config->total_size - sizeof(struct mali_c55_params_block_header);
> > > +	while (block_offset < max_offset) {
> > > +		const struct mali_c55_block_handler *block_handler;
> > > +		union mali_c55_params_block block;
> > > +
> > > +		block = (union mali_c55_params_block)
> > > +			 &config->data[block_offset];
> > > +
> > > +		if (block.header->type >= ARRAY_SIZE(mali_c55_block_handlers)) {
> > > +			dev_dbg(mali_c55->dev, "Invalid parameters block type\n");
> > > +			goto err_buffer_done;
> > > +		}
> > > +
> > > +		if (block_offset + block.header->size > config->total_size) {
> > > +			dev_dbg(mali_c55->dev, "Parameters block too large\n");
> > > +			goto err_buffer_done;
> > > +		}
> > > +
> > > +		block_handler = &mali_c55_block_handlers[block.header->type];
> > > +
> > > +		/*
> > > +		 * Userspace can optionally omit all but the header of a block
> > > +		 * if it only intends to disable the block.
> > > +		 */
> > > +		if (block.header->size != block_handler->size &&
> > > +		    block.header->size != sizeof(*block.header)) {
> > > +			dev_dbg(mali_c55->dev, "Invalid parameters block size\n");
> > > +			goto err_buffer_done;
> > > +		}
> > > +
> > > +		block_offset += block.header->size;
> >
> > I recall discussing with Jacopo in the context of another ISP driver
> > (Rockchip?) that this piece of non-trivial code should make it into the
> 
> I think it was in the context of the recent Amlogic C3 ISP review
> 
> and yes, c3_isp_params_vb2_buf_prepare() and
> rkisp1_params_prepare_ext_params() are identical.
> 
> The here introduced mali_c55_params_buf_queue() is indeed very
> similar (*). I guess it has been developed before the RkISP1
> implementation (from which the Amlogic one was copied) was refined.
> 
> *) The Mali C55 implementation happens at .buf_queue time and not at
> .buf_prepare time, not sure if it's intentional (and doesn't probably
> make much difference anyway). It also copies the parameters buffer
> -after- having validated the content, creating a tiny window between
> validation and copy where userspace can modify the buffer, Very
> unlikely but possible.
> 
> Understanding if the rkisp1/c3 routines can be copied in the mali c55
> implementation would make sure that yes, we should defintely move the
> validation to the core.
> 
> 
> > framework side before a next driver using it has been added. What's the
> > status of that? Same for other related bits.
> >
> 
> Yes, three users (if confirmed that mali can use the same validation
> code as the other two) are enough as indication we should move this to
> the framework. Problem is how to better design this.
> 
> My first thought would be to provide a .buf_prepare helper specific
> for extensible parameters that drivers can use. It would however need
> to receive some configuration parameters the validation code would
> need to know about: the version flags, the enable/disable flags, the
> expected sizes etc
> 
> Looking at other parts of the code that could be facotrized out,
> indeed the rkisp1_ext_params_config() and c3_isp_params_cfg_blocks()
> functions are very similar. The RkISP1 is a bit more complex
> (supports 'group' and 'features') and the handlers function
> signatures are slightly different. With some type punning and macro
> magic I'm sure we could factorize t out as well, but I would indeed
> start with validation...
> 
> Time for a drivers/media/v4l2-core/v4l2_extensible_params.c ?

I'd call this v4l2-params.c or something alike. Feel free to post an RFC.
:-)

If there are details that require different handling, it's possible to
have callbacks as well.

-- 
Kind regards,

Sakari Ailus