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

[Emil Velikov]

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".

As a parting thought I would kindly ask you to minimise the "you
should fix $someones_code". I've been doing for a while now ;-)

Thanks
-Emil

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/drm_ioctl.c?h=v4.12-rc1#n41