diff mbox series

drm: pl111: enable render node

Message ID 20200423223459.200858-1-pcc@google.com (mailing list archive)
State New, archived
Headers show
Series drm: pl111: enable render node | expand

Commit Message

Peter Collingbourne April 23, 2020, 10:34 p.m. UTC
The render node is required by Android which does not support the legacy
drmAuth authentication process.

Signed-off-by: Peter Collingbourne <pcc@google.com>
---
 drivers/gpu/drm/pl111/pl111_drv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Eric Anholt April 24, 2020, 4:29 a.m. UTC | #1
On Thu, Apr 23, 2020 at 3:35 PM Peter Collingbourne <pcc@google.com> wrote:
>
> The render node is required by Android which does not support the legacy
> drmAuth authentication process.

There's no rendering engine on pl111, so only the control node makes
sense for this HW since all this driver does is display.  Can you
clarify what you're trying to do with pl111?
Emil Velikov April 24, 2020, 11:09 a.m. UTC | #2
On Thu, 23 Apr 2020 at 23:51, Peter Collingbourne <pcc@google.com> wrote:
>
> The render node is required by Android which does not support the legacy
> drmAuth authentication process.
>
> Signed-off-by: Peter Collingbourne <pcc@google.com>
> ---
>  drivers/gpu/drm/pl111/pl111_drv.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
The summary talks about drmAuth, yet exposes a render node. Even
through there's no rendering engine in the HW, as mentioned by Eric.

AFAICT the only way drmAuth is relevant with pl111 is when you want to
export/import dma bufs.
Although that is handled in core and the artificial DRM_AUTH
restriction has been lifted with commit [1].

-Emil

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?h=v5.7-rc2&id=30a958526d2cc6df38347336a602479d048d92e7
Peter Collingbourne April 24, 2020, 6:53 p.m. UTC | #3
On Fri, Apr 24, 2020 at 4:11 AM Emil Velikov <emil.l.velikov@gmail.com> wrote:
>
> On Thu, 23 Apr 2020 at 23:51, Peter Collingbourne <pcc@google.com> wrote:
> >
> > The render node is required by Android which does not support the legacy
> > drmAuth authentication process.
> >
> > Signed-off-by: Peter Collingbourne <pcc@google.com>
> > ---
> >  drivers/gpu/drm/pl111/pl111_drv.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> The summary talks about drmAuth, yet exposes a render node. Even
> through there's no rendering engine in the HW, as mentioned by Eric.
>
> AFAICT the only way drmAuth is relevant with pl111 is when you want to
> export/import dma bufs.
> Although that is handled in core and the artificial DRM_AUTH
> restriction has been lifted with commit [1].
>
> -Emil
>
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?h=v5.7-rc2&id=30a958526d2cc6df38347336a602479d048d92e7

Okay, most likely drmAuth is irrelevant here (I don't know much about
it to be honest; I know that Android uses render nodes, so I figured
that drmAuth must therefore be the thing that it doesn't use). Sorry
for the confusion. Here is a better explanation of why I needed this
change.

Android has a composer process that opens the primary node and uses
DRM_IOCTL_MODE_ATOMIC to switch between frame buffers, and a renderer
process (surfaceflinger) that opens the render node, prepares frame
buffers and sends them to the composer. One idea for adapting this
architecture to devices without render nodes is to have the renderer
process open the primary node instead. But this runs into a problem:
suppose that the renderer process starts before the composer process.
In this case, the kernel makes the renderer the DRM master, so the
composer can't change the frame buffer. Render nodes don't have this
problem because opening them doesn't affect the master.

I considered fixing this by having the composer issue
DRM_IOCTL_SET_MASTER, but this requires root privileges. If we require
drivers to provide render nodes and control access to the primary node
while opening up the render node, we can ensure that only authorized
processes can become the master without requiring them to be root.

Peter
Emil Velikov April 27, 2020, 2:43 p.m. UTC | #4
On Fri, 24 Apr 2020 at 19:54, Peter Collingbourne <pcc@google.com> wrote:
>
> On Fri, Apr 24, 2020 at 4:11 AM Emil Velikov <emil.l.velikov@gmail.com> wrote:
> >
> > On Thu, 23 Apr 2020 at 23:51, Peter Collingbourne <pcc@google.com> wrote:
> > >
> > > The render node is required by Android which does not support the legacy
> > > drmAuth authentication process.
> > >
> > > Signed-off-by: Peter Collingbourne <pcc@google.com>
> > > ---
> > >  drivers/gpu/drm/pl111/pl111_drv.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > The summary talks about drmAuth, yet exposes a render node. Even
> > through there's no rendering engine in the HW, as mentioned by Eric.
> >
> > AFAICT the only way drmAuth is relevant with pl111 is when you want to
> > export/import dma bufs.
> > Although that is handled in core and the artificial DRM_AUTH
> > restriction has been lifted with commit [1].
> >
> > -Emil
> >
> > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?h=v5.7-rc2&id=30a958526d2cc6df38347336a602479d048d92e7
>
> Okay, most likely drmAuth is irrelevant here (I don't know much about
> it to be honest; I know that Android uses render nodes, so I figured
> that drmAuth must therefore be the thing that it doesn't use). Sorry
> for the confusion. Here is a better explanation of why I needed this
> change.
>
> Android has a composer process that opens the primary node and uses
> DRM_IOCTL_MODE_ATOMIC to switch between frame buffers, and a renderer
> process (surfaceflinger) that opens the render node, prepares frame
> buffers and sends them to the composer. One idea for adapting this
> architecture to devices without render nodes is to have the renderer
> process open the primary node instead. But this runs into a problem:
> suppose that the renderer process starts before the composer process.
> In this case, the kernel makes the renderer the DRM master, so the
> composer can't change the frame buffer. Render nodes don't have this
> problem because opening them doesn't affect the master.
>
> I considered fixing this by having the composer issue
> DRM_IOCTL_SET_MASTER, but this requires root privileges. If we require
> drivers to provide render nodes and control access to the primary node
> while opening up the render node, we can ensure that only authorized
> processes can become the master without requiring them to be root.
>
I think that the crux, is trying to open a _primary_ node for
_rendering_ purposes.
While that may work - as you outline above - it's usually a bad idea.

To step back a bit, in practise we have three SoC combinations:
 - complete lack of render device - then you default to software rendering [1]
 - display and render device are one and the same - no change needed,
things should just work
 - display and render devices are separate - surfaceflinger should
open the correct _rendering_ device node.

[1] Mesa's libEGL (don't recall exact name on Android) can open VGEM
render node, to work with the kms-swrast_dri.so software rasteriser
module.

While I did not try [1] with Android, it was working fine with CrOS. I
suggest giving it a try and reporting bugs.

Thanks
Emil
Peter Collingbourne April 27, 2020, 3:58 p.m. UTC | #5
On Mon, Apr 27, 2020 at 7:45 AM Emil Velikov <emil.l.velikov@gmail.com> wrote:
>
> On Fri, 24 Apr 2020 at 19:54, Peter Collingbourne <pcc@google.com> wrote:
> >
> > On Fri, Apr 24, 2020 at 4:11 AM Emil Velikov <emil.l.velikov@gmail.com> wrote:
> > >
> > > On Thu, 23 Apr 2020 at 23:51, Peter Collingbourne <pcc@google.com> wrote:
> > > >
> > > > The render node is required by Android which does not support the legacy
> > > > drmAuth authentication process.
> > > >
> > > > Signed-off-by: Peter Collingbourne <pcc@google.com>
> > > > ---
> > > >  drivers/gpu/drm/pl111/pl111_drv.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > The summary talks about drmAuth, yet exposes a render node. Even
> > > through there's no rendering engine in the HW, as mentioned by Eric.
> > >
> > > AFAICT the only way drmAuth is relevant with pl111 is when you want to
> > > export/import dma bufs.
> > > Although that is handled in core and the artificial DRM_AUTH
> > > restriction has been lifted with commit [1].
> > >
> > > -Emil
> > >
> > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?h=v5.7-rc2&id=30a958526d2cc6df38347336a602479d048d92e7
> >
> > Okay, most likely drmAuth is irrelevant here (I don't know much about
> > it to be honest; I know that Android uses render nodes, so I figured
> > that drmAuth must therefore be the thing that it doesn't use). Sorry
> > for the confusion. Here is a better explanation of why I needed this
> > change.
> >
> > Android has a composer process that opens the primary node and uses
> > DRM_IOCTL_MODE_ATOMIC to switch between frame buffers, and a renderer
> > process (surfaceflinger) that opens the render node, prepares frame
> > buffers and sends them to the composer. One idea for adapting this
> > architecture to devices without render nodes is to have the renderer
> > process open the primary node instead. But this runs into a problem:
> > suppose that the renderer process starts before the composer process.
> > In this case, the kernel makes the renderer the DRM master, so the
> > composer can't change the frame buffer. Render nodes don't have this
> > problem because opening them doesn't affect the master.
> >
> > I considered fixing this by having the composer issue
> > DRM_IOCTL_SET_MASTER, but this requires root privileges. If we require
> > drivers to provide render nodes and control access to the primary node
> > while opening up the render node, we can ensure that only authorized
> > processes can become the master without requiring them to be root.
> >
> I think that the crux, is trying to open a _primary_ node for
> _rendering_ purposes.
> While that may work - as you outline above - it's usually a bad idea.
>
> To step back a bit, in practise we have three SoC combinations:
>  - complete lack of render device - then you default to software rendering [1]
>  - display and render device are one and the same - no change needed,
> things should just work
>  - display and render devices are separate - surfaceflinger should
> open the correct _rendering_ device node.
>
> [1] Mesa's libEGL (don't recall exact name on Android) can open VGEM

(Android uses SwiftShader for software rendering, not Mesa, FWIW.)

> render node, to work with the kms-swrast_dri.so software rasteriser
> module.
>
> While I did not try [1] with Android, it was working fine with CrOS. I
> suggest giving it a try and reporting bugs.

I'm not sure I understand your suggestion, or at least how it would
work on Android. The composer process wouldn't be able to use
DRM_IOCTL_MODE_ATOMIC to display a frame buffer produced by the
surfaceflinger process using a vgem render node, would it? It's a
different driver so the memory region used for the frame buffer
wouldn't necessarily be compatible with the pl111 device. As far as I
know, the frame buffer would need to be produced using a pl111 render
node.

Peter
Eric Anholt April 27, 2020, 4:47 p.m. UTC | #6
On Mon, Apr 27, 2020 at 7:45 AM Emil Velikov <emil.l.velikov@gmail.com> wrote:
>
> On Fri, 24 Apr 2020 at 19:54, Peter Collingbourne <pcc@google.com> wrote:
> >
> > On Fri, Apr 24, 2020 at 4:11 AM Emil Velikov <emil.l.velikov@gmail.com> wrote:
> > >
> > > On Thu, 23 Apr 2020 at 23:51, Peter Collingbourne <pcc@google.com> wrote:
> > > >
> > > > The render node is required by Android which does not support the legacy
> > > > drmAuth authentication process.
> > > >
> > > > Signed-off-by: Peter Collingbourne <pcc@google.com>
> > > > ---
> > > >  drivers/gpu/drm/pl111/pl111_drv.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > The summary talks about drmAuth, yet exposes a render node. Even
> > > through there's no rendering engine in the HW, as mentioned by Eric.
> > >
> > > AFAICT the only way drmAuth is relevant with pl111 is when you want to
> > > export/import dma bufs.
> > > Although that is handled in core and the artificial DRM_AUTH
> > > restriction has been lifted with commit [1].
> > >
> > > -Emil
> > >
> > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?h=v5.7-rc2&id=30a958526d2cc6df38347336a602479d048d92e7
> >
> > Okay, most likely drmAuth is irrelevant here (I don't know much about
> > it to be honest; I know that Android uses render nodes, so I figured
> > that drmAuth must therefore be the thing that it doesn't use). Sorry
> > for the confusion. Here is a better explanation of why I needed this
> > change.
> >
> > Android has a composer process that opens the primary node and uses
> > DRM_IOCTL_MODE_ATOMIC to switch between frame buffers, and a renderer
> > process (surfaceflinger) that opens the render node, prepares frame
> > buffers and sends them to the composer. One idea for adapting this
> > architecture to devices without render nodes is to have the renderer
> > process open the primary node instead. But this runs into a problem:
> > suppose that the renderer process starts before the composer process.
> > In this case, the kernel makes the renderer the DRM master, so the
> > composer can't change the frame buffer. Render nodes don't have this
> > problem because opening them doesn't affect the master.
> >
> > I considered fixing this by having the composer issue
> > DRM_IOCTL_SET_MASTER, but this requires root privileges. If we require
> > drivers to provide render nodes and control access to the primary node
> > while opening up the render node, we can ensure that only authorized
> > processes can become the master without requiring them to be root.
> >
> I think that the crux, is trying to open a _primary_ node for
> _rendering_ purposes.
> While that may work - as you outline above - it's usually a bad idea.
>
> To step back a bit, in practise we have three SoC combinations:
>  - complete lack of render device - then you default to software rendering [1]
>  - display and render device are one and the same - no change needed,
> things should just work
>  - display and render devices are separate - surfaceflinger should
> open the correct _rendering_ device node.
>
> [1] Mesa's libEGL (don't recall exact name on Android) can open VGEM
> render node, to work with the kms-swrast_dri.so software rasteriser
> module.
>
> While I did not try [1] with Android, it was working fine with CrOS. I
> suggest giving it a try and reporting bugs.

VGEM is not a solution, because it doesn't coordinate caching behavior
with your display node and so you get corruption if you try to to
share dma-bufs between them.  In cros it's used only for some tests as
far as I've heard, and it's been a source of pain.

If we want to go the route of "kms-only nodes also provide a render
node interface for doing swrast on", I think that would be a good
idea, but we should do this at a core DRM level (probably "does this
driver expose dma-buf? then also expose render nodes") rather than per
kms driver.
Peter Collingbourne April 27, 2020, 7:57 p.m. UTC | #7
On Mon, Apr 27, 2020 at 9:48 AM Eric Anholt <eric@anholt.net> wrote:
> If we want to go the route of "kms-only nodes also provide a render
> node interface for doing swrast on", I think that would be a good
> idea, but we should do this at a core DRM level (probably "does this
> driver expose dma-buf? then also expose render nodes") rather than per
> kms driver.

This sounds like a good idea to me. I will send a patch shortly.

Peter
Daniel Vetter April 28, 2020, 3:05 p.m. UTC | #8
On Mon, Apr 27, 2020 at 09:47:57AM -0700, Eric Anholt wrote:
> On Mon, Apr 27, 2020 at 7:45 AM Emil Velikov <emil.l.velikov@gmail.com> wrote:
> >
> > On Fri, 24 Apr 2020 at 19:54, Peter Collingbourne <pcc@google.com> wrote:
> > >
> > > On Fri, Apr 24, 2020 at 4:11 AM Emil Velikov <emil.l.velikov@gmail.com> wrote:
> > > >
> > > > On Thu, 23 Apr 2020 at 23:51, Peter Collingbourne <pcc@google.com> wrote:
> > > > >
> > > > > The render node is required by Android which does not support the legacy
> > > > > drmAuth authentication process.
> > > > >
> > > > > Signed-off-by: Peter Collingbourne <pcc@google.com>
> > > > > ---
> > > > >  drivers/gpu/drm/pl111/pl111_drv.c | 2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > The summary talks about drmAuth, yet exposes a render node. Even
> > > > through there's no rendering engine in the HW, as mentioned by Eric.
> > > >
> > > > AFAICT the only way drmAuth is relevant with pl111 is when you want to
> > > > export/import dma bufs.
> > > > Although that is handled in core and the artificial DRM_AUTH
> > > > restriction has been lifted with commit [1].
> > > >
> > > > -Emil
> > > >
> > > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?h=v5.7-rc2&id=30a958526d2cc6df38347336a602479d048d92e7
> > >
> > > Okay, most likely drmAuth is irrelevant here (I don't know much about
> > > it to be honest; I know that Android uses render nodes, so I figured
> > > that drmAuth must therefore be the thing that it doesn't use). Sorry
> > > for the confusion. Here is a better explanation of why I needed this
> > > change.
> > >
> > > Android has a composer process that opens the primary node and uses
> > > DRM_IOCTL_MODE_ATOMIC to switch between frame buffers, and a renderer
> > > process (surfaceflinger) that opens the render node, prepares frame
> > > buffers and sends them to the composer. One idea for adapting this
> > > architecture to devices without render nodes is to have the renderer
> > > process open the primary node instead. But this runs into a problem:
> > > suppose that the renderer process starts before the composer process.
> > > In this case, the kernel makes the renderer the DRM master, so the
> > > composer can't change the frame buffer. Render nodes don't have this
> > > problem because opening them doesn't affect the master.
> > >
> > > I considered fixing this by having the composer issue
> > > DRM_IOCTL_SET_MASTER, but this requires root privileges. If we require
> > > drivers to provide render nodes and control access to the primary node
> > > while opening up the render node, we can ensure that only authorized
> > > processes can become the master without requiring them to be root.
> > >
> > I think that the crux, is trying to open a _primary_ node for
> > _rendering_ purposes.
> > While that may work - as you outline above - it's usually a bad idea.
> >
> > To step back a bit, in practise we have three SoC combinations:
> >  - complete lack of render device - then you default to software rendering [1]
> >  - display and render device are one and the same - no change needed,
> > things should just work
> >  - display and render devices are separate - surfaceflinger should
> > open the correct _rendering_ device node.
> >
> > [1] Mesa's libEGL (don't recall exact name on Android) can open VGEM
> > render node, to work with the kms-swrast_dri.so software rasteriser
> > module.
> >
> > While I did not try [1] with Android, it was working fine with CrOS. I
> > suggest giving it a try and reporting bugs.
> 
> VGEM is not a solution, because it doesn't coordinate caching behavior
> with your display node and so you get corruption if you try to to
> share dma-bufs between them.  In cros it's used only for some tests as
> far as I've heard, and it's been a source of pain.
> 
> If we want to go the route of "kms-only nodes also provide a render
> node interface for doing swrast on", I think that would be a good
> idea, but we should do this at a core DRM level (probably "does this
> driver expose dma-buf? then also expose render nodes") rather than per
> kms driver.

Hm I thought to fix that vgem was switched over to wc mmap?

Probably still broken in some hilarious ways somewhere.
-Daniel
Emil Velikov April 30, 2020, 10:50 a.m. UTC | #9
On Mon, 27 Apr 2020 at 17:48, Eric Anholt <eric@anholt.net> wrote:
>
> On Mon, Apr 27, 2020 at 7:45 AM Emil Velikov <emil.l.velikov@gmail.com> wrote:
> >
> > On Fri, 24 Apr 2020 at 19:54, Peter Collingbourne <pcc@google.com> wrote:
> > >
> > > On Fri, Apr 24, 2020 at 4:11 AM Emil Velikov <emil.l.velikov@gmail.com> wrote:
> > > >
> > > > On Thu, 23 Apr 2020 at 23:51, Peter Collingbourne <pcc@google.com> wrote:
> > > > >
> > > > > The render node is required by Android which does not support the legacy
> > > > > drmAuth authentication process.
> > > > >
> > > > > Signed-off-by: Peter Collingbourne <pcc@google.com>
> > > > > ---
> > > > >  drivers/gpu/drm/pl111/pl111_drv.c | 2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > The summary talks about drmAuth, yet exposes a render node. Even
> > > > through there's no rendering engine in the HW, as mentioned by Eric.
> > > >
> > > > AFAICT the only way drmAuth is relevant with pl111 is when you want to
> > > > export/import dma bufs.
> > > > Although that is handled in core and the artificial DRM_AUTH
> > > > restriction has been lifted with commit [1].
> > > >
> > > > -Emil
> > > >
> > > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?h=v5.7-rc2&id=30a958526d2cc6df38347336a602479d048d92e7
> > >
> > > Okay, most likely drmAuth is irrelevant here (I don't know much about
> > > it to be honest; I know that Android uses render nodes, so I figured
> > > that drmAuth must therefore be the thing that it doesn't use). Sorry
> > > for the confusion. Here is a better explanation of why I needed this
> > > change.
> > >
> > > Android has a composer process that opens the primary node and uses
> > > DRM_IOCTL_MODE_ATOMIC to switch between frame buffers, and a renderer
> > > process (surfaceflinger) that opens the render node, prepares frame
> > > buffers and sends them to the composer. One idea for adapting this
> > > architecture to devices without render nodes is to have the renderer
> > > process open the primary node instead. But this runs into a problem:
> > > suppose that the renderer process starts before the composer process.
> > > In this case, the kernel makes the renderer the DRM master, so the
> > > composer can't change the frame buffer. Render nodes don't have this
> > > problem because opening them doesn't affect the master.
> > >
> > > I considered fixing this by having the composer issue
> > > DRM_IOCTL_SET_MASTER, but this requires root privileges. If we require
> > > drivers to provide render nodes and control access to the primary node
> > > while opening up the render node, we can ensure that only authorized
> > > processes can become the master without requiring them to be root.
> > >
> > I think that the crux, is trying to open a _primary_ node for
> > _rendering_ purposes.
> > While that may work - as you outline above - it's usually a bad idea.
> >
> > To step back a bit, in practise we have three SoC combinations:
> >  - complete lack of render device - then you default to software rendering [1]
> >  - display and render device are one and the same - no change needed,
> > things should just work
> >  - display and render devices are separate - surfaceflinger should
> > open the correct _rendering_ device node.
> >
> > [1] Mesa's libEGL (don't recall exact name on Android) can open VGEM
> > render node, to work with the kms-swrast_dri.so software rasteriser
> > module.
> >
> > While I did not try [1] with Android, it was working fine with CrOS. I
> > suggest giving it a try and reporting bugs.
>
> VGEM is not a solution, because it doesn't coordinate caching behavior
> with your display node and so you get corruption if you try to to
> share dma-bufs between them.  In cros it's used only for some tests as
> far as I've heard, and it's been a source of pain.
>
My understanding is that dma_buf_{begin,end}_cpu_access should be used
to handle the caching concerns.
Perhaps we're missing some calls, apart from the wc mmap Daniel mentioned?

Fwiw I've played around with CrOS for 30 minutes w/o any corruption.
Mind you it was a boot + casual web browsing.

> If we want to go the route of "kms-only nodes also provide a render
> node interface for doing swrast on", I think that would be a good
> idea, but we should do this at a core DRM level (probably "does this
> driver expose dma-buf? then also expose render nodes") rather than per
> kms driver.

This sounds doable, although I'm concerned about existing
applications, which do not expect this behaviour.
Be that mesa, compositors or otherwise.

-Emil
Emil Velikov April 30, 2020, 11:07 a.m. UTC | #10
On Mon, 27 Apr 2020 at 16:58, Peter Collingbourne <pcc@google.com> wrote:
>
> On Mon, Apr 27, 2020 at 7:45 AM Emil Velikov <emil.l.velikov@gmail.com> wrote:
> >
> > On Fri, 24 Apr 2020 at 19:54, Peter Collingbourne <pcc@google.com> wrote:
> > >
> > > On Fri, Apr 24, 2020 at 4:11 AM Emil Velikov <emil.l.velikov@gmail.com> wrote:
> > > >
> > > > On Thu, 23 Apr 2020 at 23:51, Peter Collingbourne <pcc@google.com> wrote:
> > > > >
> > > > > The render node is required by Android which does not support the legacy
> > > > > drmAuth authentication process.
> > > > >
> > > > > Signed-off-by: Peter Collingbourne <pcc@google.com>
> > > > > ---
> > > > >  drivers/gpu/drm/pl111/pl111_drv.c | 2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > The summary talks about drmAuth, yet exposes a render node. Even
> > > > through there's no rendering engine in the HW, as mentioned by Eric.
> > > >
> > > > AFAICT the only way drmAuth is relevant with pl111 is when you want to
> > > > export/import dma bufs.
> > > > Although that is handled in core and the artificial DRM_AUTH
> > > > restriction has been lifted with commit [1].
> > > >
> > > > -Emil
> > > >
> > > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?h=v5.7-rc2&id=30a958526d2cc6df38347336a602479d048d92e7
> > >
> > > Okay, most likely drmAuth is irrelevant here (I don't know much about
> > > it to be honest; I know that Android uses render nodes, so I figured
> > > that drmAuth must therefore be the thing that it doesn't use). Sorry
> > > for the confusion. Here is a better explanation of why I needed this
> > > change.
> > >
> > > Android has a composer process that opens the primary node and uses
> > > DRM_IOCTL_MODE_ATOMIC to switch between frame buffers, and a renderer
> > > process (surfaceflinger) that opens the render node, prepares frame
> > > buffers and sends them to the composer. One idea for adapting this
> > > architecture to devices without render nodes is to have the renderer
> > > process open the primary node instead. But this runs into a problem:
> > > suppose that the renderer process starts before the composer process.
> > > In this case, the kernel makes the renderer the DRM master, so the
> > > composer can't change the frame buffer. Render nodes don't have this
> > > problem because opening them doesn't affect the master.
> > >
> > > I considered fixing this by having the composer issue
> > > DRM_IOCTL_SET_MASTER, but this requires root privileges. If we require
> > > drivers to provide render nodes and control access to the primary node
> > > while opening up the render node, we can ensure that only authorized
> > > processes can become the master without requiring them to be root.
> > >
> > I think that the crux, is trying to open a _primary_ node for
> > _rendering_ purposes.
> > While that may work - as you outline above - it's usually a bad idea.
> >
> > To step back a bit, in practise we have three SoC combinations:
> >  - complete lack of render device - then you default to software rendering [1]
> >  - display and render device are one and the same - no change needed,
> > things should just work
> >  - display and render devices are separate - surfaceflinger should
> > open the correct _rendering_ device node.
> >
> > [1] Mesa's libEGL (don't recall exact name on Android) can open VGEM
>
> (Android uses SwiftShader for software rendering, not Mesa, FWIW.)
>
I don't know much about SwiftShader.

> > render node, to work with the kms-swrast_dri.so software rasteriser
> > module.
> >
> > While I did not try [1] with Android, it was working fine with CrOS. I
> > suggest giving it a try and reporting bugs.
>
> I'm not sure I understand your suggestion, or at least how it would
> work on Android. The composer process wouldn't be able to use
> DRM_IOCTL_MODE_ATOMIC to display a frame buffer produced by the
> surfaceflinger process using a vgem render node, would it? It's a
> different driver so the memory region used for the frame buffer
> wouldn't necessarily be compatible with the pl111 device. As far as I
> know, the frame buffer would need to be produced using a pl111 render
> node.
>
The pl111 will create a buffer, exports it. Renderer will import and
draw onto it.
Upon completion, the composer will DRM_IOCTL_MODE_ATOMIC the pl111 buffer.

I believe this approach has been used for a few years now.

-Emil
diff mbox series

Patch

diff --git a/drivers/gpu/drm/pl111/pl111_drv.c b/drivers/gpu/drm/pl111/pl111_drv.c
index aa8aa8d9e405..246662129715 100644
--- a/drivers/gpu/drm/pl111/pl111_drv.c
+++ b/drivers/gpu/drm/pl111/pl111_drv.c
@@ -225,7 +225,7 @@  DEFINE_DRM_GEM_CMA_FOPS(drm_fops);
 
 static struct drm_driver pl111_drm_driver = {
 	.driver_features =
-		DRIVER_MODESET | DRIVER_GEM | DRIVER_ATOMIC,
+		DRIVER_MODESET | DRIVER_GEM | DRIVER_ATOMIC | DRIVER_RENDER,
 	.ioctls = NULL,
 	.fops = &drm_fops,
 	.name = "pl111",