[Mesa-dev] [PATCH 0/2] anv/i965: drop libdrm dependency completely

[Emil Velikov]

On 4 May 2017 at 21:56, Jason Ekstrand <jason at jlekstrand.net> wrote:
> On Thu, May 4, 2017 at 12:21 PM, Kristian Høgsberg <hoegsberg at gmail.com>
> wrote:
>>
>> On Thu, May 4, 2017 at 11:43 AM, Lionel Landwerlin
>> <lionel.g.landwerlin at intel.com> wrote:
>> > On 04/05/17 07:52, Emil Velikov wrote:
>> >>
>> >> On 4 May 2017 at 14:46, Daniel Vetter <daniel at ffwll.ch> wrote:
>> >>>
>> >>> On Thu, Apr 27, 2017 at 10:58:42AM -0700, Lionel Landwerlin wrote:
>> >>>>
>> >>>> On 27/04/17 08:20, Eric Anholt wrote:
>> >>>>>
>> >>>>> Emil Velikov <emil.l.velikov at gmail.com> writes:
>> >>>>>
>> >>>>>> On 25 April 2017 at 23:56, Lionel Landwerlin
>> >>>>>> <lionel.g.landwerlin at intel.com> wrote:
>> >>>>>>>
>> >>>>>>> Hi,
>> >>>>>>>
>> >>>>>>> While working with changes that span from kernel to user space,
>> >>>>>>> I've
>> >>>>>>> been wondering whether we need to depend on libdrm at all for the
>> >>>>>>> anv
>> >>>>>>> & i965 drivers. Indeed with Ken's recent changes, we only depend
>> >>>>>>> on
>> >>>>>>> libdrm for its kernel header files which we could just embed
>> >>>>>>> ourselves.
>> >>>>>>>
>> >>>>>>> I've only included the minimal set of header files we need from
>> >>>>>>> the
>> >>>>>>> kernel for anv & i965. Maybe other drivers would be interested and
>> >>>>>>> maybe we should put all the kernel drm uapi headers into include?
>> >>>>>>>
>> >>>>>> AFAICT the goal behind the libdrm_intel fold was to allow rapid and
>> >>>>>> seamless rework of the interface.
>> >>>>>> With a potential goal to make it a shared one, as it gets
>> >>>>>> stabilised.
>> >>>>>>
>> >>>>>> Currently ANV uses more than just the UAPI headers. But if we
>> >>>>>> ignore
>> >>>>>> that, coping them is a bad idea.
>> >>>>
>> >>>> What else is anv using from libdrm? (maybe I missed something)
>> >>>>
>> >> Lionel, I stand corrected, we have instances in anv, wsi/x11, loader
>> >> and dri/i965 (the dri/common ones are just comments).
>> >> In total they seem to be over three dozen instances. Experiment with
>> >> the following:
>> >>
>> >> for i in `nm -CD --defined-only /lib/libdrm.so | cut -c 20-| grep -v
>> >> "^_" `; do git grep -w --name-only $i -- src ; done
>> >
>> >
>> > Thanks for the code snippet.
>> >
>> > For anv :
>> >     drmGetDevices2
>>
>> anv was designed to not rely on libdrm or libdrm_intel. I see the
>> commit that added the libdrm dependency is from you and was not
>> Reviewed or acked by Jason or any other core anv developer. I suggest
>> we revert that, I don't see anything in the drmGetDevices2 code that
>> is better suited for anv than what was there before.
>
>
> I did comment on e-mail that I was begrudgingly ok with it.  In retrospect,
> it looks pretty pointless.  As far as I can tell, drmGetDevices2 gains us
> exactly two things over the old method of just trial and error:
>
>  1. It stats the file to make sure that it's an actual DRM device before
> opening the file.  Does this actually matter?  If so, it's easy enough to
> add the dozen or so lines of code to do it in ANV.
>  2. It walks over all files in /dev/dri rather than just /dev/dri/renderD#.
> That's also very easy to add.
>
> I agree with Kristian that the right thing to do is to revert that patch and
> just write the dozen lines of code needed to do it "properly" if it even
> matters.
>
Reverting the patch will lead to
 - 3s starup delays since the open() will wake up the $other GPU
Rare, but we've had reports already.
 - slower porting/adoption of Vulkan on !Linux Unixes

In either case, not my call.
-Emil