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

[Eric Anholt]

Christian Gmeiner  writes:

> 2017-05-11 1:06 GMT+02:00 Eric Anholt :
>> 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.
>
>
> With Emil's comment (regarding  drmOpen* API) taken care of
>
> Reviewed-by: Christian Gmeiner 

Emil was confused how the API works, and there's no problem here.


signature.asc
Description: PGP signature
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Christian Gmeiner]

2017-05-11 1:06 GMT+02:00 Eric Anholt :
> 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.


With Emil's comment (regarding  drmOpen* API) taken care of

Reviewed-by: Christian Gmeiner 

greets
--
Christian Gmeiner, MSc

https://www.youtube.com/user/AloryOFFICIAL
https://soundcloud.com/christian-gmeiner
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Emil Velikov]

On 19 May 2017 at 23:41, Emil Velikov  wrote:
> On 19 May 2017 at 23:21, Eric Anholt  wrote:
>> Emil Velikov  writes:
>>
>>> On 17 May 2017 at 20:13, Emil Velikov  wrote:
 On 17 May 2017 at 18:53, Eric Anholt  wrote:
> Emil Velikov  writes:
>
>> Hi Eric,
>>
>> On 11 May 2017 at 00:06, Eric Anholt  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 
>>>  #include 
>>> +#include 
>>>
>>> -#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 

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

[Emil Velikov]

On 19 May 2017 at 23:21, Eric Anholt  wrote:
> Emil Velikov  writes:
>
>> On 17 May 2017 at 20:13, Emil Velikov  wrote:
>>> On 17 May 2017 at 18:53, Eric Anholt  wrote:
 Emil Velikov  writes:

> Hi Eric,
>
> On 11 May 2017 at 00:06, Eric Anholt  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 
>>  #include 
>> +#include 
>>
>> -#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, 

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

[Eric Anholt]

Emil Velikov  writes:

> On 17 May 2017 at 20:13, Emil Velikov  wrote:
>> On 17 May 2017 at 18:53, Eric Anholt  wrote:
>>> Emil Velikov  writes:
>>>
 Hi Eric,

 On 11 May 2017 at 00:06, Eric Anholt  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 
>  #include 
> +#include 
>
> -#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 

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

[Emil Velikov]

On 17 May 2017 at 20:13, Emil Velikov  wrote:
> On 17 May 2017 at 18:53, Eric Anholt  wrote:
>> Emil Velikov  writes:
>>
>>> Hi Eric,
>>>
>>> On 11 May 2017 at 00:06, Eric Anholt  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 
  #include 
 +#include 

 -#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
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org

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

[Emil Velikov]

On 17 May 2017 at 18:53, Eric Anholt  wrote:
> Emil Velikov  writes:
>
>> Hi Eric,
>>
>> On 11 May 2017 at 00:06, Eric Anholt  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 
>>>  #include 
>>> +#include 
>>>
>>> -#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
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Eric Anholt]

Emil Velikov  writes:

> Hi Eric,
>
> On 11 May 2017 at 00:06, Eric Anholt  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 
>>  #include 
>> +#include 
>>
>> -#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.


signature.asc
Description: PGP signature
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Emil Velikov]

Hi Eric,

On 11 May 2017 at 00:06, Eric Anholt  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 
>  #include 
> +#include 
>
> -#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.

