* [PATCH v2 0/2] Fixup drm_atomic_helper_connector_hdmi_check()
@ 2025-01-08 22:03 Cristian Ciocaltea
2025-01-08 22:03 ` [PATCH v2 1/2] drm/connector: hdmi: Handle NULL display mode in state check helper Cristian Ciocaltea
2025-01-08 22:03 ` [PATCH v2 2/2] drm/tests: hdmi: Add connector's CRTC deactivation tests Cristian Ciocaltea
0 siblings, 2 replies; 5+ messages in thread
From: Cristian Ciocaltea @ 2025-01-08 22:03 UTC (permalink / raw)
To: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann,
David Airlie, Simona Vetter, Dave Stevenson, Dmitry Baryshkov
Cc: kernel, dri-devel, linux-kernel, AngeloGioacchino Del Regno
Fix a NULL pointer dereference discovered while unloading a DRM module
and provide a couple of tests to make sure the helper works as expected
in case the connector's CRTC is shut down.
Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
Changes in v2:
- Collected R-b tags from Dmitry and Angelo
- Add a 2nd patch providing a couple of KUnit tests (Maxime)
- Link to v1: https://lore.kernel.org/r/20250107-hdmi-conn-null-mode-v1-1-fff4be46d106@collabora.com
---
Cristian Ciocaltea (2):
drm/connector: hdmi: Handle NULL display mode in state check helper
drm/tests: hdmi: Add connector's CRTC deactivation tests
drivers/gpu/drm/display/drm_hdmi_state_helper.c | 3 +
drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c | 173 +++++++++++++++++++++
2 files changed, 176 insertions(+)
---
base-commit: 938fbb16aba8f7b88e0fdcf56f315a5bbad41aad
change-id: 20250107-hdmi-conn-null-mode-be0c5e49ed8c
^ permalink raw reply [flat|nested] 5+ messages in thread* [PATCH v2 1/2] drm/connector: hdmi: Handle NULL display mode in state check helper 2025-01-08 22:03 [PATCH v2 0/2] Fixup drm_atomic_helper_connector_hdmi_check() Cristian Ciocaltea @ 2025-01-08 22:03 ` Cristian Ciocaltea 2025-01-08 22:03 ` [PATCH v2 2/2] drm/tests: hdmi: Add connector's CRTC deactivation tests Cristian Ciocaltea 1 sibling, 0 replies; 5+ messages in thread From: Cristian Ciocaltea @ 2025-01-08 22:03 UTC (permalink / raw) To: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie, Simona Vetter, Dave Stevenson, Dmitry Baryshkov Cc: kernel, dri-devel, linux-kernel, AngeloGioacchino Del Regno drm_atomic_helper_connector_hdmi_check() helper makes use of connector_state_get_mode() to obtain a drm_display_mode pointer, but it doesn't validate it, which may lead to a NULL pointer dereference in some cases, i.e. unloading a DRM module: [ 1002.910414] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000000 [...] [ 1002.923833] Hardware name: Radxa ROCK 5B (DT) [ 1002.924819] pc : drm_match_cea_mode+0x30/0x280 [drm] [ 1002.925318] lr : hdmi_try_format_bpc+0x7c/0x580 [drm_display_helper] [...] [ 1002.932411] Call trace: [ 1002.932626] drm_match_cea_mode+0x30/0x280 [drm] (P) [ 1002.933120] hdmi_try_format_bpc+0x7c/0x580 [drm_display_helper] [ 1002.933662] drm_atomic_helper_connector_hdmi_check+0x10c/0x478 [drm_display_helper] [ 1002.934355] drm_bridge_connector_atomic_check+0x20/0x40 [drm_display_helper] [ 1002.934993] drm_atomic_helper_check_modeset+0x698/0xd28 [drm_kms_helper] [ 1002.935607] drm_atomic_helper_check+0x28/0xb8 [drm_kms_helper] [ 1002.936143] drm_atomic_check_only+0x794/0x988 [drm] [ 1002.936635] drm_atomic_commit+0x60/0xe0 [drm] [ 1002.937082] drm_atomic_helper_disable_all+0x184/0x218 [drm_kms_helper] [ 1002.937678] drm_atomic_helper_shutdown+0x90/0x150 [drm_kms_helper] [ 1002.938243] rockchip_drm_unbind+0x38/0x80 [rockchipdrm] [ 1002.938720] component_master_del+0xac/0xf8 [ 1002.939089] rockchip_drm_platform_remove+0x34/0x78 [rockchipdrm] [...] Add the missing NULL check before passing the mode pointer further. Fixes: f035f4097f1e ("drm/connector: hdmi: Calculate TMDS character rate") Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com> --- drivers/gpu/drm/display/drm_hdmi_state_helper.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/gpu/drm/display/drm_hdmi_state_helper.c b/drivers/gpu/drm/display/drm_hdmi_state_helper.c index cfc2aaee1da08a103cbf933c891d5cc31b0886a8..e4d4fce6ab59d37067327d3e6da235c9a5a45b42 100644 --- a/drivers/gpu/drm/display/drm_hdmi_state_helper.c +++ b/drivers/gpu/drm/display/drm_hdmi_state_helper.c @@ -503,6 +503,9 @@ int drm_atomic_helper_connector_hdmi_check(struct drm_connector *connector, connector_state_get_mode(new_conn_state); int ret; + if (!mode) + return 0; + new_conn_state->hdmi.is_limited_range = hdmi_is_limited_range(connector, new_conn_state); ret = hdmi_compute_config(connector, new_conn_state, mode); -- 2.47.1 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v2 2/2] drm/tests: hdmi: Add connector's CRTC deactivation tests 2025-01-08 22:03 [PATCH v2 0/2] Fixup drm_atomic_helper_connector_hdmi_check() Cristian Ciocaltea 2025-01-08 22:03 ` [PATCH v2 1/2] drm/connector: hdmi: Handle NULL display mode in state check helper Cristian Ciocaltea @ 2025-01-08 22:03 ` Cristian Ciocaltea 2025-01-14 17:41 ` Maxime Ripard 1 sibling, 1 reply; 5+ messages in thread From: Cristian Ciocaltea @ 2025-01-08 22:03 UTC (permalink / raw) To: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie, Simona Vetter, Dave Stevenson, Dmitry Baryshkov Cc: kernel, dri-devel, linux-kernel Following up a fixed bug in drm_atomic_helper_connector_hdmi_check(), discovered while unloading a DRM module, add a couple of tests to make sure the helper will not exhibit any abnormal behaviour for use cases that involve shutting down the connector's CRTC. Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com> --- drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c | 173 +++++++++++++++++++++ 1 file changed, 173 insertions(+) diff --git a/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c b/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c index c3b693bb966f1f8b04066d19f520bfa4bf11c23d..98187ecee5d77b5f758af29f4c4bfddbd1f658fd 100644 --- a/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c +++ b/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c @@ -1568,6 +1568,177 @@ static void drm_test_check_output_bpc_format_display_8bpc_only(struct kunit *tes KUNIT_EXPECT_EQ(test, conn_state->hdmi.output_format, HDMI_COLORSPACE_RGB); } +/* + * Test that if we deactivate connector's CRTC, we trigger a mode change + * on that CRTC, along with setting {connectors|active}_changed. + */ +static void drm_test_check_crtc_deactivate_mode_changed(struct kunit *test) +{ + struct drm_atomic_helper_connector_hdmi_priv *priv; + struct drm_modeset_acquire_ctx *ctx; + struct drm_connector_state *old_conn_state; + struct drm_connector_state *new_conn_state; + struct drm_crtc_state *old_crtc_state; + struct drm_crtc_state *new_crtc_state; + struct drm_atomic_state *state; + struct drm_display_mode *preferred; + struct drm_connector *conn; + struct drm_device *drm; + struct drm_crtc *crtc; + int ret; + + priv = drm_kunit_helper_connector_hdmi_init(test, + BIT(HDMI_COLORSPACE_RGB), + 8); + KUNIT_ASSERT_NOT_NULL(test, priv); + + conn = &priv->connector; + KUNIT_ASSERT_TRUE(test, conn->display_info.is_hdmi); + + ctx = drm_kunit_helper_acquire_ctx_alloc(test); + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx); + + preferred = find_preferred_mode(conn); + KUNIT_ASSERT_NOT_NULL(test, preferred); + + drm = &priv->drm; + crtc = priv->crtc; + ret = light_up_connector(test, drm, crtc, conn, preferred, 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, conn); + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, new_conn_state); + + old_conn_state = drm_atomic_get_old_connector_state(state, conn); + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, old_conn_state); + + new_crtc_state = drm_atomic_get_crtc_state(state, crtc); + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, new_crtc_state); + + old_crtc_state = drm_atomic_get_old_crtc_state(state, crtc); + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, old_crtc_state); + + ret = drm_atomic_set_mode_for_crtc(new_crtc_state, NULL); + KUNIT_EXPECT_EQ(test, ret, 0); + KUNIT_ASSERT_NE(test, old_crtc_state->enable, new_crtc_state->enable); + + new_crtc_state->active = false; + KUNIT_ASSERT_NE(test, old_crtc_state->active, new_crtc_state->active); + + ret = drm_atomic_set_crtc_for_connector(new_conn_state, NULL); + KUNIT_ASSERT_EQ(test, ret, 0); + + KUNIT_ASSERT_PTR_NE(test, old_conn_state->crtc, new_conn_state->crtc); + + ret = drm_atomic_check_only(state); + KUNIT_ASSERT_EQ(test, ret, 0); + + new_crtc_state = drm_atomic_get_new_crtc_state(state, crtc); + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, new_crtc_state); + KUNIT_EXPECT_TRUE(test, new_crtc_state->mode_changed); + KUNIT_EXPECT_TRUE(test, new_crtc_state->connectors_changed); + KUNIT_EXPECT_TRUE(test, new_crtc_state->active_changed); +} + +/* + * Test that if we deactivate connector's CRTC, while changing the max + * bpc property to a different value, will not have any effect on the + * output bpc property. However, we still trigger a mode change on that + * CRTC, along with setting {connectors|active}_changed. + */ +static void drm_test_check_crtc_deactivate_output_bpc_not_changed(struct kunit *test) +{ + struct drm_atomic_helper_connector_hdmi_priv *priv; + struct drm_modeset_acquire_ctx *ctx; + struct drm_connector_state *old_conn_state; + struct drm_connector_state *new_conn_state; + struct drm_crtc_state *old_crtc_state; + struct drm_crtc_state *new_crtc_state; + struct drm_atomic_state *state; + struct drm_display_mode *preferred; + struct drm_connector *conn; + struct drm_device *drm; + struct drm_crtc *crtc; + int ret; + + priv = drm_kunit_helper_connector_hdmi_init(test, + BIT(HDMI_COLORSPACE_RGB), + 10); + KUNIT_ASSERT_NOT_NULL(test, priv); + + conn = &priv->connector; + ret = set_connector_edid(test, conn, + test_edid_hdmi_1080p_rgb_yuv_dc_max_200mhz, + ARRAY_SIZE(test_edid_hdmi_1080p_rgb_yuv_dc_max_200mhz)); + KUNIT_ASSERT_GT(test, ret, 0); + + ctx = drm_kunit_helper_acquire_ctx_alloc(test); + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx); + + preferred = find_preferred_mode(conn); + KUNIT_ASSERT_NOT_NULL(test, preferred); + + drm = &priv->drm; + crtc = priv->crtc; + ret = light_up_connector(test, drm, crtc, conn, preferred, 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, conn); + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, new_conn_state); + + old_conn_state = drm_atomic_get_old_connector_state(state, conn); + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, old_conn_state); + + new_conn_state->max_requested_bpc = 8; + + KUNIT_ASSERT_NE(test, + old_conn_state->max_requested_bpc, + new_conn_state->max_requested_bpc); + + new_crtc_state = drm_atomic_get_crtc_state(state, crtc); + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, new_crtc_state); + + old_crtc_state = drm_atomic_get_old_crtc_state(state, crtc); + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, old_crtc_state); + + ret = drm_atomic_set_mode_for_crtc(new_crtc_state, NULL); + KUNIT_EXPECT_EQ(test, ret, 0); + KUNIT_ASSERT_NE(test, old_crtc_state->enable, new_crtc_state->enable); + + new_crtc_state->active = false; + KUNIT_ASSERT_NE(test, old_crtc_state->active, new_crtc_state->active); + + ret = drm_atomic_set_crtc_for_connector(new_conn_state, NULL); + KUNIT_ASSERT_EQ(test, ret, 0); + + KUNIT_ASSERT_PTR_NE(test, old_conn_state->crtc, new_conn_state->crtc); + + ret = drm_atomic_check_only(state); + KUNIT_ASSERT_EQ(test, ret, 0); + + old_conn_state = drm_atomic_get_old_connector_state(state, conn); + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, old_conn_state); + + new_conn_state = drm_atomic_get_new_connector_state(state, conn); + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, new_conn_state); + + KUNIT_EXPECT_EQ(test, + old_conn_state->hdmi.output_bpc, + new_conn_state->hdmi.output_bpc); + + new_crtc_state = drm_atomic_get_new_crtc_state(state, crtc); + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, new_crtc_state); + KUNIT_EXPECT_TRUE(test, new_crtc_state->mode_changed); + KUNIT_EXPECT_TRUE(test, new_crtc_state->connectors_changed); + KUNIT_EXPECT_TRUE(test, new_crtc_state->active_changed); +} + static struct kunit_case drm_atomic_helper_connector_hdmi_check_tests[] = { KUNIT_CASE(drm_test_check_broadcast_rgb_auto_cea_mode), KUNIT_CASE(drm_test_check_broadcast_rgb_auto_cea_mode_vic_1), @@ -1596,6 +1767,8 @@ static struct kunit_case drm_atomic_helper_connector_hdmi_check_tests[] = { KUNIT_CASE(drm_test_check_tmds_char_rate_rgb_8bpc), KUNIT_CASE(drm_test_check_tmds_char_rate_rgb_10bpc), KUNIT_CASE(drm_test_check_tmds_char_rate_rgb_12bpc), + KUNIT_CASE(drm_test_check_crtc_deactivate_mode_changed), + KUNIT_CASE(drm_test_check_crtc_deactivate_output_bpc_not_changed), /* * TODO: We should have tests to check that a change in the * format triggers a CRTC mode change just like we do for the -- 2.47.1 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2 2/2] drm/tests: hdmi: Add connector's CRTC deactivation tests 2025-01-08 22:03 ` [PATCH v2 2/2] drm/tests: hdmi: Add connector's CRTC deactivation tests Cristian Ciocaltea @ 2025-01-14 17:41 ` Maxime Ripard 2025-01-14 17:58 ` Cristian Ciocaltea 0 siblings, 1 reply; 5+ messages in thread From: Maxime Ripard @ 2025-01-14 17:41 UTC (permalink / raw) To: Cristian Ciocaltea Cc: Maarten Lankhorst, Thomas Zimmermann, David Airlie, Simona Vetter, Dave Stevenson, Dmitry Baryshkov, kernel, dri-devel, linux-kernel [-- Attachment #1: Type: text/plain, Size: 7770 bytes --] Hi, On Thu, Jan 09, 2025 at 12:03:40AM +0200, Cristian Ciocaltea wrote: > Following up a fixed bug in drm_atomic_helper_connector_hdmi_check(), > discovered while unloading a DRM module, add a couple of tests to make > sure the helper will not exhibit any abnormal behaviour for use cases > that involve shutting down the connector's CRTC. > > Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com> > --- > drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c | 173 +++++++++++++++++++++ > 1 file changed, 173 insertions(+) > > diff --git a/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c b/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c > index c3b693bb966f1f8b04066d19f520bfa4bf11c23d..98187ecee5d77b5f758af29f4c4bfddbd1f658fd 100644 > --- a/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c > +++ b/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c > @@ -1568,6 +1568,177 @@ static void drm_test_check_output_bpc_format_display_8bpc_only(struct kunit *tes > KUNIT_EXPECT_EQ(test, conn_state->hdmi.output_format, HDMI_COLORSPACE_RGB); > } > > +/* > + * Test that if we deactivate connector's CRTC, we trigger a mode change > + * on that CRTC, along with setting {connectors|active}_changed. > + */ > +static void drm_test_check_crtc_deactivate_mode_changed(struct kunit *test) > +{ > + struct drm_atomic_helper_connector_hdmi_priv *priv; > + struct drm_modeset_acquire_ctx *ctx; > + struct drm_connector_state *old_conn_state; > + struct drm_connector_state *new_conn_state; > + struct drm_crtc_state *old_crtc_state; > + struct drm_crtc_state *new_crtc_state; > + struct drm_atomic_state *state; > + struct drm_display_mode *preferred; > + struct drm_connector *conn; > + struct drm_device *drm; > + struct drm_crtc *crtc; > + int ret; > + > + priv = drm_kunit_helper_connector_hdmi_init(test, > + BIT(HDMI_COLORSPACE_RGB), > + 8); > + KUNIT_ASSERT_NOT_NULL(test, priv); > + > + conn = &priv->connector; > + KUNIT_ASSERT_TRUE(test, conn->display_info.is_hdmi); > + > + ctx = drm_kunit_helper_acquire_ctx_alloc(test); > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx); > + > + preferred = find_preferred_mode(conn); > + KUNIT_ASSERT_NOT_NULL(test, preferred); > + > + drm = &priv->drm; > + crtc = priv->crtc; > + ret = light_up_connector(test, drm, crtc, conn, preferred, 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, conn); > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, new_conn_state); > + > + old_conn_state = drm_atomic_get_old_connector_state(state, conn); > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, old_conn_state); > + > + new_crtc_state = drm_atomic_get_crtc_state(state, crtc); > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, new_crtc_state); > + > + old_crtc_state = drm_atomic_get_old_crtc_state(state, crtc); > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, old_crtc_state); > + > + ret = drm_atomic_set_mode_for_crtc(new_crtc_state, NULL); > + KUNIT_EXPECT_EQ(test, ret, 0); > + KUNIT_ASSERT_NE(test, old_crtc_state->enable, new_crtc_state->enable); > + > + new_crtc_state->active = false; > + KUNIT_ASSERT_NE(test, old_crtc_state->active, new_crtc_state->active); > + > + ret = drm_atomic_set_crtc_for_connector(new_conn_state, NULL); > + KUNIT_ASSERT_EQ(test, ret, 0); > + > + KUNIT_ASSERT_PTR_NE(test, old_conn_state->crtc, new_conn_state->crtc); > + > + ret = drm_atomic_check_only(state); > + KUNIT_ASSERT_EQ(test, ret, 0); > + > + new_crtc_state = drm_atomic_get_new_crtc_state(state, crtc); > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, new_crtc_state); > + KUNIT_EXPECT_TRUE(test, new_crtc_state->mode_changed); > + KUNIT_EXPECT_TRUE(test, new_crtc_state->connectors_changed); > + KUNIT_EXPECT_TRUE(test, new_crtc_state->active_changed); > +} This one looks good to me > +/* > + * Test that if we deactivate connector's CRTC, while changing the max > + * bpc property to a different value, will not have any effect on the > + * output bpc property. However, we still trigger a mode change on that > + * CRTC, along with setting {connectors|active}_changed. > + */ > +static void drm_test_check_crtc_deactivate_output_bpc_not_changed(struct kunit *test) > +{ > + struct drm_atomic_helper_connector_hdmi_priv *priv; > + struct drm_modeset_acquire_ctx *ctx; > + struct drm_connector_state *old_conn_state; > + struct drm_connector_state *new_conn_state; > + struct drm_crtc_state *old_crtc_state; > + struct drm_crtc_state *new_crtc_state; > + struct drm_atomic_state *state; > + struct drm_display_mode *preferred; > + struct drm_connector *conn; > + struct drm_device *drm; > + struct drm_crtc *crtc; > + int ret; > + > + priv = drm_kunit_helper_connector_hdmi_init(test, > + BIT(HDMI_COLORSPACE_RGB), > + 10); > + KUNIT_ASSERT_NOT_NULL(test, priv); > + > + conn = &priv->connector; > + ret = set_connector_edid(test, conn, > + test_edid_hdmi_1080p_rgb_yuv_dc_max_200mhz, > + ARRAY_SIZE(test_edid_hdmi_1080p_rgb_yuv_dc_max_200mhz)); > + KUNIT_ASSERT_GT(test, ret, 0); > + > + ctx = drm_kunit_helper_acquire_ctx_alloc(test); > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx); > + > + preferred = find_preferred_mode(conn); > + KUNIT_ASSERT_NOT_NULL(test, preferred); > + > + drm = &priv->drm; > + crtc = priv->crtc; > + ret = light_up_connector(test, drm, crtc, conn, preferred, 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, conn); > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, new_conn_state); > + > + old_conn_state = drm_atomic_get_old_connector_state(state, conn); > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, old_conn_state); > + > + new_conn_state->max_requested_bpc = 8; > + > + KUNIT_ASSERT_NE(test, > + old_conn_state->max_requested_bpc, > + new_conn_state->max_requested_bpc); > + > + new_crtc_state = drm_atomic_get_crtc_state(state, crtc); > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, new_crtc_state); > + > + old_crtc_state = drm_atomic_get_old_crtc_state(state, crtc); > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, old_crtc_state); > + > + ret = drm_atomic_set_mode_for_crtc(new_crtc_state, NULL); > + KUNIT_EXPECT_EQ(test, ret, 0); > + KUNIT_ASSERT_NE(test, old_crtc_state->enable, new_crtc_state->enable); > + > + new_crtc_state->active = false; > + KUNIT_ASSERT_NE(test, old_crtc_state->active, new_crtc_state->active); > + > + ret = drm_atomic_set_crtc_for_connector(new_conn_state, NULL); > + KUNIT_ASSERT_EQ(test, ret, 0); > + > + KUNIT_ASSERT_PTR_NE(test, old_conn_state->crtc, new_conn_state->crtc); > + > + ret = drm_atomic_check_only(state); > + KUNIT_ASSERT_EQ(test, ret, 0); > + > + old_conn_state = drm_atomic_get_old_connector_state(state, conn); > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, old_conn_state); > + > + new_conn_state = drm_atomic_get_new_connector_state(state, conn); > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, new_conn_state); > + > + KUNIT_EXPECT_EQ(test, > + old_conn_state->hdmi.output_bpc, > + new_conn_state->hdmi.output_bpc); > + > + new_crtc_state = drm_atomic_get_new_crtc_state(state, crtc); > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, new_crtc_state); > + KUNIT_EXPECT_TRUE(test, new_crtc_state->mode_changed); > + KUNIT_EXPECT_TRUE(test, new_crtc_state->connectors_changed); > + KUNIT_EXPECT_TRUE(test, new_crtc_state->active_changed); > +} > + However, it's not clear to me why changing bpc should change anything, or why it's worth testing? Maxime [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 273 bytes --] ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2 2/2] drm/tests: hdmi: Add connector's CRTC deactivation tests 2025-01-14 17:41 ` Maxime Ripard @ 2025-01-14 17:58 ` Cristian Ciocaltea 0 siblings, 0 replies; 5+ messages in thread From: Cristian Ciocaltea @ 2025-01-14 17:58 UTC (permalink / raw) To: Maxime Ripard Cc: Maarten Lankhorst, Thomas Zimmermann, David Airlie, Simona Vetter, Dave Stevenson, Dmitry Baryshkov, kernel, dri-devel, linux-kernel Hi Maxime, On 1/14/25 7:41 PM, Maxime Ripard wrote: > Hi, > > On Thu, Jan 09, 2025 at 12:03:40AM +0200, Cristian Ciocaltea wrote: >> Following up a fixed bug in drm_atomic_helper_connector_hdmi_check(), >> discovered while unloading a DRM module, add a couple of tests to make >> sure the helper will not exhibit any abnormal behaviour for use cases >> that involve shutting down the connector's CRTC. >> >> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com> >> --- >> drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c | 173 +++++++++++++++++++++ >> 1 file changed, 173 insertions(+) >> >> diff --git a/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c b/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c >> index c3b693bb966f1f8b04066d19f520bfa4bf11c23d..98187ecee5d77b5f758af29f4c4bfddbd1f658fd 100644 >> --- a/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c >> +++ b/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c >> @@ -1568,6 +1568,177 @@ static void drm_test_check_output_bpc_format_display_8bpc_only(struct kunit *tes >> KUNIT_EXPECT_EQ(test, conn_state->hdmi.output_format, HDMI_COLORSPACE_RGB); >> } >> >> +/* >> + * Test that if we deactivate connector's CRTC, we trigger a mode change >> + * on that CRTC, along with setting {connectors|active}_changed. >> + */ >> +static void drm_test_check_crtc_deactivate_mode_changed(struct kunit *test) >> +{ >> + struct drm_atomic_helper_connector_hdmi_priv *priv; >> + struct drm_modeset_acquire_ctx *ctx; >> + struct drm_connector_state *old_conn_state; >> + struct drm_connector_state *new_conn_state; >> + struct drm_crtc_state *old_crtc_state; >> + struct drm_crtc_state *new_crtc_state; >> + struct drm_atomic_state *state; >> + struct drm_display_mode *preferred; >> + struct drm_connector *conn; >> + struct drm_device *drm; >> + struct drm_crtc *crtc; >> + int ret; >> + >> + priv = drm_kunit_helper_connector_hdmi_init(test, >> + BIT(HDMI_COLORSPACE_RGB), >> + 8); >> + KUNIT_ASSERT_NOT_NULL(test, priv); >> + >> + conn = &priv->connector; >> + KUNIT_ASSERT_TRUE(test, conn->display_info.is_hdmi); >> + >> + ctx = drm_kunit_helper_acquire_ctx_alloc(test); >> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx); >> + >> + preferred = find_preferred_mode(conn); >> + KUNIT_ASSERT_NOT_NULL(test, preferred); >> + >> + drm = &priv->drm; >> + crtc = priv->crtc; >> + ret = light_up_connector(test, drm, crtc, conn, preferred, 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, conn); >> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, new_conn_state); >> + >> + old_conn_state = drm_atomic_get_old_connector_state(state, conn); >> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, old_conn_state); >> + >> + new_crtc_state = drm_atomic_get_crtc_state(state, crtc); >> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, new_crtc_state); >> + >> + old_crtc_state = drm_atomic_get_old_crtc_state(state, crtc); >> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, old_crtc_state); >> + >> + ret = drm_atomic_set_mode_for_crtc(new_crtc_state, NULL); >> + KUNIT_EXPECT_EQ(test, ret, 0); >> + KUNIT_ASSERT_NE(test, old_crtc_state->enable, new_crtc_state->enable); >> + >> + new_crtc_state->active = false; >> + KUNIT_ASSERT_NE(test, old_crtc_state->active, new_crtc_state->active); >> + >> + ret = drm_atomic_set_crtc_for_connector(new_conn_state, NULL); >> + KUNIT_ASSERT_EQ(test, ret, 0); >> + >> + KUNIT_ASSERT_PTR_NE(test, old_conn_state->crtc, new_conn_state->crtc); >> + >> + ret = drm_atomic_check_only(state); >> + KUNIT_ASSERT_EQ(test, ret, 0); >> + >> + new_crtc_state = drm_atomic_get_new_crtc_state(state, crtc); >> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, new_crtc_state); >> + KUNIT_EXPECT_TRUE(test, new_crtc_state->mode_changed); >> + KUNIT_EXPECT_TRUE(test, new_crtc_state->connectors_changed); >> + KUNIT_EXPECT_TRUE(test, new_crtc_state->active_changed); >> +} > > This one looks good to me > >> +/* >> + * Test that if we deactivate connector's CRTC, while changing the max >> + * bpc property to a different value, will not have any effect on the >> + * output bpc property. However, we still trigger a mode change on that >> + * CRTC, along with setting {connectors|active}_changed. >> + */ >> +static void drm_test_check_crtc_deactivate_output_bpc_not_changed(struct kunit *test) >> +{ >> + struct drm_atomic_helper_connector_hdmi_priv *priv; >> + struct drm_modeset_acquire_ctx *ctx; >> + struct drm_connector_state *old_conn_state; >> + struct drm_connector_state *new_conn_state; >> + struct drm_crtc_state *old_crtc_state; >> + struct drm_crtc_state *new_crtc_state; >> + struct drm_atomic_state *state; >> + struct drm_display_mode *preferred; >> + struct drm_connector *conn; >> + struct drm_device *drm; >> + struct drm_crtc *crtc; >> + int ret; >> + >> + priv = drm_kunit_helper_connector_hdmi_init(test, >> + BIT(HDMI_COLORSPACE_RGB), >> + 10); >> + KUNIT_ASSERT_NOT_NULL(test, priv); >> + >> + conn = &priv->connector; >> + ret = set_connector_edid(test, conn, >> + test_edid_hdmi_1080p_rgb_yuv_dc_max_200mhz, >> + ARRAY_SIZE(test_edid_hdmi_1080p_rgb_yuv_dc_max_200mhz)); >> + KUNIT_ASSERT_GT(test, ret, 0); >> + >> + ctx = drm_kunit_helper_acquire_ctx_alloc(test); >> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx); >> + >> + preferred = find_preferred_mode(conn); >> + KUNIT_ASSERT_NOT_NULL(test, preferred); >> + >> + drm = &priv->drm; >> + crtc = priv->crtc; >> + ret = light_up_connector(test, drm, crtc, conn, preferred, 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, conn); >> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, new_conn_state); >> + >> + old_conn_state = drm_atomic_get_old_connector_state(state, conn); >> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, old_conn_state); >> + >> + new_conn_state->max_requested_bpc = 8; >> + >> + KUNIT_ASSERT_NE(test, >> + old_conn_state->max_requested_bpc, >> + new_conn_state->max_requested_bpc); >> + >> + new_crtc_state = drm_atomic_get_crtc_state(state, crtc); >> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, new_crtc_state); >> + >> + old_crtc_state = drm_atomic_get_old_crtc_state(state, crtc); >> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, old_crtc_state); >> + >> + ret = drm_atomic_set_mode_for_crtc(new_crtc_state, NULL); >> + KUNIT_EXPECT_EQ(test, ret, 0); >> + KUNIT_ASSERT_NE(test, old_crtc_state->enable, new_crtc_state->enable); >> + >> + new_crtc_state->active = false; >> + KUNIT_ASSERT_NE(test, old_crtc_state->active, new_crtc_state->active); >> + >> + ret = drm_atomic_set_crtc_for_connector(new_conn_state, NULL); >> + KUNIT_ASSERT_EQ(test, ret, 0); >> + >> + KUNIT_ASSERT_PTR_NE(test, old_conn_state->crtc, new_conn_state->crtc); >> + >> + ret = drm_atomic_check_only(state); >> + KUNIT_ASSERT_EQ(test, ret, 0); >> + >> + old_conn_state = drm_atomic_get_old_connector_state(state, conn); >> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, old_conn_state); >> + >> + new_conn_state = drm_atomic_get_new_connector_state(state, conn); >> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, new_conn_state); >> + >> + KUNIT_EXPECT_EQ(test, >> + old_conn_state->hdmi.output_bpc, >> + new_conn_state->hdmi.output_bpc); >> + >> + new_crtc_state = drm_atomic_get_new_crtc_state(state, crtc); >> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, new_crtc_state); >> + KUNIT_EXPECT_TRUE(test, new_crtc_state->mode_changed); >> + KUNIT_EXPECT_TRUE(test, new_crtc_state->connectors_changed); >> + KUNIT_EXPECT_TRUE(test, new_crtc_state->active_changed); >> +} >> + > > However, it's not clear to me why changing bpc should change anything, > or why it's worth testing? I suppose I may have gone a little too far with all this testing :-) Will drop and send v3. Thanks for reviewing, Cristian ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2025-01-14 17:58 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2025-01-08 22:03 [PATCH v2 0/2] Fixup drm_atomic_helper_connector_hdmi_check() Cristian Ciocaltea 2025-01-08 22:03 ` [PATCH v2 1/2] drm/connector: hdmi: Handle NULL display mode in state check helper Cristian Ciocaltea 2025-01-08 22:03 ` [PATCH v2 2/2] drm/tests: hdmi: Add connector's CRTC deactivation tests Cristian Ciocaltea 2025-01-14 17:41 ` Maxime Ripard 2025-01-14 17:58 ` Cristian Ciocaltea
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®