Re: [PATCH 3/6] media: v4l2-isp: Rename v4l2_isp_params_buffer_size

[Antoine Bouyer <antoine.bouyer@nxp.com>]

On 5/6/26 10:35 AM, Jacopo Mondi wrote:
>
> 
> Hello Antoine
> 
> On Wed, May 06, 2026 at 10:11:31AM +0200, Antoine Bouyer wrote:
>> On 5/5/26 4:12 PM, Jacopo Mondi wrote:
>>>
>>>
>>> Rename v4l2_isp_params_buffer_size() to v4l2_isp_buffer_size() to
>>> support statistics.
>>>
>>> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
>>> ---
>>>    .../media/platform/arm/mali-c55/mali-c55-params.c  | 12 ++++++------
>>
>> Hi Jacopo
>>
>> Is it ok to meld driver and include changes and remove all occurrences of
>> v4l2_isp_params_buffer_size ? Of course it prevents build errors, but what
>> if pending changes are already using this macro ?
> 
> This is an internal header, we don't keep compatibility for internal
> kAPI symbols.
> 
> If you're referring to your in-review patches which use the now
> renamed 'v4l2_isp_params_buffer_size' symbol, I'm sorry but we'll
> have to synchronize here, there's no way around it.

That is fine, I already moved to common helper locally after your review.

> 
>>   >   include/media/v4l2-isp.h                           | 22
>> +++++++++++++---------
>>>    2 files changed, 19 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/drivers/media/platform/arm/mali-c55/mali-c55-params.c b/drivers/media/platform/arm/mali-c55/mali-c55-params.c
>>> index de0e9d898db7..dc483f0322d6 100644
>>> --- a/drivers/media/platform/arm/mali-c55/mali-c55-params.c
>>> +++ b/drivers/media/platform/arm/mali-c55/mali-c55-params.c
>>> @@ -487,7 +487,7 @@ static int mali_c55_params_g_fmt_meta_out(struct file *file, void *fh,
>>>    {
>>>           static const struct v4l2_meta_format mfmt = {
>>>                   .dataformat = V4L2_META_FMT_MALI_C55_PARAMS,
>>> -               .buffersize = v4l2_isp_params_buffer_size(MALI_C55_PARAMS_MAX_SIZE),
>>> +               .buffersize = v4l2_isp_buffer_size(MALI_C55_PARAMS_MAX_SIZE),
>>>           };
>>>
>>>           f->fmt.meta = mfmt;
>>> @@ -540,13 +540,13 @@ mali_c55_params_queue_setup(struct vb2_queue *q, unsigned int *num_buffers,
>>>           if (*num_planes && *num_planes > 1)
>>>                   return -EINVAL;
>>>
>>> -       if (sizes[0] && sizes[0] < v4l2_isp_params_buffer_size(MALI_C55_PARAMS_MAX_SIZE))
>>> +       if (sizes[0] && sizes[0] < v4l2_isp_buffer_size(MALI_C55_PARAMS_MAX_SIZE))
>>>                   return -EINVAL;
>>>
>>>           *num_planes = 1;
>>>
>>>           if (!sizes[0])
>>> -               sizes[0] = v4l2_isp_params_buffer_size(MALI_C55_PARAMS_MAX_SIZE);
>>> +               sizes[0] = v4l2_isp_buffer_size(MALI_C55_PARAMS_MAX_SIZE);
>>>
>>>           return 0;
>>>    }
>>> @@ -556,7 +556,7 @@ static int mali_c55_params_buf_init(struct vb2_buffer *vb)
>>>           struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
>>>           struct mali_c55_params_buf *buf = to_mali_c55_params_buf(vbuf);
>>>
>>> -       buf->config = kvmalloc(v4l2_isp_params_buffer_size(MALI_C55_PARAMS_MAX_SIZE),
>>> +       buf->config = kvmalloc(v4l2_isp_buffer_size(MALI_C55_PARAMS_MAX_SIZE),
>>>                                  GFP_KERNEL);
>>>           if (!buf->config)
>>>                   return -ENOMEM;
>>> @@ -583,7 +583,7 @@ static int mali_c55_params_buf_prepare(struct vb2_buffer *vb)
>>>           int ret;
>>>
>>>           ret = v4l2_isp_params_validate_buffer_size(mali_c55->dev, vb,
>>> -                       v4l2_isp_params_buffer_size(MALI_C55_PARAMS_MAX_SIZE));
>>> +                       v4l2_isp_buffer_size(MALI_C55_PARAMS_MAX_SIZE));
>>>           if (ret)
>>>                   return ret;
>>>
>>> @@ -593,7 +593,7 @@ static int mali_c55_params_buf_prepare(struct vb2_buffer *vb)
>>>            * changed to the buffer content whilst the driver processes it.
>>>            */
>>>
>>> -       memcpy(buf->config, config, v4l2_isp_params_buffer_size(MALI_C55_PARAMS_MAX_SIZE));
>>> +       memcpy(buf->config, config, v4l2_isp_buffer_size(MALI_C55_PARAMS_MAX_SIZE));
>>>
>>>           return v4l2_isp_params_validate_buffer(mali_c55->dev, vb, buf->config,
>>>                                                  mali_c55_params_block_types_info,
>>> diff --git a/include/media/v4l2-isp.h b/include/media/v4l2-isp.h
>>> index f3a6d0edcb24..d70ed6b431e7 100644
>>> --- a/include/media/v4l2-isp.h
>>> +++ b/include/media/v4l2-isp.h
>>> @@ -15,17 +15,21 @@ struct device;
>>>    struct vb2_buffer;
>>>
>>>    /**
>>> - * v4l2_isp_params_buffer_size - Calculate size of v4l2_isp_params_buffer
>>> - * @max_params_size: The total size of the ISP configuration blocks
>>> + * v4l2_isp_buffer_size - Calculate size of v4l2_isp_buffer
>>> + * @max_size: The total size of the ISP configuration or statistics blocks
>>> + *
>>> + * Users of v4l2-isp will have differing sized data arrays for parameters and
>>> + * statistics, depending on their specific blocks. Drivers need to be able to
>>> + * calculate the appropriate size of the buffer to accommodate all ISP blocks
>>> + * supported by the platform. This macro provides a convenient tool for the
>>> + * calculation.
>>> + *
>>> + * The intended users of this function are drivers initializing the size
>>> + * of their metadata (parameters and statistics) buffers.
>>>     *
>>> - * Users of the v4l2 extensible parameters will have differing sized data arrays
>>> - * depending on their specific parameter buffers. Drivers and userspace will
>>> - * need to be able to calculate the appropriate size of the struct to
>>> - * accommodate all ISP configuration blocks provided by the platform.
>>> - * This macro provides a convenient tool for the calculation.
>>>     */
>>> -#define v4l2_isp_params_buffer_size(max_params_size) \
>>> -       (offsetof(struct v4l2_isp_params_buffer, data) + (max_params_size))
>>> +#define v4l2_isp_buffer_size(max_size)                 \
>>> +       (offsetof(struct v4l2_isp_buffer, data) + (max_size))
>>
>> Shouldn't we keep v4l2_isp_params_buffer_size for compatibility ?
> 
> I don't think so, no. This is internal stuff, not exposed to
> userspace. We should port all existing users to the new symbol, don't
> we ?

Ok, I'm fine with this too.

BR
Antoine

> 
>>
>> BR
>> Antoine
> 
> Thanks!
> 
>>
>>>
>>>    /**
>>>     * v4l2_isp_params_validate_buffer_size - Validate a V4L2 ISP buffer sizes
>>>
>>> --
>>> 2.53.0
>>>
>>
>>