[Mesa-dev] [PATCH 05/11] nouveau: use common screen ref counting

[Ilia Mirkin]

On Fri, Jun 24, 2016 at 12:20 AM, Rob Herring <robh at kernel.org> wrote:
> On Thu, Jun 23, 2016 at 7:01 PM, Ilia Mirkin <imirkin at alum.mit.edu> wrote:
>> As before, you need to keep the dupfd. There's even a long comment there to
>> explain why.
>
> And I responded why I don't think it is:
>
> "Those 2 commits predate commit 13bccee87d63 which dup's the fd for the
> DRI2 ST before calling pipe loader functions. XA and vl_winsys_drm do
> the same. vl_winsys_dri opens the fd itself. I'm not sure about
> vl_winsys_dri3. d3dadapter9 appears to take ownership of the fd. So my
> conclusion is still that it shouldn't be necessary to dup the fd."

Hmmm... I must have spaced on that reply, or it never reached me.
Sorry, I thought you had just ignored my feedback =/

>
> To put it another way the call stack for DRI2 looks like this where fd
> is the original and dup_fd is the dup() one:
>
> driCreateNewScreen2(fd)
>   dri2_init_screen(fd)
>     dup(fd)
>     pipe_loader_drm_probe_fd(dup_fd)
>     pipe_loader_create_screen()
>       pipe_nouveau_create_screen(dup_fd)
>
> Why does the fd need to be dup'ed twice? I looks to me like the same
> problem was fixed twice, once in some drivers and once in common code
> at around the same time. I could be missing something, but I'd like to
> understand what.

The simplest way to achieve the error is with zaphod heads. The X
server code goes something like

create:
for each head {
  fd = dupfd()
  driCreateNewScreen2(fd)
}

destroy:
for each head {
  driDestroyScreen(fd) [function name approximate]
  close(fd);
}

What this means is that we get a sequence like Create fd1, Create fd2,
Destroy fd1, Destroy fd2. The nouveau_device we create wraps fd1, so
when you close fd1, the device is gone. This causes issues for
Destroy2.

The more complex way is to do the exact same thing but with, e.g., GL
+ vdpau. You can create a GL context, then create a vdpau context, and
then destroy the GL context, which will leave the vdpau context
similarly messed up.

The fundamental issue though, is described in the comment above the
dup - the lifetime of the fd has to be that of the screen. However the
fd can get closed whenever by the surrounding st whenever it feels
like it's done. Which will implicitly cause the nouveau_device to die,
which in turn makes everything that comes after fail.

Hope this clarifies things.

  -ilia