The drm_test_dp_mst_calc_pbn_mode is based on a loop that executes tests
for a couple of test cases. This could be better represented by
parameterized tests, provided by KUnit.
So, convert the drm_test_dp_mst_calc_pbn_mode into parameterized tests.
Signed-off-by: Maíra Canal <[email protected]>
Reviewed-by: Michał Winiarski <[email protected]>
---
v1 -> v2: https://lore.kernel.org/dri-devel/[email protected]/T/#m056610a23a63109484afeafefb5846178c4d36b2
- Add Michał's Reviewed-by tag.
---
.../gpu/drm/tests/drm_dp_mst_helper_test.c | 77 +++++++++++++------
1 file changed, 53 insertions(+), 24 deletions(-)
diff --git a/drivers/gpu/drm/tests/drm_dp_mst_helper_test.c b/drivers/gpu/drm/tests/drm_dp_mst_helper_test.c
index 65c9d225b558..12f41881db6b 100644
--- a/drivers/gpu/drm/tests/drm_dp_mst_helper_test.c
+++ b/drivers/gpu/drm/tests/drm_dp_mst_helper_test.c
@@ -16,33 +16,62 @@
#include "../display/drm_dp_mst_topology_internal.h"
+struct drm_dp_mst_calc_pbn_mode_test {
+ const int clock;
+ const int bpp;
+ const bool dsc;
+ const int expected;
+};
+
+static const struct drm_dp_mst_calc_pbn_mode_test drm_dp_mst_calc_pbn_mode_cases[] = {
+ {
+ .clock = 154000,
+ .bpp = 30,
+ .dsc = false,
+ .expected = 689
+ },
+ {
+ .clock = 234000,
+ .bpp = 30,
+ .dsc = false,
+ .expected = 1047
+ },
+ {
+ .clock = 297000,
+ .bpp = 24,
+ .dsc = false,
+ .expected = 1063
+ },
+ {
+ .clock = 332880,
+ .bpp = 24,
+ .dsc = true,
+ .expected = 50
+ },
+ {
+ .clock = 324540,
+ .bpp = 24,
+ .dsc = true,
+ .expected = 49
+ },
+};
+
static void drm_test_dp_mst_calc_pbn_mode(struct kunit *test)
{
- int pbn, i;
- const struct {
- int rate;
- int bpp;
- int expected;
- bool dsc;
- } test_params[] = {
- { 154000, 30, 689, false },
- { 234000, 30, 1047, false },
- { 297000, 24, 1063, false },
- { 332880, 24, 50, true },
- { 324540, 24, 49, true },
- };
-
- for (i = 0; i < ARRAY_SIZE(test_params); i++) {
- pbn = drm_dp_calc_pbn_mode(test_params[i].rate,
- test_params[i].bpp,
- test_params[i].dsc);
- KUNIT_EXPECT_EQ_MSG(test, pbn, test_params[i].expected,
- "Expected PBN %d for clock %d bpp %d, got %d\n",
- test_params[i].expected, test_params[i].rate,
- test_params[i].bpp, pbn);
- }
+ const struct drm_dp_mst_calc_pbn_mode_test *params = test->param_value;
+
+ KUNIT_EXPECT_EQ(test, drm_dp_calc_pbn_mode(params->clock, params->bpp, params->dsc),
+ params->expected);
}
+static void dp_mst_calc_pbn_mode_desc(const struct drm_dp_mst_calc_pbn_mode_test *t, char *desc)
+{
+ sprintf(desc, "Clock %d BPP %d DSC %s", t->clock, t->bpp, t->dsc ? "enabled" : "disabled");
+}
+
+KUNIT_ARRAY_PARAM(drm_dp_mst_calc_pbn_mode, drm_dp_mst_calc_pbn_mode_cases,
+ dp_mst_calc_pbn_mode_desc);
+
static bool
sideband_msg_req_equal(const struct drm_dp_sideband_msg_req_body *in,
const struct drm_dp_sideband_msg_req_body *out)
@@ -271,7 +300,7 @@ static void drm_test_dp_mst_sideband_msg_req_decode(struct kunit *test)
}
static struct kunit_case drm_dp_mst_helper_tests[] = {
- KUNIT_CASE(drm_test_dp_mst_calc_pbn_mode),
+ KUNIT_CASE_PARAM(drm_test_dp_mst_calc_pbn_mode, drm_dp_mst_calc_pbn_mode_gen_params),
KUNIT_CASE(drm_test_dp_mst_sideband_msg_req_decode),
{ }
};
--
2.37.3
The drm_test_dp_mst_sideband_msg_req_decode repeats the same test
structure with different parameters. This could be better represented
by parameterized tests, provided by KUnit.
In order to convert the tests to parameterized tests, the test case for
the client ID was changed: instead of using get_random_bytes to generate
the client ID, the client ID is now hardcoded in the test case.
So, convert drm_test_dp_mst_sideband_msg_req_decode into parameterized
tests and make the tests' allocations and prints completely managed by KUnit.
Signed-off-by: Maíra Canal <[email protected]>
---
v1 -> v2: https://lore.kernel.org/dri-devel/[email protected]/T/#m056610a23a63109484afeafefb5846178c4d36b2
- Mention on the commit message the change on the test case for the client ID (Michał Winiarski).
---
.../gpu/drm/tests/drm_dp_mst_helper_test.c | 370 ++++++++++++------
1 file changed, 243 insertions(+), 127 deletions(-)
diff --git a/drivers/gpu/drm/tests/drm_dp_mst_helper_test.c b/drivers/gpu/drm/tests/drm_dp_mst_helper_test.c
index 12f41881db6b..545beea33e8c 100644
--- a/drivers/gpu/drm/tests/drm_dp_mst_helper_test.c
+++ b/drivers/gpu/drm/tests/drm_dp_mst_helper_test.c
@@ -5,12 +5,8 @@
* Copyright (c) 2022 Maíra Canal <[email protected]>
*/
-#define PREFIX_STR "[drm_dp_mst_helper]"
-
#include <kunit/test.h>
-#include <linux/random.h>
-
#include <drm/display/drm_dp_mst_helper.h>
#include <drm/drm_print.h>
@@ -72,6 +68,217 @@ static void dp_mst_calc_pbn_mode_desc(const struct drm_dp_mst_calc_pbn_mode_test
KUNIT_ARRAY_PARAM(drm_dp_mst_calc_pbn_mode, drm_dp_mst_calc_pbn_mode_cases,
dp_mst_calc_pbn_mode_desc);
+static u8 data[] = { 0xff, 0x00, 0xdd };
+
+struct drm_dp_mst_sideband_msg_req_test {
+ const char *desc;
+ const struct drm_dp_sideband_msg_req_body in;
+};
+
+static const struct drm_dp_mst_sideband_msg_req_test drm_dp_mst_sideband_msg_req_cases[] = {
+ {
+ .desc = "DP_ENUM_PATH_RESOURCES with port number",
+ .in = {
+ .req_type = DP_ENUM_PATH_RESOURCES,
+ .u.port_num.port_number = 5,
+ },
+ },
+ {
+ .desc = "DP_POWER_UP_PHY with port number",
+ .in = {
+ .req_type = DP_POWER_UP_PHY,
+ .u.port_num.port_number = 5,
+ },
+ },
+ {
+ .desc = "DP_POWER_DOWN_PHY with port number",
+ .in = {
+ .req_type = DP_POWER_DOWN_PHY,
+ .u.port_num.port_number = 5,
+ },
+ },
+ {
+ .desc = "DP_ALLOCATE_PAYLOAD with SDP stream sinks",
+ .in = {
+ .req_type = DP_ALLOCATE_PAYLOAD,
+ .u.allocate_payload.number_sdp_streams = 3,
+ .u.allocate_payload.sdp_stream_sink = { 1, 2, 3 },
+ },
+ },
+ {
+ .desc = "DP_ALLOCATE_PAYLOAD with port number",
+ .in = {
+ .req_type = DP_ALLOCATE_PAYLOAD,
+ .u.allocate_payload.port_number = 0xf,
+ },
+ },
+ {
+ .desc = "DP_ALLOCATE_PAYLOAD with VCPI",
+ .in = {
+ .req_type = DP_ALLOCATE_PAYLOAD,
+ .u.allocate_payload.vcpi = 0x7f,
+ },
+ },
+ {
+ .desc = "DP_ALLOCATE_PAYLOAD with PBN",
+ .in = {
+ .req_type = DP_ALLOCATE_PAYLOAD,
+ .u.allocate_payload.pbn = U16_MAX,
+ },
+ },
+ {
+ .desc = "DP_QUERY_PAYLOAD with port number",
+ .in = {
+ .req_type = DP_QUERY_PAYLOAD,
+ .u.query_payload.port_number = 0xf,
+ },
+ },
+ {
+ .desc = "DP_QUERY_PAYLOAD with VCPI",
+ .in = {
+ .req_type = DP_QUERY_PAYLOAD,
+ .u.query_payload.vcpi = 0x7f,
+ },
+ },
+ {
+ .desc = "DP_REMOTE_DPCD_READ with port number",
+ .in = {
+ .req_type = DP_REMOTE_DPCD_READ,
+ .u.dpcd_read.port_number = 0xf,
+ },
+ },
+ {
+ .desc = "DP_REMOTE_DPCD_READ with DPCD address",
+ .in = {
+ .req_type = DP_REMOTE_DPCD_READ,
+ .u.dpcd_read.dpcd_address = 0xfedcb,
+ },
+ },
+ {
+ .desc = "DP_REMOTE_DPCD_READ with max number of bytes",
+ .in = {
+ .req_type = DP_REMOTE_DPCD_READ,
+ .u.dpcd_read.num_bytes = U8_MAX,
+ },
+ },
+ {
+ .desc = "DP_REMOTE_DPCD_WRITE with port number",
+ .in = {
+ .req_type = DP_REMOTE_DPCD_WRITE,
+ .u.dpcd_write.port_number = 0xf,
+ },
+ },
+ {
+ .desc = "DP_REMOTE_DPCD_WRITE with DPCD address",
+ .in = {
+ .req_type = DP_REMOTE_DPCD_WRITE,
+ .u.dpcd_write.dpcd_address = 0xfedcb,
+ },
+ },
+ {
+ .desc = "DP_REMOTE_DPCD_WRITE with data array",
+ .in = {
+ .req_type = DP_REMOTE_DPCD_WRITE,
+ .u.dpcd_write.num_bytes = ARRAY_SIZE(data),
+ .u.dpcd_write.bytes = data,
+ },
+ },
+ {
+ .desc = "DP_REMOTE_I2C_READ with port number",
+ .in = {
+ .req_type = DP_REMOTE_I2C_READ,
+ .u.i2c_read.port_number = 0xf,
+ },
+ },
+ {
+ .desc = "DP_REMOTE_I2C_READ with I2C device ID",
+ .in = {
+ .req_type = DP_REMOTE_I2C_READ,
+ .u.i2c_read.read_i2c_device_id = 0x7f,
+ },
+ },
+ {
+ .desc = "DP_REMOTE_I2C_READ with transactions array",
+ .in = {
+ .req_type = DP_REMOTE_I2C_READ,
+ .u.i2c_read.num_transactions = 3,
+ .u.i2c_read.num_bytes_read = ARRAY_SIZE(data) * 3,
+ .u.i2c_read.transactions = {
+ { .bytes = data, .num_bytes = ARRAY_SIZE(data), .i2c_dev_id = 0x7f,
+ .i2c_transaction_delay = 0xf, },
+ { .bytes = data, .num_bytes = ARRAY_SIZE(data), .i2c_dev_id = 0x7e,
+ .i2c_transaction_delay = 0xe, },
+ { .bytes = data, .num_bytes = ARRAY_SIZE(data), .i2c_dev_id = 0x7d,
+ .i2c_transaction_delay = 0xd, },
+ },
+ },
+ },
+ {
+ .desc = "DP_REMOTE_I2C_WRITE with port number",
+ .in = {
+ .req_type = DP_REMOTE_I2C_WRITE,
+ .u.i2c_write.port_number = 0xf,
+ },
+ },
+ {
+ .desc = "DP_REMOTE_I2C_WRITE with I2C device ID",
+ .in = {
+ .req_type = DP_REMOTE_I2C_WRITE,
+ .u.i2c_write.write_i2c_device_id = 0x7f,
+ },
+ },
+ {
+ .desc = "DP_REMOTE_I2C_WRITE with data array",
+ .in = {
+ .req_type = DP_REMOTE_I2C_WRITE,
+ .u.i2c_write.num_bytes = ARRAY_SIZE(data),
+ .u.i2c_write.bytes = data,
+ },
+ },
+ {
+ .desc = "DP_QUERY_STREAM_ENC_STATUS with stream ID",
+ .in = {
+ .req_type = DP_QUERY_STREAM_ENC_STATUS,
+ .u.enc_status.stream_id = 1,
+ },
+ },
+ {
+ .desc = "DP_QUERY_STREAM_ENC_STATUS with client ID",
+ .in = {
+ .req_type = DP_QUERY_STREAM_ENC_STATUS,
+ .u.enc_status.client_id = { 0x4f, 0x7f, 0xb4, 0x00, 0x8c, 0x0d, 0x67 },
+ },
+ },
+ {
+ .desc = "DP_QUERY_STREAM_ENC_STATUS with stream event",
+ .in = {
+ .req_type = DP_QUERY_STREAM_ENC_STATUS,
+ .u.enc_status.stream_event = 3,
+ },
+ },
+ {
+ .desc = "DP_QUERY_STREAM_ENC_STATUS with valid stream event",
+ .in = {
+ .req_type = DP_QUERY_STREAM_ENC_STATUS,
+ .u.enc_status.valid_stream_event = 0,
+ },
+ },
+ {
+ .desc = "DP_QUERY_STREAM_ENC_STATUS with stream behavior",
+ .in = {
+ .req_type = DP_QUERY_STREAM_ENC_STATUS,
+ .u.enc_status.stream_behavior = 3,
+ },
+ },
+ {
+ .desc = "DP_QUERY_STREAM_ENC_STATUS with a valid stream behavior",
+ .in = {
+ .req_type = DP_QUERY_STREAM_ENC_STATUS,
+ .u.enc_status.valid_stream_behavior = 1,
+ }
+ },
+};
+
static bool
sideband_msg_req_equal(const struct drm_dp_sideband_msg_req_body *in,
const struct drm_dp_sideband_msg_req_body *out)
@@ -147,41 +354,41 @@ sideband_msg_req_equal(const struct drm_dp_sideband_msg_req_body *in,
return true;
}
-static bool
-sideband_msg_req_encode_decode(struct drm_dp_sideband_msg_req_body *in)
+static void drm_test_dp_mst_msg_printf(struct drm_printer *p, struct va_format *vaf)
+{
+ struct kunit *test = p->arg;
+
+ kunit_err(test, "%pV", vaf);
+}
+
+static void drm_test_dp_mst_sideband_msg_req_decode(struct kunit *test)
{
+ const struct drm_dp_mst_sideband_msg_req_test *params = test->param_value;
+ const struct drm_dp_sideband_msg_req_body *in = ¶ms->in;
struct drm_dp_sideband_msg_req_body *out;
- struct drm_printer p = drm_err_printer(PREFIX_STR);
struct drm_dp_sideband_msg_tx *txmsg;
- int i, ret;
- bool result = true;
+ struct drm_printer p = {
+ .printfn = drm_test_dp_mst_msg_printf,
+ .arg = test
+ };
+ int i;
- out = kzalloc(sizeof(*out), GFP_KERNEL);
- if (!out)
- return false;
+ out = kunit_kzalloc(test, sizeof(*out), GFP_KERNEL);
+ KUNIT_ASSERT_NOT_NULL(test, out);
- txmsg = kzalloc(sizeof(*txmsg), GFP_KERNEL);
- if (!txmsg) {
- kfree(out);
- return false;
- }
+ txmsg = kunit_kzalloc(test, sizeof(*txmsg), GFP_KERNEL);
+ KUNIT_ASSERT_NOT_NULL(test, txmsg);
drm_dp_encode_sideband_req(in, txmsg);
- ret = drm_dp_decode_sideband_req(txmsg, out);
- if (ret < 0) {
- drm_printf(&p, "Failed to decode sideband request: %d\n",
- ret);
- result = false;
- goto out;
- }
+ KUNIT_EXPECT_GE_MSG(test, drm_dp_decode_sideband_req(txmsg, out), 0,
+ "Failed to decode sideband request");
if (!sideband_msg_req_equal(in, out)) {
- drm_printf(&p, "Encode/decode failed, expected:\n");
+ KUNIT_FAIL(test, "Encode/decode failed");
+ kunit_err(test, "Expected:");
drm_dp_dump_sideband_msg_req_body(in, 1, &p);
- drm_printf(&p, "Got:\n");
+ kunit_err(test, "Got:");
drm_dp_dump_sideband_msg_req_body(out, 1, &p);
- result = false;
- goto out;
}
switch (in->req_type) {
@@ -196,112 +403,21 @@ sideband_msg_req_encode_decode(struct drm_dp_sideband_msg_req_body *in)
kfree(out->u.i2c_write.bytes);
break;
}
-
- /* Clear everything but the req_type for the input */
- memset(&in->u, 0, sizeof(in->u));
-
-out:
- kfree(out);
- kfree(txmsg);
- return result;
}
-static void drm_test_dp_mst_sideband_msg_req_decode(struct kunit *test)
+static void
+drm_dp_mst_sideband_msg_req_desc(const struct drm_dp_mst_sideband_msg_req_test *t, char *desc)
{
- struct drm_dp_sideband_msg_req_body in = { 0 };
- u8 data[] = { 0xff, 0x0, 0xdd };
- int i;
-
- in.req_type = DP_ENUM_PATH_RESOURCES;
- in.u.port_num.port_number = 5;
- KUNIT_EXPECT_TRUE(test, sideband_msg_req_encode_decode(&in));
-
- in.req_type = DP_POWER_UP_PHY;
- in.u.port_num.port_number = 5;
- KUNIT_EXPECT_TRUE(test, sideband_msg_req_encode_decode(&in));
-
- in.req_type = DP_POWER_DOWN_PHY;
- in.u.port_num.port_number = 5;
- KUNIT_EXPECT_TRUE(test, sideband_msg_req_encode_decode(&in));
-
- in.req_type = DP_ALLOCATE_PAYLOAD;
- in.u.allocate_payload.number_sdp_streams = 3;
- for (i = 0; i < in.u.allocate_payload.number_sdp_streams; i++)
- in.u.allocate_payload.sdp_stream_sink[i] = i + 1;
- KUNIT_EXPECT_TRUE(test, sideband_msg_req_encode_decode(&in));
- in.u.allocate_payload.port_number = 0xf;
- KUNIT_EXPECT_TRUE(test, sideband_msg_req_encode_decode(&in));
- in.u.allocate_payload.vcpi = 0x7f;
- KUNIT_EXPECT_TRUE(test, sideband_msg_req_encode_decode(&in));
- in.u.allocate_payload.pbn = U16_MAX;
- KUNIT_EXPECT_TRUE(test, sideband_msg_req_encode_decode(&in));
-
- in.req_type = DP_QUERY_PAYLOAD;
- in.u.query_payload.port_number = 0xf;
- KUNIT_EXPECT_TRUE(test, sideband_msg_req_encode_decode(&in));
- in.u.query_payload.vcpi = 0x7f;
- KUNIT_EXPECT_TRUE(test, sideband_msg_req_encode_decode(&in));
-
- in.req_type = DP_REMOTE_DPCD_READ;
- in.u.dpcd_read.port_number = 0xf;
- KUNIT_EXPECT_TRUE(test, sideband_msg_req_encode_decode(&in));
- in.u.dpcd_read.dpcd_address = 0xfedcb;
- KUNIT_EXPECT_TRUE(test, sideband_msg_req_encode_decode(&in));
- in.u.dpcd_read.num_bytes = U8_MAX;
- KUNIT_EXPECT_TRUE(test, sideband_msg_req_encode_decode(&in));
-
- in.req_type = DP_REMOTE_DPCD_WRITE;
- in.u.dpcd_write.port_number = 0xf;
- KUNIT_EXPECT_TRUE(test, sideband_msg_req_encode_decode(&in));
- in.u.dpcd_write.dpcd_address = 0xfedcb;
- KUNIT_EXPECT_TRUE(test, sideband_msg_req_encode_decode(&in));
- in.u.dpcd_write.num_bytes = ARRAY_SIZE(data);
- in.u.dpcd_write.bytes = data;
- KUNIT_EXPECT_TRUE(test, sideband_msg_req_encode_decode(&in));
-
- in.req_type = DP_REMOTE_I2C_READ;
- in.u.i2c_read.port_number = 0xf;
- KUNIT_EXPECT_TRUE(test, sideband_msg_req_encode_decode(&in));
- in.u.i2c_read.read_i2c_device_id = 0x7f;
- KUNIT_EXPECT_TRUE(test, sideband_msg_req_encode_decode(&in));
- in.u.i2c_read.num_transactions = 3;
- in.u.i2c_read.num_bytes_read = ARRAY_SIZE(data) * 3;
- for (i = 0; i < in.u.i2c_read.num_transactions; i++) {
- in.u.i2c_read.transactions[i].bytes = data;
- in.u.i2c_read.transactions[i].num_bytes = ARRAY_SIZE(data);
- in.u.i2c_read.transactions[i].i2c_dev_id = 0x7f & ~i;
- in.u.i2c_read.transactions[i].i2c_transaction_delay = 0xf & ~i;
- }
- KUNIT_EXPECT_TRUE(test, sideband_msg_req_encode_decode(&in));
-
- in.req_type = DP_REMOTE_I2C_WRITE;
- in.u.i2c_write.port_number = 0xf;
- KUNIT_EXPECT_TRUE(test, sideband_msg_req_encode_decode(&in));
- in.u.i2c_write.write_i2c_device_id = 0x7f;
- KUNIT_EXPECT_TRUE(test, sideband_msg_req_encode_decode(&in));
- in.u.i2c_write.num_bytes = ARRAY_SIZE(data);
- in.u.i2c_write.bytes = data;
- KUNIT_EXPECT_TRUE(test, sideband_msg_req_encode_decode(&in));
-
- in.req_type = DP_QUERY_STREAM_ENC_STATUS;
- in.u.enc_status.stream_id = 1;
- KUNIT_EXPECT_TRUE(test, sideband_msg_req_encode_decode(&in));
- get_random_bytes(in.u.enc_status.client_id,
- sizeof(in.u.enc_status.client_id));
- KUNIT_EXPECT_TRUE(test, sideband_msg_req_encode_decode(&in));
- in.u.enc_status.stream_event = 3;
- KUNIT_EXPECT_TRUE(test, sideband_msg_req_encode_decode(&in));
- in.u.enc_status.valid_stream_event = 0;
- KUNIT_EXPECT_TRUE(test, sideband_msg_req_encode_decode(&in));
- in.u.enc_status.stream_behavior = 3;
- KUNIT_EXPECT_TRUE(test, sideband_msg_req_encode_decode(&in));
- in.u.enc_status.valid_stream_behavior = 1;
- KUNIT_EXPECT_TRUE(test, sideband_msg_req_encode_decode(&in));
+ strcpy(desc, t->desc);
}
+KUNIT_ARRAY_PARAM(drm_dp_mst_sideband_msg_req, drm_dp_mst_sideband_msg_req_cases,
+ drm_dp_mst_sideband_msg_req_desc);
+
static struct kunit_case drm_dp_mst_helper_tests[] = {
KUNIT_CASE_PARAM(drm_test_dp_mst_calc_pbn_mode, drm_dp_mst_calc_pbn_mode_gen_params),
- KUNIT_CASE(drm_test_dp_mst_sideband_msg_req_decode),
+ KUNIT_CASE_PARAM(drm_test_dp_mst_sideband_msg_req_decode,
+ drm_dp_mst_sideband_msg_req_gen_params),
{ }
};
--
2.37.3
On Tue, Sep 27, 2022 at 07:12:06PM -0300, Maíra Canal wrote:
> The drm_test_dp_mst_sideband_msg_req_decode repeats the same test
> structure with different parameters. This could be better represented
> by parameterized tests, provided by KUnit.
>
> In order to convert the tests to parameterized tests, the test case for
> the client ID was changed: instead of using get_random_bytes to generate
> the client ID, the client ID is now hardcoded in the test case.
Generally "random" usage is not incompatible with parameterized tests, we can
create parameterized tests that use random data.
The idea is to pass a function that generates the actual param (where we have a
pointer to function as one of the members in "params" struct).
For example, see "random_dp_query_enc_client_id" usage here:
https://lore.kernel.org/dri-devel/[email protected]/
In this case, we just compare data going in with data going out (and the data
itself is not transformed in any way), so it doesn't really matter for coverage
and we can hardcode.
-Michał
> So, convert drm_test_dp_mst_sideband_msg_req_decode into parameterized
> tests and make the tests' allocations and prints completely managed by KUnit.
>
> Signed-off-by: Maíra Canal <[email protected]>
> ---
> v1 -> v2: https://lore.kernel.org/dri-devel/[email protected]/T/#m056610a23a63109484afeafefb5846178c4d36b2
> - Mention on the commit message the change on the test case for the client ID (Michał Winiarski).
> ---
> .../gpu/drm/tests/drm_dp_mst_helper_test.c | 370 ++++++++++++------
> 1 file changed, 243 insertions(+), 127 deletions(-)
On Fri, Sep 30, 2022 at 6:33 AM Michał Winiarski
<[email protected]> wrote:
>
> On Tue, Sep 27, 2022 at 07:12:06PM -0300, Maíra Canal wrote:
> > The drm_test_dp_mst_sideband_msg_req_decode repeats the same test
> > structure with different parameters. This could be better represented
> > by parameterized tests, provided by KUnit.
> >
> > In order to convert the tests to parameterized tests, the test case for
> > the client ID was changed: instead of using get_random_bytes to generate
> > the client ID, the client ID is now hardcoded in the test case.
>
> Generally "random" usage is not incompatible with parameterized tests, we can
> create parameterized tests that use random data.
> The idea is to pass a function that generates the actual param (where we have a
> pointer to function as one of the members in "params" struct).
>
> For example, see "random_dp_query_enc_client_id" usage here:
> https://lore.kernel.org/dri-devel/[email protected]/
>
> In this case, we just compare data going in with data going out (and the data
> itself is not transformed in any way), so it doesn't really matter for coverage
> and we can hardcode.
>
> -Michał
FWIW, while the uses of randomness in DRM tests so far haven't
concerned me much, I think we'll eventually want to have some way of
ensuring the inputs to tests are deterministic.
My thoughts are that (at some point) we'll add a kunit_random()
function or similar, which will use a pseudorandom number generator
which can be set to a deterministic seed before each test case. That
way, there'd be a way to reproduce an error easily if it occurred. (Of
course, there'd be a way of setting different or random seeds to
preserve the extra coverage you'd otherwise get.)
I don't think this is something worth holding up or changing existing
tests at the moment, but having tests behave deterministically is
definitely desirable, so +1 to avoiding get_random_bytes() if it's not
giving you any real benefit.
We've also had a few requests in the past for being able to pass in a
custom set of parameters from userspace, which opens up some other
interesting possibilities, though it's not a priority at the moment.
Cheers,
-- David
On Fri, Sep 30, 2022 at 02:50:43PM +0800, David Gow wrote:
> On Fri, Sep 30, 2022 at 6:33 AM Michał Winiarski
> <[email protected]> wrote:
> >
> > On Tue, Sep 27, 2022 at 07:12:06PM -0300, Maíra Canal wrote:
> > > The drm_test_dp_mst_sideband_msg_req_decode repeats the same test
> > > structure with different parameters. This could be better represented
> > > by parameterized tests, provided by KUnit.
> > >
> > > In order to convert the tests to parameterized tests, the test case for
> > > the client ID was changed: instead of using get_random_bytes to generate
> > > the client ID, the client ID is now hardcoded in the test case.
> >
> > Generally "random" usage is not incompatible with parameterized tests, we can
> > create parameterized tests that use random data.
> > The idea is to pass a function that generates the actual param (where we have a
> > pointer to function as one of the members in "params" struct).
> >
> > For example, see "random_dp_query_enc_client_id" usage here:
> > https://lore.kernel.org/dri-devel/[email protected]/
> >
> > In this case, we just compare data going in with data going out (and the data
> > itself is not transformed in any way), so it doesn't really matter for coverage
> > and we can hardcode.
> >
> > -Michał
>
> FWIW, while the uses of randomness in DRM tests so far haven't
> concerned me much, I think we'll eventually want to have some way of
> ensuring the inputs to tests are deterministic.
>
> My thoughts are that (at some point) we'll add a kunit_random()
> function or similar, which will use a pseudorandom number generator
> which can be set to a deterministic seed before each test case. That
> way, there'd be a way to reproduce an error easily if it occurred. (Of
> course, there'd be a way of setting different or random seeds to
> preserve the extra coverage you'd otherwise get.)
That's exactly what DRM tests do (well... most DRM tests, this one being the
exception, and those other tests also seem to have lost a printk with seed value
after being refactored into kunit).
See the usage of DRM_RND_STATE in drm_mm_test and drm_buddy_test.
Having kunit_random() would definitely be useful and let us remove bunch of
boilerplate from the tests, but it doesn't prevent using reproducible random
data in existing tests.
> I don't think this is something worth holding up or changing existing
> tests at the moment, but having tests behave deterministically is
> definitely desirable, so +1 to avoiding get_random_bytes() if it's not
> giving you any real benefit.
Yeah - all I was refering to in my previous message was the wording of the
commit message. We're just removing it because it is desirable in this
particular case, not because of the fact that the test is now parameterized and
that's somehow preventing get_random_bytes() usage.
-Michał
> We've also had a few requests in the past for being able to pass in a
> custom set of parameters from userspace, which opens up some other
> interesting possibilities, though it's not a priority at the moment.
>
> Cheers,
> -- David
On 9/30/22 06:11, Michał Winiarski wrote:
> On Fri, Sep 30, 2022 at 02:50:43PM +0800, David Gow wrote:
>> On Fri, Sep 30, 2022 at 6:33 AM Michał Winiarski
>> <[email protected]> wrote:
>>>
>>> On Tue, Sep 27, 2022 at 07:12:06PM -0300, Maíra Canal wrote:
>>>> The drm_test_dp_mst_sideband_msg_req_decode repeats the same test
>>>> structure with different parameters. This could be better represented
>>>> by parameterized tests, provided by KUnit.
>>>>
>>>> In order to convert the tests to parameterized tests, the test case for
>>>> the client ID was changed: instead of using get_random_bytes to generate
>>>> the client ID, the client ID is now hardcoded in the test case.
>>>
>>> Generally "random" usage is not incompatible with parameterized tests, we can
>>> create parameterized tests that use random data.
>>> The idea is to pass a function that generates the actual param (where we have a
>>> pointer to function as one of the members in "params" struct).
>>>
>>> For example, see "random_dp_query_enc_client_id" usage here:
>>> https://lore.kernel.org/dri-devel/[email protected]/
Although it is possible, I don't see the benefit in this case to use the
get_random_bytes instead of hardcoding. I believe it will only add more
boilerplate to the tests.
>>>
>>> In this case, we just compare data going in with data going out (and the data
>>> itself is not transformed in any way), so it doesn't really matter for coverage
>>> and we can hardcode.
>>>
>>> -Michał
>>
>> FWIW, while the uses of randomness in DRM tests so far haven't
>> concerned me much, I think we'll eventually want to have some way of
>> ensuring the inputs to tests are deterministic.
>>
>> My thoughts are that (at some point) we'll add a kunit_random()
>> function or similar, which will use a pseudorandom number generator
>> which can be set to a deterministic seed before each test case. That
>> way, there'd be a way to reproduce an error easily if it occurred. (Of
>> course, there'd be a way of setting different or random seeds to
>> preserve the extra coverage you'd otherwise get.)
>
> That's exactly what DRM tests do (well... most DRM tests, this one being the
> exception, and those other tests also seem to have lost a printk with seed value
> after being refactored into kunit).
I will take a look at those lost printk in drm_mm_test and
drm_buddy_test, as they are important to assure the reproducibility of
the tests.
> See the usage of DRM_RND_STATE in drm_mm_test and drm_buddy_test.
> Having kunit_random() would definitely be useful and let us remove bunch of
> boilerplate from the tests, but it doesn't prevent using reproducible random
> data in existing tests.
>
>> I don't think this is something worth holding up or changing existing
>> tests at the moment, but having tests behave deterministically is
>> definitely desirable, so +1 to avoiding get_random_bytes() if it's not
>> giving you any real benefit.
>
> Yeah - all I was refering to in my previous message was the wording of the
> commit message. We're just removing it because it is desirable in this
> particular case, not because of the fact that the test is now parameterized and
> that's somehow preventing get_random_bytes() usage.
I will send a v3 rewording the commit message to make it more clear.
Best Regards,
- Maíra Canal
>
> -Michał
>
>> We've also had a few requests in the past for being able to pass in a
>> custom set of parameters from userspace, which opens up some other
>> interesting possibilities, though it's not a priority at the moment.
>>
>> Cheers,
>> -- David
>
>