Re: [PATCH] drm/tests: Add test case for drm_rect_clip_scaled()

[Arthur Grillo Queiroz Cabral <arthurgrillo@riseup.net>]

On 14/06/23 14:54, nelsonbogado99 wrote:
> From: Nelson Bogado <nelosonbrizue99@gmail.com>
> 
> To evaluate the behavior of the drm_rect_clip_scaled() helper function
> with misplaced rectangles and improve the robustness and quality of it.
> 
> The new test was executed using the following command:
> 
>   $ ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/gpu/drm/tests/
>   [01:48:12] ================== drm_rect (10 subtests) ==================
>   ...
>   [01:48:12] [PASSED] drm_test_rect_clip_scaled_out_of_bounds
> 
> Signed-off-by: Nelson Bogado <nelosonbrizue99@gmail.com>
> ---
>  drivers/gpu/drm/tests/drm_rect_test.c | 53 +++++++++++++++++++++++++++
>  1 file changed, 53 insertions(+)
Hello,

here are a couple of suggestions to make the code more readable ;).

> 
> diff --git a/drivers/gpu/drm/tests/drm_rect_test.c b/drivers/gpu/drm/tests/drm_rect_test.c
> index 76332cd2ead8..b3bfdd420123 100644
> --- a/drivers/gpu/drm/tests/drm_rect_test.c
> +++ b/drivers/gpu/drm/tests/drm_rect_test.c
> @@ -209,6 +209,58 @@ static void drm_test_rect_clip_scaled_signed_vs_unsigned(struct kunit *test)
>  	KUNIT_EXPECT_FALSE_MSG(test, drm_rect_visible(&src), "Source should not be visible\n");
>  }
>  
> +static void drm_test_rect_clip_scaled_out_of_bounds(struct kunit *test)
> +{
> +	/* Definition of the rectangles and visible variables */
I think it would be great to decrease the amount of comments, you don't need
to comment that you're declaring some variable. Maybe just keep the comments
that explain what you're testing

> +	struct drm_rect src, dst, clip;
> +	bool visible;
> +
> +	/*
> +	 * Both rectangles are completely out of bounds, initialize the src,
> +	 * dst and clip rectangles with specific coordinates and sizes.
> +	 */
like this one.

> +	drm_rect_init(&src, -10, -10, -5, -5);
> +	drm_rect_init(&dst, -20, -20, -15, -15);
> +	drm_rect_init(&clip, 0, 0, 100, 100);
> +
> +	/* Function call drm_rect_clip_scaled to determine visibility */
> +	visible = drm_rect_clip_scaled(&src, &dst, &clip);
> +
> +	/* Check expected results */
> +	KUNIT_EXPECT_FALSE_MSG(test, visible, "Destination should not be visible\n");
> +	KUNIT_EXPECT_FALSE_MSG(test, drm_rect_visible(&src), "Source should not be visible\n");
> +
> +	/*
> +	 * Only source rectangle is out of bounds, reinitialize the src,
> +	 * dst and clip rectangles with new values.
> +	 */
> +	drm_rect_init(&src, -10, -10, -5, -5);
Use `DRM_RECT_INIT` instead, this way you don't need to pass the variable as an
argument. I think it would be more readable.

~Arthur Grillo

> +	drm_rect_init(&dst, 0, 0, 10, 10);
> +	drm_rect_init(&clip, 0, 0, 100, 100);
> +
> +	/* Function call drm_rect_clip_scaled to determine visibility */
> +	visible = drm_rect_clip_scaled(&src, &dst, &clip);
> +
> +	/* Check expected results */
> +	KUNIT_EXPECT_TRUE_MSG(test, visible, "Destination should be visible\n\n");
> +	KUNIT_EXPECT_FALSE_MSG(test, drm_rect_visible(&src), "Source should not be visible\n");
> +
> +	/*
> +	 * Only source rectangle is out of bounds, reinitialize the src,
> +	 * dst and clip rectangles with new values.
> +	 */
> +	drm_rect_init(&src, 0, 0, 10, 10);
> +	drm_rect_init(&dst, -10, -10, -5, -5);
> +	drm_rect_init(&clip, 0, 0, 100, 100);
> +
> +	/* Function call drm_rect_clip_scaled to determine visibility */
> +	visible = drm_rect_clip_scaled(&src, &dst, &clip);
> +
> +	/* Check expected results */
> +	KUNIT_EXPECT_FALSE_MSG(test, visible, "Destination should not be visible\n");
> +	KUNIT_EXPECT_FALSE_MSG(test, drm_rect_visible(&src), "Source should not be visible\n");
> +}
> +
>  struct drm_rect_intersect_case {
>  	const char *description;
>  	struct drm_rect r1, r2;
> @@ -511,6 +563,7 @@ static struct kunit_case drm_rect_tests[] = {
>  	KUNIT_CASE(drm_test_rect_clip_scaled_not_clipped),
>  	KUNIT_CASE(drm_test_rect_clip_scaled_clipped),
>  	KUNIT_CASE(drm_test_rect_clip_scaled_signed_vs_unsigned),
> +	KUNIT_CASE(drm_test_rect_clip_scaled_out_of_bounds),
>  	KUNIT_CASE_PARAM(drm_test_rect_intersect, drm_rect_intersect_gen_params),
>  	KUNIT_CASE_PARAM(drm_test_rect_calc_hscale, drm_rect_scale_gen_params),
>  	KUNIT_CASE_PARAM(drm_test_rect_calc_vscale, drm_rect_scale_gen_params),