Re: [PATCH xf86-video-amdgpu] amdgpu_probe: Do not close server managed drm fds

[Michel Dänzer]

[ Moving to the amd-gfx mailing list, where xf86-video-amdgpu (and -ati)
patches are reviewed ]

On 18/10/16 11:48 PM, Hans de Goede wrote:
> This fixes the xserver only seeing AMD/ATI devices supported by the amdgpu
> driver, as by the time xf86-video-ati gets a chance to probe them, the
> fd has been closed.

That basically makes sense, but I have one question: Why does amdgpu get
probed in the first place in that case? I was under the impression that
this should only happen for GPUs bound to the amdgpu kernel driver, due
to Section "OutputClass" in /usr/share/X11/xorg.conf.d/10-amdgpu.conf .


> diff --git a/src/amdgpu_probe.c b/src/amdgpu_probe.c
> index 213d245..5d17fe5 100644
> --- a/src/amdgpu_probe.c
> +++ b/src/amdgpu_probe.c
> @@ -142,6 +142,15 @@ static int amdgpu_kernel_open_fd(ScrnInfoPtr pScrn, char 
> *busid,
>   return fd;
>  }
>  
> +static void amdgpu_kernel_close_fd(AMDGPUEntPtr pAMDGPUEnt)
> +{
> +#ifdef XF86_PDEV_SERVER_FD
> + if (!(pAMDGPUEnt->platform_dev &&
> +   pAMDGPUEnt->platform_dev->flags & XF86_PDEV_SERVER_FD))
> +#endif
> + drmClose(pAMDGPUEnt->fd);
> +}

AMDGPUFreeRec could also be refactored to call this function.


> @@ -164,7 +173,7 @@ static Bool amdgpu_open_drm_master(ScrnInfoPtr pScrn, 
> AMDGPUEntPtr pAMDGPUEnt,
>   if (err != 0) {
>   xf86DrvMsg(pScrn->scrnIndex, X_ERROR,
>  "[drm] failed to set drm interface version.\n");
> - drmClose(pAMDGPUEnt->fd);
> + amdgpu_kernel_close_fd(pAMDGPUEnt);
>   return FALSE;
>   }
> @@ -252,7 +261,7 @@ static Bool amdgpu_get_scrninfo(int entity_num, struct 
> pci_device *pci_dev)
>   return TRUE;
>  
>  error_amdgpu:
> - drmClose(pAMDGPUEnt->fd);
> + amdgpu_kernel_close_fd(pAMDGPUEnt);
>  error_fd:
>   free(pPriv->ptr);
>  error:

These two hunks aren't really necessary, right? These only get called
from amdgpu_pci_probe, in which case pAMDGPUEnt->platform_dev remains NULL.


> @@ -347,6 +356,7 @@ amdgpu_platform_probe(DriverPtr pDriver,
>  
>   pPriv->ptr = xnfcalloc(sizeof(AMDGPUEntRec), 1);
>   pAMDGPUEnt = pPriv->ptr;
> + pAMDGPUEnt->platform_dev = dev;
>   pAMDGPUEnt->fd = amdgpu_kernel_open_fd(pScrn, busid, dev);
>   if (pAMDGPUEnt->fd < 0)
>   goto error_fd;
> @@ -365,7 +375,6 @@ amdgpu_platform_probe(DriverPtr pDriver,
>   pAMDGPUEnt = pPriv->ptr;
>   pAMDGPUEnt->fd_ref++;
>   }
> - pAMDGPUEnt->platform_dev = dev;
>  
>   xf86SetEntityInstanceForScreen(pScrn, pEnt->index,
>  xf86GetNumEntityInstances(pEnt->

These two hunks aren't really necessary either, are they?


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

[PATCH xf86-video-amdgpu] amdgpu_probe: Do not close server managed drm fds

[Hans de Goede]

This fixes the xserver only seeing AMD/ATI devices supported by the amdgpu
driver, as by the time xf86-video-ati gets a chance to probe them, the
fd has been closed.

This fixes e.g. Xorg not seeing the dGPU on a Lenovo Thinkpad E465 laptop
with a CARRIZO iGPU and a HAINAN dGPU.

Signed-off-by: Hans de Goede 
---
 src/amdgpu_probe.c | 17 +
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/src/amdgpu_probe.c b/src/amdgpu_probe.c
index 213d245..5d17fe5 100644
--- a/src/amdgpu_probe.c
+++ b/src/amdgpu_probe.c
@@ -142,6 +142,15 @@ static int amdgpu_kernel_open_fd(ScrnInfoPtr pScrn, char 
*busid,
return fd;
 }
 
+static void amdgpu_kernel_close_fd(AMDGPUEntPtr pAMDGPUEnt)
+{
+#ifdef XF86_PDEV_SERVER_FD
+   if (!(pAMDGPUEnt->platform_dev &&
+ pAMDGPUEnt->platform_dev->flags & XF86_PDEV_SERVER_FD))
+#endif
+   drmClose(pAMDGPUEnt->fd);
+}
+
 static Bool amdgpu_open_drm_master(ScrnInfoPtr pScrn, AMDGPUEntPtr pAMDGPUEnt,
   char *busid)
 {
@@ -164,7 +173,7 @@ static Bool amdgpu_open_drm_master(ScrnInfoPtr pScrn, 
AMDGPUEntPtr pAMDGPUEnt,
if (err != 0) {
xf86DrvMsg(pScrn->scrnIndex, X_ERROR,
   "[drm] failed to set drm interface version.\n");
-   drmClose(pAMDGPUEnt->fd);
+   amdgpu_kernel_close_fd(pAMDGPUEnt);
return FALSE;
}
 
@@ -252,7 +261,7 @@ static Bool amdgpu_get_scrninfo(int entity_num, struct 
pci_device *pci_dev)
return TRUE;
 
 error_amdgpu:
-   drmClose(pAMDGPUEnt->fd);
+   amdgpu_kernel_close_fd(pAMDGPUEnt);
 error_fd:
free(pPriv->ptr);
 error:
@@ -347,6 +356,7 @@ amdgpu_platform_probe(DriverPtr pDriver,
 
pPriv->ptr = xnfcalloc(sizeof(AMDGPUEntRec), 1);
pAMDGPUEnt = pPriv->ptr;
+   pAMDGPUEnt->platform_dev = dev;
pAMDGPUEnt->fd = amdgpu_kernel_open_fd(pScrn, busid, dev);
if (pAMDGPUEnt->fd < 0)
goto error_fd;
@@ -365,7 +375,6 @@ amdgpu_platform_probe(DriverPtr pDriver,
pAMDGPUEnt = pPriv->ptr;
pAMDGPUEnt->fd_ref++;
}
-   pAMDGPUEnt->platform_dev = dev;
 
xf86SetEntityInstanceForScreen(pScrn, pEnt->index,
   xf86GetNumEntityInstances(pEnt->
@@ -377,7 +386,7 @@ amdgpu_platform_probe(DriverPtr pDriver,
return TRUE;
 
 error_amdgpu:
-   drmClose(pAMDGPUEnt->fd);
+   amdgpu_kernel_close_fd(pAMDGPUEnt);
 error_fd:
free(pPriv->ptr);
 error:
-- 
2.9.3

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel