[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."
>
> 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.

On second thought, my original explanation may have been
imprecise/unclear. So let me try again - feel free to take either
answer.

In your example above, dri2_init_screen(fd) dups the fd and passes
that in to the pipe loader. Great. Now pipe_loader_drm_probe_fd stores
that fd into pipe_loader_drm_device, whose lifetime is tied to the
creating dri screen as far as I can tell. At some point in time, that
screen will get closed and pipe_loader_drm_release will close that fd.
If every other screen that was created since then has been destroyed
already - fine. But the whole point of this exercise is to allow
screens to be created and destroyed at any time, but to only have at
most one underlying pipe_screen alive at a time. So we end up in a
situation where closing of some screen can end up closing the
resources for ALL the screens.

Hope at least one of my explanations is clear.

Cheers,

  -ilia