[PATCH 0/3] drm/amd/display: Fix assorted test failures

[PATCH 0/3] drm/amd/display: Fix assorted test failures

[John Harrison]

Running the KUNIT tests with KASAN enabled hits a bunch of
use-after-free errors. Also, the colorop tests were failing due to the
pipeline having been extended. So, fix those up.

Note that the following tests are still failing for me. They are
EXPECTATION failures though, rather than memory splats. So, the test
run does at least complete now.
  dm_test_crtc_reset_state_replaces_existing
  dm_test_detect_hides_secondary_tile
  dm_test_fs_caps_dp_msa_not_allowed
  dm_test_update_plane_color_mgmt_colorop_shaper
  dm_test_update_plane_color_mgmt_colorop_3dlut
  dm_test_update_plane_color_mgmt_colorop_blend

Signed-off-by: John Harrison <John.Harrison@Igalia.com>

John Harrison (3):
  drm/amd/display: Fix some memory lifetime issues in KUnit tests
  drm/amd/display: Fix use-after-free in
    dm_test_hpd_rx_offload_work_msg_rdy
  drm/amd/display: Update colorop pipeline tests for new fixed matrix op

 .../amd/display/amdgpu_dm/tests/amdgpu_dm_colorop_test.c  | 4 ++++
 .../display/amdgpu_dm/tests/amdgpu_dm_connector_test.c    | 2 +-
 .../drm/amd/display/amdgpu_dm/tests/amdgpu_dm_irq_test.c  | 8 +++++++-
 .../display/amdgpu_dm/tests/amdgpu_dm_mst_types_test.c    | 2 +-
 .../drm/amd/display/amdgpu_dm/tests/amdgpu_dm_wb_test.c   | 2 +-
 5 files changed, 14 insertions(+), 4 deletions(-)

-- 
2.43.0

[PATCH 1/3] drm/amd/display: Fix some memory lifetime issues in KUnit tests

[John Harrison]

When running the KUnit tests with memory debugging enabled, a bunch of
use-after-free errors are hit. Fix them by using the appropriate alloc
manager (which might be none - write back connectors and DP MST ports
both have their own lifetime management).

Signed-off-by: John Harrison <John.Harrison@Igalia.com>
CC: Alex Hung <alex.hung@amd.com>
CC: Harry Wentland <harry.wentland@amd.com>
---
 .../drm/amd/display/amdgpu_dm/tests/amdgpu_dm_connector_test.c  | 2 +-
 .../drm/amd/display/amdgpu_dm/tests/amdgpu_dm_mst_types_test.c  | 2 +-
 drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_wb_test.c | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_connector_test.c b/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_connector_test.c
index 5fdf8ce023b9..aa72934e1fea 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_connector_test.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_connector_test.c
@@ -9702,7 +9702,7 @@ dm_test_conn_init_ctx_alloc(struct kunit *test, enum signal_type signal)
 	ctx->adev->mode_info.num_crtc = 1;
 	dm_test_create_mode_props(test, ctx->adev);
 
-	pdev = kunit_kzalloc(test, sizeof(*pdev), GFP_KERNEL);
+	pdev = drmm_kzalloc(ctx->drm, sizeof(*pdev), GFP_KERNEL);
 	KUNIT_ASSERT_NOT_NULL(test, pdev);
 	device_initialize(&pdev->dev);
 	pdev->dev.parent = dev;
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_mst_types_test.c b/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_mst_types_test.c
index eafa3e6733a6..5dc6cd880169 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_mst_types_test.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_mst_types_test.c
@@ -3833,7 +3833,7 @@ dm_mst_test_alloc_dsc_link_ctx(struct kunit *test, int total_avail_slots)
 	link_srv = kunit_kzalloc(test, sizeof(*link_srv), GFP_KERNEL);
 	res_pool = kunit_kzalloc(test, sizeof(*res_pool), GFP_KERNEL);
 	sink = kunit_kzalloc(test, sizeof(*sink), GFP_KERNEL);
-	port = kunit_kzalloc(test, sizeof(*port), GFP_KERNEL);
+	port = kzalloc(sizeof(*port), GFP_KERNEL);
 	KUNIT_ASSERT_NOT_NULL(test, link_srv);
 	KUNIT_ASSERT_NOT_NULL(test, res_pool);
 	KUNIT_ASSERT_NOT_NULL(test, sink);
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_wb_test.c b/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_wb_test.c
index 4bc033fdfa97..f3b64a66cf28 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_wb_test.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_wb_test.c
@@ -500,7 +500,7 @@ static void dm_test_wb_connector_init_success(struct kunit *test)
 	dc->links[0] = link;
 	dm->dc = dc;
 
