[Mesa-dev] [PATCH 2/2] gallium: Add renderonly-based support for pl111+vc4.

[Emil Velikov]

On 19 May 2017 at 23:41, Emil Velikov <emil.l.velikov at gmail.com> wrote:
> On 19 May 2017 at 23:21, Eric Anholt <eric at anholt.net> wrote:
>> Emil Velikov <emil.l.velikov at gmail.com> writes:
>>
>>> On 17 May 2017 at 20:13, Emil Velikov <emil.l.velikov at gmail.com> wrote:
>>>> On 17 May 2017 at 18:53, Eric Anholt <eric at anholt.net> wrote:
>>>>> Emil Velikov <emil.l.velikov at gmail.com> writes:
>>>>>
>>>>>> Hi Eric,
>>>>>>
>>>>>> On 11 May 2017 at 00:06, Eric Anholt <eric at anholt.net> wrote:
>>>>>>> This follows the model of imx (display) and etnaviv (render): pl111 is a
>>>>>>> display-only device, so when asked to do GL for it, we see if we have a
>>>>>>> vc4 renderer, make the vc4 screen, and have vc4 call back to pl111 to do
>>>>>>> scanout allocations.
>>>>>>>
>>>>>>> The difference from etnaviv is that we share the same BO between vc4 and
>>>>>>> pl111, rather than having a vc4 bo and a pl11 bo and copies between the
>>>>>>> two.  The only mismatch between their requirements is that vc4 requires
>>>>>>> 4-pixel (at 32bpp) stride alignment, while pl111 requires that stride
>>>>>>> match width.  The kernel will reject any modesets to an incorrect stride,
>>>>>>> so the 3D driver doesn't need to worry about that.
>>>>>>> ---
>>>>>> I'm not familiar with the hardware itself so I cannot comment on those.
>>>>>> There's a couple of small notes within, but overall the patch looks good.
>>>>>>
>>>>>>>  .travis.yml                                        |  2 +-
>>>>>>>  Makefile.am                                        |  2 +-
>>>>>> Yes, thank you!
>>>>>>
>>>>>>
>>>>>>> --- a/Android.mk
>>>>>>> +++ b/Android.mk
>>>>>>
>>>>>>>  classic_drivers := i915 i965
>>>>>>> -gallium_drivers := swrast freedreno i915g nouveau r300g r600g radeonsi vmwgfx vc4 virgl
>>>>>>> +gallium_drivers := swrast freedreno i915g nouveau pl111 r300g r600g radeonsi vmwgfx vc4 virgl
>>>>>>>
>>>>>> The recent Android cleanups by RobH which will cause a clash. The gist
>>>>>> is below, but feel free to skim through commit
>>>>>> 3f097396a1642bb7033002d0bdd37e194afce06a.
>>>>>>  - rework for the new gallium_drivers format
>>>>>>  - add a couple of lines in src/gallium/drivers/pl111/Android.mk
>>>>>> analogous to the vc4 - the "ifneq $HAVE_foo" hunk
>>>>>>  - drop the guard in src/gallium/Android.mk
>>>>>>
>>>>>>
>>>>>>> +++ b/src/gallium/winsys/pl111/drm/pl111_drm_winsys.c
>>>>>>
>>>>>>> -#include <unistd.h>
>>>>>>>  #include <fcntl.h>
>>>>>>> +#include <unistd.h>
>>>>>>>
>>>>>>> -#include "vc4_drm_public.h"
>>>>>>> +#include "pl111_drm_public.h"
>>>>>>> +#include "vc4/drm/vc4_drm_public.h"
>>>>>>> +#include "xf86drm.h"
>>>>>>>
>>>>>>> -#include "vc4/vc4_screen.h"
>>>>>>> +#include "pipe/p_screen.h"
>>>>>>> +#include "renderonly/renderonly.h"
>>>>>>
>>>>>>> +#include "util/u_format.h"
>>>>>> Seems unused.
>>>>>>
>>>>>>>
>>>>>>> -struct pipe_screen *
>>>>>>> -vc4_drm_screen_create(int fd)
>>>>>>> +struct pipe_screen *pl111_drm_screen_create(int fd)
>>>>>>>  {
>>>>>>> -       return vc4_screen_create(fcntl(fd, F_DUPFD_CLOEXEC, 3));
>>>>>>> +   struct renderonly ro = {
>>>>>>> +      /* Passes the vc4-allocated BO through to the pl111 DRM device using
>>>>>>> +       * PRIME buffer sharing.  The VC4 BO must be linear, which the SCANOUT
>>>>>>> +       * flag on allocation will have ensured.
>>>>>>> +       */
>>>>>>> +      .create_for_resource = renderonly_create_gpu_import_for_resource,
>>>>>>> +      .kms_fd = fd,
>>>>>>> +      .gpu_fd = drmOpenWithType("vc4", NULL, DRM_NODE_RENDER),
>>>>>> Please don't use the drmOpen* API. It's a legacy dragon with very
>>>>>> subtle and inner workings.
>>>>>> Using drmGetDevice[s] should provide any information that you need.
>>>>>> Alternatively please let us know what's missing so we can address it.
>>>>>
>>>>> One this platform, there are exactly two devices, one is vc4 and the
>>>>> other is pl111.  drmOpenWithType is exactly the API we want, and if you
>>>>> want something else, then you should probably replace its insides
>>>>> instead of telling people to use a different API.
>>>>
>>>> Changing the insides also changes the behaviour, which could break users :-\
>>>>
>>> A couple of things from our IRC chat last night:
>>>
>>> - My suggestion was never meant as requirement or a blocker. Let along
>>> "exerting control" as you call it :-\
>>> It's aimed to save you/others time since the drmOpen* API
>>> implementation is aimed for UMS and broken for KMS drivers.
>>>
>>> - You asked a couple of times "how this can break", despite my pointer
>>> to DanielV's summary [1] and some encouragement to skim through the
>>> drmOpen* code yourself.
>>> Now a bit less tired, here it is the exact scenario how/why things are broken.
>>>
>>> A) User opens the vc4 device and calls drmSetInterfaceVersion
>>> effectively populating the "unique name"
>>> Plymouth, modesetting ddx, Xserver itself and others can easily do so.
>>> B) Consecutive calls to  drmOpenWithType("vc4", NULL..) will fail
>>> since the drmGetBusid/GET_UNIQUE return the non-zero "unique name".
>>> That happens in drmOpenByName which considers the latter as "device
>>> already opened".
>>
>> According to LIBGL_DEBUG=verbose, drmGetBusid is returning NULL on this
>> platform.  drm_getunique() is banned on render nodes, resulting in an
>> EACCES and drmGetBusid returns NULL.
>>
> Yes, SET_VERSION is not allowed for render nodes.
>
> Have you tried opening the master vc4 node, calling the
> drmSetInterfaceVersion and attempting drmOpen...?
> If I'm not misreading the drm_file::master specifics, unique will be
> set and drmOpen... will fail.
>
Scratch that - dull moment.

-Emil