Re: [igt-dev] [PATCH v2 1/2] igt_kms: add hotspot plane property

["Modem, Bhanuprakash" <bhanuprakash.modem@intel.com>]

On Mon-31-07-2023 01:42 pm, Albert Esteve wrote:
> 
> 
> 
> On Mon, Jul 31, 2023 at 8:30 AM Modem, Bhanuprakash 
> <bhanuprakash.modem@intel.com <mailto:bhanuprakash.modem@intel.com>> wrote:
> 
>     Hello,
> 
>     On Fri-28-07-2023 08:07 pm, Albert Esteve wrote:
>      > Introduce LOCAL_DRM_CLIENT_CAP_CURSOR_PLANE_HOTSPOT
>      > capability definition until patched drm.h uapi header
>      > is upstreamed and lands on IGT.
>      >
>      > CURSOR_PLANE_HOTSPOT capability is enabled
>      > conditionally, and then tracked in the igt_display
>      > struct.
>      >
>      > Add HOTSPOT_X and HOTSPOT_Y properties
>      > to the atomic_plane_properties struct.
>      >
>      > Signed-off-by: Albert Esteve <aesteve@redhat.com
>     <mailto:aesteve@redhat.com>>
>      > ---
>      >   lib/igt_kms.c | 10 ++++++++++
>      >   lib/igt_kms.h | 11 ++++++++++-
>      >   2 files changed, 20 insertions(+), 1 deletion(-)
>      >
>      > diff --git a/lib/igt_kms.c b/lib/igt_kms.c
>      > index f2b0eed57..73119d0e5 100644
>      > --- a/lib/igt_kms.c
>      > +++ b/lib/igt_kms.c
>      > @@ -601,6 +601,8 @@ const char * const
>     igt_plane_prop_names[IGT_NUM_PLANE_PROPS] = {
>      >       [IGT_PLANE_CRTC_Y] = "CRTC_Y",
>      >       [IGT_PLANE_CRTC_W] = "CRTC_W",
>      >       [IGT_PLANE_CRTC_H] = "CRTC_H",
>      > +     [IGT_PLANE_HOTSPOT_X] = "HOTSPOT_X",
>      > +     [IGT_PLANE_HOTSPOT_Y] = "HOTSPOT_Y",
>      >       [IGT_PLANE_FB_ID] = "FB_ID",
>      >       [IGT_PLANE_CRTC_ID] = "CRTC_ID",
>      >       [IGT_PLANE_IN_FENCE_FD] = "IN_FENCE_FD",
>      > @@ -2296,6 +2298,11 @@ static void igt_plane_reset(igt_plane_t
>     *plane)
>      >       if (igt_plane_has_prop(plane, IGT_PLANE_SCALING_FILTER))
>      >               igt_plane_set_prop_enum(plane,
>     IGT_PLANE_SCALING_FILTER, "Default");
>      >
>      > +     if (igt_plane_has_prop(plane, IGT_PLANE_HOTSPOT_X))
>      > +             igt_plane_set_prop_value(plane,
>     IGT_PLANE_HOTSPOT_X, 0);
>      > +     if (igt_plane_has_prop(plane, IGT_PLANE_HOTSPOT_Y))
>      > +             igt_plane_set_prop_value(plane,
>     IGT_PLANE_HOTSPOT_Y, 0);
>      > +
>      >       igt_plane_clear_prop_changed(plane, IGT_PLANE_IN_FENCE_FD);
>      >       plane->values[IGT_PLANE_IN_FENCE_FD] = ~0ULL;
>      >       plane->gem_handle = 0;
>      > @@ -2679,6 +2686,9 @@ void igt_display_require(igt_display_t
>     *display, int drm_fd)
>      >       drmSetClientCap(drm_fd, DRM_CLIENT_CAP_UNIVERSAL_PLANES, 1);
>      >       if (drmSetClientCap(drm_fd, DRM_CLIENT_CAP_ATOMIC, 1) == 0)
>      >               display->is_atomic = 1;
>      > +
>      > +     if (drmSetClientCap(drm_fd,
>     LOCAL_DRM_CLIENT_CAP_CURSOR_PLANE_HOTSPOT, 1) == 0)
>      > +             display->set_hotspots = 1;
> 
>     Any special reason to add this to display struct?
>     IMHO, it is better to handle it at test level.
> 
> 
> Hi Bhanu,
> 
> Mostly for keeping it consistent with the `is_atomic` capability.
> 
> Are you suggesting moving the `drmSetClientCap` to the test?

Yes.

> I am ok with changing this, just trying to understand what would
> be the preferred way here.

Either way (adding to display struct or checking at test level) is fine.

Generally to maintain the re-usability (ex: checking for the same 
capability multiple times) we used to cache the capabilities to display 
struct. If multiple tests (or multiple places in your test) want to use 
this hotspot capability, you can add it to the display struct.

- Bhanu

> 
> Thanks!
> 
> 
>     - Bhanu
> 
>      >
>      >       plane_resources = drmModeGetPlaneResources(display->drm_fd);
>      >       igt_assert(plane_resources);
>      > diff --git a/lib/igt_kms.h b/lib/igt_kms.h
>      > index 1b6988c17..cf0a6d82b 100644
>      > --- a/lib/igt_kms.h
>      > +++ b/lib/igt_kms.h
>      > @@ -41,6 +41,12 @@
>      >
>      >   /* Low-level helpers with kmstest_ prefix */
>      >
>      > +/**
>      > + * Clients which do set cursor hotspot and treat the cursor plane
>      > + * like a mouse cursor should set this property.
>      > + */
>      > +#define LOCAL_DRM_CLIENT_CAP_CURSOR_PLANE_HOTSPOT    6
>      > +
>      >   /**
>      >    * pipe:
>      >    * @PIPE_NONE: Invalid pipe, used for disconnecting a output
>     from a pipe.
>      > @@ -318,7 +324,9 @@ enum igt_atomic_plane_properties {
>      >          IGT_PLANE_ZPOS,
>      >          IGT_PLANE_FB_DAMAGE_CLIPS,
>      >          IGT_PLANE_SCALING_FILTER,
>      > -       IGT_NUM_PLANE_PROPS
>      > +       IGT_PLANE_HOTSPOT_X,
>      > +       IGT_PLANE_HOTSPOT_Y,
>      > +       IGT_NUM_PLANE_PROPS
>      >   };
>      >
>      >   /**
>      > @@ -448,6 +456,7 @@ struct igt_display {
>      >       igt_pipe_t *pipes;
>      >       bool has_cursor_plane;
>      >       bool is_atomic;
>      > +     bool set_hotspots;
>      >       bool first_commit;
>      >
>      >       uint64_t *modifiers;
>