Re: [Mesa-dev] [PATCH 3/5] egl/android: remove droid_probe_driver()
Hi Emil, On Mon, Aug 20, 2018 at 10:47 PM Emil Velikov wrote: > > On 13 August 2018 at 17:18, Tomasz Figa wrote: > > On Tue, Aug 14, 2018 at 1:09 AM Emil Velikov > > wrote: > >> > >> On 13 August 2018 at 16:43, Tomasz Figa wrote: > >> > On Tue, Aug 14, 2018 at 12:35 AM Emil Velikov > >> > wrote: > >> >> > >> >> On 13 August 2018 at 16:16, Tomasz Figa wrote: > >> >> > Hi Emil, > >> >> > > >> >> > On Mon, Aug 13, 2018 at 11:48 PM Emil Velikov > >> >> > wrote: > >> >> >> > >> >> >> From: Emil Velikov > >> >> >> > >> >> >> The function name is misleading - it effectively checks if > >> >> >> loader_get_driver_for_fd fails. Which can happen only only on strdup > >> >> >> error - a close to impossible scenario. > >> >> > > >> >> > How about a DRI node which doesn't have a driver in Mesa? > >> >> > > >> >> Can you elaborate a bit - are you thinking of any of the following or > >> >> something else: > >> >> - no support for vendor X > >> >> - supported vendor, missing vendor/device pci id for device X > >> >> - supported vendor, built w/o it > >> >> > >> >> All these are fairly different cases, with somewhat different solution > >> >> for each one. > >> > > >> > Let's say "no support for vendor X", but supported vendor Y GPU next > >> > to it. We want this code to skip vendor X DRI node and choose vendor Y > >> > DRI node. > >> > > >> >> > >> >> Fwiw the function loader_get_driver_for_fd does: > >> >> - gets the vendor/device pci id and maps that to a driver_name > >> >> - if device not a pci device (or query fails) - fallback to the name > >> >> as returned in drmGetVersion > >> > > >> > Good catch. Looks like I misunderstood what it does when reviewing > >> > Rob's series and existing code doesn't work as I expected. I think it > >> > would just error out in the case above, right? > >> > > >> Determining if a device is "supported" is fairly subtle: > >> For example, even if you open the foo_dri.so the driver can fail due > >> to old kernel module, LLVM version, etc. > >> One solution is to continue loading up-to dri2_create_screen() - if it > >> fails fall-back to the next device. > >> > >> Any objections if I do that as follow-up patch, if you agree of course? > > > > Our downstream patch [1] actually loaded until > > dri2_load_driver(_dri3)() in probe, but if you think > > dri2_create_screen() could make more sense, I'm okay with it. Thanks. > > > Right, patch that does that can be seen here (copy in your inbox) > https://patchwork.freedesktop.org/patch/244276/ > > Can you Ack/R-b/T-b the series - I think all your concerns have been > addressed. > > https://patchwork.freedesktop.org/patch/244366/ > https://patchwork.freedesktop.org/patch/244243/ > https://patchwork.freedesktop.org/patch/244244/ > https://patchwork.freedesktop.org/patch/244240/ > https://patchwork.freedesktop.org/patch/244242/ > Sorry for being late. Generally, for the original series of 5 patches: Reviewed-by: Tomasz Figa As for patch 6/5, it needs one fix I mentioned there and I also need to get a chance to test it. Best regards, Tomasz ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 3/5] egl/android: remove droid_probe_driver()
On 13 August 2018 at 17:18, Tomasz Figa wrote: > On Tue, Aug 14, 2018 at 1:09 AM Emil Velikov wrote: >> >> On 13 August 2018 at 16:43, Tomasz Figa wrote: >> > On Tue, Aug 14, 2018 at 12:35 AM Emil Velikov >> > wrote: >> >> >> >> On 13 August 2018 at 16:16, Tomasz Figa wrote: >> >> > Hi Emil, >> >> > >> >> > On Mon, Aug 13, 2018 at 11:48 PM Emil Velikov >> >> > wrote: >> >> >> >> >> >> From: Emil Velikov >> >> >> >> >> >> The function name is misleading - it effectively checks if >> >> >> loader_get_driver_for_fd fails. Which can happen only only on strdup >> >> >> error - a close to impossible scenario. >> >> > >> >> > How about a DRI node which doesn't have a driver in Mesa? >> >> > >> >> Can you elaborate a bit - are you thinking of any of the following or >> >> something else: >> >> - no support for vendor X >> >> - supported vendor, missing vendor/device pci id for device X >> >> - supported vendor, built w/o it >> >> >> >> All these are fairly different cases, with somewhat different solution >> >> for each one. >> > >> > Let's say "no support for vendor X", but supported vendor Y GPU next >> > to it. We want this code to skip vendor X DRI node and choose vendor Y >> > DRI node. >> > >> >> >> >> Fwiw the function loader_get_driver_for_fd does: >> >> - gets the vendor/device pci id and maps that to a driver_name >> >> - if device not a pci device (or query fails) - fallback to the name >> >> as returned in drmGetVersion >> > >> > Good catch. Looks like I misunderstood what it does when reviewing >> > Rob's series and existing code doesn't work as I expected. I think it >> > would just error out in the case above, right? >> > >> Determining if a device is "supported" is fairly subtle: >> For example, even if you open the foo_dri.so the driver can fail due >> to old kernel module, LLVM version, etc. >> One solution is to continue loading up-to dri2_create_screen() - if it >> fails fall-back to the next device. >> >> Any objections if I do that as follow-up patch, if you agree of course? > > Our downstream patch [1] actually loaded until > dri2_load_driver(_dri3)() in probe, but if you think > dri2_create_screen() could make more sense, I'm okay with it. Thanks. > Right, patch that does that can be seen here (copy in your inbox) https://patchwork.freedesktop.org/patch/244276/ Can you Ack/R-b/T-b the series - I think all your concerns have been addressed. https://patchwork.freedesktop.org/patch/244366/ https://patchwork.freedesktop.org/patch/244243/ https://patchwork.freedesktop.org/patch/244244/ https://patchwork.freedesktop.org/patch/244240/ https://patchwork.freedesktop.org/patch/244242/ Thanks Emil ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 3/5] egl/android: remove droid_probe_driver()
On Tue, Aug 14, 2018 at 1:09 AM Emil Velikov wrote: > > On 13 August 2018 at 16:43, Tomasz Figa wrote: > > On Tue, Aug 14, 2018 at 12:35 AM Emil Velikov > > wrote: > >> > >> On 13 August 2018 at 16:16, Tomasz Figa wrote: > >> > Hi Emil, > >> > > >> > On Mon, Aug 13, 2018 at 11:48 PM Emil Velikov > >> > wrote: > >> >> > >> >> From: Emil Velikov > >> >> > >> >> The function name is misleading - it effectively checks if > >> >> loader_get_driver_for_fd fails. Which can happen only only on strdup > >> >> error - a close to impossible scenario. > >> > > >> > How about a DRI node which doesn't have a driver in Mesa? > >> > > >> Can you elaborate a bit - are you thinking of any of the following or > >> something else: > >> - no support for vendor X > >> - supported vendor, missing vendor/device pci id for device X > >> - supported vendor, built w/o it > >> > >> All these are fairly different cases, with somewhat different solution > >> for each one. > > > > Let's say "no support for vendor X", but supported vendor Y GPU next > > to it. We want this code to skip vendor X DRI node and choose vendor Y > > DRI node. > > > >> > >> Fwiw the function loader_get_driver_for_fd does: > >> - gets the vendor/device pci id and maps that to a driver_name > >> - if device not a pci device (or query fails) - fallback to the name > >> as returned in drmGetVersion > > > > Good catch. Looks like I misunderstood what it does when reviewing > > Rob's series and existing code doesn't work as I expected. I think it > > would just error out in the case above, right? > > > Determining if a device is "supported" is fairly subtle: > For example, even if you open the foo_dri.so the driver can fail due > to old kernel module, LLVM version, etc. > One solution is to continue loading up-to dri2_create_screen() - if it > fails fall-back to the next device. > > Any objections if I do that as follow-up patch, if you agree of course? Our downstream patch [1] actually loaded until dri2_load_driver(_dri3)() in probe, but if you think dri2_create_screen() could make more sense, I'm okay with it. Thanks. [1] https://chromium-review.googlesource.com/c/chromiumos/third_party/mesa/+/558138 Best regards, Tomasz ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 3/5] egl/android: remove droid_probe_driver()
On 13 August 2018 at 16:43, Tomasz Figa wrote: > On Tue, Aug 14, 2018 at 12:35 AM Emil Velikov > wrote: >> >> On 13 August 2018 at 16:16, Tomasz Figa wrote: >> > Hi Emil, >> > >> > On Mon, Aug 13, 2018 at 11:48 PM Emil Velikov >> > wrote: >> >> >> >> From: Emil Velikov >> >> >> >> The function name is misleading - it effectively checks if >> >> loader_get_driver_for_fd fails. Which can happen only only on strdup >> >> error - a close to impossible scenario. >> > >> > How about a DRI node which doesn't have a driver in Mesa? >> > >> Can you elaborate a bit - are you thinking of any of the following or >> something else: >> - no support for vendor X >> - supported vendor, missing vendor/device pci id for device X >> - supported vendor, built w/o it >> >> All these are fairly different cases, with somewhat different solution >> for each one. > > Let's say "no support for vendor X", but supported vendor Y GPU next > to it. We want this code to skip vendor X DRI node and choose vendor Y > DRI node. > >> >> Fwiw the function loader_get_driver_for_fd does: >> - gets the vendor/device pci id and maps that to a driver_name >> - if device not a pci device (or query fails) - fallback to the name >> as returned in drmGetVersion > > Good catch. Looks like I misunderstood what it does when reviewing > Rob's series and existing code doesn't work as I expected. I think it > would just error out in the case above, right? > Determining if a device is "supported" is fairly subtle: For example, even if you open the foo_dri.so the driver can fail due to old kernel module, LLVM version, etc. One solution is to continue loading up-to dri2_create_screen() - if it fails fall-back to the next device. Any objections if I do that as follow-up patch, if you agree of course? Feels a bit dirty, but will work until we get something like EGL_EXT_explicit_device and EGL_MESA_query_renderer. Thanks Emil ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 3/5] egl/android: remove droid_probe_driver()
On Tue, Aug 14, 2018 at 12:35 AM Emil Velikov wrote: > > On 13 August 2018 at 16:16, Tomasz Figa wrote: > > Hi Emil, > > > > On Mon, Aug 13, 2018 at 11:48 PM Emil Velikov > > wrote: > >> > >> From: Emil Velikov > >> > >> The function name is misleading - it effectively checks if > >> loader_get_driver_for_fd fails. Which can happen only only on strdup > >> error - a close to impossible scenario. > > > > How about a DRI node which doesn't have a driver in Mesa? > > > Can you elaborate a bit - are you thinking of any of the following or > something else: > - no support for vendor X > - supported vendor, missing vendor/device pci id for device X > - supported vendor, built w/o it > > All these are fairly different cases, with somewhat different solution > for each one. Let's say "no support for vendor X", but supported vendor Y GPU next to it. We want this code to skip vendor X DRI node and choose vendor Y DRI node. > > Fwiw the function loader_get_driver_for_fd does: > - gets the vendor/device pci id and maps that to a driver_name > - if device not a pci device (or query fails) - fallback to the name > as returned in drmGetVersion Good catch. Looks like I misunderstood what it does when reviewing Rob's series and existing code doesn't work as I expected. I think it would just error out in the case above, right? Best regards, Tomasz ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 3/5] egl/android: remove droid_probe_driver()
On 13 August 2018 at 16:16, Tomasz Figa wrote: > Hi Emil, > > On Mon, Aug 13, 2018 at 11:48 PM Emil Velikov > wrote: >> >> From: Emil Velikov >> >> The function name is misleading - it effectively checks if >> loader_get_driver_for_fd fails. Which can happen only only on strdup >> error - a close to impossible scenario. > > How about a DRI node which doesn't have a driver in Mesa? > Can you elaborate a bit - are you thinking of any of the following or something else: - no support for vendor X - supported vendor, missing vendor/device pci id for device X - supported vendor, built w/o it All these are fairly different cases, with somewhat different solution for each one. Fwiw the function loader_get_driver_for_fd does: - gets the vendor/device pci id and maps that to a driver_name - if device not a pci device (or query fails) - fallback to the name as returned in drmGetVersion Thanks Emil ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 3/5] egl/android: remove droid_probe_driver()
Hi Emil, On Mon, Aug 13, 2018 at 11:48 PM Emil Velikov wrote: > > From: Emil Velikov > > The function name is misleading - it effectively checks if > loader_get_driver_for_fd fails. Which can happen only only on strdup > error - a close to impossible scenario. How about a DRI node which doesn't have a driver in Mesa? Best regards, Tomasz ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
