Emil Velikov <emil.l.veli...@gmail.com> writes:
> On 17 May 2017 at 20:13, Emil Velikov <emil.l.veli...@gmail.com> wrote:
>> On 17 May 2017 at 18:53, Eric Anholt <e...@anholt.net> wrote:
>>> Emil Velikov <emil.l.veli...@gmail.com> writes:
>>>
>>>> Hi Eric,
>>>>
>>>> On 11 May 2017 at 00:06, Eric Anholt <e...@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. Thanks for *finally* explaining your concern, though.
signature.asc
Description: PGP signature
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
