Re: [igt-dev] [PATCH 3/3] kms_cursor_legacy: modeset-atomic-cursor-hotspot

[Javier Martinez Canillas <javierm@redhat.com>]

Albert Esteve <aesteve@redhat.com> writes:

The patch looks good to me as well.

Acked-by: Javier Martinez Canillas <javierm@redhat.com>

Some comments below though:

> Add a test for modesetting an atomic cursor plane
> hotspot property. The test first checks if the
> plane is atomic and has the hotspot property.
> and if it does, it sets different random hot_x
> and hot_y values and checks that it is updated
> correctly after an atomic commit.
>
> Signed-off-by: Albert Esteve <aesteve@redhat.com>
> ---
>  tests/kms_cursor_legacy.c | 75 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 75 insertions(+)
>
> diff --git a/tests/kms_cursor_legacy.c b/tests/kms_cursor_legacy.c
> index 1ebac9d31..38258ed8d 100644
> --- a/tests/kms_cursor_legacy.c
> +++ b/tests/kms_cursor_legacy.c

This file is called kms_cursor_legacy.c but it seems there are tests for
both legacy KMS and atomic KMS? That confused me :)

> @@ -1607,6 +1607,70 @@ static void flip_vs_cursor_busy_crc(igt_display_t *display, bool atomic)
>  	put_ahnd(ahnd);
>  }
>  
> +static void modeset_atomic_cursot_hotspot(igt_display_t *display)
> +{

I think you meant this to be "cursor_hotspot" instead ?

[...]

> +
> +	width = height = 64;

maybe you can rename these to cursor_{width,height} ?

Also, there's https://docs.kernel.org/gpu/drm-uapi.html#c.DRM_CAP_CURSOR_WIDTH
and DRM_CAP_CURSOR_HEIGHT to query the hardware cursor size. Maybe you should
use that instead of assume a 64x64 cursor size ?

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat