[PATCH i-g-t v2] tests/kms_properties: rework immutability checks
Ville Syrjälä
ville.syrjala at linux.intel.com
Fri Oct 25 14:36:24 UTC 2024
On Fri, Oct 25, 2024 at 01:26:47PM +0300, Dmitry Baryshkov wrote:
> On Fri, 25 Oct 2024 at 13:13, Ville Syrjälä
> <ville.syrjala at linux.intel.com> wrote:
> >
> > On Fri, Oct 25, 2024 at 12:53:29PM +0300, Dmitry Baryshkov wrote:
> > > On Fri, 25 Oct 2024 at 08:51, Ville Syrjälä
> > > <ville.syrjala at linux.intel.com> wrote:
> > > >
> > > > On Thu, Oct 24, 2024 at 09:29:03PM +0300, Dmitry Baryshkov wrote:
> > > > > On Fri, 18 Oct 2024 at 15:01, Dmitry Baryshkov
> > > > > <dmitry.baryshkov at linaro.org> wrote:
> > > > > >
> > > > > > Following the discussion on IRC, it is actually an error to require that
> > > > > > properties that can not be chaged are marked as immutable.
> > > > > >
> > > > > > First of all, it creates inconsistent uAPI. Some drivers might have an
> > > > > > immutable property, while others will have it mutable. Yes, there are
> > > > > > known examples for such behaviour (e.g. zpos), but they are clearly
> > > > > > documented in this way.
> > > > > >
> > > > > > Second, by the nature of the flag, the DRM_MODE_PROP_IMMUTABLE defines
> > > > > > more of the 'direction' of the property (whether it is set by the kernel
> > > > > > or it is expected to be set by the userspace) rather than simply states
> > > > > > that there is no way for the userspace to change the property.
> > > > > >
> > > > > > Rework the immutability checks to verify that the properties defined as
> > > > > > immutable have this flag set. Keep the "immutable if single value"
> > > > > > property just for the "zpos" property.
> > > > > >
> > > > > > Fixes: 29ae12bd764e ("tests/kms_properties: Validate properties harder")
> > > > > > Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > > > > Link: https://oftc.irclog.whitequark.org/dri-devel/2024-07-16#33374622
> > > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov at linaro.org>
> > > > >
> > > > > Gracious ping for the patch. We need it to be able to proceed with the
> > > > > HDMI rework for the drm/msm driver, otherwise IGT tests fail.
> > > > >
> > > > > >
> > > > > > ---
> > > > > > Changes since v1:
> > > > > > - Moved GAMMA_LUT_SIZE and DEGAMMA_LUT_SIZE to DRM_MODE_OBJECT_CRTC.
> > > > > > - Added debug print to help debugging possible issues.
> > > > > > ---
> > > > > > tests/kms_properties.c | 110 ++++++++++++++++++++++++++++++++---------
> > > > > > 1 file changed, 87 insertions(+), 23 deletions(-)
> > > > > >
> > > > > > diff --git a/tests/kms_properties.c b/tests/kms_properties.c
> > > > > > index a93c93cccf64..57f07e69909a 100644
> > > > > > --- a/tests/kms_properties.c
> > > > > > +++ b/tests/kms_properties.c
> > > > > > @@ -416,16 +416,80 @@ static void test_object_invalid_properties(igt_display_t *display,
> > > > > > test_invalid_properties(display->drm_fd, id, type, output->id, DRM_MODE_OBJECT_CONNECTOR, atomic);
> > > > > > }
> > > > > >
> > > > > > +enum prop_imm_flags {
> > > > > > + IMMUTABLE_REQ,
> > > > > > + IMMUTABLE_IF_SINGLE_VALUE,
> > > > > > +};
> > > > > > +
> > > > > > +static const struct {
> > > > > > + uint32_t obj_type;
> > > > > > + const char *name;
> > > > > > + enum prop_imm_flags flags;
> > > > > > +} prop_settings[] = {
> > > > > > + /* generic */
> > > > > > + { DRM_MODE_OBJECT_CONNECTOR, "EDID", IMMUTABLE_REQ },
> > > > > > + { DRM_MODE_OBJECT_CONNECTOR, "PATH", IMMUTABLE_REQ },
> > > > > > + { DRM_MODE_OBJECT_CONNECTOR, "TILE", IMMUTABLE_REQ },
> > > > > > + { DRM_MODE_OBJECT_CONNECTOR, "WRITEBACK_PIXEL_FORMATS", IMMUTABLE_REQ },
> > > > > > + { DRM_MODE_OBJECT_CONNECTOR, "non-desktop", IMMUTABLE_REQ },
> > > > > > + { DRM_MODE_OBJECT_CONNECTOR, "panel orientation" ,IMMUTABLE_REQ },
> > > > > > + { DRM_MODE_OBJECT_CONNECTOR, "privacy-screen hw-state", IMMUTABLE_REQ },
> > > > > > + { DRM_MODE_OBJECT_CONNECTOR, "subconnector", IMMUTABLE_REQ },
> > > > > > + { DRM_MODE_OBJECT_CONNECTOR, "suggested X", IMMUTABLE_REQ },
> > > > > > + { DRM_MODE_OBJECT_CONNECTOR, "suggested Y", IMMUTABLE_REQ },
> > > > > > + { DRM_MODE_OBJECT_CONNECTOR, "vrr_capable", IMMUTABLE_REQ },
> > > > > > +
> > > > > > + { DRM_MODE_OBJECT_CRTC, "DEGAMMA_LUT_SIZE", IMMUTABLE_REQ },
> > > > > > + { DRM_MODE_OBJECT_CRTC, "GAMMA_LUT_SIZE", IMMUTABLE_REQ },
> > > > > > +
> > > > > > + { DRM_MODE_OBJECT_PLANE, "IN_FORMATS", IMMUTABLE_REQ },
> > > > > > + { DRM_MODE_OBJECT_PLANE, "SIZE_HINTS", IMMUTABLE_REQ },
> > > > > > + { DRM_MODE_OBJECT_PLANE, "type", IMMUTABLE_REQ },
> > > > > > + { DRM_MODE_OBJECT_PLANE, "zpos", IMMUTABLE_IF_SINGLE_VALUE },
> > > > > > +
> > > > > > + /* driver-specific */
> > > > > > + { DRM_MODE_OBJECT_CONNECTOR, "hotplug_mode_update", IMMUTABLE_REQ }, // qxl, vmwgfx
> > > > > > + { DRM_MODE_OBJECT_CONNECTOR, "implicit_placement", IMMUTABLE_REQ }, // vmwgfx
> > > > > > + { DRM_MODE_OBJECT_PLANE, "AMD_PLANE_BLEND_LUT_SIZE", IMMUTABLE_REQ }, // amdgpu
> > > > > > + { DRM_MODE_OBJECT_PLANE, "AMD_PLANE_DEGAMMA_LUT_SIZE", IMMUTABLE_REQ }, // amdgpu
> > > > > > + { DRM_MODE_OBJECT_PLANE, "AMD_PLANE_LUT3D_SIZE", IMMUTABLE_REQ }, // amdgpu
> > > > > > + { DRM_MODE_OBJECT_PLANE, "AMD_PLANE_SHAPER_LUT_SIZE", IMMUTABLE_REQ }, // amdgpu
> > > > > > +};
> > > >
> > > > Not really a fan of having a list like this. All of these look
> > > > like they are just regular immutable properties, with zpos being
> > > > the only exception. Or is that not the case?
> > >
> > > Yes. The goal is to perform the ABI check: which properties are
> > > documented to be immutable. All other properties are expected to be
> > > mutable.
> > >
> > > >
> > > > What was the problem of just dropping the check that non-immutable
> > > > properties must have multiple possible values?
> > >
> > > See the discussion at [1] and then the response by Sima [2] to the
> > > patch similar to your proposal.
> > >
> > > [1] https://oftc.irclog.whitequark.org/dri-devel/2024-07-16#33374622
> > > [2] https://lore.kernel.org/igt-dev/Zpjn2dTHDrBBuTVH@phenom.ffwll.local/
> >
> > I'm not seeing any real conclusions there. Looks to me
> > like the correct thing would be to just remove that
> > single value => immutable assumption.
>
> Sima rejected such a patch, [2].
Looks rather to me that a bunch of people agreed we don't
need the immutable on zpos. And if we don't have other wonky
properties like this then the problem is would appear to be
solved.
--
Ville Syrjälä
Intel
More information about the igt-dev
mailing list