[Intel-gfx] [PATCH i-g-t] tests/kms_setmode: increase MAX_CRTCS to 6

[Harry Wentland]

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 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
>>>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx