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 at 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 at lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>
