| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <7c1172cb-a2a3-459a-802f-950a3302ba94@quicinc.com>
Date: Tue, 10 Dec 2024 14:23:31 -0800
From: Jessica Zhang <quic_jesszhan@...cinc.com>
To: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
CC: Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
Maxime Ripard
<mripard@...nel.org>,
Thomas Zimmermann <tzimmermann@...e.de>,
David Airlie
<airlied@...il.com>, Simona Vetter <simona@...ll.ch>,
<quic_abhinavk@...cinc.com>, <dri-devel@...ts.freedesktop.org>,
<linux-kernel@...r.kernel.org>, <robdclark@...il.com>
Subject: Re: [PATCH v2 2/2] drm/tests: Add test for
drm_atomic_helper_commit_modeset_disables()
On 12/10/2024 1:10 PM, Dmitry Baryshkov wrote:
> On Mon, Dec 09, 2024 at 05:09:25PM -0800, Jessica Zhang wrote:
>> Add a subtest to check that modeset is called when the connector is
>> changed
>>
>> Signed-off-by: Jessica Zhang <quic_jesszhan@...cinc.com>
>> ---
>> drivers/gpu/drm/tests/Makefile | 1 +
>> drivers/gpu/drm/tests/drm_atomic_state_test.c | 242 ++++++++++++++++++++++++++
>> 2 files changed, 243 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/tests/Makefile b/drivers/gpu/drm/tests/Makefile
>> index 56dab563abd7a7ee7c147bd6b4927e2436b82e1d..0109bcf7faa54993cce337f522eae78f0fa6ffcb 100644
>> --- a/drivers/gpu/drm/tests/Makefile
>> +++ b/drivers/gpu/drm/tests/Makefile
>> @@ -4,6 +4,7 @@ obj-$(CONFIG_DRM_KUNIT_TEST_HELPERS) += \
>> drm_kunit_helpers.o
>>
>> obj-$(CONFIG_DRM_KUNIT_TEST) += \
>> + drm_atomic_state_test.o \
>> drm_buddy_test.o \
>> drm_cmdline_parser_test.o \
>> drm_connector_test.o \
>> diff --git a/drivers/gpu/drm/tests/drm_atomic_state_test.c b/drivers/gpu/drm/tests/drm_atomic_state_test.c
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..ffc1198ccc9ce103b9bc4c5ca8c83ca7f6ab19cc
>> --- /dev/null
>> +++ b/drivers/gpu/drm/tests/drm_atomic_state_test.c
>> @@ -0,0 +1,242 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Test cases for the drm_atomic_state helpers
>> + *
>> + * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved.
>> + */
>> +
>> +#include <drm/drm_atomic.h>
>> +#include <drm/drm_atomic_helper.h>
>> +#include <drm/drm_atomic_uapi.h>
>> +#include <drm/drm_kunit_helpers.h>
>> +#include <drm/drm_probe_helper.h>
>> +
>> +#define DRM_TEST_ENC_0 BIT(0)
>> +#define DRM_TEST_ENC_1 BIT(1)
>> +#define DRM_TEST_ENC_2 BIT(2)
>> +
>> +#define DRM_TEST_CONN_0 BIT(0)
>> +
>> +static const struct drm_display_mode drm_atomic_test_mode = {
>> + DRM_MODE("1024x768", 0, 65000, 1024, 1048,
>> + 1184, 1344, 0, 768, 771, 777, 806, 0,
>> + DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC)
>> +};
>> +
>> +struct drm_atomic_check_priv {
>> + struct drm_device drm;
>> + struct drm_plane *plane;
>> + struct drm_crtc *crtc;
>> + struct drm_encoder encoders[3];
>> + struct drm_connector connectors[2];
>> +};
>> +
>> +static int modeset_counter;
>> +
>> +static void drm_test_encoder_mode_set(struct drm_encoder *encoder,
>> + struct drm_crtc_state *crtc_state,
>> + struct drm_connector_state *conn_state)
>> +{
>> + modeset_counter++;
>> +}
>> +
>> +static const struct drm_encoder_helper_funcs drm_test_encoder_funcs = {
>
> Name is too generic, please use something more test-suite specific.
Hey Dmitry,
Thanks for the review, ack for all of these comments.
- Jessica Zhang
>
>> + .atomic_mode_set = drm_test_encoder_mode_set,
>> +};
>> +
>> +static const struct drm_connector_funcs dummy_connector_funcs = {
>> + .atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
>> + .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
>> + .reset = drm_atomic_helper_connector_reset,
>> +};
>> +
>> +static int drm_atomic_test_dummy_get_modes(struct drm_connector *connector)
>> +{
>> + drm_connector_helper_get_modes_fixed(connector, &drm_atomic_test_mode);
>> + return 1;
>
> return drm_connector_helper_get_modes_fixed()
>
>> +}
>> +
>> +static const struct drm_connector_helper_funcs dummy_connector_helper_funcs = {
>> + .get_modes = drm_atomic_test_dummy_get_modes,
>> +};
>> +
>> +static struct drm_atomic_check_priv *
>> +drm_test_init_drm_components(struct kunit *test, bool has_connectors)
>
> I think this is too generic, please use a file-specific name.
>
>> +{
>> + struct drm_atomic_check_priv *priv;
>> + struct drm_encoder *enc;
>> + struct drm_device *drm;
>> + struct device *dev;
>> + int ret;
>> +
>> + dev = drm_kunit_helper_alloc_device(test);
>> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, dev);
>> +
>> + priv = drm_kunit_helper_alloc_drm_device(test, dev,
>> + struct drm_atomic_check_priv,
>> + drm,
>> + DRIVER_MODESET | DRIVER_ATOMIC);
>> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv);
>> + test->priv = priv;
>> +
>> + drm = &priv->drm;
>> + priv->plane = drm_kunit_helper_create_primary_plane(test, drm,
>> + NULL,
>> + NULL,
>> + NULL, 0,
>> + NULL);
>> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv->plane);
>> +
>> + priv->crtc = drm_kunit_helper_create_crtc(test, drm,
>> + priv->plane, NULL,
>> + NULL,
>> + NULL);
>> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv->crtc);
>> +
>> + for (int i = 0; i < ARRAY_SIZE(priv->encoders); i++) {
>> + enc = &priv->encoders[i];
>> +
>> + ret = drmm_encoder_init(drm, enc, NULL,
>> + DRM_MODE_ENCODER_DSI, NULL);
>> + KUNIT_ASSERT_EQ(test, ret, 0);
>> +
>> + enc->possible_crtcs = drm_crtc_mask(priv->crtc);
>> + }
>> +
>> + priv->encoders[0].possible_clones = DRM_TEST_ENC_0 | DRM_TEST_ENC_1;
>> + priv->encoders[1].possible_clones = DRM_TEST_ENC_0 | DRM_TEST_ENC_1;
>> + priv->encoders[2].possible_clones = DRM_TEST_ENC_2;
>> +
>> + if (!has_connectors)
>> + goto done;
>> +
>> + struct drm_connector *conn;
>
> Please move next ot other variable definitions.
>
>> +
>> + KUNIT_ASSERT_LE(test, ARRAY_SIZE(priv->connectors),
>> + ARRAY_SIZE(priv->encoders));
>
> I'd say it's BUILD_BUG_ON
>
>> +
>> +
>> + for (int i = 0; i < ARRAY_SIZE(priv->connectors); i++) {
>> + conn = &priv->connectors[i];
>> +
>> + ret = drmm_connector_init(drm, conn, &dummy_connector_funcs,
>> + DRM_MODE_CONNECTOR_DSI, NULL);
>> + KUNIT_ASSERT_EQ(test, ret, 0);
>> +
>> + drm_connector_helper_add(conn, &dummy_connector_helper_funcs);
>> + drm_encoder_helper_add(&priv->encoders[i], &drm_test_encoder_funcs);
>> +
>> + drm_connector_attach_encoder(conn, &priv->encoders[i]);
>> + }
>> +
>> +done:
>> + drm_mode_config_reset(drm);
>> +
>> + return priv;
>> +}
>> +
>> +static int set_up_atomic_state(struct kunit *test,
>> + struct drm_atomic_check_priv *priv,
>> + struct drm_connector *connector,
>> + struct drm_modeset_acquire_ctx *ctx)
>> +{
>> + struct drm_device *drm = &priv->drm;
>> + struct drm_crtc *crtc = priv->crtc;
>> + struct drm_atomic_state *state;
>> + struct drm_connector_state *conn_state;
>> + struct drm_crtc_state *crtc_state;
>> + int ret;
>> +
>> + state = drm_kunit_helper_atomic_state_alloc(test, drm, ctx);
>> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, state);
>> +
>> + if (connector) {
>> + conn_state = drm_atomic_get_connector_state(state, connector);
>> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, conn_state);
>> +
>> + ret = drm_atomic_set_crtc_for_connector(conn_state, crtc);
>> + KUNIT_EXPECT_EQ(test, ret, 0);
>> + }
>> +
>> + crtc_state = drm_atomic_get_crtc_state(state, crtc);
>> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, crtc_state);
>> +
>> + ret = drm_atomic_set_mode_for_crtc(crtc_state, &drm_atomic_test_mode);
>> + KUNIT_EXPECT_EQ(test, ret, 0);
>> +
>> + crtc_state->enable = true;
>> + crtc_state->active = true;
>> +
>> + if (connector) {
>> + ret = drm_atomic_commit(state);
>> + KUNIT_ASSERT_EQ(test, ret, 0);
>> + } else {
>> + // dummy connector mask
>> + crtc_state->connector_mask = DRM_TEST_CONN_0;
>> + }
>> +
>> + return 0;
>> +}
>> +
>
> Please provide a description of what the test does and what it checks
> (and why). See how other tests are described.
>
>> +static void drm_test_modeset(struct kunit *test)
>> +{
>> + struct drm_atomic_check_priv *priv;
>> + struct drm_modeset_acquire_ctx *ctx;
>> + struct drm_connector *old_conn, *new_conn;
>> + struct drm_atomic_state *state;
>> + struct drm_device *drm;
>> + struct drm_connector_state *new_conn_state, *old_conn_state;
>> + int ret, initial_modeset_count;
>> +
>> + priv = drm_test_init_drm_components(test, true);
>> + KUNIT_ASSERT_NOT_NULL(test, priv);
>> +
>> + drm = &priv->drm;
>> + old_conn = &priv->connectors[0];
>> + new_conn = &priv->connectors[1];
>> +
>> + ctx = drm_kunit_helper_acquire_ctx_alloc(test);
>> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx);
>> +
>> + // first modeset to enable
>> + ret = set_up_atomic_state(test, priv, old_conn, ctx);
>> + KUNIT_ASSERT_EQ(test, ret, 0);
>> +
>> + state = drm_kunit_helper_atomic_state_alloc(test, drm, ctx);
>> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, state);
>> +
>> + new_conn_state = drm_atomic_get_connector_state(state, new_conn);
>> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, new_conn_state);
>> +
>> + old_conn_state = drm_atomic_get_connector_state(state, old_conn);
>> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, old_conn_state);
>> +
>> + ret = drm_atomic_set_crtc_for_connector(old_conn_state, NULL);
>> + KUNIT_EXPECT_EQ(test, ret, 0);
>> +
>> + ret = drm_atomic_set_crtc_for_connector(new_conn_state, priv->crtc);
>> + KUNIT_EXPECT_EQ(test, ret, 0);
>> +
>> + initial_modeset_count = modeset_counter;
>> +
>> + // modeset_disables is called as part of the atomic commit tail
>> + ret = drm_atomic_commit(state);
>> + KUNIT_ASSERT_EQ(test, ret, 0);
>> + KUNIT_ASSERT_EQ(test, modeset_counter, initial_modeset_count + 1);
>> +}
>> +
>> +static struct kunit_case drm_test_check_modeset_test[] = {
>> + KUNIT_CASE(drm_test_modeset),
>> + {}
>> +};
>> +
>> +static struct kunit_suite drm_test_check_modeset_test_suite = {
>> + .name = "drm_validate_modeset",
>> + .test_cases = drm_test_check_modeset_test,
>> +};
>> +
>> +kunit_test_suite(drm_test_check_modeset_test_suite);
>> +
>> +MODULE_AUTHOR("Jessica Zhang <quic_jesszhan@...cinc.com");
>> +MODULE_DESCRIPTION("Test cases for the drm_atomic_helper functions");
>> +MODULE_LICENSE("GPL");
>>
>> --
>> 2.34.1
>>
>
> --
> With best wishes
> Dmitry
Powered by blists - more mailing lists