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

[Albert Esteve]

On Tue, Jul 25, 2023 at 4:26 PM Javier Martinez Canillas <javierm at redhat.com>
wrote:

> Albert Esteve <aesteve at redhat.com> writes:
>
> The patch looks good to me as well.
>
> Acked-by: Javier Martinez Canillas <javierm at 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 at 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 :)
>

+1 :)


>
> > @@ -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 ?
>
>
Oh, the typo, yes. Nice catch!


> [...]
>
> > +
> > +     width = height = 64;
>
> maybe you can rename these to cursor_{width,height} ?
>
>
Ok.


> 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 ?
>

In many other tests in the same file the size of the cursor is hardcoded to
64, same as here.
So I kept it consistent.
Could be a nice refactor, but I guess it is outside the scope of this patch.


>
> --
> Best regards,
>
> Javier Martinez Canillas
> Core Platforms
> Red Hat
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/igt-dev/attachments/20230726/e6f2d13e/attachment-0001.htm>