<div dir="ltr"><div dir="ltr"><br clear="all"><div><div dir="ltr" class="gmail_signature"><div dir="ltr"><p style="color:rgb(0,0,0);font-family:RedHatText,sans-serif;font-weight:bold;margin:0px;padding:0px;font-size:14px"><br></p></div></div></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Jul 25, 2023 at 4:26 PM Javier Martinez Canillas <<a href="mailto:javierm@redhat.com">javierm@redhat.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Albert Esteve <<a href="mailto:aesteve@redhat.com" target="_blank">aesteve@redhat.com</a>> writes:<br>
<br>
The patch looks good to me as well.<br>
<br>
Acked-by: Javier Martinez Canillas <<a href="mailto:javierm@redhat.com" target="_blank">javierm@redhat.com</a>><br>
<br>
Some comments below though:<br>
<br>
> Add a test for modesetting an atomic cursor plane<br>
> hotspot property. The test first checks if the<br>
> plane is atomic and has the hotspot property.<br>
> and if it does, it sets different random hot_x<br>
> and hot_y values and checks that it is updated<br>
> correctly after an atomic commit.<br>
><br>
> Signed-off-by: Albert Esteve <<a href="mailto:aesteve@redhat.com" target="_blank">aesteve@redhat.com</a>><br>
> ---<br>
>  tests/kms_cursor_legacy.c | 75 +++++++++++++++++++++++++++++++++++++++<br>
>  1 file changed, 75 insertions(+)<br>
><br>
> diff --git a/tests/kms_cursor_legacy.c b/tests/kms_cursor_legacy.c<br>
> index 1ebac9d31..38258ed8d 100644<br>
> --- a/tests/kms_cursor_legacy.c<br>
> +++ b/tests/kms_cursor_legacy.c<br>
<br>
This file is called kms_cursor_legacy.c but it seems there are tests for<br>
both legacy KMS and atomic KMS? That confused me :)<br></blockquote><div><br></div><div>+1 :)<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> @@ -1607,6 +1607,70 @@ static void flip_vs_cursor_busy_crc(igt_display_t *display, bool atomic)<br>
>       put_ahnd(ahnd);<br>
>  }<br>
>  <br>
> +static void modeset_atomic_cursot_hotspot(igt_display_t *display)<br>
> +{<br>
<br>
I think you meant this to be "cursor_hotspot" instead ?<br>
<br></blockquote><div><br></div><div>Oh, the typo, yes. Nice catch!</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
[...]<br>
<br>
> +<br>
> +     width = height = 64;<br>
<br>
maybe you can rename these to cursor_{width,height} ?<br>
<br></blockquote><div><br></div><div>Ok.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Also, there's <a href="https://docs.kernel.org/gpu/drm-uapi.html#c.DRM_CAP_CURSOR_WIDTH" rel="noreferrer" target="_blank">https://docs.kernel.org/gpu/drm-uapi.html#c.DRM_CAP_CURSOR_WIDTH</a><br>
and DRM_CAP_CURSOR_HEIGHT to query the hardware cursor size. Maybe you should<br>
use that instead of assume a 64x64 cursor size ?<br></blockquote><div><br></div><div>In many other tests in the same file the size of the cursor is hardcoded to 64, same as here.</div><div>So I kept it consistent.</div><div>Could be a nice refactor, but I guess it is outside the scope of this patch.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
-- <br>
Best regards,<br>
<br>
Javier Martinez Canillas<br>
Core Platforms<br>
Red Hat<br>
<br>
</blockquote></div></div>