[PATCH v7 7/8] media: imagination: Round to closest multiple for cropping region

[Devarsh Thakkar]

Hi Nicolas,

Thanks for the review.

On 15/05/24 01:52, Nicolas Dufresne wrote:
> Le samedi 11 mai 2024 à 22:38 +0530, Devarsh Thakkar a écrit :
>> Hi Andy,
>>
>> Thanks for the quick review.
>> On 10/05/24 20:40, Andy Shevchenko wrote:
>>> On Fri, May 10, 2024 at 12:10:01AM +0530, Devarsh Thakkar wrote:
>>>> If neither of the flags to round down (V4L2_SEL_FLAG_LE) or round up
>>>> (V4L2_SEL_FLAG_GE) are specified by the user, then round to nearest
>>>> multiple of requested value while updating the crop rectangle coordinates.
>>>>
>>>> Use the rounding macro which gives preference to rounding down in case two
>>>> nearest values (high and low) are possible to raise the probability of
>>>> cropping rectangle falling inside the bound region.
>>>
>>> This is arguable. How do we know that the bigger range is supported?
>>> The safest side is to go smaller than bigger.
>>>
>>
>> Yes and that's what the driver does when do when application passes
>>  while doing the selection. If application does not
>> specify explicitly whether to round down or round up the cropping
>> parameters requested by it (i.e app is neither passing V4L2_SEL_FLAG_LE
>> nor V4L2_SEL_FLAG_GE flags), then it is preferred by driver to round the
>> cropping parameters to nearest possible value by either rounding down or
>> rounding up to align with hardware requirements.
>>
>> For e.g. If requested width for cropping region is 127 and HW requires
>> width to be multiple of 64 then we would prefer to round it up to 128
>> rather than rounding down to a more distant value (i.e. 64), but if
>> requested cropping width is 129 then we would prefer to instead round it
>> down to 128. But if requested cropping width is 160 then there are two
>> nearest possible values 160 - 32 = 128 and 160 + 32 = 192 and in which
>> case we prefer the smaller value as you suggested and that's why the
>> driver uses round_closest_down.
>>
>> For any reason, if still the cropping rectangle falls beyond the bound
>> region, then driver will return out of range error (-ERANGE) to
>> application.
> 
> I would appreciate if this change was based on specification text, meaning
> improving the next if that behaviour is undefined. We might not be able to fix
> it everywhere, but we can recommend something.
> 

Yes, this change is based on specification text. This complies with the
VIDIOC_G_SELECTION, VIDIOC_S_SELECTION ioctl description as documented in
v4l uapi [1] which means driver should choose crop rectangle as close as
possible if no flags are passed by user-space, as quoted below :

"``0`` - The driver can adjust the rectangle size freely and shall
 choose a crop/compose rectangle as close as possible to the requested
 one."

If the user-space has specific requirement to either round down, round up or
honor exact values, it should pass V4L2_SEL_FLAG_LE, V4L2_SEL_FLAG_GE or
V4L2_SEL_FLAG_LE | V4L2_SEL_FLAG_GE flags respectively.

[1]
https://www.kernel.org/doc/Documentation/userspace-api/media/v4l/vidioc-g-selection.rst#:~:text=compose%20rectangle%20as-,close,-as%20possible%20to

Regards
Devarsh