-	wbcon = kunit_kzalloc(test, sizeof(*wbcon), GFP_KERNEL);
+	wbcon = kzalloc_obj(*wbcon);
 	KUNIT_ASSERT_NOT_NULL(test, wbcon);
 
 	ret = amdgpu_dm_wb_connector_init(dm, wbcon, 0);
-- 
2.43.0

[PATCH 2/3] drm/amd/display: Fix use-after-free in dm_test_hpd_rx_offload_work_msg_rdy

[John Harrison]

The test runs a worker thread and verifies that it did something.
However, the worker frees up the object that the 'did something' check
inspects.

The check is actually looking at a bool inside a different object (the
work queue) but using the work item object to get there. So just save
a pointer to the queue object and use that in the test.

Signed-off-by: John Harrison <John.Harrison@Igalia.com>
---
 .../drm/amd/display/amdgpu_dm/tests/amdgpu_dm_irq_test.c  | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_irq_test.c b/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_irq_test.c
index 6e599657dac1..ee96ddb9669c 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_irq_test.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_irq_test.c
@@ -2363,6 +2363,7 @@ static void dm_test_hpd_rx_offload_work_in_reset(struct kunit *test)
  */
 static void dm_test_hpd_rx_offload_work_msg_rdy(struct kunit *test)
 {
+	struct hpd_rx_irq_offload_work_queue *offload_wq;
 	struct hpd_rx_irq_offload_work *offload_work;
 	struct amdgpu_dm_connector *aconn;
 	struct link_service *link_srv;
@@ -2384,9 +2385,14 @@ static void dm_test_hpd_rx_offload_work_msg_rdy(struct kunit *test)
 	offload_work->offload_wq->is_handling_mst_msg_rdy_event = true;
 	offload_work->data.bytes.device_service_irq.bits.UP_REQ_MSG_RDY = 1;
 
+	/* NB: offload_work is freed by dm_handle_hpd_rx_offload_work so need to
+	 * save a pointer to the wq to do the final KUNIT_EXPECT. This is safe
+	 * because the wq is actually a kunit_alloc and is not freed along with
+	 * the offload_work object. */
+	offload_wq = offload_work->offload_wq;
 	dm_handle_hpd_rx_offload_work(&offload_work->work);
 
-	KUNIT_EXPECT_FALSE(test, offload_work->offload_wq->is_handling_mst_msg_rdy_event);
+	KUNIT_EXPECT_FALSE(test, offload_wq->is_handling_mst_msg_rdy_event);
 }
 
 /**
-- 
2.43.0

[PATCH 3/3] drm/amd/display: Update colorop pipeline tests for new fixed matrix op

[John Harrison]

A bunch of tests verify that the pipeline created by the driver
matches an expected pipeline hard coded into the test. The below patch
added a new op into the driver's pipeline but did not update the tests
to match. So add it in.

Fixes: 6d103ad11583 ("drm/amd/display: Add fixed matrix colorop to color pipeline")
Cc: Harry Wentland <harry.wentland@amd.com>
Cc: Alex Hung <alex.hung@amd.com>
Signed-off-by: John Harrison <John.Harrison@Igalia.com>
---
 .../drm/amd/display/amdgpu_dm/tests/amdgpu_dm_colorop_test.c  | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_colorop_test.c b/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_colorop_test.c
index 90cc5592ef5d..00f1d5b33b1f 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_colorop_test.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_colorop_test.c
@@ -192,6 +192,7 @@ static void dm_expect_colorop_pipeline(struct kunit *test, struct drm_device *dr
 static void dm_test_initialize_default_pipeline(struct kunit *test)
 {
 	static const enum drm_colorop_type expected[] = {
+		DRM_COLOROP_FIXED_MATRIX,
 		DRM_COLOROP_1D_CURVE,	/* degam TF */
 		DRM_COLOROP_MULTIPLIER,
 		DRM_COLOROP_CTM_3X4,
