Re: [PATCH v12 12/13] media: imagination: Round to closest multiple for cropping region

[Sebastian Fricke]

Hey,

On 04.06.2024 16:23, 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 complies with the VIDIOC_G_SELECTION, VIDIOC_S_SELECTION ioctl
> description as documented in v4l uapi [1] which specifies that 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."
>
> Link: https://www.kernel.org/doc/Documentation/userspace-api/media/v4l/vidioc-g-selection.rst [1]
> Signed-off-by: Devarsh Thakkar <devarsht@xxxxxx>

Acked-by: Sebastian Fricke <sebastian.fricke@xxxxxxxxxxxxx>

Can, whoever picks up the math changes, pick up this change as well?
I will send 1-6 via the media subsystem.

Regards,
Sebastian

> ---
> V12: No change
> V11: No change
> V10: No change
> V9:  No change
> V8:  Update commit message with specification reference
> V1->V7 (No change, patch introduced in V7)
> ---
> drivers/media/platform/imagination/e5010-jpeg-enc.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/media/platform/imagination/e5010-jpeg-enc.c b/drivers/media/platform/imagination/e5010-jpeg-enc.c
> index e701d573a26a..d65646f0c38c 100644
> --- a/drivers/media/platform/imagination/e5010-jpeg-enc.c
> +++ b/drivers/media/platform/imagination/e5010-jpeg-enc.c
> @@ -517,10 +517,10 @@ static int e5010_s_selection(struct file *file, void *fh, struct v4l2_selection
>
> 	switch (s->flags) {
> 	case 0:
> -		s->r.width = round_down(s->r.width, queue->fmt->frmsize.step_width);
> -		s->r.height = round_down(s->r.height, queue->fmt->frmsize.step_height);
> -		s->r.left = round_down(s->r.left, queue->fmt->frmsize.step_width);
> -		s->r.top = round_down(s->r.top, 2);
> +		s->r.width = round_closest_down(s->r.width, queue->fmt->frmsize.step_width);
> +		s->r.height = round_closest_down(s->r.height, queue->fmt->frmsize.step_height);
> +		s->r.left = round_closest_down(s->r.left, queue->fmt->frmsize.step_width);
> +		s->r.top = round_closest_down(s->r.top, 2);
>
> 		if (s->r.left + s->r.width > queue->width)
> 			s->r.width = round_down(s->r.width + s->r.left - queue->width,
> --
> 2.39.1