Thanks Emil, I'll submit v2 to address your comments.
I'm using office365, not sure this mail is OK for formatting, otherwise I'll switch to a mail client.
Regards,
Qiang
From: Emil Velikov <emil.l.velikov@xxxxxxxxx>
Sent: Wednesday, July 13, 2016 7:15:43 PM
To: Yu, Qiang
Cc: amd-gfx@xxxxxxxxxxxxxxxxxxxxx; ML dri-devel
Subject: Re: [PATCH][libdrm] drm: Fix multi GPU drmGetDevice return wrong device
Sent: Wednesday, July 13, 2016 7:15:43 PM
To: Yu, Qiang
Cc: amd-gfx@xxxxxxxxxxxxxxxxxxxxx; ML dri-devel
Subject: Re: [PATCH][libdrm] drm: Fix multi GPU drmGetDevice return wrong device
On 13 July 2016 at 11:17, Yu, Qiang <Qiang.Yu@xxxxxxx> wrote:
> Hi Emil,
>
>
> Nice to hear from you.
>
>
> On 11 July 2016 at 06:17, Qiang Yu <Qiang.Yu@xxxxxxx> wrote:
>> drmGetDevice will always return the first device it find
>> under /dev/dri/. This is not true for multi GPU situation.
>>
> How does the following alternative solution sound:
> - keep drmFoldDuplicatedDevices as is
> - after the drmFoldDuplicatedDevices call use the find_rdev to find
> the correct device in local_devices.
>
> [yuq] This is also OK. But drmFoldDuplicatedDevices() has to be changed for
> the
> drmFreeDevices() in the error handling path: it also exit after see a NULL
> in the array.
>
>> Plus fix the memory leak in error handling path of
>> drmGetDevices.
>>
> Unless I'm missing something, there is no memory leak fix below ?
> Alternatively please keep it as separate patch.
>
> [yuq] This is fixed at the same time by changing drmFoldDuplicatedDevices().
>
Heh, silly me was assumed that your earlier patch fixed all the
codepaths to handle the "holes" made by drmFoldDuplicatedDevices.
Seems like the ones drmFreeDevices and drmGetDevice are still
outstanding, thus the predicament.
In this case we could do either:
- go with the above making sure drmFoldDuplicatedDevices doesn't create 'holes'
Note: we still want to fix drmFreeDevices to continue (as opposed to
break) when it sees one.
- or, fix drmGetDevice/drmFreeDevices
In either case we want that as separate patch, bonus points for adding
a inline comment about the behaviour of drmFoldDuplicatedDevices.
About the core issue a trivial suggestion - s/move target to the first
of local_devices/store target at local_devices[0] for ease to use
below/
Thanks
Emil
P.S. When working with mailing lists please use plain text emails.
> Hi Emil,
>
>
> Nice to hear from you.
>
>
> On 11 July 2016 at 06:17, Qiang Yu <Qiang.Yu@xxxxxxx> wrote:
>> drmGetDevice will always return the first device it find
>> under /dev/dri/. This is not true for multi GPU situation.
>>
> How does the following alternative solution sound:
> - keep drmFoldDuplicatedDevices as is
> - after the drmFoldDuplicatedDevices call use the find_rdev to find
> the correct device in local_devices.
>
> [yuq] This is also OK. But drmFoldDuplicatedDevices() has to be changed for
> the
> drmFreeDevices() in the error handling path: it also exit after see a NULL
> in the array.
>
>> Plus fix the memory leak in error handling path of
>> drmGetDevices.
>>
> Unless I'm missing something, there is no memory leak fix below ?
> Alternatively please keep it as separate patch.
>
> [yuq] This is fixed at the same time by changing drmFoldDuplicatedDevices().
>
Heh, silly me was assumed that your earlier patch fixed all the
codepaths to handle the "holes" made by drmFoldDuplicatedDevices.
Seems like the ones drmFreeDevices and drmGetDevice are still
outstanding, thus the predicament.
In this case we could do either:
- go with the above making sure drmFoldDuplicatedDevices doesn't create 'holes'
Note: we still want to fix drmFreeDevices to continue (as opposed to
break) when it sees one.
- or, fix drmGetDevice/drmFreeDevices
In either case we want that as separate patch, bonus points for adding
a inline comment about the behaviour of drmFoldDuplicatedDevices.
About the core issue a trivial suggestion - s/move target to the first
of local_devices/store target at local_devices[0] for ease to use
below/
Thanks
Emil
P.S. When working with mailing lists please use plain text emails.
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
