[PATCH i-g-t v2] tests/kms_properties: rework immutability checks
Dmitry Baryshkov
dmitry.baryshkov at linaro.org
Fri Oct 25 10:26:47 UTC 2024
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].
>
> 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.
I think the best thing I can do is to split this patch into two, one
dropping the immutable check and another one adding immutable checks
via properties enumeration. Then we can work on adding more tests to
the "standard properties" feature tests. WDYT?
>
> > >
> > > 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.
The subconnector comes to my mind: the kernel specifies to userspace
(so, immutable) the subtype of the connected dongle (so, one value
from many supported by the enum).
--
With best wishes
Dmitry
More information about the igt-dev
mailing list