Re: [PATCH v4 01/11] media: v4l2-common: Add helpers to calculate bytesperline and sizeimage

[Jonas Karlman]

On 2023-11-08 03:45, Nicolas Dufresne wrote:
> Le dimanche 05 novembre 2023 à 16:55 +0000, Jonas Karlman a écrit :
>> Add helper functions to calculate plane bytesperline and sizeimage,
>> these new helpers consider bpp div, block width and height when
>> calculating plane bytesperline and sizeimage.
> 
> Is this only refactoring to reduce duplicated code ? I haven't seen
> what is new in there yet, maybe the commit message could clarify.

Yes, this is just some refactoring and is not really required.

Cannot remember exactly why I though this was needed back in v1/v2. I
probably had an issue with wrong bytesperline/sizeimage due to the
fractal bytes per pixel nature of the NV15/NV20 pixel formats. Hacked
the calculation and forgot to update in one of the 3 places they get
calculated, then decided to refactor after that failed.

Will update commit message to clarify that this is only refactoring and
has no real impact for this series in v5.

Regards,
Jonas

> 
> regards,
> Nicolas
> 
>>
>> Signed-off-by: Jonas Karlman <jonas@xxxxxxxxx>
>> ---
>> v4:
>> - No change
>>
>> v3:
>> - Consider bpp_div in calculation
>>
>>  drivers/media/v4l2-core/v4l2-common.c | 78 +++++++++++++--------------
>>  1 file changed, 39 insertions(+), 39 deletions(-)
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c
>> index 3a4b15a98e02..834b426da8b1 100644
>> --- a/drivers/media/v4l2-core/v4l2-common.c
>> +++ b/drivers/media/v4l2-core/v4l2-common.c
>> @@ -350,6 +350,34 @@ static inline unsigned int v4l2_format_block_height(const struct v4l2_format_inf
>>  	return info->block_h[plane];
>>  }
>>  
>> +static inline unsigned int v4l2_format_plane_width(const struct v4l2_format_info *info, int plane,
>> +						   unsigned int width)
>> +{
>> +	unsigned int hdiv = plane ? info->hdiv : 1;
>> +	unsigned int aligned_width =
>> +		ALIGN(width, v4l2_format_block_width(info, plane));
>> +
>> +	return DIV_ROUND_UP(aligned_width, hdiv) *
>> +	       info->bpp[plane] / info->bpp_div[plane];
>> +}
>> +
>> +static inline unsigned int v4l2_format_plane_height(const struct v4l2_format_info *info, int plane,
>> +						    unsigned int height)
>> +{
>> +	unsigned int vdiv = plane ? info->vdiv : 1;
>> +	unsigned int aligned_height =
>> +		ALIGN(height, v4l2_format_block_height(info, plane));
>> +
>> +	return DIV_ROUND_UP(aligned_height, vdiv);
>> +}
>> +
>> +static inline unsigned int v4l2_format_plane_size(const struct v4l2_format_info *info, int plane,
>> +						  unsigned int width, unsigned int height)
>> +{
>> +	return v4l2_format_plane_width(info, plane, width) *
>> +	       v4l2_format_plane_height(info, plane, height);
>> +}
>> +
>>  void v4l2_apply_frmsize_constraints(u32 *width, u32 *height,
>>  				    const struct v4l2_frmsize_stepwise *frmsize)
>>  {
>> @@ -385,37 +413,19 @@ int v4l2_fill_pixfmt_mp(struct v4l2_pix_format_mplane *pixfmt,
>>  
>>  	if (info->mem_planes == 1) {
>>  		plane = &pixfmt->plane_fmt[0];
>> -		plane->bytesperline = ALIGN(width, v4l2_format_block_width(info, 0)) * info->bpp[0] / info->bpp_div[0];
>> +		plane->bytesperline = v4l2_format_plane_width(info, 0, width);
>>  		plane->sizeimage = 0;
>>  
>> -		for (i = 0; i < info->comp_planes; i++) {
>> -			unsigned int hdiv = (i == 0) ? 1 : info->hdiv;
>> -			unsigned int vdiv = (i == 0) ? 1 : info->vdiv;
>> -			unsigned int aligned_width;
>> -			unsigned int aligned_height;
>> -
>> -			aligned_width = ALIGN(width, v4l2_format_block_width(info, i));
>> -			aligned_height = ALIGN(height, v4l2_format_block_height(info, i));
>> -
>> -			plane->sizeimage += info->bpp[i] *
>> -				DIV_ROUND_UP(aligned_width, hdiv) *
>> -				DIV_ROUND_UP(aligned_height, vdiv) / info->bpp_div[i];
>> -		}
>> +		for (i = 0; i < info->comp_planes; i++)
>> +			plane->sizeimage +=
>> +				v4l2_format_plane_size(info, i, width, height);
>>  	} else {
>>  		for (i = 0; i < info->comp_planes; i++) {
>> -			unsigned int hdiv = (i == 0) ? 1 : info->hdiv;
>> -			unsigned int vdiv = (i == 0) ? 1 : info->vdiv;
>> -			unsigned int aligned_width;
>> -			unsigned int aligned_height;
>> -
>> -			aligned_width = ALIGN(width, v4l2_format_block_width(info, i));
>> -			aligned_height = ALIGN(height, v4l2_format_block_height(info, i));
>> -
>>  			plane = &pixfmt->plane_fmt[i];
>>  			plane->bytesperline =
>> -				info->bpp[i] * DIV_ROUND_UP(aligned_width, hdiv) / info->bpp_div[i];
>> -			plane->sizeimage =
>> -				plane->bytesperline * DIV_ROUND_UP(aligned_height, vdiv);
>> +				v4l2_format_plane_width(info, i, width);
>> +			plane->sizeimage = plane->bytesperline *
>> +				v4l2_format_plane_height(info, i, height);
>>  		}
>>  	}
>>  	return 0;
>> @@ -439,22 +449,12 @@ int v4l2_fill_pixfmt(struct v4l2_pix_format *pixfmt, u32 pixelformat,
>>  	pixfmt->width = width;
>>  	pixfmt->height = height;
>>  	pixfmt->pixelformat = pixelformat;
>> -	pixfmt->bytesperline = ALIGN(width, v4l2_format_block_width(info, 0)) * info->bpp[0] / info->bpp_div[0];
>> +	pixfmt->bytesperline = v4l2_format_plane_width(info, 0, width);
>>  	pixfmt->sizeimage = 0;
>>  
>> -	for (i = 0; i < info->comp_planes; i++) {
>> -		unsigned int hdiv = (i == 0) ? 1 : info->hdiv;
>> -		unsigned int vdiv = (i == 0) ? 1 : info->vdiv;
>> -		unsigned int aligned_width;
>> -		unsigned int aligned_height;
>> -
>> -		aligned_width = ALIGN(width, v4l2_format_block_width(info, i));
>> -		aligned_height = ALIGN(height, v4l2_format_block_height(info, i));
>> -
>> -		pixfmt->sizeimage += info->bpp[i] *
>> -			DIV_ROUND_UP(aligned_width, hdiv) *
>> -			DIV_ROUND_UP(aligned_height, vdiv) / info->bpp_div[i];
>> -	}
>> +	for (i = 0; i < info->comp_planes; i++)
>> +		pixfmt->sizeimage +=
>> +			v4l2_format_plane_size(info, i, width, height);
>>  	return 0;
>>  }
>>  EXPORT_SYMBOL_GPL(v4l2_fill_pixfmt);
>