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 at lists.freedesktop.org; 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 at amd.com> wrote: > Hi Emil, > > > Nice to hear from you. > > > On 11 July 2016 at 06:17, Qiang Yu <Qiang.Yu at amd.com> 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. -------------- next part -------------- An HTML attachment was scrubbed... URL: <https://lists.freedesktop.org/archives/amd-gfx/attachments/20160714/a0a85a76/attachment.html>