Thanks
Emil
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Eric Anholt]

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.
---
 .travis.yml|  2 +-
 Android.mk |  4 +--
 Makefile.am|  2 +-
 configure.ac   | 12 ++-
 src/gallium/Android.mk |  5 +++
 src/gallium/Makefile.am|  4 +++
 .../auxiliary/pipe-loader/pipe_loader_drm.c|  5 +++
 src/gallium/auxiliary/target-helpers/drm_helper.h  | 23 
 .../auxiliary/target-helpers/drm_helper_public.h   |  3 ++
 src/gallium/drivers/pl111/Android.mk   | 34 ++
 src/gallium/drivers/pl111/Automake.inc |  9 +
 src/gallium/drivers/pl111/Makefile.am  |  8 +
 src/gallium/drivers/vc4/vc4_resource.c | 38 
 src/gallium/drivers/vc4/vc4_resource.h |  1 +
 src/gallium/drivers/vc4/vc4_screen.c   | 12 ++-
 src/gallium/drivers/vc4/vc4_screen.h   |  5 ++-
 src/gallium/targets/dri/Android.mk |  4 +++
 src/gallium/targets/dri/Makefile.am|  1 +
 src/gallium/targets/dri/target.c   |  3 ++
 src/gallium/winsys/pl111/drm/Android.mk| 33 +
 src/gallium/winsys/pl111/drm/Makefile.am   | 34 ++
 src/gallium/winsys/pl111/drm/Makefile.sources  |  3 ++
 .../drm/pl111_drm_public.h}| 19 +-
 .../drm/pl111_drm_winsys.c}| 41 --
 src/gallium/winsys/vc4/drm/vc4_drm_public.h|  2 ++
 src/gallium/winsys/vc4/drm/vc4_drm_winsys.c| 10 --
 26 files changed, 290 insertions(+), 27 deletions(-)
 create mode 100644 src/gallium/drivers/pl111/Android.mk
 create mode 100644 src/gallium/drivers/pl111/Automake.inc
 create mode 100644 src/gallium/drivers/pl111/Makefile.am
 create mode 100644 src/gallium/winsys/pl111/drm/Android.mk
 create mode 100644 src/gallium/winsys/pl111/drm/Makefile.am
 create mode 100644 src/gallium/winsys/pl111/drm/Makefile.sources
 copy src/gallium/winsys/{vc4/drm/vc4_drm_public.h => 
pl111/drm/pl111_drm_public.h} (71%)
 copy src/gallium/winsys/{vc4/drm/vc4_drm_winsys.c => 
pl111/drm/pl111_drm_winsys.c} (51%)

diff --git a/.travis.yml b/.travis.yml
index 5e060d0335cb..895bed1fecbb 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -89,7 +89,7 @@ matrix:
 - DRI_LOADERS="--disable-glx --disable-gbm --disable-egl"
 - DRI_DRIVERS=""
 - GALLIUM_ST="--enable-dri --disable-opencl --disable-xa 
--disable-nine --disable-xvmc --disable-vdpau --disable-va --disable-omx 
--disable-gallium-osmesa"
-- 
GALLIUM_DRIVERS="i915,nouveau,r300,r600,radeonsi,freedreno,svga,swrast,vc4,virgl,etnaviv,imx"
+- 
GALLIUM_DRIVERS="i915,nouveau,pl111,r300,r600,radeonsi,freedreno,svga,swrast,vc4,virgl,etnaviv,imx"
 - VULKAN_DRIVERS=""
   addons:
 apt:
diff --git a/Android.mk b/Android.mk
index fdbf22fe643a..c8ba88f54ecd 100644
--- a/Android.mk
+++ b/Android.mk
@@ -24,7 +24,7 @@
 # BOARD_GPU_DRIVERS should be defined.  The valid values are
 #
 #   classic drivers: i915 i965
-#   gallium drivers: swrast freedreno i915g nouveau r300g r600g radeonsi vc4 
virgl vmwgfx
+#   gallium drivers: swrast freedreno i915g nouveau pl111 r300g r600g radeonsi 
vc4 virgl vmwgfx
 #
 # The main target is libGLES_mesa.  For each classic driver enabled, a DRI
 # module will also be built.  DRI modules will be loaded by libGLES_mesa.
@@ -41,7 +41,7 @@ MESA_COMMON_MK := $(MESA_TOP)/Android.common.mk
 MESA_PYTHON2 := python
 
 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
 
 MESA_GPU_DRIVERS := $(strip $(BOARD_GPU_DRIVERS))
 
diff --git a/Makefile.am b/Makefile.am
index 787174d0050a..fee132e9726e 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -43,7 +43,7 @@ AM_DISTCHECK_CONFIGURE_FLAGS = \
--enable-llvm-shared-libs \
--with-platforms=x11,wayland,drm,surfaceless \
--with-dri-drivers=i915,i965,nouveau,radeon,r200,swrast \
-   
--with-gallium-drivers=i915,nouveau,r300,r600,radeonsi,freedreno,svga,swrast,vc4,virgl,swr,etnaviv,imx
 \
+