From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from bali.collaboradmins.com (bali.collaboradmins.com [148.251.105.195]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id B013B1FBC9B for ; Tue, 14 Jan 2025 17:58:05 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=148.251.105.195 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1736877488; cv=none; b=ez+zDkpeCc4LzkSiB0rhQXTpmdt7MzyuWJmvp8u1lMtOYibokU2/UvYenGi183MBJOrADzuj8mm3/IM8bJ4TcSB/l9B9PUfjAAKv8XUdBITkwMP0hhyD54GOKtmBJhGJZvA2FgJB/xrfBUlNN1psXqgIOUeKUOq7zfbSCifk55Y= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1736877488; c=relaxed/simple; bh=uI2jVIEACIwlosWoUvhmG7oLqSycVp5dzwis05sLrgA=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=dB+c2sm/XC9OLluUYrS3WUbAQFlnhAY/N1ac7M5wru70bxDREpavlT0cflcahGGi/e/ZRHpiORbCsrJ54XWtvcZPKK0Ta0/kDVIj0k/IspvXOnOijVRVK9sEP2WBb3vW8bWw6MtEA9VQ+zgqvPhHvIRaYhz3EHLrDcijig4UFxQ= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=collabora.com; spf=pass smtp.mailfrom=collabora.com; dkim=pass (2048-bit key) header.d=collabora.com header.i=@collabora.com header.b=RIcucp9f; arc=none smtp.client-ip=148.251.105.195 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=collabora.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=collabora.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=collabora.com header.i=@collabora.com header.b="RIcucp9f" DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=collabora.com; s=mail; t=1736877483; bh=uI2jVIEACIwlosWoUvhmG7oLqSycVp5dzwis05sLrgA=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=RIcucp9fVig/9IImlRPr4sRX3wo2aY+6LozDOjzlcSyZOAp/djc8C0WTeZ6WYgCE6 JpDWmBWfNxLoJINSb3y80BOHRx874GykXOLatXdWinT+vimuCBBqb1oIpNNPw9JLrK RxjAI+pV81VMtRDl1+0aV2jMuRvXoalosm2/YdqR5ObVzP2WRpoC/3cLkGquO8OqWV tQbJIZAR3VRNj56qEVBLQWhnChbKkWolDc/n9boX5YBsfYjhCo9xA5QzcNtDPDp/0t Vz/VEEr7TsndzUHtvW0x4SaIsYvZTKUr8jSP2vGOj/Lc4AR0Y7Zne0TKsryVTlDFeb tMfoekKKEGcJQ== Received: from [192.168.1.90] (unknown [82.76.59.196]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) (Authenticated sender: cristicc) by bali.collaboradmins.com (Postfix) with ESMTPSA id 122F917E0CD2; Tue, 14 Jan 2025 18:58:03 +0100 (CET) Message-ID: <81d36d57-7661-410b-b5e9-bd7af44686f0@collabora.com> Date: Tue, 14 Jan 2025 19:58:01 +0200 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v2 2/2] drm/tests: hdmi: Add connector's CRTC deactivation tests To: Maxime Ripard Cc: Maarten Lankhorst , Thomas Zimmermann , David Airlie , Simona Vetter , Dave Stevenson , Dmitry Baryshkov , kernel@collabora.com, dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org References: <20250109-hdmi-conn-null-mode-v2-0-9eeaf1109547@collabora.com> <20250109-hdmi-conn-null-mode-v2-2-9eeaf1109547@collabora.com> <20250114-silky-nippy-woodlouse-0f6d4a@houat> From: Cristian Ciocaltea Content-Language: en-US In-Reply-To: <20250114-silky-nippy-woodlouse-0f6d4a@houat> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit 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 >> --- >> 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