Hi Umang, Stefan,
Thank you for the patch.
On Fri, Oct 25, 2024 at 06:33:00PM +0530, Umang Jain wrote:
> From: Stefan Klug <stefan.klug@xxxxxxxxxxxxxxxx>
>
> The current scaler code fails in many cases which can be validated
> by the following python script:
>
> ```
> error_count = 0
> valid_count = 0
>
> def check_scaling_error(src_w, dst_w, add_point_five):
> global error_count
> global valid_count
>
> qscale = (dst_w << 16) // src_w
>
> if (add_point_five):
> delta = 1 << 15; # 0.5 in Q16.16
> else:
> delta = 0
>
> scaled_w = ((((src_w << 16) + delta) * qscale) >> 32)
> if dst_w != scaled_w:
> print(f'scale_error: src_w: {src_w} | dst_w:{dst_w} | scaled_w:{scaled_w}')
> error_count += 1
> else:
> valid_count += 1
>
> print(f'==== Test without delta=0.5 ====\n')
> for i in range(1000, 1920):
> check_scaling_error(1920, i, False)
> print(f'Error: {error_count} | Valid: {valid_count}\n\n')
>
> error_count = 0
You need to reset valid_count too here.
> print(f'==== Test with delta=0.5 ====')
> for i in range(1000, 1920):
> check_scaling_error(1920, i, True)
> print(f'Error: {error_count} | Valid: {valid_count}\n\n')
> ```
>
> Excerpt of the output is retrieved:
> ```
> ==== Test without delta=0.5 ====
> ...
> ...
> scale_error: src_w: 1920 | dst_w:1915 | scaled_w:1914
> scale_error: src_w: 1920 | dst_w:1916 | scaled_w:1915
> scale_error: src_w: 1920 | dst_w:1917 | scaled_w:1916
> scale_error: src_w: 1920 | dst_w:1918 | scaled_w:1917
> scale_error: src_w: 1920 | dst_w:1919 | scaled_w:1918
> Error: 859 | Valid: 61
>
> ==== Test with delta=0.5 ====
> Error: 0 | Valid: 981
> ```
> Hence, fixing the scaling rounding error by adding 0.5 to the
> frame dimensions before applying the scale.
>
> Signed-off-by: Stefan Klug <stefan.klug@xxxxxxxxxxxxxxxx>
> Signed-off-by: Umang Jain <umang.jain@xxxxxxxxxxxxxxxx>
> ---
> drivers/media/platform/nxp/dw100/dw100.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/platform/nxp/dw100/dw100.c b/drivers/media/platform/nxp/dw100/dw100.c
> index 42712ccff754..541706f0aec4 100644
> --- a/drivers/media/platform/nxp/dw100/dw100.c
> +++ b/drivers/media/platform/nxp/dw100/dw100.c
> @@ -984,6 +984,7 @@ static int dw100_s_selection(struct file *file, void *fh,
> u32 qscalex, qscaley, qscale;
> int x, y, w, h;
> unsigned int wframe, hframe;
> + uint32_t zero_point_five;
>
> if (sel->type != V4L2_BUF_TYPE_VIDEO_OUTPUT)
> return -EINVAL;
> @@ -1032,8 +1033,9 @@ static int dw100_s_selection(struct file *file, void *fh,
> }
> }
>
> - w = (u32)((((u64)wframe << 16) * qscale) >> 32);
> - h = (u32)((((u64)hframe << 16) * qscale) >> 32);
> + zero_point_five = 1 << 15;
> + w = (u32)(((((u64)wframe << 16)+zero_point_five) * qscale) >> 32);
I'm having trouble understanding what you're really fixing here. It
seems the patch ensures that the crop rectangle requested by the
application doesn't get adjusted, but doesn't take into account what is
actually programmed to the hardware. Looking at dw100_hw_set_src_crop(),
I see that the hardware scaling factor is expressed as a UQ1.7 integer.
Shouldn't the rounding applied by the hardware need to be reported in
the crop rectangle returned by dw100_s_selection(), instead of
pretending it can be pixel-perfect ? At the very least I would expect
this function to use Q1.7 encoding for the scaling factor, and apply the
same rounding as the hardware to calculate the width and height back.
> + h = (u32)(((((u64)hframe << 16)+zero_point_five) * qscale) >> 32);
Missing spaces around '+'/
> x = x + (sel->r.width - w) / 2;
> y = y + (sel->r.height - h) / 2;
> x = min(wframe - w, (unsigned int)max(0, x));
--
Regards,
Laurent Pinchart
