| Message ID | 20170530200140.25763-1-harry.wentland@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, May 30, 2017 at 04:01:40PM -0400, Harry Wentland wrote: > AMD GPUs can have 6 CRTCs. > > This requires us to allocate the combinations on the heap. > > Signed-off-by: Harry Wentland <harry.wentland@amd.com> I think just dynamically allocating stuff directly and dropping the #define would be even neater ... GetResources can tell us how much of each exists. -Daniel > --- > tests/kms_setmode.c | 25 +++++++++++++++---------- > 1 file changed, 15 insertions(+), 10 deletions(-) > > diff --git a/tests/kms_setmode.c b/tests/kms_setmode.c > index 430568a1c24e..847ad650d27f 100644 > --- a/tests/kms_setmode.c > +++ b/tests/kms_setmode.c > @@ -35,11 +35,13 @@ > #include "intel_bufmgr.h" > > #define MAX_CONNECTORS 10 > -#define MAX_CRTCS 3 > +#define MAX_CRTCS 6 > > /* max combinations with repetitions */ > +/* MAX_CONNECTORS ^ MAX_CRTCS */ > +/* TODO should really be MAX_CONNECTORS ^ MAX_CONNECTORS ??? */ > #define MAX_COMBINATION_COUNT \ > - (MAX_CONNECTORS * MAX_CONNECTORS * MAX_CONNECTORS) > + (MAX_CONNECTORS * MAX_CONNECTORS * MAX_CONNECTORS * MAX_CONNECTORS * MAX_CONNECTORS * MAX_CONNECTORS) > #define MAX_COMBINATION_ELEMS MAX_CRTCS > > static int drm_fd; > @@ -743,22 +745,25 @@ static void get_combinations(int n, int k, bool allow_repetitions, > static void test_combinations(const struct test_config *tconf, > int connector_count) > { > - struct combination_set connector_combs; > - struct combination_set crtc_combs; > + struct combination_set *connector_combs; > + struct combination_set *crtc_combs; > struct connector_config *cconfs; > int i; > > if (connector_count > 2 && (tconf->flags & TEST_STEALING)) > return; > > + connector_combs = malloc(sizeof(*connector_combs)); > + crtc_combs = malloc(sizeof(*crtc_combs)); > + > get_combinations(tconf->resources->count_connectors, connector_count, > - false, &connector_combs); > + false, connector_combs); > get_combinations(tconf->resources->count_crtcs, connector_count, > - true, &crtc_combs); > + true, crtc_combs); > > igt_info("Testing: %s %d connector combinations\n", tconf->name, > connector_count); > - for (i = 0; i < connector_combs.count; i++) { > + for (i = 0; i < connector_combs->count; i++) { > int *connector_idxs; > int ret; > int j; > @@ -766,14 +771,14 @@ static void test_combinations(const struct test_config *tconf, > cconfs = malloc(sizeof(*cconfs) * connector_count); > igt_assert(cconfs); > > - connector_idxs = &connector_combs.items[i].elems[0]; > + connector_idxs = &connector_combs->items[i].elems[0]; > ret = get_connectors(tconf->resources, connector_idxs, > connector_count, cconfs); > if (ret < 0) > goto free_cconfs; > > - for (j = 0; j < crtc_combs.count; j++) { > - int *crtc_idxs = &crtc_combs.items[j].elems[0]; > + for (j = 0; j < crtc_combs->count; j++) { > + int *crtc_idxs = &crtc_combs->items[j].elems[0]; > ret = assign_crtc_to_connectors(tconf, crtc_idxs, > connector_count, > cconfs); > -- > 2.11.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On 2017-05-31 05:37 AM, Daniel Vetter wrote: > On Tue, May 30, 2017 at 04:01:40PM -0400, Harry Wentland wrote: >> AMD GPUs can have 6 CRTCs. >> >> This requires us to allocate the combinations on the heap. >> >> Signed-off-by: Harry Wentland <harry.wentland@amd.com> > > I think just dynamically allocating stuff directly and dropping the > #define would be even neater ... GetResources can tell us how much of each > exists. > -Daniel > Agreed. I'll send out a v3 later. Harry >> --- >> tests/kms_setmode.c | 25 +++++++++++++++---------- >> 1 file changed, 15 insertions(+), 10 deletions(-) >> >> diff --git a/tests/kms_setmode.c b/tests/kms_setmode.c >> index 430568a1c24e..847ad650d27f 100644 >> --- a/tests/kms_setmode.c >> +++ b/tests/kms_setmode.c >> @@ -35,11 +35,13 @@ >> #include "intel_bufmgr.h" >> >> #define MAX_CONNECTORS 10 >> -#define MAX_CRTCS 3 >> +#define MAX_CRTCS 6 >> >> /* max combinations with repetitions */ >> +/* MAX_CONNECTORS ^ MAX_CRTCS */ >> +/* TODO should really be MAX_CONNECTORS ^ MAX_CONNECTORS ??? */ >> #define MAX_COMBINATION_COUNT \ >> - (MAX_CONNECTORS * MAX_CONNECTORS * MAX_CONNECTORS) >> + (MAX_CONNECTORS * MAX_CONNECTORS * MAX_CONNECTORS * MAX_CONNECTORS * MAX_CONNECTORS * MAX_CONNECTORS) >> #define MAX_COMBINATION_ELEMS MAX_CRTCS >> >> static int drm_fd; >> @@ -743,22 +745,25 @@ static void get_combinations(int n, int k, bool allow_repetitions, >> static void test_combinations(const struct test_config *tconf, >> int connector_count) >> { >> - struct combination_set connector_combs; >> - struct combination_set crtc_combs; >> + struct combination_set *connector_combs; >> + struct combination_set *crtc_combs; >> struct connector_config *cconfs; >> int i; >> >> if (connector_count > 2 && (tconf->flags & TEST_STEALING)) >> return; >> >> + connector_combs = malloc(sizeof(*connector_combs)); >> + crtc_combs = malloc(sizeof(*crtc_combs)); >> + >> get_combinations(tconf->resources->count_connectors, connector_count, >> - false, &connector_combs); >> + false, connector_combs); >> get_combinations(tconf->resources->count_crtcs, connector_count, >> - true, &crtc_combs); >> + true, crtc_combs); >> >> igt_info("Testing: %s %d connector combinations\n", tconf->name, >> connector_count); >> - for (i = 0; i < connector_combs.count; i++) { >> + for (i = 0; i < connector_combs->count; i++) { >> int *connector_idxs; >> int ret; >> int j; >> @@ -766,14 +771,14 @@ static void test_combinations(const struct test_config *tconf, >> cconfs = malloc(sizeof(*cconfs) * connector_count); >> igt_assert(cconfs); >> >> - connector_idxs = &connector_combs.items[i].elems[0]; >> + connector_idxs = &connector_combs->items[i].elems[0]; >> ret = get_connectors(tconf->resources, connector_idxs, >> connector_count, cconfs); >> if (ret < 0) >> goto free_cconfs; >> >> - for (j = 0; j < crtc_combs.count; j++) { >> - int *crtc_idxs = &crtc_combs.items[j].elems[0]; >> + for (j = 0; j < crtc_combs->count; j++) { >> + int *crtc_idxs = &crtc_combs->items[j].elems[0]; >> ret = assign_crtc_to_connectors(tconf, crtc_idxs, >> connector_count, >> cconfs); >> -- >> 2.11.0 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx >
On 2017-05-31 09:32 AM, Harry Wentland wrote: > On 2017-05-31 05:37 AM, Daniel Vetter wrote: >> On Tue, May 30, 2017 at 04:01:40PM -0400, Harry Wentland wrote: >>> AMD GPUs can have 6 CRTCs. >>> >>> This requires us to allocate the combinations on the heap. >>> >>> Signed-off-by: Harry Wentland <harry.wentland@amd.com> >> >> I think just dynamically allocating stuff directly and dropping the >> #define would be even neater ... GetResources can tell us how much of >> each >> exists. >> -Daniel >> > > Agreed. I'll send out a v3 later. > About to send v3 but this code is quite majorly wrong as-is. In particular I see these two issues: 1) In iterate_combinations we assign a stack object to our list of combinations which is then popped off the stack as soon as get_combinations returns. Later on in test_combinations we then try to access it with connector_idxs = &connector_combs.items[i].elems[0]; ... int *crtc_idxs = &crtc_combs.items[j].elems[0]; 2) This for loop in iterate_combinations will simply override comb->elems[depth] with new values: for (v = base; v < n; v++) { comb->elems[depth] = v; [...] } It looks like whoever wrote the code tried to do some k choose n algorithm to find all possible sets of crtcs and connectors but then only ended up picking one crtc and connector out of each list anyways (after the item popped from the stack). If I find time I might rewrite the test_combinations function with a simple nested for loop that tries all crtcs with all connectors one-to-one as this seems to be currently the intention of this function. Harry > Harry > >>> --- >>> tests/kms_setmode.c | 25 +++++++++++++++---------- >>> 1 file changed, 15 insertions(+), 10 deletions(-) >>> >>> diff --git a/tests/kms_setmode.c b/tests/kms_setmode.c >>> index 430568a1c24e..847ad650d27f 100644 >>> --- a/tests/kms_setmode.c >>> +++ b/tests/kms_setmode.c >>> @@ -35,11 +35,13 @@ >>> #include "intel_bufmgr.h" >>> #define MAX_CONNECTORS 10 >>> -#define MAX_CRTCS 3 >>> +#define MAX_CRTCS 6 >>> /* max combinations with repetitions */ >>> +/* MAX_CONNECTORS ^ MAX_CRTCS */ >>> +/* TODO should really be MAX_CONNECTORS ^ MAX_CONNECTORS ??? */ >>> #define MAX_COMBINATION_COUNT \ >>> - (MAX_CONNECTORS * MAX_CONNECTORS * MAX_CONNECTORS) >>> + (MAX_CONNECTORS * MAX_CONNECTORS * MAX_CONNECTORS * >>> MAX_CONNECTORS * MAX_CONNECTORS * MAX_CONNECTORS) >>> #define MAX_COMBINATION_ELEMS MAX_CRTCS >>> static int drm_fd; >>> @@ -743,22 +745,25 @@ static void get_combinations(int n, int k, bool >>> allow_repetitions, >>> static void test_combinations(const struct test_config *tconf, >>> int connector_count) >>> { >>> - struct combination_set connector_combs; >>> - struct combination_set crtc_combs; >>> + struct combination_set *connector_combs; >>> + struct combination_set *crtc_combs; >>> struct connector_config *cconfs; >>> int i; >>> if (connector_count > 2 && (tconf->flags & TEST_STEALING)) >>> return; >>> + connector_combs = malloc(sizeof(*connector_combs)); >>> + crtc_combs = malloc(sizeof(*crtc_combs)); >>> + >>> get_combinations(tconf->resources->count_connectors, >>> connector_count, >>> - false, &connector_combs); >>> + false, connector_combs); >>> get_combinations(tconf->resources->count_crtcs, connector_count, >>> - true, &crtc_combs); >>> + true, crtc_combs); >>> igt_info("Testing: %s %d connector combinations\n", tconf->name, >>> connector_count); >>> - for (i = 0; i < connector_combs.count; i++) { >>> + for (i = 0; i < connector_combs->count; i++) { >>> int *connector_idxs; >>> int ret; >>> int j; >>> @@ -766,14 +771,14 @@ static void test_combinations(const struct >>> test_config *tconf, >>> cconfs = malloc(sizeof(*cconfs) * connector_count); >>> igt_assert(cconfs); >>> - connector_idxs = &connector_combs.items[i].elems[0]; >>> + connector_idxs = &connector_combs->items[i].elems[0]; >>> ret = get_connectors(tconf->resources, connector_idxs, >>> connector_count, cconfs); >>> if (ret < 0) >>> goto free_cconfs; >>> - for (j = 0; j < crtc_combs.count; j++) { >>> - int *crtc_idxs = &crtc_combs.items[j].elems[0]; >>> + for (j = 0; j < crtc_combs->count; j++) { >>> + int *crtc_idxs = &crtc_combs->items[j].elems[0]; >>> ret = assign_crtc_to_connectors(tconf, crtc_idxs, >>> connector_count, >>> cconfs); >>> -- >>> 2.11.0 >>> >>> _______________________________________________ >>> Intel-gfx mailing list >>> Intel-gfx@lists.freedesktop.org >>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx >>
Please ignore what I wrote. Looks like I was a bit hasty in my judgement. The code just doesn't read easily but seems correct, other than the array size. Harry On 2017-05-31 04:17 PM, Harry Wentland wrote: > On 2017-05-31 09:32 AM, Harry Wentland wrote: >> On 2017-05-31 05:37 AM, Daniel Vetter wrote: >>> On Tue, May 30, 2017 at 04:01:40PM -0400, Harry Wentland wrote: >>>> AMD GPUs can have 6 CRTCs. >>>> >>>> This requires us to allocate the combinations on the heap. >>>> >>>> Signed-off-by: Harry Wentland <harry.wentland@amd.com> >>> >>> I think just dynamically allocating stuff directly and dropping the >>> #define would be even neater ... GetResources can tell us how much of >>> each >>> exists. >>> -Daniel >>> >> >> Agreed. I'll send out a v3 later. >> > > About to send v3 but this code is quite majorly wrong as-is. In > particular I see these two issues: > > 1) In iterate_combinations we assign a stack object to our list of > combinations which is then popped off the stack as soon as > get_combinations returns. Later on in test_combinations we then try > to access it with > connector_idxs = &connector_combs.items[i].elems[0]; > ... > int *crtc_idxs = &crtc_combs.items[j].elems[0]; > > 2) This for loop in iterate_combinations will simply override > comb->elems[depth] with new values: > > for (v = base; v < n; v++) { > comb->elems[depth] = v; > [...] > } > > It looks like whoever wrote the code tried to do some k choose n > algorithm to find all possible sets of crtcs and connectors but then > only ended up picking one crtc and connector out of each list anyways > (after the item popped from the stack). > > If I find time I might rewrite the test_combinations function with a > simple nested for loop that tries all crtcs with all connectors > one-to-one as this seems to be currently the intention of this function. > > Harry > >> Harry >> >>>> --- >>>> tests/kms_setmode.c | 25 +++++++++++++++---------- >>>> 1 file changed, 15 insertions(+), 10 deletions(-) >>>> >>>> diff --git a/tests/kms_setmode.c b/tests/kms_setmode.c >>>> index 430568a1c24e..847ad650d27f 100644 >>>> --- a/tests/kms_setmode.c >>>> +++ b/tests/kms_setmode.c >>>> @@ -35,11 +35,13 @@ >>>> #include "intel_bufmgr.h" >>>> #define MAX_CONNECTORS 10 >>>> -#define MAX_CRTCS 3 >>>> +#define MAX_CRTCS 6 >>>> /* max combinations with repetitions */ >>>> +/* MAX_CONNECTORS ^ MAX_CRTCS */ >>>> +/* TODO should really be MAX_CONNECTORS ^ MAX_CONNECTORS ??? */ >>>> #define MAX_COMBINATION_COUNT \ >>>> - (MAX_CONNECTORS * MAX_CONNECTORS * MAX_CONNECTORS) >>>> + (MAX_CONNECTORS * MAX_CONNECTORS * MAX_CONNECTORS * >>>> MAX_CONNECTORS * MAX_CONNECTORS * MAX_CONNECTORS) >>>> #define MAX_COMBINATION_ELEMS MAX_CRTCS >>>> static int drm_fd; >>>> @@ -743,22 +745,25 @@ static void get_combinations(int n, int k, >>>> bool allow_repetitions, >>>> static void test_combinations(const struct test_config *tconf, >>>> int connector_count) >>>> { >>>> - struct combination_set connector_combs; >>>> - struct combination_set crtc_combs; >>>> + struct combination_set *connector_combs; >>>> + struct combination_set *crtc_combs; >>>> struct connector_config *cconfs; >>>> int i; >>>> if (connector_count > 2 && (tconf->flags & TEST_STEALING)) >>>> return; >>>> + connector_combs = malloc(sizeof(*connector_combs)); >>>> + crtc_combs = malloc(sizeof(*crtc_combs)); >>>> + >>>> get_combinations(tconf->resources->count_connectors, >>>> connector_count, >>>> - false, &connector_combs); >>>> + false, connector_combs); >>>> get_combinations(tconf->resources->count_crtcs, connector_count, >>>> - true, &crtc_combs); >>>> + true, crtc_combs); >>>> igt_info("Testing: %s %d connector combinations\n", tconf->name, >>>> connector_count); >>>> - for (i = 0; i < connector_combs.count; i++) { >>>> + for (i = 0; i < connector_combs->count; i++) { >>>> int *connector_idxs; >>>> int ret; >>>> int j; >>>> @@ -766,14 +771,14 @@ static void test_combinations(const struct >>>> test_config *tconf, >>>> cconfs = malloc(sizeof(*cconfs) * connector_count); >>>> igt_assert(cconfs); >>>> - connector_idxs = &connector_combs.items[i].elems[0]; >>>> + connector_idxs = &connector_combs->items[i].elems[0]; >>>> ret = get_connectors(tconf->resources, connector_idxs, >>>> connector_count, cconfs); >>>> if (ret < 0) >>>> goto free_cconfs; >>>> - for (j = 0; j < crtc_combs.count; j++) { >>>> - int *crtc_idxs = &crtc_combs.items[j].elems[0]; >>>> + for (j = 0; j < crtc_combs->count; j++) { >>>> + int *crtc_idxs = &crtc_combs->items[j].elems[0]; >>>> ret = assign_crtc_to_connectors(tconf, crtc_idxs, >>>> connector_count, >>>> cconfs); >>>> -- >>>> 2.11.0 >>>> >>>> _______________________________________________ >>>> Intel-gfx mailing list >>>> Intel-gfx@lists.freedesktop.org >>>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx >>> > _______________________________________________ > amd-gfx mailing list > amd-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/amd-gfx
diff --git a/tests/kms_setmode.c b/tests/kms_setmode.c index 430568a1c24e..847ad650d27f 100644 --- a/tests/kms_setmode.c +++ b/tests/kms_setmode.c @@ -35,11 +35,13 @@ #include "intel_bufmgr.h" #define MAX_CONNECTORS 10 -#define MAX_CRTCS 3 +#define MAX_CRTCS 6 /* max combinations with repetitions */ +/* MAX_CONNECTORS ^ MAX_CRTCS */ +/* TODO should really be MAX_CONNECTORS ^ MAX_CONNECTORS ??? */ #define MAX_COMBINATION_COUNT \ - (MAX_CONNECTORS * MAX_CONNECTORS * MAX_CONNECTORS) + (MAX_CONNECTORS * MAX_CONNECTORS * MAX_CONNECTORS * MAX_CONNECTORS * MAX_CONNECTORS * MAX_CONNECTORS) #define MAX_COMBINATION_ELEMS MAX_CRTCS static int drm_fd; @@ -743,22 +745,25 @@ static void get_combinations(int n, int k, bool allow_repetitions, static void test_combinations(const struct test_config *tconf, int connector_count) { - struct combination_set connector_combs; - struct combination_set crtc_combs; + struct combination_set *connector_combs; + struct combination_set *crtc_combs; struct connector_config *cconfs; int i; if (connector_count > 2 && (tconf->flags & TEST_STEALING)) return; + connector_combs = malloc(sizeof(*connector_combs)); + crtc_combs = malloc(sizeof(*crtc_combs)); + get_combinations(tconf->resources->count_connectors, connector_count, - false, &connector_combs); + false, connector_combs); get_combinations(tconf->resources->count_crtcs, connector_count, - true, &crtc_combs); + true, crtc_combs); igt_info("Testing: %s %d connector combinations\n", tconf->name, connector_count); - for (i = 0; i < connector_combs.count; i++) { + for (i = 0; i < connector_combs->count; i++) { int *connector_idxs; int ret; int j; @@ -766,14 +771,14 @@ static void test_combinations(const struct test_config *tconf, cconfs = malloc(sizeof(*cconfs) * connector_count); igt_assert(cconfs); - connector_idxs = &connector_combs.items[i].elems[0]; + connector_idxs = &connector_combs->items[i].elems[0]; ret = get_connectors(tconf->resources, connector_idxs, connector_count, cconfs); if (ret < 0) goto free_cconfs; - for (j = 0; j < crtc_combs.count; j++) { - int *crtc_idxs = &crtc_combs.items[j].elems[0]; + for (j = 0; j < crtc_combs->count; j++) { + int *crtc_idxs = &crtc_combs->items[j].elems[0]; ret = assign_crtc_to_connectors(tconf, crtc_idxs, connector_count, cconfs);
AMD GPUs can have 6 CRTCs. This requires us to allocate the combinations on the heap. Signed-off-by: Harry Wentland <harry.wentland@amd.com> --- tests/kms_setmode.c | 25 +++++++++++++++---------- 1 file changed, 15 insertions(+), 10 deletions(-)