@@ -257,6 +258,7 @@ static void dm_test_initialize_default_pipeline_caps(struct kunit *test,
 static void dm_test_initialize_default_pipeline_dpp_3d_lut(struct kunit *test)
 {
 	static const enum drm_colorop_type expected[] = {
+		DRM_COLOROP_FIXED_MATRIX,
 		DRM_COLOROP_1D_CURVE,
 		DRM_COLOROP_MULTIPLIER,
 		DRM_COLOROP_CTM_3X4,
@@ -278,6 +280,7 @@ static void dm_test_initialize_default_pipeline_dpp_3d_lut(struct kunit *test)
 static void dm_test_initialize_default_pipeline_mpc_preblend(struct kunit *test)
 {
 	static const enum drm_colorop_type expected[] = {
+		DRM_COLOROP_FIXED_MATRIX,
 		DRM_COLOROP_1D_CURVE,
 		DRM_COLOROP_MULTIPLIER,
 		DRM_COLOROP_CTM_3X4,
@@ -299,6 +302,7 @@ static void dm_test_initialize_default_pipeline_mpc_preblend(struct kunit *test)
 static void dm_test_initialize_default_pipeline_no_3d_lut(struct kunit *test)
 {
 	static const enum drm_colorop_type expected[] = {
+		DRM_COLOROP_FIXED_MATRIX,
 		DRM_COLOROP_1D_CURVE,
 		DRM_COLOROP_MULTIPLIER,
 		DRM_COLOROP_CTM_3X4,
-- 
2.43.0

Re: [PATCH 3/3] drm/amd/display: Update colorop pipeline tests for new fixed matrix op

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] drm/amd/display: undeclared DRM_COLOROP_FIXED_MATRIX breaks KUnit test build
--

commit fa1c05a038fce65c050bce3e6d89515560ce7081
Author: John Harrison <John.Harrison@Igalia.com>

drm/amd/display: Update colorop pipeline tests for new fixed matrix op

This commit updates the expected colorop pipeline arrays in the amdgpu_dm
KUnit tests to begin with DRM_COLOROP_FIXED_MATRIX.

> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_colorop_test.c b/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_colorop_test.c
> index 90cc5592ef5de..00f1d5b33b1f0 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_colorop_test.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_colorop_test.c
> @@ -192,6 +192,7 @@ static void dm_expect_colorop_pipeline(struct kunit *test, struct drm_device *dr
>  static void dm_test_initialize_default_pipeline(struct kunit *test)
>  {
>  	static const enum drm_colorop_type expected[] = {
> +		DRM_COLOROP_FIXED_MATRIX,
>  		DRM_COLOROP_1D_CURVE,	/* degam TF */
>  		DRM_COLOROP_MULTIPLIER,
>  		DRM_COLOROP_CTM_3X4,
> @@ -257,6 +258,7 @@ static void dm_test_initialize_default_pipeline_caps(struct kunit *test,
>  static void dm_test_initialize_default_pipeline_dpp_3d_lut(struct kunit *test)
>  {
>  	static const enum drm_colorop_type expected[] = {
> +		DRM_COLOROP_FIXED_MATRIX,
>  		DRM_COLOROP_1D_CURVE,
>  		DRM_COLOROP_MULTIPLIER,
>  		DRM_COLOROP_CTM_3X4,
> @@ -278,6 +280,7 @@ static void dm_test_initialize_default_pipeline_dpp_3d_lut(struct kunit *test)
>  static void dm_test_initialize_default_pipeline_mpc_preblend(struct kunit *test)
>  {
>  	static const enum drm_colorop_type expected[] = {
> +		DRM_COLOROP_FIXED_MATRIX,
>  		DRM_COLOROP_1D_CURVE,
>  		DRM_COLOROP_MULTIPLIER,
>  		DRM_COLOROP_CTM_3X4,
> @@ -299,6 +302,7 @@ static void dm_test_initialize_default_pipeline_mpc_preblend(struct kunit *test)
>  static void dm_test_initialize_default_pipeline_no_3d_lut(struct kunit *test)
>  {
>  	static const enum drm_colorop_type expected[] = {
> +		DRM_COLOROP_FIXED_MATRIX,
>  		DRM_COLOROP_1D_CURVE,
>  		DRM_COLOROP_MULTIPLIER,
>  		DRM_COLOROP_CTM_3X4,

[Severity: Medium]
Does this break the build when CONFIG_DRM_AMD_DC_KUNIT_TEST is enabled?

This patch adds DRM_COLOROP_FIXED_MATRIX to the expected[] arrays in
dm_test_initialize_default_pipeline(),
dm_test_initialize_default_pipeline_dpp_3d_lut(),
dm_test_initialize_default_pipeline_mpc_preblend(), and
dm_test_initialize_default_pipeline_no_3d_lut() in
drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_colorop_test.c.

The commit references Fixes: 6d103ad11583 ("drm/amd/display: Add fixed matrix
colorop to color pipeline"), which in turn depends on f6c58e4ae87c
("drm/colorop: Add DRM_COLOROP_FIXED_MATRIX"), but neither commit is present
in this branch.

In include/uapi/drm/drm_mode.h, enum drm_colorop_type does not define
DRM_COLOROP_FIXED_MATRIX:

    enum drm_colorop_type {
            DRM_COLOROP_1D_CURVE,
            DRM_COLOROP_1D_LUT,
            DRM_COLOROP_CTM_3X4,
            DRM_COLOROP_MULTIPLIER,
            DRM_COLOROP_3D_LUT,
    };

In addition, amdgpu_dm_build_default_pipeline() in
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_colorop.c still starts the
default pipeline with DRM_COLOROP_1D_CURVE:

    /* 1D curve - DEGAM TF */
    ops[i] = colorop_kzalloc_obj();
    if (!ops[i]) {
            ret = -ENOMEM;
            goto cleanup;
    }

    ret = colorop_curve_1d_init(dev, ops[i], plane, &dm_colorop_funcs,
                                amdgpu_dm_supported_degam_tfs,
                                DRM_COLOROP_FLAG_ALLOW_BYPASS);

Should this patch only be applied to a branch that already contains commits
f6c58e4ae87c and 6d103ad11583?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261022173655.2318987-1-John.Harrison@Igalia.com?part=3

Re: [PATCH 1/3] drm/amd/display: Fix some memory lifetime issues in KUnit tests

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] drm/amd/display: memory leak of wbcon in dm_test_wb_connector_init_success()
- [Medium] drm/amd/display: memory leak of port in dm_mst_test_alloc_dsc_link_ctx()
--

commit 46c185efd7ffdbf05586b8f2bd12c8e611ccd969
Author: John Harrison <John.Harrison@Igalia.com>

drm/amd/display: Fix some memory lifetime issues in KUnit tests

This commit updates memory allocations in several amdgpu display KUnit
tests to avoid use-after-free errors during test teardown when memory
debugging is enabled.

[ ... ]

> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_mst_types_test.c b/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_mst_types_test.c
> index eafa3e6733a64..5dc6cd8801691 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_mst_types_test.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_mst_types_test.c
> @@ -3833,7 +3833,7 @@ dm_mst_test_alloc_dsc_link_ctx(struct kunit *test, int total_avail_slots)
>  	link_srv = kunit_kzalloc(test, sizeof(*link_srv), GFP_KERNEL);
>  	res_pool = kunit_kzalloc(test, sizeof(*res_pool), GFP_KERNEL);
>  	sink = kunit_kzalloc(test, sizeof(*sink), GFP_KERNEL);
> -	port = kunit_kzalloc(test, sizeof(*port), GFP_KERNEL);
> +	port = kzalloc(sizeof(*port), GFP_KERNEL);
>  	KUNIT_ASSERT_NOT_NULL(test, link_srv);
>  	KUNIT_ASSERT_NOT_NULL(test, res_pool);
>  	KUNIT_ASSERT_NOT_NULL(test, sink);

[Severity: Medium]
Does this leak port in dm_mst_test_alloc_dsc_link_ctx() in
drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_mst_types_test.c
when running the dm_mst_test_dsc_link_* tests?

Later in dm_mst_test_alloc_dsc_link_ctx(), port->malloc_kref is
initialized to 1:

drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_mst_types_test.c:
dm_mst_test_alloc_dsc_link_ctx() {
    ...
	/* The payload allocation takes a reference, so start above zero. */
	kref_init(&port->malloc_kref);
	port->mgr = &ctx->aconnector->mst_mgr;
	port->connector = &ctx->aconnector->base;
	port->full_pbn = 2000;
    ...
}

When pre_compute_mst_dsc_configs_for_state() runs
compute_mst_dsc_configs_for_link() -> drm_dp_atomic_find_time_slots(), it
adds a payload to ctx->mst_state->payloads and calls
drm_dp_mst_get_port_malloc(port), incrementing port->malloc_kref from 1 to
2.

On test exit, tearing down ctx->state calls drm_dp_mst_destroy_state() in
drivers/gpu/drm/display/drm_dp_mst_topology.c:

drivers/gpu/drm/display/drm_dp_mst_topology.c:drm_dp_mst_destroy_state() {
    ...
	list_for_each_entry_safe(pos, tmp, &mst_state->payloads, next) {
		/* We only keep references to ports with active payloads */
		if (!pos->delete)
			drm_dp_mst_put_port_malloc(pos->port);
		kfree(pos);
	}
    ...
}

That drops only the payload reference, decrementing port->malloc_kref from
2 back to 1. Because ctx->aconnector is initialized with
dm_mst_test_dsc_link_conn_funcs rather than dm_dp_mst_connector_destroy()
and port is not attached to any branch in ctx->aconnector->mst_mgr,
nothing ever drops the initial malloc_kref reference (and if it did reach
0, drm_dp_free_mst_port() would call drm_dp_mst_put_mstb_malloc() on a
null port->parent).

The original use-after-free happened because kunit_kzalloc() for port was
registered after drm_kunit_helper_atomic_state_alloc(), so KUnit's
last-in, first-out cleanup freed port before ctx->state teardown ran
drm_dp_mst_destroy_state(). Could port be allocated with
drmm_kzalloc(ctx->drm, ...) or allocated with kunit_kzalloc() prior to
drm_kunit_helper_atomic_state_alloc() instead?

> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_wb_test.c b/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_wb_test.c
> index 4bc033fdfa97a..f3b64a66cf28d 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_wb_test.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_wb_test.c
> @@ -500,7 +500,7 @@ static void dm_test_wb_connector_init_success(struct kunit *test)
>  	dc->links[0] = link;
>  	dm->dc = dc;
>  
> -	wbcon = kunit_kzalloc(test, sizeof(*wbcon), GFP_KERNEL);
> +	wbcon = kzalloc_obj(*wbcon);
>  	KUNIT_ASSERT_NOT_NULL(test, wbcon);
>  
>  	ret = amdgpu_dm_wb_connector_init(dm, wbcon, 0);

[Severity: Medium]
Won't changing wbcon to kzalloc_obj(*wbcon) in
dm_test_wb_connector_init_success() in
drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_wb_test.c leak wbcon
on every run of this test?

In drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_wb.c,
amdgpu_dm_wb_connector_init() initializes wbcon->base through
drm_writeback_connector_init() using amdgpu_dm_wb_connector_funcs:

drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_wb.c:
static const struct drm_connector_funcs amdgpu_dm_wb_connector_funcs = {
	.fill_modes = drm_helper_probe_single_connector_modes,
	.destroy = drm_connector_cleanup,
	.reset = amdgpu_dm_connector_funcs_reset,
	.atomic_duplicate_state = amdgpu_dm_connector_atomic_duplicate_state,
	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
};

Unlike regular amdgpu_dm_connector_funcs (which uses
amdgpu_dm_connector_destroy() to kfree() the connector),
amdgpu_dm_wb_connector_funcs sets .destroy to drm_connector_cleanup(),
which in drivers/gpu/drm/drm_connector.c only cleans up internal connector
state without freeing the enclosing struct amdgpu_dm_wb_connector.

The original use-after-free occurred because wbcon was allocated with
kunit_kzalloc() after adev = dm_kunit_alloc_adev(test), causing KUnit to
free wbcon before drm_mode_config_cleanup(&adev->ddev) cleaned up the
writeback connector and encoder. Would allocating wbcon with
kunit_kzalloc() before dm_kunit_alloc_adev(test) (or via drmm_kzalloc())
fix the ordering issue without leaking wbcon?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261022173655.2318987-1-John.Harrison@Igalia.com?part=1

Re: [PATCH 3/3] drm/amd/display: Update colorop pipeline tests for new fixed matrix op

[John Harrison]

On 10/22/26 10:50, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] drm/amd/display: undeclared DRM_COLOROP_FIXED_MATRIX breaks KUnit test build
> --
>
> commit fa1c05a038fce65c050bce3e6d89515560ce7081
> Author: John Harrison <John.Harrison@Igalia.com>
>
> drm/amd/display: Update colorop pipeline tests for new fixed matrix op
>
> This commit updates the expected colorop pipeline arrays in the amdgpu_dm
> KUnit tests to begin with DRM_COLOROP_FIXED_MATRIX.
>
>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_colorop_test.c b/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_colorop_test.c
>> index 90cc5592ef5de..00f1d5b33b1f0 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_colorop_test.c
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_colorop_test.c
>> @@ -192,6 +192,7 @@ static void dm_expect_colorop_pipeline(struct kunit *test, struct drm_device *dr
>>   static void dm_test_initialize_default_pipeline(struct kunit *test)
>>   {
>>   	static const enum drm_colorop_type expected[] = {
>> +		DRM_COLOROP_FIXED_MATRIX,
>>   		DRM_COLOROP_1D_CURVE,	/* degam TF */
>>   		DRM_COLOROP_MULTIPLIER,
>>   		DRM_COLOROP_CTM_3X4,
>> @@ -257,6 +258,7 @@ static void dm_test_initialize_default_pipeline_caps(struct kunit *test,
>>   static void dm_test_initialize_default_pipeline_dpp_3d_lut(struct kunit *test)
>>   {
>>   	static const enum drm_colorop_type expected[] = {
>> +		DRM_COLOROP_FIXED_MATRIX,
>>   		DRM_COLOROP_1D_CURVE,
>>   		DRM_COLOROP_MULTIPLIER,
>>   		DRM_COLOROP_CTM_3X4,
>> @@ -278,6 +280,7 @@ static void dm_test_initialize_default_pipeline_dpp_3d_lut(struct kunit *test)
>>   static void dm_test_initialize_default_pipeline_mpc_preblend(struct kunit *test)
>>   {
>>   	static const enum drm_colorop_type expected[] = {
>> +		DRM_COLOROP_FIXED_MATRIX,
>>   		DRM_COLOROP_1D_CURVE,
>>   		DRM_COLOROP_MULTIPLIER,
>>   		DRM_COLOROP_CTM_3X4,
>> @@ -299,6 +302,7 @@ static void dm_test_initialize_default_pipeline_mpc_preblend(struct kunit *test)
>>   static void dm_test_initialize_default_pipeline_no_3d_lut(struct kunit *test)
>>   {
>>   	static const enum drm_colorop_type expected[] = {
>> +		DRM_COLOROP_FIXED_MATRIX,
>>   		DRM_COLOROP_1D_CURVE,
>>   		DRM_COLOROP_MULTIPLIER,
>>   		DRM_COLOROP_CTM_3X4,
> [Severity: Medium]
> Does this break the build when CONFIG_DRM_AMD_DC_KUNIT_TEST is enabled?
>
> This patch adds DRM_COLOROP_FIXED_MATRIX to the expected[] arrays in
> dm_test_initialize_default_pipeline(),
> dm_test_initialize_default_pipeline_dpp_3d_lut(),
> dm_test_initialize_default_pipeline_mpc_preblend(), and
> dm_test_initialize_default_pipeline_no_3d_lut() in
> drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_colorop_test.c.
>
> The commit references Fixes: 6d103ad11583 ("drm/amd/display: Add fixed matrix
> colorop to color pipeline"), which in turn depends on f6c58e4ae87c
> ("drm/colorop: Add DRM_COLOROP_FIXED_MATRIX"), but neither commit is present
> in this branch.
>
> In include/uapi/drm/drm_mode.h, enum drm_colorop_type does not define
> DRM_COLOROP_FIXED_MATRIX:
>
>      enum drm_colorop_type {
>              DRM_COLOROP_1D_CURVE,
>              DRM_COLOROP_1D_LUT,
>              DRM_COLOROP_CTM_3X4,
>              DRM_COLOROP_MULTIPLIER,
>              DRM_COLOROP_3D_LUT,
>      };
>
> In addition, amdgpu_dm_build_default_pipeline() in
> drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_colorop.c still starts the
> default pipeline with DRM_COLOROP_1D_CURVE:
>
>      /* 1D curve - DEGAM TF */
>      ops[i] = colorop_kzalloc_obj();
>      if (!ops[i]) {
>              ret = -ENOMEM;
>              goto cleanup;
>      }
>
>      ret = colorop_curve_1d_init(dev, ops[i], plane, &dm_colorop_funcs,
>                                  amdgpu_dm_supported_degam_tfs,
>                                  DRM_COLOROP_FLAG_ALLOW_BYPASS);
>
> Should this patch only be applied to a branch that already contains commits
> f6c58e4ae87c and 6d103ad11583?
>
What branch is Sashiko using? The web page report says it is based on 
77f548b127c6de34ce271fa09ba270cb1ef9f2da which is a hash that does not 
exist in any tree I have locally. My patches are based on drm-tip, which 
very definitely contains the FIXED_MATRIX support (merged August 27th 
according to the patch itself).

John.