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

[Emil Velikov]

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 :-\

-Emil