Received-SPF: pass (google.com: domain of linux-kernel+bounces-179447-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <d8df8023-daac-380a-0591-59c920d6eacd@ti.com>
Date: Wed, 15 May 2024 10:36:44 +0530
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.11.0
Subject: Re: [PATCH v7 7/8] media: imagination: Round to closest multiple for
 cropping region
To: Nicolas Dufresne <nicolas@ndufresne.ca>,
        Andy Shevchenko
	<andriy.shevchenko@linux.intel.com>
CC: <mchehab@kernel.org>, <hverkuil-cisco@xs4all.nl>,
        <linux-media@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <benjamin.gaignard@collabora.com>, <sebastian.fricke@collabora.com>,
        <dri-devel@lists.freedesktop.org>, <laurent.pinchart@ideasonboard.com>,
        <praneeth@ti.com>, <nm@ti.com>, <vigneshr@ti.com>, <a-bhatia1@ti.com>,
        <j-luthra@ti.com>, <b-brnich@ti.com>, <detheridge@ti.com>,
        <p-mantena@ti.com>, <vijayp@ti.com>, <andrzej.p@collabora.com>,
        <p.zabel@pengutronix.de>, <airlied@gmail.com>, <daniel@ffwll.ch>,
        <akpm@linux-foundation.org>, <gregkh@linuxfoundation.org>,
        <adobriyan@gmail.com>, <jani.nikula@intel.com>
References: <20240509184001.4064820-1-devarsht@ti.com>
 <Zj447ePSnccbj76v@smile.fi.intel.com>
 <7e3ad816-6a2a-4e02-9b41-03a8562812ad@ti.com>
 <432f336e6d4b0666cfbb431591624d0e1fefd773.camel@ndufresne.ca>
Content-Language: en-US
From: Devarsh Thakkar <devarsht@ti.com>
In-Reply-To: <432f336e6d4b0666cfbb431591624d0e1fefd773.camel@ndufresne.ca>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8bit

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