[PATCH i-g-t v2] tests/kms_properties: rework immutability checks
Ville Syrjälä
ville.syrjala at linux.intel.com
Fri Oct 25 10:13:39 UTC 2024
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.
And I'd follow that up with a kernel patch to make zpos
non-immutable. I'm thinking this shouldn't break anything
since the property can already be non-immutable.
And checking that all standard properties look sane
should probably be a new subtest, and it should
validate more than the immutable bit.
> >
> > And perhaps there should be instead a check that immutable
> > properties must not have more than one possible value?
>
> This is not a correct assumption, immutable props might have any
> number of values. The difference is on semantics side: the driver
> controls immutable properties, userspace controls mutable ones.
Hmm, yeah I suppose one could use it eg. to relay back some
sink capabilities as an enum property.
--
Ville Syrjälä
Intel
More information about the igt-dev
mailing list