[Mesa-dev] [PATCH 3/5] egl/android: remove droid_probe_driver()

[Tomasz Figa]

Hi Emil,

On Mon, Aug 20, 2018 at 10:47 PM Emil Velikov <emil.l.velikov at gmail.com> wrote:
>
> On 13 August 2018 at 17:18, Tomasz Figa <tfiga at chromium.org> wrote:
> > On Tue, Aug 14, 2018 at 1:09 AM Emil Velikov <emil.l.velikov at gmail.com> wrote:
> >>
> >> On 13 August 2018 at 16:43, Tomasz Figa <tfiga at chromium.org> wrote:
> >> > On Tue, Aug 14, 2018 at 12:35 AM Emil Velikov <emil.l.velikov at gmail.com> wrote:
> >> >>
> >> >> On 13 August 2018 at 16:16, Tomasz Figa <tfiga at chromium.org> wrote:
> >> >> > Hi Emil,
> >> >> >
> >> >> > On Mon, Aug 13, 2018 at 11:48 PM Emil Velikov <emil.l.velikov at gmail.com> wrote:
> >> >> >>
> >> >> >> From: Emil Velikov <emil.velikov at collabora.com>
> >> >> >>
> >> >> >> 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 <tfiga at chromium.org>

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